* [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup @ 2020-03-05 13:49 Philippe Schenker 2020-03-05 13:53 ` Russell King - ARM Linux admin 2020-03-05 14:38 ` Oleksij Rempel 0 siblings, 2 replies; 15+ messages in thread From: Philippe Schenker @ 2020-03-05 13:49 UTC (permalink / raw) To: Russell King, linux-arm-kernel, NXP Linux Team, Fabio Estevam Cc: Philippe Schenker, Allison Randal, linux-kernel, Thomas Gleixner, Kate Stewart, Greg Kroah-Hartman, Pengutronix Kernel Team, Sascha Hauer, Shawn Guo The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 PHY is like KSZ9031 adhering to RGMII v2.0 specification. This means the MAC should provide a delay to the TXC line. Because the i.MX6 MAC does not provide this delay this has to be done in the PHY. This patch adds by default ~1.6ns delay to the TXC line. This should be good for all boards that have the RGMII signals routed with the same length. The KSZ9131 has relatively high tolerances on skew registers from MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used and then as little as possibly subtracted from that so we get more accurate delay. This is actually needed because the i.MX6 SoC has an asynchron skew on TXC from -100ps to 900ps, to get all RGMII values within spec. Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> --- arch/arm/mach-imx/mach-imx6q.c | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index edd26e0ffeec..8ae5f2fa33e2 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, int device, int reg, int val) phy_write(dev, 0x0e, val); } +static int mmd_read_reg(struct phy_device *dev, int device, int reg) +{ + phy_write(dev, 0x0d, device); + phy_write(dev, 0x0e, reg); + phy_write(dev, 0x0d, (1 << 14) | device); + return phy_read(dev, 0x0e); +} + static int ksz9031rn_phy_fixup(struct phy_device *dev) { /* @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev) return 0; } +#define KSZ9131_RXTXDLL_BYPASS 12 + +static int ksz9131rn_phy_fixup(struct phy_device *dev) +{ + int tmp; + + tmp = mmd_read_reg(dev, 2, 0x4c); + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); + mmd_write_reg(dev, 2, 0x4c, tmp); + + tmp = mmd_read_reg(dev, 2, 0x4d); + /* disable txdll bypass (enable 2ns skew delay on TXC) */ + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); + mmd_write_reg(dev, 2, 0x4d, tmp); + + /* + * Subtract ~0.6ns from txdll = ~1.4ns delay. + * leave RXC path untouched + */ + mmd_write_reg(dev, 2, 4, 0x007d); + mmd_write_reg(dev, 2, 6, 0xdddd); + mmd_write_reg(dev, 2, 8, 0x0007); + + return 0; +} + /* * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High * as they are used for slots1-7 PERST# @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) 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_KSZ9131, MICREL_PHY_ID_MASK, + ksz9131rn_phy_fixup); phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, ar8031_phy_fixup); phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 13:49 [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup Philippe Schenker @ 2020-03-05 13:53 ` Russell King - ARM Linux admin 2020-03-06 9:57 ` Philippe Schenker 2020-03-05 14:38 ` Oleksij Rempel 1 sibling, 1 reply; 15+ messages in thread From: Russell King - ARM Linux admin @ 2020-03-05 13:53 UTC (permalink / raw) To: Philippe Schenker Cc: linux-arm-kernel, NXP Linux Team, Fabio Estevam, Allison Randal, linux-kernel, Thomas Gleixner, Kate Stewart, Greg Kroah-Hartman, Pengutronix Kernel Team, Sascha Hauer, Shawn Guo On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 PHY > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > MAC should provide a delay to the TXC line. Because the i.MX6 MAC does > not provide this delay this has to be done in the PHY. > > This patch adds by default ~1.6ns delay to the TXC line. This should > be good for all boards that have the RGMII signals routed with the > same length. > > The KSZ9131 has relatively high tolerances on skew registers from > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > and then as little as possibly subtracted from that so we get more > accurate delay. This is actually needed because the i.MX6 SoC has > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > values within spec. > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > --- > > arch/arm/mach-imx/mach-imx6q.c | 37 ++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c > index edd26e0ffeec..8ae5f2fa33e2 100644 > --- a/arch/arm/mach-imx/mach-imx6q.c > +++ b/arch/arm/mach-imx/mach-imx6q.c > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, int device, int reg, int val) > phy_write(dev, 0x0e, val); > } > > +static int mmd_read_reg(struct phy_device *dev, int device, int reg) > +{ > + phy_write(dev, 0x0d, device); > + phy_write(dev, 0x0e, reg); > + phy_write(dev, 0x0d, (1 << 14) | device); > + return phy_read(dev, 0x0e); > +} These look like the standard MII MMD registers, and it also looks like you're reinventing phy_read_mmd() - but badly due to lack of locking. I guess you need this because phy_read_mmd() may be modular - maybe we should arrange for the accessors to be separately buildable into the kernel, so that such fixups can stop badly reinventing the wheel? > + > static int ksz9031rn_phy_fixup(struct phy_device *dev) > { > /* > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev) > return 0; > } > > +#define KSZ9131_RXTXDLL_BYPASS 12 > + > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > +{ > + int tmp; > + > + tmp = mmd_read_reg(dev, 2, 0x4c); > + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > + mmd_write_reg(dev, 2, 0x4c, tmp); > + > + tmp = mmd_read_reg(dev, 2, 0x4d); > + /* disable txdll bypass (enable 2ns skew delay on TXC) */ > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > + mmd_write_reg(dev, 2, 0x4d, tmp); > + > + /* > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > + * leave RXC path untouched > + */ > + mmd_write_reg(dev, 2, 4, 0x007d); > + mmd_write_reg(dev, 2, 6, 0xdddd); > + mmd_write_reg(dev, 2, 8, 0x0007); > + > + return 0; > +} > + > /* > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High > * as they are used for slots1-7 PERST# > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > 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_KSZ9131, MICREL_PHY_ID_MASK, > + ksz9131rn_phy_fixup); > phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, > ar8031_phy_fixup); > phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, > -- > 2.25.1 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 13:53 ` Russell King - ARM Linux admin @ 2020-03-06 9:57 ` Philippe Schenker 2020-03-06 10:52 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 9:57 UTC (permalink / raw) To: linux Cc: linux-imx, gregkh, festevam, kernel, linux-kernel, tglx, linux-arm-kernel, kstewart, allison, shawnguo, s.hauer On Thu, 2020-03-05 at 13:53 +0000, Russell King - ARM Linux admin wrote: > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 > > PHY > > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > > MAC should provide a delay to the TXC line. Because the i.MX6 MAC > > does > > not provide this delay this has to be done in the PHY. > > > > This patch adds by default ~1.6ns delay to the TXC line. This should > > be good for all boards that have the RGMII signals routed with the > > same length. > > > > The KSZ9131 has relatively high tolerances on skew registers from > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > > and then as little as possibly subtracted from that so we get more > > accurate delay. This is actually needed because the i.MX6 SoC has > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > values within spec. > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > --- > > > > arch/arm/mach-imx/mach-imx6q.c | 37 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach- > > imx/mach-imx6q.c > > index edd26e0ffeec..8ae5f2fa33e2 100644 > > --- a/arch/arm/mach-imx/mach-imx6q.c > > +++ b/arch/arm/mach-imx/mach-imx6q.c > > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, > > int device, int reg, int val) > > phy_write(dev, 0x0e, val); > > } > > > > +static int mmd_read_reg(struct phy_device *dev, int device, int > > reg) > > +{ > > + phy_write(dev, 0x0d, device); > > + phy_write(dev, 0x0e, reg); > > + phy_write(dev, 0x0d, (1 << 14) | device); > > + return phy_read(dev, 0x0e); > > +} > > These look like the standard MII MMD registers, and it also looks like > you're reinventing phy_read_mmd() - but badly due to lack of locking. > > I guess you need this because phy_read_mmd() may be modular - maybe > we should arrange for the accessors to be separately buildable into > the kernel, so that such fixups can stop badly reinventing the wheel? Yes, I did that because of two reasons: 1. I tried phy_read_mmd() and phy_write_mmd() but this panic'd 2. There is already mmd_write_reg in that code so I thought it would be no big deal to also have a read in there. But yeah, you're right that mmd_write_reg is from 2013... How do you suggest to implement that? Philippe > > > + > > static int ksz9031rn_phy_fixup(struct phy_device *dev) > > { > > /* > > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device > > *dev) > > return 0; > > } > > > > +#define KSZ9131_RXTXDLL_BYPASS 12 > > + > > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > > +{ > > + int tmp; > > + > > + tmp = mmd_read_reg(dev, 2, 0x4c); > > + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > + mmd_write_reg(dev, 2, 0x4c, tmp); > > + > > + tmp = mmd_read_reg(dev, 2, 0x4d); > > + /* disable txdll bypass (enable 2ns skew delay on TXC) */ > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > + mmd_write_reg(dev, 2, 0x4d, tmp); > > + > > + /* > > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > > + * leave RXC path untouched > > + */ > > + mmd_write_reg(dev, 2, 4, 0x007d); > > + mmd_write_reg(dev, 2, 6, 0xdddd); > > + mmd_write_reg(dev, 2, 8, 0x0007); > > + > > + return 0; > > +} > > + > > /* > > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High > > * as they are used for slots1-7 PERST# > > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > > 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_KSZ9131, > > MICREL_PHY_ID_MASK, > > + ksz9131rn_phy_fixup); > > phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, > > ar8031_phy_fixup); > > phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, > > -- > > 2.25.1 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 9:57 ` Philippe Schenker @ 2020-03-06 10:52 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 15+ messages in thread From: Russell King - ARM Linux admin @ 2020-03-06 10:52 UTC (permalink / raw) To: Philippe Schenker Cc: linux-imx, gregkh, festevam, kernel, linux-kernel, tglx, linux-arm-kernel, kstewart, allison, shawnguo, s.hauer On Fri, Mar 06, 2020 at 09:57:15AM +0000, Philippe Schenker wrote: > On Thu, 2020-03-05 at 13:53 +0000, Russell King - ARM Linux admin wrote: > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 > > > PHY > > > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > > > MAC should provide a delay to the TXC line. Because the i.MX6 MAC > > > does > > > not provide this delay this has to be done in the PHY. > > > > > > This patch adds by default ~1.6ns delay to the TXC line. This should > > > be good for all boards that have the RGMII signals routed with the > > > same length. > > > > > > The KSZ9131 has relatively high tolerances on skew registers from > > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > > > and then as little as possibly subtracted from that so we get more > > > accurate delay. This is actually needed because the i.MX6 SoC has > > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > > values within spec. > > > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > > > --- > > > > > > arch/arm/mach-imx/mach-imx6q.c | 37 > > > ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 37 insertions(+) > > > > > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach- > > > imx/mach-imx6q.c > > > index edd26e0ffeec..8ae5f2fa33e2 100644 > > > --- a/arch/arm/mach-imx/mach-imx6q.c > > > +++ b/arch/arm/mach-imx/mach-imx6q.c > > > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, > > > int device, int reg, int val) > > > phy_write(dev, 0x0e, val); > > > } > > > > > > +static int mmd_read_reg(struct phy_device *dev, int device, int > > > reg) > > > +{ > > > + phy_write(dev, 0x0d, device); > > > + phy_write(dev, 0x0e, reg); > > > + phy_write(dev, 0x0d, (1 << 14) | device); > > > + return phy_read(dev, 0x0e); > > > +} > > > > These look like the standard MII MMD registers, and it also looks like > > you're reinventing phy_read_mmd() - but badly due to lack of locking. > > > > I guess you need this because phy_read_mmd() may be modular - maybe > > we should arrange for the accessors to be separately buildable into > > the kernel, so that such fixups can stop badly reinventing the wheel? > > Yes, I did that because of two reasons: > 1. I tried phy_read_mmd() and phy_write_mmd() but this panic'd That is because phydev->drv->read_mmd and phydev->drv is NULL at this point. There has been a patch around to solve that though. > 2. There is already mmd_write_reg in that code so I thought it would be > no big deal to also have a read in there. > > But yeah, you're right that mmd_write_reg is from 2013... > > How do you suggest to implement that? > > Philippe -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 13:49 [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup Philippe Schenker 2020-03-05 13:53 ` Russell King - ARM Linux admin @ 2020-03-05 14:38 ` Oleksij Rempel 2020-03-05 16:51 ` Andrew Lunn 2020-03-06 9:55 ` Philippe Schenker 1 sibling, 2 replies; 15+ messages in thread From: Oleksij Rempel @ 2020-03-05 14:38 UTC (permalink / raw) To: Philippe Schenker Cc: Russell King, linux-arm-kernel, NXP Linux Team, Fabio Estevam, Kate Stewart, Greg Kroah-Hartman, Sascha Hauer, linux-kernel, Pengutronix Kernel Team, Thomas Gleixner, Shawn Guo, Allison Randal Hi Philippe, On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 PHY > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > MAC should provide a delay to the TXC line. Because the i.MX6 MAC does > not provide this delay this has to be done in the PHY. > > This patch adds by default ~1.6ns delay to the TXC line. This should > be good for all boards that have the RGMII signals routed with the > same length. > > The KSZ9131 has relatively high tolerances on skew registers from > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > and then as little as possibly subtracted from that so we get more > accurate delay. This is actually needed because the i.MX6 SoC has > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > values within spec. This configuration has nothing to do in mach-imx/* It belongs to the board devicetree. Please see DT binding documentation for needed properties: Documentation/devicetree/bindings/net/micrel-ksz90x1.txt All of this mach-imx fixups are evil and should be removed or disabled by Kconfig option. Since they will run on all i.MX based boards even if this PHY are connected to some switch and not connected to the FEC directly. And.. If driver didn't made this configuration all this changes will be lost on suspend/resume cycle or on PHY reset. Regards, Oleksij > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > --- > > arch/arm/mach-imx/mach-imx6q.c | 37 ++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c > index edd26e0ffeec..8ae5f2fa33e2 100644 > --- a/arch/arm/mach-imx/mach-imx6q.c > +++ b/arch/arm/mach-imx/mach-imx6q.c > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, int device, int reg, int val) > phy_write(dev, 0x0e, val); > } > > +static int mmd_read_reg(struct phy_device *dev, int device, int reg) > +{ > + phy_write(dev, 0x0d, device); > + phy_write(dev, 0x0e, reg); > + phy_write(dev, 0x0d, (1 << 14) | device); > + return phy_read(dev, 0x0e); > +} > + > static int ksz9031rn_phy_fixup(struct phy_device *dev) > { > /* > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev) > return 0; > } > > +#define KSZ9131_RXTXDLL_BYPASS 12 > + > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > +{ > + int tmp; > + > + tmp = mmd_read_reg(dev, 2, 0x4c); > + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > + mmd_write_reg(dev, 2, 0x4c, tmp); > + > + tmp = mmd_read_reg(dev, 2, 0x4d); > + /* disable txdll bypass (enable 2ns skew delay on TXC) */ > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > + mmd_write_reg(dev, 2, 0x4d, tmp); > + > + /* > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > + * leave RXC path untouched > + */ > + mmd_write_reg(dev, 2, 4, 0x007d); > + mmd_write_reg(dev, 2, 6, 0xdddd); > + mmd_write_reg(dev, 2, 8, 0x0007); > + > + return 0; > +} > + > /* > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High > * as they are used for slots1-7 PERST# > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > 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_KSZ9131, MICREL_PHY_ID_MASK, > + ksz9131rn_phy_fixup); > phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, > ar8031_phy_fixup); > phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, > -- > 2.25.1 > > > -- 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 | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 14:38 ` Oleksij Rempel @ 2020-03-05 16:51 ` Andrew Lunn 2020-03-06 7:42 ` Ahmad Fatoum 2020-03-06 9:55 ` Philippe Schenker 1 sibling, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2020-03-05 16:51 UTC (permalink / raw) To: Oleksij Rempel Cc: Philippe Schenker, Kate Stewart, Greg Kroah-Hartman, Sascha Hauer, Allison Randal, Russell King, linux-kernel, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo, Thomas Gleixner, Fabio Estevam, linux-arm-kernel On Thu, Mar 05, 2020 at 03:38:05PM +0100, Oleksij Rempel wrote: > Hi Philippe, > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 PHY > > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > > MAC should provide a delay to the TXC line. Because the i.MX6 MAC does > > not provide this delay this has to be done in the PHY. > > > > This patch adds by default ~1.6ns delay to the TXC line. This should > > be good for all boards that have the RGMII signals routed with the > > same length. > > > > The KSZ9131 has relatively high tolerances on skew registers from > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > > and then as little as possibly subtracted from that so we get more > > accurate delay. This is actually needed because the i.MX6 SoC has > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > values within spec. > > This configuration has nothing to do in mach-imx/* It belongs to the > board devicetree. Please see DT binding documentation for needed > properties: > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt It probably does not even need that. Just phy-mode = <rgmii-txid> Also, please Cc: netdev for network code. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 16:51 ` Andrew Lunn @ 2020-03-06 7:42 ` Ahmad Fatoum 2020-03-06 9:46 ` Philippe Schenker 2020-03-06 13:38 ` Andrew Lunn 0 siblings, 2 replies; 15+ messages in thread From: Ahmad Fatoum @ 2020-03-06 7:42 UTC (permalink / raw) To: Andrew Lunn, Oleksij Rempel Cc: Kate Stewart, Fabio Estevam, Greg Kroah-Hartman, Sascha Hauer, Russell King, linux-kernel, Philippe Schenker, NXP Linux Team, Pengutronix Kernel Team, Thomas Gleixner, Shawn Guo, Allison Randal, linux-arm-kernel Hello Andrew, On 3/5/20 5:51 PM, Andrew Lunn wrote: > On Thu, Mar 05, 2020 at 03:38:05PM +0100, Oleksij Rempel wrote: >> Hi Philippe, >> >> On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: >>> The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 PHY >>> is like KSZ9031 adhering to RGMII v2.0 specification. This means the >>> MAC should provide a delay to the TXC line. Because the i.MX6 MAC does >>> not provide this delay this has to be done in the PHY. >>> >>> This patch adds by default ~1.6ns delay to the TXC line. This should >>> be good for all boards that have the RGMII signals routed with the >>> same length. >>> >>> The KSZ9131 has relatively high tolerances on skew registers from >>> MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used >>> and then as little as possibly subtracted from that so we get more >>> accurate delay. This is actually needed because the i.MX6 SoC has >>> an asynchron skew on TXC from -100ps to 900ps, to get all RGMII >>> values within spec. >> >> This configuration has nothing to do in mach-imx/* It belongs to the >> board devicetree. Please see DT binding documentation for needed >> properties: >> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt > > It probably does not even need that. Just > > phy-mode = <rgmii-txid> Looks to me like this isn't supported by the Micrel PHY driver or am I missing something? Cheers Ahmad -- 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 | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 7:42 ` Ahmad Fatoum @ 2020-03-06 9:46 ` Philippe Schenker 2020-03-06 11:14 ` Ahmad Fatoum 2020-03-06 13:38 ` Andrew Lunn 1 sibling, 1 reply; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 9:46 UTC (permalink / raw) To: o.rempel, a.fatoum, andrew Cc: shawnguo, kernel, gregkh, festevam, linux, linux-kernel, linux-imx, tglx, s.hauer, allison, linux-arm-kernel, kstewart On Fri, 2020-03-06 at 08:42 +0100, Ahmad Fatoum wrote: > Hello Andrew, > > On 3/5/20 5:51 PM, Andrew Lunn wrote: > > On Thu, Mar 05, 2020 at 03:38:05PM +0100, Oleksij Rempel wrote: > > > Hi Philippe, > > > > > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The > > > > KSZ9131 PHY > > > > is like KSZ9031 adhering to RGMII v2.0 specification. This means > > > > the > > > > MAC should provide a delay to the TXC line. Because the i.MX6 > > > > MAC does > > > > not provide this delay this has to be done in the PHY. > > > > > > > > This patch adds by default ~1.6ns delay to the TXC line. This > > > > should > > > > be good for all boards that have the RGMII signals routed with > > > > the > > > > same length. > > > > > > > > The KSZ9131 has relatively high tolerances on skew registers > > > > from > > > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is > > > > used > > > > and then as little as possibly subtracted from that so we get > > > > more > > > > accurate delay. This is actually needed because the i.MX6 SoC > > > > has > > > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > > > values within spec. > > > > > > This configuration has nothing to do in mach-imx/* It belongs to > > > the > > > board devicetree. Please see DT binding documentation for needed > > > properties: > > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt > > > > It probably does not even need that. Just > > > > phy-mode = <rgmii-txid> > > Looks to me like this isn't supported by the Micrel PHY driver or am > I missing something? > > Cheers > Ahmad > Hi Andrew and Ahmad, thanks for your comments. I totally forgot about those more specific phy-modes. But just because none of our driver supports that. Either the i.MX6 fec-driver as well as the micrel.c PHY driver supports this tags. What do you guys suggest then how I should implement that skew stuff? The problem is that i.MX6 has an asynchronic skew of -100 to 900ps only enabling the PHY-delay on TXC and RXC is not in all cases within the RGMII timing specs. That's why I implemented this 'weird' numbers. Philippe ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 9:46 ` Philippe Schenker @ 2020-03-06 11:14 ` Ahmad Fatoum 2020-03-06 12:16 ` Philippe Schenker 0 siblings, 1 reply; 15+ messages in thread From: Ahmad Fatoum @ 2020-03-06 11:14 UTC (permalink / raw) To: Philippe Schenker, o.rempel, andrew Cc: shawnguo, kernel, gregkh, festevam, linux, linux-kernel, linux-imx, tglx, s.hauer, allison, linux-arm-kernel, kstewart Hello Philippe, On 3/6/20 10:46 AM, Philippe Schenker wrote: > Hi Andrew and Ahmad, thanks for your comments. I totally forgot about > those more specific phy-modes. But just because none of our driver > supports that. Either the i.MX6 fec-driver as well as the micrel.c PHY > driver supports this tags. > What do you guys suggest then how I should implement that skew stuff? I think implementing them in the Micrel driver would make sense. When more specific skews are supplied, these are used. If not, the rgmii_[tx]?id applies the appropriate timings for length matched lines. Device trees matching your use case will then only have to specify rgmii-txid. > The problem is that i.MX6 has an asynchronic skew of -100 to 900ps only > enabling the PHY-delay on TXC and RXC is not in all cases within the > RGMII timing specs. That's why I implemented this 'weird' numbers. I am not too well-versed with this. What's an asynchronic skew? A non-deterministic internal delay..? So, you try to be as accurate as possible, so the skew is within the acceptable margin? Cheers Ahmad > > Philippe > -- 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 | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 11:14 ` Ahmad Fatoum @ 2020-03-06 12:16 ` Philippe Schenker 0 siblings, 0 replies; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 12:16 UTC (permalink / raw) To: o.rempel, a.fatoum, andrew Cc: linux, kernel, festevam, linux-imx, gregkh, linux-kernel, tglx, allison, shawnguo, linux-arm-kernel, kstewart, s.hauer On Fri, 2020-03-06 at 12:14 +0100, Ahmad Fatoum wrote: > Hello Philippe, > > On 3/6/20 10:46 AM, Philippe Schenker wrote: > > Hi Andrew and Ahmad, thanks for your comments. I totally forgot > > about > > those more specific phy-modes. But just because none of our driver > > supports that. Either the i.MX6 fec-driver as well as the micrel.c > > PHY > > driver supports this tags. > > What do you guys suggest then how I should implement that skew > > stuff? > > I think implementing them in the Micrel driver would make sense. > When more specific skews are supplied, these are used. > If not, the rgmii_[tx]?id applies the appropriate timings for length > matched > lines. Device trees matching your use case will then only have to > specify > rgmii-txid. > > > The problem is that i.MX6 has an asynchronic skew of -100 to 900ps > > only > > enabling the PHY-delay on TXC and RXC is not in all cases within the > > RGMII timing specs. That's why I implemented this 'weird' numbers. > > I am not too well-versed with this. What's an asynchronic skew? > A non-deterministic internal delay..? So, you try to be as accurate as > possible, so the skew is within the acceptable margin? Asynchronic was a term I introduced because in RGMII spec, TXC of a MAC should have -500 to 500ps skew. However the i.MX6 has "asynchronic" -100 to 900ps. I did a worst-case study of those timing values. If I only enable the 2ns delay on the KSZ9131 PHY this is resulting in a T_setup_T of 1.9- 2.4ns (min-max). Under the assumption that tcyc (cycle time of the clock) has min-max of 7.2-8.8ns this results in T_hold_T values of min- max 0.7-2.5ns. The 0.7ns should be at least 1ns according to spec. If I fine tune these values with the other registers I can "middle-out" the clock-edges in relation to data signals and therefore I get all values to be within RGMII timing specs. > > Cheers > Ahmad > > > > Philippe > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 7:42 ` Ahmad Fatoum 2020-03-06 9:46 ` Philippe Schenker @ 2020-03-06 13:38 ` Andrew Lunn 2020-03-06 16:30 ` Philippe Schenker 1 sibling, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2020-03-06 13:38 UTC (permalink / raw) To: Ahmad Fatoum Cc: Oleksij Rempel, Kate Stewart, Fabio Estevam, Greg Kroah-Hartman, Sascha Hauer, Russell King, linux-kernel, Philippe Schenker, NXP Linux Team, Pengutronix Kernel Team, Thomas Gleixner, Shawn Guo, Allison Randal, linux-arm-kernel > > It probably does not even need that. Just > > > > phy-mode = <rgmii-txid> > > Looks to me like this isn't supported by the Micrel PHY driver or am > I missing something? Ah, you are correct. It just has: if (of_node) { ksz9021_load_values_from_of(phydev, of_node, MII_KSZPHY_CLK_CONTROL_PAD_SKEW, "txen-skew-ps", "txc-skew-ps", "rxdv-skew-ps", "rxc-skew-ps"); ksz9021_load_values_from_of(phydev, of_node, MII_KSZPHY_RX_DATA_PAD_SKEW, "rxd0-skew-ps", "rxd1-skew-ps", "rxd2-skew-ps", "rxd3-skew-ps"); ksz9021_load_values_from_of(phydev, of_node, MII_KSZPHY_TX_DATA_PAD_SKEW, "txd0-skew-ps", "txd1-skew-ps", "txd2-skew-ps", "txd3-skew-ps"); } and no support for phydev->interface. At minimum, you should use these DT properties, not a platform fixup. If you want to, you can add support for rgmii-id, etc. There are five modes you need to support: PHY_INTERFACE_MODE_NA, PHY_INTERFACE_MODE_RGMII, PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID, PHY_INTERFACE_MODE_RGMII_TXID, NA means "don't touch". Leave the RGMII delays alone, as configured by hardware default, strapping, bootloader, etc. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 13:38 ` Andrew Lunn @ 2020-03-06 16:30 ` Philippe Schenker 0 siblings, 0 replies; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 16:30 UTC (permalink / raw) To: a.fatoum, andrew Cc: shawnguo, linux, gregkh, festevam, s.hauer, linux-kernel, linux-imx, tglx, o.rempel, allison, kernel, kstewart, linux-arm-kernel On Fri, 2020-03-06 at 14:38 +0100, Andrew Lunn wrote: > > > It probably does not even need that. Just > > > > > > phy-mode = <rgmii-txid> > > > > Looks to me like this isn't supported by the Micrel PHY driver or am > > I missing something? > > Ah, you are correct. It just has: > > if (of_node) { > ksz9021_load_values_from_of(phydev, of_node, > MII_KSZPHY_CLK_CONTROL_PAD_SKEW, > "txen-skew-ps", "txc-skew-ps", > "rxdv-skew-ps", "rxc-skew-ps"); > ksz9021_load_values_from_of(phydev, of_node, > MII_KSZPHY_RX_DATA_PAD_SKEW, > "rxd0-skew-ps", "rxd1-skew-ps", > "rxd2-skew-ps", "rxd3-skew-ps"); > ksz9021_load_values_from_of(phydev, of_node, > MII_KSZPHY_TX_DATA_PAD_SKEW, > "txd0-skew-ps", "txd1-skew-ps", > "txd2-skew-ps", "txd3-skew-ps"); > } > > and no support for phydev->interface. > > At minimum, you should use these DT properties, not a platform fixup. As I said, I still think it is a good idea to have similar solutions at the same place, especially for a successor PHY. I also see the downsides so I'll go with your proposed solution. Thanks everyone for the discussion! Philippe > > If you want to, you can add support for rgmii-id, etc. There are five > modes you need to support: > > PHY_INTERFACE_MODE_NA, > PHY_INTERFACE_MODE_RGMII, > PHY_INTERFACE_MODE_RGMII_ID, > PHY_INTERFACE_MODE_RGMII_RXID, > PHY_INTERFACE_MODE_RGMII_TXID, > > NA means "don't touch". Leave the RGMII delays alone, as configured by > hardware default, strapping, bootloader, etc. > > Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-05 14:38 ` Oleksij Rempel 2020-03-05 16:51 ` Andrew Lunn @ 2020-03-06 9:55 ` Philippe Schenker 2020-03-06 10:38 ` Oleksij Rempel 1 sibling, 1 reply; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 9:55 UTC (permalink / raw) To: o.rempel Cc: linux, gregkh, festevam, s.hauer, linux-kernel, linux-imx, linux-arm-kernel, kstewart, tglx, kernel, shawnguo, allison On Thu, 2020-03-05 at 15:38 +0100, Oleksij Rempel wrote: > Hi Philippe, > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 > > PHY > > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > > MAC should provide a delay to the TXC line. Because the i.MX6 MAC > > does > > not provide this delay this has to be done in the PHY. > > > > This patch adds by default ~1.6ns delay to the TXC line. This should > > be good for all boards that have the RGMII signals routed with the > > same length. > > > > The KSZ9131 has relatively high tolerances on skew registers from > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > > and then as little as possibly subtracted from that so we get more > > accurate delay. This is actually needed because the i.MX6 SoC has > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > values within spec. Hi Oleksij! Thanks for your comments and review. > > This configuration has nothing to do in mach-imx/* It belongs to the > board devicetree. Please see DT binding documentation for needed > properties: > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt I know that nowadays this stuff only belongs in the devicetree. I fully agree with you. I am also aware of the devicetree bindings. > > All of this mach-imx fixups are evil and should be removed or disabled > by Kconfig > option. Since they will run on all i.MX based boards even if this PHY > are > connected to some switch and not connected to the FEC directly. > And.. If driver didn't made this configuration all this changes will > be lost on > suspend/resume cycle or on PHY reset. I am also aware of this behaviour. But the i.MX6 is a SoC used in embedded applications and I guess no one comes and plugs in a PCIe MAC card in an embedded device. But yes you're right you never know. Because the i.MX6 is an embedded processor I still think we should place that fixup in mach-imx. There is already a fixup for the predecessor KSZ9031 in that code. The KSZ9131 is pin-compatible with KSZ9031 and also software compatible, just not with the skew settings. I really dislike reinventing the weel here for an old SoC. Philippe > > Regards, > Oleksij > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > --- > > > > arch/arm/mach-imx/mach-imx6q.c | 37 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach- > > imx/mach-imx6q.c > > index edd26e0ffeec..8ae5f2fa33e2 100644 > > --- a/arch/arm/mach-imx/mach-imx6q.c > > +++ b/arch/arm/mach-imx/mach-imx6q.c > > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, > > int device, int reg, int val) > > phy_write(dev, 0x0e, val); > > } > > > > +static int mmd_read_reg(struct phy_device *dev, int device, int > > reg) > > +{ > > + phy_write(dev, 0x0d, device); > > + phy_write(dev, 0x0e, reg); > > + phy_write(dev, 0x0d, (1 << 14) | device); > > + return phy_read(dev, 0x0e); > > +} > > + > > static int ksz9031rn_phy_fixup(struct phy_device *dev) > > { > > /* > > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device > > *dev) > > return 0; > > } > > > > +#define KSZ9131_RXTXDLL_BYPASS 12 > > + > > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > > +{ > > + int tmp; > > + > > + tmp = mmd_read_reg(dev, 2, 0x4c); > > + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > + mmd_write_reg(dev, 2, 0x4c, tmp); > > + > > + tmp = mmd_read_reg(dev, 2, 0x4d); > > + /* disable txdll bypass (enable 2ns skew delay on TXC) */ > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > + mmd_write_reg(dev, 2, 0x4d, tmp); > > + > > + /* > > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > > + * leave RXC path untouched > > + */ > > + mmd_write_reg(dev, 2, 4, 0x007d); > > + mmd_write_reg(dev, 2, 6, 0xdddd); > > + mmd_write_reg(dev, 2, 8, 0x0007); > > + > > + return 0; > > +} > > + > > /* > > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High > > * as they are used for slots1-7 PERST# > > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > > 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_KSZ9131, > > MICREL_PHY_ID_MASK, > > + ksz9131rn_phy_fixup); > > phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, > > ar8031_phy_fixup); > > phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, > > -- > > 2.25.1 > > > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 9:55 ` Philippe Schenker @ 2020-03-06 10:38 ` Oleksij Rempel 2020-03-06 12:36 ` Philippe Schenker 0 siblings, 1 reply; 15+ messages in thread From: Oleksij Rempel @ 2020-03-06 10:38 UTC (permalink / raw) To: Philippe Schenker Cc: kstewart, gregkh, s.hauer, allison, linux, linux-kernel, linux-imx, kernel, shawnguo, tglx, festevam, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 5914 bytes --] Hi Philippe, On Fri, Mar 06, 2020 at 09:55:06AM +0000, Philippe Schenker wrote: > On Thu, 2020-03-05 at 15:38 +0100, Oleksij Rempel wrote: > > Hi Philippe, > > > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The KSZ9131 > > > PHY > > > is like KSZ9031 adhering to RGMII v2.0 specification. This means the > > > MAC should provide a delay to the TXC line. Because the i.MX6 MAC > > > does > > > not provide this delay this has to be done in the PHY. > > > > > > This patch adds by default ~1.6ns delay to the TXC line. This should > > > be good for all boards that have the RGMII signals routed with the > > > same length. > > > > > > The KSZ9131 has relatively high tolerances on skew registers from > > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is used > > > and then as little as possibly subtracted from that so we get more > > > accurate delay. This is actually needed because the i.MX6 SoC has > > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > > values within spec. > > Hi Oleksij! Thanks for your comments and review. > > > > This configuration has nothing to do in mach-imx/* It belongs to the > > board devicetree. Please see DT binding documentation for needed > > properties: > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt > > I know that nowadays this stuff only belongs in the devicetree. I fully > agree with you. I am also aware of the devicetree bindings. > > > > All of this mach-imx fixups are evil and should be removed or disabled > > by Kconfig > > option. Since they will run on all i.MX based boards even if this PHY > > are > > connected to some switch and not connected to the FEC directly. > > And.. If driver didn't made this configuration all this changes will > > be lost on > > suspend/resume cycle or on PHY reset. > > I am also aware of this behaviour. ... ò_ô ... > But the i.MX6 is a SoC used in > embedded applications and I guess no one comes and plugs in a PCIe MAC > card in an embedded device. ... hm ... > But yes you're right you never know. well, it is not theoretical discussion. This devices do exist.. With this patch you will break other existing systems. > Because the i.MX6 is an embedded processor I still think we should place > that fixup in mach-imx. There is already a fixup for the predecessor > KSZ9031 in that code. The KSZ9131 is pin-compatible with KSZ9031 and > also software compatible, just not with the skew settings. This fixups will be removed or disabled with Kconfig option: https://lore.kernel.org/patchwork/patch/1164172/ > I really dislike reinventing the weel here for an old SoC. Well, you are doing it not for a SoC (old or new), you are doing it for PHY. PHY fixes belong to PHY driver. > Philippe > > > > Regards, > > Oleksij > > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > > > --- > > > > > > arch/arm/mach-imx/mach-imx6q.c | 37 > > > ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 37 insertions(+) > > > > > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach- > > > imx/mach-imx6q.c > > > index edd26e0ffeec..8ae5f2fa33e2 100644 > > > --- a/arch/arm/mach-imx/mach-imx6q.c > > > +++ b/arch/arm/mach-imx/mach-imx6q.c > > > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device *dev, > > > int device, int reg, int val) > > > phy_write(dev, 0x0e, val); > > > } > > > > > > +static int mmd_read_reg(struct phy_device *dev, int device, int > > > reg) > > > +{ > > > + phy_write(dev, 0x0d, device); > > > + phy_write(dev, 0x0e, reg); > > > + phy_write(dev, 0x0d, (1 << 14) | device); > > > + return phy_read(dev, 0x0e); > > > +} > > > + > > > static int ksz9031rn_phy_fixup(struct phy_device *dev) > > > { > > > /* > > > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct phy_device > > > *dev) > > > return 0; > > > } > > > > > > +#define KSZ9131_RXTXDLL_BYPASS 12 > > > + > > > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > > > +{ > > > + int tmp; > > > + > > > + tmp = mmd_read_reg(dev, 2, 0x4c); > > > + /* disable rxdll bypass (enable 2ns skew delay on RXC) */ > > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > > + mmd_write_reg(dev, 2, 0x4c, tmp); > > > + > > > + tmp = mmd_read_reg(dev, 2, 0x4d); > > > + /* disable txdll bypass (enable 2ns skew delay on TXC) */ > > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > > + mmd_write_reg(dev, 2, 0x4d, tmp); > > > + > > > + /* > > > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > > > + * leave RXC path untouched > > > + */ > > > + mmd_write_reg(dev, 2, 4, 0x007d); > > > + mmd_write_reg(dev, 2, 6, 0xdddd); > > > + mmd_write_reg(dev, 2, 8, 0x0007); > > > + > > > + return 0; > > > +} > > > + > > > /* > > > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output High > > > * as they are used for slots1-7 PERST# > > > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > > > 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_KSZ9131, > > > MICREL_PHY_ID_MASK, > > > + ksz9131rn_phy_fixup); > > > phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, > > > ar8031_phy_fixup); > > > phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, > > > -- > > > 2.25.1 > > > > > > > > > -- 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 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup 2020-03-06 10:38 ` Oleksij Rempel @ 2020-03-06 12:36 ` Philippe Schenker 0 siblings, 0 replies; 15+ messages in thread From: Philippe Schenker @ 2020-03-06 12:36 UTC (permalink / raw) To: o.rempel Cc: kernel, gregkh, s.hauer, linux, linux-kernel, allison, tglx, kstewart, linux-imx, shawnguo, festevam, linux-arm-kernel On Fri, 2020-03-06 at 11:38 +0100, Oleksij Rempel wrote: > Hi Philippe, > > On Fri, Mar 06, 2020 at 09:55:06AM +0000, Philippe Schenker wrote: > > On Thu, 2020-03-05 at 15:38 +0100, Oleksij Rempel wrote: > > > Hi Philippe, > > > > > > On Thu, Mar 05, 2020 at 02:49:28PM +0100, Philippe Schenker wrote: > > > > The MAC of the i.MX6 SoC is compliant with RGMII v1.3. The > > > > KSZ9131 > > > > PHY > > > > is like KSZ9031 adhering to RGMII v2.0 specification. This means > > > > the > > > > MAC should provide a delay to the TXC line. Because the i.MX6 > > > > MAC > > > > does > > > > not provide this delay this has to be done in the PHY. > > > > > > > > This patch adds by default ~1.6ns delay to the TXC line. This > > > > should > > > > be good for all boards that have the RGMII signals routed with > > > > the > > > > same length. > > > > > > > > The KSZ9131 has relatively high tolerances on skew registers > > > > from > > > > MMD 2.4 to MMD 2.8. Therefore the new DLL-based delay of 2ns is > > > > used > > > > and then as little as possibly subtracted from that so we get > > > > more > > > > accurate delay. This is actually needed because the i.MX6 SoC > > > > has > > > > an asynchron skew on TXC from -100ps to 900ps, to get all RGMII > > > > values within spec. > > > > Hi Oleksij! Thanks for your comments and review. > > > This configuration has nothing to do in mach-imx/* It belongs to > > > the > > > board devicetree. Please see DT binding documentation for needed > > > properties: > > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt > > > > I know that nowadays this stuff only belongs in the devicetree. I > > fully > > agree with you. I am also aware of the devicetree bindings. > > > All of this mach-imx fixups are evil and should be removed or > > > disabled > > > by Kconfig > > > option. Since they will run on all i.MX based boards even if this > > > PHY > > > are > > > connected to some switch and not connected to the FEC directly. > > > And.. If driver didn't made this configuration all this changes > > > will > > > be lost on > > > suspend/resume cycle or on PHY reset. > > > > I am also aware of this behaviour. > > ... ò_ô ... This does not help in finding a solution. > > > But the i.MX6 is a SoC used in > > embedded applications and I guess no one comes and plugs in a PCIe > > MAC > > card in an embedded device. > > ... hm ... > > > But yes you're right you never know. > > well, it is not theoretical discussion. This devices do exist.. With > this patch you will break other existing systems. > > > Because the i.MX6 is an embedded processor I still think we should > > place > > that fixup in mach-imx. There is already a fixup for the predecessor > > KSZ9031 in that code. The KSZ9131 is pin-compatible with KSZ9031 and > > also software compatible, just not with the skew settings. > > This fixups will be removed or disabled with Kconfig option: > https://lore.kernel.org/patchwork/patch/1164172/ With this patch you will break our iMX6 board... Can you point me to the v2 you mentioned in there? > > > I really dislike reinventing the weel here for an old SoC. > > Well, you are doing it not for a SoC (old or new), you are doing it > for > PHY. PHY fixes belong to PHY driver. Please be more precise. My patch fixes the combination of i.MX6 MAC and KSZ9131 PHY mostly because of that strange TXC clock skew the i.MX6 has. I agree that my patch might be evil. I also want to avoid a second method solving this problem when the solution I chose now already exists. If you going to fix that phy-fixups in mach-imx of i.MX6 I will implement the rgmii-txid delay in KSZ9131 driver for our boards. OK? > > > Philippe > > > Regards, > > > Oleksij > > > > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > > > > > --- > > > > > > > > arch/arm/mach-imx/mach-imx6q.c | 37 > > > > ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 37 insertions(+) > > > > > > > > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach- > > > > imx/mach-imx6q.c > > > > index edd26e0ffeec..8ae5f2fa33e2 100644 > > > > --- a/arch/arm/mach-imx/mach-imx6q.c > > > > +++ b/arch/arm/mach-imx/mach-imx6q.c > > > > @@ -61,6 +61,14 @@ static void mmd_write_reg(struct phy_device > > > > *dev, > > > > int device, int reg, int val) > > > > phy_write(dev, 0x0e, val); > > > > } > > > > > > > > +static int mmd_read_reg(struct phy_device *dev, int device, int > > > > reg) > > > > +{ > > > > + phy_write(dev, 0x0d, device); > > > > + phy_write(dev, 0x0e, reg); > > > > + phy_write(dev, 0x0d, (1 << 14) | device); > > > > + return phy_read(dev, 0x0e); > > > > +} > > > > + > > > > static int ksz9031rn_phy_fixup(struct phy_device *dev) > > > > { > > > > /* > > > > @@ -74,6 +82,33 @@ static int ksz9031rn_phy_fixup(struct > > > > phy_device > > > > *dev) > > > > return 0; > > > > } > > > > > > > > +#define KSZ9131_RXTXDLL_BYPASS 12 > > > > + > > > > +static int ksz9131rn_phy_fixup(struct phy_device *dev) > > > > +{ > > > > + int tmp; > > > > + > > > > + tmp = mmd_read_reg(dev, 2, 0x4c); > > > > + /* disable rxdll bypass (enable 2ns skew delay on RXC) > > > > */ > > > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > > > + mmd_write_reg(dev, 2, 0x4c, tmp); > > > > + > > > > + tmp = mmd_read_reg(dev, 2, 0x4d); > > > > + /* disable txdll bypass (enable 2ns skew delay on TXC) > > > > */ > > > > + tmp &= ~(1 << KSZ9131_RXTXDLL_BYPASS); > > > > + mmd_write_reg(dev, 2, 0x4d, tmp); > > > > + > > > > + /* > > > > + * Subtract ~0.6ns from txdll = ~1.4ns delay. > > > > + * leave RXC path untouched > > > > + */ > > > > + mmd_write_reg(dev, 2, 4, 0x007d); > > > > + mmd_write_reg(dev, 2, 6, 0xdddd); > > > > + mmd_write_reg(dev, 2, 8, 0x0007); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /* > > > > * fixup for PLX PEX8909 bridge to configure GPIO1-7 as output > > > > High > > > > * as they are used for slots1-7 PERST# > > > > @@ -167,6 +202,8 @@ static void __init imx6q_enet_phy_init(void) > > > > 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_KSZ9131, > > > > MICREL_PHY_ID_MASK, > > > > + ksz9131rn_phy_fixup); > > > > phy_register_fixup_for_uid(PHY_ID_AR8031, > > > > 0xffffffef, > > > > ar8031_phy_fixup); > > > > phy_register_fixup_for_uid(PHY_ID_AR8035, > > > > 0xffffffef, > > > > -- > > > > 2.25.1 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-03-06 16:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-05 13:49 [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup Philippe Schenker 2020-03-05 13:53 ` Russell King - ARM Linux admin 2020-03-06 9:57 ` Philippe Schenker 2020-03-06 10:52 ` Russell King - ARM Linux admin 2020-03-05 14:38 ` Oleksij Rempel 2020-03-05 16:51 ` Andrew Lunn 2020-03-06 7:42 ` Ahmad Fatoum 2020-03-06 9:46 ` Philippe Schenker 2020-03-06 11:14 ` Ahmad Fatoum 2020-03-06 12:16 ` Philippe Schenker 2020-03-06 13:38 ` Andrew Lunn 2020-03-06 16:30 ` Philippe Schenker 2020-03-06 9:55 ` Philippe Schenker 2020-03-06 10:38 ` Oleksij Rempel 2020-03-06 12:36 ` Philippe Schenker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).