[PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[Kai-Heng Feng]

Not all PHY has an ACPI node, for those nodes auto probing is still
needed.

So only mask those PHYs with ACPI nodes.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/mdio/acpi_mdio.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/acpi_mdio.c b/drivers/net/mdio/acpi_mdio.c
index d77c987fda9cd..f9369319ada19 100644
--- a/drivers/net/mdio/acpi_mdio.c
+++ b/drivers/net/mdio/acpi_mdio.c
@@ -33,8 +33,15 @@ int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
 	u32 addr;
 	int ret;
 
-	/* Mask out all PHYs from auto probing. */
-	mdio->phy_mask = GENMASK(31, 0);
+	/* Loop over the child nodes and mask out PHY from auto probing */
+	fwnode_for_each_child_node(fwnode, child) {
+		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+		if (ret || addr >= PHY_MAX_ADDR)
+			continue;
+
+		mdio->phy_mask |= BIT(addr);
+	}
+
 	ret = mdiobus_register(mdio);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH 2/5] net: mdio: Add "use-firmware-led" firmware property

[Kai-Heng Feng]

Some system may prefer preset PHY LED setting instead of the one
hardcoded in the PHY driver, so adding a new firmware
property, "use-firmware-led", to cope with that.

On ACPI based system the ASL looks like this:

    Scope (_SB.PC00.OTN0)
    {
        Device (PHY0)
        {
            Name (_ADR, One)  // _ADR: Address
            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
                    }
                }
            })
        }

        Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
        {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
            Package (0x01)
            {
                Package (0x02)
                {
                    "phy-handle",
                    PHY0
                }
            }
        })
    }

Basically use the "phy-handle" reference to read the "use-firmware-led"
boolean.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/mdio/fwnode_mdio.c | 4 ++++
 include/linux/phy.h            | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1c1584fca6327..bfca67b42164b 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -144,6 +144,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	 */
 	if (mii_ts)
 		phy->mii_ts = mii_ts;
+
+	phy->use_firmware_led =
+		fwnode_property_read_bool(child, "use-firmware-led");
+
 	return 0;
 }
 EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c22533..53e693b3430ec 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -656,6 +656,7 @@ struct phy_device {
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
 
+	bool use_firmware_led;
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
 	unsigned int phy_num_led_triggers;
-- 
2.34.1

[PATCH 3/5] net: phy: Add helpers to save and restore firmware LED

[Kai-Heng Feng]

PHY drivers may set hardcoded LED config in phy_init_hw(), so to
preserve the firmware LED after init or system sleep, it needs to be
saved before init and be restored after.

To do so, create helpers and driver callbacks to access and save LED
config for the need.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/phy/phy_device.c | 22 ++++++++++++++++++++++
 include/linux/phy.h          |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def8..33b402279febe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1154,6 +1154,24 @@ static int phy_poll_reset(struct phy_device *phydev)
 	return 0;
 }
 
+static void phy_save_led(struct phy_device *phydev)
+{
+	if (!phydev->use_firmware_led)
+		return;
+
+	if (phydev->drv->get_led_config)
+		phydev->led_config = phydev->drv->get_led_config(phydev);
+}
+
+static void phy_restore_led(struct phy_device *phydev)
+{
+	if (!phydev->use_firmware_led)
+		return;
+
+	if (phydev->drv->set_led_config && phydev->led_config)
+		phydev->drv->set_led_config(phydev, phydev->led_config);
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
@@ -1463,6 +1481,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (dev)
 		netif_carrier_off(phydev->attached_dev);
 
+	phy_save_led(phydev);
+
 	/* Do initial configuration here, now that
 	 * we have certain key parameters
 	 * (dev_flags and interface)
@@ -1803,6 +1823,8 @@ int __phy_resume(struct phy_device *phydev)
 	if (!ret)
 		phydev->suspended = false;
 
+	phy_restore_led(phydev);
+
 	return ret;
 }
 EXPORT_SYMBOL(__phy_resume);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 53e693b3430ec..cd9c05ff75ee1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -657,6 +657,7 @@ struct phy_device {
 	u32 eee_broken_modes;
 
 	bool use_firmware_led;
+	int led_config;
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
 	unsigned int phy_num_led_triggers;
@@ -933,6 +934,9 @@ struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+
+	int (*get_led_config)(struct phy_device *dev);
+	void (*set_led_config)(struct phy_device *dev, int led_config);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.34.1

[PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Kai-Heng Feng]

Implement get_led_config() and set_led_config() callbacks so phy core
can use firmware LED as platform requested.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/phy/marvell.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2702faf7b0f60..c5f13e09b0692 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -750,6 +750,30 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+static int marvell_get_led_config(struct phy_device *phydev)
+{
+	int led;
+
+	led = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
+	if (led < 0) {
+		phydev_warn(phydev, "Fail to get marvell phy LED.\n");
+		led = 0;
+	}
+
+	return led;
+}
+
+static void marvell_set_led_config(struct phy_device *phydev, int led_config)
+{
+	int err;
+
+	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+			      led_config);
+
+	if (err < 0)
+		phydev_warn(phydev, "Fail to set marvell phy LED.\n");
+}
+
 static void marvell_config_led(struct phy_device *phydev)
 {
 	u16 def_config;
@@ -3139,6 +3163,8 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		.get_led_config = marvell_get_led_config,
+		.set_led_config = marvell_set_led_config,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
-- 
2.34.1

[PATCH 5/5] net: stmmac: Use acpi_mdiobus_register() for ACPI based system

[Kai-Heng Feng]

Child nodes of stmmac ACPI node may have additional properties that the
PHY layer can use.

To achieve that, use acpi_mdiobus_register() to find PHY nodes
references via "phy-handle", so the properties of PHY nodes can be used
by the PHY layer.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a5d150c5f3d8c..37cebd8f2ec5c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -10,6 +10,8 @@
   Maintainer: Giuseppe Cavallaro <peppe.cavallaro@st.com>
 *******************************************************************************/
 
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -445,6 +447,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct device_node *mdio_node = priv->plat->mdio_node;
 	struct device *dev = ndev->dev.parent;
+	struct fwnode_handle *fwnode = dev->fwnode;
 	int addr, found, max_addr;
 
 	if (!mdio_bus_data)
@@ -488,7 +491,10 @@ int stmmac_mdio_register(struct net_device *ndev)
 	new_bus->phy_mask = mdio_bus_data->phy_mask;
 	new_bus->parent = priv->device;
 
-	err = of_mdiobus_register(new_bus, mdio_node);
+	if (is_acpi_node(fwnode))
+		err = acpi_mdiobus_register(new_bus, fwnode);
+	else
+		err = of_mdiobus_register(new_bus, mdio_node);
 	if (err != 0) {
 		dev_err(dev, "Cannot register the MDIO bus\n");
 		goto bus_register_fail;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] net: stmmac: Use acpi_mdiobus_register() for ACPI based system

[Kai-Heng Feng]

Child nodes of stmmac ACPI node may have additional properties that the
PHY layer can use.

To achieve that, use acpi_mdiobus_register() to find PHY nodes
references via "phy-handle", so the properties of PHY nodes can be used
by the PHY layer.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a5d150c5f3d8c..37cebd8f2ec5c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -10,6 +10,8 @@
   Maintainer: Giuseppe Cavallaro <peppe.cavallaro@st.com>
 *******************************************************************************/
 
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -445,6 +447,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct device_node *mdio_node = priv->plat->mdio_node;
 	struct device *dev = ndev->dev.parent;
+	struct fwnode_handle *fwnode = dev->fwnode;
 	int addr, found, max_addr;
 
 	if (!mdio_bus_data)
@@ -488,7 +491,10 @@ int stmmac_mdio_register(struct net_device *ndev)
 	new_bus->phy_mask = mdio_bus_data->phy_mask;
 	new_bus->parent = priv->device;
 
-	err = of_mdiobus_register(new_bus, mdio_node);
+	if (is_acpi_node(fwnode))
+		err = acpi_mdiobus_register(new_bus, fwnode);
+	else
+		err = of_mdiobus_register(new_bus, mdio_node);
 	if (err != 0) {
 		dev_err(dev, "Cannot register the MDIO bus\n");
 		goto bus_register_fail;
-- 
2.34.1

Re: [PATCH 2/5] net: mdio: Add "use-firmware-led" firmware property

[Andrew Lunn]

On Wed, Apr 20, 2022 at 08:40:49PM +0800, Kai-Heng Feng wrote:
> Some system may prefer preset PHY LED setting instead of the one
> hardcoded in the PHY driver, so adding a new firmware
> property, "use-firmware-led", to cope with that.
> 
> On ACPI based system the ASL looks like this:
> 
>     Scope (_SB.PC00.OTN0)
>     {
>         Device (PHY0)
>         {
>             Name (_ADR, One)  // _ADR: Address
>             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
>                     }
>                 }
>             })
>         }
> 
>         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>         {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>             Package (0x01)
>             {
>                 Package (0x02)
>                 {
>                     "phy-handle",
>                     PHY0
>                 }
>             }
>         })
>     }
> 
> Basically use the "phy-handle" reference to read the "use-firmware-led"
> boolean.

Please update Documentation/firmware-guide/acpi/dsd/phy.rst

Please also Cc: the ACPI list. I have no idea what the naming
convention is for ACPI properties.

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/net/mdio/fwnode_mdio.c | 4 ++++
>  include/linux/phy.h            | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1c1584fca6327..bfca67b42164b 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -144,6 +144,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  	 */
>  	if (mii_ts)
>  		phy->mii_ts = mii_ts;
> +
> +	phy->use_firmware_led =
> +		fwnode_property_read_bool(child, "use-firmware-led");
> +

This is an ACPI only property. It is not valid in DT. It does not
fulfil the DT naming requirement etc. So please move this up inside
the if (is_acpi_node(child)) clause.

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36ca2b5c22533..53e693b3430ec 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -656,6 +656,7 @@ struct phy_device {
>  	/* Energy efficient ethernet modes which should be prohibited */
>  	u32 eee_broken_modes;
>  
> +	bool use_firmware_led;

There is probably a two byte hole after mdix_ctrl. So please consider
adding it there. Also, don't forget to update the documentation.

       Andrew

Re: [PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[Andrew Lunn]

On Wed, Apr 20, 2022 at 08:40:48PM +0800, Kai-Heng Feng wrote:
> Not all PHY has an ACPI node, for those nodes auto probing is still
> needed.

Why do you need this?

Documentation/firmware-guide/acpi/dsd/phy.rst 

There is nothing here about there being PHYs which are not listed in
ACPI. If you have decided to go the ACPI route, you need to list the
PHYs.

	Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Andrew Lunn]

On Wed, Apr 20, 2022 at 08:40:51PM +0800, Kai-Heng Feng wrote:
> Implement get_led_config() and set_led_config() callbacks so phy core
> can use firmware LED as platform requested.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/net/phy/marvell.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 2702faf7b0f60..c5f13e09b0692 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -750,6 +750,30 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
>  	return err;
>  }
>  
> +static int marvell_get_led_config(struct phy_device *phydev)
> +{
> +	int led;
> +
> +	led = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
> +	if (led < 0) {
> +		phydev_warn(phydev, "Fail to get marvell phy LED.\n");
> +		led = 0;
> +	}

I've said this multiple times, there are three LED registers, The
Function Control register, the Priority Control register and the Timer
control register. It is the combination of all three that defines how
the LEDs work. You need to save all of them.

And you need to make your API generic enough that the PHY driver can
save anywhere from 1 bit to 42 bytes of configuration.

I don't know ACPI, but i'm pretty sure this is not the ACPI way of
doing this. I think you should be defining an ACPI method, which
phylib can call after initialising the hardware to allow the firmware
to configure the LED.

     Andrew

Re: [PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[Kai-Heng Feng]

On Wed, Apr 20, 2022 at 10:47 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Apr 20, 2022 at 08:40:48PM +0800, Kai-Heng Feng wrote:
> > Not all PHY has an ACPI node, for those nodes auto probing is still
> > needed.
>
> Why do you need this?
>
> Documentation/firmware-guide/acpi/dsd/phy.rst
>
> There is nothing here about there being PHYs which are not listed in
> ACPI. If you have decided to go the ACPI route, you need to list the
> PHYs.

This is for backward-compatibility. MAC can have ACPI node but PHY may
not have one.

On ACPI based platform, stmmac is using mdiobus_register() and its PHY
is using autoprobing, so masking all PHYs from autoprobing will break
those stmmac users.

Kai-Heng

>
>         Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Kai-Heng Feng]

On Wed, Apr 20, 2022 at 11:03 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Apr 20, 2022 at 08:40:51PM +0800, Kai-Heng Feng wrote:
> > Implement get_led_config() and set_led_config() callbacks so phy core
> > can use firmware LED as platform requested.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/net/phy/marvell.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 2702faf7b0f60..c5f13e09b0692 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -750,6 +750,30 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
> >       return err;
> >  }
> >
> > +static int marvell_get_led_config(struct phy_device *phydev)
> > +{
> > +     int led;
> > +
> > +     led = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
> > +     if (led < 0) {
> > +             phydev_warn(phydev, "Fail to get marvell phy LED.\n");
> > +             led = 0;
> > +     }
>
> I've said this multiple times, there are three LED registers, The
> Function Control register, the Priority Control register and the Timer
> control register. It is the combination of all three that defines how
> the LEDs work. You need to save all of them.

OK, will do.

>
> And you need to make your API generic enough that the PHY driver can
> save anywhere from 1 bit to 42 bytes of configuration.

OK, I guess I'll let the implementation stays within driver callback,
so each driver can decide how many bits need to be saved.

>
> I don't know ACPI, but i'm pretty sure this is not the ACPI way of
> doing this. I think you should be defining an ACPI method, which
> phylib can call after initialising the hardware to allow the firmware
> to configure the LED.

This is not feasible.
If BIOS can define a method and restore the LED by itself, it can put
the method inside its S3 method and I don't have to work on this at
the first place.

Kai-Heng

>
>      Andrew

Re: [PATCH 2/5] net: mdio: Add "use-firmware-led" firmware property

[Kai-Heng Feng]

On Wed, Apr 20, 2022 at 9:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Apr 20, 2022 at 08:40:49PM +0800, Kai-Heng Feng wrote:
> > Some system may prefer preset PHY LED setting instead of the one
> > hardcoded in the PHY driver, so adding a new firmware
> > property, "use-firmware-led", to cope with that.
> >
> > On ACPI based system the ASL looks like this:
> >
> >     Scope (_SB.PC00.OTN0)
> >     {
> >         Device (PHY0)
> >         {
> >             Name (_ADR, One)  // _ADR: Address
> >             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
> >                     }
> >                 }
> >             })
> >         }
> >
> >         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
> >         {
> >             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> >             Package (0x01)
> >             {
> >                 Package (0x02)
> >                 {
> >                     "phy-handle",
> >                     PHY0
> >                 }
> >             }
> >         })
> >     }
> >
> > Basically use the "phy-handle" reference to read the "use-firmware-led"
> > boolean.
>
> Please update Documentation/firmware-guide/acpi/dsd/phy.rst
>
> Please also Cc: the ACPI list. I have no idea what the naming
> convention is for ACPI properties.
>
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/net/mdio/fwnode_mdio.c | 4 ++++
> >  include/linux/phy.h            | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index 1c1584fca6327..bfca67b42164b 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -144,6 +144,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> >        */
> >       if (mii_ts)
> >               phy->mii_ts = mii_ts;
> > +
> > +     phy->use_firmware_led =
> > +             fwnode_property_read_bool(child, "use-firmware-led");
> > +
>
> This is an ACPI only property. It is not valid in DT. It does not
> fulfil the DT naming requirement etc. So please move this up inside
> the if (is_acpi_node(child)) clause.
>
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 36ca2b5c22533..53e693b3430ec 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -656,6 +656,7 @@ struct phy_device {
> >       /* Energy efficient ethernet modes which should be prohibited */
> >       u32 eee_broken_modes;
> >
> > +     bool use_firmware_led;
>
> There is probably a two byte hole after mdix_ctrl. So please consider
> adding it there. Also, don't forget to update the documentation.

OK, will do once other concerns are ironed out.

Kai-Heng

>
>        Andrew

Re: [PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[Andrew Lunn]

On Thu, Apr 21, 2022 at 10:58:40AM +0800, Kai-Heng Feng wrote:
> On Wed, Apr 20, 2022 at 10:47 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Wed, Apr 20, 2022 at 08:40:48PM +0800, Kai-Heng Feng wrote:
> > > Not all PHY has an ACPI node, for those nodes auto probing is still
> > > needed.
> >
> > Why do you need this?
> >
> > Documentation/firmware-guide/acpi/dsd/phy.rst
> >
> > There is nothing here about there being PHYs which are not listed in
> > ACPI. If you have decided to go the ACPI route, you need to list the
> > PHYs.
> 
> This is for backward-compatibility. MAC can have ACPI node but PHY may
> not have one.

And if the PHY does not have an ACPI node, fall back to
mdiobus_register(). This is what of_mdiobus_register() does. If
np=NULL, it calls mdiobus_register() and skips all the OF stuff.

	 Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Andrew Lunn]

> This is not feasible.
> If BIOS can define a method and restore the LED by itself, it can put
> the method inside its S3 method and I don't have to work on this at
> the first place.

So maybe just declare the BIOS as FUBAR and move on to the next issue
assigned to you.

Do we really want the maintenance burden of this code for one machines
BIOS? Maybe the better solution is to push back on the vendor and its
BIOS, tell them how they should of done this, if the BIOS wants to be
in control of the LEDs it needs to offer the methods to control the
LEDs. And then hopefully the next machine the vendor produces will
have working BIOS.

Your other option is to take part in the effort to add control of the
LEDs via the standard Linux LED subsystem. The Marvel PHY driver is
likely to be one of the first to gain support this for. So you can
then totally take control of the LED from the BIOS and put it in the
users hands. And such a solution will be applicable to many machines,
not just one.

       Andrew

Re: [PATCH 1/5] net: mdio: Mask PHY only when its ACPI node is present

[Kai-Heng Feng]

On Thu, Apr 21, 2022 at 7:40 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Apr 21, 2022 at 10:58:40AM +0800, Kai-Heng Feng wrote:
> > On Wed, Apr 20, 2022 at 10:47 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Wed, Apr 20, 2022 at 08:40:48PM +0800, Kai-Heng Feng wrote:
> > > > Not all PHY has an ACPI node, for those nodes auto probing is still
> > > > needed.
> > >
> > > Why do you need this?
> > >
> > > Documentation/firmware-guide/acpi/dsd/phy.rst
> > >
> > > There is nothing here about there being PHYs which are not listed in
> > > ACPI. If you have decided to go the ACPI route, you need to list the
> > > PHYs.
> >
> > This is for backward-compatibility. MAC can have ACPI node but PHY may
> > not have one.
>
> And if the PHY does not have an ACPI node, fall back to
> mdiobus_register(). This is what of_mdiobus_register() does. If
> np=NULL, it calls mdiobus_register() and skips all the OF stuff.

The equivalent to this scenario is that when MAC doesn't have ACPI node.
But yes it can unmask the PHYs if no ACPI node is found, then call
mdiobus_register().

Kai-Heng

>
>          Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Kai-Heng Feng]

On Thu, Apr 21, 2022 at 7:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > This is not feasible.
> > If BIOS can define a method and restore the LED by itself, it can put
> > the method inside its S3 method and I don't have to work on this at
> > the first place.
>
> So maybe just declare the BIOS as FUBAR and move on to the next issue
> assigned to you.
>
> Do we really want the maintenance burden of this code for one machines
> BIOS?

Wasn't this the "set precedence" we discussed earlier for? Someone has
to be the first, and more users will leverage the new property we
added.

> Maybe the better solution is to push back on the vendor and its
> BIOS, tell them how they should of done this, if the BIOS wants to be
> in control of the LEDs it needs to offer the methods to control the
> LEDs. And then hopefully the next machine the vendor produces will
> have working BIOS.

The BIOS doesn't want to control the LED. It just provides a default
LED setting suitable for this platform, so the driver can use this
value over the hardcoded one in marvell phy driver.
So this really has nothing to do with with any ACPI method.
I believe the new property can be useful for DT world too.

>
> Your other option is to take part in the effort to add control of the
> LEDs via the standard Linux LED subsystem. The Marvel PHY driver is
> likely to be one of the first to gain support this for. So you can
> then totally take control of the LED from the BIOS and put it in the
> users hands. And such a solution will be applicable to many machines,
> not just one.

This series just wants to use the default value platform firmware provides.
Create a sysfs to let user meddle with LED value doesn't really help
the case here.

Kai-Heng

>
>        Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Andrew Lunn]

On Thu, Apr 21, 2022 at 08:24:00PM +0800, Kai-Heng Feng wrote:
> On Thu, Apr 21, 2022 at 7:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > This is not feasible.
> > > If BIOS can define a method and restore the LED by itself, it can put
> > > the method inside its S3 method and I don't have to work on this at
> > > the first place.
> >
> > So maybe just declare the BIOS as FUBAR and move on to the next issue
> > assigned to you.
> >
> > Do we really want the maintenance burden of this code for one machines
> > BIOS?
> 
> Wasn't this the "set precedence" we discussed earlier for? Someone has
> to be the first, and more users will leverage the new property we
> added.

I both agree and disagree. I'm trying to make this feature generic,
unlike you who seem to be doing the minimal, only saving one of three
LED configuration registers. But on the other hand, i'm not sure there
will be more users. Do you have a list of machines where the BIOS is
FUBAR? Is it one machine? A range of machines from one vendor, or
multiple vendors with multiple machines. I would feel better about the
maintenance burden if i knew that this was going to be used a lot.
 
> > Maybe the better solution is to push back on the vendor and its
> > BIOS, tell them how they should of done this, if the BIOS wants to be
> > in control of the LEDs it needs to offer the methods to control the
> > LEDs. And then hopefully the next machine the vendor produces will
> > have working BIOS.
> 
> The BIOS doesn't want to control the LED. It just provides a default
> LED setting suitable for this platform, so the driver can use this
> value over the hardcoded one in marvell phy driver.

Exactly, it wants to control the LED, and tell the OS not to touch it
ever.

> So this really has nothing to do with with any ACPI method.
> I believe the new property can be useful for DT world too.

DT generally never trusts the bootloader to do anything. So i doubt
such a DT property would ever be used. Also, DT is about describing
the hardware, not how to configure the hardware. So you could list
there is a PHY LED, what colour it is, etc. But in general, you would
not describe how it is configured, that something else is configuring
it and it should be left alone.

> > Your other option is to take part in the effort to add control of the
> > LEDs via the standard Linux LED subsystem. The Marvel PHY driver is
> > likely to be one of the first to gain support this for. So you can
> > then totally take control of the LED from the BIOS and put it in the
> > users hands. And such a solution will be applicable to many machines,
> > not just one.
> 
> This series just wants to use the default value platform firmware provides.
> Create a sysfs to let user meddle with LED value doesn't really help
> the case here.

I would disagree. You can add a systemd service to configure it at
boot however you want. It opens up the possibility to implement
ethtool --identify in a generic way, etc. It is a much more powerful
and useful feature than saying 'don't touch', and also it justify the
maintenance burden.

     Andrew

Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Kai-Heng Feng]

On Thu, Apr 21, 2022 at 8:57 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Apr 21, 2022 at 08:24:00PM +0800, Kai-Heng Feng wrote:
> > On Thu, Apr 21, 2022 at 7:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > This is not feasible.
> > > > If BIOS can define a method and restore the LED by itself, it can put
> > > > the method inside its S3 method and I don't have to work on this at
> > > > the first place.
> > >
> > > So maybe just declare the BIOS as FUBAR and move on to the next issue
> > > assigned to you.
> > >
> > > Do we really want the maintenance burden of this code for one machines
> > > BIOS?
> >
> > Wasn't this the "set precedence" we discussed earlier for? Someone has
> > to be the first, and more users will leverage the new property we
> > added.
>
> I both agree and disagree. I'm trying to make this feature generic,
> unlike you who seem to be doing the minimal, only saving one of three
> LED configuration registers. But on the other hand, i'm not sure there
> will be more users. Do you have a list of machines where the BIOS is
> FUBAR? Is it one machine? A range of machines from one vendor, or
> multiple vendors with multiple machines. I would feel better about the
> maintenance burden if i knew that this was going to be used a lot.

Right now it's only one machine. But someone has to be the first :)

>
> > > Maybe the better solution is to push back on the vendor and its
> > > BIOS, tell them how they should of done this, if the BIOS wants to be
> > > in control of the LEDs it needs to offer the methods to control the
> > > LEDs. And then hopefully the next machine the vendor produces will
> > > have working BIOS.
> >
> > The BIOS doesn't want to control the LED. It just provides a default
> > LED setting suitable for this platform, so the driver can use this
> > value over the hardcoded one in marvell phy driver.
>
> Exactly, it wants to control the LED, and tell the OS not to touch it
> ever.

That doesn't mean it wants to control the LED, it's still the phy
driver controls it.

>
> > So this really has nothing to do with with any ACPI method.
> > I believe the new property can be useful for DT world too.
>
> DT generally never trusts the bootloader to do anything. So i doubt
> such a DT property would ever be used. Also, DT is about describing
> the hardware, not how to configure the hardware. So you could list
> there is a PHY LED, what colour it is, etc. But in general, you would
> not describe how it is configured, that something else is configuring
> it and it should be left alone.

What if let the property list to the raw value of the LED should be?
So it can fall under "describing hardware" like 'clock-frequency' property.

>
> > > Your other option is to take part in the effort to add control of the
> > > LEDs via the standard Linux LED subsystem. The Marvel PHY driver is
> > > likely to be one of the first to gain support this for. So you can
> > > then totally take control of the LED from the BIOS and put it in the
> > > users hands. And such a solution will be applicable to many machines,
> > > not just one.
> >
> > This series just wants to use the default value platform firmware provides.
> > Create a sysfs to let user meddle with LED value doesn't really help
> > the case here.
>
> I would disagree. You can add a systemd service to configure it at
> boot however you want. It opens up the possibility to implement
> ethtool --identify in a generic way, etc. It is a much more powerful
> and useful feature than saying 'don't touch', and also it justify the
> maintenance burden.

That just pushed the maintenance burden to another subsystem and I
doubt it will bring more users than current approach.

Kai-Heng

>
>      Andrew