Messages in this thread | | | From | Kai-Heng Feng <> | Date | Fri, 21 Jan 2022 11:54:11 +0800 | Subject | Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware |
| |
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
| |