drivers/net/ethernet/micrel/ks8851_common.c | 5 ----- 1 file changed, 5 deletions(-)
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.0This 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
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 ?
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?
On 9/5/26 3:28 AM, Jakub Kicinski wrote: > 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) I do understand. This happens in other areas/projects too, although it seems netdev is really pummeled by this. >>>> @@ -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? I will do so in V2. I appreciate the human feedback. Thank you.
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
© 2016 - 2026 Red Hat, Inc.