2021-03-09 11:29:35

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v2 0/7] remove different PHY fixups

changes v2:
- rebase against latest kernel
- fix networking on RIoTBoard

This patch series tries to remove most of the imx6 and imx7 board
specific PHY configuration via fixup, as this breaks the PHYs when
connected to switch chips or USB Ethernet MACs.

Each patch has the possibility to break boards, but contains a
recommendation to fix the problem in a more portable and future-proof
way.

regards,
Oleksij

Oleksij Rempel (7):
ARM: imx6q: remove PHY fixup for KSZ9031
ARM: imx6q: remove TX clock delay of ar8031_phy_fixup()
ARM: imx6q: remove hand crafted PHY power up in ar8035_phy_fixup()
ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035
PHYs
ARM: imx6q: remove Atheros AR8035 SmartEEE fixup
ARM: imx6sx: remove Atheros AR8031 PHY fixup
ARM: imx7d: remove Atheros AR8031 PHY fixup

arch/arm/boot/dts/imx6dl-riotboard.dts | 2 +
arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
arch/arm/mach-imx/mach-imx6q.c | 85 -------------------------
arch/arm/mach-imx/mach-imx6sx.c | 26 --------
arch/arm/mach-imx/mach-imx7d.c | 22 -------
5 files changed, 3 insertions(+), 134 deletions(-)

--
2.29.2


2021-03-09 11:30:12

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v2 1/7] ARM: imx6q: remove PHY fixup for KSZ9031

Starting with:

bcf3440c6dd7 ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")

the micrel phy driver started respecting phy-mode for the KSZ9031 PHY.
At least with kernel v5.8 configuration provided by this fixup was
overwritten by the micrel driver.

This fixup was providing following configuration:

RX path: 2.58ns delay
rx -0.42 (left shift) + rx_clk +0.96ns (right shift) =
1,38 + 1,2 internal RX delay = 2.58ns
TX path: 0.96ns delay
tx (no delay) + tx_clk 0.96ns (right shift) = 0.96ns

This configuration is outside of the recommended RGMII clock skew delays
and about in the middle of: rgmii-idrx and rgmii-id

Since most embedded systems do not have enough place to introduce
significant clock skew, rgmii-id is the way to go.

In case this patch breaks network functionality on your system, build
kernel with enabled MICREL_PHY. If it is still not working then try
following device tree options:
1. Set (or change) phy-mode in DT to:
phy-mode = "rgmii-id";
This actives internal delay for both RX and TX.
1. Set (or change) phy-mode in DT to:
phy-mode = "rgmii-idrx";
This actives internal delay for RX only.
3. Use following DT properties:
phy-mode = "rgmii";
txen-skew-psec = <0>;
rxdv-skew-psec = <0>;
rxd0-skew-psec = <0>;
rxd1-skew-psec = <0>;
rxd2-skew-psec = <0>;
rxd3-skew-psec = <0>;
rxc-skew-psec = <1860>;
txc-skew-psec = <1860>;
This activates the internal delays for RX and TX, with the value as
the fixup that is removed in this patch.

Signed-off-by: Oleksij Rempel <[email protected]>
Acked-by: Philippe Schenker <[email protected]>
---
arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
arch/arm/mach-imx/mach-imx6q.c | 23 -----------------------
2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts b/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
index fa2307d8ce86..c713ac03b3b9 100644
--- a/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
+++ b/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
@@ -112,7 +112,7 @@ flash: m25p80@0 {
&fec {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_enet>;
- phy-mode = "rgmii";
+ phy-mode = "rgmii-id";
phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
phy-supply = <&vgen2_1v2_eth>;
status = "okay";
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 703998ebb52e..78205f90da27 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -40,27 +40,6 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
return 0;
}

-static void mmd_write_reg(struct phy_device *dev, int device, int reg, int val)
-{
- phy_write(dev, 0x0d, device);
- phy_write(dev, 0x0e, reg);
- phy_write(dev, 0x0d, (1 << 14) | device);
- phy_write(dev, 0x0e, val);
-}
-
-static int ksz9031rn_phy_fixup(struct phy_device *dev)
-{
- /*
- * min rx data delay, max rx/tx clock delay,
- * min rx/tx control delay
- */
- mmd_write_reg(dev, 2, 4, 0);
- mmd_write_reg(dev, 2, 5, 0);
- mmd_write_reg(dev, 2, 8, 0x003ff);
-
- return 0;
-}
-
/*
* fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High
* as they are used for slots1-7 PERST#
@@ -152,8 +131,6 @@ static void __init imx6q_enet_phy_init(void)
if (IS_BUILTIN(CONFIG_PHYLIB)) {
phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
ksz9021rn_phy_fixup);
- phy_register_fixup_for_uid(PHY_ID_KSZ9031, MICREL_PHY_ID_MASK,
- ksz9031rn_phy_fixup);
phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef,
ar8031_phy_fixup);
phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef,
--
2.29.2

2021-03-09 11:30:41

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH v2 4/7] ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035 PHYs

This configuration should be set over device tree.

If this patch breaks network functionality on your system, enable the
AT803X_PHY driver and set following device tree property in the PHY
node:

qca,clk-out-frequency = <125000000>;

Signed-off-by: Oleksij Rempel <[email protected]>
---
arch/arm/boot/dts/imx6dl-riotboard.dts | 2 ++
arch/arm/mach-imx/mach-imx6q.c | 30 --------------------------
2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/arch/arm/boot/dts/imx6dl-riotboard.dts b/arch/arm/boot/dts/imx6dl-riotboard.dts
index 065d3ab0f50a..e7d9bfbfd0e4 100644
--- a/arch/arm/boot/dts/imx6dl-riotboard.dts
+++ b/arch/arm/boot/dts/imx6dl-riotboard.dts
@@ -106,6 +106,8 @@ rgmii_phy: ethernet-phy@4 {
reset-gpios = <&gpio3 31 GPIO_ACTIVE_LOW>;
reset-assert-us = <10000>;
reset-deassert-us = <1000>;
+ qca,smarteee-tw-us-1g = <24>;
+ qca,clk-out-frequency = <125000000>;
};
};
};
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 4c840e116003..d12b571a61ac 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -68,25 +68,6 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_PLX, 0x8609, ventana_pciesw_early_fixup);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_PLX, 0x8606, ventana_pciesw_early_fixup);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_PLX, 0x8604, ventana_pciesw_early_fixup);

-static int ar8031_phy_fixup(struct phy_device *dev)
-{
- u16 val;
-
- /* To enable AR8031 output a 125MHz clk from CLK_25M */
- phy_write(dev, 0xd, 0x7);
- phy_write(dev, 0xe, 0x8016);
- phy_write(dev, 0xd, 0x4007);
-
- val = phy_read(dev, 0xe);
- val &= 0xffe3;
- val |= 0x18;
- phy_write(dev, 0xe, val);
-
- return 0;
-}
-
-#define PHY_ID_AR8031 0x004dd074
-
static int ar8035_phy_fixup(struct phy_device *dev)
{
u16 val;
@@ -101,15 +82,6 @@ static int ar8035_phy_fixup(struct phy_device *dev)
val = phy_read(dev, 0xe);
phy_write(dev, 0xe, val & ~(1 << 8));

- /*
- * Enable 125MHz clock from CLK_25M on the AR8031. This
- * is fed in to the IMX6 on the ENET_REF_CLK (V22) pad.
- * Also, introduce a tx clock delay.
- *
- * This is the same as is the AR8031 fixup.
- */
- ar8031_phy_fixup(dev);
-
return 0;
}

@@ -120,8 +92,6 @@ static void __init imx6q_enet_phy_init(void)
if (IS_BUILTIN(CONFIG_PHYLIB)) {
phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
ksz9021rn_phy_fixup);
- phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef,
- ar8031_phy_fixup);
phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef,
ar8035_phy_fixup);
}
--
2.29.2

2021-03-25 01:08:31

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hi Shawn,

ping, do this patches need some ACK from some one?

Regards,
Oleksij

On Tue, Mar 09, 2021 at 12:26:08PM +0100, Oleksij Rempel wrote:
> changes v2:
> - rebase against latest kernel
> - fix networking on RIoTBoard
>
> This patch series tries to remove most of the imx6 and imx7 board
> specific PHY configuration via fixup, as this breaks the PHYs when
> connected to switch chips or USB Ethernet MACs.
>
> Each patch has the possibility to break boards, but contains a
> recommendation to fix the problem in a more portable and future-proof
> way.
>
> regards,
> Oleksij
>
> Oleksij Rempel (7):
> ARM: imx6q: remove PHY fixup for KSZ9031
> ARM: imx6q: remove TX clock delay of ar8031_phy_fixup()
> ARM: imx6q: remove hand crafted PHY power up in ar8035_phy_fixup()
> ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035
> PHYs
> ARM: imx6q: remove Atheros AR8035 SmartEEE fixup
> ARM: imx6sx: remove Atheros AR8031 PHY fixup
> ARM: imx7d: remove Atheros AR8031 PHY fixup
>
> arch/arm/boot/dts/imx6dl-riotboard.dts | 2 +
> arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
> arch/arm/mach-imx/mach-imx6q.c | 85 -------------------------
> arch/arm/mach-imx/mach-imx6sx.c | 26 --------
> arch/arm/mach-imx/mach-imx7d.c | 22 -------
> 5 files changed, 3 insertions(+), 134 deletions(-)
>
> --
> 2.29.2
>
>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2021-03-29 00:48:43

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

On Wed, Mar 24, 2021 at 06:54:24AM +0100, Oleksij Rempel wrote:
> Hi Shawn,
>
> ping, do this patches need some ACK from some one?

As this will break existing DTBs, I need more ACKs from people to see
the consensus that this is the right thing to do.

Shawn

>
> Regards,
> Oleksij
>
> On Tue, Mar 09, 2021 at 12:26:08PM +0100, Oleksij Rempel wrote:
> > changes v2:
> > - rebase against latest kernel
> > - fix networking on RIoTBoard
> >
> > This patch series tries to remove most of the imx6 and imx7 board
> > specific PHY configuration via fixup, as this breaks the PHYs when
> > connected to switch chips or USB Ethernet MACs.
> >
> > Each patch has the possibility to break boards, but contains a
> > recommendation to fix the problem in a more portable and future-proof
> > way.
> >
> > regards,
> > Oleksij
> >
> > Oleksij Rempel (7):
> > ARM: imx6q: remove PHY fixup for KSZ9031
> > ARM: imx6q: remove TX clock delay of ar8031_phy_fixup()
> > ARM: imx6q: remove hand crafted PHY power up in ar8035_phy_fixup()
> > ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035
> > PHYs
> > ARM: imx6q: remove Atheros AR8035 SmartEEE fixup
> > ARM: imx6sx: remove Atheros AR8031 PHY fixup
> > ARM: imx7d: remove Atheros AR8031 PHY fixup
> >
> > arch/arm/boot/dts/imx6dl-riotboard.dts | 2 +
> > arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
> > arch/arm/mach-imx/mach-imx6q.c | 85 -------------------------
> > arch/arm/mach-imx/mach-imx6sx.c | 26 --------
> > arch/arm/mach-imx/mach-imx7d.c | 22 -------
> > 5 files changed, 3 insertions(+), 134 deletions(-)
> >
> > --
> > 2.29.2
> >
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2021-03-29 08:24:24

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

On Mon, Mar 29, 2021 at 08:44:59AM +0800, Shawn Guo wrote:
> On Wed, Mar 24, 2021 at 06:54:24AM +0100, Oleksij Rempel wrote:
> > Hi Shawn,
> >
> > ping, do this patches need some ACK from some one?
>
> As this will break existing DTBs, I need more ACKs from people to see
> the consensus that this is the right thing to do.

Do you need ACKs from some concrete people?

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2021-03-30 14:03:10

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hi Oleksij,

On Tue, Mar 9, 2021 at 8:26 AM Oleksij Rempel <[email protected]> wrote:
>
> changes v2:
> - rebase against latest kernel
> - fix networking on RIoTBoard
>
> This patch series tries to remove most of the imx6 and imx7 board
> specific PHY configuration via fixup, as this breaks the PHYs when
> connected to switch chips or USB Ethernet MACs.
>
> Each patch has the possibility to break boards, but contains a
> recommendation to fix the problem in a more portable and future-proof
> way.

I think this series moves us in the right direction, even with the
possibility to break old dtb's.

Reviewed-by: Fabio Estevam <[email protected]>

Andrew, what do you think?

Thanks

2021-03-30 14:32:02

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

On Tue, Mar 30, 2021 at 11:00:52AM -0300, Fabio Estevam wrote:
> Hi Oleksij,
>
> On Tue, Mar 9, 2021 at 8:26 AM Oleksij Rempel <[email protected]> wrote:
> >
> > changes v2:
> > - rebase against latest kernel
> > - fix networking on RIoTBoard
> >
> > This patch series tries to remove most of the imx6 and imx7 board
> > specific PHY configuration via fixup, as this breaks the PHYs when
> > connected to switch chips or USB Ethernet MACs.
> >
> > Each patch has the possibility to break boards, but contains a
> > recommendation to fix the problem in a more portable and future-proof
> > way.
>
> I think this series moves us in the right direction, even with the
> possibility to break old dtb's.
>
> Reviewed-by: Fabio Estevam <[email protected]>
>
> Andrew, what do you think?

Hi Fabio

I think it should be merged, and we fixup anything which does break.
We are probably at the point where more is broken by not merging it
than merging it.

Andrew

2021-03-30 15:07:52

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hi Andrew,

On Tue, Mar 30, 2021 at 11:30 AM Andrew Lunn <[email protected]> wrote:

> Hi Fabio
>
> I think it should be merged, and we fixup anything which does break.
> We are probably at the point where more is broken by not merging it
> than merging it.

Thanks for your feedback. I agree.

Shawn wants to collect some Acked-by for this series.

Could you please give your Acked-by for this series?

Thanks

2021-04-13 10:11:28

by Lucas Stach

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hi all,

Am Dienstag, dem 09.03.2021 um 12:26 +0100 schrieb Oleksij Rempel:
> changes v2:
> - rebase against latest kernel
> - fix networking on RIoTBoard
>
> This patch series tries to remove most of the imx6 and imx7 board
> specific PHY configuration via fixup, as this breaks the PHYs when
> connected to switch chips or USB Ethernet MACs.
>
> Each patch has the possibility to break boards, but contains a
> recommendation to fix the problem in a more portable and future-proof
> way.

I agree with the opinion that those PHY fixups introduce more harm than
good. Essentially they are pushing board specific configuration values
into the PHY, without any checks that the fixup is even running on the
specific board it was targeted at.

While there is a real chance to break some out of tree boards or
incomplete configs I think that's something we can accept here. If
someone makes a case why they can absolutely not fixup their DT or
kernel config we could even bring back some of those fixups with a
proper board compatible check to avoid mashing things up for other
boards with the same PHY. I guess the only realistic way to learn if
someone can make such a case is to apply this series and look for any
fallout.

So for what it is worth:
Acked-by: Lucas Stach <[email protected]>

> regards,
> Oleksij
>
> Oleksij Rempel (7):
>   ARM: imx6q: remove PHY fixup for KSZ9031
>   ARM: imx6q: remove TX clock delay of ar8031_phy_fixup()
>   ARM: imx6q: remove hand crafted PHY power up in ar8035_phy_fixup()
>   ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035
>     PHYs
>   ARM: imx6q: remove Atheros AR8035 SmartEEE fixup
>   ARM: imx6sx: remove Atheros AR8031 PHY fixup
>   ARM: imx7d: remove Atheros AR8031 PHY fixup
>
>  arch/arm/boot/dts/imx6dl-riotboard.dts | 2 +
>  arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
>  arch/arm/mach-imx/mach-imx6q.c | 85 -------------------------
>  arch/arm/mach-imx/mach-imx6sx.c | 26 --------
>  arch/arm/mach-imx/mach-imx7d.c | 22 -------
>  5 files changed, 3 insertions(+), 134 deletions(-)
>


2021-04-13 14:19:04

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hello,

On Tue, Mar 30, 2021 at 12:04:50PM -0300, Fabio Estevam wrote:
> Hi Andrew,
>
> On Tue, Mar 30, 2021 at 11:30 AM Andrew Lunn <[email protected]> wrote:
>
> > Hi Fabio
> >
> > I think it should be merged, and we fixup anything which does break.
> > We are probably at the point where more is broken by not merging it
> > than merging it.
>
> Thanks for your feedback. I agree.
>
> Shawn wants to collect some Acked-by for this series.
>
> Could you please give your Acked-by for this series?

Andrew, can you please add you ACK?

Shawn will it be enough or you need more ACKs?

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2021-04-13 14:34:49

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

On Tue, Apr 13, 2021 at 12:00:45PM +0200, Lucas Stach wrote:
> I agree with the opinion that those PHY fixups introduce more harm than
> good. Essentially they are pushing board specific configuration values
> into the PHY, without any checks that the fixup is even running on the
> specific board it was targeted at.

Yes and no. The problem is, that's an easy statement to make when one
doesn't understand what they're all doing.

Some are "board specific" in that the normal setup for e.g. iMX6 would
be to enable clock output from the AR8035 PHY and feed that into the
iMX6 - as far as I'm aware, that's the only working configuration for
that SoC and PHY. However, it's also true that this fixup should not
be applied unconditionally.

Then there's SmartEEE - it has been found that the PHY defaults for
this lead to link drops independent of the board and SoC that it is
connected to. It seems that the PHY is essentially broken - it powers
up with SmartEEE enabled, and when connected to another SmartEEE
supporting device, it seems guaranteed that it will result in link
drops in its default configuration.

Freescale's approach has apparently been to unconditionally disable
SmartEEE for all their platforms because of this. With a bit of
research however (as has been done by Jon and myself) we've found
that increasing the Tw parameter for 1G connections results in a
much more stable link.

So, just saying that these are bad without actually understanding what
they are doing is _also_ bad.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2021-04-13 14:44:14

by Lucas Stach

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] remove different PHY fixups

Hi Russell,

sorry for the noise of this arriving in your inbox twice. Apparently I
messed up and replied in private in my last mail.

Am Dienstag, dem 13.04.2021 um 11:51 +0100 schrieb Russell King - ARM Linux admin:
> On Tue, Apr 13, 2021 at 12:00:45PM +0200, Lucas Stach wrote:
> > I agree with the opinion that those PHY fixups introduce more harm than
> > good. Essentially they are pushing board specific configuration values
> > into the PHY, without any checks that the fixup is even running on the
> > specific board it was targeted at.
>
> Yes and no. The problem is, that's an easy statement to make when one
> doesn't understand what they're all doing.
>
> Some are "board specific" in that the normal setup for e.g. iMX6 would
> be to enable clock output from the AR8035 PHY and feed that into the
> iMX6 - as far as I'm aware, that's the only working configuration for
> that SoC and PHY. However, it's also true that this fixup should not
> be applied unconditionally.
>
> Then there's SmartEEE - it has been found that the PHY defaults for
> this lead to link drops independent of the board and SoC that it is
> connected to. It seems that the PHY is essentially broken - it powers
> up with SmartEEE enabled, and when connected to another SmartEEE
> supporting device, it seems guaranteed that it will result in link
> drops in its default configuration.
>
> Freescale's approach has apparently been to unconditionally disable
> SmartEEE for all their platforms because of this. With a bit of
> research however (as has been done by Jon and myself) we've found
> that increasing the Tw parameter for 1G connections results in a
> much more stable link.
>
> So, just saying that these are bad without actually understanding what
> they are doing is _also_ bad.

I'm not saying the fixups are bad per se. What I'm saying is that they
are inherently board specific and the right way to apply them is either
via DT properties, or if absolutely necessary via a fixup that at least
checks that it is running on the specific board it was targeted at.

While SmartEEE disabling will cause no big harm, aside from a bit more
power consumption, a wrong clock configuration can cause major
confusion. Especially if the configuration in DT and values put into
the PHY via fixups differ from each other.

Regards,
Lucas


2021-05-11 01:53:13

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH v2 1/7] ARM: imx6q: remove PHY fixup for KSZ9031

On Tue, Mar 09, 2021 at 12:26:09PM +0100, Oleksij Rempel wrote:
> Starting with:
>
> bcf3440c6dd7 ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
>
> the micrel phy driver started respecting phy-mode for the KSZ9031 PHY.
> At least with kernel v5.8 configuration provided by this fixup was
> overwritten by the micrel driver.
>
> This fixup was providing following configuration:
>
> RX path: 2.58ns delay
> rx -0.42 (left shift) + rx_clk +0.96ns (right shift) =
> 1,38 + 1,2 internal RX delay = 2.58ns
> TX path: 0.96ns delay
> tx (no delay) + tx_clk 0.96ns (right shift) = 0.96ns
>
> This configuration is outside of the recommended RGMII clock skew delays
> and about in the middle of: rgmii-idrx and rgmii-id
>
> Since most embedded systems do not have enough place to introduce
> significant clock skew, rgmii-id is the way to go.
>
> In case this patch breaks network functionality on your system, build
> kernel with enabled MICREL_PHY. If it is still not working then try
> following device tree options:
> 1. Set (or change) phy-mode in DT to:
> phy-mode = "rgmii-id";
> This actives internal delay for both RX and TX.
> 1. Set (or change) phy-mode in DT to:
> phy-mode = "rgmii-idrx";
> This actives internal delay for RX only.
> 3. Use following DT properties:
> phy-mode = "rgmii";
> txen-skew-psec = <0>;
> rxdv-skew-psec = <0>;
> rxd0-skew-psec = <0>;
> rxd1-skew-psec = <0>;
> rxd2-skew-psec = <0>;
> rxd3-skew-psec = <0>;
> rxc-skew-psec = <1860>;
> txc-skew-psec = <1860>;
> This activates the internal delays for RX and TX, with the value as
> the fixup that is removed in this patch.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> Acked-by: Philippe Schenker <[email protected]>
> ---
> arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +-
> arch/arm/mach-imx/mach-imx6q.c | 23 -----------------------

I have different branch for DTS and mach-imx change. Please split the
changes.

Shawn

> 2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts b/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
> index fa2307d8ce86..c713ac03b3b9 100644
> --- a/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
> +++ b/arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts
> @@ -112,7 +112,7 @@ flash: m25p80@0 {
> &fec {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_enet>;
> - phy-mode = "rgmii";
> + phy-mode = "rgmii-id";
> phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
> phy-supply = <&vgen2_1v2_eth>;
> status = "okay";
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 703998ebb52e..78205f90da27 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -40,27 +40,6 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
> return 0;
> }
>
> -static void mmd_write_reg(struct phy_device *dev, int device, int reg, int val)
> -{
> - phy_write(dev, 0x0d, device);
> - phy_write(dev, 0x0e, reg);
> - phy_write(dev, 0x0d, (1 << 14) | device);
> - phy_write(dev, 0x0e, val);
> -}
> -
> -static int ksz9031rn_phy_fixup(struct phy_device *dev)
> -{
> - /*
> - * min rx data delay, max rx/tx clock delay,
> - * min rx/tx control delay
> - */
> - mmd_write_reg(dev, 2, 4, 0);
> - mmd_write_reg(dev, 2, 5, 0);
> - mmd_write_reg(dev, 2, 8, 0x003ff);
> -
> - return 0;
> -}
> -
> /*
> * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High
> * as they are used for slots1-7 PERST#
> @@ -152,8 +131,6 @@ static void __init imx6q_enet_phy_init(void)
> if (IS_BUILTIN(CONFIG_PHYLIB)) {
> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
> ksz9021rn_phy_fixup);
> - phy_register_fixup_for_uid(PHY_ID_KSZ9031, MICREL_PHY_ID_MASK,
> - ksz9031rn_phy_fixup);
> phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef,
> ar8031_phy_fixup);
> phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef,
> --
> 2.29.2
>