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

Commit Message

Kai-Heng Feng Jan. 20, 2022, 5:19 a.m. UTC

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.

Introduce config_led() callback in phy_driver() to make the implemtation
generic.

Signed-off-by: Kai-Heng Feng <[e-mail]>
---
v2:
 - Split with a new helper to find default LED config.
 - Make the patch more generic.

 drivers/net/phy/marvell.c    | 43 +++++++++++++++++++++++++++++-------
 drivers/net/phy/phy_device.c | 21 ++++++++++++++++++
 include/linux/phy.h          |  9 ++++++++
 3 files changed, 65 insertions(+), 8 deletions(-)

Comments

Heiner Kallweit Jan. 20, 2022, 7:58 a.m. UTC | #1

On 20.01.2022 06:19, 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.
> 
> Introduce config_led() callback in phy_driver() to make the implemtation
> generic.
> 
> Signed-off-by: Kai-Heng Feng <[e-mail]>
> ---
> v2:
>  - Split with a new helper to find default LED config.
>  - Make the patch more generic.
> 
>  drivers/net/phy/marvell.c    | 43 +++++++++++++++++++++++++++++-------
>  drivers/net/phy/phy_device.c | 21 ++++++++++++++++++
>  include/linux/phy.h          |  9 ++++++++
>  3 files changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 739859c0dfb18..54ee54a6895c9 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -746,10 +746,14 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
>  	return err;
>  }
>  
> -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;
> +	}
>  
>  	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
>  	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> @@ -769,20 +773,30 @@ static void marvell_config_led(struct phy_device *phydev)
>  			def_config = MII_88E1510_PHY_LED_DEF;
>  		break;
>  	default:
> -		return;
> +		return -1;
>  	}
>  
> +	return def_config;
> +}
> +
> +static void marvell_config_led(struct phy_device *phydev, bool resume)
> +{
> +	int err;
> +
> +	if (!resume)
> +		phydev->led_config = marvell_find_led_config(phydev);
> +
> +	if (phydev->led_config == -1)
> +		return;
> +
>  	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
> -			      def_config);
> +			      phydev->led_config);
>  	if (err < 0)
>  		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
>  }
>  
>  static int marvell_config_init(struct phy_device *phydev)
>  {
> -	/* Set default LED */
> -	marvell_config_led(phydev);
> -
>  	/* Set registers from marvell,reg-init DT property */
>  	return marvell_of_reg_init(phydev);
>  }
> @@ -2845,6 +2859,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_GBIT_FEATURES */
>  		.probe = marvell_probe,
>  		.config_init = marvell_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1101_config_aneg,
>  		.config_intr = marvell_config_intr,
>  		.handle_interrupt = marvell_handle_interrupt,
> @@ -2944,6 +2959,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_GBIT_FEATURES */
>  		.probe = marvell_probe,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1121_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -2965,6 +2981,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_GBIT_FEATURES */
>  		.probe = marvell_probe,
>  		.config_init = m88e1318_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1318_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3044,6 +3061,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_GBIT_FEATURES */
>  		.probe = marvell_probe,
>  		.config_init = m88e1116r_config_init,
> +		.config_led = marvell_config_led,
>  		.config_intr = marvell_config_intr,
>  		.handle_interrupt = marvell_handle_interrupt,
>  		.resume = genphy_resume,
> @@ -3065,6 +3083,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.probe = m88e1510_probe,
>  		.config_init = m88e1510_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3094,6 +3113,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.probe = marvell_probe,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3120,6 +3140,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_GBIT_FEATURES */
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3144,6 +3165,7 @@ static struct phy_driver marvell_drivers[] = {
>  		/* PHY_BASIC_FEATURES */
>  		.probe = marvell_probe,
>  		.config_init = m88e3016_config_init,
> +		.config_led = marvell_config_led,
>  		.aneg_done = marvell_aneg_done,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3165,6 +3187,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.probe = marvell_probe,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e6390_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3191,6 +3214,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.probe = marvell_probe,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e6390_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3217,6 +3241,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.flags = PHY_POLL_CABLE_TEST,
>  		.probe = marvell_probe,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3242,6 +3267,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.probe = marvell_probe,
>  		/* PHY_GBIT_FEATURES */
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> @@ -3264,6 +3290,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.probe = marvell_probe,
>  		.features = PHY_GBIT_FIBRE_FEATURES,
>  		.config_init = marvell_1011gbe_config_init,
> +		.config_led = marvell_config_led,
>  		.config_aneg = m88e1510_config_aneg,
>  		.read_status = marvell_read_status,
>  		.config_intr = marvell_config_intr,
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 74d8e1dc125f8..c9e97206aa9e8 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -12,6 +12,7 @@
>  #include <linux/acpi.h>
>  #include <linux/bitmap.h>
>  #include <linux/delay.h>
> +#include <linux/dmi.h>
>  #include <linux/errno.h>
>  #include <linux/etherdevice.h>
>  #include <linux/ethtool.h>
> @@ -1157,6 +1158,7 @@ static int phy_poll_reset(struct phy_device *phydev)
>  int phy_init_hw(struct phy_device *phydev)
>  {
>  	int ret = 0;
> +	bool resume = phydev->suspended;
>  
>  	/* Deassert the reset signal */
>  	phy_device_reset(phydev, 0);
> @@ -1184,6 +1186,9 @@ int phy_init_hw(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> +	if (phydev->drv->config_led)
> +		phydev->drv->config_led(phydev, resume);
> +
>  	if (phydev->drv->config_intr) {
>  		ret = phydev->drv->config_intr(phydev);
>  		if (ret < 0)
> @@ -1342,6 +1347,17 @@ int phy_sfp_probe(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL(phy_sfp_probe);
>  
> +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,
> +	},
> +	{}
> +};
> +
>  /**
>   * 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;
> +
>  	phydev->dev_flags |= flags;
>  
>  	phydev->interface = interface;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 6de8d7a90d78e..3a944a6564f43 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -517,6 +517,8 @@ struct phy_c45_device_ids {
>  struct macsec_context;
>  struct macsec_ops;
>  
> +#define PHY_USE_FIRMWARE_LED 0x1000000
> +
>  /**
>   * 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;
>  
>  	/*
>  	 * 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);
> +
>  	/**
>  	 * @probe: Called during discovery.  Used to set
>  	 * up device-specific structures, if any

All this looks quite hacky to me. Why do we touch the LED config at all
in the PHY driver? The current code deals with the LED Function Control
register only, for the LED Polarity Control and LED Timer Control we
rely on the boot loader anyway.
I see that previous LED-related changes like a93f7fe13454 ("net: phy:
marvell: add new default led configure for m88e151x") were committed
w/o involvement of the PHY maintainers.
Flags like MARVELL_PHY_LED0_LINK_LED1_ACTIVE I see as a workaround
because the feature as such isn't Marvell-specific. Most PHY's provide
means to configure whether LED pins are triggered by selected link speeds
and/or rx/tx activity.

Unfortunately the discussion with the LED subsystem maintainers about
how to deal best with MAC/PHY-controlled LEDs (and hw triggers in general)
didn't result in anything tangible yet. Latest attempt I'm aware of:
https://lore.kernel.org/linux-leds/[e-mail]/T/#t

Kai-Heng Feng Jan. 20, 2022, 11:40 a.m. UTC | #2

Hi Heiner,

On Thu, Jan 20, 2022 at 3:58 PM Heiner Kallweit <[e-mail]> wrote:
>
> On 20.01.2022 06:19, 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.
> >
> > Introduce config_led() callback in phy_driver() to make the implemtation
> > generic.
> >
> > Signed-off-by: Kai-Heng Feng <[e-mail]>
> > ---
> > v2:
> >  - Split with a new helper to find default LED config.
> >  - Make the patch more generic.
> >
> >  drivers/net/phy/marvell.c    | 43 +++++++++++++++++++++++++++++-------
> >  drivers/net/phy/phy_device.c | 21 ++++++++++++++++++
> >  include/linux/phy.h          |  9 ++++++++
> >  3 files changed, 65 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 739859c0dfb18..54ee54a6895c9 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -746,10 +746,14 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
> >       return err;
> >  }
> >
> > -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;
> > +     }
> >
> >       switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> >       /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> > @@ -769,20 +773,30 @@ static void marvell_config_led(struct phy_device *phydev)
> >                       def_config = MII_88E1510_PHY_LED_DEF;
> >               break;
> >       default:
> > -             return;
> > +             return -1;
> >       }
> >
> > +     return def_config;
> > +}
> > +
> > +static void marvell_config_led(struct phy_device *phydev, bool resume)
> > +{
> > +     int err;
> > +
> > +     if (!resume)
> > +             phydev->led_config = marvell_find_led_config(phydev);
> > +
> > +     if (phydev->led_config == -1)
> > +             return;
> > +
> >       err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
> > -                           def_config);
> > +                           phydev->led_config);
> >       if (err < 0)
> >               phydev_warn(phydev, "Fail to config marvell phy LED.\n");
> >  }
> >
> >  static int marvell_config_init(struct phy_device *phydev)
> >  {
> > -     /* Set default LED */
> > -     marvell_config_led(phydev);
> > -
> >       /* Set registers from marvell,reg-init DT property */
> >       return marvell_of_reg_init(phydev);
> >  }
> > @@ -2845,6 +2859,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_GBIT_FEATURES */
> >               .probe = marvell_probe,
> >               .config_init = marvell_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1101_config_aneg,
> >               .config_intr = marvell_config_intr,
> >               .handle_interrupt = marvell_handle_interrupt,
> > @@ -2944,6 +2959,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_GBIT_FEATURES */
> >               .probe = marvell_probe,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1121_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -2965,6 +2981,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_GBIT_FEATURES */
> >               .probe = marvell_probe,
> >               .config_init = m88e1318_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1318_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3044,6 +3061,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_GBIT_FEATURES */
> >               .probe = marvell_probe,
> >               .config_init = m88e1116r_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_intr = marvell_config_intr,
> >               .handle_interrupt = marvell_handle_interrupt,
> >               .resume = genphy_resume,
> > @@ -3065,6 +3083,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .probe = m88e1510_probe,
> >               .config_init = m88e1510_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3094,6 +3113,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .probe = marvell_probe,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3120,6 +3140,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_GBIT_FEATURES */
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3144,6 +3165,7 @@ static struct phy_driver marvell_drivers[] = {
> >               /* PHY_BASIC_FEATURES */
> >               .probe = marvell_probe,
> >               .config_init = m88e3016_config_init,
> > +             .config_led = marvell_config_led,
> >               .aneg_done = marvell_aneg_done,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3165,6 +3187,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .probe = marvell_probe,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e6390_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3191,6 +3214,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .probe = marvell_probe,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e6390_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3217,6 +3241,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .flags = PHY_POLL_CABLE_TEST,
> >               .probe = marvell_probe,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3242,6 +3267,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .probe = marvell_probe,
> >               /* PHY_GBIT_FEATURES */
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > @@ -3264,6 +3290,7 @@ static struct phy_driver marvell_drivers[] = {
> >               .probe = marvell_probe,
> >               .features = PHY_GBIT_FIBRE_FEATURES,
> >               .config_init = marvell_1011gbe_config_init,
> > +             .config_led = marvell_config_led,
> >               .config_aneg = m88e1510_config_aneg,
> >               .read_status = marvell_read_status,
> >               .config_intr = marvell_config_intr,
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 74d8e1dc125f8..c9e97206aa9e8 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/delay.h>
> > +#include <linux/dmi.h>
> >  #include <linux/errno.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/ethtool.h>
> > @@ -1157,6 +1158,7 @@ static int phy_poll_reset(struct phy_device *phydev)
> >  int phy_init_hw(struct phy_device *phydev)
> >  {
> >       int ret = 0;
> > +     bool resume = phydev->suspended;
> >
> >       /* Deassert the reset signal */
> >       phy_device_reset(phydev, 0);
> > @@ -1184,6 +1186,9 @@ int phy_init_hw(struct phy_device *phydev)
> >                       return ret;
> >       }
> >
> > +     if (phydev->drv->config_led)
> > +             phydev->drv->config_led(phydev, resume);
> > +
> >       if (phydev->drv->config_intr) {
> >               ret = phydev->drv->config_intr(phydev);
> >               if (ret < 0)
> > @@ -1342,6 +1347,17 @@ int phy_sfp_probe(struct phy_device *phydev,
> >  }
> >  EXPORT_SYMBOL(phy_sfp_probe);
> >
> > +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,
> > +     },
> > +     {}
> > +};
> > +
> >  /**
> >   * 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;
> > +
> >       phydev->dev_flags |= flags;
> >
> >       phydev->interface = interface;
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 6de8d7a90d78e..3a944a6564f43 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -517,6 +517,8 @@ struct phy_c45_device_ids {
> >  struct macsec_context;
> >  struct macsec_ops;
> >
> > +#define PHY_USE_FIRMWARE_LED 0x1000000
> > +
> >  /**
> >   * 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;
> >
> >       /*
> >        * 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);
> > +
> >       /**
> >        * @probe: Called during discovery.  Used to set
> >        * up device-specific structures, if any
>
> All this looks quite hacky to me. Why do we touch the LED config at all
> in the PHY driver? The current code deals with the LED Function Control
> register only, for the LED Polarity Control and LED Timer Control we
> rely on the boot loader anyway.

If it's not advised to touch LED config in the PHY driver, where
should we do it?

> I see that previous LED-related changes like a93f7fe13454 ("net: phy:
> marvell: add new default led configure for m88e151x") were committed
> w/o involvement of the PHY maintainers.
> Flags like MARVELL_PHY_LED0_LINK_LED1_ACTIVE I see as a workaround
> because the feature as such isn't Marvell-specific. Most PHY's provide
> means to configure whether LED pins are triggered by selected link speeds
> and/or rx/tx activity.

I guess that's why maintainers asked me to make the approach more generic?

>
> Unfortunately the discussion with the LED subsystem maintainers about
> how to deal best with MAC/PHY-controlled LEDs (and hw triggers in general)
> didn't result in anything tangible yet. Latest attempt I'm aware of:
> https://lore.kernel.org/linux-leds/[e-mail]/T/#t

This series is overkill for the issue I am addressing. The platform
only needs two things:
1) Use whatever LED config handed over by system firmware.
2) Restore the saved LED config on system resume.

Kai-Heng

Andrew Lunn Jan. 20, 2022, 1:46 p.m. UTC | #3

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

Andrew Lunn Jan. 20, 2022, 2:26 p.m. UTC | #4

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.
> 
> Introduce config_led() callback in phy_driver() to make the implemtation
> generic.

I'm also wondering if we need to take a step back here and get the
ACPI guys involved. I don't know much about ACPI, but shouldn't it
provide a control method to configure the PHYs LEDs?

We already have the basics for defining a PHY in ACPI. See:

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/phy.html

so you could extend this to include a method to configure the LEDs for
a specific PHY.

  Andrew

kernel test robot Jan. 20, 2022, 3:33 p.m. UTC | #5

Hi Kai-Heng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.16 next-20220120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/net-phy-marvell-Honor-phy-LED-set-by-system-firmware-on-a-Dell-hardware/20220120-132020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1d1df41c5a33359a00e919d54eaebfb789711fdc
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220120/[e-mail]/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7556c47e56e3c39c1b4ddb5a8171f943b10be919
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kai-Heng-Feng/net-phy-marvell-Honor-phy-LED-set-by-system-firmware-on-a-Dell-hardware/20220120-132020
        git checkout 7556c47e56e3c39c1b4ddb5a8171f943b10be919
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/net/phy/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

   drivers/net/phy/phy_device.c: In function 'phy_attach_direct':
>> drivers/net/phy/phy_device.c:1465:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1465 |                 phydev->dev_flags |= (u32)dmi->driver_data;
         |                                      ^


vim +1465 drivers/net/phy/phy_device.c

  1360	
  1361	/**
  1362	 * phy_attach_direct - attach a network device to a given PHY device pointer
  1363	 * @dev: network device to attach
  1364	 * @phydev: Pointer to phy_device to attach
  1365	 * @flags: PHY device's dev_flags
  1366	 * @interface: PHY device's interface
  1367	 *
  1368	 * Description: Called by drivers to attach to a particular PHY
  1369	 *     device. The phy_device is found, and properly hooked up
  1370	 *     to the phy_driver.  If no driver is attached, then a
  1371	 *     generic driver is used.  The phy_device is given a ptr to
  1372	 *     the attaching device, and given a callback for link status
  1373	 *     change.  The phy_device is returned to the attaching driver.
  1374	 *     This function takes a reference on the phy device.
  1375	 */
  1376	int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
  1377			      u32 flags, phy_interface_t interface)
  1378	{
  1379		struct mii_bus *bus = phydev->mdio.bus;
  1380		struct device *d = &phydev->mdio.dev;
  1381		struct module *ndev_owner = NULL;
  1382		const struct dmi_system_id *dmi;
  1383		bool using_genphy = false;
  1384		int err;
  1385	
  1386		/* For Ethernet device drivers that register their own MDIO bus, we
  1387		 * will have bus->owner match ndev_mod, so we do not want to increment
  1388		 * our own module->refcnt here, otherwise we would not be able to
  1389		 * unload later on.
  1390		 */
  1391		if (dev)
  1392			ndev_owner = dev->dev.parent->driver->owner;
  1393		if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
  1394			phydev_err(phydev, "failed to get the bus module\n");
  1395			return -EIO;
  1396		}
  1397	
  1398		get_device(d);
  1399	
  1400		/* Assume that if there is no driver, that it doesn't
  1401		 * exist, and we should use the genphy driver.
  1402		 */
  1403		if (!d->driver) {
  1404			if (phydev->is_c45)
  1405				d->driver = &genphy_c45_driver.mdiodrv.driver;
  1406			else
  1407				d->driver = &genphy_driver.mdiodrv.driver;
  1408	
  1409			using_genphy = true;
  1410		}
  1411	
  1412		if (!try_module_get(d->driver->owner)) {
  1413			phydev_err(phydev, "failed to get the device driver module\n");
  1414			err = -EIO;
  1415			goto error_put_device;
  1416		}
  1417	
  1418		if (using_genphy) {
  1419			err = d->driver->probe(d);
  1420			if (err >= 0)
  1421				err = device_bind_driver(d);
  1422	
  1423			if (err)
  1424				goto error_module_put;
  1425		}
  1426	
  1427		if (phydev->attached_dev) {
  1428			dev_err(&dev->dev, "PHY already attached\n");
  1429			err = -EBUSY;
  1430			goto error;
  1431		}
  1432	
  1433		phydev->phy_link_change = phy_link_change;
  1434		if (dev) {
  1435			phydev->attached_dev = dev;
  1436			dev->phydev = phydev;
  1437	
  1438			if (phydev->sfp_bus_attached)
  1439				dev->sfp_bus = phydev->sfp_bus;
  1440			else if (dev->sfp_bus)
  1441				phydev->is_on_sfp_module = true;
  1442		}
  1443	
  1444		/* Some Ethernet drivers try to connect to a PHY device before
  1445		 * calling register_netdevice() -> netdev_register_kobject() and
  1446		 * does the dev->dev.kobj initialization. Here we only check for
  1447		 * success which indicates that the network device kobject is
  1448		 * ready. Once we do that we still need to keep track of whether
  1449		 * links were successfully set up or not for phy_detach() to
  1450		 * remove them accordingly.
  1451		 */
  1452		phydev->sysfs_links = false;
  1453	
  1454		phy_sysfs_create_links(phydev);
  1455	
  1456		if (!phydev->attached_dev) {
  1457			err = sysfs_create_file(&phydev->mdio.dev.kobj,
  1458						&dev_attr_phy_standalone.attr);
  1459			if (err)
  1460				phydev_err(phydev, "error creating 'phy_standalone' sysfs entry\n");
  1461		}
  1462	
  1463		dmi = dmi_first_match(platform_flags);
  1464		if (dmi)
> 1465			phydev->dev_flags |= (u32)dmi->driver_data;
  1466	
  1467		phydev->dev_flags |= flags;
  1468	
  1469		phydev->interface = interface;
  1470	
  1471		phydev->state = PHY_READY;
  1472	
  1473		/* Port is set to PORT_TP by default and the actual PHY driver will set
  1474		 * it to different value depending on the PHY configuration. If we have
  1475		 * the generic PHY driver we can't figure it out, thus set the old
  1476		 * legacy PORT_MII value.
  1477		 */
  1478		if (using_genphy)
  1479			phydev->port = PORT_MII;
  1480	
  1481		/* Initial carrier state is off as the phy is about to be
  1482		 * (re)initialized.
  1483		 */
  1484		if (dev)
  1485			netif_carrier_off(phydev->attached_dev);
  1486	
  1487		/* Do initial configuration here, now that
  1488		 * we have certain key parameters
  1489		 * (dev_flags and interface)
  1490		 */
  1491		err = phy_init_hw(phydev);
  1492		if (err)
  1493			goto error;
  1494	
  1495		err = phy_disable_interrupts(phydev);
  1496		if (err)
  1497			return err;
  1498	
  1499		phy_resume(phydev);
  1500		phy_led_triggers_register(phydev);
  1501	
  1502		return err;
  1503	
  1504	error:
  1505		/* phy_detach() does all of the cleanup below */
  1506		phy_detach(phydev);
  1507		return err;
  1508	
  1509	error_module_put:
  1510		module_put(d->driver->owner);
  1511	error_put_device:
  1512		put_device(d);
  1513		if (ndev_owner != bus->owner)
  1514			module_put(bus->owner);
  1515		return err;
  1516	}
  1517	EXPORT_SYMBOL(phy_attach_direct);
  1518	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Kai-Heng Feng Jan. 21, 2022, 3:54 a.m. UTC | #6

On Thu, Jan 20, 2022 at 9:46 PM Andrew Lunn <[e-mail]> 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

Kai-Heng Feng Jan. 21, 2022, 4:01 a.m. UTC | #7

On Thu, Jan 20, 2022 at 10:26 PM Andrew Lunn <[e-mail]> 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.
> >
> > Introduce config_led() callback in phy_driver() to make the implemtation
> > generic.
>
> I'm also wondering if we need to take a step back here and get the
> ACPI guys involved. I don't know much about ACPI, but shouldn't it
> provide a control method to configure the PHYs LEDs?
>
> We already have the basics for defining a PHY in ACPI. See:
>
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/phy.html

These properties seem to come from device-tree.

>
> so you could extend this to include a method to configure the LEDs for
> a specific PHY.

How to add new properties? Is it required to add new properties to
both DT and ACPI?
Looks like many drivers use _DSD freely, but those properties are not
defined in ACPI spec...

Kai-Heng

>
>   Andrew

kernel test robot Jan. 21, 2022, 6:47 a.m. UTC | #8

Hi Kai-Heng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.16 next-20220121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/net-phy-marvell-Honor-phy-LED-set-by-system-firmware-on-a-Dell-hardware/20220120-132020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1d1df41c5a33359a00e919d54eaebfb789711fdc
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220121/[e-mail]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project f7b7138a62648f4019c55e4671682af1f851f295)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7556c47e56e3c39c1b4ddb5a8171f943b10be919
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kai-Heng-Feng/net-phy-marvell-Honor-phy-LED-set-by-system-firmware-on-a-Dell-hardware/20220120-132020
        git checkout 7556c47e56e3c39c1b4ddb5a8171f943b10be919
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/net/phy/phy_device.c:1465:24: warning: cast to smaller integer type 'u32' (aka 'unsigned int') from 'void *' [-Wvoid-pointer-to-int-cast]
                   phydev->dev_flags |= (u32)dmi->driver_data;
                                        ^~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +1465 drivers/net/phy/phy_device.c

  1360	
  1361	/**
  1362	 * phy_attach_direct - attach a network device to a given PHY device pointer
  1363	 * @dev: network device to attach
  1364	 * @phydev: Pointer to phy_device to attach
  1365	 * @flags: PHY device's dev_flags
  1366	 * @interface: PHY device's interface
  1367	 *
  1368	 * Description: Called by drivers to attach to a particular PHY
  1369	 *     device. The phy_device is found, and properly hooked up
  1370	 *     to the phy_driver.  If no driver is attached, then a
  1371	 *     generic driver is used.  The phy_device is given a ptr to
  1372	 *     the attaching device, and given a callback for link status
  1373	 *     change.  The phy_device is returned to the attaching driver.
  1374	 *     This function takes a reference on the phy device.
  1375	 */
  1376	int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
  1377			      u32 flags, phy_interface_t interface)
  1378	{
  1379		struct mii_bus *bus = phydev->mdio.bus;
  1380		struct device *d = &phydev->mdio.dev;
  1381		struct module *ndev_owner = NULL;
  1382		const struct dmi_system_id *dmi;
  1383		bool using_genphy = false;
  1384		int err;
  1385	
  1386		/* For Ethernet device drivers that register their own MDIO bus, we
  1387		 * will have bus->owner match ndev_mod, so we do not want to increment
  1388		 * our own module->refcnt here, otherwise we would not be able to
  1389		 * unload later on.
  1390		 */
  1391		if (dev)
  1392			ndev_owner = dev->dev.parent->driver->owner;
  1393		if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
  1394			phydev_err(phydev, "failed to get the bus module\n");
  1395			return -EIO;
  1396		}
  1397	
  1398		get_device(d);
  1399	
  1400		/* Assume that if there is no driver, that it doesn't
  1401		 * exist, and we should use the genphy driver.
  1402		 */
  1403		if (!d->driver) {
  1404			if (phydev->is_c45)
  1405				d->driver = &genphy_c45_driver.mdiodrv.driver;
  1406			else
  1407				d->driver = &genphy_driver.mdiodrv.driver;
  1408	
  1409			using_genphy = true;
  1410		}
  1411	
  1412		if (!try_module_get(d->driver->owner)) {
  1413			phydev_err(phydev, "failed to get the device driver module\n");
  1414			err = -EIO;
  1415			goto error_put_device;
  1416		}
  1417	
  1418		if (using_genphy) {
  1419			err = d->driver->probe(d);
  1420			if (err >= 0)
  1421				err = device_bind_driver(d);
  1422	
  1423			if (err)
  1424				goto error_module_put;
  1425		}
  1426	
  1427		if (phydev->attached_dev) {
  1428			dev_err(&dev->dev, "PHY already attached\n");
  1429			err = -EBUSY;
  1430			goto error;
  1431		}
  1432	
  1433		phydev->phy_link_change = phy_link_change;
  1434		if (dev) {
  1435			phydev->attached_dev = dev;
  1436			dev->phydev = phydev;
  1437	
  1438			if (phydev->sfp_bus_attached)
  1439				dev->sfp_bus = phydev->sfp_bus;
  1440			else if (dev->sfp_bus)
  1441				phydev->is_on_sfp_module = true;
  1442		}
  1443	
  1444		/* Some Ethernet drivers try to connect to a PHY device before
  1445		 * calling register_netdevice() -> netdev_register_kobject() and
  1446		 * does the dev->dev.kobj initialization. Here we only check for
  1447		 * success which indicates that the network device kobject is
  1448		 * ready. Once we do that we still need to keep track of whether
  1449		 * links were successfully set up or not for phy_detach() to
  1450		 * remove them accordingly.
  1451		 */
  1452		phydev->sysfs_links = false;
  1453	
  1454		phy_sysfs_create_links(phydev);
  1455	
  1456		if (!phydev->attached_dev) {
  1457			err = sysfs_create_file(&phydev->mdio.dev.kobj,
  1458						&dev_attr_phy_standalone.attr);
  1459			if (err)
  1460				phydev_err(phydev, "error creating 'phy_standalone' sysfs entry\n");
  1461		}
  1462	
  1463		dmi = dmi_first_match(platform_flags);
  1464		if (dmi)
> 1465			phydev->dev_flags |= (u32)dmi->driver_data;
  1466	
  1467		phydev->dev_flags |= flags;
  1468	
  1469		phydev->interface = interface;
  1470	
  1471		phydev->state = PHY_READY;
  1472	
  1473		/* Port is set to PORT_TP by default and the actual PHY driver will set
  1474		 * it to different value depending on the PHY configuration. If we have
  1475		 * the generic PHY driver we can't figure it out, thus set the old
  1476		 * legacy PORT_MII value.
  1477		 */
  1478		if (using_genphy)
  1479			phydev->port = PORT_MII;
  1480	
  1481		/* Initial carrier state is off as the phy is about to be
  1482		 * (re)initialized.
  1483		 */
  1484		if (dev)
  1485			netif_carrier_off(phydev->attached_dev);
  1486	
  1487		/* Do initial configuration here, now that
  1488		 * we have certain key parameters
  1489		 * (dev_flags and interface)
  1490		 */
  1491		err = phy_init_hw(phydev);
  1492		if (err)
  1493			goto error;
  1494	
  1495		err = phy_disable_interrupts(phydev);
  1496		if (err)
  1497			return err;
  1498	
  1499		phy_resume(phydev);
  1500		phy_led_triggers_register(phydev);
  1501	
  1502		return err;
  1503	
  1504	error:
  1505		/* phy_detach() does all of the cleanup below */
  1506		phy_detach(phydev);
  1507		return err;
  1508	
  1509	error_module_put:
  1510		module_put(d->driver->owner);
  1511	error_put_device:
  1512		put_device(d);
  1513		if (ndev_owner != bus->owner)
  1514			module_put(bus->owner);
  1515		return err;
  1516	}
  1517	EXPORT_SYMBOL(phy_attach_direct);
  1518	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Heiner Kallweit Jan. 21, 2022, 7:49 a.m. UTC | #9

On 20.01.2022 14:46, Andrew Lunn 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.
> 
>> -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

I had a look at the history of LED settings in the Marvell PHY driver:

In marvell_config_aneg() we do
phy_write(phydev, MII_M1111_PHY_LED_CONTROL, MII_M1111_PHY_LED_DIRECT);

This originates from 2007: 76884679c644 ("phylib: Add support for Marvell 88e1111S and 88e1145")
and sets the LED control to the reset default (for no obvious reason).
Especially strange is that this is done in config_aneg.
Only non-LED bit here is bit 11 that forces the interrupt pin to assert.

Simply wrong is that register MII_M1111_PHY_LED_CONTROL (reg 24, page 0)
is written also on other chip versions (like 88E1112) where it's not
defined and marked as reserved.
I think we should remove this code.

Then we set some LED defaults in marvell_config_led().
This also originates from > 10yrs ago:
140bc9290328 ("phylib: Basic support for the M88E1121R Marvell chip")
Again there's no obvious reason.

Last but not least we have a93f7fe13454 ("net: phy: marvell: add new default led configure for m88e151x")
This adds a flag to set the PHY LED mode from hns3 MAC driver.
Intention of this patch is to set the LED mode for specific boards.
The chosen approach doesn't seem to be the best. As it's meant to be
board-specific maybe better the reg-init DT property would have been
used.

I'd say we can remove all LED config code and accept whatever boot
loader or BIOS set.

Heiner

Andrew Lunn Jan. 21, 2022, 1:04 p.m. UTC | #10

> > 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.

I'm surprised about that. Are you really sure?

What are you doing for resume? pressing the power button?

> > > +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.

More specific, but still will go wrong at some point, A PCEe card
using that MAC etc. And this is general infrastructure you are adding
here, it can be used by any machine, any combination of MAC and PHY
etc. So you need to clearly document its limits so others are not
surprised.

	Andrew

Andrew Lunn Jan. 21, 2022, 1:10 p.m. UTC | #11

> > > -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.

Have you read the datasheet for the PHY? It has three registers for
configuring the LEDs. There is one register which controls the blink
mode, a register which controls polarity, and a third register which
controls how long each blink lasts, and interrupts. If you are going
to save the configuration over suspend/resume you probably need to
save all three.

This last register is also important for WoL, which is why i asked
about it.

      Andrew

Andrew Lunn Jan. 21, 2022, 1:22 p.m. UTC | #12

On Fri, Jan 21, 2022 at 12:01:35PM +0800, Kai-Heng Feng wrote:
> On Thu, Jan 20, 2022 at 10:26 PM Andrew Lunn <[e-mail]> 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.
> > >
> > > Introduce config_led() callback in phy_driver() to make the implemtation
> > > generic.
> >
> > I'm also wondering if we need to take a step back here and get the
> > ACPI guys involved. I don't know much about ACPI, but shouldn't it
> > provide a control method to configure the PHYs LEDs?
> >
> > We already have the basics for defining a PHY in ACPI. See:
> >
> > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/phy.html
> 
> These properties seem to come from device-tree.

They are similar to what DT has, but expressed in an ACPI way. DT has
been used with PHY drivers for a long time, but ACPI is new. The ACPI
standard also says nothing about PHYs. So Linux has defined its own
properties, which we expect all ACPI machine to use. According to the
ACPI maintainers, this is within the ACPI standard. Maybe at some
point somebody will submit the current definitions to the standards
body for approval, or maybe the standard will do something completely
different, but for the moment, this is what we have, and what you
should use.

> > so you could extend this to include a method to configure the LEDs for
> > a specific PHY.
> 
> How to add new properties? Is it required to add new properties to
> both DT and ACPI?

Since all you are adding is a boolean, 'Don't touch the PHY LED
configuration', it should be easy to do for both.

What is interesting for Marvell PHYs is WoL, which is part of LED
configuration. I've not checked, but i guess there are other PHYs
which reuse LED output for a WoL interrupt. So it needs to be clearly
defined if we expect the BIOS to also correctly configure WoL, or if
Linux is responsible for configuring WoL, even though it means
changing the LED configuration.

	 Andrew

Kai-Heng Feng Jan. 21, 2022, 2:04 p.m. UTC | #13

On Fri, Jan 21, 2022 at 9:05 PM Andrew Lunn <[e-mail]> wrote:
>
> > > 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.
>
> I'm surprised about that. Are you really sure?

Yes I am sure.

>
> What are you doing for resume? pressing the power button?

Power button, RTC. The system has another igb NIC, which supports 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.
>
> More specific, but still will go wrong at some point, A PCEe card
> using that MAC etc. And this is general infrastructure you are adding
> here, it can be used by any machine, any combination of MAC and PHY
> etc. So you need to clearly document its limits so others are not
> surprised.

The dwmac-intel device is an integrated end point connects directly to
the host bridge, so it won't be in a form of PCIe addin card.

Kai-Heng

>
>         Andrew

Kai-Heng Feng Jan. 21, 2022, 2:06 p.m. UTC | #14

On Fri, Jan 21, 2022 at 9:10 PM Andrew Lunn <[e-mail]> wrote:
>
> > > > -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.
>
> Have you read the datasheet for the PHY? It has three registers for
> configuring the LEDs. There is one register which controls the blink
> mode, a register which controls polarity, and a third register which
> controls how long each blink lasts, and interrupts. If you are going
> to save the configuration over suspend/resume you probably need to
> save all three.

OK, will change it in next iteration.

>
> This last register is also important for WoL, which is why i asked
> about it.

The Marvell PHY on the system doesn't support WoL.

Kai-Heng

>
>       Andrew

Kai-Heng Feng Jan. 21, 2022, 2:17 p.m. UTC | #15

On Fri, Jan 21, 2022 at 9:22 PM Andrew Lunn <[e-mail]> wrote:
>
> On Fri, Jan 21, 2022 at 12:01:35PM +0800, Kai-Heng Feng wrote:
> > On Thu, Jan 20, 2022 at 10:26 PM Andrew Lunn <[e-mail]> 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.
> > > >
> > > > Introduce config_led() callback in phy_driver() to make the implemtation
> > > > generic.
> > >
> > > I'm also wondering if we need to take a step back here and get the
> > > ACPI guys involved. I don't know much about ACPI, but shouldn't it
> > > provide a control method to configure the PHYs LEDs?
> > >
> > > We already have the basics for defining a PHY in ACPI. See:
> > >
> > > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/phy.html
> >
> > These properties seem to come from device-tree.
>
> They are similar to what DT has, but expressed in an ACPI way. DT has
> been used with PHY drivers for a long time, but ACPI is new. The ACPI
> standard also says nothing about PHYs. So Linux has defined its own
> properties, which we expect all ACPI machine to use. According to the
> ACPI maintainers, this is within the ACPI standard. Maybe at some
> point somebody will submit the current definitions to the standards
> body for approval, or maybe the standard will do something completely
> different, but for the moment, this is what we have, and what you
> should use.

Right, so we can add a new property, document it, and just use it?
Maybe others will use the new property once we set the precedence?

>
> > > so you could extend this to include a method to configure the LEDs for
> > > a specific PHY.
> >
> > How to add new properties? Is it required to add new properties to
> > both DT and ACPI?
>
> Since all you are adding is a boolean, 'Don't touch the PHY LED
> configuration', it should be easy to do for both.

If adding a brand new property is acceptable, let me discuss it the vendor.

>
> What is interesting for Marvell PHYs is WoL, which is part of LED
> configuration. I've not checked, but i guess there are other PHYs
> which reuse LED output for a WoL interrupt. So it needs to be clearly
> defined if we expect the BIOS to also correctly configure WoL, or if
> Linux is responsible for configuring WoL, even though it means
> changing the LED configuration.

How about what Heiner proposed? Maybe we should leave the LED as is,
and restore it on system resume?

Kai-Heng

>
>          Andrew

Andrew Lunn Jan. 21, 2022, 3:05 p.m. UTC | #16

> The dwmac-intel device is an integrated end point connects directly to
> the host bridge, so it won't be in a form of PCIe addin card.

But is there anything stopping the owner adding another PCIe Ethernet
card?

Please remember, you are not adding a solution here for one specific
machine, you are adding a general infrastructure which any machine can
use, for any MAC/PHY combination. It simply does not scale adding
hacks for random machines. So you always need to keep the big picture
in mind.

	Andrew

Andrew Lunn Jan. 21, 2022, 3:08 p.m. UTC | #17

> The Marvell PHY on the system doesn't support WoL.

Not technically correct. The PHY does, the way the PHY has been
integrated into the system does not.

But again, you need to think of the general case. Somebody else wants
to make use of this feature of not touching the LED configuration, but
does have a system were WoL works. What does that imply?

     Andrew

Andrew Lunn Jan. 21, 2022, 3:15 p.m. UTC | #18

> > They are similar to what DT has, but expressed in an ACPI way. DT has
> > been used with PHY drivers for a long time, but ACPI is new. The ACPI
> > standard also says nothing about PHYs. So Linux has defined its own
> > properties, which we expect all ACPI machine to use. According to the
> > ACPI maintainers, this is within the ACPI standard. Maybe at some
> > point somebody will submit the current definitions to the standards
> > body for approval, or maybe the standard will do something completely
> > different, but for the moment, this is what we have, and what you
> > should use.
> 
> Right, so we can add a new property, document it, and just use it?

Yes. So long as you follow the scheme documented there, cleanly
integrate it into the code as needed, you can add a new property.

> Maybe others will use the new property once we set the precedence?

Yes, which is why i keep saying you need to think of the general case,
not your specific machine.

> How about what Heiner proposed? Maybe we should leave the LED as is,
> and restore it on system resume?

I don't think we can change the current code because it will cause
regressions. The LEDs probably work on some boards because of the
current code.

At some point in the future, we hope to be able to control the PHY
LEDs via /sys/class/LEDs. But until then, telling the PHY driver to
not touch the LED configuration seems a reasonable request.

    Andrew

Heiner Kallweit Jan. 22, 2022, 7:13 p.m. UTC | #19

On 21.01.2022 16:15, Andrew Lunn wrote:
>>> They are similar to what DT has, but expressed in an ACPI way. DT has
>>> been used with PHY drivers for a long time, but ACPI is new. The ACPI
>>> standard also says nothing about PHYs. So Linux has defined its own
>>> properties, which we expect all ACPI machine to use. According to the
>>> ACPI maintainers, this is within the ACPI standard. Maybe at some
>>> point somebody will submit the current definitions to the standards
>>> body for approval, or maybe the standard will do something completely
>>> different, but for the moment, this is what we have, and what you
>>> should use.
>>
>> Right, so we can add a new property, document it, and just use it?
> 
> Yes. So long as you follow the scheme documented there, cleanly
> integrate it into the code as needed, you can add a new property.
> 
>> Maybe others will use the new property once we set the precedence?
> 
> Yes, which is why i keep saying you need to think of the general case,
> not your specific machine.
> 
>> How about what Heiner proposed? Maybe we should leave the LED as is,
>> and restore it on system resume?
> 
> I don't think we can change the current code because it will cause
> regressions. The LEDs probably work on some boards because of the
> current code.
> 
One more idea:
The hw reset default for register 16 is 0x101e. If the current value
is different when entering config_init then we could preserve it
because intentionally a specific value has been set.
Only if we find the hw reset default we'd set the values according
to the current code.

> At some point in the future, we hope to be able to control the PHY
> LEDs via /sys/class/LEDs. But until then, telling the PHY driver to
> not touch the LED configuration seems a reasonable request.
> 
>     Andrew
Heiner

Andrew Lunn Jan. 22, 2022, 9:18 p.m. UTC | #20

> One more idea:
> The hw reset default for register 16 is 0x101e. If the current value
> is different when entering config_init then we could preserve it
> because intentionally a specific value has been set.
> Only if we find the hw reset default we'd set the values according
> to the current code.

We can split the problem into two.

1) I think saving LED configuration over suspend/resume is not an
issue. It is probably something we will be needed if we ever get PHY
LED configuration via sys/class/leds.

2) Knowing something else has configured the LEDs and the Linux driver
should not touch it. In general, Linux tries not to trust the
bootloader, because experience has shown bad things can happen when
you do. We cannot tell if the LED configuration is different to
defaults because something has deliberately set it, or it is just
messed up, maybe from the previous boot/kexec, maybe by the
bootloader. Even this Dell system BIOS gets it wrong, it configures
the LED on power on, but not resume !?!?!. And what about reboot?

So i really would like the bootloader to explicitly say it has
configured the LEDs and it takes full responsibility for any and all
bad behaviour as a result.

	  Andrew

Kai-Heng Feng Feb. 14, 2022, 5:40 a.m. UTC | #21

On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <[e-mail]> wrote:
>
> > One more idea:
> > The hw reset default for register 16 is 0x101e. If the current value
> > is different when entering config_init then we could preserve it
> > because intentionally a specific value has been set.
> > Only if we find the hw reset default we'd set the values according
> > to the current code.
>
> We can split the problem into two.
>
> 1) I think saving LED configuration over suspend/resume is not an
> issue. It is probably something we will be needed if we ever get PHY
> LED configuration via sys/class/leds.
>
> 2) Knowing something else has configured the LEDs and the Linux driver
> should not touch it. In general, Linux tries not to trust the
> bootloader, because experience has shown bad things can happen when
> you do. We cannot tell if the LED configuration is different to
> defaults because something has deliberately set it, or it is just
> messed up, maybe from the previous boot/kexec, maybe by the
> bootloader. Even this Dell system BIOS gets it wrong, it configures
> the LED on power on, but not resume !?!?!. And what about reboot?

The LED will be reconfigured correctly after each reboot.
The platform firmware folks doesn't want to restore the value on
resume because the Windows driver already does that. They are afraid
it may cause regression if firmware does the same thing.

>
> So i really would like the bootloader to explicitly say it has
> configured the LEDs and it takes full responsibility for any and all
> bad behaviour as a result.

This is an ACPI based platform and we are working on new firmware
property "use-firmware-led" to give driver a hint:
...
    Scope (_SB.PC00.OTN0)
    {
        Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
        {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
            Package (0x01)
            {
                Package (0x02)
                {
                    "use-firmware-led",
                    One
                }
            }
        })
    }
...

Because the property is under PCI device namespace, I am not sure how
to (cleanly) bring the property from the phylink side to phydev side.
Do you have any suggestion?

Kai-Heng

>
>           Andrew

Andrew Lunn Feb. 15, 2022, 8:27 p.m. UTC | #22

On Mon, Feb 14, 2022 at 01:40:43PM +0800, Kai-Heng Feng wrote:
> On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <[e-mail]> wrote:
> >
> > > One more idea:
> > > The hw reset default for register 16 is 0x101e. If the current value
> > > is different when entering config_init then we could preserve it
> > > because intentionally a specific value has been set.
> > > Only if we find the hw reset default we'd set the values according
> > > to the current code.
> >
> > We can split the problem into two.
> >
> > 1) I think saving LED configuration over suspend/resume is not an
> > issue. It is probably something we will be needed if we ever get PHY
> > LED configuration via sys/class/leds.
> >
> > 2) Knowing something else has configured the LEDs and the Linux driver
> > should not touch it. In general, Linux tries not to trust the
> > bootloader, because experience has shown bad things can happen when
> > you do. We cannot tell if the LED configuration is different to
> > defaults because something has deliberately set it, or it is just
> > messed up, maybe from the previous boot/kexec, maybe by the
> > bootloader. Even this Dell system BIOS gets it wrong, it configures
> > the LED on power on, but not resume !?!?!. And what about reboot?
> 
> The LED will be reconfigured correctly after each reboot.
> The platform firmware folks doesn't want to restore the value on
> resume because the Windows driver already does that. They are afraid
> it may cause regression if firmware does the same thing.

How can it cause regressions? Why would the Windows driver decide that
if the PHY already has the correct configuration is should mess it all
up? Have you looked at the sources and check what it does?

Anyway, we said that we need to save and restore the LED configuration
over suspend/resume because at some point in the maybe distant future,
we are going to support user configuration of the LEDs via
/sys/class/leds. So you can add the needed support to the PHY driver.

> This is an ACPI based platform and we are working on new firmware
> property "use-firmware-led" to give driver a hint:
> ...
>     Scope (_SB.PC00.OTN0)
>     {
>         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>         {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
> Properties for _DSD */,
>             Package (0x01)
>             {
>                 Package (0x02)
>                 {
>                     "use-firmware-led",
>                     One
>                 }
>             }
>         })
>     }
> ...
> 
> Because the property is under PCI device namespace, I am not sure how
> to (cleanly) bring the property from the phylink side to phydev side.
> Do you have any suggestion?

I'm no ACPI expert, but i think
Documentation/firmware-guide/acpi/dsd/phy.rst gives you the basis:

    During the MDIO bus driver initialization, PHYs on this bus are probed
    using the _ADR object as shown below and are registered on the MDIO bus.

      Scope(\_SB.MDI0)
      {
        Device(PHY1) {
          Name (_ADR, 0x1)
        } // end of PHY1

        Device(PHY2) {
          Name (_ADR, 0x2)
        } // end of PHY2
      }

These are the PHYs on the MDIO bus. I _think_ that next to the Name,
you can add additional properties, like your "use-firmware-led". This
would then be very similar to DT, which is in effect what ACPI is
copying. So you need to update this document with your new property,
making it clear that this property only applies to boot, not
suspend/resume. And fwnode_mdiobus_register_phy() can look for the
property and set a flag in the phydev structure indicating that ACPI
is totally responsible for LEDs at boot time.

	Andrew

Kai-Heng Feng Feb. 16, 2022, 2:30 a.m. UTC | #23

On Wed, Feb 16, 2022 at 4:27 AM Andrew Lunn <[e-mail]> wrote:
>
> On Mon, Feb 14, 2022 at 01:40:43PM +0800, Kai-Heng Feng wrote:
> > On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <[e-mail]> wrote:
> > >
> > > > One more idea:
> > > > The hw reset default for register 16 is 0x101e. If the current value
> > > > is different when entering config_init then we could preserve it
> > > > because intentionally a specific value has been set.
> > > > Only if we find the hw reset default we'd set the values according
> > > > to the current code.
> > >
> > > We can split the problem into two.
> > >
> > > 1) I think saving LED configuration over suspend/resume is not an
> > > issue. It is probably something we will be needed if we ever get PHY
> > > LED configuration via sys/class/leds.
> > >
> > > 2) Knowing something else has configured the LEDs and the Linux driver
> > > should not touch it. In general, Linux tries not to trust the
> > > bootloader, because experience has shown bad things can happen when
> > > you do. We cannot tell if the LED configuration is different to
> > > defaults because something has deliberately set it, or it is just
> > > messed up, maybe from the previous boot/kexec, maybe by the
> > > bootloader. Even this Dell system BIOS gets it wrong, it configures
> > > the LED on power on, but not resume !?!?!. And what about reboot?
> >
> > The LED will be reconfigured correctly after each reboot.
> > The platform firmware folks doesn't want to restore the value on
> > resume because the Windows driver already does that. They are afraid
> > it may cause regression if firmware does the same thing.
>
> How can it cause regressions? Why would the Windows driver decide that
> if the PHY already has the correct configuration is should mess it all
> up? Have you looked at the sources and check what it does?

Unfortunately I don't and won't have access to the driver source for Windows.

>
> Anyway, we said that we need to save and restore the LED configuration
> over suspend/resume because at some point in the maybe distant future,
> we are going to support user configuration of the LEDs via
> /sys/class/leds. So you can add the needed support to the PHY driver.

OK.

>
> > This is an ACPI based platform and we are working on new firmware
> > property "use-firmware-led" to give driver a hint:
> > ...
> >     Scope (_SB.PC00.OTN0)
> >     {
> >         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
> >         {
> >             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
> > Properties for _DSD */,
> >             Package (0x01)
> >             {
> >                 Package (0x02)
> >                 {
> >                     "use-firmware-led",
> >                     One
> >                 }
> >             }
> >         })
> >     }
> > ...
> >
> > Because the property is under PCI device namespace, I am not sure how
> > to (cleanly) bring the property from the phylink side to phydev side.
> > Do you have any suggestion?
>
> I'm no ACPI expert, but i think
> Documentation/firmware-guide/acpi/dsd/phy.rst gives you the basis:
>
>     During the MDIO bus driver initialization, PHYs on this bus are probed
>     using the _ADR object as shown below and are registered on the MDIO bus.
>
>       Scope(\_SB.MDI0)
>       {
>         Device(PHY1) {
>           Name (_ADR, 0x1)
>         } // end of PHY1
>
>         Device(PHY2) {
>           Name (_ADR, 0x2)
>         } // end of PHY2
>       }
>
> These are the PHYs on the MDIO bus. I _think_ that next to the Name,
> you can add additional properties, like your "use-firmware-led". This
> would then be very similar to DT, which is in effect what ACPI is
> copying. So you need to update this document with your new property,
> making it clear that this property only applies to boot, not
> suspend/resume. And fwnode_mdiobus_register_phy() can look for the
> property and set a flag in the phydev structure indicating that ACPI
> is totally responsible for LEDs at boot time.

The problem here is there's no MDIO bus in ACPI namespace, namely no
"Scope(\_SB.MDI0)" on this platform.

Since the phydev doesn't have a fwnode, the new property needs to be
passed from phylink to phydev, and right now I can't find a clean way
to do it.

Kai-Heng

>
>         Andrew

Andrew Lunn Feb. 16, 2022, 7:39 a.m. UTC | #24

> > > This is an ACPI based platform and we are working on new firmware
> > > property "use-firmware-led" to give driver a hint:
> > > ...
> > >     Scope (_SB.PC00.OTN0)
> > >     {
> > >         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
> > >         {
> > >             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
> > > Properties for _DSD */,
> > >             Package (0x01)
> > >             {
> > >                 Package (0x02)
> > >                 {
> > >                     "use-firmware-led",
> > >                     One
> > >                 }
> > >             }
> > >         })
> > >     }
> > > ...
> > >
> > > Because the property is under PCI device namespace, I am not sure how
> > > to (cleanly) bring the property from the phylink side to phydev side.
> > > Do you have any suggestion?
> >
> > I'm no ACPI expert, but i think
> > Documentation/firmware-guide/acpi/dsd/phy.rst gives you the basis:
> >
> >     During the MDIO bus driver initialization, PHYs on this bus are probed
> >     using the _ADR object as shown below and are registered on the MDIO bus.
> >
> >       Scope(\_SB.MDI0)
> >       {
> >         Device(PHY1) {
> >           Name (_ADR, 0x1)
> >         } // end of PHY1
> >
> >         Device(PHY2) {
> >           Name (_ADR, 0x2)
> >         } // end of PHY2
> >       }
> >
> > These are the PHYs on the MDIO bus. I _think_ that next to the Name,
> > you can add additional properties, like your "use-firmware-led". This
> > would then be very similar to DT, which is in effect what ACPI is
> > copying. So you need to update this document with your new property,
> > making it clear that this property only applies to boot, not
> > suspend/resume. And fwnode_mdiobus_register_phy() can look for the
> > property and set a flag in the phydev structure indicating that ACPI
> > is totally responsible for LEDs at boot time.
> 
> The problem here is there's no MDIO bus in ACPI namespace, namely no
> "Scope(\_SB.MDI0)" on this platform.

So add it. Basically, copy what DT does. I assume there is a node for
the Ethernet device? And is the MDIO bus driver instantiated by the
Ethernet device? So you can add the MDIO node as a sub node of the
Ethernet device. When you register the MDIO bus using
acpi_mdiobus_register() pass a pointer to this MDIO sub node.

     Andrew

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 739859c0dfb18..54ee54a6895c9 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -746,10 +746,14 @@  static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
-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;
+	}
 
 	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
 	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
@@ -769,20 +773,30 @@  static void marvell_config_led(struct phy_device *phydev)
 			def_config = MII_88E1510_PHY_LED_DEF;
 		break;
 	default:
-		return;
+		return -1;
 	}
 
+	return def_config;
+}
+
+static void marvell_config_led(struct phy_device *phydev, bool resume)
+{
+	int err;
+
+	if (!resume)
+		phydev->led_config = marvell_find_led_config(phydev);
+
+	if (phydev->led_config == -1)
+		return;
+
 	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
-			      def_config);
+			      phydev->led_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
 }
 
 static int marvell_config_init(struct phy_device *phydev)
 {
-	/* Set default LED */
-	marvell_config_led(phydev);
-
 	/* Set registers from marvell,reg-init DT property */
 	return marvell_of_reg_init(phydev);
 }
@@ -2845,6 +2859,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_GBIT_FEATURES */
 		.probe = marvell_probe,
 		.config_init = marvell_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1101_config_aneg,
 		.config_intr = marvell_config_intr,
 		.handle_interrupt = marvell_handle_interrupt,
@@ -2944,6 +2959,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_GBIT_FEATURES */
 		.probe = marvell_probe,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1121_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -2965,6 +2981,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_GBIT_FEATURES */
 		.probe = marvell_probe,
 		.config_init = m88e1318_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1318_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3044,6 +3061,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_GBIT_FEATURES */
 		.probe = marvell_probe,
 		.config_init = m88e1116r_config_init,
+		.config_led = marvell_config_led,
 		.config_intr = marvell_config_intr,
 		.handle_interrupt = marvell_handle_interrupt,
 		.resume = genphy_resume,
@@ -3065,6 +3083,7 @@  static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e1510_probe,
 		.config_init = m88e1510_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3094,6 +3113,7 @@  static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = marvell_probe,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3120,6 +3140,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3144,6 +3165,7 @@  static struct phy_driver marvell_drivers[] = {
 		/* PHY_BASIC_FEATURES */
 		.probe = marvell_probe,
 		.config_init = m88e3016_config_init,
+		.config_led = marvell_config_led,
 		.aneg_done = marvell_aneg_done,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3165,6 +3187,7 @@  static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = marvell_probe,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e6390_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3191,6 +3214,7 @@  static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = marvell_probe,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e6390_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3217,6 +3241,7 @@  static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = marvell_probe,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3242,6 +3267,7 @@  static struct phy_driver marvell_drivers[] = {
 		.probe = marvell_probe,
 		/* PHY_GBIT_FEATURES */
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
@@ -3264,6 +3290,7 @@  static struct phy_driver marvell_drivers[] = {
 		.probe = marvell_probe,
 		.features = PHY_GBIT_FIBRE_FEATURES,
 		.config_init = marvell_1011gbe_config_init,
+		.config_led = marvell_config_led,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
 		.config_intr = marvell_config_intr,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74d8e1dc125f8..c9e97206aa9e8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -12,6 +12,7 @@ 
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/errno.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
@@ -1157,6 +1158,7 @@  static int phy_poll_reset(struct phy_device *phydev)
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
+	bool resume = phydev->suspended;
 
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
@@ -1184,6 +1186,9 @@  int phy_init_hw(struct phy_device *phydev)
 			return ret;
 	}
 
+	if (phydev->drv->config_led)
+		phydev->drv->config_led(phydev, resume);
+
 	if (phydev->drv->config_intr) {
 		ret = phydev->drv->config_intr(phydev);
 		if (ret < 0)
@@ -1342,6 +1347,17 @@  int phy_sfp_probe(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_sfp_probe);
 
+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,
+	},
+	{}
+};
+
 /**
  * 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;
+
 	phydev->dev_flags |= flags;
 
 	phydev->interface = interface;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6de8d7a90d78e..3a944a6564f43 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -517,6 +517,8 @@  struct phy_c45_device_ids {
 struct macsec_context;
 struct macsec_ops;
 
+#define PHY_USE_FIRMWARE_LED 0x1000000
+
 /**
  * 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;
 
 	/*
 	 * 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);
+
 	/**
 	 * @probe: Called during discovery.  Used to set
 	 * up device-specific structures, if any