netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes
@ 2023-01-09 12:30 Michael Walle
  2023-01-09 12:30 ` [PATCH net-next v3 1/4] dt-bindings: vendor-prefixes: add MaxLinear Michael Walle
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Michael Walle @ 2023-01-09 12:30 UTC (permalink / raw)
  To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Andrew Lunn,
	Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Michael Walle

The GPY215 has a broken interrupt pin. This patch series tries to
workaround that and because in general that is not possible, disables the
interrupts by default and falls back to polling mode. There is an opt-in
via the devicetree.

v3:
 - move phy_device::dev_flags after the struct phy_device definition.
   also add a comment. Thanks Russell.
 - add a rationale for the new devicetree property in the commit
   message

v2:
 - new handling of how to disable the interrupts

Michael Walle (4):
  dt-bindings: vendor-prefixes: add MaxLinear
  dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  net: phy: allow a phy to opt-out of interrupt handling
  net: phy: mxl-gpy: disable interrupts on GPY215 by default

 .../bindings/net/maxlinear,gpy2xx.yaml        | 47 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 drivers/net/phy/mxl-gpy.c                     |  5 ++
 drivers/net/phy/phy_device.c                  |  7 +++
 include/linux/phy.h                           |  3 ++
 5 files changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml

-- 
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH net-next v3 1/4] dt-bindings: vendor-prefixes: add MaxLinear
  2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
@ 2023-01-09 12:30 ` Michael Walle
  2023-01-09 12:30 ` [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings Michael Walle
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Michael Walle @ 2023-01-09 12:30 UTC (permalink / raw)
  To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Andrew Lunn,
	Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Michael Walle, Krzysztof Kozlowski

MaxLinear is a manufacturer of integrated circuits.
https://www.maxlinear.com

Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 66bf0da6f60e..60ca96163937 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -777,6 +777,8 @@ patternProperties:
     description: MaxBotix Inc.
   "^maxim,.*":
     description: Maxim Integrated Products
+  "^maxlinear,.*":
+    description: MaxLinear Inc.
   "^mbvl,.*":
     description: Mobiveil Inc.
   "^mcube,.*":
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
  2023-01-09 12:30 ` [PATCH net-next v3 1/4] dt-bindings: vendor-prefixes: add MaxLinear Michael Walle
@ 2023-01-09 12:30 ` Michael Walle
  2023-01-09 13:39   ` Andrew Lunn
  2023-01-11 20:26   ` Rob Herring
  2023-01-09 12:30 ` [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling Michael Walle
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Michael Walle @ 2023-01-09 12:30 UTC (permalink / raw)
  To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Andrew Lunn,
	Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Michael Walle

Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
essentially adds just one flag: maxlinear,use-broken-interrupts.

One might argue, that if interrupts are broken, just don't use
the interrupt property in the first place. But it needs to be more
nuanced. First, this interrupt line is also used to wake up systems by
WoL, which has nothing to do with the (broken) PHY interrupt handling.

Second and more importantly, there are devicetrees which have this
property set. Thus, within the driver we have to switch off interrupt
handling by default as a workaround. But OTOH, a systems designer who
knows the hardware and knows there are no shared interrupts for example,
can use this new property as a hint to the driver that it can enable the
interrupt nonetheless.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../bindings/net/maxlinear,gpy2xx.yaml        | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml

diff --git a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
new file mode 100644
index 000000000000..d71fa9de2b64
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/maxlinear,gpy2xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MaxLinear GPY2xx PHY
+
+maintainers:
+  - Andrew Lunn <andrew@lunn.ch>
+  - Michael Walle <michael@walle.cc>
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  maxlinear,use-broken-interrupts:
+    description: |
+      Interrupts are broken on some GPY2xx PHYs in that they keep the
+      interrupt line asserted even after the interrupt status register is
+      cleared. Thus it is blocking the interrupt line which is usually bad
+      for shared lines. By default interrupts are disabled for this PHY and
+      polling mode is used. If one can live with the consequences, this
+      property can be used to enable interrupt handling.
+
+      Affected PHYs (as far as known) are GPY215B and GPY215C.
+    type: boolean
+
+dependencies:
+  maxlinear,use-broken-interrupts: [ interrupts ]
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ethernet {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+            reg = <0>;
+            interrupts-extended = <&intc 0>;
+            maxlinear,use-broken-interrupts;
+        };
+    };
+
+...
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling
  2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
  2023-01-09 12:30 ` [PATCH net-next v3 1/4] dt-bindings: vendor-prefixes: add MaxLinear Michael Walle
  2023-01-09 12:30 ` [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings Michael Walle
@ 2023-01-09 12:30 ` Michael Walle
  2023-01-09 13:40   ` Andrew Lunn
  2023-01-09 12:30 ` [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default Michael Walle
  2023-01-10 13:10 ` [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes patchwork-bot+netdevbpf
  4 siblings, 1 reply; 15+ messages in thread
From: Michael Walle @ 2023-01-09 12:30 UTC (permalink / raw)
  To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Andrew Lunn,
	Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Michael Walle

Until now, it is not possible for a PHY driver to disable interrupts
during runtime. If a driver offers the .config_intr() as well as the
.handle_interrupt() ops, it is eligible for interrupt handling.
Introduce a new flag for the dev_flags property of struct phy_device, which
can be set by PHY driver to skip interrupt setup and fall back to polling
mode.

At the moment, this is used for the MaxLinear PHY which has broken
interrupt handling and there is a need to disable interrupts in some
cases.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phy_device.c | 7 +++++++
 include/linux/phy.h          | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 716870a4499c..e4562859ac00 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1487,6 +1487,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->interrupts = PHY_INTERRUPT_DISABLED;
 
+	/* PHYs can request to use poll mode even though they have an
+	 * associated interrupt line. This could be the case if they
+	 * detect a broken interrupt handling.
+	 */
+	if (phydev->dev_flags & PHY_F_NO_IRQ)
+		phydev->irq = PHY_POLL;
+
 	/* Port is set to PORT_TP by default and the actual PHY driver will set
 	 * it to different value depending on the PHY configuration. If we have
 	 * the generic PHY driver we can't figure it out, thus set the old
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6378c997ded5..742754d72fc0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -739,6 +739,9 @@ struct phy_device {
 #endif
 };
 
+/* Generic phy_device::dev_flags */
+#define PHY_F_NO_IRQ		0x80000000
+
 static inline struct phy_device *to_phy_device(const struct device *dev)
 {
 	return container_of(to_mdio_device(dev), struct phy_device, mdio);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default
  2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
                   ` (2 preceding siblings ...)
  2023-01-09 12:30 ` [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling Michael Walle
@ 2023-01-09 12:30 ` Michael Walle
  2023-01-09 13:41   ` Andrew Lunn
  2023-01-10 13:10 ` [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes patchwork-bot+netdevbpf
  4 siblings, 1 reply; 15+ messages in thread
From: Michael Walle @ 2023-01-09 12:30 UTC (permalink / raw)
  To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Andrew Lunn,
	Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Michael Walle

The interrupts on the GPY215B and GPY215C are broken and the only viable
fix is to disable them altogether. There is still the possibilty to
opt-in via the device tree.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mxl-gpy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 147d7a5a9b35..e5972b4ef6e8 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -12,6 +12,7 @@
 #include <linux/mutex.h>
 #include <linux/phy.h>
 #include <linux/polynomial.h>
+#include <linux/property.h>
 #include <linux/netdevice.h>
 
 /* PHY ID */
@@ -292,6 +293,10 @@ static int gpy_probe(struct phy_device *phydev)
 	phydev->priv = priv;
 	mutex_init(&priv->mbox_lock);
 
+	if (gpy_has_broken_mdint(phydev) &&
+	    !device_property_present(dev, "maxlinear,use-broken-interrupts"))
+		phydev->dev_flags |= PHY_F_NO_IRQ;
+
 	fw_version = phy_read(phydev, PHY_FWV);
 	if (fw_version < 0)
 		return fw_version;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-09 12:30 ` [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings Michael Walle
@ 2023-01-09 13:39   ` Andrew Lunn
  2023-01-11 20:26   ` Rob Herring
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2023-01-09 13:39 UTC (permalink / raw)
  To: Michael Walle
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

On Mon, Jan 09, 2023 at 01:30:11PM +0100, Michael Walle wrote:
> Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
> essentially adds just one flag: maxlinear,use-broken-interrupts.
> 
> One might argue, that if interrupts are broken, just don't use
> the interrupt property in the first place. But it needs to be more
> nuanced. First, this interrupt line is also used to wake up systems by
> WoL, which has nothing to do with the (broken) PHY interrupt handling.
> 
> Second and more importantly, there are devicetrees which have this
> property set. Thus, within the driver we have to switch off interrupt
> handling by default as a workaround. But OTOH, a systems designer who
> knows the hardware and knows there are no shared interrupts for example,
> can use this new property as a hint to the driver that it can enable the
> interrupt nonetheless.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling
  2023-01-09 12:30 ` [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling Michael Walle
@ 2023-01-09 13:40   ` Andrew Lunn
  0 siblings, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2023-01-09 13:40 UTC (permalink / raw)
  To: Michael Walle
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

On Mon, Jan 09, 2023 at 01:30:12PM +0100, Michael Walle wrote:
> Until now, it is not possible for a PHY driver to disable interrupts
> during runtime. If a driver offers the .config_intr() as well as the
> .handle_interrupt() ops, it is eligible for interrupt handling.
> Introduce a new flag for the dev_flags property of struct phy_device, which
> can be set by PHY driver to skip interrupt setup and fall back to polling
> mode.
> 
> At the moment, this is used for the MaxLinear PHY which has broken
> interrupt handling and there is a need to disable interrupts in some
> cases.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default
  2023-01-09 12:30 ` [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default Michael Walle
@ 2023-01-09 13:41   ` Andrew Lunn
  0 siblings, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2023-01-09 13:41 UTC (permalink / raw)
  To: Michael Walle
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Xu Liang, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

On Mon, Jan 09, 2023 at 01:30:13PM +0100, Michael Walle wrote:
> The interrupts on the GPY215B and GPY215C are broken and the only viable
> fix is to disable them altogether. There is still the possibilty to
> opt-in via the device tree.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes
  2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
                   ` (3 preceding siblings ...)
  2023-01-09 12:30 ` [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default Michael Walle
@ 2023-01-10 13:10 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 15+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-10 13:10 UTC (permalink / raw)
  To: Michael Walle
  Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	lxu, andrew, hkallweit1, linux, netdev, devicetree, linux-kernel

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Mon,  9 Jan 2023 13:30:09 +0100 you wrote:
> The GPY215 has a broken interrupt pin. This patch series tries to
> workaround that and because in general that is not possible, disables the
> interrupts by default and falls back to polling mode. There is an opt-in
> via the devicetree.
> 
> v3:
>  - move phy_device::dev_flags after the struct phy_device definition.
>    also add a comment. Thanks Russell.
>  - add a rationale for the new devicetree property in the commit
>    message
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/4] dt-bindings: vendor-prefixes: add MaxLinear
    https://git.kernel.org/netdev/net-next/c/dd1a98a375a6
  - [net-next,v3,2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
    https://git.kernel.org/netdev/net-next/c/90c47eb169ac
  - [net-next,v3,3/4] net: phy: allow a phy to opt-out of interrupt handling
    https://git.kernel.org/netdev/net-next/c/7d885863e716
  - [net-next,v3,4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default
    https://git.kernel.org/netdev/net-next/c/97a89ed101bb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-09 12:30 ` [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings Michael Walle
  2023-01-09 13:39   ` Andrew Lunn
@ 2023-01-11 20:26   ` Rob Herring
  2023-01-11 21:24     ` Andrew Lunn
  2023-01-11 22:30     ` Michael Walle
  1 sibling, 2 replies; 15+ messages in thread
From: Rob Herring @ 2023-01-11 20:26 UTC (permalink / raw)
  To: Michael Walle
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Xu Liang, Andrew Lunn, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

On Mon, Jan 09, 2023 at 01:30:11PM +0100, Michael Walle wrote:
> Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
> essentially adds just one flag: maxlinear,use-broken-interrupts.
> 
> One might argue, that if interrupts are broken, just don't use
> the interrupt property in the first place. But it needs to be more
> nuanced. First, this interrupt line is also used to wake up systems by
> WoL, which has nothing to do with the (broken) PHY interrupt handling.

I don't understand how this is useful. If the interrupt line is asserted 
after the 1st interrupt, how is it ever deasserted later on to be 
useful. 

In any case, you could use 'wakeup-source' if that's the functionality 
you need. Then just ignore the interrupt if 'wakeup-source' is not 
present.

> Second and more importantly, there are devicetrees which have this
> property set. Thus, within the driver we have to switch off interrupt
> handling by default as a workaround. But OTOH, a systems designer who
> knows the hardware and knows there are no shared interrupts for example,
> can use this new property as a hint to the driver that it can enable the
> interrupt nonetheless.

Pretty sure I said this already, but this schema has no effect. Add an 
extra property to the example and see. No error despite your 
'unevaluatedProperties: false'. Or drop 'interrupts-extended' and no 
dependency error... 

You won't get errors as there's no defined way to decide when to apply 
this because it is based on node name or compatible unless you do a 
custom select, but I don't see what you would key off of here...

The real answer here is add a compatible. But I'm tired of pointing this 
out to the networking maintainers every damn time. Ethernet PHYs are not 
special.

Rob

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-11 20:26   ` Rob Herring
@ 2023-01-11 21:24     ` Andrew Lunn
  2023-01-11 22:30     ` Michael Walle
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2023-01-11 21:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: Michael Walle, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Xu Liang, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

> The real answer here is add a compatible. But I'm tired of pointing this 
> out to the networking maintainers every damn time. Ethernet PHYs are not 
> special.

I agree, they are the same as USB and PCI, and other enumerable
busses. The enumeration data tells you what the device is. That is how
the kernel finds the correct driver for the device, nothing special at
all for PHYs.

	Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-11 20:26   ` Rob Herring
  2023-01-11 21:24     ` Andrew Lunn
@ 2023-01-11 22:30     ` Michael Walle
  2023-01-11 22:48       ` Andrew Lunn
  2023-01-13 16:38       ` Rob Herring
  1 sibling, 2 replies; 15+ messages in thread
From: Michael Walle @ 2023-01-11 22:30 UTC (permalink / raw)
  To: Rob Herring
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Xu Liang, Andrew Lunn, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

Am 2023-01-11 21:26, schrieb Rob Herring:
> On Mon, Jan 09, 2023 at 01:30:11PM +0100, Michael Walle wrote:
>> Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
>> essentially adds just one flag: maxlinear,use-broken-interrupts.
>> 
>> One might argue, that if interrupts are broken, just don't use
>> the interrupt property in the first place. But it needs to be more
>> nuanced. First, this interrupt line is also used to wake up systems by
>> WoL, which has nothing to do with the (broken) PHY interrupt handling.
> 
> I don't understand how this is useful. If the interrupt line is 
> asserted
> after the 1st interrupt, how is it ever deasserted later on to be
> useful.

Nobody said, that the interrupt line will stay asserted. The broken
behavior is that of the PHY doesn't respond *immediately* with a
deassertion of the interrupt line after the its internal status
register is cleared. Instead there is a random delay of up to 2ms.

There is already a workaround to avoid an interrupt storm by delaying
the ISR until the line is actually cleared. *But* if this line is
a shared one. The line is blocked by these 2ms and important
interrupts (like PTP timestaming) of other devices on this line
will get delayed. Therefore, the only viabale option is to disable
the interrupts handling in the broken PHY altogether. I.e. the line
will never be asserted by the broken PHY.

> In any case, you could use 'wakeup-source' if that's the functionality
> you need. Then just ignore the interrupt if 'wakeup-source' is not
> present.

Right, that would work for the first case. But not if someone really
wants to use interrupts with the PHY, which is still a valid scenario
if it has a dedicated interrupt line.

>> Second and more importantly, there are devicetrees which have this
>> property set. Thus, within the driver we have to switch off interrupt
>> handling by default as a workaround. But OTOH, a systems designer who
>> knows the hardware and knows there are no shared interrupts for 
>> example,
>> can use this new property as a hint to the driver that it can enable 
>> the
>> interrupt nonetheless.
> 
> Pretty sure I said this already, but this schema has no effect. Add an
> extra property to the example and see. No error despite your
> 'unevaluatedProperties: false'. Or drop 'interrupts-extended' and no
> dependency error...

I know, I noticed this the first time I tested the schema. But then
I've looked at all the other PHY binding and not one has a compatible.

I presume if there is a compatible, the devicetrees also need a
compatible. So basically, "required: compatible" in the schema, right?
But that is where the PHY maintainers don't agree.

> You won't get errors as there's no defined way to decide when to apply
> this because it is based on node name or compatible unless you do a
> custom select, but I don't see what you would key off of here...

Actually, in the previous version I've asked why the custom select
of ethernet-phy.yaml doesn't get applied here, when there is a
"allOf: $ref ethernet-phy.yaml".

-michael

> The real answer here is add a compatible. But I'm tired of pointing 
> this
> out to the networking maintainers every damn time. Ethernet PHYs are 
> not
> special.
> 
> Rob

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-11 22:30     ` Michael Walle
@ 2023-01-11 22:48       ` Andrew Lunn
  2023-01-13 16:38       ` Rob Herring
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2023-01-11 22:48 UTC (permalink / raw)
  To: Michael Walle
  Cc: Rob Herring, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Xu Liang, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

> I know, I noticed this the first time I tested the schema. But then
> I've looked at all the other PHY binding and not one has a compatible.
> 
> I presume if there is a compatible, the devicetrees also need a
> compatible. So basically, "required: compatible" in the schema, right?
> But that is where the PHY maintainers don't agree.

It should not be required. The compatible is optional. The kernel is
happy without it. You can add a compatible to make the DT linter
happy, but you are only adding it to make the linter work. Hence it
needs to be optional. All real DT blobs are unlikely to have a
compatible, given that this PHY is known not to be broken in terms of
enumeration via its ID registers.

    Andrew


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-11 22:30     ` Michael Walle
  2023-01-11 22:48       ` Andrew Lunn
@ 2023-01-13 16:38       ` Rob Herring
  2023-01-13 21:48         ` Michael Walle
  1 sibling, 1 reply; 15+ messages in thread
From: Rob Herring @ 2023-01-13 16:38 UTC (permalink / raw)
  To: Michael Walle
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Xu Liang, Andrew Lunn, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

On Wed, Jan 11, 2023 at 4:30 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2023-01-11 21:26, schrieb Rob Herring:
> > On Mon, Jan 09, 2023 at 01:30:11PM +0100, Michael Walle wrote:
> >> Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
> >> essentially adds just one flag: maxlinear,use-broken-interrupts.
> >>
> >> One might argue, that if interrupts are broken, just don't use
> >> the interrupt property in the first place. But it needs to be more
> >> nuanced. First, this interrupt line is also used to wake up systems by
> >> WoL, which has nothing to do with the (broken) PHY interrupt handling.
> >
> > I don't understand how this is useful. If the interrupt line is
> > asserted
> > after the 1st interrupt, how is it ever deasserted later on to be
> > useful.
>
> Nobody said, that the interrupt line will stay asserted. The broken
> behavior is that of the PHY doesn't respond *immediately* with a
> deassertion of the interrupt line after the its internal status
> register is cleared. Instead there is a random delay of up to 2ms.

With only "keep the interrupt line asserted even after the interrupt
status register is cleared", I assume that means forever, not some
delay.

> There is already a workaround to avoid an interrupt storm by delaying
> the ISR until the line is actually cleared. *But* if this line is
> a shared one. The line is blocked by these 2ms and important
> interrupts (like PTP timestaming) of other devices on this line
> will get delayed. Therefore, the only viabale option is to disable
> the interrupts handling in the broken PHY altogether. I.e. the line
> will never be asserted by the broken PHY.

Okay, that makes more sense.

So really, this is just an 'is shared interrupt' flag. If not shared,
then there's no reason to not use the interrupt? Assuming all
interrupts are described in DT, we already have that information. It's
just hard and inefficient to get it. You have to parse all interrupts
with the same parent and check for the same cells. If we're going to
add something more explicit, we should consider something common. It's
not the first time shared interrupts have come up, and we probably
have some properties already. For something common, I'd probably make
this a flag in interrupt cells rather than a property. That would
handle cases with multiple interrupts better.

> > In any case, you could use 'wakeup-source' if that's the functionality
> > you need. Then just ignore the interrupt if 'wakeup-source' is not
> > present.
>
> Right, that would work for the first case. But not if someone really
> wants to use interrupts with the PHY, which is still a valid scenario
> if it has a dedicated interrupt line.

Right.


> >> Second and more importantly, there are devicetrees which have this
> >> property set. Thus, within the driver we have to switch off interrupt
> >> handling by default as a workaround. But OTOH, a systems designer who
> >> knows the hardware and knows there are no shared interrupts for
> >> example,
> >> can use this new property as a hint to the driver that it can enable
> >> the
> >> interrupt nonetheless.
> >
> > Pretty sure I said this already, but this schema has no effect. Add an
> > extra property to the example and see. No error despite your
> > 'unevaluatedProperties: false'. Or drop 'interrupts-extended' and no
> > dependency error...
>
> I know, I noticed this the first time I tested the schema. But then
> I've looked at all the other PHY binding and not one has a compatible.
>
> I presume if there is a compatible, the devicetrees also need a
> compatible. So basically, "required: compatible" in the schema, right?

The DTs need one, yes. The schema doesn't actually matter here. The
schema is only selected if compatible is present, so the schema
checking that is redundant.

> But that is where the PHY maintainers don't agree.

I guess Andrew agrees now for making schema checks happy. But that's
not really the reason. Usually compatibles on discoverable devices can
be constructed from the discoverable info (e.g. PCI compatible
contains VID/PID), so you can find the node and validate it matches
the device. Second, it is just good practice to provide a way to
identify exactly what each device/node is. The schema checks needing
it is just a convenient way to enforce that practice.

> > You won't get errors as there's no defined way to decide when to apply
> > this because it is based on node name or compatible unless you do a
> > custom select, but I don't see what you would key off of here...
>
> Actually, in the previous version I've asked why the custom select
> of ethernet-phy.yaml doesn't get applied here, when there is a
> "allOf: $ref ethernet-phy.yaml".

No, 'select' is ignored in anything referenced. Using it would cause
your schema to be applied to all ethernet phy nodes. That might work
here, but not if you had a required property. Doing that would
effectively create a single schema with all possible properties for
all phy nodes.

Rob

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  2023-01-13 16:38       ` Rob Herring
@ 2023-01-13 21:48         ` Michael Walle
  0 siblings, 0 replies; 15+ messages in thread
From: Michael Walle @ 2023-01-13 21:48 UTC (permalink / raw)
  To: Rob Herring
  Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Xu Liang, Andrew Lunn, Heiner Kallweit,
	Russell King, netdev, devicetree, linux-kernel

Am 2023-01-13 17:38, schrieb Rob Herring:
> On Wed, Jan 11, 2023 at 4:30 PM Michael Walle <michael@walle.cc> wrote:
>> 
>> Am 2023-01-11 21:26, schrieb Rob Herring:
>> > On Mon, Jan 09, 2023 at 01:30:11PM +0100, Michael Walle wrote:
>> >> Add the device tree bindings for the MaxLinear GPY2xx PHYs, which
>> >> essentially adds just one flag: maxlinear,use-broken-interrupts.
>> >>
>> >> One might argue, that if interrupts are broken, just don't use
>> >> the interrupt property in the first place. But it needs to be more
>> >> nuanced. First, this interrupt line is also used to wake up systems by
>> >> WoL, which has nothing to do with the (broken) PHY interrupt handling.
>> >
>> > I don't understand how this is useful. If the interrupt line is
>> > asserted
>> > after the 1st interrupt, how is it ever deasserted later on to be
>> > useful.
>> 
>> Nobody said, that the interrupt line will stay asserted. The broken
>> behavior is that of the PHY doesn't respond *immediately* with a
>> deassertion of the interrupt line after the its internal status
>> register is cleared. Instead there is a random delay of up to 2ms.
> 
> With only "keep the interrupt line asserted even after the interrupt
> status register is cleared", I assume that means forever, not some
> delay.

Fair enough. I'll send a doc patch.

>> There is already a workaround to avoid an interrupt storm by delaying
>> the ISR until the line is actually cleared. *But* if this line is
>> a shared one. The line is blocked by these 2ms and important
>> interrupts (like PTP timestaming) of other devices on this line
>> will get delayed. Therefore, the only viabale option is to disable
>> the interrupts handling in the broken PHY altogether. I.e. the line
>> will never be asserted by the broken PHY.
> 
> Okay, that makes more sense.
> 
> So really, this is just an 'is shared interrupt' flag. If not shared,
> then there's no reason to not use the interrupt?

Correct.

> Assuming all
> interrupts are described in DT, we already have that information. It's
> just hard and inefficient to get it. You have to parse all interrupts
> with the same parent and check for the same cells. If we're going to
> add something more explicit, we should consider something common. It's
> not the first time shared interrupts have come up, and we probably
> have some properties already. For something common, I'd probably make
> this a flag in interrupt cells rather than a property. That would
> handle cases with multiple interrupts better.

What kind of flag do you have in mind? Could you give an example?

-michael

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-01-13 21:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 12:30 [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes Michael Walle
2023-01-09 12:30 ` [PATCH net-next v3 1/4] dt-bindings: vendor-prefixes: add MaxLinear Michael Walle
2023-01-09 12:30 ` [PATCH net-next v3 2/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings Michael Walle
2023-01-09 13:39   ` Andrew Lunn
2023-01-11 20:26   ` Rob Herring
2023-01-11 21:24     ` Andrew Lunn
2023-01-11 22:30     ` Michael Walle
2023-01-11 22:48       ` Andrew Lunn
2023-01-13 16:38       ` Rob Herring
2023-01-13 21:48         ` Michael Walle
2023-01-09 12:30 ` [PATCH net-next v3 3/4] net: phy: allow a phy to opt-out of interrupt handling Michael Walle
2023-01-09 13:40   ` Andrew Lunn
2023-01-09 12:30 ` [PATCH net-next v3 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default Michael Walle
2023-01-09 13:41   ` Andrew Lunn
2023-01-10 13:10 ` [PATCH net-next v3 0/4] net: phy: mxl-gpy: broken interrupt fixes patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).