[PATCH net-next v1 0/4] net: phy: mxl-gpy: broken interrupt fixes

[PATCH net-next v1 0/4] net: phy: mxl-gpy: broken interrupt fixes

[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.

net vs net-next: I'm not sure. No one seems to have noticed it so far.
My board I care about has no support for older kernel. Apart from that,
the first patch might be for net. The last one would need a new device
tree property, so it might only apply for net-next? Also it will disable
interrupts by default now.
Let me know what you think. I can send the first patch independently with a
Fixes tag and resend the last ones after the merge window. (The last one
depends on the first).

Btw. I just noticed that this series won't apply cleanly, because it
references patch context changed by
https://lore.kernel.org/netdev/20221202144900.3298204-1-michael@walle.cc/
:(

Michael Walle (4):
  net: phy: mxl-gpy: add MDINT workaround
  dt-bindings: vendor-prefixes: add MaxLinear
  dt-bindings: net: phy: add MaxLinear GPY2xx bindings
  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                     | 88 +++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml

-- 
2.30.2

[PATCH net-next v1 1/4] net: phy: mxl-gpy: add MDINT workaround

[Michael Walle]

At least the GPY215B and GPY215C has a bug where it is still driving the
interrupt line (MDINT) even after the interrupt status register is read
and its bits are cleared. This will cause an interrupt storm.

Although the MDINT is multiplexed with a GPIO pin and theoretically we
could switch the pinmux to GPIO input mode, this isn't possible because
the access to this register will stall exactly as long as the interrupt
line is asserted. We exploit this very fact and just read a random
internal register in our interrupt handler. This way, it will be delayed
until the external interrupt line is released and an interrupt storm is
avoided.

The internal register access via the mailbox was deduced by looking at
the downstream PHY API because the datasheet doesn't mention any of
this.

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

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 0ff7ef076072..20e610dda891 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/bitfield.h>
 #include <linux/hwmon.h>
+#include <linux/mutex.h>
 #include <linux/phy.h>
 #include <linux/polynomial.h>
 #include <linux/netdevice.h>
@@ -81,6 +82,14 @@
 #define VSPEC1_TEMP_STA	0x0E
 #define VSPEC1_TEMP_STA_DATA	GENMASK(9, 0)
 
+/* Mailbox */
+#define VSPEC1_MBOX_DATA	0x5
+#define VSPEC1_MBOX_ADDRLO	0x6
+#define VSPEC1_MBOX_CMD		0x7
+#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
+#define VSPEC1_MBOX_CMD_RD	(0 << 8)
+#define VSPEC1_MBOX_CMD_READY	BIT(15)
+
 /* WoL */
 #define VPSPEC2_WOL_CTL		0x0E06
 #define VPSPEC2_WOL_AD01	0x0E08
@@ -88,7 +97,15 @@
 #define VPSPEC2_WOL_AD45	0x0E0A
 #define WOL_EN			BIT(0)
 
+/* Internal registers, access via mbox */
+#define REG_GPIO0_OUT		0xd3ce00
+
 struct gpy_priv {
+	struct phy_device *phydev;
+
+	/* serialize mailbox acesses */
+	struct mutex mbox_lock;
+
 	u8 fw_major;
 	u8 fw_minor;
 };
@@ -198,6 +215,40 @@ static int gpy_hwmon_register(struct phy_device *phydev)
 }
 #endif
 
+static int gpy_mbox_read(struct phy_device *phydev, u32 addr)
+{
+	struct gpy_priv *priv = phydev->priv;
+	int val, ret;
+	u16 cmd;
+
+	mutex_lock(&priv->mbox_lock);
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_ADDRLO,
+			    addr);
+	if (ret)
+		goto out;
+
+	cmd = VSPEC1_MBOX_CMD_RD;
+	cmd |= FIELD_PREP(VSPEC1_MBOX_CMD_ADDRHI, addr >> 16);
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_CMD, cmd);
+	if (ret)
+		goto out;
+
+	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
+					VSPEC1_MBOX_CMD, val,
+					(val & VSPEC1_MBOX_CMD_READY),
+					500, 10000, false);
+	if (ret)
+		goto out;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_DATA);
+
+out:
+	mutex_unlock(&priv->mbox_lock);
+	return ret;
+}
+
 static int gpy_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -212,6 +263,13 @@ static int gpy_config_init(struct phy_device *phydev)
 	return ret < 0 ? ret : 0;
 }
 
+static bool gpy_has_broken_mdint(struct phy_device *phydev)
+{
+	/* At least these PHYs are known to have broken interrupt handling */
+	return phydev->drv->phy_id == PHY_ID_GPY215B ||
+	       phydev->drv->phy_id == PHY_ID_GPY215C;
+}
+
 static int gpy_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -228,7 +286,9 @@ static int gpy_probe(struct phy_device *phydev)
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	priv->phydev = phydev;
 	phydev->priv = priv;
+	mutex_init(&priv->mbox_lock);
 
 	fw_version = phy_read(phydev, PHY_FWV);
 	if (fw_version < 0)
@@ -574,6 +634,29 @@ static irqreturn_t gpy_handle_interrupt(struct phy_device *phydev)
 	if (!(reg & PHY_IMASK_MASK))
 		return IRQ_NONE;
 
+	/* The PHY might leave the interrupt line asserted even after PHY_ISTAT
+	 * is read. To avoid interrupt storms, delay the interrupt handling as
+	 * long as the PHY drives the interrupt line. An internal bus read will
+	 * stall as long as the interrupt line is asserted, thus just read a
+	 * random register here.
+	 * Because we cannot access the internal bus at all while the interrupt
+	 * is driven by the PHY, there is no way to make the interrupt line
+	 * unstuck (e.g. by changing the pinmux to GPIO input) during that time
+	 * frame. Therefore, polling is the best we can do and won't do any more
+	 * harm.
+	 * It was observed that this bug happens on link state and link speed
+	 * changes on a GPY215B and GYP215C independent of the firmware version
+	 * (which doesn't mean that this list is exhaustive).
+	 */
+	if (gpy_has_broken_mdint(phydev) &&
+	    (reg & (PHY_IMASK_LSTC | PHY_IMASK_LSPC))) {
+		reg = gpy_mbox_read(phydev, REG_GPIO0_OUT);
+		if (reg < 0) {
+			phy_error(phydev);
+			return IRQ_NONE;
+		}
+	}
+
 	phy_trigger_machine(phydev);
 
 	return IRQ_HANDLED;
-- 
2.30.2

[PATCH net-next v1 2/4] dt-bindings: vendor-prefixes: add MaxLinear

[Michael Walle]

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

Signed-off-by: Michael Walle <michael@walle.cc>
---
 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 10c178d97b02..ae13a8776364 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -773,6 +773,8 @@ patternProperties:
     description: MaxBotix Inc.
   "^maxim,.*":
     description: Maxim Integrated Products
+  "^maxlinear,.*":
+    description: MaxLinear Inc.
   "^mbvl,.*":
     description: Mobiveil Inc.
   "^mcube,.*":
-- 
2.30.2

[PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Add the device tree bindings for the MaxLinear GPY2xx PHYs.

Signed-off-by: Michael Walle <michael@walle.cc>
---

Is the filename ok? I was unsure because that flag is only for the GPY215
for now. But it might also apply to others. Also there is no compatible
string, so..

 .../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

[PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[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 20e610dda891..edb8cd8313b0 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 */
@@ -290,6 +291,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->irq = PHY_POLL;
+
 	fw_version = phy_read(phydev, PHY_FWV);
 	if (fw_version < 0)
 		return fw_version;
-- 
2.30.2

Re: [PATCH net-next v1 2/4] dt-bindings: vendor-prefixes: add MaxLinear

[Krzysztof Kozlowski]

On 02/12/2022 16:12, Michael Walle wrote:
> 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>

Best regards,
Krzysztof

Re: [PATCH net-next v1 1/4] net: phy: mxl-gpy: add MDINT workaround

[Andrew Lunn]

On Fri, Dec 02, 2022 at 04:12:01PM +0100, Michael Walle wrote:
> At least the GPY215B and GPY215C has a bug where it is still driving the
> interrupt line (MDINT) even after the interrupt status register is read
> and its bits are cleared. This will cause an interrupt storm.
> 
> Although the MDINT is multiplexed with a GPIO pin and theoretically we
> could switch the pinmux to GPIO input mode, this isn't possible because
> the access to this register will stall exactly as long as the interrupt
> line is asserted. We exploit this very fact and just read a random
> internal register in our interrupt handler. This way, it will be delayed
> until the external interrupt line is released and an interrupt storm is
> avoided.
> 
> The internal register access via the mailbox was deduced by looking at
> the downstream PHY API because the datasheet doesn't mention any of
> this.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/phy/mxl-gpy.c | 83 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index 0ff7ef076072..20e610dda891 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/bitfield.h>
>  #include <linux/hwmon.h>
> +#include <linux/mutex.h>
>  #include <linux/phy.h>
>  #include <linux/polynomial.h>
>  #include <linux/netdevice.h>
> @@ -81,6 +82,14 @@
>  #define VSPEC1_TEMP_STA	0x0E
>  #define VSPEC1_TEMP_STA_DATA	GENMASK(9, 0)
>  
> +/* Mailbox */
> +#define VSPEC1_MBOX_DATA	0x5
> +#define VSPEC1_MBOX_ADDRLO	0x6
> +#define VSPEC1_MBOX_CMD		0x7
> +#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
> +#define VSPEC1_MBOX_CMD_RD	(0 << 8)
> +#define VSPEC1_MBOX_CMD_READY	BIT(15)
> +
>  /* WoL */
>  #define VPSPEC2_WOL_CTL		0x0E06
>  #define VPSPEC2_WOL_AD01	0x0E08
> @@ -88,7 +97,15 @@
>  #define VPSPEC2_WOL_AD45	0x0E0A
>  #define WOL_EN			BIT(0)
>  
> +/* Internal registers, access via mbox */
> +#define REG_GPIO0_OUT		0xd3ce00
> +
>  struct gpy_priv {
> +	struct phy_device *phydev;
> +
> +	/* serialize mailbox acesses */
> +	struct mutex mbox_lock;
> +

>  static int gpy_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> @@ -228,7 +286,9 @@ static int gpy_probe(struct phy_device *phydev)
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> +	priv->phydev = phydev;

I don't think you use this anywhere. Maybe in one of the following
patches?

	Andrew

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Andrew Lunn]

On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> 
> Is the filename ok? I was unsure because that flag is only for the GPY215
> for now. But it might also apply to others. Also there is no compatible
> string, so..
> 
>  .../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;
> +        };
> +    };

I'm wondering if we want this in the example. We probably don't want
people to use this property by accident, i.e. copy/paste without
reading the rest of the document. This will becomes a bigger problem
if more properties are added, RGMII delays etc.

So maybe just skip the example?

   Andrew

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Andrew Lunn]

On Fri, Dec 02, 2022 at 04:12:04PM +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>
> ---
>  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 20e610dda891..edb8cd8313b0 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 */
> @@ -290,6 +291,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->irq = PHY_POLL;
> +

I'm not sure of ordering here. It could be phydev->irq is set after
probe. The IRQ is requested as part of phy_connect_direct(), which is
much later.

I think a better place for this test is in gpy_config_intr(), return
-EOPNOTSUPP. phy_enable_interrupts() failing should then cause
phy_request_interrupt() to use polling.

	Andrew

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Am 2022-12-02 19:31, schrieb Andrew Lunn:
> On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
>> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> 
>> Is the filename ok? I was unsure because that flag is only for the 
>> GPY215
>> for now. But it might also apply to others. Also there is no 
>> compatible
>> string, so..
>> 
>>  .../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;
>> +        };
>> +    };
> 
> I'm wondering if we want this in the example. We probably don't want
> people to use this property by accident, i.e. copy/paste without
> reading the rest of the document. This will becomes a bigger problem
> if more properties are added, RGMII delays etc.
> 
> So maybe just skip the example?

I agree. Let's wait what the device tree maintainers say.

-michael

Re: [PATCH net-next v1 1/4] net: phy: mxl-gpy: add MDINT workaround

[Michael Walle]

Am 2022-12-02 19:23, schrieb Andrew Lunn:
> On Fri, Dec 02, 2022 at 04:12:01PM +0100, Michael Walle wrote:
>> At least the GPY215B and GPY215C has a bug where it is still driving 
>> the
>> interrupt line (MDINT) even after the interrupt status register is 
>> read
>> and its bits are cleared. This will cause an interrupt storm.
>> 
>> Although the MDINT is multiplexed with a GPIO pin and theoretically we
>> could switch the pinmux to GPIO input mode, this isn't possible 
>> because
>> the access to this register will stall exactly as long as the 
>> interrupt
>> line is asserted. We exploit this very fact and just read a random
>> internal register in our interrupt handler. This way, it will be 
>> delayed
>> until the external interrupt line is released and an interrupt storm 
>> is
>> avoided.
>> 
>> The internal register access via the mailbox was deduced by looking at
>> the downstream PHY API because the datasheet doesn't mention any of
>> this.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/net/phy/mxl-gpy.c | 83 
>> +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>> 
>> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
>> index 0ff7ef076072..20e610dda891 100644
>> --- a/drivers/net/phy/mxl-gpy.c
>> +++ b/drivers/net/phy/mxl-gpy.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/module.h>
>>  #include <linux/bitfield.h>
>>  #include <linux/hwmon.h>
>> +#include <linux/mutex.h>
>>  #include <linux/phy.h>
>>  #include <linux/polynomial.h>
>>  #include <linux/netdevice.h>
>> @@ -81,6 +82,14 @@
>>  #define VSPEC1_TEMP_STA	0x0E
>>  #define VSPEC1_TEMP_STA_DATA	GENMASK(9, 0)
>> 
>> +/* Mailbox */
>> +#define VSPEC1_MBOX_DATA	0x5
>> +#define VSPEC1_MBOX_ADDRLO	0x6
>> +#define VSPEC1_MBOX_CMD		0x7
>> +#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
>> +#define VSPEC1_MBOX_CMD_RD	(0 << 8)
>> +#define VSPEC1_MBOX_CMD_READY	BIT(15)
>> +
>>  /* WoL */
>>  #define VPSPEC2_WOL_CTL		0x0E06
>>  #define VPSPEC2_WOL_AD01	0x0E08
>> @@ -88,7 +97,15 @@
>>  #define VPSPEC2_WOL_AD45	0x0E0A
>>  #define WOL_EN			BIT(0)
>> 
>> +/* Internal registers, access via mbox */
>> +#define REG_GPIO0_OUT		0xd3ce00
>> +
>>  struct gpy_priv {
>> +	struct phy_device *phydev;
>> +
>> +	/* serialize mailbox acesses */
>> +	struct mutex mbox_lock;
>> +
> 
>>  static int gpy_probe(struct phy_device *phydev)
>>  {
>>  	struct device *dev = &phydev->mdio.dev;
>> @@ -228,7 +286,9 @@ static int gpy_probe(struct phy_device *phydev)
>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>> +	priv->phydev = phydev;
> 
> I don't think you use this anywhere. Maybe in one of the following
> patches?

Arg. Yes, it's an leftover from when I was using a workqueue to
reenable the interrupts again.

Any opinion whether this patch should be net or net-next?

-michael

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Michael Walle]

Am 2022-12-02 19:42, schrieb Andrew Lunn:
> On Fri, Dec 02, 2022 at 04:12:04PM +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>
>> ---
>>  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 20e610dda891..edb8cd8313b0 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 */
>> @@ -290,6 +291,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->irq = PHY_POLL;
>> +
> 
> I'm not sure of ordering here. It could be phydev->irq is set after
> probe. The IRQ is requested as part of phy_connect_direct(), which is
> much later.

I've did it that way, because phy_probe() also sets phydev->irq = 
PHY_POLL
in some cases and the phy driver .probe() is called right after it.

> I think a better place for this test is in gpy_config_intr(), return
> -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
> phy_request_interrupt() to use polling.

Which will then print a warning, which might be misleading.
Or we disable the warning if -EOPNOTSUPP is returned?

-michael

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Andrew Lunn]

> > > @@ -290,6 +291,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->irq = PHY_POLL;
> > > +
> > 
> > I'm not sure of ordering here. It could be phydev->irq is set after
> > probe. The IRQ is requested as part of phy_connect_direct(), which is
> > much later.
> 
> I've did it that way, because phy_probe() also sets phydev->irq = PHY_POLL
> in some cases and the phy driver .probe() is called right after it.

Yes, it is a valid point to do this check, but on its own i don't
think it is sufficient.

> > I think a better place for this test is in gpy_config_intr(), return
> > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
> > phy_request_interrupt() to use polling.
> 
> Which will then print a warning, which might be misleading.
> Or we disable the warning if -EOPNOTSUPP is returned?

Disabling the warning is the right thing to do.

	  Andrew

Re: [PATCH net-next v1 0/4] net: phy: mxl-gpy: broken interrupt fixes

[Andrew Lunn]

On Fri, Dec 02, 2022 at 04:12:00PM +0100, Michael Walle 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.
> 
> net vs net-next: I'm not sure. No one seems to have noticed it so far.

I guess you could fail to notice 2ms of interrupt storm, if it only
happens on link down. But we should probably fix it. So please post
the first patch to net with a Fixes: tag. The rest to net-next.

    Andrew

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Rob Herring]

On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> 
> Is the filename ok? I was unsure because that flag is only for the GPY215
> for now. But it might also apply to others. Also there is no compatible
> string, so..
> 
>  .../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.

Just omit the interrupt property if you don't want interrupts and add it 
if you do.

> +
> +      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;

This is never actually checked by be schema because there is nothing to 
match on. If you want custom properties, then you need a compatible. 

> +        };
> +    };
> +
> +...
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Am 2022-12-05 22:29, schrieb Rob Herring:
> On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
>> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> 
>> Is the filename ok? I was unsure because that flag is only for the 
>> GPY215
>> for now. But it might also apply to others. Also there is no 
>> compatible
>> string, so..
>> 
>>  .../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.
> 
> Just omit the interrupt property if you don't want interrupts and add 
> it
> if you do.

How does that work together with "the device tree describes
the hardware and not the configuration". The interrupt line
is there, its just broken sometimes and thus it's disabled
by default for these PHY revisions/firmwares. With this
flag the user can say, "hey on this hardware it is not
relevant because we don't have shared interrupts or because
I know what I'm doing".

>> +
>> +      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;
> 
> This is never actually checked by be schema because there is nothing to
> match on. If you want custom properties, then you need a compatible.

This seems to be a problem for any phy bindings then.

-michael

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Am 2022-12-05 22:53, schrieb Michael Walle:
> Am 2022-12-05 22:29, schrieb Rob Herring:
>> On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
>>> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
>>> 
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> 
>>> Is the filename ok? I was unsure because that flag is only for the 
>>> GPY215
>>> for now. But it might also apply to others. Also there is no 
>>> compatible
>>> string, so..
>>> 
>>>  .../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.
>> 
>> Just omit the interrupt property if you don't want interrupts and add 
>> it
>> if you do.
> 
> How does that work together with "the device tree describes
> the hardware and not the configuration". The interrupt line
> is there, its just broken sometimes and thus it's disabled
> by default for these PHY revisions/firmwares. With this
> flag the user can say, "hey on this hardware it is not
> relevant because we don't have shared interrupts or because
> I know what I'm doing".

Specifically you can't do the following: Have the same device
tree and still being able to use it with a future PHY firmware
update/revision. Because according to your suggestion, this
won't have the interrupt property set. With this flag you can
have the following cases:
  (1) the interrupt information is there and can be used in the
      future by non-broken PHY revisions,
  (2) broken PHYs will ignore the interrupt line
  (3) except the system designer opts-in with this flag (because
      maybe this is the only PHY on the interrupt line etc).

-michael

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Krzysztof Kozlowski]

On 06/12/2022 09:29, Michael Walle wrote:
> Am 2022-12-05 22:53, schrieb Michael Walle:
>> Am 2022-12-05 22:29, schrieb Rob Herring:
>>> On Fri, Dec 02, 2022 at 04:12:03PM +0100, Michael Walle wrote:
>>>> Add the device tree bindings for the MaxLinear GPY2xx PHYs.
>>>>
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>> ---
>>>>
>>>> Is the filename ok? I was unsure because that flag is only for the 
>>>> GPY215
>>>> for now. But it might also apply to others. Also there is no 
>>>> compatible
>>>> string, so..
>>>>
>>>>  .../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.
>>>
>>> Just omit the interrupt property if you don't want interrupts and add 
>>> it
>>> if you do.
>>
>> How does that work together with "the device tree describes
>> the hardware and not the configuration". The interrupt line
>> is there, its just broken sometimes and thus it's disabled
>> by default for these PHY revisions/firmwares. With this
>> flag the user can say, "hey on this hardware it is not
>> relevant because we don't have shared interrupts or because
>> I know what I'm doing".

Yeah, that's a good question. In your case broken interrupts could be
understood the same as "not connected", so property not present. When
things are broken, you do not describe them fully in DTS for the
completeness of hardware description, right?

> 
> Specifically you can't do the following: Have the same device
> tree and still being able to use it with a future PHY firmware
> update/revision. Because according to your suggestion, this
> won't have the interrupt property set. With this flag you can
> have the following cases:
>   (1) the interrupt information is there and can be used in the
>       future by non-broken PHY revisions,
>   (2) broken PHYs will ignore the interrupt line
>   (3) except the system designer opts-in with this flag (because
>       maybe this is the only PHY on the interrupt line etc).

I am not sure if I understand the case. You want to have a DTS with
interrupts and "maxlinear,use-broken-interrupts", where the latter will
be ignored by some future firmware? Isn't then the property not really
correct? Broken for one firmware on the same device, working for other
firmware on the same device?

I would assume that in such cases you (or bootloader or overlay) should
patch the DTS...



Best regards,
Krzysztof

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Am 2022-12-06 09:38, schrieb Krzysztof Kozlowski:

>>>> Just omit the interrupt property if you don't want interrupts and
>>>> add it if you do.
>>> 
>>> How does that work together with "the device tree describes
>>> the hardware and not the configuration". The interrupt line
>>> is there, its just broken sometimes and thus it's disabled
>>> by default for these PHY revisions/firmwares. With this
>>> flag the user can say, "hey on this hardware it is not
>>> relevant because we don't have shared interrupts or because
>>> I know what I'm doing".
> 
> Yeah, that's a good question. In your case broken interrupts could be
> understood the same as "not connected", so property not present. When
> things are broken, you do not describe them fully in DTS for the
> completeness of hardware description, right?

I'd agree here, but in this case it's different. First, it isn't
obvious in the first place that things are broken and boards in
the field wouldn't/couldn't get that update. I'd really expect
an erratum from MaxLinear here. And secondly, (which I
just noticed right now, sorry), is that the interrupt line
is also used for wake-on-lan, which can also be used even for
the "broken" PHYs.

To work around this, the basic idea was to just disable the
normal interrupts and fall back to polling mode, as the PHY
driver just use it for link detection and don't offer any
advanced features like PTP (for now). But still get the system
integrator a knob to opt-in to the old behavior on new device
trees.

>> Specifically you can't do the following: Have the same device
>> tree and still being able to use it with a future PHY firmware
>> update/revision. Because according to your suggestion, this
>> won't have the interrupt property set. With this flag you can
>> have the following cases:
>>   (1) the interrupt information is there and can be used in the
>>       future by non-broken PHY revisions,
>>   (2) broken PHYs will ignore the interrupt line
>>   (3) except the system designer opts-in with this flag (because
>>       maybe this is the only PHY on the interrupt line etc).
> 
> I am not sure if I understand the case. You want to have a DTS with
> interrupts and "maxlinear,use-broken-interrupts", where the latter will
> be ignored by some future firmware?

Yes, that's correct.

> Isn't then the property not really correct? Broken for one firmware
> on the same device, working for other firmware on the same device?

Arguable, but you can interpret "use broken-interrupts" as no-op
if there are no broken interrupts.

> I would assume that in such cases you (or bootloader or overlay)
> should patch the DTS...

I think this would turn the opt-in into an opt-out and we'd rely
on the bootloader to workaround the erratum. Which isn't what we
want here.

-michael

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

Am 2022-12-06 10:44, schrieb Michael Walle:
> Am 2022-12-06 09:38, schrieb Krzysztof Kozlowski:
> 
>>>>> Just omit the interrupt property if you don't want interrupts and
>>>>> add it if you do.
>>>> 
>>>> How does that work together with "the device tree describes
>>>> the hardware and not the configuration". The interrupt line
>>>> is there, its just broken sometimes and thus it's disabled
>>>> by default for these PHY revisions/firmwares. With this
>>>> flag the user can say, "hey on this hardware it is not
>>>> relevant because we don't have shared interrupts or because
>>>> I know what I'm doing".
>> 
>> Yeah, that's a good question. In your case broken interrupts could be
>> understood the same as "not connected", so property not present. When
>> things are broken, you do not describe them fully in DTS for the
>> completeness of hardware description, right?
> 
> I'd agree here, but in this case it's different. First, it isn't
> obvious in the first place that things are broken and boards in
> the field wouldn't/couldn't get that update. I'd really expect
> an erratum from MaxLinear here. And secondly, (which I
> just noticed right now, sorry), is that the interrupt line
> is also used for wake-on-lan, which can also be used even for
> the "broken" PHYs.
> 
> To work around this, the basic idea was to just disable the
> normal interrupts and fall back to polling mode, as the PHY
> driver just use it for link detection and don't offer any
> advanced features like PTP (for now). But still get the system
> integrator a knob to opt-in to the old behavior on new device
> trees.
> 
>>> Specifically you can't do the following: Have the same device
>>> tree and still being able to use it with a future PHY firmware
>>> update/revision. Because according to your suggestion, this
>>> won't have the interrupt property set. With this flag you can
>>> have the following cases:
>>>   (1) the interrupt information is there and can be used in the
>>>       future by non-broken PHY revisions,
>>>   (2) broken PHYs will ignore the interrupt line
>>>   (3) except the system designer opts-in with this flag (because
>>>       maybe this is the only PHY on the interrupt line etc).
>> 
>> I am not sure if I understand the case. You want to have a DTS with
>> interrupts and "maxlinear,use-broken-interrupts", where the latter 
>> will
>> be ignored by some future firmware?
> 
> Yes, that's correct.
> 
>> Isn't then the property not really correct? Broken for one firmware
>> on the same device, working for other firmware on the same device?
> 
> Arguable, but you can interpret "use broken-interrupts" as no-op
> if there are no broken interrupts.
> 
>> I would assume that in such cases you (or bootloader or overlay)
>> should patch the DTS...
> 
> I think this would turn the opt-in into an opt-out and we'd rely
> on the bootloader to workaround the erratum. Which isn't what we
> want here.

Just a recap what happened on IRC:
  (1) Krzysztof signalled that such a property might be ok but the
      commit message should be explain it better. For reference
      here is what I explained there:

       maybe that property has a wrong name, but ultimately, it's just
       a hint that the systems designer wants to use the interrupts
       even if they don't work as expected, because they work on that
       particular hardware.
       the interrupt line is there but it's broken, there are device
       trees out there with that property, so all we can do is to not
       use the interrupts for that PHY. but as a systems designer who
       is aware of the consequences and knowing that they don't apply
       to my board, how could i then tell the driver to use it anyway.

  (2) Krzysztof pointed out that there is still the issue raised by
      Rob, that the schemas haven't any compatible and cannot be
      validated. I think that applies to all the network PHY bindings
      in the tree right now. I don't know how to fix them.

  (3) The main problem with the broken interrupt handling of the PHY
      is that it will disturb other devices on that interrupt line.
      IOW if the interrupt line is shared the PHY should fall back
      to polling mode. I haven't found anything in the interrupt
      subsys to query if a line is shared and I guess it's also
      conceptually impossible to do such a thing, because there
      might be any driver probed at a later time which also uses
      that line.
      Rob had the idea to walk the device tree and determine if
      a particular interrupt is used by other devices, too. If
      feasable, this sounds like a good enough heuristic for our
      problem. Although there might be some edge cases, like
      DT overlays loaded at linux runtime (?!).

So this is what I'd do now: I'd skip a new device tree property
for now and determine if the interrupt line is shared (by solely
looking at the DT) and then disable the interrupt in the PHY
driver. This begs the question what we do if there is no DT,
interrupts disabled or enabled?

Andrew, what do you think?

-michael

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Michael Walle]

Am 2022-12-03 21:36, schrieb Andrew Lunn:
>> > > @@ -290,6 +291,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->irq = PHY_POLL;
>> > > +
>> >
>> > I'm not sure of ordering here. It could be phydev->irq is set after
>> > probe. The IRQ is requested as part of phy_connect_direct(), which is
>> > much later.
>> 
>> I've did it that way, because phy_probe() also sets phydev->irq = 
>> PHY_POLL
>> in some cases and the phy driver .probe() is called right after it.
> 
> Yes, it is a valid point to do this check, but on its own i don't
> think it is sufficient.

Care to elaborate a bit? E.g. what is the difference to the case
the phy would have an interrupt described but no .config_intr()
op.

>> > I think a better place for this test is in gpy_config_intr(), return
>> > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
>> > phy_request_interrupt() to use polling.
>> 
>> Which will then print a warning, which might be misleading.
>> Or we disable the warning if -EOPNOTSUPP is returned?
> 
> Disabling the warning is the right thing to do.

There is more to this. .config_intr() is also used in
phy_init_hw() and phy_drv_supports_irq(). The latter would
still return true in our case. I'm not sure that is correct.

After trying your suggestion, I'm still in favor of somehow
tell the phy core to force polling mode during probe() of the
driver. The same way it's done if there is no .config_intr().

It's not like we'd need change the mode after probe during
runtime. Also with your proposed changed the attachment print
is wrong/misleading as it still prints the original irq instead
of PHY_POLL.

-michael

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Andrew Lunn]

>  (2) Krzysztof pointed out that there is still the issue raised by
>      Rob, that the schemas haven't any compatible and cannot be
>      validated. I think that applies to all the network PHY bindings
>      in the tree right now. I don't know how to fix them.

i've been offline for a while, i sabotaged my own mail server...

You can always add an unneeded compatible, using the PHY devices ID:

      - pattern: "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$"
        description:
          If the PHY reports an incorrect ID (or none at all) then the
          compatible list may contain an entry with the correct PHY ID
          in the above form.
          The first group of digits is the 16 bit Phy Identifier 1
          register, this is the chip vendor OUI bits 3:18. The
          second group of digits is the Phy Identifier 2 register,
          this is the chip vendor OUI bits 19:24, followed by 10
          bits of a vendor specific ID.

It would be fine to do this in the example in the binding, but i would
add a comment something like:

"Compatible generally only needed to make DT lint tools work. Mostly
not needed for real DT descriptions"

Examples often get cut/paste without thinking, and we don't really
want the compatible used unless it is really needed.

This is however a bigger problem than just PHYs. It applies to any
device which can be enumerated on a bus, e.g. USB, PCI. So maybe this
limitation of the DT linting tools should be fixed once at a higher
level?

>  (3) The main problem with the broken interrupt handling of the PHY
>      is that it will disturb other devices on that interrupt line.
>      IOW if the interrupt line is shared the PHY should fall back
>      to polling mode. I haven't found anything in the interrupt
>      subsys to query if a line is shared and I guess it's also
>      conceptually impossible to do such a thing, because there
>      might be any driver probed at a later time which also uses
>      that line.
>      Rob had the idea to walk the device tree and determine if
>      a particular interrupt is used by other devices, too. If
>      feasable, this sounds like a good enough heuristic for our
>      problem. Although there might be some edge cases, like
>      DT overlays loaded at linux runtime (?!).

My humble opinion is that it is not worth the complexity for just one
PHY which should work in polling mode without problems. I think the
boolean property you propose is KISS and does what is needed.

	Andrew

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Andrew Lunn]

> > Yes, it is a valid point to do this check, but on its own i don't
> > think it is sufficient.
> 
> Care to elaborate a bit? E.g. what is the difference to the case
> the phy would have an interrupt described but no .config_intr()
> op.
> 
> > > > I think a better place for this test is in gpy_config_intr(), return
> > > > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
> > > > phy_request_interrupt() to use polling.
> > > 
> > > Which will then print a warning, which might be misleading.
> > > Or we disable the warning if -EOPNOTSUPP is returned?
> > 
> > Disabling the warning is the right thing to do.
> 
> There is more to this. .config_intr() is also used in
> phy_init_hw() and phy_drv_supports_irq(). The latter would
> still return true in our case. I'm not sure that is correct.
> 
> After trying your suggestion, I'm still in favor of somehow
> tell the phy core to force polling mode during probe() of the
> driver.

The problem is that the MAC can set the interrupt number after the PHY
probe has been called. e.g.

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L524

The interrupt needs to be set by the time the PHY is connected to the
MAC, which is often in the MAC open method, much later than the PHY
probe.

     Andrew

Re: [PATCH net-next v1 4/4] net: phy: mxl-gpy: disable interrupts on GPY215 by default

[Michael Walle]

Am 2022-12-20 14:33, schrieb Andrew Lunn:
>> > > > I think a better place for this test is in gpy_config_intr(), return
>> > > > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause
>> > > > phy_request_interrupt() to use polling.
>> > >
>> > > Which will then print a warning, which might be misleading.
>> > > Or we disable the warning if -EOPNOTSUPP is returned?
>> >
>> > Disabling the warning is the right thing to do.
>> 
>> There is more to this. .config_intr() is also used in
>> phy_init_hw() and phy_drv_supports_irq(). The latter would
>> still return true in our case. I'm not sure that is correct.
>> 
>> After trying your suggestion, I'm still in favor of somehow
>> tell the phy core to force polling mode during probe() of the
>> driver.
> 
> The problem is that the MAC can set the interrupt number after the PHY
> probe has been called. e.g.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L524
> 
> The interrupt needs to be set by the time the PHY is connected to the
> MAC, which is often in the MAC open method, much later than the PHY
> probe.

Ok, then phydev->irq should be updated within the phy_attach_direct().
Something like the following:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e865be3d7f01..c6c5830f5214 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1537,6 +1537,14 @@ 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->drv->force_polling_mode &&
+           phydev->drv->force_polling_mode(phydev))
+               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

That callback could be too specifc, I don't know. We could also have
phydev->drv->pre_attach() which then can update the phydev->irq itself.

Btw. the phy_attached_info() in the stmmac seems to be a leftover
from before the phylink conversion. phylink will print a similar info
but when the PHY is actually attached.

-michael

Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear GPY2xx bindings

[Michael Walle]

>> +
>> +      Affected PHYs (as far as known) are GPY215B and GPY215C.
>> +    type: boolean
>> +
>> +dependencies:
>> +  maxlinear,use-broken-interrupts: [ interrupts ]

Btw. I'd presume that the tools will also allow interrupts-extended, but 
that
doesn't seem to be the case. Do I need some kind of anyOf here?

>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    ethernet {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ethernet-phy@0 {
>> +            reg = <0>;
>> +            interrupts-extended = <&intc 0>;
>> +            maxlinear,use-broken-interrupts;
> 
> This is never actually checked by be schema because there is nothing to
> match on. If you want custom properties, then you need a compatible.

I can add an unwanted compatible here, or skip the example altogether. 
But
what puzzles me is that this schema pulls in the ethernet-phy.yaml. The 
latter
then has a custom select statement on the $nodename and even a comment:

# The dt-schema tools will generate a select statement first by using
# the compatible, and second by using the node name if any. In our
# case, the node name is the one we want to match on, while the
# compatible is optional.

Why doesn't that work?

-michael