| Message ID | 20210213034632.2420998-4-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5d4358ede8ebe2e4ae03a633082f3ce21ec2df3e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: broadcom: Cleanups and APD | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 12 this patch: 12 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 12 this patch: 12 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote: > BCM54210E/BCM50212E has been verified to work correctly with the > auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it > to the list of PHYs working. > > While we are at it, provide an appropriate name for the bit we are > changing which disables the RXC and TXC during auto-power down when > there is no energy on the cable. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > drivers/net/phy/broadcom.c | 8 +++++--- > include/linux/brcmphy.h | 2 +- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > index 3ce266ab521b..91fbd26c809e 100644 > --- a/drivers/net/phy/broadcom.c > +++ b/drivers/net/phy/broadcom.c > @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) > if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 && > BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 && > BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M && > + BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E && > BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 && > BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811) > return; > @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) > val |= BCM54XX_SHD_SCR3_DLLAPD_DIS; > > if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) { > - if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || > - BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811) > - val |= BCM54810_SHD_SCR3_TRDDAPD; > + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E || > + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || > + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) > + val |= BCM54XX_SHD_SCR3_RXCTXC_DIS; > else > val |= BCM54XX_SHD_SCR3_TRDDAPD; > } > diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h > index 844dcfe789a2..16597d3fa011 100644 > --- a/include/linux/brcmphy.h > +++ b/include/linux/brcmphy.h > @@ -193,6 +193,7 @@ > #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 > #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 > #define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 > +#define BCM54XX_SHD_SCR3_RXCTXC_DIS 0x0100 Curiously enough, my BCM5464R datasheet does say: The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1. but when I go to the definition of the register, bit 8 is hidden. Odd. How can I ensure that the auto power down feature is doing something? > > /* 01010: Auto Power-Down */ > #define BCM54XX_SHD_APD 0x0a > @@ -253,7 +254,6 @@ > #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0) > #define BCM54810_SHD_CLK_CTL 0x3 > #define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9) > -#define BCM54810_SHD_SCR3_TRDDAPD 0x0100 > > /* BCM54612E Registers */ > #define BCM54612E_EXP_SPARE0 (MII_BCM54XX_EXP_SEL_ETC + 0x34) > -- > 2.25.1 >
On 2/13/2021 2:42 AM, Vladimir Oltean wrote: > On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote: >> BCM54210E/BCM50212E has been verified to work correctly with the >> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it >> to the list of PHYs working. >> >> While we are at it, provide an appropriate name for the bit we are >> changing which disables the RXC and TXC during auto-power down when >> there is no energy on the cable. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > >> drivers/net/phy/broadcom.c | 8 +++++--- >> include/linux/brcmphy.h | 2 +- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >> index 3ce266ab521b..91fbd26c809e 100644 >> --- a/drivers/net/phy/broadcom.c >> +++ b/drivers/net/phy/broadcom.c >> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) >> if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 && >> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 && >> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M && >> + BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E && >> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 && >> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811) >> return; >> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) >> val |= BCM54XX_SHD_SCR3_DLLAPD_DIS; >> >> if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) { >> - if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || >> - BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811) >> - val |= BCM54810_SHD_SCR3_TRDDAPD; >> + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E || >> + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || >> + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) >> + val |= BCM54XX_SHD_SCR3_RXCTXC_DIS; >> else >> val |= BCM54XX_SHD_SCR3_TRDDAPD; >> } >> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h >> index 844dcfe789a2..16597d3fa011 100644 >> --- a/include/linux/brcmphy.h >> +++ b/include/linux/brcmphy.h >> @@ -193,6 +193,7 @@ >> #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 >> #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 >> #define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 >> +#define BCM54XX_SHD_SCR3_RXCTXC_DIS 0x0100 > > Curiously enough, my BCM5464R datasheet does say: > > The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T > Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1. > > but when I go to the definition of the register, bit 8 is hidden. Odd. > > How can I ensure that the auto power down feature is doing something? I am trying to confirm what the expected power levels should be from the 54210E product engineer so I can give you an estimate of what you should see with and without while measure the PHY's regulators.
On 2/14/2021 8:29 PM, Florian Fainelli wrote: > > > On 2/13/2021 2:42 AM, Vladimir Oltean wrote: >> On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote: >>> BCM54210E/BCM50212E has been verified to work correctly with the >>> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it >>> to the list of PHYs working. >>> >>> While we are at it, provide an appropriate name for the bit we are >>> changing which disables the RXC and TXC during auto-power down when >>> there is no energy on the cable. >>> >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >> >> Reviewed-by: Vladimir Oltean <olteanv@gmail.com> >> >>> drivers/net/phy/broadcom.c | 8 +++++--- >>> include/linux/brcmphy.h | 2 +- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>> index 3ce266ab521b..91fbd26c809e 100644 >>> --- a/drivers/net/phy/broadcom.c >>> +++ b/drivers/net/phy/broadcom.c >>> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) >>> if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 && >>> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 && >>> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M && >>> + BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E && >>> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 && >>> BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811) >>> return; >>> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) >>> val |= BCM54XX_SHD_SCR3_DLLAPD_DIS; >>> >>> if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) { >>> - if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || >>> - BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811) >>> - val |= BCM54810_SHD_SCR3_TRDDAPD; >>> + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E || >>> + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || >>> + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) >>> + val |= BCM54XX_SHD_SCR3_RXCTXC_DIS; >>> else >>> val |= BCM54XX_SHD_SCR3_TRDDAPD; >>> } >>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h >>> index 844dcfe789a2..16597d3fa011 100644 >>> --- a/include/linux/brcmphy.h >>> +++ b/include/linux/brcmphy.h >>> @@ -193,6 +193,7 @@ >>> #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 >>> #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 >>> #define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 >>> +#define BCM54XX_SHD_SCR3_RXCTXC_DIS 0x0100 >> >> Curiously enough, my BCM5464R datasheet does say: >> >> The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T >> Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1. >> >> but when I go to the definition of the register, bit 8 is hidden. Odd. >> >> How can I ensure that the auto power down feature is doing something? > > I am trying to confirm what the expected power levels should be from the > 54210E product engineer so I can give you an estimate of what you should > see with and without while measure the PHY's regulators. Took a while but for the 54210E reference board here are the numbers, your mileage will vary depending on the supplies, regulator efficiency and PCB design around the PHY obviously: BMCR.PDOWN: 86.12 mW auto-power down: 77.84 mW auto-power-down, DLL disabled: 30.83 mW IDDQ-low power: 9.85 mW (requires a RESETn toggle) IDDQ with soft recovery: 10.75 mW Interestingly, the 50212E that I am using requires writing the PDOWN bit and only that bit (not a RMW) in order to get in a correct state, both LEDs keep flashing when that happens, fixes coming. When net-next opens back up I will submit patches to support IDDQ with soft recovery since that is clearly much better than the standard power down and it does not require a RESETn toggle.
On Tue, Mar 02, 2021 at 07:37:34PM -0800, Florian Fainelli wrote: > Took a while but for the 54210E reference board here are the numbers, > your mileage will vary depending on the supplies, regulator efficiency > and PCB design around the PHY obviously: > > BMCR.PDOWN: 86.12 mW > auto-power down: 77.84 mW Quite curious that the APD power is lower than the normal BMCR.PDOWN value. As far as my understanding goes, when in APD mode, the PHY even wakes up from time to time to send pulses to the link partner? > auto-power-down, DLL disabled: 30.83 mW The jump from simple APD to APD with DLL disabled is pretty big. Correct me if I'm wrong, but there's an intermediary step which was not measured, where the CLK125 is disabled but the internal DLL (Delay Locked Loop?) is still enabled. I think powering off the internal DLL also implies powering off the CLK125 pin, at least that's how the PHY driver treats things at the moment. But we don't know if the huge reduction in power is due just to CLK125 or the DLL (it's more likely it's due to both, in equal amounts). Anyway, it's great to have some results which tell us exactly what is worthwhile and what isn't. In other news, I've added the BCM5464 to the list of PHYs with APD and I didn't see any issues thus far. > IDDQ-low power: 9.85 mW (requires a RESETn toggle) > IDDQ with soft recovery: 10.75 mW > > Interestingly, the 50212E that I am using requires writing the PDOWN bit > and only that bit (not a RMW) in order to get in a correct state, both > LEDs keep flashing when that happens, fixes coming. > > When net-next opens back up I will submit patches to support IDDQ with > soft recovery since that is clearly much better than the standard power > down and it does not require a RESETn toggle. Iddq must be the quiescent supply current, isn't it (but in that case, I'm a bit confused to not see a value in mA)? Is it an actual operating mode (I don't see anything about that mentioned in the BCM5464 sheet) and if it is, what is there exactly to support?
On 3/4/2021 5:08 PM, Vladimir Oltean wrote: > On Tue, Mar 02, 2021 at 07:37:34PM -0800, Florian Fainelli wrote: >> Took a while but for the 54210E reference board here are the numbers, >> your mileage will vary depending on the supplies, regulator efficiency >> and PCB design around the PHY obviously: >> >> BMCR.PDOWN: 86.12 mW >> auto-power down: 77.84 mW > > Quite curious that the APD power is lower than the normal BMCR.PDOWN > value. As far as my understanding goes, when in APD mode, the PHY even > wakes up from time to time to send pulses to the link partner? Auto-power down kicks in when the cable is disconnected. There is another IDDQ mode that supports energy detection though I am unsure of when it would be useful for most Linux enabled systems. > >> auto-power-down, DLL disabled: 30.83 mW > > The jump from simple APD to APD with DLL disabled is pretty big. > Correct me if I'm wrong, but there's an intermediary step which was not > measured, where the CLK125 is disabled but the internal DLL (Delay > Locked Loop?) is still enabled. I think powering off the internal DLL > also implies powering off the CLK125 pin, at least that's how the PHY > driver treats things at the moment. But we don't know if the huge > reduction in power is due just to CLK125 or the DLL (it's more likely > it's due to both, in equal amounts). Agree, I do not have the break down though. > > Anyway, it's great to have some results which tell us exactly what is > worthwhile and what isn't. In other news, I've added the BCM5464 to the > list of PHYs with APD and I didn't see any issues thus far. > >> IDDQ-low power: 9.85 mW (requires a RESETn toggle) >> IDDQ with soft recovery: 10.75 mW >> >> Interestingly, the 50212E that I am using requires writing the PDOWN bit >> and only that bit (not a RMW) in order to get in a correct state, both >> LEDs keep flashing when that happens, fixes coming. >> >> When net-next opens back up I will submit patches to support IDDQ with >> soft recovery since that is clearly much better than the standard power >> down and it does not require a RESETn toggle. > > Iddq must be the quiescent supply current, isn't it (but in that case, > I'm a bit confused to not see a value in mA)? Is it an actual operating > mode (I don't see anything about that mentioned in the BCM5464 sheet) > and if it is, what is there exactly to support? You would put the PHY in IDDQ with soft recovery (or ultra low power) when you are administratively bringing down the network interface (and its PHY), or when suspending to a low power state where Wake-on-LAN is not enabled.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 3ce266ab521b..91fbd26c809e 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 && BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 && BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M && + BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E && BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 && BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811) return; @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev) val |= BCM54XX_SHD_SCR3_DLLAPD_DIS; if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) { - if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || - BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811) - val |= BCM54810_SHD_SCR3_TRDDAPD; + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E || + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 || + BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) + val |= BCM54XX_SHD_SCR3_RXCTXC_DIS; else val |= BCM54XX_SHD_SCR3_TRDDAPD; } diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h index 844dcfe789a2..16597d3fa011 100644 --- a/include/linux/brcmphy.h +++ b/include/linux/brcmphy.h @@ -193,6 +193,7 @@ #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 #define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 +#define BCM54XX_SHD_SCR3_RXCTXC_DIS 0x0100 /* 01010: Auto Power-Down */ #define BCM54XX_SHD_APD 0x0a @@ -253,7 +254,6 @@ #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0) #define BCM54810_SHD_CLK_CTL 0x3 #define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9) -#define BCM54810_SHD_SCR3_TRDDAPD 0x0100 /* BCM54612E Registers */ #define BCM54612E_EXP_SPARE0 (MII_BCM54XX_EXP_SEL_ETC + 0x34)
BCM54210E/BCM50212E has been verified to work correctly with the auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it to the list of PHYs working. While we are at it, provide an appropriate name for the bit we are changing which disables the RXC and TXC during auto-power down when there is no energy on the cable. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/broadcom.c | 8 +++++--- include/linux/brcmphy.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)