Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Mar 2020 13:53:46 +0000
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Philippe Schenker <philippe.schenker@toradex.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        NXP Linux Team <linux-imx@nxp.com>,
        Fabio Estevam <festevam@gmail.com>,
        Allison Randal <allison@lohutok.net>,
        linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Kate Stewart <kstewart@linuxfoundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>
Subject: Re: [PATCH] ARM: mach-imx6q: add ksz9131rn_phy_fixup
Message-ID: <20200305135346.GD25745@shell.armlinux.org.uk>
References: <20200305134928.19775-1-philippe.schenker@toradex.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200305134928.19775-1-philippe.schenker@toradex.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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