| Message ID | 20210212205721.2406849-3-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: broadcom: APD improvements | 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 | warning | 1 maintainers not CCed: mcarlson@broadcom.com |
| 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, 8 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 12:57:20PM -0800, Florian Fainelli wrote: > When support for optionally disabling the TXC was introduced, bit 2 was > used to do that operation but the datasheet for 50610M from 2009 does > not show bit 2 as being defined. Bit 8 is the one that allows automatic > disabling of the RXC/TXC auto disabling during auto power down. > > Fixes: 52fae0837153 ("tg3 / broadcom: Optionally disable TXC if no link") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > include/linux/brcmphy.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h > index da7bf9dfef5b..3dd8203cf780 100644 > --- a/include/linux/brcmphy.h > +++ b/include/linux/brcmphy.h > @@ -193,7 +193,7 @@ > #define BCM54XX_SHD_SCR3 0x05 > #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 > #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 > -#define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 > +#define BCM54XX_SHD_SCR3_TRDDAPD 0x0100 > > /* 01010: Auto Power-Down */ > #define BCM54XX_SHD_APD 0x0a > -- > 2.25.1 > We may have a problem here, with the layout of the Spare Control 3 register not being as universal as we think. Your finding may have been the same as Kevin Lo's from commit b0ed0bbfb304 ("net: phy: broadcom: add support for BCM54811 PHY"), therefore your change is making BCM54XX_SHD_SCR3_TRDDAPD == BCM54810_SHD_SCR3_TRDDAPD, so currently this if condition is redundant and probably something else is wrong too: 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; else val |= BCM54XX_SHD_SCR3_TRDDAPD; } I'm not sure what "TRDD" stands for, but my copy of the BCM5464R datasheet shows both bits 2 as well as 8 as being reserved. I have "CLK125 Output" in bit 0, "DLL Auto Power-Down" in bit 1, "SD/Energy Detect Change" in bit 5, "TXC Disable" in bit 6, and that's about it. But I think it doesn't matter what BCM5464R has, since this feature is gated by PHY_BRCM_DIS_TXCRXC_NOENRGY.
On 2/12/2021 5:11 PM, Vladimir Oltean wrote: > On Fri, Feb 12, 2021 at 12:57:20PM -0800, Florian Fainelli wrote: >> When support for optionally disabling the TXC was introduced, bit 2 was >> used to do that operation but the datasheet for 50610M from 2009 does >> not show bit 2 as being defined. Bit 8 is the one that allows automatic >> disabling of the RXC/TXC auto disabling during auto power down. >> >> Fixes: 52fae0837153 ("tg3 / broadcom: Optionally disable TXC if no link") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> include/linux/brcmphy.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h >> index da7bf9dfef5b..3dd8203cf780 100644 >> --- a/include/linux/brcmphy.h >> +++ b/include/linux/brcmphy.h >> @@ -193,7 +193,7 @@ >> #define BCM54XX_SHD_SCR3 0x05 >> #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 >> #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 >> -#define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 >> +#define BCM54XX_SHD_SCR3_TRDDAPD 0x0100 >> >> /* 01010: Auto Power-Down */ >> #define BCM54XX_SHD_APD 0x0a >> -- >> 2.25.1 >> > > We may have a problem here, with the layout of the Spare Control 3 > register not being as universal as we think. > > Your finding may have been the same as Kevin Lo's from commit > b0ed0bbfb304 ("net: phy: broadcom: add support for BCM54811 PHY"), > therefore your change is making BCM54XX_SHD_SCR3_TRDDAPD == > BCM54810_SHD_SCR3_TRDDAPD, so currently this if condition is redundant > and probably something else is wrong too: > > 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; > else > val |= BCM54XX_SHD_SCR3_TRDDAPD; > } > > I'm not sure what "TRDD" stands for, but my copy of the BCM5464R > datasheet shows both bits 2 as well as 8 as being reserved. I have > "CLK125 Output" in bit 0, "DLL Auto Power-Down" in bit 1, "SD/Energy > Detect Change" in bit 5, "TXC Disable" in bit 6, and that's about it. Let me go back to the datasheet of all of the PHYs supported by bcm54xx_adjust_rxrefclk() and make sure we set the right bit. I also have no idea what TRDD stands for. > > But I think it doesn't matter what BCM5464R has, since this feature is > gated by PHY_BRCM_DIS_TXCRXC_NOENRGY. Yes, but it should be working nonetheless.
On 2/12/2021 5:14 PM, Florian Fainelli wrote: > > > On 2/12/2021 5:11 PM, Vladimir Oltean wrote: >> On Fri, Feb 12, 2021 at 12:57:20PM -0800, Florian Fainelli wrote: >>> When support for optionally disabling the TXC was introduced, bit 2 was >>> used to do that operation but the datasheet for 50610M from 2009 does >>> not show bit 2 as being defined. Bit 8 is the one that allows automatic >>> disabling of the RXC/TXC auto disabling during auto power down. >>> >>> Fixes: 52fae0837153 ("tg3 / broadcom: Optionally disable TXC if no link") >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >>> include/linux/brcmphy.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h >>> index da7bf9dfef5b..3dd8203cf780 100644 >>> --- a/include/linux/brcmphy.h >>> +++ b/include/linux/brcmphy.h >>> @@ -193,7 +193,7 @@ >>> #define BCM54XX_SHD_SCR3 0x05 >>> #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 >>> #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 >>> -#define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 >>> +#define BCM54XX_SHD_SCR3_TRDDAPD 0x0100 >>> >>> /* 01010: Auto Power-Down */ >>> #define BCM54XX_SHD_APD 0x0a >>> -- >>> 2.25.1 >>> >> >> We may have a problem here, with the layout of the Spare Control 3 >> register not being as universal as we think. >> >> Your finding may have been the same as Kevin Lo's from commit >> b0ed0bbfb304 ("net: phy: broadcom: add support for BCM54811 PHY"), >> therefore your change is making BCM54XX_SHD_SCR3_TRDDAPD == >> BCM54810_SHD_SCR3_TRDDAPD, so currently this if condition is redundant >> and probably something else is wrong too: >> >> 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; >> else >> val |= BCM54XX_SHD_SCR3_TRDDAPD; >> } >> >> I'm not sure what "TRDD" stands for, but my copy of the BCM5464R >> datasheet shows both bits 2 as well as 8 as being reserved. I have >> "CLK125 Output" in bit 0, "DLL Auto Power-Down" in bit 1, "SD/Energy >> Detect Change" in bit 5, "TXC Disable" in bit 6, and that's about it. > > Let me go back to the datasheet of all of the PHYs supported by > bcm54xx_adjust_rxrefclk() and make sure we set the right bit. I really don't know what the situation is with the 50610 and 50610M and the datasheet appears to reflect that the latest PHYs should be revision 3 or newer (which we explicitly check for earlier), and that bit 8 is supposed to control the disabling of RXC/TXC during auto-power down. I can only trust that Matt checked with the design team back then and that the datasheet must be wrong (would not be an isolated incident), let's try not to fix something that we do not know for sure is broken. I will respin without this patch and with another clean up added.
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h index da7bf9dfef5b..3dd8203cf780 100644 --- a/include/linux/brcmphy.h +++ b/include/linux/brcmphy.h @@ -193,7 +193,7 @@ #define BCM54XX_SHD_SCR3 0x05 #define BCM54XX_SHD_SCR3_DEF_CLK125 0x0001 #define BCM54XX_SHD_SCR3_DLLAPD_DIS 0x0002 -#define BCM54XX_SHD_SCR3_TRDDAPD 0x0004 +#define BCM54XX_SHD_SCR3_TRDDAPD 0x0100 /* 01010: Auto Power-Down */ #define BCM54XX_SHD_APD 0x0a
When support for optionally disabling the TXC was introduced, bit 2 was used to do that operation but the datasheet for 50610M from 2009 does not show bit 2 as being defined. Bit 8 is the one that allows automatic disabling of the RXC/TXC auto disabling during auto power down. Fixes: 52fae0837153 ("tg3 / broadcom: Optionally disable TXC if no link") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- include/linux/brcmphy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)