Re: [PATCH 06/12] dt-bindings: watchdog: qcom-wdt: merge MSM timer

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 13/12/2022 16:40, Rob Herring wrote:
> On Mon, Dec 12, 2022 at 05:35:26PM +0100, Krzysztof Kozlowski wrote:
>> Merge Qualcomm MSM timer bindings into watchdog, because the timer
>> compatibles are already included here and the hardware is quite similar.
>>
>> While converting the MSM timer bindings, adjust clock-frequency
>> property to take only one frequency, instead of two, because:
>> 1. DT schema does not allow to frequencies,
>> 2. The Linux timer driver reads only first frequency.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  .../bindings/timer/qcom,msm-timer.txt         | 47 ------------------
>>  .../bindings/watchdog/qcom-wdt.yaml           | 49 +++++++++++++++++++
>>  2 files changed, 49 insertions(+), 47 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/timer/qcom,msm-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/timer/qcom,msm-timer.txt b/Documentation/devicetree/bindings/timer/qcom,msm-timer.txt
>> deleted file mode 100644
>> index 5e10c345548f..000000000000
>> --- a/Documentation/devicetree/bindings/timer/qcom,msm-timer.txt
>> +++ /dev/null
>> @@ -1,47 +0,0 @@
>> -* MSM Timer
>> -
>> -Properties:
>> -
>> -- compatible : Should at least contain "qcom,msm-timer". More specific
>> -               properties specify which subsystem the timers are paired with.
>> -
>> -               "qcom,kpss-timer" - krait subsystem
>> -               "qcom,scss-timer" - scorpion subsystem
>> -
>> -- interrupts : Interrupts for the debug timer, the first general purpose
>> -               timer, and optionally a second general purpose timer, and
>> -               optionally as well, 2 watchdog interrupts, in that order.
>> -
>> -- reg : Specifies the base address of the timer registers.
>> -
>> -- clocks: Reference to the parent clocks, one per output clock. The parents
>> -          must appear in the same order as the clock names.
>> -
>> -- clock-names: The name of the clocks as free-form strings. They should be in
>> -               the same order as the clocks.
>> -
>> -- clock-frequency : The frequency of the debug timer and the general purpose
>> -                    timer(s) in Hz in that order.
>> -
>> -Optional:
>> -
>> -- cpu-offset : per-cpu offset used when the timer is accessed without the
>> -               CPU remapping facilities. The offset is
>> -               cpu-offset + (0x10000 * cpu-nr).
>> -
>> -Example:
>> -
>> -       timer@200a000 {
>> -               compatible = "qcom,scss-timer", "qcom,msm-timer";
>> -               interrupts = <1 1 0x301>,
>> -                            <1 2 0x301>,
>> -                            <1 3 0x301>,
>> -                            <1 4 0x301>,
>> -                            <1 5 0x301>;
>> -               reg = <0x0200a000 0x100>;
>> -               clock-frequency = <19200000>,
>> -                                 <32768>;
>> -               clocks = <&sleep_clk>;
>> -               clock-names = "sleep";
>> -               cpu-offset = <0x40000>;
>> -       };
>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>> index b7fc57f4800e..697caf1937cc 100644
>> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>> @@ -10,6 +10,9 @@ maintainers:
>>    - Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>  
>>  properties:
>> +  $nodename:
>> +    pattern: "^(watchdog|timer)@[0-9a-f]+$"
>> +
>>    compatible:
>>      oneOf:
>>        - items:
>> @@ -48,6 +51,20 @@ properties:
>>    clocks:
>>      maxItems: 1
>>  
>> +  clock-names:
>> +    items:
>> +      - const: sleep
>> +
>> +  clock-frequency:
>> +    description:
>> +      The frequency of the general purpose timer in Hz in that order.
> 
> 'in that order'?

Left over, as I initial binding had here two frequencies. I'll remove it.

> 
>> +
>> +  cpu-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Per-CPU offset used when the timer is accessed without the CPU remapping
>> +      facilities. The offset is cpu-offset + (0x10000 * cpu-nr).
>> +
>>    interrupts:
>>      minItems: 1
>>      maxItems: 5
>> @@ -67,12 +84,27 @@ allOf:
>>              const: qcom,kpss-wdt
>>      then:
>>        properties:
>> +        clock-frequency: false
>> +        cpu-offset: false
>>          interrupts:
>>            minItems: 1
>>            items:
>>              - description: Bark
>>              - description: Bite
>>  
>> +    else:
>> +      properties:
>> +        interrupts:
>> +          minItems: 3
>> +          items:
>> +            - description: Debug
>> +            - description: First general purpose timer
>> +            - description: Second general purpose timer
>> +            - description: First watchdog
>> +            - description: Second watchdog
> 
> Okay, here they are. No need to set it to 2 then change to 5 in the 
> series.

My previous patch where you noticed the difference sets it to maxItems:5
in top-level and only specific variant has two described interrupts.

So if I understand your comment correctly, it is okay and I will just
fix the clock-frequency description ("in that order").

Best regards,
Krzysztof