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

[Giulio Benetti]

Hello Andrew,

thank you for reviewing,

On 12/08/23 00:16, Andrew Lunn wrote:
> On Fri, Aug 11, 2023 at 11:53:22PM +0200, Giulio Benetti wrote:
> > This patch adds the BCM5221 PHY support by reusing
> > brcm_fet_config_intr() and brcm_fet_handle_interrupt() and
> > implementing config_init()/suspend()/resume().
> >
> > Sponsored by: Tekvox Inc.
>
> That is a new tag. Maybe you should update
> Documentation/process/submitting-patches.rst ?

I've picked this tag from commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=73c289bac05919286f8c7e1660fcaf6ec0468363

it's not "Sponsored-by:", do you think it's better using this last form?

I can send a patch along with V2 of this one for that.

> > +static int bcm5221_config_init(struct phy_device *phydev)
> > +{
> > +	int reg, err, err2, brcmtest;
> > +
> > +	/* Reset the PHY to bring it to a known state. */
> > +	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* The datasheet indicates the PHY needs up to 1us to complete a reset,
> > +	 * build some slack here.
> > +	 */
> > +	usleep_range(1000, 2000);
> > +
> > +	/* The PHY requires 65 MDC clock cycles to complete a write operation
> > +	 * and turnaround the line properly.
> > +	 *
> > +	 * We ignore -EIO here as the MDIO controller (e.g.: mdio-bcm-unimac)
> > +	 * may flag the lack of turn-around as a read failure. This is
> > +	 * particularly true with this combination since the MDIO controller
> > +	 * only used 64 MDC cycles. This is not a critical failure in this
> > +	 * specific case and it has no functional impact otherwise, so we let
> > +	 * that one go through. If there is a genuine bus error, the next read
> > +	 * of MII_BRCM_FET_INTREG will error out.
> > +	 */
> > +	err = phy_read(phydev, MII_BMCR);
> > +	if (err < 0 && err != -EIO)
> > +		return err;
>
> It is pretty normal to check the value of MII_BMCR and ensure that
> BMCR_RESET has cleared. See phy_poll_reset(). It might not be needed,
> if you trust the datasheet, but 802.3 C22 says it should clear.

oh ok, I'll do that on V2

> > +	/* Enable auto MDIX */
> > +	err = phy_clear_bits(phydev, BCM5221_AEGSR, BCM5221_AEGSR_MDIX_DIS);
> > +	if (err < 0)
> > +		return err;
>
> It is better to set it based on phydev->mdix_ctrl.

ok,

> > @@ -1288,6 +1431,7 @@ static struct mdio_device_id __maybe_unused broadcom_tbl[] = {
> >   	{ PHY_ID_BCM53125, 0xfffffff0 },
> >   	{ PHY_ID_BCM53128, 0xfffffff0 },
> >   	{ PHY_ID_BCM89610, 0xfffffff0 },
> > +	{ PHY_ID_BCM5221, 0xfffffff0 },
>
> This table has some sort of sorting. I would put this new entry before
> PHY_ID_BCM5241.

right, this and even the struct phy_driver driver entry too then,

> >   #define PHY_ID_BCM50610			0x0143bd60
> >   #define PHY_ID_BCM50610M		0x0143bd70
> >   #define PHY_ID_BCM5241			0x0143bc30
> > +#define PHY_ID_BCM5221			0x004061e0
>
> The value looks odd. Is the OUI correct? Is that a broadcom OUI?

Yes, you can check it in the pdf page 37:
https://docs.broadcom.com/doc/5221-DS07-405-RDS.pdf
and yes, it's odd.

Thanks again for the review, I'll do the changes here and the others
suggested by the other and send V2 patch.

Best regards
--
Giulio Benetti
CEO&CTO@Benetti Engineering sas