Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system watchdog driver

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

On 04/05/2023 11:02, Bharat Bhushan wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, May 4, 2023 12:25 PM
>> To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org;
>> linux@roeck-us.net; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
>> watchdog driver
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 03/05/2023 14:10, Bharat Bhushan wrote:
>>> Add binding documentation for the Marvell GTI system watchdog driver.
>>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
>>> ---
>>> v5:
>>>  - Added wdt-timer-index property
>>
>> I did not ask for it...
>>
>>>  - Get clock frequency from clocks/clock-name device tree property
>>
>> Where? It's not possible in current code. I don't think you tested this at all.
> 
> My bad, Missed clock related binding changes while doing last minute rework.
> Will send updated patch adding the dt-binding properties.
> 
> It is testing exactly with below node:
>                 watchdog@802000040000 {
>                         compatible = "marvell,gti-wdt";
>                         reg = <0x8020 0x40000 0x0 0x20000>;
>                         interrupts = <0 62 1>;
>                         wdt-timer-index = <63>;
>                         clocks = <&sclk>;
>                         clock-names = "ref_clk";
> 
>>
>>>
>>>  .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
>>>  1 file changed, 54 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> new file mode 100644
>>> index 000000000000..e3315653f961
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> @@ -0,0 +1,54 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id:
>>> +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_sc
>>> +hemas_watchdog_marvell-2Cgti-2Dwdt.yaml-
>> 23&d=DwICaQ&c=nKjWec2b6R0mOyP
>>>
>> +az7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=ts41IUd
>> c422tK
>>> +gF0cnYI7jTqJ1dvTm-FNq1pILCyrOuwqKu2UVnwWEVy-
>> aZAMsme&s=fVo903PvFGVqR_P
>>> +G6r91BBtzOTLz4Mixh1Tqu1GAp6E&e=
>>> +$schema:
>>> +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_me
>>> +ta-2Dschemas_core.yaml-
>> 23&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWsw
>>>
>> +Pe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=ts41IUdc422tKgF0cnYI7jTqJ1
>> dvTm
>>> +-FNq1pILCyrOuwqKu2UVnwWEVy-
>> aZAMsme&s=ebh6bxE3wbSmwrOnHmUHMM_L77f6bY6W
>>> +Ifye_sXDNzI&e=
>>> +
>>> +title: Marvell Global Timer (GTI) system watchdog
>>> +
>>> +allOf:
>>> +  - $ref: watchdog.yaml#
>>> +
>>> +maintainers:
>>> +  - Bharat Bhushan <bbhushan2@marvell.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: marvell,gti-wdt

So I guess we all thought "gti" means some soc. Now it is clear - you
miss specific compatibles. Generic blocks alone or wildcards are not
allowed.

And we have it clearly documented:

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  wdt-timer-index:
>>
>> missing vendor prefix
> 
> ack
> 
>>
>> missing type
> 
> Will add
> 
>>
>>> +    maxItems: 1
>>
>> ???
> 
> Removed
> 
>>
>>> +    description:
>>> +      This contains the timer number out of total 64 timers supported
>>> +      by GTI hardware block.
>>
>> Why do you need it? What does it represent?
>>
>> We do not keep indices of devices other than something in reg, so please justify
>> why exception must be made here.
> 
> Different platform have different number of GTI timers, for example some platform have total 64 timer and other have 32 timers.
> So which GTI timer will be used for watchdog will depend on platform. So added this property to enable this driver on platforms.

This should be deducted from compatible.

Best regards,
Krzysztof