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