Skip to contentSkip to conversation

ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

PATCH New Andrey Panov 3 participants 9 messages Mar 3, 2015 – Mar 5, 2015

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape
lore
Share & tools
lore

Andrey Panov

author
#0 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

RTL8211E Ethernet PHY found in Embedsky E9 board is need an additional initialization.
Taken from vendor's binary-only module.

Signed-off-by Andrey Panov <[email protected]>
---
 arch/arm/mach-imx/mach-imx6q.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
arch/arm/mach-imx/mach-imx6q.c +14 -0
index 4ad6e47..41708f5 100644
@@ -166,6 +166,18 @@ static int ar8035_phy_fixup(struct phy_device *dev)
 
 #define PHY_ID_AR8035 0x004dd072
 
+static int rtl8211e_phy_fixup(struct phy_device *dev)
+{
+	phy_write(dev, 0x00, 0x3140);
+	mdelay(10);
+	phy_write(dev, 0x00, 0x3340);
+	mdelay(10);
+
+	return 0;
+}
+
+#define PHY_ID_RTL8211E 0x001cc915
+
 static void __init imx6q_enet_phy_init(void)
 {
 	if (IS_BUILTIN(CONFIG_PHYLIB)) {

@@ -177,6 +189,8 @@ static void __init imx6q_enet_phy_init(void)
 				ar8031_phy_fixup);
 		phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef,
 				ar8035_phy_fixup);
+		phy_register_fixup_for_uid(PHY_ID_RTL8211E, 0xffffffff,
+				rtl8211e_phy_fixup);
 	}
 }
Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

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

Quoted 10 lines · On Tuesday 03 March 2015 20:42:59 Andrey Panov wrote:
arch/arm/mach-imx/mach-imx6q.c +9 -0
+static int rtl8211e_phy_fixup(struct phy_device *dev)
+{
+       phy_write(dev, 0x00, 0x3140);
+       mdelay(10);
+       phy_write(dev, 0x00, 0x3340);
+       mdelay(10);
+
+       return 0;
+}

mdelay(10) is rather evil, can you use msleep(10) here instead?

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_enet_phy_init(void) · line 193

Quoted 2 lines
arch/arm/mach-imx/mach-imx6q.c +2 -0
+               phy_register_fixup_for_uid(PHY_ID_RTL8211E, 0xffffffff,
+                               rtl8211e_phy_fixup);

How do you ensure that this fixup is only applied on the boards that need it, rather than all machines that happen to have this phy?

Arnd

author
#2 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

Hello!

04.03.2015, 02:29, "Arnd Bergmann" <[email protected]>:

Quoted 11 lines · On Tuesday 03 March 2015 20:42:59 Andrey Panov wrote:
Quoted 9 lines

?+static int rtl8211e_phy_fixup(struct phy_device *dev) ?+{
?+ ??????phy_write(dev, 0x00, 0x3140);
?+ ??????mdelay(10);
?+ ??????phy_write(dev, 0x00, 0x3340);
?+ ??????mdelay(10);
?+
?+ ??????return 0;
?+}

Quoted 1 line

mdelay(10) is rather evil, can you use msleep(10) here instead?

Yes

Quoted 5 lines
Quoted 2 lines

?+ ??????????????phy_register_fixup_for_uid(PHY_ID_RTL8211E, 0xffffffff, ?+ ??????????????????????????????rtl8211e_phy_fixup);

How do you ensure that this fixup is only applied on the boards that need it, rather than all machines that happen to have this phy?

I've thought that if there no other fixups in code here checking a particular board where they run, this isn't necessary.
This SoC has only one ethernet MAC and if it is connected to this PHY,
it should be initalized this way independently of board, like other code here does, I think.

This code just does forcibly restart autonegotiation.
I can wrap it in if(!of_machine_is_compatible(...)){}, if it is need.

And I see, mask is wrong - I should use 0x001fffff.

I'll resend a patch, thanks.

--
??????

#3 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

Quoted 7 lines · On Wednesday 04 March 2015 22:08:42 ????? ?????? wrote:
Quoted 6 lines
Quoted 3 lines

+ phy_register_fixup_for_uid(PHY_ID_RTL8211E, 0xffffffff,
+ rtl8211e_phy_fixup);

How do you ensure that this fixup is only applied on the boards that need it, rather than all machines that happen to have this phy?

Quoted 7 lines

I've thought that if there no other fixups in code here checking a particular board where they run, this isn't necessary.
This SoC has only one ethernet MAC and if it is connected to this PHY,
it should be initalized this way independently of board, like other code here does, I think.

This code just does forcibly restart autonegotiation.
I can wrap it in if(!of_machine_is_compatible(...)){}, if it is need.

Your explanation makes sense to me, though I wonder what the property of this SoC is that requires the PHY fixup. Is this something we could or should be doing in a more general way using the PHY API, by having the device driver call a phy API function to restart autoneg independent of the PHY?

Quoted 3 lines

And I see, mask is wrong - I should use 0x001fffff.

I'll resend a patch, thanks.

Ok.

Arnd

author
#4 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

04.03.2015, 22:44, "Arnd Bergmann" <[email protected]>:

Quoted 10 lines · On Wednesday 04 March 2015 22:08:42 ????? ?????? wrote:
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

??+ ??????????????phy_register_fixup_for_uid(PHY_ID_RTL8211E, 0xffffffff, ??+ ??????????????????????????????rtl8211e_phy_fixup);

?How do you ensure that this fixup is only applied on the boards that need ?it, rather than all machines that happen to have this phy?

?I've thought that if there no other fixups in code here checking a particular ?board where they run, this isn't necessary.
?This SoC has only one ethernet MAC and if it is connected to this PHY,
?it should be initalized this way independently of board, like other code here does, I think.

Quoted 7 lines
Quoted 2 lines

?This code just does forcibly restart autonegotiation.
?I can wrap it in if(!of_machine_is_compatible(...)){}, if it is need.

Your explanation makes sense to me, though I wonder what the property of this SoC is that requires the PHY fixup. Is this something we could or should be doing in a more general way using the PHY API, by having the device driver call a phy API function to restart autoneg independent of the PHY?

Actually, I don't know. I've discovered this when I wanted to boot a more recent kernel on this board, and network wasn't working. So I've had to disassembly a vendor's binary only module to discover how they handle this situation.

--
??????

reviewer
#5 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

Quoted 3 lines · ????? ??????

Actually, I don't know. I've discovered this when I wanted to boot a more recent kernel on this board, and network wasn't working. So I've had to disassembly a vendor's binary only module to discover how they handle this situation.

Do you use the generic PHY driver or do you have CONFIG_REALTEK_PHY enabled?

In case you use the realtek PHY driver and still see the issue, can't you fix this in drivers/net/phy/realtek.c instead?

author
#6 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

04.03.2015, 23:18, "Fabio Estevam" <[email protected]>:

Quoted 6 lines · ????? ??????
Quoted 3 lines

?Actually, I don't know. I've discovered this when I wanted to boot a more recent kernel ?on this board, and network wasn't working. So I've had to disassembly a vendor's binary only module ?to discover how they handle this situation.

Do you use the generic PHY driver or do you have CONFIG_REALTEK_PHY enabled?

CONFIG_REALTEK_PHY=y
CONFIG_FIXED_PHY=y

Quoted 2 lines

In case you use the realtek PHY driver and still see the issue, can't you fix this in drivers/net/phy/realtek.c instead?

I think there it will require a more work with board detection than here and will populate a driver with unnecessary for most users code.

--
??????

#7 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

Quoted 8 lines · On Wednesday 04 March 2015 23:32:38 ????? ?????? wrote:

04.03.2015, 23:18, "Fabio Estevam" <[email protected]>:

Quoted 6 lines · ????? ??????
Quoted 3 lines

Actually, I don't know. I've discovered this when I wanted to boot a more recent kernel on this board, and network wasn't working. So I've had to disassembly a vendor's binary only module to discover how they handle this situation.

Do you use the generic PHY driver or do you have CONFIG_REALTEK_PHY enabled?

Quoted 8 lines

CONFIG_REALTEK_PHY=y
CONFIG_FIXED_PHY=y

Quoted 2 lines

In case you use the realtek PHY driver and still see the issue, can't you fix this in drivers/net/phy/realtek.c instead?

I think there it will require a more work with board detection than here and will populate a driver with unnecessary for most users code.

But you said earlier that it's board independent and a property of the SoC instead. Can't the ethernet driver call phy_start_aneg() for the same effect, like a lot of other ethernet drivers do?

Arnd

author
#8 lore raw

PATCH 1/1 ARM: i.MX6Q: Add fixup for RTL8211E Gigabit Ethernet PHY

05.03.2015, 00:32, "Arnd Bergmann" <[email protected]>:

Quoted 9 lines · On Wednesday 04 March 2015 23:32:38 ????? ?????? wrote:
Quoted 8 lines

?04.03.2015, 23:18, "Fabio Estevam" <[email protected]>:

Quoted 5 lines
Quoted 3 lines · ????? ??????

??Actually, I don't know. I've discovered this when I wanted to boot a more recent kernel ??on this board, and network wasn't working. So I've had to disassembly a vendor's binary only module ??to discover how they handle this situation.

?Do you use the generic PHY driver or do you have CONFIG_REALTEK_PHY enabled?

?CONFIG_REALTEK_PHY=y
?CONFIG_FIXED_PHY=y

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

?In case you use the realtek PHY driver and still see the issue, can't ?you fix this in drivers/net/phy/realtek.c instead?

?I think there it will require a more work with board detection than here and will ?populate a driver with unnecessary for most users code.

But you said earlier that it's board independent and a property of the SoC instead. Can't the ethernet driver call phy_start_aneg() for the same effect, like a lot of other ethernet drivers do?

Of course. I'll try to make it this way.

--
??????