Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

On Thu, Jan 20, 2022 at 9:46 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
>
> Please split this patch into two:
>
> Don't touch the LEDs
>
> Save and restore the LED configuration over suspend/resume.

Will split into two patch for next iteration.

>
> > -static void marvell_config_led(struct phy_device *phydev)
> > +static int marvell_find_led_config(struct phy_device *phydev)
> >  {
> > -     u16 def_config;
> > -     int err;
> > +     int def_config;
> > +
> > +     if (phydev->dev_flags & PHY_USE_FIRMWARE_LED) {
> > +             def_config = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
> > +             return def_config < 0 ? -1 : def_config;
>
> What about the other two registers which configure the LEDs?

Do you mean the other two LEDs? They are not used on this machine.

>
> Since you talked about suspend/resume, does this machine support WoL?
> Is the BIOS configuring LED2 to be used as an interrupt when WoL is
> enabled in the BIOS? Do you need to save/restore that configuration
> over suspend/review? And prevent the driver from changing the
> configuration?

This NIC on the machine doesn't support WoL.

>
> > +static const struct dmi_system_id platform_flags[] = {
> > +     {
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> > +             },
> > +             .driver_data = (void *)PHY_USE_FIRMWARE_LED,
> > +     },
>
> This needs a big fat warning, that it will affect all LEDs for PHYs
> which linux is driving, on that machine. So PHYs on USB dongles, PHYs
> in SFPs, PHYs on plugin PCIe card etc.
>
> Have you talked with Dells Product Manager and do they understand the
> implications of this?

Right, that's why the original approach is passing the flag from the MAC driver.
That approach can be more specific and doesn't touch unrelated PHYs.

>
> > +     {}
> > +};
> > +
> >  /**
> >   * phy_attach_direct - attach a network device to a given PHY device pointer
> >   * @dev: network device to attach
> > @@ -1363,6 +1379,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> >       struct mii_bus *bus = phydev->mdio.bus;
> >       struct device *d = &phydev->mdio.dev;
> >       struct module *ndev_owner = NULL;
> > +     const struct dmi_system_id *dmi;
> >       bool using_genphy = false;
> >       int err;
> >
> > @@ -1443,6 +1460,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> >                       phydev_err(phydev, "error creating 'phy_standalone' sysfs entry\n");
> >       }
> >
> > +     dmi = dmi_first_match(platform_flags);
> > +     if (dmi)
> > +             phydev->dev_flags |= (u32)dmi->driver_data;
>
> Please us your new flag directly. We don't want this abused to pass
> any old flag to the PHY.

Will change it.

>
> > +
> >  /**
> >   * struct phy_device - An instance of a PHY
> >   *
> > @@ -663,6 +665,7 @@ struct phy_device {
> >
> >       struct phy_led_trigger *led_link_trigger;
> >  #endif
> > +     int led_config;
>
> You cannot put this here because you don't know how many registers are
> used to hold the configuration. Marvell has 3, other drivers can have
> other numbers. The information needs to be saved into the drivers on
> priv structure.

Ok.

>
> >
> >       /*
> >        * Interrupt number for this PHY
> > @@ -776,6 +779,12 @@ struct phy_driver {
> >        */
> >       int (*config_init)(struct phy_device *phydev);
> >
> > +     /**
> > +      * @config_led: Called to config the PHY LED,
> > +      * Use the resume flag to indicate init or resume
> > +      */
> > +     void (*config_led)(struct phy_device *phydev, bool resume);
>
> I don't see any need for this.

Ok.

Kai-Heng

>
>   Andrew