| Message ID | 20210208140341.9271-2-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | New |
| Headers | show |
| Series | net: stmmac: Obvious cleanups and several fixes | expand |
On 09.02.2021 11:15, Serge Semin wrote: > On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote: >> On 08.02.2021 15:03, Serge Semin wrote: >>> It has been noticed that RTL8211E PHY stops detecting and reporting events >>> when EEE is successfully advertised and RXC stopping in LPI is enabled. >>> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable" >>> register) is set. At the same time LED2 stops blinking as if EEE mode has >>> been disabled. Notably the network traffic still flows through the PHY >>> with no obvious problem. Anyway if any MDIO read procedure is performed >>> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2 >>> starts blinking and PHY interrupts happens again. The problem has been >>> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and >>> reporting its event via a dedicated IRQ signal. (Obviously the problem has >>> been unnoticed in the polling mode, since it gets naturally fixed by the >>> periodic MDIO read procedure from the PHY status register - BMSR.) >>> >>> In order to fix that problem we suggest to locally re-implement the MMD >>> write method for RTL8211E PHY and perform a dummy read right after the >>> PC1R register is accessed to enable the RXC stopping in LPI mode. >>> >>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> >>> --- >>> drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 37 insertions(+) >>> >>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c >>> index 99ecd6c4c15a..cbb86c257aae 100644 >>> --- a/drivers/net/phy/realtek.c >>> +++ b/drivers/net/phy/realtek.c >>> @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum, >>> return ret; >>> } >>> >>> +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum, >>> + u16 val) >>> +{ >>> + int ret; >>> + >>> + /* Write to the MMD registers by using the standard control/data pair. >>> + * The only difference is that we need to perform a dummy read after >>> + * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue >>> + * of a partial core freeze so LED2 stops blinking in EEE mode, PHY >>> + * stops detecting the link change and raising IRQs until any read from >>> + * its registers performed. That happens only if and right after the PHY >>> + * is enabled to stop RXC in LPI mode. >>> + */ >>> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum); >>> + if (ret) >>> + return ret; >>> + >>> + ret = __phy_write(phydev, MII_MMD_DATA, regnum); >>> + if (ret) >>> + return ret; >>> + >>> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR); >>> + if (ret) >>> + return ret; >>> + >> > >> Nice analysis. Alternatively to duplicating this code piece we could >> export mmd_phy_indirect(). But up to you. > > I also considered creating a generic method to access the MMD > registers of a generic PHY, something like phy_read()/phy_write(), but > for MMD (alas just exporting mmd_phy_indirect() would not be enough). > But as I see it such methods need to be created only after we get to > have at least several places with duplicating direct MMD-read/write > patterns. Doing that just for a single place seems redundant. Anyway it's > up to maintainers to decide whether they want to see a generic part > of the phy_read_mmd()/phy_write_mmd() methods being detached and > exported as something like genphy_{read,write}_mmd() methods. I can do > that in v2 if you ask me to. > Right, adding something like a genphy_{read,write}_mmd() doesn't make too much sense for now. What I meant is just exporting mmd_phy_indirect(). Then you don't have to open-code the first three steps of a mmd read/write. And it requires no additional code in phylib. But that's not at all a showstopper here. > -Sergey > >> >>> + ret = __phy_write(phydev, MII_MMD_DATA, val); >>> + if (ret) >>> + return ret; >>> + >>> + if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 && >>> + val & MDIO_PCS_CTRL1_CLKSTOP_EN) >>> + ret = __phy_read(phydev, MII_MMD_DATA); >>> + >>> + return ret < 0 ? ret : 0; >>> +} >>> + >>> static int rtl822x_get_features(struct phy_device *phydev) >>> { >>> int val; >>> @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = { >>> .resume = genphy_resume, >>> .read_page = rtl821x_read_page, >>> .write_page = rtl821x_write_page, >>> + .write_mmd = rtl8211e_write_mmd, >>> }, { >>> PHY_ID_MATCH_EXACT(0x001cc916), >>> .name = "RTL8211F Gigabit Ethernet", >>> >>
On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: > Right, adding something like a genphy_{read,write}_mmd() doesn't make > too much sense for now. What I meant is just exporting mmd_phy_indirect(). > Then you don't have to open-code the first three steps of a mmd read/write. > And it requires no additional code in phylib. ... but at the cost that the compiler can no longer inline that code, as I mentioned in my previous reply. (However, the cost of the accesses will be higher.) On the plus side, less I-cache footprint, and smaller kernel code.
On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote: > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: > > Right, adding something like a genphy_{read,write}_mmd() doesn't make > > too much sense for now. What I meant is just exporting mmd_phy_indirect(). > > Then you don't have to open-code the first three steps of a mmd read/write. > > And it requires no additional code in phylib. > > ... but at the cost that the compiler can no longer inline that code, > as I mentioned in my previous reply. (However, the cost of the accesses > will be higher.) On the plus side, less I-cache footprint, and smaller > kernel code. Just to note mmd_phy_indirect() isn't defined with inline specifier, but just as static and it's used twice in the drivers/net/phy/phy-core.c unit. So most likely the compiler won't inline the function code in there. Anyway it's up to the PHY library maintainers to decide. Please settle the issue with Heiner and Andrew then. I am ok with both solutions and will do as you decide. -Sergey > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote: > On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote: > > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: > > > Right, adding something like a genphy_{read,write}_mmd() doesn't make > > > too much sense for now. What I meant is just exporting mmd_phy_indirect(). > > > Then you don't have to open-code the first three steps of a mmd read/write. > > > And it requires no additional code in phylib. > > > > ... but at the cost that the compiler can no longer inline that code, > > as I mentioned in my previous reply. (However, the cost of the accesses > > will be higher.) On the plus side, less I-cache footprint, and smaller > > kernel code. > > Just to note mmd_phy_indirect() isn't defined with inline specifier, > but just as static and it's used twice in the > drivers/net/phy/phy-core.c unit. So most likely the compiler won't > inline the function code in there. You can't always tell whether the compiler will inline a static function or not. > Anyway it's up to the PHY > library maintainers to decide. Please settle the issue with Heiner and > Andrew then. I am ok with both solutions and will do as you decide. FYI, *I* am one of the phylib maintainers. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote: > On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote: > > On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote: > > > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: > > > > Right, adding something like a genphy_{read,write}_mmd() doesn't make > > > > too much sense for now. What I meant is just exporting mmd_phy_indirect(). > > > > Then you don't have to open-code the first three steps of a mmd read/write. > > > > And it requires no additional code in phylib. > > > > > > ... but at the cost that the compiler can no longer inline that code, > > > as I mentioned in my previous reply. (However, the cost of the accesses > > > will be higher.) On the plus side, less I-cache footprint, and smaller > > > kernel code. > > > > Just to note mmd_phy_indirect() isn't defined with inline specifier, > > but just as static and it's used twice in the > > drivers/net/phy/phy-core.c unit. So most likely the compiler won't > > inline the function code in there. > > You can't always tell whether the compiler will inline a static function > or not. > > > Anyway it's up to the PHY > > library maintainers to decide. Please settle the issue with Heiner and > > Andrew then. I am ok with both solutions and will do as you decide. > > FYI, *I* am one of the phylib maintainers. Of course I saw you in the list of maintainers. My message was that currently two maintainers claims contradicting requests. Thus in order to go further with this patch first you need to get to some agreement between yourself. That's why we need to have a response from Hainer about your arguments against his suggestion. -Sergey > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote: > On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote: > > On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote: > > > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: > > > > Right, adding something like a genphy_{read,write}_mmd() doesn't make > > > > too much sense for now. What I meant is just exporting mmd_phy_indirect(). > > > > Then you don't have to open-code the first three steps of a mmd read/write. > > > > And it requires no additional code in phylib. > > > > > > ... but at the cost that the compiler can no longer inline that code, > > > as I mentioned in my previous reply. (However, the cost of the accesses > > > will be higher.) On the plus side, less I-cache footprint, and smaller > > > kernel code. > > > > Just to note mmd_phy_indirect() isn't defined with inline specifier, > > but just as static and it's used twice in the > > drivers/net/phy/phy-core.c unit. So most likely the compiler won't > > inline the function code in there. > > You can't always tell whether the compiler will inline a static function > or not. Andrew, Heiner, Russell, what is your final decision about this? Shall we export the mmd_phy_indirect() method, implement new genphy_{read,write}_mmd() or just leave the patch as is manually accessing the MMD register in the driver? -Sergey > > > Anyway it's up to the PHY > > library maintainers to decide. Please settle the issue with Heiner and > > Andrew then. I am ok with both solutions and will do as you decide. > > FYI, *I* am one of the phylib maintainers. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 20.02.2021 10:02, Serge Semin wrote: > On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote: >> On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote: >>> On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote: >>>> On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote: >>>>> Right, adding something like a genphy_{read,write}_mmd() doesn't make >>>>> too much sense for now. What I meant is just exporting mmd_phy_indirect(). >>>>> Then you don't have to open-code the first three steps of a mmd read/write. >>>>> And it requires no additional code in phylib. >>>> >>>> ... but at the cost that the compiler can no longer inline that code, >>>> as I mentioned in my previous reply. (However, the cost of the accesses >>>> will be higher.) On the plus side, less I-cache footprint, and smaller >>>> kernel code. >>> >>> Just to note mmd_phy_indirect() isn't defined with inline specifier, >>> but just as static and it's used twice in the >>> drivers/net/phy/phy-core.c unit. So most likely the compiler won't >>> inline the function code in there. >> >> You can't always tell whether the compiler will inline a static function >> or not. > > Andrew, Heiner, Russell, what is your final decision about this? Shall > we export the mmd_phy_indirect() method, implement new > genphy_{read,write}_mmd() or just leave the patch as is manually > accessing the MMD register in the driver? > If in doubt, leaving the patch as is would be fine with me. > -Sergey > >> >>> Anyway it's up to the PHY >>> library maintainers to decide. Please settle the issue with Heiner and >>> Andrew then. I am ok with both solutions and will do as you decide. >> >> FYI, *I* am one of the phylib maintainers. >> >> -- >> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ >> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> If in doubt, leaving the patch as is would be fine with me.
The patch is O.K. as is, no need to export something so simple for a
single users. When the next user come along, we can reconsider.
Andrew
On Sat, Feb 20, 2021 at 04:49:22PM +0100, Andrew Lunn wrote: > > If in doubt, leaving the patch as is would be fine with me. > > The patch is O.K. as is, no need to export something so simple for a > single users. When the next user come along, we can reconsider. Ok. Thanks for clarification. I performed some additional tests to make sure the bug was on the PHY side. They proved my original conclusion. It's indeed Realtek PHY to blame for the weird behavior. So I've added a few more words into the patch log regarding those tests. The patch will be resent tomorrow together with the rest of the STMMAC-driver-related bug-fixes detached from the original series of the fixes and cleanups (as Andrew asked to do). -Sergey > > Andrew
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 99ecd6c4c15a..cbb86c257aae 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum, return ret; } +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum, + u16 val) +{ + int ret; + + /* Write to the MMD registers by using the standard control/data pair. + * The only difference is that we need to perform a dummy read after + * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue + * of a partial core freeze so LED2 stops blinking in EEE mode, PHY + * stops detecting the link change and raising IRQs until any read from + * its registers performed. That happens only if and right after the PHY + * is enabled to stop RXC in LPI mode. + */ + ret = __phy_write(phydev, MII_MMD_CTRL, devnum); + if (ret) + return ret; + + ret = __phy_write(phydev, MII_MMD_DATA, regnum); + if (ret) + return ret; + + ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR); + if (ret) + return ret; + + ret = __phy_write(phydev, MII_MMD_DATA, val); + if (ret) + return ret; + + if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 && + val & MDIO_PCS_CTRL1_CLKSTOP_EN) + ret = __phy_read(phydev, MII_MMD_DATA); + + return ret < 0 ? ret : 0; +} + static int rtl822x_get_features(struct phy_device *phydev) { int val; @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = { .resume = genphy_resume, .read_page = rtl821x_read_page, .write_page = rtl821x_write_page, + .write_mmd = rtl8211e_write_mmd, }, { PHY_ID_MATCH_EXACT(0x001cc916), .name = "RTL8211F Gigabit Ethernet",
It has been noticed that RTL8211E PHY stops detecting and reporting events when EEE is successfully advertised and RXC stopping in LPI is enabled. The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable" register) is set. At the same time LED2 stops blinking as if EEE mode has been disabled. Notably the network traffic still flows through the PHY with no obvious problem. Anyway if any MDIO read procedure is performed after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2 starts blinking and PHY interrupts happens again. The problem has been noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and reporting its event via a dedicated IRQ signal. (Obviously the problem has been unnoticed in the polling mode, since it gets naturally fixed by the periodic MDIO read procedure from the PHY status register - BMSR.) In order to fix that problem we suggest to locally re-implement the MMD write method for RTL8211E PHY and perform a dummy read right after the PC1R register is accessed to enable the RXC stopping in LPI mode. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)