RE: [PATCH] net: fec: Fix dual ethernet issue in VFxx

[Stefan Agner]

On 2015-01-09 04:00, fugang.duan@xxxxxxxxxxxxx wrote:
> From: Stefan Agner <stefan@xxxxxxxx> Sent: Friday, January 09, 2015 2:59 AM
>> To: Duan Fugang-B38611
>> Cc: Bhuvanchandra DV; linux-kernel@xxxxxxxxxxxxxxx; Zhou Luwei-B45643;
>> LW@xxxxxxxxxxxxxxxxxxx; Li Frank-B20596; davem@xxxxxxxxxxxxx; u.kleine-
>> koenig@xxxxxxxxxxxxxx; shawn.guo@xxxxxxxxxx
>> Subject: RE: [PATCH] net: fec: Fix dual ethernet issue in VFxx
>>
>> On 2015-01-08 04:33, fugang.duan@xxxxxxxxxxxxx wrote:
>> > From: Stefan Agner <stefan@xxxxxxxx> Sent: Wednesday, January 07, 2015
>> > 6:30 PM
>> >> To: Duan Fugang-B38611
>> >> Cc: Bhuvanchandra DV; linux-kernel@xxxxxxxxxxxxxxx; Zhou
>> >> Luwei-B45643; LW@xxxxxxxxxxxxxxxxxxx; Li Frank-B20596;
>> >> davem@xxxxxxxxxxxxx
>> >> Subject: RE: [PATCH] net: fec: Fix dual ethernet issue in VFxx
>> >>
>> >> On 2015-01-07 03:11, fugang.duan@xxxxxxxxxxxxx wrote:
>> >> > From: Stefan Agner <stefan@xxxxxxxx> Sent: Tuesday, January 06,
>> >> > 2015
>> >> > 10:52 PM
>> >> >> To: Bhuvanchandra DV
>> >> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; Zhou Luwei-B45643; LW@karo-
>> >> >> electronics.de; Li Frank-B20596; Duan Fugang-B38611;
>> >> >> davem@xxxxxxxxxxxxx
>> >> >> Subject: Re: [PATCH] net: fec: Fix dual ethernet issue in VFxx
>> >> >>
>> >> >> Fwiw, this patch really touches many devices and disables the
>> >> >> quirk for some devices, but this is done by intent.
>> >> >>
>> >> >> The quirk FEC_QUIRK_ENET_MAC was active for i.MX28, i.MX6Q, Vybrid
>> >> >> (mvf600-fec) and i.MX6SX. However, the new quirk is only enabled
>> >> >> for i.MX28. i.MX6Q doesn't need the quirk since there is one FEC
>> >> >> instance only anyway. Vybrid and i.MX6SX have a MDIO bus for each
>> instance.
>> >> >>
>> >> >> Acked-by: Stefan Agner <stefan@xxxxxxxx>
>> >> >
>> >> > We cannot do it by adding a quirk.
>> >> > For Vybrid and i.MX6SX and later i.MX7 serial,  there have their
>> >> > own MDIO bus for each MAC.
>> >> > But, for board design, to save two pin (MDIO, MDC), MAC0 and MAC1
>> >> > share the MDIO bus. For example, i.MX6SX sdb/sabreai/arm2 boards
>> >> > did like this.
>> >>
>> >> Hm, so those board use a circumstance which was SoC specific back at
>> >> i.MX28 time. IMHO, "Out of luck" the shared MDIO bus is the first one
>> >> even for those boards, hence this SoC specific work around still works.
>> >> So I still think for the i.MX28 case, the quirk would be a viable
>> >> solution, but not for those boards, I agree.
>> >>
>> >> > So we must add one dts property to distinguish it, not a quirk.
>> >>
>> >> Just adding a property to the FEC instance who's MDIO bus is in use
>> >> seems somewhat archaic. There is a MDIO bus description for other
>> >> SoC, do you have in mind how this should look like for fec?
>> >>
>> >> --
>> >> Stefan
>>
>> (added Uwe to CC since he added the device tree support for MDIO bus)
>>
>> > In general,  MAC2 use MAC1 MDIO bus. Because MAC1 is registered
>> > firstly, then register MAC2.
>> > We can invent one property like "shared-mii-bus" for MAC1 to tell
>> > driver there have other MACs use itself mii bus.
>> > Of course, the property needs to add for MAC2 device node to tell
>> > driver it will share the MAC1 mii bus.
>>
>> There is actually already lot of device tree support for MDIO bus
>> description in place (see of_mdiobus_register call in the fec_main
>> driver), just the device tree's do not make a lot use of it currently.
>>
>> The vf610-twr.dts has both FEC enabled as well as the quirk is enabled in
>> the driver for Vybrid. I tested the Tower System Module TWR-SER2 with
>> Vybrid. This module has two ethernet PHY's, but _both_ are connected to
>> the first RMII's MDIO bus (FEC0). Thanks to the quirk, both ports work
>> fine.
>>
>> As a side note: Don't be fooled that there are both MDIO buses available
>> on the elevator: The module makes use of the MDIO of the first RMII
>> signals only, see TWR-SER2 schematic.
>>
>> I then disabled the quirk and altered the device tree (patch courtesy of
>> Bhuvan):
>>
>> diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-
>> twr.dts index a0f7621..876c80a 100644
>> --- a/arch/arm/boot/dts/vf610-twr.dts
>> +++ b/arch/arm/boot/dts/vf610-twr.dts
>> @@ -129,13 +129,28 @@
>>
>>  &fec0 {
>>         phy-mode = "rmii";
>> +       phy-handle = <&ethphy0>;
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_fec0>;
>>         status = "okay";
>> +
>> +       fec0mdio: mdio {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               ethphy0: ethernet-phy@0 {
>> +                       reg = <0>;
>> +               };
>> +
>> +               ethphy1: ethernet-phy@1 {
>> +                       reg = <1>;
>> +               };
>> +       };
>>  };
>>
>>  &fec1 {
>>         phy-mode = "rmii";
>> +       phy-handle = <&ethphy1>;
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_fec1>;
>>         status = "okay";
>>
>> This worked also fine! Hence we actually already have all device tree
>> support needed. For the case we try to solve (using the MDIO bus of each
>> RMII), a device tree like this should be sufficient:
>>
>> &fec0 {
>> 	phy-mode = "rmii";
>> 	phy-handle = <&ethphy0>;
>> ...
>>
>> 	fec0mdio: mdio {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		ethphy0: ethernet-phy@0 {
>> 			reg = <0>;
>> 		};
>> 	};
>> };
>>
>> &fec1 {
>> ...
>> 	phy-handle = <&ethphy1>;
>> ...
>>
>> 	fec1mdio: mdio {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		ethphy1: ethernet-phy@1 {
>> 			reg = <1>;
>> 		};
>> 	};
>> };
>>
>> Unfortunately, the quirk FEC_QUIRK_ENET_MAC leads to fec_enet_mii_init
>> return early for the _second_ FEC. Even if the PHY's would be properly
>> described in dt, parsing is prevented by that quirk.
>>
>> Hence my suggestion is this:
>> - Implement the FEC_QUIRK_SINGLE_MDIO and add it to as in this patch,
>> which makes the quirk always work for i.MX28 even without dt changes...
>> (backward compatible)
>> - Fix the Tower device tree using the patch above to make the Tower work
>> again
>> - i.MX6SX is similar to Vybrid in that regard, adding a patch similar to
>> the one above should make the SX boards work too
>> - i.MX6S/D/Q is not affected since only one FEC instance is available
>> - i.MX7 is not yet merged, so a proper device tree description can be
>> used upfront
>>
>> This would break old device trees with newer kernels for i.MX6SX and
>> Vybrid.. The first is quite new, and the latter is not very widespread,
>> IMHO this would be acceptable. Also, the device tree just did not
>> describe the hardware completely. "Out of a luck" (just because the MDIO
>> quirk for the i.MX28 SoC is active for all SoC's)..., it currently
>> works...
>>
>> Alternatively, we can add the FEC_QUIRK_SINGLE_MDIO quirk to driver_data
>> of imx6sx-fec and mvf600-fec. Then, check early if a MDIO bus description
>> is available. This would keep the current device tree's working while
>> having support to describe other boards in device tree's in the future
>> (something along these lines):
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 5ebdf8d..527eb3e 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1952,7 +1952,8 @@ static int fec_enet_mii_init(struct platform_device
>> *pdev)
>>          * mdio interface in board design, and need to be configured by
>>          * fec0 mii_bus.
>>          */
>> -       if ((fep->quirks & FEC_QUIRK_ENET_MAC) && fep->dev_id > 0) {
>> +       node = of_get_child_by_name(pdev->dev.of_node, "mdio");
>> +       if (!node && fep->quirks & FEC_QUIRK_SINGLE_MDIO && fep->dev_id
>> > 0) {
>>                 /* fec1 uses fec0 mii_bus */
>>                 if (mii_cnt && fec0_mii_bus) {
>>                         fep->mii_bus = fec0_mii_bus; @@ -2001,7 +2002,6
>> @@ static int fec_enet_mii_init(struct platform_device *pdev)
>>         for (i = 0; i < PHY_MAX_ADDR; i++)
>>                 fep->mii_bus->irq[i] = PHY_POLL;
>>
>> -       node = of_get_child_by_name(pdev->dev.of_node, "mdio");
>>         if (node) {
>>                 err = of_mdiobus_register(fep->mii_bus, node);
>>                 of_node_put(node);
>>
>>
>> However, IMHO this would add a quirk to SoC's which actually are not
>> affected... Hence I would prefer to not add that quirk on those SoC's and
>> just break the (uncomplete) device tree's....
>>
> 
> I suggest using one patch to fix all SOCs and boards issue. Don't
> leave the issue for other boards.

I agree. I never suggested to leave the issue for some boards.

> 
> i.MX serial with two MACs always have two MDIO bus, whether sharing
> MDIO bus only depend on boards design.
> For example, i.MX6SX silicon, freescale reference boards use sharing
> MDIO bus, maybe customer's boards use independent MDIO bus (two MII
> bus).


Yes, I have understood that.

> 
> There have four combination for i.MX28, i.MX6SX, Vybird, and the i.MX7....
> 1. MAC1 share MAC0 mii bus,  and no "phy_node" property and no "mdio"
> sub-node in dts. [legacy]
> 2. MAC1 share MAC0 mii bus,  and have "phy_node" property in both MAC
> node,  and only MAC0 node has "mdio" sub-node in dts.
> 3. MAC0 and MAC1 use independent MII bus,  and no "phy_node" property
> and no "mdio" sub-node in dts. [legacy]
> 4. MAC0 and MAC1 use independent MII bus,   and have "phy_node"
> property in both MAC node,  and only MAC0 node has "mdio" sub-node in
> dts.

Currently, MAC1 is broken in case 3 on Vybrid (as well as i.MX6XS and
i.MX7, because the FEC_QUIRK_ENET_MAC is enabled for all of them).

> For all these cases, your above patches cannot handle these.

Yes, in it's current state, I agree. A new revision is required.

> So I still insist to add another property for this to avoid the
> existing dts files change.

But I don't think adding another device tree property is helping here.
There is already a device tree standard how to describe the MDIO buses,
why not just make use of them?

I will create a v2 patchset, so we have a new base to discuss this
further.

--
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/