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

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:

quoted

+	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

quoted

+
+	/* 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,

quoted

+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,

quoted

+
+	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