Messages in this thread | | | Date | Thu, 20 Jan 2022 14:46:54 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware |
| |
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.
> -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?
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?
> +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?
> + {} > +}; > + > /** > * 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.
> + > /** > * 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.
> > /* > * 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.
Andrew
| |