[PATCH net-next v7 0/3] w5100: restore GPIO-based link detection

[PATCH net-next v7 0/3] w5100: restore GPIO-based link detection

[Arthur Crepin Leblond]

Hi,

In the previous version of the w5100 driver, the LINKLED pin could be
wired to a GPIO to detect the link status changes via an interrupt.
This only works on w5500 as it is the only one documenting its LINKLED
pin to hold the link status (on w5100 the LINKLED pin is changing with
RX/TX activity).

This series of patches is bringing back the link status detection on
w5500 using the Link Status bit of the PHY Configuration Register.
Additionally, the LINKLED pin can also be wired to detect link status
changes and read the register in the interrupt handler.

Arthur Crepin Leblond

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
- Link to v6: https://patch.msgid.link/20260831-wiznet-link-gpio-v6-0-abee6c5c966e@marmottus.net
- Link to v5: https://patch.msgid.link/20260813-wiznet-link-gpio-v5-0-ac8046f24413@marmottus.net
- Link to v4: https://patch.msgid.link/20260811-wiznet-link-gpio-v4-0-4d5f7da885a8@marmottus.net
- Link to v3: https://patch.msgid.link/20260806-wiznet-link-gpio-v3-0-532d4a143805@marmottus.net
- Link to v2: https://patch.msgid.link/20260804-wiznet-link-gpio-v2-0-3b1d0c870f35@marmottus.net
- Link to v1: https://patch.msgid.link/20260804-wiznet-link-gpio-v1-1-b626fd4f7ccb@marmottus.net
---
Arthur Crepin Leblond (3):
      dt-bindings: net: wiznet,w5100: convert to DT schema
      dt-bindings: net: wiznet,w5100: add link status interrupt
      w5100: detect carrier state using link status bit and optional interrupt

 .../devicetree/bindings/net/wiznet,w5100.yaml      |  71 ++++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       |  50 ---------
 drivers/net/ethernet/wiznet/w5100.c                | 123 +++++++++++++++++++--
 3 files changed, 183 insertions(+), 61 deletions(-)

[PATCH net-next v7 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

[Arthur Crepin Leblond]

Convert the Wiznet w5x00 SPI Ethernet controller binding from
plain text to DT schema.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---
v6:
 - Put back the reviewed by tag from Krzysztof Kozlowski from v4

v5:
 - Remove gpio/gpio.h include from DT example
 - Make the DT interrupts property dynamic from the compatible one
 - Fix DT bindings errors

v4:
 - Update interrupts in DT bindings

v3:
 - Fix DT binding errors

v2:
 - Convert device tree binding to YAML
---
 .../devicetree/bindings/net/wiznet,w5100.yaml      | 66 ++++++++++++++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       | 50 ----------------
 2 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
new file mode 100644
index 000000000000..601d828bae92
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/wiznet,w5100.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Wiznet w5100/w5200/w5500 SPI Ethernet Controller
+
+maintainers:
+  - Arthur Crepin Leblond <arthur@marmottus.net>
+
+description: |
+  This is a standalone 10/100 MBit Ethernet controller with SPI interface.
+
+allOf:
+  - $ref: /schemas/net/ethernet-controller.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - wiznet,w5100
+      - wiznet,w5200
+      - wiznet,w5500
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    description:
+      According to the w5500 datasheet, the chip allows a maximum of 80 MHz,
+      however, board designs may need to limit this value.
+    maximum: 80000000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /* Example (for Raspberry Pi with pin control stuff for GPIO irq) */
+
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet@0 {
+            compatible = "wiznet,w5500";
+            reg = <0>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&eth1_pins>;
+            interrupt-parent = <&gpio>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            spi-max-frequency = <30000000>;
+            local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
+        };
+    };
+...
diff --git a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt b/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
deleted file mode 100644
index e9665798c4be..000000000000
--- a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-* Wiznet w5x00
-
-This is a standalone 10/100 MBit Ethernet controller with SPI interface.
-
-For each device connected to a SPI bus, define a child node within
-the SPI master node.
-
-Required properties:
-- compatible: Should be one of the following strings:
-	      "wiznet,w5100"
-	      "wiznet,w5200"
-	      "wiznet,w5500"
-- reg: Specify the SPI chip select the chip is wired to.
-- interrupts: Specify the interrupt index within the interrupt controller (referred
-              to above in interrupt-parent) and interrupt type. w5x00 natively
-              generates falling edge interrupts, however, additional board logic
-              might invert the signal.
-- pinctrl-names: List of assigned state names, see pinctrl binding documentation.
-- pinctrl-0: List of phandles to configure the GPIO pin used as interrupt line,
-             see also generic and your platform specific pinctrl binding
-             documentation.
-
-Optional properties:
-- spi-max-frequency: Maximum frequency of the SPI bus when accessing the w5500.
-  According to the w5500 datasheet, the chip allows a maximum of 80 MHz, however,
-  board designs may need to limit this value.
-- local-mac-address: See ethernet.txt in the same directory.
-
-
-Example (for Raspberry Pi with pin control stuff for GPIO irq):
-
-&spi {
-	ethernet@0: w5500@0 {
-		compatible = "wiznet,w5500";
-		reg = <0>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&eth1_pins>;
-		interrupt-parent = <&gpio>;
-		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-		spi-max-frequency = <30000000>;
-	};
-};
-
-&gpio {
-	eth1_pins: eth1_pins {
-		brcm,pins = <25>;
-		brcm,function = <0>; /* in */
-		brcm,pull = <0>; /* none */
-	};
-};

-- 
2.55.0

Re: [PATCH net-next v7 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1 (1)

- [Low] In the newly added
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml, `spi-max-…

Pre-existing issues:
- [Low] Documentation/devicetree/bindings/net/wiznet,w5100.yaml (added by
  this patch) constrains `interrupts` only with a count (`maxItems: 1`…

--

dt-bindings: net: wiznet,w5100: convert to DT schema

This converts the Wiznet w5x00 SPI Ethernet controller binding from plain
text to a YAML schema, adding the file wiznet,w5100.yaml and deleting
wiznet,w5x00.txt.  The new schema enumerates the three compatibles,
constrains reg/interrupts/spi-max-frequency, and carries over the
Raspberry Pi example.

A couple of questions below about the interrupt and spi-max-frequency
descriptions.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> new file mode 100644
> index 0000000000000..601d828bae927
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -0,0 +1,66 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - wiznet,w5100
> +      - wiznet,w5200
> +      - wiznet,w5500
> +
> +  interrupts:
> +    maxItems: 1

[Severity: Low]
This isn't an issue introduced by this patch, but the interrupt polarity
disagreement between the binding and the driver is now the only in-tree
record of it, and the wording that described it is being dropped here.

The deleted wiznet,w5x00.txt said:

  w5x00 natively generates falling edge interrupts, however, additional
  board logic might invert the signal.

The new schema has no description and no type constraint under
interrupts, so that information disappears from the tree.

Meanwhile w5100_probe() in drivers/net/ethernet/wiznet/w5100.c hardcodes a
level-low trigger in both registration paths:

	err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
				   netdev_name(ndev), ndev);
	} else {
		err = request_irq(priv->irq, w5100_interrupt,
				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);

and __setup_irq() in kernel/irq/manage.c applies the driver flags over
whatever the DT node asked for:

		if (new->flags & IRQF_TRIGGER_MASK) {
			ret = __irq_set_trigger(desc,
						new->flags & IRQF_TRIGGER_MASK);

So no interrupts are lost, but the type in a node copied from the example
is never the one the line is programmed with, and request_irq() fails
outright on an irqchip that cannot do level triggering.

Would it make sense to either describe/constrain the type as level-low in
the schema, or have the driver pass IRQF_TRIGGER_NONE and honour the DT
type?

> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      According to the w5500 datasheet, the chip allows a maximum of 80 MHz,
> +      however, board designs may need to limit this value.
> +    maximum: 80000000

[Severity: Low]
Should this ceiling be per-compatible?  The description scopes the 80 MHz
figure to the w5500 only ("According to the w5500 datasheet"), and the
deleted .txt scoped it the same way ("when accessing the w5500"), but the
constraint sits in the shared properties block so wiznet,w5100 and
wiznet,w5200 inherit it too.

The older parts have lower SCLK limits, so a node like:

    ethernet@0 {
        compatible = "wiznet,w5100";
        spi-max-frequency = <80000000>;
        ...
    };

passes make dt_binding_check with no diagnostic, and w5100-spi.c then
clocks the bus at that rate.

Other net SPI bindings in the same directory express this per variant
inside a compatible conditional, for example the spi-max-frequency block
in Documentation/devicetree/bindings/net/qca,qca7000.yaml:

        spi-max-frequency:
          default: 8000000
          maximum: 16000000
          minimum: 1000000

Would an allOf/if:properties:compatible:contains: block per part be
better here, or a description explaining why 80 MHz is safe for all
three?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:

[ ... ]

> +        ethernet@0 {
> +            compatible = "wiznet,w5500";
> +            reg = <0>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&eth1_pins>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;

[Severity: Low]
This is the same pre-existing mismatch noted above and not something this
patch introduces; the edge-falling type was in the old .txt example
verbatim.  Since this example is now the only surviving statement of the
expected trigger type, is IRQ_TYPE_LEVEL_LOW the value to show, given
w5100_probe() requests IRQF_TRIGGER_LOW?

Nothing in-tree uses these compatibles today (a grep for wiznet,w5 matches
only the binding and w5100-spi.c), so no shipped board DT is affected
either way.

> +            spi-max-frequency = <30000000>;
> +            local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
> +        };
> +    };
> +...

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net

[PATCH net-next v7 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[Arthur Crepin Leblond]

Add an optional link status interrupt which can be connected to the
LINKLED pin on the device to detect link status changes.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---
v6:
 - Fix DT error: use minItems
 - Remove conditional properties

v5:
 - Make the DT interrupts property dynamic from the compatible one
 - Fix DT bindings errors

v4:
 - Update interrupts in DT bindings

v3:
 - Fix DT binding errors

v2:
 - Convert device tree binding to YAML
---
 Documentation/devicetree/bindings/net/wiznet,w5100.yaml | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
index 601d828bae92..d0717ab45de6 100644
--- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
+++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
@@ -24,7 +24,11 @@ properties:
       - wiznet,w5500
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Main interrupt (INT pin on the device).
+      - description:
+          Optional interrupt for link status changes (LINKLED pin on w5500).
 
   reg:
     maxItems: 1
@@ -58,7 +62,8 @@ examples:
             pinctrl-names = "default";
             pinctrl-0 = <&eth1_pins>;
             interrupt-parent = <&gpio>;
-            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>,
+                         <24 IRQ_TYPE_EDGE_BOTH>;
             spi-max-frequency = <30000000>;
             local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
         };

-- 
2.55.0

Re: [PATCH net-next v7 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Documentation/devicetree/bindings/net/wiznet,w5100.yaml: the
  newly added second `interrupts` entry is described as being…
- [Low] Concern (assessed as a FALSE POSITIVE; retained only because
  requires_validation=true): the claim that the newly documented second…

--

dt-bindings: net: wiznet,w5100: add link status interrupt

The wiznet,w5100 binding is extended so that the interrupts property can
carry a second entry for a link status interrupt wired to the LINKLED pin,
and the example is updated to show both interrupts.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 601d828bae927..d0717ab45de6c 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -24,7 +24,11 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Main interrupt (INT pin on the device).
> +      - description:
> +          Optional interrupt for link status changes (LINKLED pin on w5500).

[Severity: Low]

The description of the second entry says the pin is on the w5500, but the
constraint sits in the top-level properties block with no allOf/if-then
keyed on compatible.  The compatible enum in this same file is:

  compatible:
    enum:
      - wiznet,w5100
      - wiznet,w5200
      - wiznet,w5500

so a node such as:

  ethernet@0 {
      compatible = "wiznet,w5100";
      interrupts = <25 IRQ_TYPE_EDGE_FALLING>,
                   <24 IRQ_TYPE_EDGE_BOTH>;
      ...
  };

passes make dt_binding_check even though the schema text says the pin only
exists on the w5500.

Should the item count be tied to the compatible string, for example:

  allOf:
    - if:
        properties:
          compatible:
            contains:
              enum:
                - wiznet,w5100
                - wiznet,w5200
      then:
        properties:
          interrupts:
            maxItems: 1

The driver side of the series treats index 1 as w5500-only:

drivers/net/ethernet/wiznet/w5100.c:w5100_probe() {
    ...
    priv->link_irq = ops->chip_id == W5500 ?
                     fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;
    ...
}

with w5100_get_link() returning a hardcoded 1 for the parts that have no
PHYCFGR link-status register.  So for w5100/w5200 a second interrupt in DT
would validate but then be silently ignored, since fwnode_irq_get() is
never called for index 1 on those chip ids.

Alternatively, if LINKLED wiring is equally describable on w5100 and w5200,
should the description be widened instead of naming only the w5500?

Nothing in the rest of the series changes this block, so the published
schema stays permissive for all three compatibles.  Adding the conditional
later would tighten the schema rather than break the ABI, so it seems it
could still be done, but it seems better to get it right here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net

[PATCH net-next v7 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Arthur Crepin Leblond]

Detect the link status on the w5500 using the Link Status bit from the
PHY Configuration register (PHYCFGR).

On w5100/w5200, which lack this register, the link is always reported
as up.

Also add an optional interrupt, wired to the LINKLED pin of
the w5500, to detect link status changes and read the PHYCFGR register.
The initial link state is always probed from PHYCFGR regardless of
whether the link interrupt is available. The interrupt is only needed
to detect subsequent link changes.

Make sure that the netdev is registered last in the probe function so
no operation can be performed before the probing is finished.

The w5500 interrupt is only enabled when the netdev is opened and
disabled on stop. Same behavior for suspend/resume.

Commit dacf281771a9 ("w5100: remove unused gpio link detection")
dropped the link_gpio/link_irq handling on the grounds that no
devicetree user passed a "link" gpio at the time and that it used the
old gpio interface.
This isn't a plain revert of that removal: link detection is now done
using a second interrupt rather than a gpio with a documented DT binding.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

---
v7:
 - Address Sashiko reviews
 - Propagate spi read error to the caller
 - Do not change the state of the carrier on SPI read error
 - Enable the link IRQ before reading the PHYCFGR bit
 - Call unregister_netdev before reset
 - Update commit message

v6:
 - Revert to the reviewed v4 version by Arnd Bergmann with a few
changes based on Sashiko's review:
 - call netif_carrier_off in open if there is no link_irq or  get_link
 returns false
 - call register_netdev at the very end of probe
 - call unregister_netdev before destroying work queue
 - handle link irq probe defer error
 - call netif_device_attach in resume before reenabling the link irq
 - disable the link irq first in suspend
 - log netif_err when PHYCFGR cannot be read
 - enable link irq in open
 - disable link irq in stop
 - free irq and invalidate it in remove

v5:
 - Read the link status register only on w5500 instead of checking link_irq
 - Address sashiko review
 - call netif_carrier_off on w5500 link status off in open
 - enable/disable the link irq in open/stop
 - enable/disable the link irq in resume/suspend
 - handle link irq probe defer error
 - register the netdev last

v4:
 - Use directly an interrupt line instead of gpio -> irq
 - Address sashiko reviews
 - drop devm_ on request_threaded_irq to avoid use after free
 - disable/enable the link_irq in the suspend/resume
 - only call netif_carrier_on|off if the link interrupt is present

v3:
 - Use the Link Status bit of the PHY Configuration Register
 - Use the LINKLED gpio binding for change detection only

v2:
 - Use devm_request_threaded_irq instead of request_any_context_irq
 - Use devm_gpiod_get_optional instead of gpiod_get_optional
 - Call dev_err_probe on gpiod_to_irq failure
 - Remove link_irq from priv
 - Use a fixed string for the IRQ name
 - Remove empty new lines
---
 drivers/net/ethernet/wiznet/w5100.c | 123 ++++++++++++++++++++++++++++++++----
 1 file changed, 112 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
index 53d8dc642fbd..ecb99621d0a1 100644
--- a/drivers/net/ethernet/wiznet/w5100.c
+++ b/drivers/net/ethernet/wiznet/w5100.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
+#include <linux/property.h>
 #include <linux/irq.h>
 
 #include "w5100.h"
@@ -124,6 +125,8 @@ MODULE_LICENSE("GPL");
  */
 #define W5500_SIMR		0x0018 /* Socket Interrupt Mask Register */
 #define W5500_RTR		0x0019 /* Retry Time-value Register */
+#define W5500_PHYCFGR		0x002e /* PHY Configuration Register */
+#define   PHYCFGR_LNK		  0x01 /* Link status */
 
 #define W5500_S0_REGS		0x10000
 
@@ -154,6 +157,7 @@ struct w5100_priv {
 	u16 s0_rx_buf_size;
 
 	int irq;
+	int link_irq;
 
 	struct napi_struct napi;
 	struct net_device *ndev;
@@ -345,6 +349,44 @@ static void w5500_memory_configure(struct w5100_priv *priv)
 	}
 }
 
+static int w5500_get_phycfgr_lnk(struct net_device *ndev)
+{
+	struct w5100_priv *priv = netdev_priv(ndev);
+	int ret = w5100_read(priv, W5500_PHYCFGR);
+
+	if (ret < 0) {
+		netif_err(priv, link, ndev,
+			  "failed to read link status: %d\n", ret);
+		return ret;
+	}
+
+	return ret & PHYCFGR_LNK;
+}
+
+static irqreturn_t w5500_detect_link(int irq, void *ndev_instance)
+{
+	struct net_device *ndev = ndev_instance;
+	struct w5100_priv *priv = netdev_priv(ndev);
+	int state;
+
+	if (!netif_running(ndev))
+		return IRQ_HANDLED;
+
+	state = w5500_get_phycfgr_lnk(ndev);
+	if (state < 0)
+		return IRQ_HANDLED;
+
+	if (state) {
+		netif_info(priv, link, ndev, "link is up\n");
+		netif_carrier_on(ndev);
+	} else {
+		netif_info(priv, link, ndev, "link is down\n");
+		netif_carrier_off(ndev);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int w5100_hw_reset(struct w5100_priv *priv)
 {
 	u32 rtr;
@@ -414,6 +456,16 @@ static void w5100_get_drvinfo(struct net_device *ndev,
 		sizeof(info->bus_info));
 }
 
+static u32 w5100_get_link(struct net_device *ndev)
+{
+	struct w5100_priv *priv = netdev_priv(ndev);
+
+	if (priv->ops->chip_id == W5500)
+		return w5500_get_phycfgr_lnk(ndev) > 0;
+
+	return 1;
+}
+
 static u32 w5100_get_msglevel(struct net_device *ndev)
 {
 	struct w5100_priv *priv = netdev_priv(ndev);
@@ -659,6 +711,15 @@ static int w5100_open(struct net_device *ndev)
 	w5100_hw_start(priv);
 	napi_enable(&priv->napi);
 	netif_start_queue(ndev);
+
+	if (priv->link_irq >= 0)
+		enable_irq(priv->link_irq);
+
+	if (w5100_get_link(ndev))
+		netif_carrier_on(ndev);
+	else
+		netif_carrier_off(ndev);
+
 	return 0;
 }
 
@@ -667,6 +728,10 @@ static int w5100_stop(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifdown, ndev, "shutting down\n");
+
+	if (priv->link_irq >= 0)
+		disable_irq(priv->link_irq);
+
 	w5100_hw_close(priv);
 	netif_carrier_off(ndev);
 	netif_stop_queue(ndev);
@@ -678,6 +743,7 @@ static const struct ethtool_ops w5100_ethtool_ops = {
 	.get_drvinfo		= w5100_get_drvinfo,
 	.get_msglevel		= w5100_get_msglevel,
 	.set_msglevel		= w5100_set_msglevel,
+	.get_link		= w5100_get_link,
 	.get_regs_len		= w5100_get_regs_len,
 	.get_regs		= w5100_get_regs,
 };
@@ -745,13 +811,21 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 		break;
 	default:
 		err = -EINVAL;
-		goto err_register;
+		goto err_wq;
 	}
 
 	priv->ndev = ndev;
 	priv->ops = ops;
 	priv->irq = irq;
 
+	priv->link_irq = ops->chip_id == W5500 ?
+			 fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;
+	if (priv->link_irq == -EPROBE_DEFER) {
+		err = dev_err_probe(dev, priv->link_irq,
+				    "failed to get link irq\n");
+		goto err_wq;
+	}
+
 	ndev->netdev_ops = &w5100_netdev_ops;
 	ndev->ethtool_ops = &w5100_ethtool_ops;
 	netif_napi_add_weight(ndev, &priv->napi, w5100_napi_poll, 16);
@@ -761,12 +835,8 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 	 */
 	ndev->features |= NETIF_F_VLAN_CHALLENGED;
 
-	err = register_netdev(ndev);
-	if (err < 0)
-		goto err_register;
-
 	priv->xfer_wq = alloc_workqueue("%s", WQ_MEM_RECLAIM | WQ_PERCPU, 0,
-					netdev_name(ndev));
+					dev_name(dev));
 	if (!priv->xfer_wq) {
 		err = -ENOMEM;
 		goto err_wq;
@@ -795,21 +865,39 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 	if (ops->may_sleep) {
 		err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
 					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-					   netdev_name(ndev), ndev);
+					   dev_name(dev), ndev);
 	} else {
 		err = request_irq(priv->irq, w5100_interrupt,
-				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);
+				  IRQF_TRIGGER_LOW, dev_name(dev), ndev);
 	}
 	if (err)
 		goto err_hw;
 
+	if (priv->link_irq >= 0) {
+		err = request_threaded_irq(priv->link_irq, NULL,
+					   w5500_detect_link,
+					   IRQF_TRIGGER_RISING |
+					   IRQF_TRIGGER_FALLING |
+					   IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					   "w5100-link", priv->ndev);
+		if (err < 0)
+			goto err_link_irq;
+	}
+
+	err = register_netdev(ndev);
+	if (err < 0)
+		goto err_register;
+
 	return 0;
 
+err_register:
+	if (priv->link_irq >= 0)
+		free_irq(priv->link_irq, ndev);
+err_link_irq:
+	free_irq(priv->irq, ndev);
 err_hw:
 	destroy_workqueue(priv->xfer_wq);
 err_wq:
-	unregister_netdev(ndev);
-err_register:
 	free_netdev(ndev);
 	return err;
 }
@@ -820,6 +908,11 @@ void w5100_remove(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct w5100_priv *priv = netdev_priv(ndev);
 
+	unregister_netdev(ndev);
+
+	if (priv->link_irq >= 0)
+		free_irq(priv->link_irq, ndev);
+
 	w5100_hw_reset(priv);
 	free_irq(priv->irq, ndev);
 
@@ -827,7 +920,6 @@ void w5100_remove(struct device *dev)
 	flush_work(&priv->restart_work);
 	destroy_workqueue(priv->xfer_wq);
 
-	unregister_netdev(ndev);
 	free_netdev(ndev);
 }
 EXPORT_SYMBOL_GPL(w5100_remove);
@@ -839,6 +931,9 @@ static int w5100_suspend(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
+		if (priv->link_irq >= 0)
+			disable_irq(priv->link_irq);
+
 		netif_carrier_off(ndev);
 		netif_device_detach(ndev);
 
@@ -857,6 +952,12 @@ static int w5100_resume(struct device *dev)
 		w5100_hw_start(priv);
 
 		netif_device_attach(ndev);
+
+		if (priv->link_irq >= 0)
+			enable_irq(priv->link_irq);
+
+		if (w5100_get_link(ndev))
+			netif_carrier_on(ndev);
 	}
 	return 0;
 }

-- 
2.55.0

Re: [PATCH net-next v7 3/3] w5100: detect carrier state using link status bit and optional interrupt

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 1

- [High] On w5500 the carrier state is now sampled exactly once per ifup
  (and once per resume) from PHYCFGR, with no polling fallback and no…
- [Medium] `w5500_get_phycfgr_lnk()` returns a tri-state value (<0 on
  SPI/register read error, 0 link down, 1 link up), and its consumers…
- [Medium] The carrier state is computed and published by two
  unsynchronized contexts.
- [Low] The YAML binding added earlier in this series
  (Documentation/devicetree/bindings/net/wiznet,w5100.yaml, commit "dt-…

--

w5100: detect carrier state using link status bit and optional interrupt

This adds link detection on the w5500 by reading the Link Status bit of
the PHY Configuration register, and adds an optional second interrupt
(LINKLED) to notice link changes. The netdev is now registered as the
last step of w5100_probe(), and the link interrupt is enabled/disabled
in open/stop and resume/suspend.

A few questions about the carrier handling below.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..ecb99621d0a1e 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -345,6 +349,44 @@ static void w5500_memory_configure(struct w5100_priv *priv)
>  	}
>  }
>  
> +static int w5500_get_phycfgr_lnk(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +	int ret = w5100_read(priv, W5500_PHYCFGR);
> +
> +	if (ret < 0) {
> +		netif_err(priv, link, ndev,
> +			  "failed to read link status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret & PHYCFGR_LNK;
> +}
> +
> +static irqreturn_t w5500_detect_link(int irq, void *ndev_instance)
> +{
> +	struct net_device *ndev = ndev_instance;
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +	int state;
> +
> +	if (!netif_running(ndev))
> +		return IRQ_HANDLED;
> +
> +	state = w5500_get_phycfgr_lnk(ndev);
> +	if (state < 0)
> +		return IRQ_HANDLED;
> +
> +	if (state) {
> +		netif_info(priv, link, ndev, "link is up\n");
> +		netif_carrier_on(ndev);
> +	} else {
> +		netif_info(priv, link, ndev, "link is down\n");
> +		netif_carrier_off(ndev);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +

[ ... ]

> @@ -414,6 +456,16 @@ static void w5100_get_drvinfo(struct net_device *ndev,
>  		sizeof(info->bus_info));
>  }
>  
> +static u32 w5100_get_link(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (priv->ops->chip_id == W5500)
> +		return w5500_get_phycfgr_lnk(ndev) > 0;

[Severity: Medium]
Does the "> 0" here turn a register read failure into a definite "link
down"?

w5500_get_phycfgr_lnk() returns three distinct things: a negative errno,
0 for link down and 1 for link up.  w5500_spi_read() can really return an
error, since it propagates spi_write_then_read():

	return ret ? ret : data;

w5500_detect_link() deliberately leaves the carrier alone in that case:

	state = w5500_get_phycfgr_lnk(ndev);
	if (state < 0)
		return IRQ_HANDLED;

but w5100_get_link() folds the errno into 0, so w5100_open() will call
netif_carrier_off() on a transient SPI failure even when the link is up,
and w5100_resume() (which has no else branch) will leave the carrier off.
The same lossy mapping is what the newly added ethtool .get_link reports,
so an SPI error shows up as "Link detected: no".

Would it be better to keep the error case out of the carrier decision, the
way the interrupt handler does?

> +
> +	return 1;
> +}
> +

[ ... ]

> @@ -659,6 +711,15 @@ static int w5100_open(struct net_device *ndev)
>  	w5100_hw_start(priv);
>  	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
> +
> +	if (priv->link_irq >= 0)
> +		enable_irq(priv->link_irq);
> +
> +	if (w5100_get_link(ndev))
> +		netif_carrier_on(ndev);
> +	else
> +		netif_carrier_off(ndev);

[Severity: High]
Can the interface end up permanently NO-CARRIER on a w5500 board whose
device tree only declares the single INT interrupt?

The second (LINKLED) interrupt is optional and is introduced by this
series, so on existing device trees w5100_probe() does:

	priv->link_irq = ops->chip_id == W5500 ?
			 fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;

fwnode_irq_get() returns a negative value for index 1, no link interrupt
is requested, and every "if (priv->link_irq >= 0)" site is skipped.

That leaves the sample taken here as the only carrier update for the whole
lifetime of the ifup.  If PHYCFGR.LNK reads 0 at that moment (cable not
plugged, or auto-negotiation not finished yet after the reset done in
w5100_probe() -> w5100_hw_reset()), netif_carrier_off() is called and
nothing turns it back on: the only netif_carrier_on() call sites are
w5500_detect_link(), w5100_open() and w5100_resume(), and there is no
timer or delayed work polling PHYCFGR.

Before this patch the w5500 carrier was never cleared at ifup, so is this
a behaviour change for existing w5500 device trees, where a link that
comes up after ifup is never noticed until the user bounces the interface?

The commit message says:

	"The interrupt is only needed to detect subsequent link changes."

Could this state that without the interrupt there is no detection of
subsequent link changes at all, including the initial link-up?

[Severity: Medium]
Separately, does the ordering of enable_irq() and the PHYCFGR sample here
allow the threaded handler's newer verdict to be overwritten by an older
one?

__dev_open() sets __LINK_STATE_START before calling ndo_open, so
netif_running() is already true and the early bail in w5500_detect_link()
does not exclude a concurrent run.  enable_irq() can also replay an edge
that arrived while the line was masked:

w5100_open()
	enable_irq(priv->link_irq);
	w5100_get_link()	  /* sleeping SPI read, old value */

			w5500_detect_link()	 /* newer PHYCFGR value */
				netif_carrier_on(ndev);

	netif_carrier_off(ndev);  /* overwrites with the stale sample */

Nothing serializes the read-and-publish pair: the handler takes no lock,
open holds only RTNL which the IRQ thread never takes, and struct
w5100_priv has no lock covering the carrier update.  With edge-only
notification and no polling, the wrong state then persists until the link
toggles again.

w5100_resume() has the same enable-then-sample order and only ever calls
netif_carrier_on(), so a "down" seen by the handler can be discarded.

Note the teardown paths do it the other way around and disable_irq()
first, which does synchronize with the threaded handler.

> +
>  	return 0;
>  }
>  

[ ... ]

> @@ -745,13 +811,21 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  		break;
>  	default:
>  		err = -EINVAL;
> -		goto err_register;
> +		goto err_wq;
>  	}
>  
>  	priv->ndev = ndev;
>  	priv->ops = ops;
>  	priv->irq = irq;
>  
> +	priv->link_irq = ops->chip_id == W5500 ?
> +			 fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;

[Severity: Low]
This isn't a bug in the C code, but does the binding added earlier in the
series match this restriction?

Documentation/devicetree/bindings/net/wiznet,w5100.yaml describes the
interrupts as:

  interrupts:
    minItems: 1
    items:
      - description: Main interrupt (INT pin on the device).
      - description:
          Optional interrupt for link status changes (LINKLED pin on w5500).

with no allOf/if-then keyed on the compatible string, so wiznet,w5100 and
wiznet,w5200 nodes may declare two interrupts and still pass
dt_binding_check.  Here index 1 is only looked up for chip_id == W5500, so
such a line is silently ignored (priv->link_irq stays -ENODEV).

Would an if-then block limiting the second interrupt to wiznet,w5500 (or
honouring the second interrupt for all parts) express this better?  The
w5500-only restriction is currently only in free-form description text,
which validation does not enforce.

> +	if (priv->link_irq == -EPROBE_DEFER) {
> +		err = dev_err_probe(dev, priv->link_irq,
> +				    "failed to get link irq\n");
> +		goto err_wq;
> +	}
> +

[ ... ]

> @@ -839,6 +931,9 @@ static int w5100_suspend(struct device *dev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> +		if (priv->link_irq >= 0)
> +			disable_irq(priv->link_irq);
> +
>  		netif_carrier_off(ndev);
>  		netif_device_detach(ndev);
>  
> @@ -857,6 +952,12 @@ static int w5100_resume(struct device *dev)
>  		w5100_hw_start(priv);
>  
>  		netif_device_attach(ndev);
> +
> +		if (priv->link_irq >= 0)
> +			enable_irq(priv->link_irq);
> +
> +		if (w5100_get_link(ndev))
> +			netif_carrier_on(ndev);
>  	}
>  	return 0;
>  }

Since suspend unconditionally clears the carrier and resume only sets it
when this single read reports link, is there any path that re-asserts the
carrier later when there is no link interrupt or when the PHYCFGR read
fails here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net

Re: [PATCH net-next v7 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Paolo Abeni]

On 9/9/26 2:39 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 2 · Low: 1
> 
> - [High] On w5500 the carrier state is now sampled exactly once per ifup
>   (and once per resume) from PHYCFGR, with no polling fallback and no…

I read this one as 'with a bad config/devtree the eth could non functional'
which does look a big deal to me...

> - [Medium] `w5500_get_phycfgr_lnk()` returns a tri-state value (<0 on
>   SPI/register read error, 0 link down, 1 link up), and its consumers…
> - [Medium] The carrier state is computed and published by two
>   unsynchronized contexts.

... but the above 2 looks relevant to me. Note that ordering the irq
enabling and initial link detection is not enough, you need explicit
synchronization.

Finally please note commit c82ff94592fb68f529afe63ca7f5ddb7dae4ba83, you
should explicitly handle sashiko's comments.

/P

Re: [PATCH net-next v7 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Arthur Crepin Leblond]

On Thu, Sep 10, 2026 at 01:27:42PM +0200, Paolo Abeni wrote:
>On 9/9/26 2:39 PM, netdev-bot+sashiko@kernel.org wrote:

Hi Paolo,

>> Thank you for your contribution! Sashiko AI review found 4 potential
>> issue(s) to consider:
>>
>> Critical: 0 · High: 1 · Medium: 2 · Low: 1
>>
>> - [High] On w5500 the carrier state is now sampled exactly once per ifup
>>   (and once per resume) from PHYCFGR, with no polling fallback and no…
>
>I read this one as 'with a bad config/devtree the eth could non functional'
>which does look a big deal to me...

Yeah I would also say so, wtihout an additional interrupt configured
you cannot really get notified of the carrier state changes. Only
.get_link is useful (and only for w5500).

But where Sashiko might be right is that the previous implementation
was not calling netif_carrier_on/off on w5100_open and did not have
.get_link set. So maybe .get_link should only be set on w5500 and
netif_carrier_on/off called only if the link irq is set.

>
>> - [Medium] `w5500_get_phycfgr_lnk()` returns a tri-state value (<0 on
>>   SPI/register read error, 0 link down, 1 link up), and its consumers…
>> - [Medium] The carrier state is computed and published by two
>>   unsynchronized contexts.
>
>... but the above 2 looks relevant to me. Note that ordering the irq
>enabling and initial link detection is not enough, you need explicit
>synchronization.

I'll try to use rtnl_lock.

>
>Finally please note commit c82ff94592fb68f529afe63ca7f5ddb7dae4ba83, you
>should explicitly handle sashiko's comments.
>
>/P
>

Thank you for pointing this out, I'll address the AI reviews.

Arthur