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;
Message-ID: <e627f60f-b13d-d82f-ff7c-f4b2bef21303@benettiengineering.com>
Date:   Sat, 12 Aug 2023 19:15:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy
Content-Language: en-US
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
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>
References: <20230811215322.8679-1-giulio.benetti@benettiengineering.com>
 <ZNa0qlICwsUq6H2C@shell.armlinux.org.uk>
From:   Giulio Benetti <giulio.benetti@benettiengineering.com>
In-Reply-To: <ZNa0qlICwsUq6H2C@shell.armlinux.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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