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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Tuesday, May 9, 2023 12:23 PM
> To: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> linux@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> linux-watchdog@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>
> Subject: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 08/05/2023 15:15, Bharat Bhushan wrote:
> > Add binding documentation for the Marvell GTI system watchdog driver.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>
> > ---
> > v7:
> >  - Corrected compatible to have soc name
> >  - Converted marvell,wdt-timer-index to optional
> >
> >  .../watchdog/marvell,octeontx2-wdt.yaml       | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > new file mode 100644
> > index 000000000000..72951b10f1f3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt
> > +++ .yaml
> > @@ -0,0 +1,80 @@
> > +# 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-2Cocteontx2-2Dwdt.yaml-
> 23&d=DwICaQ&c=nKjWec2b6
> >
> +R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m
> =4PmICMz
> >
> +jaooxZnBaJszNihWBPjS99OeVsnJTSsOxNyAhC83AsKQuNVano9_YP2Oj&s=s1E00
> h2dU
> > +roSz4xdrUQMO3_19ren2IoeshKFZjDWKrw&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=4PmICMzjaooxZnBaJszNihW
> BPjS99O
> >
> +eVsnJTSsOxNyAhC83AsKQuNVano9_YP2Oj&s=MFuqeS8S00OEgao3hV2RkzNGX
> 2p0Jycp
> > +nZo_sLygcbk&e=
> > +
> > +title: Marvell Global Timer (GTI) system watchdog
> > +
> > +allOf:
> > +  - $ref: watchdog.yaml#
> 
> Put allOf after maintainers:.

Ok

> 
> > +
> > +maintainers:
> > +  - Bharat Bhushan <bbhushan2@xxxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: marvell,octeontx2-wdt
> 
> Why is this alone? Judging by the enum below, octeontx2 is not specific.
> 
> > +      - items:
> > +          - enum:
> > +              - marvell,octeontx2-95xx-wdt
> > +              - marvell,octeontx2-96xx-wdt
> > +              - marvell,octeontx2-98xx-wdt
> 
> We don't allow wildcards in general

Marvell have octeontx2 series of processor which have watchdog timer.
In 95xx,98xx,96xx are the processors in octeontx2 series of processor. So octeontx2-95xx is on soc, octeontx2-96xx is another and so on.

> 
> > +          - const: marvell,octeontx2-wdt
> > +      - const: marvell,cn10k-wdt
> 
> Same question - why is this alone?
Same here, Marvell have cn10k series of processors and cn10kx and cnf10kx are the processor in this series.

One of the difference between octeontx2 and cn10k series processor is number of timers available. Which within the available set of timers one of the timer is programmed to be watchdog timer. 

Can you please propose how you want these compatible to be defined?

> 
> Second question - it should be rather part of enum with the first one if accepted.
> 
> > +      - items:
> > +          - enum:
> > +              - marvell,cn10kx-wdt
> > +              - marvell,cnf10kx-wdt
> > +          - const: marvell,cn10k-wdt
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> 
> maxItems instead. You see it is different than above properties?

Okay,

> 
> > +
> > +  clock-names:
> > +    minItems: 1
> 
> Need to define names.

Okay,

Thanks
-Bharat

> 
> Best regards,
> Krzysztof