| Message ID | 20210208140341.9271-2-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: Obvious cleanups and several fixes | expand |
On Mon, Feb 08, 2021 at 05:03:22PM +0300, 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. Hi Serge Is this listed in an Errata from Realtek? Andrew
On Mon, Feb 08, 2021 at 04:27:36PM +0100, Andrew Lunn wrote: > On Mon, Feb 08, 2021 at 05:03:22PM +0300, 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. > > Hi Serge > > Is this listed in an Errata from Realtek? Hi Andrew, I honestly tried to find any doc with a glimpse of errata for RTL8211E PHY, but with no luck. Official datasheet didn't have any info regarding possible hw bugs too. Thus I had no choice but to find a fix of the problem myself. It took me some time to figure out why the events weren't reported after the very first link setup (turned out only a full HW reset clears the PC1R.10 bit state). I thought it could have been connected with some sleep/idle/power-safe mode. So I disabled the EEE initialization in the STMMAC driver. It worked. Then I left the EEE mode enabled, but called the phy_init_eee(phy, 0) method with "clk_stop_enable==0", so PHY wouldn't stop RXC in LPI mode. And it wonderfully worked. Then I started to dig in from another side. I left "RXC disable in LPI" mode enabled and tried to figure out what was going on with the PHY when it stopped reporting events just by reading from its CSR using phytool utility. It was curious to discover that any attempt to read from any PHY register caused the problem disappearance (LED2 started blinking, events got to be reported). Since I did nothing but a mere reading from a random even EEE-unrelated register I inferred that the problem must be in some HW/PHY bug. That's how I've got to the patch introduced here. If you have any better idea what could be a reason of that weird behavior I'd be glad to test it out on my device. -Sergey > > Andrew
> Hi Andrew, > > I honestly tried to find any doc with a glimpse of errata for RTL8211E > PHY, but with no luck. Official datasheet didn't have any info regarding > possible hw bugs too. Thus I had no choice but to find a fix of the > problem myself. > > It took me some time to figure out why the events weren't reported after > the very first link setup (turned out only a full HW reset clears the > PC1R.10 bit state). I thought it could have been connected with some > sleep/idle/power-safe mode. So I disabled the EEE initialization in the > STMMAC driver. It worked. Then I left the EEE mode enabled, but called the > phy_init_eee(phy, 0) method with "clk_stop_enable==0", so PHY wouldn't > stop RXC in LPI mode. And it wonderfully worked. Then I started to dig in > from another side. I left "RXC disable in LPI" mode enabled and tried to > figure out what was going on with the PHY when it stopped reporting events > just by reading from its CSR using phytool utility. It was curious to > discover that any attempt to read from any PHY register caused the problem > disappearance (LED2 started blinking, events got to be reported). Since I > did nothing but a mere reading from a random even EEE-unrelated register I > inferred that the problem must be in some HW/PHY bug. That's how I've got > to the patch introduced here. If you have any better idea what could be a > reason of that weird behavior I'd be glad to test it out on my device. It is a reasonable explanation, and a read should not do any harm. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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. > + 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 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. -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 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 01:15:28PM +0300, Serge Semin wrote: > On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote: > > 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. Please not genphy_* - that namespace is used for up-to-1G PHYs. I thought about suggesting what you are proposing, but the problem is this is just making things less and less efficient. Every time we break a function up and export it, we increase the execution overhead of the code. That said, the PHY accesses are relatively slow. My opinion is that as this is just a single location at the moment, it is not worth the effort - but if we get more of examples of this, then it makes sense to provide the common accessor.
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.
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(+)