Patchwork ARM: i.MX6: update KSZ9031 phy fixup

login
register
mail settings
Submitter Hubert Chaumette
Date March 21, 2014, 5:08 p.m.
Message ID <1395421687-12934-1-git-send-email-hchaumette@adeneo-embedded.com>
Download mbox | patch
Permalink /patch/332685/
State New
Headers show

Comments

Hubert Chaumette - March 21, 2014, 5:08 p.m.
Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

Signed-off-by: Hubert Chaumette <hchaumette@adeneo-embedded.com>
---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
Eric Benard - March 21, 2014, 9:23 p.m.
Hi Hubert,

Le Fri, 21 Mar 2014 18:08:07 +0100,
Hubert Chaumette <hchaumette@adeneo-embedded.com> a écrit :

> 
> Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
> set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.
> 
> Signed-off-by: Hubert Chaumette <hchaumette@adeneo-embedded.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 76e5db4..db307c2 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -77,6 +77,9 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev)
>  	mmd_write_reg(dev, 2, 5, 0);
>  	mmd_write_reg(dev, 2, 8, 0x003ff);
>  
> +	/* For Congatec conga-QMX6 board */
> +	mmd_write_reg(dev, 0x02, 0x06, 0xffff);
> +
>  	return 0;
>  }
>  
that's board specific (the needed delay depends on the routing delay on
the PCB), so this should not go in a generic file.

Eric
Hubert Chaumette - March 24, 2014, 3:30 p.m.
Hi Eric,

----- Original Message -----
> From: "Eric Bénard" <eric@eukrea.com>
> To: "Hubert Chaumette" <hchaumette@adeneo-embedded.com>
> Cc: linux-arm-kernel@lists.infradead.org, linux@arm.linux.org.uk, "shawn guo" <shawn.guo@linaro.org>,
> linux-kernel@vger.kernel.org, kernel@pengutronix.de
> Sent: Friday, March 21, 2014 10:23:20 PM
> Subject: Re: [PATCH] ARM: i.MX6: update KSZ9031 phy fixup
> 
> Hi Hubert,
> 
> Le Fri, 21 Mar 2014 18:08:07 +0100,
> Hubert Chaumette <hchaumette@adeneo-embedded.com> a écrit :
> 
> > 
> > Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
> > set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.
> > 
> > Signed-off-by: Hubert Chaumette <hchaumette@adeneo-embedded.com>
> > ---
> >  arch/arm/mach-imx/mach-imx6q.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c
> > b/arch/arm/mach-imx/mach-imx6q.c
> > index 76e5db4..db307c2 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -77,6 +77,9 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev)
> >          mmd_write_reg(dev, 2, 5, 0);
> >          mmd_write_reg(dev, 2, 8, 0x003ff);
> >  
> > +        /* For Congatec conga-QMX6 board */
> > +        mmd_write_reg(dev, 0x02, 0x06, 0xffff);
> > +
> >          return 0;
> >  }
> >  
> that's board specific (the needed delay depends on the routing delay on
> the PCB), so this should not go in a generic file.
> 
> Eric
> 

I admit it may need to be in a separate function, but this file already contains
board-specific fixups (for imx6q sabrelite, Data Modul eDM-QMX6).

Anyway, do you have any suggestion for the location I should put it ?

Regards,

Hubert
Eric Benard - March 24, 2014, 3:41 p.m.
Hi Hubert,

Le Mon, 24 Mar 2014 16:30:49 +0100 (CET),
CHAUMETTE Hubert <hchaumette@adeneo-embedded.com> a écrit :
> > From: "Eric Bénard" <eric@eukrea.com>
> > that's board specific (the needed delay depends on the routing delay on
> > the PCB), so this should not go in a generic file.
> > 
> > Eric
> > 
> 
> I admit it may need to be in a separate function, but this file already contains
> board-specific fixups (for imx6q sabrelite, Data Modul eDM-QMX6).
> 
true but that's not a reason to add more ;-)

> Anyway, do you have any suggestion for the location I should put it ?
> 
no idea at the moment, I simply used your patch to bring this issue on
the ML as we recently needed to patch the default values for testing
mainline kernel on a custom boards.

Eric
Shawn Guo - April 2, 2014, 1:14 p.m.
On Mon, Mar 24, 2014 at 04:41:41PM +0100, Eric Bénard wrote:
> Hi Hubert,
> 
> Le Mon, 24 Mar 2014 16:30:49 +0100 (CET),
> CHAUMETTE Hubert <hchaumette@adeneo-embedded.com> a écrit :
> > > From: "Eric Bénard" <eric@eukrea.com>
> > > that's board specific (the needed delay depends on the routing delay on
> > > the PCB), so this should not go in a generic file.
> > > 
> > > Eric
> > > 
> > 
> > I admit it may need to be in a separate function, but this file already contains
> > board-specific fixups (for imx6q sabrelite, Data Modul eDM-QMX6).
> > 
> true but that's not a reason to add more ;-)
> 
> > Anyway, do you have any suggestion for the location I should put it ?
> > 
> no idea at the moment, I simply used your patch to bring this issue on
> the ML as we recently needed to patch the default values for testing
> mainline kernel on a custom boards.

Hmm, can such board-specific fixups be pushed down to bootloader?

Shawn
Eric Benard - April 2, 2014, 1:38 p.m.
Hi Shawn,

Le Wed, 2 Apr 2014 21:14:08 +0800,
Shawn Guo <shawn.guo@linaro.org> a écrit :

> 
> On Mon, Mar 24, 2014 at 04:41:41PM +0100, Eric Bénard wrote:
> > Hi Hubert,
> > 
> > Le Mon, 24 Mar 2014 16:30:49 +0100 (CET),
> > CHAUMETTE Hubert <hchaumette@adeneo-embedded.com> a écrit :
> > > > From: "Eric Bénard" <eric@eukrea.com>
> > > > that's board specific (the needed delay depends on the routing delay on
> > > > the PCB), so this should not go in a generic file.
> > > > 
> > > > Eric
> > > > 
> > > 
> > > I admit it may need to be in a separate function, but this file already contains
> > > board-specific fixups (for imx6q sabrelite, Data Modul eDM-QMX6).
> > > 
> > true but that's not a reason to add more ;-)
> > 
> > > Anyway, do you have any suggestion for the location I should put it ?
> > > 
> > no idea at the moment, I simply used your patch to bring this issue on
> > the ML as we recently needed to patch the default values for testing
> > mainline kernel on a custom boards.
> 
> Hmm, can such board-specific fixups be pushed down to bootloader?
> 
that won't work if the kernel reset the PHY.

Eric
Ben Dooks - April 2, 2014, 2:14 p.m.
On 02/04/14 14:38, Eric Bénard wrote:
> Hi Shawn,
>
> Le Wed, 2 Apr 2014 21:14:08 +0800,
> Shawn Guo <shawn.guo@linaro.org> a écrit :
>
>>
>> On Mon, Mar 24, 2014 at 04:41:41PM +0100, Eric Bénard wrote:
>>> Hi Hubert,
>>>
>>> Le Mon, 24 Mar 2014 16:30:49 +0100 (CET),
>>> CHAUMETTE Hubert <hchaumette@adeneo-embedded.com> a écrit :
>>>>> From: "Eric Bénard" <eric@eukrea.com>
>>>>> that's board specific (the needed delay depends on the routing delay on
>>>>> the PCB), so this should not go in a generic file.
>>>>>
>>>>> Eric
>>>>>
>>>>
>>>> I admit it may need to be in a separate function, but this file already contains
>>>> board-specific fixups (for imx6q sabrelite, Data Modul eDM-QMX6).
>>>>
>>> true but that's not a reason to add more ;-)
>>>
>>>> Anyway, do you have any suggestion for the location I should put it ?
>>>>
>>> no idea at the moment, I simply used your patch to bring this issue on
>>> the ML as we recently needed to patch the default values for testing
>>> mainline kernel on a custom boards.
>>
>> Hmm, can such board-specific fixups be pushed down to bootloader?
>>
> that won't work if the kernel reset the PHY.

If it is in device-tree, then you can add it to the phy node.

For earlier Micrel Phys we have a LED mode setting on initialisaion.
Anatolij Gustschin - April 2, 2014, 8:01 p.m.
On Fri, 21 Mar 2014 18:08:07 +0100
Hubert Chaumette <hchaumette@adeneo-embedded.com> wrote:

> Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
> set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.
> 
> Signed-off-by: Hubert Chaumette <hchaumette@adeneo-embedded.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 76e5db4..db307c2 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -77,6 +77,9 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev)
>  	mmd_write_reg(dev, 2, 5, 0);
>  	mmd_write_reg(dev, 2, 8, 0x003ff);
>  
> +	/* For Congatec conga-QMX6 board */
> +	mmd_write_reg(dev, 0x02, 0x06, 0xffff);

The patch sets TX Data Pad Skew TXD0-TXD3 but the commit message states
that it sets GTX/RX CLK pad skew. The GTX/RX CLK pad skew is already
set to +0.96ns by writing 0x003ff to the register 8.

It would be better to configure the pad skews in the board specific
way in the device tree. There is a binding for ksz9021 PHY in
Documentation/devicetree/bindings/net/micrel-ksz9021.txt.
I have a patch for setting ksz9031rn GTX/RX CLK pad skew in a similar
way over device tree and plan to submit it when the net-next merge
window for v3.16 opens.
Hubert Chaumette - April 3, 2014, 2:05 p.m.
Le mercredi 02 avril 2014 à 22:01 +0200, Anatolij Gustschin a écrit :
> On Fri, 21 Mar 2014 18:08:07 +0100
> Hubert Chaumette <hchaumette@adeneo-embedded.com> wrote:
> 
> > Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
> > set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.
> > 
> > Signed-off-by: Hubert Chaumette <hchaumette@adeneo-embedded.com>
> > ---
> >  arch/arm/mach-imx/mach-imx6q.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> > index 76e5db4..db307c2 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -77,6 +77,9 @@ static int ksz9031rn_phy_fixup(struct phy_device *dev)
> >  	mmd_write_reg(dev, 2, 5, 0);
> >  	mmd_write_reg(dev, 2, 8, 0x003ff);
> >  
> > +	/* For Congatec conga-QMX6 board */
> > +	mmd_write_reg(dev, 0x02, 0x06, 0xffff);
> 
> The patch sets TX Data Pad Skew TXD0-TXD3 but the commit message states
> that it sets GTX/RX CLK pad skew. The GTX/RX CLK pad skew is already
> set to +0.96ns by writing 0x003ff to the register 8.

Indeed, the message should be "set RGMII TXD0 to TXD3 output pad skew to
+0.48ns".

> It would be better to configure the pad skews in the board specific
> way in the device tree. There is a binding for ksz9021 PHY in
> Documentation/devicetree/bindings/net/micrel-ksz9021.txt.

I wonder why it it's not used in arch/arm/boot/dts/imx6q-sabrelite.dts
instead of ksz9021rn_phy_fixup().

> I have a patch for setting ksz9031rn GTX/RX CLK pad skew in a similar
> way over device tree and plan to submit it when the net-next merge
> window for v3.16 opens.

So, you have already implemented the ksz9031 binding ?
Shawn Guo - April 6, 2014, 11:06 a.m.
On Thu, Apr 03, 2014 at 04:05:47PM +0200, Hubert Chaumette wrote:
> Le mercredi 02 avril 2014 à 22:01 +0200, Anatolij Gustschin a écrit :
> > It would be better to configure the pad skews in the board specific
> > way in the device tree. There is a binding for ksz9021 PHY in
> > Documentation/devicetree/bindings/net/micrel-ksz9021.txt.
> 
> I wonder why it it's not used in arch/arm/boot/dts/imx6q-sabrelite.dts
> instead of ksz9021rn_phy_fixup().

Oh, if you look at arch/arm/boot/dts/imx6qdl-sabrelite.dtsi on mainline
tree today, you will find it.

Shawn
Russell King - ARM Linux - April 6, 2014, 11:26 a.m.
On Sun, Apr 06, 2014 at 07:06:38PM +0800, Shawn Guo wrote:
> On Thu, Apr 03, 2014 at 04:05:47PM +0200, Hubert Chaumette wrote:
> > Le mercredi 02 avril 2014 à 22:01 +0200, Anatolij Gustschin a écrit :
> > > It would be better to configure the pad skews in the board specific
> > > way in the device tree. There is a binding for ksz9021 PHY in
> > > Documentation/devicetree/bindings/net/micrel-ksz9021.txt.
> > 
> > I wonder why it it's not used in arch/arm/boot/dts/imx6q-sabrelite.dts
> > instead of ksz9021rn_phy_fixup().
> 
> Oh, if you look at arch/arm/boot/dts/imx6qdl-sabrelite.dtsi on mainline
> tree today, you will find it.

Ergh.  So much for "DT describes the hardware, not the software
implementation."

commit 954c396756e3d31985f7bc6a414a988a4736a7d0
Author: Sean Cross <xobs@kosagi.com>
Date:   Wed Aug 21 01:46:12 2013 +0000

    net/phy: micrel: Add OF configuration support for ksz9021

    Some boards require custom PHY configuration, for example due to trace
    length differences.  Add the ability to configure these registers in
    order to get the PHY to function on boards that need it.

    Because PHYs are auto-detected based on MDIO device IDs, allow PHY
    configuration to be specified in the parent Ethernet device node if no
    PHY device node is present.

If we were describing the hardware, we'd create a node for the phy and put
the phy specific properties in there, and reference it from the ethernet
driver, and have some way to look up the phy node when we automatically
discover the phy.

Well, it's too late to do anything else now, we're stuck with this, and
I guess everyone's going to be stuffing the phy chip's configuration into
the ethernet device's node from now on. :(
Shawn Guo - April 6, 2014, 12:04 p.m.
On Sun, Apr 06, 2014 at 12:26:55PM +0100, Russell King - ARM Linux wrote:
> Ergh.  So much for "DT describes the hardware, not the software
> implementation."
> 
> commit 954c396756e3d31985f7bc6a414a988a4736a7d0
> Author: Sean Cross <xobs@kosagi.com>
> Date:   Wed Aug 21 01:46:12 2013 +0000
> 
>     net/phy: micrel: Add OF configuration support for ksz9021
> 
>     Some boards require custom PHY configuration, for example due to trace
>     length differences.  Add the ability to configure these registers in
>     order to get the PHY to function on boards that need it.
> 
>     Because PHYs are auto-detected based on MDIO device IDs, allow PHY
>     configuration to be specified in the parent Ethernet device node if no
>     PHY device node is present.
> 
> If we were describing the hardware, we'd create a node for the phy and put
> the phy specific properties in there, and reference it from the ethernet
> driver, and have some way to look up the phy node when we automatically
> discover the phy.

This is exactly how new style devices work, like allwinner case below.

  arch/arm/boot/dts/sun4i-a10-a1000.dts
  drivers/net/ethernet/allwinner/sun4i-emac.c

> 
> Well, it's too late to do anything else now, we're stuck with this, and
> I guess everyone's going to be stuffing the phy chip's configuration into
> the ethernet device's node from now on. :(

Let's take a look at ksz9021_config_init().

	if (!of_node && dev->parent->of_node)
		of_node = dev->parent->of_node;

It's a stepping back in case that the phy device has no of_node
attached.  So for phy devices that are initiated from device tree like
allwinner case above, they will just work in the sane way.

Shawn

Patch

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 76e5db4..db307c2 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -77,6 +77,9 @@  static int ksz9031rn_phy_fixup(struct phy_device *dev)
 	mmd_write_reg(dev, 2, 5, 0);
 	mmd_write_reg(dev, 2, 8, 0x003ff);
 
+	/* For Congatec conga-QMX6 board */
+	mmd_write_reg(dev, 0x02, 0x06, 0xffff);
+
 	return 0;
 }