[net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[Marek Vasut]

KSZ8851 errata sheet DS80000716D-page 4 Module 3 [1] states that,
when issuing a software power-down (PMECR[1:0] = 10) followed by a
power-on (PMECR[1:0] = 00), the receiver circuit can fail to start
properly preventing communication. The Transmitter will still send
data, but no data will be received.

The errata sheet also includes a workaround, which states that,
it is recommended that the software power-down feature not be used.

Implement that workaround and drop the entry into software power-down
mode. The ks8851_write_mac_addr() calls entry into normal power-on
mode at the very beginning of the function, therefore dropping the
second call to enter software power-down mode is sufficient here.
The ks8851_net_stop() can only be called after ks8851_net_start()
was already called, and ks8851_net_start() also makes the MAC enter
normal power-on mode, therefore it is also fine to drop the call to
enter software power-down mode from ks8851_net_stop().

This will lead to slight increase in power consumption, but it also
fixes a sporadic problem which occurs at least on KSZ8851-16MLL, on
which this fix is tested.

[1] https://ww1.microchip.com/downloads/en/DeviceDoc/80000716D.pdf

Fixes: 3ba81f3ece3c ("net: Micrel KS8851 SPI network driver")
Signed-off-by: Marek Vasut <marex@nabladev.com>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Yicong Hui <yiconghui@gmail.com>
Cc: kernel@dh-electronics.com
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/micrel/ks8851_common.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 4afbb40bc0e4a..cd8dfca22720b 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -139,17 +139,14 @@ static int ks8851_write_mac_addr(struct net_device *dev)
 	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
 
 	for (i = 0; i < ETH_ALEN; i += 2) {
 		val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1];
 		ks8851_wrreg16(ks, KS_MAR(i), val);
 	}
 
-	if (!netif_running(dev))
-		ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
-
 	ks8851_unlock(ks);
 
 	return 0;
 }
 
 /**
  * ks8851_read_mac_addr - read mac address from device registers
@@ -502,16 +499,14 @@ static int ks8851_net_stop(struct net_device *dev)
 	ks8851_lock(ks);
 	/* shutdown RX process */
 	ks8851_wrreg16(ks, KS_RXCR1, 0x0000);
 
 	/* shutdown TX process */
 	ks8851_wrreg16(ks, KS_TXCR, 0x0000);
 
-	/* set powermode to soft power down to save power */
-	ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
 	ks8851_unlock(ks);
 
 	/* ensure any queued tx buffers are dumped */
 	while (!skb_queue_empty(&ks->txq)) {
 		struct sk_buff *txb = skb_dequeue(&ks->txq);
 
 		netif_dbg(ks, ifdown, ks->netdev,
-- 
2.53.0

Re: [net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[Sebastian Andrzej Siewior]

On 2026-09-01 19:39:14 [+0200], Marek Vasut wrote:
> KSZ8851 errata sheet DS80000716D-page 4 Module 3 [1] states that,
> when issuing a software power-down (PMECR[1:0] = 10) followed by a
> power-on (PMECR[1:0] = 00), the receiver circuit can fail to start
> properly preventing communication. The Transmitter will still send
> data, but no data will be received.
> 
> The errata sheet also includes a workaround, which states that,
> it is recommended that the software power-down feature not be used.
> 
> Implement that workaround and drop the entry into software power-down
> mode. The ks8851_write_mac_addr() calls entry into normal power-on
> mode at the very beginning of the function, therefore dropping the
> second call to enter software power-down mode is sufficient here.
> The ks8851_net_stop() can only be called after ks8851_net_start()
> was already called, and ks8851_net_start() also makes the MAC enter
> normal power-on mode, therefore it is also fine to drop the call to
> enter software power-down mode from ks8851_net_stop().
> 
> This will lead to slight increase in power consumption, but it also
> fixes a sporadic problem which occurs at least on KSZ8851-16MLL, on
> which this fix is tested.
> 
> [1] https://ww1.microchip.com/downloads/en/DeviceDoc/80000716D.pdf
> 
> Fixes: 3ba81f3ece3c ("net: Micrel KS8851 SPI network driver")
> Signed-off-by: Marek Vasut <marex@nabladev.com>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The patch description matches the errata document and the change.

Sebastian

Re: [net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

Per the KSZ8851 errata (DS80000716D, Module 3), a software power-down
followed by a power-on can leave the receiver circuit non-functional. This
patch implements the documented workaround by dropping both transitions into
PMECR_PM_SOFTDOWN, in ks8851_write_mac_addr() and ks8851_net_stop().

Link: https://ww1.microchip.com/downloads/en/DeviceDoc/80000716D.pdf

> diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
> index 4afbb40bc0e4a..cd8dfca22720b 100644
> --- a/drivers/net/ethernet/micrel/ks8851_common.c
> +++ b/drivers/net/ethernet/micrel/ks8851_common.c
> @@ -143,9 +143,6 @@ static int ks8851_write_mac_addr(struct net_device *dev)
>  		ks8851_wrreg16(ks, KS_MAR(i), val);
>  	}
>  
> -	if (!netif_running(dev))
> -		ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
> -
>  	ks8851_unlock(ks);
>  
>  	return 0;

[Severity: Low]
This isn't a bug, but should the two comments describing the software
power-down state be updated in the same patch?

After this hunk, ks8851_write_mac_addr() still carries this comment above
its remaining ks8851_set_powermode() call:

	/*
	 * Wake up chip in case it was powered off when stopped; otherwise,
	 * the first write to the MAC address does not take effect.
	 */
	ks8851_set_powermode(ks, PMECR_PM_NORMAL);

Nothing in the driver can put the chip into software power-down anymore, so
the "in case it was powered off when stopped" precondition can no longer
occur. A grep of the tree confirms PMECR_PM_SOFTDOWN has no users left,
only its definition in ks8851.h, and the two remaining
ks8851_set_powermode() call sites in ks8851_write_mac_addr() and
ks8851_net_start() both write PMECR_PM_NORMAL.

Since the commit message uses that very call site as the argument that
dropping the power-down here is safe, would it help to note in the comment
that the PM_NORMAL write is kept deliberately (e.g. referencing the errata),
so a later cleanup does not remove it or reintroduce the SOFTDOWN
transition?

> @@ -506,8 +503,6 @@ static int ks8851_net_stop(struct net_device *dev)
>  	/* shutdown TX process */
>  	ks8851_wrreg16(ks, KS_TXCR, 0x0000);
>  
> -	/* set powermode to soft power down to save power */
> -	ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
>  	ks8851_unlock(ks);

[Severity: Low]
Related to the above: the kernel-doc for ks8851_net_stop() just above this
function still promises the low power transition that is being removed here:

	/**
	 * ks8851_net_stop - close network device
	 * @dev: The device being closed.
	 *
	 * Called to close down a network device which has been active. Cancel any
	 * work, shutdown the RX and TX process and then place the chip into a low
	 * power state whilst it is not being used.
	 */

The post-patch body only masks IER/ISR, zeroes RXCR1/TXCR, drains ks->txq
and calls free_irq(), with no PMECR access left. Should that last sentence
be dropped or reworded to mention the errata, so readers of
ks8851_suspend()->ks8851_net_stop() are not told a low power transition
happens?
-- 
pw-bot: cr

Re: [net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[Marek Vasut]

On 9/4/26 2:37 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch.

Sigh, a wall of generated text.

> The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down
> 
> Per the KSZ8851 errata (DS80000716D, Module 3), a software power-down
> followed by a power-on can leave the receiver circuit non-functional. This
> patch implements the documented workaround by dropping both transitions into
> PMECR_PM_SOFTDOWN, in ks8851_write_mac_addr() and ks8851_net_stop().
> 
> Link: https://ww1.microchip.com/downloads/en/DeviceDoc/80000716D.pdf
> 
>> diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
>> index 4afbb40bc0e4a..cd8dfca22720b 100644
>> --- a/drivers/net/ethernet/micrel/ks8851_common.c
>> +++ b/drivers/net/ethernet/micrel/ks8851_common.c
>> @@ -143,9 +143,6 @@ static int ks8851_write_mac_addr(struct net_device *dev)
>>   		ks8851_wrreg16(ks, KS_MAR(i), val);
>>   	}
>>   
>> -	if (!netif_running(dev))
>> -		ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
>> -
>>   	ks8851_unlock(ks);
>>   
>>   	return 0;
> 
> [Severity: Low]
> This isn't a bug

Aha ...

>, but should the two comments describing the software
> power-down state be updated in the same patch?
> 
> After this hunk, ks8851_write_mac_addr() still carries this comment above
> its remaining ks8851_set_powermode() call:
> 
> 	/*
> 	 * Wake up chip in case it was powered off when stopped; otherwise,
> 	 * the first write to the MAC address does not take effect.
> 	 */
> 	ks8851_set_powermode(ks, PMECR_PM_NORMAL);
> 
> Nothing in the driver can put the chip into software power-down anymore, so
> the "in case it was powered off when stopped" precondition can no longer
> occur. A grep of the tree confirms PMECR_PM_SOFTDOWN has no users left,
> only its definition in ks8851.h, and the two remaining
> ks8851_set_powermode() call sites in ks8851_write_mac_addr() and
> ks8851_net_start() both write PMECR_PM_NORMAL.

The driver obviously does not exist in a vacuum and prior stage before 
Linux could have easily left the MAC in whatever powered down state. 
This is bogus feedback.

> Since the commit message uses that very call site as the argument that
> dropping the power-down here is safe, would it help to note in the comment
> that the PM_NORMAL write is kept deliberately (e.g. referencing the errata),
> so a later cleanup does not remove it or reintroduce the SOFTDOWN
> transition?
> 
>> @@ -506,8 +503,6 @@ static int ks8851_net_stop(struct net_device *dev)
>>   	/* shutdown TX process */
>>   	ks8851_wrreg16(ks, KS_TXCR, 0x0000);
>>   
>> -	/* set powermode to soft power down to save power */
>> -	ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
>>   	ks8851_unlock(ks);
> 
> [Severity: Low]
> Related to the above: the kernel-doc for ks8851_net_stop() just above this
> function still promises the low power transition that is being removed here:
> 
> 	/**
> 	 * ks8851_net_stop - close network device
> 	 * @dev: The device being closed.
> 	 *
> 	 * Called to close down a network device which has been active. Cancel any
> 	 * work, shutdown the RX and TX process and then place the chip into a low
> 	 * power state whilst it is not being used.
> 	 */
> 
> The post-patch body only masks IER/ISR, zeroes RXCR1/TXCR, drains ks->txq
> and calls free_irq(), with no PMECR access left. Should that last sentence
> be dropped or reworded to mention the errata, so readers of
> ks8851_suspend()->ks8851_net_stop() are not told a low power transition
> happens?

What exactly should I adjust in V2 ?

Re: [net,PATCH v1] net: ks8851: Fix receiver error in 100BASE-TX mode following software power-down

[Jakub Kicinski]

On Sat, 5 Sep 2026 03:16:12 +0200 Marek Vasut wrote:
> On 9/4/26 2:37 AM, Jakub Kicinski wrote:
> > This is an AI-generated review of your patch.  
> 
> Sigh, a wall of generated text.

I'd feel bad if most of the patches I have to suffer thru
these days weren't entirely YOLO'ed slop where every lines comes 
with a 15 line comment :/ (not speaking about your patch, just
a general vent)

> >> @@ -506,8 +503,6 @@ static int ks8851_net_stop(struct net_device *dev)
> >>   	/* shutdown TX process */
> >>   	ks8851_wrreg16(ks, KS_TXCR, 0x0000);
> >>   
> >> -	/* set powermode to soft power down to save power */
> >> -	ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
> >>   	ks8851_unlock(ks);  
> > 
> > [Severity: Low]
> > Related to the above: the kernel-doc for ks8851_net_stop() just above this
> > function still promises the low power transition that is being removed here:
> > 
> > 	/**
> > 	 * ks8851_net_stop - close network device
> > 	 * @dev: The device being closed.
> > 	 *
> > 	 * Called to close down a network device which has been active. Cancel any
                                                vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
> > 	 * work, shutdown the RX and TX process and then place the chip into a low
           vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvXXXX^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 	 * power state whilst it is not being used.\
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 	 */
> > 
> > The post-patch body only masks IER/ISR, zeroes RXCR1/TXCR, drains ks->txq
> > and calls free_irq(), with no PMECR access left. Should that last sentence
> > be dropped or reworded to mention the errata, so readers of
> > ks8851_suspend()->ks8851_net_stop() are not told a low power transition
> > happens?  
> 
> What exactly should I adjust in V2 ?

Drop the part of the sentence in the ks8851_net_stop() kdoc that says
we'll put the device in low power mode?