Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer
From: Caleb James DeLisle
Date: Sun Mar 23 2025 - 19:53:36 EST
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@xxxxxxxx>
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@xxxxxxxx>
+
+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