From: Heiner Kallweit <hkallweit1@gmail.com> To: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Serge Semin <fancer.lancer@gmail.com>, Giuseppe Cavallaro <peppe.cavallaro@st.com>, Alexandre Torgue <alexandre.torgue@st.com>, Jose Abreu <joabreu@synopsys.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Joao Pinto <Joao.Pinto@synopsys.com>, Jose Abreu <Jose.Abreu@synopsys.com>, Andrew Lunn <andrew@lunn.ch>, Russell King <linux@armlinux.org.uk>, Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>, Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>, Vyacheslav Mitrofanov <Vyacheslav.Mitrofanov@baikalelectronics.ru>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Date: Tue, 9 Feb 2021 11:37:29 +0100 [thread overview] Message-ID: <a652c69b-94d3-9dc6-c529-1ebc0ed407ac@gmail.com> (raw) In-Reply-To: <20210209101528.3lf47ouaedfgq74n@mobilestation> 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", >>> >>
WARNING: multiple messages have this Message-ID (diff)
From: Heiner Kallweit <hkallweit1@gmail.com> To: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Jose Abreu <Jose.Abreu@synopsys.com>, Andrew Lunn <andrew@lunn.ch>, Joao Pinto <Joao.Pinto@synopsys.com>, linux-kernel@vger.kernel.org, Alexandre Torgue <alexandre.torgue@st.com>, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, Russell King <linux@armlinux.org.uk>, Serge Semin <fancer.lancer@gmail.com>, Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>, Jose Abreu <joabreu@synopsys.com>, Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Jakub Kicinski <kuba@kernel.org>, Giuseppe Cavallaro <peppe.cavallaro@st.com>, Vyacheslav Mitrofanov <Vyacheslav.Mitrofanov@baikalelectronics.ru>, "David S. Miller" <davem@davemloft.net>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Date: Tue, 9 Feb 2021 11:37:29 +0100 [thread overview] Message-ID: <a652c69b-94d3-9dc6-c529-1ebc0ed407ac@gmail.com> (raw) In-Reply-To: <20210209101528.3lf47ouaedfgq74n@mobilestation> 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", >>> >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-02-09 10:50 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 15:27 ` Andrew Lunn 2021-02-08 15:27 ` Andrew Lunn 2021-02-08 17:44 ` Serge Semin 2021-02-08 17:44 ` Serge Semin 2021-02-08 19:03 ` Andrew Lunn 2021-02-08 19:03 ` Andrew Lunn 2021-02-08 20:14 ` Heiner Kallweit 2021-02-08 20:14 ` Heiner Kallweit 2021-02-09 10:15 ` Serge Semin 2021-02-09 10:15 ` Serge Semin 2021-02-09 10:37 ` Heiner Kallweit [this message] 2021-02-09 10:37 ` Heiner Kallweit 2021-02-09 10:56 ` Russell King - ARM Linux admin 2021-02-09 10:56 ` Russell King - ARM Linux admin 2021-02-10 16:47 ` Serge Semin 2021-02-10 16:47 ` Serge Semin 2021-02-11 10:39 ` Russell King - ARM Linux admin 2021-02-11 10:39 ` Russell King - ARM Linux admin 2021-02-11 10:52 ` Serge Semin 2021-02-11 10:52 ` Serge Semin 2021-02-20 9:02 ` Serge Semin 2021-02-20 9:02 ` Serge Semin 2021-02-20 12:51 ` Heiner Kallweit 2021-02-20 12:51 ` Heiner Kallweit 2021-02-20 15:49 ` Andrew Lunn 2021-02-20 15:49 ` Andrew Lunn 2021-02-21 13:51 ` Serge Semin 2021-02-21 13:51 ` Serge Semin 2021-02-09 10:54 ` Russell King - ARM Linux admin 2021-02-09 10:54 ` Russell King - ARM Linux admin 2021-02-08 14:03 ` [PATCH 02/20] net: stmmac: Free Rx descs on Tx allocation failure Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 03/20] net: stmmac: Fix false MTL RX overflow handling for higher queues Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 04/20] net: stmmac: Assert reset control after MDIO de-registration Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 05/20] net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 06/20] net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 07/20] net: stmmac: Clear descriptors before initializing them Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 08/20] net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 09/20] net: stmmac: Discard mii_irq array from private data Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 19:13 ` Andrew Lunn 2021-02-08 19:13 ` Andrew Lunn 2021-02-08 14:03 ` [PATCH 10/20] net: stmmac: Discard Rx copybreak ethtool setting Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 11/20] net: stmmac: Discard index usage in the dirty_rx init Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 12/20] net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 13/20] net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 14/20] net: stmmac: Add DW GMAC disable LPI IRQ mask macro Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 15/20] net: stmmac: Discard STMMAC_RESETING flag Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 16/20] net: stmmac: Discard conditional service task execution Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 17/20] net: stmmac: Add 'cause' arg to the service task executioner Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 18/20] net: stmmac: Move PTP clock enabling to PTP-init method Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 19/20] net: stmmac: Move DMA stop procedure to HW-setup antagonist Serge Semin 2021-02-08 14:03 ` Serge Semin 2021-02-08 14:03 ` [PATCH 20/20] net: stmmac: Move MAC Tx/Rx disabling " Serge Semin 2021-02-08 14:03 ` Serge Semin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a652c69b-94d3-9dc6-c529-1ebc0ed407ac@gmail.com \ --to=hkallweit1@gmail.com \ --cc=Alexey.Malahov@baikalelectronics.ru \ --cc=Joao.Pinto@synopsys.com \ --cc=Jose.Abreu@synopsys.com \ --cc=Pavel.Parkhomenko@baikalelectronics.ru \ --cc=Sergey.Semin@baikalelectronics.ru \ --cc=Vyacheslav.Mitrofanov@baikalelectronics.ru \ --cc=alexandre.torgue@st.com \ --cc=andrew@lunn.ch \ --cc=davem@davemloft.net \ --cc=fancer.lancer@gmail.com \ --cc=joabreu@synopsys.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=linux@armlinux.org.uk \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=peppe.cavallaro@st.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.