Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Gilles BULOZ]

Dear developers,

On a custom Elkhartlake board based on the Intel CRB, it turns out I have the 88E1512 PHY configured in polled mode ("intel-eth-pci 
0000:00:1e.4 eno1: PHY [stmmac-1:01] driver [Marvell 88E1510] (irq=POLL)" in dmesg) and the LED2/INT# pin is configured in LED2 mode 
by marvell_config_led() in drivers/net/phy/marvell.c (MII_88E1510_PHY_LED_DEF written to MII_PHY_LED_CTRL). This pin is connected as 
on the CRB to an Elkhartlake pin for a PHY interrupt but for some reason the interrupt is enabled on the Elkhartlake.
So when I shutdown the system (S5), any activity on link makes LED2/INT# toggle and power the system back on.

I tried to force  phydev->dev_flag to use MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE instead of MII_88E1510_PHY_LED_DEF but I've been 
unable to find how to force this flag. And I discovered that the value of MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE = 0x1040 is not OK 
for me because LED2 is set to "link status" so if I use this value the system is back "on" on link change (better than on activity 
but still not OK).

As a final workaround I've patched drivers/net/phy/marvell.c at marvell_config_led() to have "LED0=link LED1=activity LED2=off" by 
writing 0x1840 to MII_PHY_LED_CTRL, but I know this is a ugly workaround.

So I'm wondering if PHY "irq=POLL" is the expected operating mode ?
In this case what should disable the interrupt on the Elkhartlake pin ?
Is wake on Lan supported if PHY is set to "irq=POLL" ?

Thanks for tips.

Re: Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Andrew Lunn]

On Tue, Jul 19, 2022 at 06:36:20PM +0200, Gilles BULOZ wrote:
> Dear developers,
> 
> On a custom Elkhartlake board based on the Intel CRB, it turns out I have
> the 88E1512 PHY configured in polled mode ("intel-eth-pci 0000:00:1e.4 eno1:
> PHY [stmmac-1:01] driver [Marvell 88E1510] (irq=POLL)" in dmesg) and the
> LED2/INT# pin is configured in LED2 mode by marvell_config_led() in
> drivers/net/phy/marvell.c (MII_88E1510_PHY_LED_DEF written to
> MII_PHY_LED_CTRL). This pin is connected as on the CRB to an Elkhartlake pin
> for a PHY interrupt but for some reason the interrupt is enabled on the
> Elkhartlake.
> So when I shutdown the system (S5), any activity on link makes LED2/INT# toggle and power the system back on.
> 
> I tried to force  phydev->dev_flag to use
> MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE instead of MII_88E1510_PHY_LED_DEF but
> I've been unable to find how to force this flag. And I discovered that the
> value of MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE = 0x1040 is not OK for me
> because LED2 is set to "link status" so if I use this value the system is
> back "on" on link change (better than on activity but still not OK).
> 
> As a final workaround I've patched drivers/net/phy/marvell.c at
> marvell_config_led() to have "LED0=link LED1=activity LED2=off" by writing
> 0x1840 to MII_PHY_LED_CTRL, but I know this is a ugly workaround.
> 
> So I'm wondering if PHY "irq=POLL" is the expected operating mode ?
> In this case what should disable the interrupt on the Elkhartlake pin ?
> Is wake on Lan supported if PHY is set to "irq=POLL" ?

This sounds a bit like:

https://lore.kernel.org/lkml/YelxMFOiqnfIVmyy@lunn.ch/T/

If you comment out marvell_config_led(), and read back the registers,
does it look like the firmware has setup the LEDs to something
sensible?

Is the IRQ described in ACPI? Maybe you could wire it up. Set
phydev->irq before connecting the PHY, and then phylib will use the
IRQ, not polling. That might also solve your wakeup problem, in that
when the interrupt is disabled at shutdown, it should disable it in
the PHY.

    Andrew

Re: Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Gilles BULOZ]

Le 19/07/2022 à 23:35, Andrew Lunn a écrit :
> On Tue, Jul 19, 2022 at 06:36:20PM +0200, Gilles BULOZ wrote:
>> Dear developers,
>>
>> On a custom Elkhartlake board based on the Intel CRB, it turns out I have
>> the 88E1512 PHY configured in polled mode ("intel-eth-pci 0000:00:1e.4 eno1:
>> PHY [stmmac-1:01] driver [Marvell 88E1510] (irq=POLL)" in dmesg) and the
>> LED2/INT# pin is configured in LED2 mode by marvell_config_led() in
>> drivers/net/phy/marvell.c (MII_88E1510_PHY_LED_DEF written to
>> MII_PHY_LED_CTRL). This pin is connected as on the CRB to an Elkhartlake pin
>> for a PHY interrupt but for some reason the interrupt is enabled on the
>> Elkhartlake.
>> So when I shutdown the system (S5), any activity on link makes LED2/INT# toggle and power the system back on.
>>
>> I tried to force  phydev->dev_flag to use
>> MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE instead of MII_88E1510_PHY_LED_DEF but
>> I've been unable to find how to force this flag. And I discovered that the
>> value of MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE = 0x1040 is not OK for me
>> because LED2 is set to "link status" so if I use this value the system is
>> back "on" on link change (better than on activity but still not OK).
>>
>> As a final workaround I've patched drivers/net/phy/marvell.c at
>> marvell_config_led() to have "LED0=link LED1=activity LED2=off" by writing
>> 0x1840 to MII_PHY_LED_CTRL, but I know this is a ugly workaround.
>>
>> So I'm wondering if PHY "irq=POLL" is the expected operating mode ?
>> In this case what should disable the interrupt on the Elkhartlake pin ?
>> Is wake on Lan supported if PHY is set to "irq=POLL" ?
> This sounds a bit like:
>
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FYelxMFOiqnfIVmyy%40lunn.ch%2FT%2F&data=05%7C01%7Cgilles.buloz%40kontron.com%7C3c6eef4b24214d5c7a8408da69ce9e4b%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637938633398700195%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2FIM7eVVfMfpjNCDpJaMZTXejNWENIHZWKExGmSTz%2FFQ%3D&reserved=0
>
> If you comment out marvell_config_led(), and read back the registers,
> does it look like the firmware has setup the LEDs to something
> sensible?
The value programmed by the BIOS to MII_PHY_LED_CTRL is 0x0030 meaning LED2=link, LED1=activity, and LED0=link (and reserved bit 12 
is set to 0 instead of keeping it to its default 1). So this is also not something OK if the interrupt is enabled on the Elkartlake 
side for LED2/INT#
>
> Is the IRQ described in ACPI?
OK, I'm going to check for it
> Maybe you could wire it up. Set
> phydev->irq before connecting the PHY,
OK do you do that (set phydev->irq) ?
> and then phylib will use the
> IRQ, not polling. That might also solve your wakeup problem, in that
> when the interrupt is disabled at shutdown, it should disable it in
> the PHY.
Is the PHY interrupt needed to support WakeOnLan ?
And is the PHY POLL mode what we have on the EHL CRB (I don't have the CRB here so I can't check that) ?
Thanks
Gilles
>
>      Andrew
> .

Re: Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Andrew Lunn]

> The value programmed by the BIOS to MII_PHY_LED_CTRL is 0x0030 meaning
> LED2=link, LED1=activity, and LED0=link (and reserved bit 12 is set to 0
> instead of keeping it to its default 1). So this is also not something OK if
> the interrupt is enabled on the Elkartlake side for LED2/INT#

O.K, so it is a different situation to the link i gave.

> > Is the IRQ described in ACPI?
> OK, I'm going to check for it
> > Maybe you could wire it up. Set
> > phydev->irq before connecting the PHY,
> OK do you do that (set phydev->irq) ?
> > and then phylib will use the
> > IRQ, not polling. That might also solve your wakeup problem, in that
> > when the interrupt is disabled at shutdown, it should disable it in
> > the PHY.
> Is the PHY interrupt needed to support WakeOnLan ?
> And is the PHY POLL mode what we have on the EHL CRB (I don't have the CRB here so I can't check that) ?

Needing the interrupt will depend on how power management works on
your device.

There are two basic architectures which can be used.

1) The interrupt controller is kept running on suspend, and it can
wake the system up when an interrupt happens. You need to call
enable_irq_wake() to let the interrupt core code know this. Picking a random example:

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bcmsysport.c#L546

2) The output from the PHY is connected to PMIC. The pin changing
state causes the PMIC to turn the power back on. The SoC itself is not
involved. And the driver does not need to use interrupts, in fact it
cannot use interrupts, if the pin is connected to the PMIC and not the
SoC.

It sounds like you have 1), so yes, you should be using the interrupt
for WOL to work.

    Andrew

RE: Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Ong, Boon Leong]

>Is the PHY interrupt needed to support WakeOnLan ?
>And is the PHY POLL mode what we have on the EHL CRB (I don't have the CRB
>here so I can't check that) ?

Gilles, I have sent you a separate email to channel to question to intel ticket.
 

Re: Marvell 88E1512 PHY LED2 mode mismatch with Elkhartlake pin mode

[Gilles BULOZ]

Thanks Andrew for the explainations.

Having a look to the Intel ElkhartLake CRB schematics, the three 88E1512 have their LED2/INT# pin connected to the Elkhartlake so It 
seems this board is using architecture 1)
I'm currious to see if booting this CRB with a Fedora 36 would give the same issue than the one I have on my custom board based on 
the CRB : PHY in POLL mode with LED2/INT# set to link/activity by marvell.ko, but Elkhartlake pin with interrupt enabled, leading to 
a spurious power-on on link activity when in off state (S5)
I'm going to ask Intel support for the status on the CRB or try to get a CRB to test by myself.

Today, I discovered that if I blacklist or remove marvell.ko, a generic PHY driver is used instead (phy-generic.ko). This driver 
seems to keep the state of the LED2/INT# pin set by the BIOS that is "link up". So I n this case I get a spurious power-on when the 
link goes down then up.
Regardless of the PHY driver, shutting down the interface with "ifconfig <ethname> down" before shutting down the system is a 
working workaround (no toggle on LED2/INT# so no spurious power on)

Gilles

Le 26/07/2022 à 17:55, Andrew Lunn a écrit :
>> The value programmed by the BIOS to MII_PHY_LED_CTRL is 0x0030 meaning
>> LED2=link, LED1=activity, and LED0=link (and reserved bit 12 is set to 0
>> instead of keeping it to its default 1). So this is also not something OK if
>> the interrupt is enabled on the Elkartlake side for LED2/INT#
> O.K, so it is a different situation to the link i gave.
>
>>> Is the IRQ described in ACPI?
>> OK, I'm going to check for it
>>> Maybe you could wire it up. Set
>>> phydev->irq before connecting the PHY,
>> OK do you do that (set phydev->irq) ?
>>> and then phylib will use the
>>> IRQ, not polling. That might also solve your wakeup problem, in that
>>> when the interrupt is disabled at shutdown, it should disable it in
>>> the PHY.
>> Is the PHY interrupt needed to support WakeOnLan ?
>> And is the PHY POLL mode what we have on the EHL CRB (I don't have the CRB here so I can't check that) ?
> Needing the interrupt will depend on how power management works on
> your device.
>
> There are two basic architectures which can be used.
>
> 1) The interrupt controller is kept running on suspend, and it can
> wake the system up when an interrupt happens. You need to call
> enable_irq_wake() to let the interrupt core code know this. Picking a random example:
>
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fethernet%2Fbroadcom%2Fbcmsysport.c%23L546&amp;data=05%7C01%7Cgilles.buloz%40kontron.com%7C8bf9a027f842497e93ce08da6f1f3fec%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637944477262739726%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xSQxiM3lLrYyeKEbCS%2BRGI%2BdhCl7fqngYCIyBU0OOEs%3D&amp;reserved=0
>
> 2) The output from the PHY is connected to PMIC. The pin changing
> state causes the PMIC to turn the power back on. The SoC itself is not
> involved. And the driver does not need to use interrupts, in fact it
> cannot use interrupts, if the pin is connected to the PMIC and not the
> SoC.
>
> It sounds like you have 1), so yes, you should be using the interrupt
> for WOL to work.
>
>      Andrew
>
> .