Messages in this thread | | | From | Kai-Heng Feng <> | Date | Wed, 16 Feb 2022 10:30:41 +0800 | Subject | Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware |
| |
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
| |