> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Thursday, May 4, 2023 4:38 PM
> To: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> linux@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> linux-watchdog@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
>
> On 04/05/2023 11:02, Bharat Bhushan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >> Sent: Thursday, May 4, 2023 12:25 PM
> >> To: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> >> linux@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> >> krzysztof.kozlowski+dt@xxxxxxxxxx;
> >> linux-watchdog@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxxx>
> >>> ---
> >>> 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.yam
> >>> +++ l
> >>> @@ -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@xxxxxxxxxxx>
> >>> +
> >>> +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://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.1-
> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> 2YezyK7O3Hv_t2heGnouBw&m=uy18AXnbGKMIcyPkTnWrPZoVBj8yzyjlGeNemR
> MFgMVqkT6-4JVU5hWsVbcKPzTJ&s=XkqN7nbFOrtRnOJVqrgEDA9zinZxML4-
> qSYQPElzVeg&e=
So Say currently Marvell have GTI watchdog timer in following series of SoCs
- octeon
- octeontx2
- cn10k
Compatible for octeon series is "marvell,octeon-wdt"
Compatible for octeontx2 series is "marvell,octeontx2-wdt"
Compatible for cn10k series is "marvell,cn10k-wdt"
So effectively we will have 3 compatibles, Is that correct?
>
> >>> +
> >>> + 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.
If I understood correctly, we should add different compatible for each soc and use same to get the information we tried to get using "wdt-timer-index" property, is that correct?
But each series have many socs (10s) and GTI hardware is same except number of timers they supports, so should we add that many compatibles or add a property like this?
Thanks
-Bharat
>
> Best regards,
> Krzysztof
