Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Aug 2023 23:22:34 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Giulio Benetti <giulio.benetti@benettiengineering.com>
Cc:     Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Florian Fainelli <florian.fainelli@broadcom.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jim Reinhart <jimr@tekvox.com>,
        James Autry <jautry@tekvox.com>,
        Matthew Maron <matthewm@tekvox.com>
Subject: Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy
Message-ID: <ZNa0qlICwsUq6H2C@shell.armlinux.org.uk>
References: <20230811215322.8679-1-giulio.benetti@benettiengineering.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230811215322.8679-1-giulio.benetti@benettiengineering.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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!