[PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Krzysztof Hałasa]

Gateworks Ventana boards seem to need "RGMII-ID" (internal delay)
PHY mode, instead of simple "RGMII", for their Marvell 88E1510
transceiver. Otherwise, the Ethernet MAC doesn't work with Marvell PHY
driver (TX doesn't seem to work correctly).

Tested on GW5400 rev. C.

This bug affects ARM Fedora 23.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

diff --git a/arch/arm/boot/dts/imx6q-gw5400-a.dts b/arch/arm/boot/dts/imx6q-gw5400-a.dts
index 822ffb2..6c168dc 100644
--- a/arch/arm/boot/dts/imx6q-gw5400-a.dts
+++ b/arch/arm/boot/dts/imx6q-gw5400-a.dts
@@ -154,7 +154,7 @@
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
index f2867c4..90496aa 100644
--- a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
@@ -94,7 +94,7 @@
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
index 4493f6e..0a6730b 100644
--- a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
@@ -154,7 +154,7 @@
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
index cfad214..2c549ad 100644
--- a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
@@ -174,7 +174,7 @@
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
index 535b536..b4ea087 100644
--- a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
@@ -164,7 +164,7 @@
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };

-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Fabio Estevam]

On Mon, Dec 7, 2015 at 10:56 AM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Gateworks Ventana boards seem to need "RGMII-ID" (internal delay)
> PHY mode, instead of simple "RGMII", for their Marvell 88E1510
> transceiver. Otherwise, the Ethernet MAC doesn't work with Marvell PHY
> driver (TX doesn't seem to work correctly).
>
> Tested on GW5400 rev. C.
>
> This bug affects ARM Fedora 23.
>
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

It seems you missed to Cc the i.MX maintainer, Shawn Guo.

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Tim Harvey]

On Mon, Dec 7, 2015 at 4:56 AM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Gateworks Ventana boards seem to need "RGMII-ID" (internal delay)
> PHY mode, instead of simple "RGMII", for their Marvell 88E1510
> transceiver. Otherwise, the Ethernet MAC doesn't work with Marvell PHY
> driver (TX doesn't seem to work correctly).
>
> Tested on GW5400 rev. C.
>
> This bug affects ARM Fedora 23.
>
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
>
> diff --git a/arch/arm/boot/dts/imx6q-gw5400-a.dts b/arch/arm/boot/dts/imx6q-gw5400-a.dts
> index 822ffb2..6c168dc 100644
> --- a/arch/arm/boot/dts/imx6q-gw5400-a.dts
> +++ b/arch/arm/boot/dts/imx6q-gw5400-a.dts
> @@ -154,7 +154,7 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> -       phy-mode = "rgmii";
> +       phy-mode = "rgmii-id";
>         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_HIGH>;
>         status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
> index f2867c4..90496aa 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
> @@ -94,7 +94,7 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> -       phy-mode = "rgmii";
> +       phy-mode = "rgmii-id";
>         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
>         status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
> index 4493f6e..0a6730b 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
> @@ -154,7 +154,7 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> -       phy-mode = "rgmii";
> +       phy-mode = "rgmii-id";
>         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
>         status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
> index cfad214..2c549ad 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
> @@ -174,7 +174,7 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> -       phy-mode = "rgmii";
> +       phy-mode = "rgmii-id";
>         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
>         status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
> index 535b536..b4ea087 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
> @@ -164,7 +164,7 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> -       phy-mode = "rgmii";
> +       phy-mode = "rgmii-id";
>         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
>         status = "okay";
>  };
>
> --
> Krzysztof Halasa
>
> Industrial Research Institute for Automation and Measurements PIAP
> Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Krzysztof,

It sounds like your saying this controls whether the phy is in charge
of delay vs the MAC. I have never needed to set this and haven't found
where its actually used (in at least 4.3). Is this caused by something
new in the kernel I haven't seen yet or is it possible you have board
that has an Ethernet issue?

Regards,

Tim

Tim Harvey - Principal Software Engineer
Gateworks Corporation - http://www.gateworks.com/
3026 S. Higuera St. San Luis Obispo CA 93401
805-781-2000

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Krzysztof Hałasa]

Tim Harvey <tharvey@gateworks.com> writes:

> It sounds like your saying this controls whether the phy is in charge
> of delay vs the MAC. I have never needed to set this and haven't found
> where its actually used (in at least 4.3). Is this caused by something
> new in the kernel I haven't seen yet or is it possible you have board
> that has an Ethernet issue?

I think you aren't using the Marvell PHY driver :-)
The existing DTS files work fine with the "default" PHY driver.
It's only the Marvell driver (drivers/net/phy/marvell.c, aka
CONFIG_MARVELL_PHY) which have the issue.

The problem is caused by the following code in m88e1121_config_aneg():

        if (phy_interface_is_rgmii(phydev)) {

                mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG) &
                        MII_88E1121_PHY_MSCR_DELAY_MASK;

                if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
                        mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |
                                 MII_88E1121_PHY_MSCR_TX_DELAY);
                else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
                        mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
                else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
                        mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;

                err = phy_write(phydev, MII_88E1121_PHY_MSCR_REG, mscr);
                if (err < 0)
                        return err;
        }

mscr initially holds the correct value (one with RX_DELAY and TX_DELAY
bits set). Since the _*ID are not specified, these bits are reset and
(now incorrect) value is written back to the register.
OTOH the generic PHY driver knows nothing about the MSCR and thus
doesn't "corrupt" it.

The whole -ID thing is about signal trace lengths (on PCB), where the
non-ID setup requires special, longer traces for certain signals to
compensate for propagation times etc. The -ID version does it internally
(when enabled) and the PCB layout can be a bit simpler.

To be honest I only checked it on a single GW5400 rev. C, but I'd expect
it to be exactly the same across all GW5[234]*. Can test on GW5[23] if
there is any doubt (in few days I guess).


This issue doesn't show itself on Yocto and OpenWrt, because they
apparently don't enable marvell PHY driver. Fedora 23 does and it
doesn't work (this probably means it could be worth it to push it to
stable).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Tim Harvey]

On Tue, Dec 8, 2015 at 6:13 AM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Tim Harvey <tharvey@gateworks.com> writes:
>
>> It sounds like your saying this controls whether the phy is in charge
>> of delay vs the MAC. I have never needed to set this and haven't found
>> where its actually used (in at least 4.3). Is this caused by something
>> new in the kernel I haven't seen yet or is it possible you have board
>> that has an Ethernet issue?
>
> I think you aren't using the Marvell PHY driver :-)
> The existing DTS files work fine with the "default" PHY driver.
> It's only the Marvell driver (drivers/net/phy/marvell.c, aka
> CONFIG_MARVELL_PHY) which have the issue.
>

Kryzsztof,

True - I have not used CONFIG_MARVELL_PHY and didn't know of its existance.

> The problem is caused by the following code in m88e1121_config_aneg():
>
>         if (phy_interface_is_rgmii(phydev)) {
>
>                 mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG) &
>                         MII_88E1121_PHY_MSCR_DELAY_MASK;
>
>                 if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>                         mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |
>                                  MII_88E1121_PHY_MSCR_TX_DELAY);
>                 else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>                         mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
>                 else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>                         mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
>
>                 err = phy_write(phydev, MII_88E1121_PHY_MSCR_REG, mscr);
>                 if (err < 0)
>                         return err;
>         }
>
> mscr initially holds the correct value (one with RX_DELAY and TX_DELAY
> bits set). Since the _*ID are not specified, these bits are reset and
> (now incorrect) value is written back to the register.
> OTOH the generic PHY driver knows nothing about the MSCR and thus
> doesn't "corrupt" it.
>
> The whole -ID thing is about signal trace lengths (on PCB), where the
> non-ID setup requires special, longer traces for certain signals to
> compensate for propagation times etc. The -ID version does it internally
> (when enabled) and the PCB layout can be a bit simpler.

right - I understand what the delay lines are, it just struck me as
strange that I've never needed this.

>
> To be honest I only checked it on a single GW5400 rev. C, but I'd expect
> it to be exactly the same across all GW5[234]*. Can test on GW5[23] if
> there is any doubt (in few days I guess).
>

no doubt now. I re-created your config and see the problem as well.

The marvell phy driver (drivers/net/phy/marvell.c) has a very lacking
Kconfig stating its a PHY driver for the 88E1011S when in fact it has
id's for 12 ID's including the 88E1510 we use on the GW51xx, GW52xx,
GW53xx, GW54xx.

This may be a stupid question but what does the Marvell phy driver
offer that the generic phy driver does not?

Thanks for finding this and submitting a patch!

Acked-by: Tim Harvey <tharvey@gateworks.com>

Tim

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Andrew Lunn]

> This may be a stupid question but what does the Marvell phy driver
> offer that the generic phy driver does not?

It reliably configures the RGMII delays :-)

No need to depend on your bootloader doing it.

It allows interrupts to be used instead of polling the PHY all the
time, assuming you have the interrupt wired up.  There are
peculiarities for enabling autoneg which is knows about. It might put
the LEDs into a well known state, but that depends on the model. It
might enable auto-crossover on some PHYs. It has some errata
workarounds, support for WOL. You can put a list of register writes in
DT which will be performed when auto neg is enabled. This could be
used for example to configure the LEDs.

Not yet posted, but i have a patch to add HWMON support to read the
temperature sensor in some of the PHYs.

	    Andrew

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[David Miller]

From: khalasa@piap.pl (Krzysztof Hałasa)
Date: Mon, 07 Dec 2015 13:56:23 +0100

> Gateworks Ventana boards seem to need "RGMII-ID" (internal delay)
> PHY mode, instead of simple "RGMII", for their Marvell 88E1510
> transceiver. Otherwise, the Ethernet MAC doesn't work with Marvell PHY
> driver (TX doesn't seem to work correctly).
> 
> Tested on GW5400 rev. C.
> 
> This bug affects ARM Fedora 23.
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

Since this only touched DT files in the ARM platform code, maybe
the ARM tree is the best path for this patch rather than mine?

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Krzysztof Hałasa]

David Miller <davem@davemloft.net> writes:

> Since this only touched DT files in the ARM platform code, maybe
> the ARM tree is the best path for this patch rather than mine?

I think so.

Tim, would you like to handle this patch yourself, or should I send it
to the ARM contacts?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

[Tim Harvey]

On Tue, Dec 8, 2015 at 9:50 PM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> David Miller <davem@davemloft.net> writes:
>
>> Since this only touched DT files in the ARM platform code, maybe
>> the ARM tree is the best path for this patch rather than mine?
>
> I think so.
>
> Tim, would you like to handle this patch yourself, or should I send it
> to the ARM contacts?

Krzysztof,

Go ahead and send it. You can add my:

Acked-by: Tim Harvey <tharvey@gateworks.com>

I know Shawn would be happy to pick this up as well as he has an
imx-fixes tree:
http://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/log/?h=imx/soc

Tim