Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, May 28, 2019 at 10:34:42AM +0100, Russell King wrote:
> Some boards do not have the PHY firmware programmed in the 3310's flash,
> which leads to the PHY not working as expected.  Warn the user when the
> PHY fails to boot the firmware and refuse to initialise.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> I think this patch needs testing with the Marvell 88x2110 PHY before
> this can be merged into mainline, but I think it should go into -rc
> and be back-ported to stable trees to avoid user frustration. I spent
> some time last night debugging one such instance, and the user
> afterwards indicated that they'd had the problem for a long time, and
> had thought of throwing the hardware out the window!  Clearly not a
> good user experience.
> 
>  drivers/net/phy/marvell10g.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 100b401b1f4a..754cde873dde 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -31,6 +31,9 @@
>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>  
>  enum {
> +	MV_PMA_BOOT		= 0xc050,
> +	MV_PMA_BOOT_FATAL	= BIT(0),
> +
>  	MV_PCS_BASE_T		= 0x0000,
>  	MV_PCS_BASE_R		= 0x1000,
>  	MV_PCS_1000BASEX	= 0x2000,
> @@ -211,6 +214,16 @@ static int mv3310_probe(struct phy_device *phydev)
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;
>  
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MV_PMA_BOOT_FATAL) {
> +		dev_warn(&phydev->mdio.dev,
> +			 "PHY failed to boot firmware, status=%04x\n", ret);
> +		return -ENODEV;

One question: are we happy with failing the probe like this, or would it
be better to allow the probe to suceed?

As has been pointed out in the C45 MII access patch, we need the PHY
to bind to the network driver for the MII bus to be accessible to
userspace, so if we're going to have userspace tools to upload the
firmware, rather than using u-boot, we need the PHY to be present and
bound to the network interface.

> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up