Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware

[Kai-Heng Feng]

On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <andrew@xxxxxxx> 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