| Message ID | 20220815174356.2681127-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: broadcom: Implement suspend/resume for AC131 and BCM5241 | expand |
On Mon, Aug 15, 2022 at 10:43:56AM -0700, Florian Fainelli wrote: > + /* We cannot use a read/modify/write here otherwise the PHY continues > + * to drive LEDs which defeats the purpose of low power mode. > + */ ... > + /* Set standby mode */ > + reg = phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); > + if (reg < 0) { > + err = reg; > + goto done; > + } > + > + reg |= MII_BRCM_FET_SHDW_AM4_STANDBY; > + > + err = phy_write(phydev, MII_BRCM_FET_SHDW_AUXMODE4, reg); Does the read-modify-write problem extend to this register? Why would the PHY behave differently whether you used phy_modify() here or not? On the mdio bus, it should be exactly the same - the only difference is that we're guaranteed to hold the lock over the sequence whereas this drops and re-acquires the lock. If it's sensitive to the timing of the read and the write, it suggests the above code is fragile - maybe there needs to be a minimum delay inserted between the read and the write?
On 8/15/22 11:00, Russell King (Oracle) wrote: > On Mon, Aug 15, 2022 at 10:43:56AM -0700, Florian Fainelli wrote: >> + /* We cannot use a read/modify/write here otherwise the PHY continues >> + * to drive LEDs which defeats the purpose of low power mode. >> + */ > ... >> + /* Set standby mode */ >> + reg = phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); >> + if (reg < 0) { >> + err = reg; >> + goto done; >> + } >> + >> + reg |= MII_BRCM_FET_SHDW_AM4_STANDBY; >> + >> + err = phy_write(phydev, MII_BRCM_FET_SHDW_AUXMODE4, reg); > > Does the read-modify-write problem extend to this register? Why would > the PHY behave differently whether you used phy_modify() here or not? > On the mdio bus, it should be exactly the same - the only difference > is that we're guaranteed to hold the lock over the sequence whereas > this drops and re-acquires the lock. What read-modify-write problem are you referring to, that is, are you talking about my statement about setting BMCR.PDOWN only or something else? I could use phy_modify(), sure.
On 8/15/22 11:09, Florian Fainelli wrote: > On 8/15/22 11:00, Russell King (Oracle) wrote: >> On Mon, Aug 15, 2022 at 10:43:56AM -0700, Florian Fainelli wrote: >>> + /* We cannot use a read/modify/write here otherwise the PHY >>> continues >>> + * to drive LEDs which defeats the purpose of low power mode. >>> + */ >> ... >>> + /* Set standby mode */ >>> + reg = phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); >>> + if (reg < 0) { >>> + err = reg; >>> + goto done; >>> + } >>> + >>> + reg |= MII_BRCM_FET_SHDW_AM4_STANDBY; >>> + >>> + err = phy_write(phydev, MII_BRCM_FET_SHDW_AUXMODE4, reg); >> >> Does the read-modify-write problem extend to this register? Why would >> the PHY behave differently whether you used phy_modify() here or not? >> On the mdio bus, it should be exactly the same - the only difference >> is that we're guaranteed to hold the lock over the sequence whereas >> this drops and re-acquires the lock. > > What read-modify-write problem are you referring to, that is, are you > talking about my statement about setting BMCR.PDOWN only or something else? Sorry, hit send too quickly, I see what problem you are referring to. v2 coming up shortly utilizing phy_modify() and simplifying the return path (no need for done label, etc.) Thanks
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 31fbcdddc9ad..739348b3f135 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -766,6 +766,50 @@ static irqreturn_t brcm_fet_handle_interrupt(struct phy_device *phydev) return IRQ_HANDLED; } +static int brcm_fet_suspend(struct phy_device *phydev) +{ + int reg, err, err2, brcmtest; + + /* We cannot use a read/modify/write here otherwise the PHY continues + * to drive LEDs which defeats the purpose of low power mode. + */ + err = phy_write(phydev, MII_BMCR, BMCR_PDOWN); + if (err < 0) + return err; + + /* Enable shadow register access */ + brcmtest = phy_read(phydev, MII_BRCM_FET_BRCMTEST); + if (brcmtest < 0) + return brcmtest; + + reg = brcmtest | MII_BRCM_FET_BT_SRE; + + err = phy_write(phydev, MII_BRCM_FET_BRCMTEST, reg); + if (err < 0) + return err; + + /* Set standby mode */ + reg = phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); + if (reg < 0) { + err = reg; + goto done; + } + + reg |= MII_BRCM_FET_SHDW_AM4_STANDBY; + + err = phy_write(phydev, MII_BRCM_FET_SHDW_AUXMODE4, reg); + if (err < 0) + goto done; + +done: + /* Disable shadow register access */ + err2 = phy_write(phydev, MII_BRCM_FET_BRCMTEST, brcmtest); + if (!err) + err = err2; + + return err; +} + static int bcm54xx_phy_probe(struct phy_device *phydev) { struct bcm54xx_phy_priv *priv; @@ -1033,6 +1077,8 @@ static struct phy_driver broadcom_drivers[] = { .config_init = brcm_fet_config_init, .config_intr = brcm_fet_config_intr, .handle_interrupt = brcm_fet_handle_interrupt, + .suspend = brcm_fet_suspend, + .resume = brcm_fet_config_init, }, { .phy_id = PHY_ID_BCM5241, .phy_id_mask = 0xfffffff0, @@ -1041,6 +1087,8 @@ static struct phy_driver broadcom_drivers[] = { .config_init = brcm_fet_config_init, .config_intr = brcm_fet_config_intr, .handle_interrupt = brcm_fet_handle_interrupt, + .suspend = brcm_fet_suspend, + .resume = brcm_fet_config_init, }, { .phy_id = PHY_ID_BCM5395, .phy_id_mask = 0xfffffff0, diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h index 6ff567ece34a..9e77165f3ef6 100644 --- a/include/linux/brcmphy.h +++ b/include/linux/brcmphy.h @@ -293,6 +293,7 @@ #define MII_BRCM_FET_SHDW_MC_FAME 0x4000 /* Force Auto MDIX enable */ #define MII_BRCM_FET_SHDW_AUXMODE4 0x1a /* Auxiliary mode 4 */ +#define MII_BRCM_FET_SHDW_AM4_STANDBY 0x0008 /* Standby enable */ #define MII_BRCM_FET_SHDW_AM4_LED_MASK 0x0003 #define MII_BRCM_FET_SHDW_AM4_LED_MODE1 0x0001
Implement the suspend/resume procedure for the Broadcom AC131 and BCM5241 type of PHYs (10/100 only) by entering the standard power down followed by the proprietary standby mode in the auxiliary mode 4 shadow register. On resume, the PHY software reset is enough to make it come out of standby mode so we can utilize brcm_fet_config_init() as the resume hook. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/broadcom.c | 48 ++++++++++++++++++++++++++++++++++++++ include/linux/brcmphy.h | 1 + 2 files changed, 49 insertions(+)