Messages in this thread Patch in this message | | | Date | Fri, 6 Mar 2020 11:38:37 +0100 | From | Oleksij Rempel <> | Subject | Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup |
| |
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 | [unhandled content-type:application/pgp-signature] | |