| Message ID | 20210310204106.2767772-3-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: broadcom: Suspend fixes | 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 |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: bcm-kernel-feedback-list@broadcom.com linux@armlinux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 7 this patch: 7 |
| 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, 29 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
On 10.03.2021 21:41, Florian Fainelli wrote: > B50212E PHYs have been observed to get into an incorrect state with the > visible effect of having both activity and link LEDs flashing > alternatively instead of being turned off as intended when > genphy_suspend() was issued. The BCM54810 is a similar design and > equally suffers from that issue. > > The datasheet is not particularly clear whether a read/modify/write > sequence is acceptable and only indicates that BMCR.PDOWN=1 should be > utilized to enter the power down mode. When this was done the PHYs were > always measured to have power levels that match the expectations and > LEDs powered off. > > Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/phy/broadcom.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > index b8eb736fb456..b33ffd44f799 100644 > --- a/drivers/net/phy/broadcom.c > +++ b/drivers/net/phy/broadcom.c > @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) > return 0; > } > > +static int bcm54xx_suspend(struct phy_device *phydev) > +{ > + /* We cannot perform a read/modify/write like what genphy_suspend() > + * does because depending on the time we can observe the PHY having > + * both of its LEDs flashing indicating that it is in an incorrect > + * state and not powered down as expected. > + * > + * There is not a clear indication in the datasheet whether a > + * read/modify/write would be acceptable, but a blind write to the > + * register has been proven to be functional unlike the > + * Read/Modify/Write. > + */ > + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in forced mode. So you have to rely on somebody calling genphy_config_aneg() to sync the register bits with the values cached in struct phy_device on resume. Typically the phylib state machine takes care, but do we have to consider use cases where this is not the case? Heiner > +} > + > static int bcm54xx_resume(struct phy_device *phydev) > { > int ret; > @@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = { > .config_aneg = bcm5481_config_aneg, > .config_intr = bcm_phy_config_intr, > .handle_interrupt = bcm_phy_handle_interrupt, > - .suspend = genphy_suspend, > + .suspend = bcm54xx_suspend, > .resume = bcm54xx_resume, > }, { > .phy_id = PHY_ID_BCM54811, >
On 3/10/21 1:07 PM, Heiner Kallweit wrote: > On 10.03.2021 21:41, Florian Fainelli wrote: >> B50212E PHYs have been observed to get into an incorrect state with the >> visible effect of having both activity and link LEDs flashing >> alternatively instead of being turned off as intended when >> genphy_suspend() was issued. The BCM54810 is a similar design and >> equally suffers from that issue. >> >> The datasheet is not particularly clear whether a read/modify/write >> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be >> utilized to enter the power down mode. When this was done the PHYs were >> always measured to have power levels that match the expectations and >> LEDs powered off. >> >> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/net/phy/broadcom.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >> index b8eb736fb456..b33ffd44f799 100644 >> --- a/drivers/net/phy/broadcom.c >> +++ b/drivers/net/phy/broadcom.c >> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) >> return 0; >> } >> >> +static int bcm54xx_suspend(struct phy_device *phydev) >> +{ >> + /* We cannot perform a read/modify/write like what genphy_suspend() >> + * does because depending on the time we can observe the PHY having >> + * both of its LEDs flashing indicating that it is in an incorrect >> + * state and not powered down as expected. >> + * >> + * There is not a clear indication in the datasheet whether a >> + * read/modify/write would be acceptable, but a blind write to the >> + * register has been proven to be functional unlike the >> + * Read/Modify/Write. >> + */ >> + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); > > This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in > forced mode. So you have to rely on somebody calling genphy_config_aneg() > to sync the register bits with the values cached in struct phy_device > on resume. Typically the phylib state machine takes care, but do we have > to consider use cases where this is not the case? Good point, how about if we had forced the link before suspending, does PHYLIB take care of re-applying the same parameters? It arguably should do that in all cases given that power to the PHY can be cut depending on the suspend mode.
On 10.03.2021 22:15, Florian Fainelli wrote: > On 3/10/21 1:07 PM, Heiner Kallweit wrote: >> On 10.03.2021 21:41, Florian Fainelli wrote: >>> B50212E PHYs have been observed to get into an incorrect state with the >>> visible effect of having both activity and link LEDs flashing >>> alternatively instead of being turned off as intended when >>> genphy_suspend() was issued. The BCM54810 is a similar design and >>> equally suffers from that issue. >>> >>> The datasheet is not particularly clear whether a read/modify/write >>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be >>> utilized to enter the power down mode. When this was done the PHYs were >>> always measured to have power levels that match the expectations and >>> LEDs powered off. >>> >>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >>> drivers/net/phy/broadcom.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>> index b8eb736fb456..b33ffd44f799 100644 >>> --- a/drivers/net/phy/broadcom.c >>> +++ b/drivers/net/phy/broadcom.c >>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) >>> return 0; >>> } >>> >>> +static int bcm54xx_suspend(struct phy_device *phydev) >>> +{ >>> + /* We cannot perform a read/modify/write like what genphy_suspend() >>> + * does because depending on the time we can observe the PHY having >>> + * both of its LEDs flashing indicating that it is in an incorrect >>> + * state and not powered down as expected. >>> + * >>> + * There is not a clear indication in the datasheet whether a >>> + * read/modify/write would be acceptable, but a blind write to the >>> + * register has been proven to be functional unlike the >>> + * Read/Modify/Write. >>> + */ >>> + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); >> >> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in >> forced mode. So you have to rely on somebody calling genphy_config_aneg() >> to sync the register bits with the values cached in struct phy_device >> on resume. Typically the phylib state machine takes care, but do we have >> to consider use cases where this is not the case? > > Good point, how about if we had forced the link before suspending, does > PHYLIB take care of re-applying the same parameters? It arguably should > do that in all cases given that power to the PHY can be cut depending on > the suspend mode. > When entering power-down mode the link is lost and we go to HALTED state. On resume, phy_start() sets UP state and state machine calls phy_start_aneg(), which takes care of syncing the BMCR forced mode bits. A potential issue arises if we have a driver that doesn't use the phylib state machine and prefers to do it on its own. IIRC I once stumbled across this when I also relied on the phylib state machine running in a change. I'm not sure whether we can run into a problem, but it's worth spending a thought before somebody complains after applying the change.
On 3/10/21 1:43 PM, Heiner Kallweit wrote: > On 10.03.2021 22:15, Florian Fainelli wrote: >> On 3/10/21 1:07 PM, Heiner Kallweit wrote: >>> On 10.03.2021 21:41, Florian Fainelli wrote: >>>> B50212E PHYs have been observed to get into an incorrect state with the >>>> visible effect of having both activity and link LEDs flashing >>>> alternatively instead of being turned off as intended when >>>> genphy_suspend() was issued. The BCM54810 is a similar design and >>>> equally suffers from that issue. >>>> >>>> The datasheet is not particularly clear whether a read/modify/write >>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be >>>> utilized to enter the power down mode. When this was done the PHYs were >>>> always measured to have power levels that match the expectations and >>>> LEDs powered off. >>>> >>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>>> drivers/net/phy/broadcom.c | 17 ++++++++++++++++- >>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>>> index b8eb736fb456..b33ffd44f799 100644 >>>> --- a/drivers/net/phy/broadcom.c >>>> +++ b/drivers/net/phy/broadcom.c >>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) >>>> return 0; >>>> } >>>> >>>> +static int bcm54xx_suspend(struct phy_device *phydev) >>>> +{ >>>> + /* We cannot perform a read/modify/write like what genphy_suspend() >>>> + * does because depending on the time we can observe the PHY having >>>> + * both of its LEDs flashing indicating that it is in an incorrect >>>> + * state and not powered down as expected. >>>> + * >>>> + * There is not a clear indication in the datasheet whether a >>>> + * read/modify/write would be acceptable, but a blind write to the >>>> + * register has been proven to be functional unlike the >>>> + * Read/Modify/Write. >>>> + */ >>>> + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); >>> >>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in >>> forced mode. So you have to rely on somebody calling genphy_config_aneg() >>> to sync the register bits with the values cached in struct phy_device >>> on resume. Typically the phylib state machine takes care, but do we have >>> to consider use cases where this is not the case? >> >> Good point, how about if we had forced the link before suspending, does >> PHYLIB take care of re-applying the same parameters? It arguably should >> do that in all cases given that power to the PHY can be cut depending on >> the suspend mode. >> > > When entering power-down mode the link is lost and we go to HALTED state. > On resume, phy_start() sets UP state and state machine calls > phy_start_aneg(), which takes care of syncing the BMCR forced mode bits. > A potential issue arises if we have a driver that doesn't use the > phylib state machine and prefers to do it on its own. > IIRC I once stumbled across this when I also relied on the phylib state > machine running in a change. > I'm not sure whether we can run into a problem, but it's worth spending > a thought before somebody complains after applying the change. That is a fair point, I could save the BMCR before modifying it and restore it in bcm54xx_resume() and phy_start_aneg() should not issue an additional re-negotiation in that case. Let me explore a bit more to find out which of these BMCR bits makes the PHY go bonkers. Thanks
On 3/10/21 2:56 PM, Florian Fainelli wrote: > On 3/10/21 1:43 PM, Heiner Kallweit wrote: >> On 10.03.2021 22:15, Florian Fainelli wrote: >>> On 3/10/21 1:07 PM, Heiner Kallweit wrote: >>>> On 10.03.2021 21:41, Florian Fainelli wrote: >>>>> B50212E PHYs have been observed to get into an incorrect state with the >>>>> visible effect of having both activity and link LEDs flashing >>>>> alternatively instead of being turned off as intended when >>>>> genphy_suspend() was issued. The BCM54810 is a similar design and >>>>> equally suffers from that issue. >>>>> >>>>> The datasheet is not particularly clear whether a read/modify/write >>>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be >>>>> utilized to enter the power down mode. When this was done the PHYs were >>>>> always measured to have power levels that match the expectations and >>>>> LEDs powered off. >>>>> >>>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") >>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>> --- >>>>> drivers/net/phy/broadcom.c | 17 ++++++++++++++++- >>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>>>> index b8eb736fb456..b33ffd44f799 100644 >>>>> --- a/drivers/net/phy/broadcom.c >>>>> +++ b/drivers/net/phy/broadcom.c >>>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) >>>>> return 0; >>>>> } >>>>> >>>>> +static int bcm54xx_suspend(struct phy_device *phydev) >>>>> +{ >>>>> + /* We cannot perform a read/modify/write like what genphy_suspend() >>>>> + * does because depending on the time we can observe the PHY having >>>>> + * both of its LEDs flashing indicating that it is in an incorrect >>>>> + * state and not powered down as expected. >>>>> + * >>>>> + * There is not a clear indication in the datasheet whether a >>>>> + * read/modify/write would be acceptable, but a blind write to the >>>>> + * register has been proven to be functional unlike the >>>>> + * Read/Modify/Write. >>>>> + */ >>>>> + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); >>>> >>>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in >>>> forced mode. So you have to rely on somebody calling genphy_config_aneg() >>>> to sync the register bits with the values cached in struct phy_device >>>> on resume. Typically the phylib state machine takes care, but do we have >>>> to consider use cases where this is not the case? >>> >>> Good point, how about if we had forced the link before suspending, does >>> PHYLIB take care of re-applying the same parameters? It arguably should >>> do that in all cases given that power to the PHY can be cut depending on >>> the suspend mode. >>> >> >> When entering power-down mode the link is lost and we go to HALTED state. >> On resume, phy_start() sets UP state and state machine calls >> phy_start_aneg(), which takes care of syncing the BMCR forced mode bits. >> A potential issue arises if we have a driver that doesn't use the >> phylib state machine and prefers to do it on its own. >> IIRC I once stumbled across this when I also relied on the phylib state >> machine running in a change. >> I'm not sure whether we can run into a problem, but it's worth spending >> a thought before somebody complains after applying the change. > > That is a fair point, I could save the BMCR before modifying it and > restore it in bcm54xx_resume() and phy_start_aneg() should not issue an > additional re-negotiation in that case. Let me explore a bit more to > find out which of these BMCR bits makes the PHY go bonkers. I re-tested with different kernels and was convinced that the problem I saw due to the lack of locking in the 4.9 kernel around phydrv->suspend() since 4.9 was where I started working on this. It turns out this was reproducible with different kernel versions as well so there is really something that is possibly specific to the BCM54810 PHY and the specific design. I will run more tests to get to the bottom of this hopefully.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index b8eb736fb456..b33ffd44f799 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev) return 0; } +static int bcm54xx_suspend(struct phy_device *phydev) +{ + /* We cannot perform a read/modify/write like what genphy_suspend() + * does because depending on the time we can observe the PHY having + * both of its LEDs flashing indicating that it is in an incorrect + * state and not powered down as expected. + * + * There is not a clear indication in the datasheet whether a + * read/modify/write would be acceptable, but a blind write to the + * register has been proven to be functional unlike the + * Read/Modify/Write. + */ + return phy_write(phydev, MII_BMCR, BMCR_PDOWN); +} + static int bcm54xx_resume(struct phy_device *phydev) { int ret; @@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = { .config_aneg = bcm5481_config_aneg, .config_intr = bcm_phy_config_intr, .handle_interrupt = bcm_phy_handle_interrupt, - .suspend = genphy_suspend, + .suspend = bcm54xx_suspend, .resume = bcm54xx_resume, }, { .phy_id = PHY_ID_BCM54811,
B50212E PHYs have been observed to get into an incorrect state with the visible effect of having both activity and link LEDs flashing alternatively instead of being turned off as intended when genphy_suspend() was issued. The BCM54810 is a similar design and equally suffers from that issue. The datasheet is not particularly clear whether a read/modify/write sequence is acceptable and only indicates that BMCR.PDOWN=1 should be utilized to enter the power down mode. When this was done the PHYs were always measured to have power levels that match the expectations and LEDs powered off. Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/broadcom.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)