Message-ID: <8f095a56-a188-45e9-945a-1d77ef175dc8@cjdns.fr>
Date: Sat, 22 Mar 2025 00:21:05 +0100
From: Caleb James DeLisle <cjd@...ns.fr>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-mips@...r.kernel.org
Cc: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, benjamin.larsson@...exis.eu
Subject: Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

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@...ns.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@...ns.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@...f0400 {
> 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

Powered by blists - more mailing lists