| Message ID | 20250321134633.2155141-5-cjd@cjdns.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add EcoNet EN751221 MIPS platform support | expand |
On 21/03/2025 14:46, Caleb James DeLisle wrote: > Add device tree binding documentation for the high-precision timer (HPT) > in the EcoNet EN751221 SoC. > > Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> Previous patch was not tested, so was this one tested? > --- > .../bindings/timer/econet,timer-hpt.yaml | 58 +++++++++++++++++++ > 1 file changed, 58 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml > > diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml > new file mode 100644 > index 000000000000..8b7ff9bce947 > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml > @@ -0,0 +1,58 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: EcoNet High Precision Timer (HPT) > + > +maintainers: > + - Calev James DeLisle <cjd@cjdns.fr> > + > +description: | Do not need '|' unless you need to preserve formatting. > + The EcoNet High Precision Timer (HPT) is a timer peripheral found in various > + EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE > + count/compare registers and a per-CPU control register, with a single interrupt > + line using a percpu-devid interrupt mechanism. > + > +properties: > + compatible: > + const: econet,timer-hpt Soc components must have soc-based compatible and then filename matching whatever you use as fallback. > + > + reg: > + minItems: 1 > + maxItems: 2 No, list items instead. > + description: | > + Physical base address and size of the timer's register space. On 34Kc > + processors, a single region is used. On 1004Kc processors, two regions are > + used, one for each core. So different hardware, different compatible. That's why you need soc-based compatibles. Follow standard SoC upstreaming rules and examples. > + > + interrupts: > + maxItems: 1 > + description: | Do not need '|' unless you need to preserve formatting. > + The interrupt number for the timer. Drop, redundant. > This is a percpu-devid interrupt shared > + across CPUs. > + > + clocks: > + maxItems: 1 > + description: | > + A clock to get the frequency of the timer. Drop description, redundant > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +additionalProperties: false > + > +examples: > + - | > + timer_hpt@1fbf0400 { No underscores Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Look how other SoCs are calling this. > + compatible = "econet,timer-hpt"; > + reg = <0x1fbf0400 0x100>; > + interrupt-parent = <&intc>; > + interrupts = <30>; > + clocks = <&hpt_clock>; > + }; > +... Best regards, Krzysztof
Thank you for the review. On 21/03/2025 21:56, Krzysztof Kozlowski wrote: > On 21/03/2025 14:46, Caleb James DeLisle wrote: >> Add device tree binding documentation for the high-precision timer (HPT) >> in the EcoNet EN751221 SoC. >> >> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> > Previous patch was not tested, so was this one tested? Yes, all of this has been tested on multiple devices, I believe I was unclear in the question I added in patch 3. > >> --- >> .../bindings/timer/econet,timer-hpt.yaml | 58 +++++++++++++++++++ >> 1 file changed, 58 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >> >> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >> new file mode 100644 >> index 000000000000..8b7ff9bce947 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >> @@ -0,0 +1,58 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: EcoNet High Precision Timer (HPT) >> + >> +maintainers: >> + - Calev James DeLisle <cjd@cjdns.fr> >> + >> +description: | > Do not need '|' unless you need to preserve formatting. Ok > >> + The EcoNet High Precision Timer (HPT) is a timer peripheral found in various >> + EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE >> + count/compare registers and a per-CPU control register, with a single interrupt >> + line using a percpu-devid interrupt mechanism. >> + >> +properties: >> + compatible: >> + const: econet,timer-hpt > Soc components must have soc-based compatible and then filename matching > whatever you use as fallback. I have so far been unable to find good documentation on writing DT bindings specifically for SoC devices. If you have anything to point me to, I will read it. If not, even a good example of someone else doing it right is helpful. Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence of any other advice, I can try to do what they do. > >> + >> + reg: >> + minItems: 1 >> + maxItems: 2 > No, list items instead. I see qcom,pdc.yaml using items: with per-item description so can follow that. > >> + description: | >> + Physical base address and size of the timer's register space. On 34Kc >> + processors, a single region is used. On 1004Kc processors, two regions are >> + used, one for each core. > So different hardware, different compatible. That's why you need > soc-based compatibles. Follow standard SoC upstreaming rules and examples. I presume this should ideally be with If: statements to further validate the DT (?) > >> + >> + interrupts: >> + maxItems: 1 >> + description: | > Do not need '|' unless you need to preserve formatting. Ok > >> + The interrupt number for the timer. > Drop, redundant. Ok > > >> This is a percpu-devid interrupt shared >> + across CPUs. >> + >> + clocks: >> + maxItems: 1 >> + description: | >> + A clock to get the frequency of the timer. > Drop description, redundant Ok > >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + timer_hpt@1fbf0400 { > No underscores I knew that, my mistake. > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Thank you, this is useful. > > Look how other SoCs are calling this. As said, any documentation link or example of someone who does this right is much appreciated. In any case, thank you very much for your time and I will address these points in v2. Thanks, Caleb > >> + compatible = "econet,timer-hpt"; >> + reg = <0x1fbf0400 0x100>; >> + interrupt-parent = <&intc>; >> + interrupts = <30>; >> + clocks = <&hpt_clock>; >> + }; >> +... > > Best regards, > Krzysztof
On 22/03/2025 00:21, Caleb James DeLisle wrote: > Thank you for the review. > > On 21/03/2025 21:56, Krzysztof Kozlowski wrote: >> On 21/03/2025 14:46, Caleb James DeLisle wrote: >>> Add device tree binding documentation for the high-precision timer (HPT) >>> in the EcoNet EN751221 SoC. >>> >>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> >> Previous patch was not tested, so was this one tested? > > Yes, all of this has been tested on multiple devices, I believe I was > unclear in the question I added in patch 3. Hm? How can you test a binding on a device? I meant here bindings - they were not tested. > >> >>> --- >>> .../bindings/timer/econet,timer-hpt.yaml | 58 +++++++++++++++++++ >>> 1 file changed, 58 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>> new file mode 100644 >>> index 000000000000..8b7ff9bce947 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>> @@ -0,0 +1,58 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: EcoNet High Precision Timer (HPT) >>> + >>> +maintainers: >>> + - Calev James DeLisle <cjd@cjdns.fr> >>> + >>> +description: | >> Do not need '|' unless you need to preserve formatting. > Ok >> >>> + The EcoNet High Precision Timer (HPT) is a timer peripheral found in various >>> + EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE >>> + count/compare registers and a per-CPU control register, with a single interrupt >>> + line using a percpu-devid interrupt mechanism. >>> + >>> +properties: >>> + compatible: >>> + const: econet,timer-hpt >> Soc components must have soc-based compatible and then filename matching >> whatever you use as fallback. > > I have so far been unable to find good documentation on writing DT bindings > specifically for SoC devices. If you have anything to point me to, I will read it. > If not, even a good example of someone else doing it right is helpful. > > Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence > of any other advice, I can try to do what they do. Just don't use generic fallback. > >> >>> + >>> + reg: >>> + minItems: 1 >>> + maxItems: 2 >> No, list items instead. > I see qcom,pdc.yaml using items: with per-item description so can follow that. >> >>> + description: | >>> + Physical base address and size of the timer's register space. On 34Kc >>> + processors, a single region is used. On 1004Kc processors, two regions are >>> + used, one for each core. >> So different hardware, different compatible. That's why you need >> soc-based compatibles. Follow standard SoC upstreaming rules and examples. > I presume this should ideally be with If: statements to further validate the DT (?) Yes >> >>> + >>> + interrupts: >>> + maxItems: 1 >>> + description: | >> Do not need '|' unless you need to preserve formatting. > Ok >> >>> + The interrupt number for the timer. >> Drop, redundant. > Ok >> >> >>> This is a percpu-devid interrupt shared >>> + across CPUs. >>> + >>> + clocks: >>> + maxItems: 1 >>> + description: | >>> + A clock to get the frequency of the timer. >> Drop description, redundant > Ok >> >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clocks >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + timer_hpt@1fbf0400 { >> No underscores > I knew that, my mistake. >> >> Node names should be generic. See also an explanation and list of >> examples (not exhaustive) in DT specification: >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > Thank you, this is useful. >> >> Look how other SoCs are calling this. > As said, any documentation link or example of someone who does this right > is much appreciated. In any case, thank you very much for your time and I > will address these points in v2. I gave one link above. Other could be one of my talks... or maybe what elinux.org has, but I did not verify it. Best regards, Krzysztof
On 23/03/2025 13:39, Krzysztof Kozlowski wrote: > On 22/03/2025 00:21, Caleb James DeLisle wrote: >> Thank you for the review. >> >> On 21/03/2025 21:56, Krzysztof Kozlowski wrote: >>> On 21/03/2025 14:46, Caleb James DeLisle wrote: >>>> Add device tree binding documentation for the high-precision timer (HPT) >>>> in the EcoNet EN751221 SoC. >>>> >>>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> >>> Previous patch was not tested, so was this one tested? >> Yes, all of this has been tested on multiple devices, I believe I was >> unclear in the question I added in patch 3. > Hm? How can you test a binding on a device? I meant here bindings - they > were not tested. I see. For bindings I ran `make dt_binding_check` and assumed it good because it ran to completion. I now know that isn't reliable, but re-checked that it didn't log any errors (warnings?) about econet,timer-hpt.yaml > >>>> --- >>>> .../bindings/timer/econet,timer-hpt.yaml | 58 +++++++++++++++++++ >>>> 1 file changed, 58 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>>> new file mode 100644 >>>> index 000000000000..8b7ff9bce947 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml >>>> @@ -0,0 +1,58 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: EcoNet High Precision Timer (HPT) >>>> + >>>> +maintainers: >>>> + - Calev James DeLisle <cjd@cjdns.fr> >>>> + >>>> +description: | >>> Do not need '|' unless you need to preserve formatting. >> Ok >>>> + The EcoNet High Precision Timer (HPT) is a timer peripheral found in various >>>> + EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE >>>> + count/compare registers and a per-CPU control register, with a single interrupt >>>> + line using a percpu-devid interrupt mechanism. >>>> + >>>> +properties: >>>> + compatible: >>>> + const: econet,timer-hpt >>> Soc components must have soc-based compatible and then filename matching >>> whatever you use as fallback. >> I have so far been unable to find good documentation on writing DT bindings >> specifically for SoC devices. If you have anything to point me to, I will read it. >> If not, even a good example of someone else doing it right is helpful. >> >> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence >> of any other advice, I can try to do what they do. > Just don't use generic fallback. Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my homework). If I understand this correctly, you prefer that I use something specific like econet,en751221-timer as the fallback case, so for example on EN751627, it would be: compatible = "econet,en751627-timer", "econet,en751221-timer"; The reason why I didn't do this is because this timer seems to show up in a lot of places. Vendor code says that it's older than EN751221, and (if my reading is correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink as well as EcoNet. Now that I'll be adding strict checks on the number of register blocks, this way also has the advantage of allowing a case for users of the timer in SoCs we don't know about: // Only valid with 2 register blocks compatible = "econet,en751627-timer", "econet,timer-hpt"; // Only valid with 1 register block compatible = "econet,en751612-timer", "econet,timer-hpt"; // No restriction because we don't know how many timers the SoC has compatible = "econet,timer-hpt"; That said, I'm fine to do it however you want as long as I'm clear on what you're asking for and you have all of the context behind my original decision. Thanks, Caleb > >>>> + >>>> + reg: >>>> + minItems: 1 >>>> + maxItems: 2 >>> No, list items instead. >> I see qcom,pdc.yaml using items: with per-item description so can follow that. >>>> + description: | >>>> + Physical base address and size of the timer's register space. On 34Kc >>>> + processors, a single region is used. On 1004Kc processors, two regions are >>>> + used, one for each core. >>> So different hardware, different compatible. That's why you need >>> soc-based compatibles. Follow standard SoC upstreaming rules and examples. >> I presume this should ideally be with If: statements to further validate the DT (?) > Yes > >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + description: | >>> Do not need '|' unless you need to preserve formatting. >> Ok >>>> + The interrupt number for the timer. >>> Drop, redundant. >> Ok >>> >>>> This is a percpu-devid interrupt shared >>>> + across CPUs. >>>> + >>>> + clocks: >>>> + maxItems: 1 >>>> + description: | >>>> + A clock to get the frequency of the timer. >>> Drop description, redundant >> Ok >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - interrupts >>>> + - clocks >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + timer_hpt@1fbf0400 { >>> No underscores >> I knew that, my mistake. >>> Node names should be generic. See also an explanation and list of >>> examples (not exhaustive) in DT specification: >>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> Thank you, this is useful. >>> Look how other SoCs are calling this. >> As said, any documentation link or example of someone who does this right >> is much appreciated. In any case, thank you very much for your time and I >> will address these points in v2. > I gave one link above. Other could be one of my talks... or maybe what > elinux.org has, but I did not verify it. > > Best regards, > Krzysztof
On 24/03/2025 00:53, Caleb James DeLisle wrote: >>>>> + compatible: >>>>> + const: econet,timer-hpt >>>> Soc components must have soc-based compatible and then filename matching >>>> whatever you use as fallback. >>> I have so far been unable to find good documentation on writing DT bindings >>> specifically for SoC devices. If you have anything to point me to, I will read it. >>> If not, even a good example of someone else doing it right is helpful. >>> >>> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence >>> of any other advice, I can try to do what they do. >> Just don't use generic fallback. > > > Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my > homework). If I understand this correctly, you prefer that I use something specific > like econet,en751221-timer as the fallback case, so for example on EN751627, > it would be: > > compatible = "econet,en751627-timer", "econet,en751221-timer"; Yes > > The reason why I didn't do this is because this timer seems to show up in a lot of > places. Vendor code says that it's older than EN751221, and (if my reading is Just like every other SoC component for every other SoC. > correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink > as well as EcoNet. > > Now that I'll be adding strict checks on the number of register blocks, this way > also has the advantage of allowing a case for users of the timer in SoCs we don't > know about: > > // Only valid with 2 register blocks > compatible = "econet,en751627-timer", "econet,timer-hpt"; > > // Only valid with 1 register block > compatible = "econet,en751612-timer", "econet,timer-hpt"; Above do not differ... > > // No restriction because we don't know how many timers the SoC has > compatible = "econet,timer-hpt"; How can you not know? This is strictly defined on given hardware. > > > That said, I'm fine to do it however you want as long as I'm clear on what you're > asking for and you have all of the context behind my original decision. Generic compatible as fallback is accepted if you have evidence that it is the same IP, with same programming interface, across all these SoCs. Or if its version is discoverable. If you do not know about other SoC implementations it is rather a proof that above statement cannot be fulfilled - you just don't know how it is implemented. Best regards, Krzysztof
On 24/03/2025 08:13, Krzysztof Kozlowski wrote: > On 24/03/2025 00:53, Caleb James DeLisle wrote: >>>>>> + compatible: >>>>>> + const: econet,timer-hpt >>>>> Soc components must have soc-based compatible and then filename matching >>>>> whatever you use as fallback. >>>> I have so far been unable to find good documentation on writing DT bindings >>>> specifically for SoC devices. If you have anything to point me to, I will read it. >>>> If not, even a good example of someone else doing it right is helpful. >>>> >>>> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence >>>> of any other advice, I can try to do what they do. >>> Just don't use generic fallback. >> >> Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my >> homework). If I understand this correctly, you prefer that I use something specific >> like econet,en751221-timer as the fallback case, so for example on EN751627, >> it would be: >> >> compatible = "econet,en751627-timer", "econet,en751221-timer"; > Yes > >> The reason why I didn't do this is because this timer seems to show up in a lot of >> places. Vendor code says that it's older than EN751221, and (if my reading is > Just like every other SoC component for every other SoC. > >> correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink >> as well as EcoNet. >> >> Now that I'll be adding strict checks on the number of register blocks, this way >> also has the advantage of allowing a case for users of the timer in SoCs we don't >> know about: >> >> // Only valid with 2 register blocks >> compatible = "econet,en751627-timer", "econet,timer-hpt"; >> >> // Only valid with 1 register block >> compatible = "econet,en751612-timer", "econet,timer-hpt"; > Above do not differ... > >> // No restriction because we don't know how many timers the SoC has >> compatible = "econet,timer-hpt"; > How can you not know? This is strictly defined on given hardware. > I mean I don't know, the person writing the DTS for that SoC needs to know. Per your preference, I'll do the following: // 2 blocks accepted compatible = "econet,en751627-timer", "econet,en751221-timer"; // 1 block accepted compatible = "econet,en751221-timer"; If someone has an SoC with more than 2 timers, it is not supported so they should update the binding, or (in downstream) they might write an invalid DTS. FWIW I have no evidence of any >2 core processor which uses this, so 2 timers is probably the maximum. Lastly I'll change the driver name to timer-econet-en751221.c to avoid the proliferation of different names. Thanks, Caleb
diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml new file mode 100644 index 000000000000..8b7ff9bce947 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml @@ -0,0 +1,58 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: EcoNet High Precision Timer (HPT) + +maintainers: + - Calev James DeLisle <cjd@cjdns.fr> + +description: | + The EcoNet High Precision Timer (HPT) is a timer peripheral found in various + EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE + count/compare registers and a per-CPU control register, with a single interrupt + line using a percpu-devid interrupt mechanism. + +properties: + compatible: + const: econet,timer-hpt + + reg: + minItems: 1 + maxItems: 2 + description: | + Physical base address and size of the timer's register space. On 34Kc + processors, a single region is used. On 1004Kc processors, two regions are + used, one for each core. + + interrupts: + maxItems: 1 + description: | + The interrupt number for the timer. This is a percpu-devid interrupt shared + across CPUs. + + clocks: + maxItems: 1 + description: | + A clock to get the frequency of the timer. + +required: + - compatible + - reg + - interrupts + - clocks + +additionalProperties: false + +examples: + - | + timer_hpt@1fbf0400 { + compatible = "econet,timer-hpt"; + reg = <0x1fbf0400 0x100>; + interrupt-parent = <&intc>; + interrupts = <30>; + clocks = <&hpt_clock>; + }; +...
Add device tree binding documentation for the high-precision timer (HPT) in the EcoNet EN751221 SoC. Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> --- .../bindings/timer/econet,timer-hpt.yaml | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml