Question about default m88e1510 LED configuration of marvell phy

Question about default m88e1510 LED configuration of marvell phy

[shenjian (K)]

Hi, all
	We encounted a problem when using the marvel 88e151x phy driver, the
link(LED[0])/ack(LED[1]) leds worked abnormally since we updated our kernel to v4.19.
The act led kept on and never blink.  It works well when we use genphy driver, aslo
works well when use marvell phy in kernel v4.18.

	I found that the default led configuration had been changed to 0x1177
by commit commit 077772468ec1(net: phy: marvell: change default m88e1510 LED configuration).

+static void marvell_config_led(struct phy_device *phydev)
+{
+	u16 def_config;
+	int err;
+
+	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
+	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
+	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
+	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
+		def_config = MII_88E1121_PHY_LED_DEF;
+		break;
+	/* Default PHY LED config:
+	 * LED[0] .. 1000Mbps Link
+	 * LED[1] .. 100Mbps Link
+	 * LED[2] .. Blink, Activity
+	 */
+	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
+		def_config = MII_88E1510_PHY_LED_DEF;
+		break;
+	default:
+		return;
+	}
+
+	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+			      def_config);
+	if (err < 0)
+		pr_warn("Fail to config marvell phy LED.\n");
+}
+

	For the led configuration is very flexible for m88e1510, people may use
different function for each led, I think it's quite reasonable to change its default
configuration when phy init.
	
If there are any better info or suggestion regarding this problem, it would be very
helpful, thanks in advance.

reference:
[1] https://patchwork.ozlabs.org/patch/937682/
[2] https://lore.kernel.org/patchwork/patch/687718/
[3] http://lkml.iu.edu/hypermail/linux/kernel/1606.1/03283.html
[4] https://web.pa.msu.edu/hep/atlas/l1calo/htm/hardware/components/Enet_Phy/marvell_alaska_phy_88e151x_datasheet_jan18.pdf

Re: Question about default m88e1510 LED configuration of marvell phy

[Andrew Lunn]

On Tue, Jan 15, 2019 at 11:07:17AM +0800, shenjian (K) wrote:
> Hi, all
> 	We encounted a problem when using the marvel 88e151x phy driver, the
> link(LED[0])/ack(LED[1]) leds worked abnormally since we updated our kernel to v4.19.
> The act led kept on and never blink.  It works well when we use genphy driver, aslo
> works well when use marvell phy in kernel v4.18.
> 
> 	I found that the default led configuration had been changed to 0x1177
> by commit commit 077772468ec1(net: phy: marvell: change default m88e1510 LED configuration).

It is a difficult job deciding what the driver should configure. I
would actually prefer to keeps with the reset default, which would be
0x101e according to the datasheet.

If you are using device tree, you have the option of poking values
into registers, using marvell,reg-init. There are a few examples you
can copy. Doing that guarantees you have the configuration you want.

    Andrew

Re: Question about default m88e1510 LED configuration of marvell phy

[shenjian (K)]

在 2019/1/15 11:39, Andrew Lunn 写道:
> On Tue, Jan 15, 2019 at 11:07:17AM +0800, shenjian (K) wrote:
>> Hi, all
>> 	We encounted a problem when using the marvel 88e151x phy driver, the
>> link(LED[0])/ack(LED[1]) leds worked abnormally since we updated our kernel to v4.19.
>> The act led kept on and never blink.  It works well when we use genphy driver, aslo
>> works well when use marvell phy in kernel v4.18.
>>
>> 	I found that the default led configuration had been changed to 0x1177
>> by commit commit 077772468ec1(net: phy: marvell: change default m88e1510 LED configuration).
> 
> It is a difficult job deciding what the driver should configure. I
> would actually prefer to keeps with the reset default, which would be
> 0x101e according to the datasheet.
> 
> If you are using device tree, you have the option of poking values
> into registers, using marvell,reg-init. There are a few examples you
> can copy. Doing that guarantees you have the configuration you want.
> 
>     Andrew
> 
> .
> 
Thanks, Andrew.

But we are using acpi mode, is there any ohter way to poke values into registers ?


	

Re: Question about default m88e1510 LED configuration of marvell phy

[Wang, Dongsheng]

On 2019/1/15 14:41, shenjian (K) wrote:
>
> 在 2019/1/15 11:39, Andrew Lunn 写道:
>> On Tue, Jan 15, 2019 at 11:07:17AM +0800, shenjian (K) wrote:
>>> Hi, all
>>> 	We encounted a problem when using the marvel 88e151x phy driver, the
>>> link(LED[0])/ack(LED[1]) leds worked abnormally since we updated our kernel to v4.19.
>>> The act led kept on and never blink.  It works well when we use genphy driver, aslo
>>> works well when use marvell phy in kernel v4.18.
>>>
>>> 	I found that the default led configuration had been changed to 0x1177
>>> by commit commit 077772468ec1(net: phy: marvell: change default m88e1510 LED configuration).
>> It is a difficult job deciding what the driver should configure. I
>> would actually prefer to keeps with the reset default, which would be
>> 0x101e according to the datasheet.
>>
>> If you are using device tree, you have the option of poking values
>> into registers, using marvell,reg-init. There are a few examples you
>> can copy. Doing that guarantees you have the configuration you want.
>>
>>     Andrew
>>
>> .
>>
> Thanks, Andrew.
>
> But we are using acpi mode, is there any ohter way to poke values into registers ?

You can describe property in ACPI _DSD. And parse then in
marvell_config_init just like of.

Cheers
Dongsheng

Re: Question about default m88e1510 LED configuration of marvell phy

[Andrew Lunn]

> Thanks, Andrew.
> 
> But we are using acpi mode, is there any ohter way to poke values into registers ?

You can register a PHY fixup.

phy_register_fixup_for_uid(), mach-orion5x/dns323-setup.c

      Andrew

Re: Question about default m88e1510 LED configuration of marvell phy

[shenjian (K)]

在 2019/1/17 0:04, Andrew Lunn 写道:
>> Thanks, Andrew.
>>
>> But we are using acpi mode, is there any ohter way to poke values into registers ?
> 
> You can register a PHY fixup.
> 
> phy_register_fixup_for_uid(), mach-orion5x/dns323-setup.c
> 
>       Andrew
> 
> .
> 
Thanks Andrew!

I have tried phy_register_fixup_for_uid(), but it doesn't work .

Maybe I missed something important, phy_scan_fixups() is called before drv->config_init() in function
phy_init_hw(). So even though I configure the led ctrl register in fixup callbacks, the configuration
will be overrided by m88e1510_config_init()->m88e1318_config_init()->marvell_config_init().

test log is below:
[  105.703297] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 UEFI RC0 - V1.10.01 01/03/2019
[  105.720192] Workqueue: events work_for_cpu_fn
[  105.728902] Call trace:
[  105.733782]  dump_backtrace+0x0/0x180
[  105.741099]  show_stack+0x14/0x20
[  105.747720]  dump_stack+0x90/0xb4
[  105.754342]  hclge_phy_marvell_fixup+0x14/0x90 [hclge] <-- configure the led ctrl reg to 0x1040 here
[  105.764620]  phy_scan_fixups+0x80/0x108
[  105.772285]  phy_init_hw+0x4c/0x78
[  105.779080]  phy_attach_direct+0x1cc/0x240
[  105.787267]  phy_connect_direct+0x20/0x70
[  105.795283]  hclge_mac_connect_phy+0x54/0xb0 [hclge]
[  105.805214]  hns3_client_init+0x24c/0x330 [hns3]
[  105.814448]  hclge_init_client_instance+0xcc/0x1a0 [hclge]
[  105.825423]  hnae3_match_n_instantiate+0x4c/0x110 [hnae3]
[  105.836223]  hnae3_register_ae_dev+0xd4/0x1a8 [hnae3]
[  105.846329]  hns3_probe+0x5c/0x88 [hns3]
[  105.854169]  local_pci_probe+0x3c/0xb0
[  105.861659]  work_for_cpu_fn+0x18/0x28
[  105.869151]  process_one_work+0x1e4/0x458
[  105.877165]  worker_thread+0x228/0x450
[  105.884655]  kthread+0x12c/0x130
[  105.891102]  ret_from_fork+0x10/0x18
[  105.899661] iommu: Adding device 0000:bd:00.0 to group 21
[  105.912042] hns3 0000:bd:00.0: The firmware version is b0600114
[  105.939661] hclge driver initialization finished.
estuary:/home$
estuary:/home$
estuary:/home$ ethtool -i eth2   <-- I add debug code here to print the led reg value
[  154.466599] hns3 0000:7d:00.2 eth2: phy led reg vaule: 0x1177
driver: hns3
version: 4.19.13-g1962b78-dirty SMP pree
firmware-version: 0xb0600114
expansion-rom-version:
bus-info: 0000:7d:00.2
supports-statistics: yes
supports-test: yes
supports-eeprom-access: no
supports-register-dump: yes
supports-priv-flags: no
estuary:/home$ uname -a
Linux (none) 4.19.13-g1962b78-dirty #2 SMP PREEMPT Mon Jan 14 16:28:34 HKT 2019 aarch64 GNU/Linux

Re: Question about default m88e1510 LED configuration of marvell phy

[shenjian (K)]

在 2019/1/15 17:08, Wang, Dongsheng 写道:
> On 2019/1/15 14:41, shenjian (K) wrote:
>>
>> 在 2019/1/15 11:39, Andrew Lunn 写道:
>>> On Tue, Jan 15, 2019 at 11:07:17AM +0800, shenjian (K) wrote:
>>>> Hi, all
>>>> 	We encounted a problem when using the marvel 88e151x phy driver, the
>>>> link(LED[0])/ack(LED[1]) leds worked abnormally since we updated our kernel to v4.19.
>>>> The act led kept on and never blink.  It works well when we use genphy driver, aslo
>>>> works well when use marvell phy in kernel v4.18.
>>>>
>>>> 	I found that the default led configuration had been changed to 0x1177
>>>> by commit commit 077772468ec1(net: phy: marvell: change default m88e1510 LED configuration).
>>> It is a difficult job deciding what the driver should configure. I
>>> would actually prefer to keeps with the reset default, which would be
>>> 0x101e according to the datasheet.
>>>
>>> If you are using device tree, you have the option of poking values
>>> into registers, using marvell,reg-init. There are a few examples you
>>> can copy. Doing that guarantees you have the configuration you want.
>>>
>>>     Andrew
>>>
>>> .
>>>
>> Thanks, Andrew.
>>
>> But we are using acpi mode, is there any ohter way to poke values into registers ?
> 
> You can describe property in ACPI _DSD. And parse then in
> marvell_config_init just like of.
> 
> Cheers
> Dongsheng
> 
> 
> 
> .
> 
I will check is this supported by our bios.

Thanks, Dongsheng.

Re: Question about default m88e1510 LED configuration of marvell phy

[Andrew Lunn]

On Thu, Jan 17, 2019 at 10:58:07AM +0800, shenjian (K) wrote:
> 
> 
> 在 2019/1/17 0:04, Andrew Lunn 写道:
> >> Thanks, Andrew.
> >>
> >> But we are using acpi mode, is there any ohter way to poke values into registers ?
> > 
> > You can register a PHY fixup.
> > 
> > phy_register_fixup_for_uid(), mach-orion5x/dns323-setup.c
> > 
> >       Andrew
> > 
> > .
> > 
> Thanks Andrew!
> 
> I have tried phy_register_fixup_for_uid(), but it doesn't work .

Sorry for the brief reply.

Look closer at mach-orion5x/dns323-setup.c

      Andrew

Re: Question about default m88e1510 LED configuration of marvell phy

[shenjian (K)]

在 2019/1/17 20:41, Andrew Lunn 写道:
> On Thu, Jan 17, 2019 at 10:58:07AM +0800, shenjian (K) wrote:
>>
>>
>> 在 2019/1/17 0:04, Andrew Lunn 写道:
>>>> Thanks, Andrew.
>>>>
>>>> But we are using acpi mode, is there any ohter way to poke values into registers ?
>>>
>>> You can register a PHY fixup.
>>>
>>> phy_register_fixup_for_uid(), mach-orion5x/dns323-setup.c
>>>
>>>       Andrew
>>>
>>> .
>>>
>> Thanks Andrew!
>>
>> I have tried phy_register_fixup_for_uid(), but it doesn't work .
> 
> Sorry for the brief reply.
> 
> Look closer at mach-orion5x/dns323-setup.c
> 
>       Andrew
> 
> .
> 
Do you mean I can define a  specified dev_flags, and set the default led value
to I wante, based on the flag in config_init() ?
Thanks!