lkml.org 
[lkml]   [2022]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2022-02-16 03:32    [W:0.116 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site