| Message ID | 20220420124053.853891-5-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/5] net: mdio: Mask PHY only when its ACPI node is present | expand |
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
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
> 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
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
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
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
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,
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(+)