| Message ID | 20221202151204.3318592-4-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: mxl-gpy: broken interrupt fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next |
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
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
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 > >
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
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
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
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
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
> (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
>> + >> + 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
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. 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