[PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Russell King - ARM Linux admin]

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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Oleksij Rempel]

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 |

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Andrew Lunn]

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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Ahmad Fatoum]

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 |

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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
> > 
> > 
> > 

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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
> > 
> > 

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Oleksij Rempel]

[-- 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 --]

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Russell King - ARM Linux admin]

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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Ahmad Fatoum]

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 |

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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
> > 

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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
> > > > 
> > > > 
> > > > 

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Andrew Lunn]

> > 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

Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup

[Philippe Schenker]

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