* [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-20 5:19 Kai-Heng Feng
2022-01-20 7:58 ` Heiner Kallweit
` (4 more replies)
0 siblings, 5 replies; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-20 5:19 UTC (permalink / raw)
To: andrew, hkallweit1, linux
Cc: Kai-Heng Feng, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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 <kai.heng.feng@canonical.com>
---
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
--
2.33.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
@ 2022-01-20 7:58 ` Heiner Kallweit
2022-01-20 11:40 ` Kai-Heng Feng
2022-01-20 13:46 ` Andrew Lunn
` (3 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Heiner Kallweit @ 2022-01-20 7:58 UTC (permalink / raw)
To: Kai-Heng Feng, andrew, linux
Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel
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 <kai.heng.feng@canonical.com>
> ---
> 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/20211112153557.26941-1-ansuelsmth@gmail.com/T/#t
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 7:58 ` Heiner Kallweit
@ 2022-01-20 11:40 ` Kai-Heng Feng
0 siblings, 0 replies; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-20 11:40 UTC (permalink / raw)
To: Heiner Kallweit
Cc: andrew, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
Hi Heiner,
On Thu, Jan 20, 2022 at 3:58 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <kai.heng.feng@canonical.com>
> > ---
> > 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/20211112153557.26941-1-ansuelsmth@gmail.com/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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
2022-01-20 7:58 ` Heiner Kallweit
@ 2022-01-20 13:46 ` Andrew Lunn
2022-01-21 3:54 ` Kai-Heng Feng
2022-01-21 7:49 ` Heiner Kallweit
2022-01-20 14:26 ` Andrew Lunn
` (2 subsequent siblings)
4 siblings, 2 replies; 27+ messages in thread
From: Andrew Lunn @ 2022-01-20 13:46 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
2022-01-20 7:58 ` Heiner Kallweit
2022-01-20 13:46 ` Andrew Lunn
@ 2022-01-20 14:26 ` Andrew Lunn
2022-01-21 4:01 ` Kai-Heng Feng
2022-01-20 15:33 ` kernel test robot
2022-01-21 6:47 ` kernel test robot
4 siblings, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-20 14:26 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
@ 2022-01-20 15:33 ` kernel test robot
2022-01-20 13:46 ` Andrew Lunn
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2022-01-20 15:33 UTC (permalink / raw)
To: Kai-Heng Feng, andrew, hkallweit1, linux
Cc: kbuild-all, Kai-Heng Feng, Jakub Kicinski, netdev, linux-kernel
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/202201202323.K0yx4TYb-lkp@intel.com/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 <lkp@intel.com>
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/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-20 15:33 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2022-01-20 15:33 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7705 bytes --]
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/202201202323.K0yx4TYb-lkp(a)intel.com/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 <lkp@intel.com>
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/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 13:46 ` Andrew Lunn
@ 2022-01-21 3:54 ` Kai-Heng Feng
2022-01-21 13:04 ` Andrew Lunn
2022-01-21 13:10 ` Andrew Lunn
2022-01-21 7:49 ` Heiner Kallweit
1 sibling, 2 replies; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-21 3:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Thu, Jan 20, 2022 at 9:46 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
>
> Please split this patch into two:
>
> Don't touch the LEDs
>
> Save and restore the LED configuration over suspend/resume.
Will split into two patch for next iteration.
>
> > -static void marvell_config_led(struct phy_device *phydev)
> > +static int marvell_find_led_config(struct phy_device *phydev)
> > {
> > - u16 def_config;
> > - int err;
> > + int def_config;
> > +
> > + if (phydev->dev_flags & PHY_USE_FIRMWARE_LED) {
> > + def_config = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
> > + return def_config < 0 ? -1 : def_config;
>
> What about the other two registers which configure the LEDs?
Do you mean the other two LEDs? They are not used on this machine.
>
> Since you talked about suspend/resume, does this machine support WoL?
> Is the BIOS configuring LED2 to be used as an interrupt when WoL is
> enabled in the BIOS? Do you need to save/restore that configuration
> over suspend/review? And prevent the driver from changing the
> configuration?
This NIC on the machine doesn't support WoL.
>
> > +static const struct dmi_system_id platform_flags[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> > + },
> > + .driver_data = (void *)PHY_USE_FIRMWARE_LED,
> > + },
>
> This needs a big fat warning, that it will affect all LEDs for PHYs
> which linux is driving, on that machine. So PHYs on USB dongles, PHYs
> in SFPs, PHYs on plugin PCIe card etc.
>
> Have you talked with Dells Product Manager and do they understand the
> implications of this?
Right, that's why the original approach is passing the flag from the MAC driver.
That approach can be more specific and doesn't touch unrelated PHYs.
>
> > + {}
> > +};
> > +
> > /**
> > * phy_attach_direct - attach a network device to a given PHY device pointer
> > * @dev: network device to attach
> > @@ -1363,6 +1379,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> > struct mii_bus *bus = phydev->mdio.bus;
> > struct device *d = &phydev->mdio.dev;
> > struct module *ndev_owner = NULL;
> > + const struct dmi_system_id *dmi;
> > bool using_genphy = false;
> > int err;
> >
> > @@ -1443,6 +1460,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> > phydev_err(phydev, "error creating 'phy_standalone' sysfs entry\n");
> > }
> >
> > + dmi = dmi_first_match(platform_flags);
> > + if (dmi)
> > + phydev->dev_flags |= (u32)dmi->driver_data;
>
> Please us your new flag directly. We don't want this abused to pass
> any old flag to the PHY.
Will change it.
>
> > +
> > /**
> > * struct phy_device - An instance of a PHY
> > *
> > @@ -663,6 +665,7 @@ struct phy_device {
> >
> > struct phy_led_trigger *led_link_trigger;
> > #endif
> > + int led_config;
>
> You cannot put this here because you don't know how many registers are
> used to hold the configuration. Marvell has 3, other drivers can have
> other numbers. The information needs to be saved into the drivers on
> priv structure.
Ok.
>
> >
> > /*
> > * Interrupt number for this PHY
> > @@ -776,6 +779,12 @@ struct phy_driver {
> > */
> > int (*config_init)(struct phy_device *phydev);
> >
> > + /**
> > + * @config_led: Called to config the PHY LED,
> > + * Use the resume flag to indicate init or resume
> > + */
> > + void (*config_led)(struct phy_device *phydev, bool resume);
>
> I don't see any need for this.
Ok.
Kai-Heng
>
> Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 14:26 ` Andrew Lunn
@ 2022-01-21 4:01 ` Kai-Heng Feng
2022-01-21 13:22 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-21 4:01 UTC (permalink / raw)
To: Andrew Lunn
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Thu, Jan 20, 2022 at 10:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
> >
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
@ 2022-01-21 6:47 ` kernel test robot
2022-01-20 13:46 ` Andrew Lunn
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2022-01-21 6:47 UTC (permalink / raw)
To: Kai-Heng Feng, andrew, hkallweit1, linux
Cc: llvm, kbuild-all, Kai-Heng Feng, Jakub Kicinski, netdev, linux-kernel
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/202201211407.0fB4ltJZ-lkp@intel.com/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 <lkp@intel.com>
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/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-21 6:47 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2022-01-21 6:47 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7774 bytes --]
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/202201211407.0fB4ltJZ-lkp(a)intel.com/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 <lkp@intel.com>
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/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-20 13:46 ` Andrew Lunn
2022-01-21 3:54 ` Kai-Heng Feng
@ 2022-01-21 7:49 ` Heiner Kallweit
1 sibling, 0 replies; 27+ messages in thread
From: Heiner Kallweit @ 2022-01-21 7:49 UTC (permalink / raw)
To: Andrew Lunn, Kai-Heng Feng
Cc: linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 3:54 ` Kai-Heng Feng
@ 2022-01-21 13:04 ` Andrew Lunn
2022-01-21 14:04 ` Kai-Heng Feng
2022-01-21 13:10 ` Andrew Lunn
1 sibling, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 13:04 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 3:54 ` Kai-Heng Feng
2022-01-21 13:04 ` Andrew Lunn
@ 2022-01-21 13:10 ` Andrew Lunn
2022-01-21 14:06 ` Kai-Heng Feng
1 sibling, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 13:10 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
> > > -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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 4:01 ` Kai-Heng Feng
@ 2022-01-21 13:22 ` Andrew Lunn
2022-01-21 14:17 ` Kai-Heng Feng
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 13:22 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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 <andrew@lunn.ch> wrote:
> >
> > On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > > instead of setting another value, keep it untouched and restore the saved
> > > value on system resume.
> > >
> > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 13:04 ` Andrew Lunn
@ 2022-01-21 14:04 ` Kai-Heng Feng
2022-01-21 15:05 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-21 14:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Fri, Jan 21, 2022 at 9:05 PM Andrew Lunn <andrew@lunn.ch> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 13:10 ` Andrew Lunn
@ 2022-01-21 14:06 ` Kai-Heng Feng
2022-01-21 15:08 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-21 14:06 UTC (permalink / raw)
To: Andrew Lunn
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Fri, Jan 21, 2022 at 9:10 PM Andrew Lunn <andrew@lunn.ch> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 13:22 ` Andrew Lunn
@ 2022-01-21 14:17 ` Kai-Heng Feng
2022-01-21 15:15 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-01-21 14:17 UTC (permalink / raw)
To: Andrew Lunn
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Fri, Jan 21, 2022 at 9:22 PM Andrew Lunn <andrew@lunn.ch> 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 <andrew@lunn.ch> wrote:
> > >
> > > On Thu, Jan 20, 2022 at 01:19:29PM +0800, Kai-Heng Feng wrote:
> > > > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > > > instead of setting another value, keep it untouched and restore the saved
> > > > value on system resume.
> > > >
> > > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 14:04 ` Kai-Heng Feng
@ 2022-01-21 15:05 ` Andrew Lunn
0 siblings, 0 replies; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 15:05 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 14:06 ` Kai-Heng Feng
@ 2022-01-21 15:08 ` Andrew Lunn
0 siblings, 0 replies; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 15:08 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 14:17 ` Kai-Heng Feng
@ 2022-01-21 15:15 ` Andrew Lunn
2022-01-22 19:13 ` Heiner Kallweit
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-21 15:15 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: hkallweit1, linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-21 15:15 ` Andrew Lunn
@ 2022-01-22 19:13 ` Heiner Kallweit
2022-01-22 21:18 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Heiner Kallweit @ 2022-01-22 19:13 UTC (permalink / raw)
To: Andrew Lunn, Kai-Heng Feng
Cc: linux, David S. Miller, Jakub Kicinski, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-22 19:13 ` Heiner Kallweit
@ 2022-01-22 21:18 ` Andrew Lunn
2022-02-14 5:40 ` Kai-Heng Feng
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-01-22 21:18 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Kai-Heng Feng, linux, David S. Miller, Jakub Kicinski, netdev,
linux-kernel
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-01-22 21:18 ` Andrew Lunn
@ 2022-02-14 5:40 ` Kai-Heng Feng
2022-02-15 20:27 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-02-14 5:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: Heiner Kallweit, linux, David S. Miller, Jakub Kicinski, netdev,
linux-kernel
On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <andrew@lunn.ch> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-02-14 5:40 ` Kai-Heng Feng
@ 2022-02-15 20:27 ` Andrew Lunn
2022-02-16 2:30 ` Kai-Heng Feng
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2022-02-15 20:27 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Heiner Kallweit, linux, David S. Miller, Jakub Kicinski, netdev,
linux-kernel
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 <andrew@lunn.ch> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-02-15 20:27 ` Andrew Lunn
@ 2022-02-16 2:30 ` Kai-Heng Feng
2022-02-16 7:39 ` Andrew Lunn
0 siblings, 1 reply; 27+ messages in thread
From: Kai-Heng Feng @ 2022-02-16 2:30 UTC (permalink / raw)
To: Andrew Lunn
Cc: Heiner Kallweit, linux, David S. Miller, Jakub Kicinski, netdev,
linux-kernel
On Wed, Feb 16, 2022 at 4:27 AM Andrew Lunn <andrew@lunn.ch> 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 <andrew@lunn.ch> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware
2022-02-16 2:30 ` Kai-Heng Feng
@ 2022-02-16 7:39 ` Andrew Lunn
0 siblings, 0 replies; 27+ messages in thread
From: Andrew Lunn @ 2022-02-16 7:39 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Heiner Kallweit, linux, David S. Miller, Jakub Kicinski, netdev,
linux-kernel
> > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2022-02-16 7:58 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-20 5:19 [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware Kai-Heng Feng
2022-01-20 7:58 ` Heiner Kallweit
2022-01-20 11:40 ` Kai-Heng Feng
2022-01-20 13:46 ` Andrew Lunn
2022-01-21 3:54 ` Kai-Heng Feng
2022-01-21 13:04 ` Andrew Lunn
2022-01-21 14:04 ` Kai-Heng Feng
2022-01-21 15:05 ` Andrew Lunn
2022-01-21 13:10 ` Andrew Lunn
2022-01-21 14:06 ` Kai-Heng Feng
2022-01-21 15:08 ` Andrew Lunn
2022-01-21 7:49 ` Heiner Kallweit
2022-01-20 14:26 ` Andrew Lunn
2022-01-21 4:01 ` Kai-Heng Feng
2022-01-21 13:22 ` Andrew Lunn
2022-01-21 14:17 ` Kai-Heng Feng
2022-01-21 15:15 ` Andrew Lunn
2022-01-22 19:13 ` Heiner Kallweit
2022-01-22 21:18 ` Andrew Lunn
2022-02-14 5:40 ` Kai-Heng Feng
2022-02-15 20:27 ` Andrew Lunn
2022-02-16 2:30 ` Kai-Heng Feng
2022-02-16 7:39 ` Andrew Lunn
2022-01-20 15:33 ` kernel test robot
2022-01-20 15:33 ` kernel test robot
2022-01-21 6:47 ` kernel test robot
2022-01-21 6:47 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.