Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy

[Giulio Benetti]

Hello Russell,

thanks for reviewing,

On 12/08/23 00:22, Russell King (Oracle) wrote:
> On Fri, Aug 11, 2023 at 11:53:22PM +0200, Giulio Benetti wrote:
> > +	reg = phy_read(phydev, MII_BRCM_FET_INTREG);
> > +	if (reg < 0)
> > +		return reg;
> > +
> > +	/* Unmask events we are interested in and mask interrupts globally. */
> > +	reg = MII_BRCM_FET_IR_ENABLE |
> > +	      MII_BRCM_FET_IR_MASK;
> > +
> > +	err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
> > +	if (err < 0)
> > +		return err;
>
> Please explain why you read MII_BRCM_FET_INTREG, then discard its value
> and write a replacement value.

ok, yes, as it is it doesn't look self-explanatory at all

> > +
> > +	/* Enable auto MDIX */
> > +	err = phy_clear_bits(phydev, BCM5221_AEGSR, BCM5221_AEGSR_MDIX_DIS);
> > +	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;
>
> I think you should consider locking the MDIO bus while the device is
> switched to the shadow register set, so that other accesses don't happen
> that may interfere with this.

oh, I haven't considered this, you're totally right,

> > +static int bcm5221_suspend(struct phy_device *phydev)
> > +{
> > +	int reg, err, err2, brcmtest;
> > +
> > +	/* 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;
> > +
> > +	/* Force Low Power Mode with clock enabled */
> > +	err = phy_set_bits(phydev, MII_BRCM_FET_SHDW_AUXMODE4,
> > +			   BCM5221_SHDW_AM4_EN_CLK_LPM |
> > +			   BCM5221_SHDW_AM4_FORCE_LPM);
> > +
> > +	/* Disable shadow register access */
> > +	err2 = phy_write(phydev, MII_BRCM_FET_BRCMTEST, brcmtest);
> > +	if (!err)
> > +		err = err2;
>
> Same here.

ok,

> > +
> > +	return err;
> > +}
> > +
> > +static int bcm5221_resume(struct phy_device *phydev)
> > +{
> > +	int reg, err, err2, brcmtest;
> > +
> > +	/* 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;
> > +
> > +	/* Exit Low Power Mode with clock enabled */
> > +	err = phy_clear_bits(phydev, MII_BRCM_FET_SHDW_AUXMODE4,
> > +			     BCM5221_SHDW_AM4_FORCE_LPM);
> > +
> > +	/* Disable shadow register access */
> > +	err2 = phy_write(phydev, MII_BRCM_FET_BRCMTEST, brcmtest);
> > +	if (!err)
> > +		err = err2;
>
> And, of course, same here.

ok, will do on V2 along with the other point from Andrew and Florian.

Thanks again for the review,
Best regards
--
Giulio Benetti
CEO&CTO@Benetti Engineering sas