| Message ID | 1425404579-30591-1-git-send-email-rockford@yandex.ru |
|---|---|
| State | New |
| Headers | show |
On Tuesday 03 March 2015 20:42:59 Andrey Panov wrote: > +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? > + 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
Hello! 04.03.2015, 02:29, "Arnd Bergmann" <arnd@arndb.de>: > On Tuesday 03 March 2015 20:42:59 Andrey Panov wrote: >> +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? Yes >> + 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. -- Андрей
On Wednesday 04 March 2015 22:08:42 Панов Андрей wrote: > > >> + 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. 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? > And I see, mask is wrong - I should use 0x001fffff. > > I'll resend a patch, thanks. Ok. Arnd
04.03.2015, 22:44, "Arnd Bergmann" <arnd@arndb.de>: > On Wednesday 04 March 2015 22:08:42 Панов Андрей wrote: >>>> + 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. > > 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. -- Андрей
On Wed, Mar 4, 2015 at 5:05 PM, Панов Андрей <rockford@yandex.ru> wrote: > 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?
04.03.2015, 23:18, "Fabio Estevam" <festevam@gmail.com>: > On Wed, Mar 4, 2015 at 5:05 PM, Панов Андрей <rockford@yandex.ru> wrote: >> 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 > 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. -- Андрей
On Wednesday 04 March 2015 23:32:38 Панов Андрей wrote: > 04.03.2015, 23:18, "Fabio Estevam" <festevam@gmail.com>: > > On Wed, Mar 4, 2015 at 5:05 PM, Панов Андрей <rockford@yandex.ru> wrote: > >> 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 > > > 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
05.03.2015, 00:32, "Arnd Bergmann" <arnd@arndb.de>: > On Wednesday 04 March 2015 23:32:38 Панов Андрей wrote: >> 04.03.2015, 23:18, "Fabio Estevam" <festevam@gmail.com>: >>> On Wed, Mar 4, 2015 at 5:05 PM, Панов Андрей <rockford@yandex.ru> wrote: >>>> 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 >>> 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. -- Андрей
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index 4ad6e47..41708f5 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -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); } }
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 <rockford@yandex.ru> --- arch/arm/mach-imx/mach-imx6q.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)