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

[Marek Vasut]

On 8/11/23 23:42, Giulio Benetti wrote:
> Add BCM5221 phy support.

Why not port Linux drivers/net/sungem_phy.c instead ?
That already supports the PHY .

> Sponsored by: Tekvox Inc.
> Cc: Jim Reinhart <jimr at tekvox.com>
> Cc: James Autry <jautry at tekvox.com>
> Cc: Matthew Maron <matthewm at tekvox.com>
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
> ---
>   drivers/net/phy/broadcom.c | 99 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 36c70da181..a1996e6059 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -34,6 +34,26 @@
>   
>   #define MIIM_BCM_CHANNEL_WIDTH    0x2000
>   
> +#define MII_BCM5221_INTREG		0x1a	/* Interrupt register */
> +#define MII_BCM5221_IR_MASK		0x0100	/* Mask all interrupts */
> +#define MII_BCM5221_IR_LINK_EN		0x0200	/* Link status change enable */
> +#define MII_BCM5221_IR_SPEED_EN	0x0400	/* Link speed change enable */
> +#define MII_BCM5221_IR_DUPLEX_EN	0x0800	/* Duplex mode change enable */
> +#define MII_BCM5221_IR_ENABLE		0x4000	/* Interrupt enable */
> +
> +#define MII_BCM5221_BRCMTEST		0x1f	/* Brcm test register */
> +#define MII_BCM5221_BT_SRE		0x0080	/* Shadow register enable */
> +
> +#define MII_BCM5221_AE_GSR		0x1c    /* BCM5221 Auxiliary Error &
> +						 * General Status Register
> +						 */
> +#define MII_BCM5221_AE_GSR_DIS_MDIX	0x0800	/* BCM5221 Disable MDIX */
> +#define MII_BCM5221_SHDW_AM4_FLPM	0x0002	/* BCM5221 Force Low Power
> +						 * Mode
> +						 */
> +
> +#define MII_BCM5221_SHDW_AUXMODE4	0x1a	/* Auxiliary mode 4 */
> +
>   static void bcm_phy_write_misc(struct phy_device *phydev,
>   			       u16 reg, u16 chl, u16 value)
>   {
> @@ -311,6 +331,75 @@ static int bcm5482_startup(struct phy_device *phydev)
>   	return bcm54xx_parse_status(phydev);
>   }
>   
> +static int bcm_bcm5221_config(struct phy_device *phydev)
> +{
> +	int reg, err, err2, brcmtest;
> +
> +	phy_reset(phydev);
> +
> +	/* The datasheet indicates the PHY needs up to 1us to complete a reset,
> +	 * build some slack here.
> +	 */
> +	udelay(2000);

1 us and 2000 us is a huge difference , why such a long delay ?

> +	/* 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_BCM5221_INTREG will error out.
> +	 */

Shouldn't this be fixed on the MDIO/MAC driver level?

> +	err = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> +	if (err < 0 && err != -EIO)
> +		return err;
> +
> +	reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BCM5221_INTREG);
> +	if (reg < 0)
> +		return reg;
> +
> +	/* Mask interrupts globally since we don't use interrupt */
> +	reg = MII_BCM5221_IR_MASK;
> +
> +	err = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_INTREG, reg);
> +	if (err < 0)
> +		return err;
> +
> +	/* Enable auto MDIX */
> +	err = phy_modify(phydev, MDIO_DEVAD_NONE, MII_BCM5221_AE_GSR,
> +			 MII_BCM5221_AE_GSR_DIS_MDIX, 0);
> +	if (err < 0)
> +		return err;
> +
> +	/* Enable shadow register access */
> +	brcmtest = phy_read(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST);
> +	if (brcmtest < 0)
> +		return brcmtest;
> +
> +	reg = brcmtest | MII_BCM5221_BT_SRE;

I think it would be best to port phy_set_bits() wrapper from Linux and 
use it here. The mmd one is already ported over.

> +	err = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST, reg);
> +	if (err < 0)
> +		return err;
> +
> +	/* Exit low power mode */
> +	err = phy_modify(phydev, MDIO_DEVAD_NONE, MII_BCM5221_SHDW_AUXMODE4,
> +			 MII_BCM5221_SHDW_AM4_FLPM, 0);
> +	if (err < 0)
> +		goto done;
> +
> +done:
> +	/* Disable shadow register access */
> +	err2 = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST, brcmtest);
> +	if (!err)
> +		err = err2;

Just ignore the return value of this write here, you want to return the 
original error value anyway, and if the write here fails, it means the 
hardware just failed badly.

[...]