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

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.

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

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

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

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!