| Message ID | 20230109123013.3094144-3-michael@walle.cc (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 90c47eb169ac5538c3376a1577cfe858c4efcf27 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: mxl-gpy: broken interrupt fixes | expand |
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
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
> 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
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
> 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
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
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
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; + }; + }; + +...
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