Skip to contentSkip to conversation

ARM: i.MX6: update KSZ9031 phy fixup

PATCH In review Hubert Chaumette 6 participants 13 messages Mar 21, 2014 – Apr 6, 2014

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +3 -0
index 76e5db4..db307c2 100644
@@ -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;
 }
 
Signature · 1 line
1.9.0
#1 lore raw

Hi Hubert,

Le Fri, 21 Mar 2014 18:08:07 +0100,
Hubert Chaumette <[email protected]> a ?crit :

Quoted 14 lines

Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +0 -0
index 76e5db4..db307c2 100644

arch/arm/mach-imx/mach-imx6q.c static int ksz9031rn_phy_fixup(struct phy_device *dev) · line 85

Quoted 9 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
@@ -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

#2 lore raw

Hi Eric,

----- Original Message -----

Quoted 39 lines

From: "Eric B?nard" <[email protected]>
To: "Hubert Chaumette" <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org, linux at arm.linux.org.uk, "shawn guo" <[email protected]>, linux-kernel at vger.kernel.org, kernel at 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 <[email protected]> a ?crit :

Quoted 15 lines

Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +0 -0
index 76e5db4..db307c2 100644

arch/arm/mach-imx/mach-imx6q.c static int ksz9031rn_phy_fixup(struct phy_device *dev) · line 85

Quoted 9 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
@@ -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.

Quoted 2 lines

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

#3 lore raw

Hi Hubert,

Le Mon, 24 Mar 2014 16:30:49 +0100 (CET), CHAUMETTE Hubert <[email protected]> a ?crit :

Quoted 10 lines
Quoted 6 lines

From: "Eric B?nard" <[email protected]>
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 ;-)

Quoted 2 lines

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

#4 lore raw
Quoted 14 lines · Eric B?nard

Hi Hubert,

Le Mon, 24 Mar 2014 16:30:49 +0100 (CET), CHAUMETTE Hubert <[email protected]> a ?crit :

Quoted 10 lines
Quoted 6 lines

From: "Eric B?nard" <[email protected]>
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).

Quoted 7 lines

true but that's not a reason to add more ;-)

Quoted 2 lines

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

#5 lore raw

Hi Shawn,

Le Wed, 2 Apr 2014 21:14:08 +0800,
Shawn Guo <[email protected]> a ?crit :

Quoted 18 lines · Eric B?nard
Quoted 16 lines

Hi Hubert,

Le Mon, 24 Mar 2014 16:30:49 +0100 (CET), CHAUMETTE Hubert <[email protected]> a ?crit :

Quoted 10 lines
Quoted 6 lines

From: "Eric B?nard" <[email protected]>
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 ;-)

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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

#6 lore raw
Quoted 25 lines · Eric B?nard

Hi Shawn,

Le Wed, 2 Apr 2014 21:14:08 +0800,
Shawn Guo <[email protected]> a ?crit :

Quoted 20 lines · Eric B?nard
Quoted 18 lines

Hi Hubert,

Le Mon, 24 Mar 2014 16:30:49 +0100 (CET), CHAUMETTE Hubert <[email protected]> a ?crit :

Quoted 10 lines
Quoted 6 lines

From: "Eric B?nard" <[email protected]>
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 ;-)

Quoted 2 lines

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

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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.

Signature · 2 lines
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius
#7 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

In reply to Hubert Chaumette
Quoted 13 lines · On Fri, 21 Mar 2014 18:08:07 +0100 Hubert Chaumette…

Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +0 -0
index 76e5db4..db307c2 100644

arch/arm/mach-imx/mach-imx6q.c static int ksz9031rn_phy_fixup(struct phy_device *dev) · line 81

Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +2 -0
@@ -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.

#8 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

Le mercredi 02 avril 2014 ? 22:01 +0200, Anatolij Gustschin a ?crit :

Quoted 19 lines
Quoted 16 lines · On Fri, 21 Mar 2014 18:08:07 +0100 Hubert Chaumette…

Update KSZ9031RN phy fixup for Congatec conga-QEVAL and conga-QMX6 combo :
set RGMII GTX_CLK and RX_CLK pad skew to +0.96ns.

---
 arch/arm/mach-imx/mach-imx6q.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +0 -0
index 76e5db4..db307c2 100644
@@ -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);
 
Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c static int ksz9031rn_phy_fixup(struct phy_device *dev) · line 81

Quoted 2 lines
arch/arm/mach-imx/mach-imx6q.c +2 -0
+	/* 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".

Quoted 3 lines

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().

Quoted 3 lines

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 ?

#9 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

On Thu, 03 Apr 2014 16:05:47 +0200
Hubert Chaumette <[email protected]> wrote:
...

Quoted 6 lines
Quoted 3 lines

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().

Probably because nobody cared to use it.

Quoted 5 lines
Quoted 3 lines

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 ?

Partially (for GTX/RX CLK). I'm attaching a patch that I currently have. For ksz9031rn in my DTS fec node I used:

	/* ksz9031rn: min 0, max 1860, step 60 */
        rxc-skew-ps = <1620>;
        txc-skew-ps = <1620>;

Unfortunately I'm short on time and won't be able to implement other pad skew settings in the near future, so please fill free to extend and push the patch.

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ARM-imx6-configure-ksz90x1rn-PHY-pad-skew-settings-o.patch
Type: text/x-patch
Size: 6002 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140403/f898a40d/attachment.bin>

#10 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

In reply to Hubert Chaumette
Quoted 7 lines · Hubert Chaumette

Le mercredi 02 avril 2014 ? 22:01 +0200, Anatolij Gustschin a ?crit :

Quoted 3 lines

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

#11 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

Quoted 11 lines · Shawn Guo
Quoted 7 lines · Hubert Chaumette

Le mercredi 02 avril 2014 ? 22:01 +0200, Anatolij Gustschin a ?crit :

Quoted 3 lines

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 <[email protected]>
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. :(

Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
#12 lore raw

PATCH 1/1 ARM: i.MX6: update KSZ9031 phy fixup

Quoted 13 lines · Russell King - ARM Linux

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

commit 954c396756e3d31985f7bc6a414a988a4736a7d0

Author: Sean Cross <[email protected]>
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.

Quoted 8 lines

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

Quoted 4 lines

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