Message-ID: <d419bcd2-fa78-4390-88b0-64ed54b87081@cjdns.fr>
Date: Mon, 24 Mar 2025 00:53:17 +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


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


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


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