Message-ID: <f2738225-564e-479b-a4f0-fac0ba6b6d53@kernel.org>
Date: Sun, 23 Mar 2025 13:39:01 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Caleb James DeLisle <cjd@...ns.fr>, 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

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@...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.

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@...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.

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@...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.

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

Powered by blists - more mailing lists