| Message ID | 20230503121016.6093-1-bbhushan2@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2,v5] dt-bindings: watchdog: marvell GTI system watchdog driver | expand |
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. > > .../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: http://devicetree.org/schemas/watchdog/marvell,gti-wdt.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Marvell Global Timer (GTI) system watchdog > + > +allOf: > + - $ref: watchdog.yaml# > + > +maintainers: > + - Bharat Bhushan <bbhushan2@marvell.com> > + > +properties: > + compatible: > + const: marvell,gti-wdt > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + wdt-timer-index: missing vendor prefix missing type > + maxItems: 1 ??? > + 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. > + > +required: > + - compatible > + - reg > + - interrupts > + - wdt-timer-index > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + watchdog@802000040000 { > + compatible = "marvell,gti-wdt"; > + reg = <0x00008020 0x00040000 0x00000000 0x00020000>; > + interrupts = <0 38 IRQ_TYPE_EDGE_RISING>; Use defines for flags. > + wdt-timer-index = <63>; Best regards, Krzysztof
> -----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 > > + > > + 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. > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - wdt-timer-index > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + watchdog@802000040000 { > > + compatible = "marvell,gti-wdt"; > > + reg = <0x00008020 0x00040000 0x00000000 0x00020000>; > > + interrupts = <0 38 IRQ_TYPE_EDGE_RISING>; > > Use defines for flags. Okay. Thanks -Bharat > > > + wdt-timer-index = <63>; > > > Best regards, > Krzysztof
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
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, May 4, 2023 4:38 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: 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@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.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@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://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
On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +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://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? I don't know your SoCs. I assume you should know. > >> >>>>> + >>>>> + 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? Same story every time... and was discussed many, many times on the lists. https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 You need anyway SoC specific compatibles. Once you add proper compatibles, you will see that property is not needed. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, May 5, 2023 12:08 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +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://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? > > I don't know your SoCs. I assume you should know. > > > > >> > >>>>> + > >>>>> + 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? > > Same story every time... and was discussed many, many times on the lists. > > 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=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa > qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG- > CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e= > > You need anyway SoC specific compatibles. Once you add proper compatibles, > you will see that property is not needed. Looks odd to add N number of compatible for N socs belong to one class of soc, but anyways will do. Thanks -Bharat > > > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, May 5, 2023 12:08 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +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://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? > > I don't know your SoCs. I assume you should know. > > > > >> > >>>>> + > >>>>> + 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? > > Same story every time... and was discussed many, many times on the lists. > > 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=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa > qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG- > CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e= > > You need anyway SoC specific compatibles. Once you add proper compatibles, > you will see that property is not needed. Also on a given soc, firmware can configure one of 64 timer to be used as system watchdog time then compatible will not work. Thanks -Bharat > > > Best regards, > Krzysztof
On 05/05/2023 09:17, Bharat Bhushan wrote: >> Same story every time... and was discussed many, many times on the lists. >> >> 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=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa >> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG- >> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e= >> >> You need anyway SoC specific compatibles. Once you add proper compatibles, >> you will see that property is not needed. > > Looks odd to add N number of compatible for N socs belong to one class of soc, but anyways will do. Why this is odd? How does it differ from other SoCs? Best regards, Krzysztof
On 05/05/2023 09:55, Bharat Bhushan wrote: >>>>> 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? >> >> Same story every time... and was discussed many, many times on the lists. >> >> 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=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa >> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG- >> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e= >> >> You need anyway SoC specific compatibles. Once you add proper compatibles, >> you will see that property is not needed. > > Also on a given soc, firmware can configure one of 64 timer to be used as system watchdog time then compatible will not work. Can't you query the firmware for that? Or can't you just choose first unused timer? DT is for non-discoverable properties. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, May 5, 2023 4:04 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 05/05/2023 09:55, Bharat Bhushan wrote: > >>>>> 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? > >> > >> Same story every time... and was discussed many, many times on the lists. > >> > >> 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=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa > >> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG- > >> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e= > >> > >> You need anyway SoC specific compatibles. Once you add proper > >> compatibles, you will see that property is not needed. > > > > Also on a given soc, firmware can configure one of 64 timer to be used as > system watchdog time then compatible will not work. > > Can't you query the firmware for that? Or can't you just choose first unused > timer? DT is for non-discoverable properties. Query to firmware required arm SMC call, to me that does not look correct approach. Thought of using first one but that is already used and moving that is as same as this. Hardcoding to 63 will make it work on some SoCs but not all. Thanks -Bharat > > Best regards, > Krzysztof
On 05/05/2023 12:41, Bharat Bhushan wrote: >>>> >>>> You need anyway SoC specific compatibles. Once you add proper >>>> compatibles, you will see that property is not needed. >>> >>> Also on a given soc, firmware can configure one of 64 timer to be used as >> system watchdog time then compatible will not work. >> >> Can't you query the firmware for that? Or can't you just choose first unused >> timer? DT is for non-discoverable properties. > > Query to firmware required arm SMC call, to me that does not look correct approach. Thought of using first one but that is already used and moving that is as same as this. > > Hardcoding to 63 will make it work on some SoCs but not all. But you know which one is started or is not. GTI_CWD_WDOG tells you this. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, May 5, 2023 4:27 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 05/05/2023 12:41, Bharat Bhushan wrote: > >>>> > >>>> You need anyway SoC specific compatibles. Once you add proper > >>>> compatibles, you will see that property is not needed. > >>> > >>> Also on a given soc, firmware can configure one of 64 timer to be > >>> used as > >> system watchdog time then compatible will not work. > >> > >> Can't you query the firmware for that? Or can't you just choose first > >> unused timer? DT is for non-discoverable properties. > > > > Query to firmware required arm SMC call, to me that does not look correct > approach. Thought of using first one but that is already used and moving that is as > same as this. > > > > Hardcoding to 63 will make it work on some SoCs but not all. > > But you know which one is started or is not. GTI_CWD_WDOG tells you this. On a given SoC, Firmware can reserve and/or use one or more timer for some other use case (customer use) and configure one of the timer as watchdog timer. Linux have to use the configured timer only and cannot decide by its own. Thanks -Bharat > > Best regards, > Krzysztof
On 05/05/2023 13:15, Bharat Bhushan wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Friday, May 5, 2023 4:27 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system >> watchdog driver >> >> On 05/05/2023 12:41, Bharat Bhushan wrote: >>>>>> >>>>>> You need anyway SoC specific compatibles. Once you add proper >>>>>> compatibles, you will see that property is not needed. >>>>> >>>>> Also on a given soc, firmware can configure one of 64 timer to be >>>>> used as >>>> system watchdog time then compatible will not work. >>>> >>>> Can't you query the firmware for that? Or can't you just choose first >>>> unused timer? DT is for non-discoverable properties. >>> >>> Query to firmware required arm SMC call, to me that does not look correct >> approach. Thought of using first one but that is already used and moving that is as >> same as this. >>> >>> Hardcoding to 63 will make it work on some SoCs but not all. >> >> But you know which one is started or is not. GTI_CWD_WDOG tells you this. > > On a given SoC, Firmware can reserve and/or use one or more timer for some other use case (customer use) and configure one of the timer as watchdog timer. Linux have to use the configured timer only and cannot decide by its own. Then the SoCs which have such firmware could use proposed property. Provide some rationale property description in your next version (adding necessary compatibles). Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, May 5, 2023 5:27 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 05/05/2023 13:15, Bharat Bhushan wrote: > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Friday, May 5, 2023 4:27 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: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell > >> GTI system watchdog driver > >> > >> On 05/05/2023 12:41, Bharat Bhushan wrote: > >>>>>> > >>>>>> You need anyway SoC specific compatibles. Once you add proper > >>>>>> compatibles, you will see that property is not needed. > >>>>> > >>>>> Also on a given soc, firmware can configure one of 64 timer to be > >>>>> used as > >>>> system watchdog time then compatible will not work. > >>>> > >>>> Can't you query the firmware for that? Or can't you just choose > >>>> first unused timer? DT is for non-discoverable properties. > >>> > >>> Query to firmware required arm SMC call, to me that does not look > >>> correct > >> approach. Thought of using first one but that is already used and > >> moving that is as same as this. > >>> > >>> Hardcoding to 63 will make it work on some SoCs but not all. > >> > >> But you know which one is started or is not. GTI_CWD_WDOG tells you this. > > > > On a given SoC, Firmware can reserve and/or use one or more timer for some > other use case (customer use) and configure one of the timer as watchdog timer. > Linux have to use the configured timer only and cannot decide by its own. > > Then the SoCs which have such firmware could use proposed property. > Provide some rationale property description in your next version (adding > necessary compatibles). Will add compatible as below: properties: compatible: enum: - marvell,cn10k-wdt - marvell,octeontx2-wdt And define this as optional property marvell,wdt-timer-index: $ref: /schemas/types.yaml#/definitions/uint32 minimum: 0 maximum: 63 description: An SoC have many timers (up to 64), firmware can reserve one or more timer for some other use case and configures one of the global timer as watchdog timer. Firmware will update this field with the timer number configured as watchdog timer. Let me know if this looks good. Thanks -Bharat > > Best regards, > Krzysztof
On 05/05/2023 14:19, Bharat Bhushan wrote: >>>>> Query to firmware required arm SMC call, to me that does not look >>>>> correct >>>> approach. Thought of using first one but that is already used and >>>> moving that is as same as this. >>>>> >>>>> Hardcoding to 63 will make it work on some SoCs but not all. >>>> >>>> But you know which one is started or is not. GTI_CWD_WDOG tells you this. >>> >>> On a given SoC, Firmware can reserve and/or use one or more timer for some >> other use case (customer use) and configure one of the timer as watchdog timer. >> Linux have to use the configured timer only and cannot decide by its own. >> >> Then the SoCs which have such firmware could use proposed property. >> Provide some rationale property description in your next version (adding >> necessary compatibles). > > Will add compatible as below: > > properties: > compatible: > enum: > - marvell,cn10k-wdt > - marvell,octeontx2-wdt And the rest? You said you have 10 SoCs. You made them not compatible with each other, so I assume on purpose and your driver will make use of it. > > And define this as optional property > marvell,wdt-timer-index: > $ref: /schemas/types.yaml#/definitions/uint32 > minimum: 0 > maximum: 63 > description: > An SoC have many timers (up to 64), firmware can reserve one or more timer > for some other use case and configures one of the global timer as watchdog > timer. Firmware will update this field with the timer number configured > as watchdog timer. Looks ok. Best regards, Krzysztof
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: http://devicetree.org/schemas/watchdog/marvell,gti-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Marvell Global Timer (GTI) system watchdog + +allOf: + - $ref: watchdog.yaml# + +maintainers: + - Bharat Bhushan <bbhushan2@marvell.com> + +properties: + compatible: + const: marvell,gti-wdt + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + wdt-timer-index: + maxItems: 1 + description: + This contains the timer number out of total 64 timers supported + by GTI hardware block. + +required: + - compatible + - reg + - interrupts + - wdt-timer-index + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + soc { + #address-cells = <2>; + #size-cells = <2>; + + watchdog@802000040000 { + compatible = "marvell,gti-wdt"; + reg = <0x00008020 0x00040000 0x00000000 0x00020000>; + interrupts = <0 38 IRQ_TYPE_EDGE_RISING>; + wdt-timer-index = <63>; + }; + }; + +...
Add binding documentation for the Marvell GTI system watchdog driver. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- v5: - Added wdt-timer-index property - Get clock frequency from clocks/clock-name device tree property .../bindings/watchdog/marvell,gti-wdt.yaml | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml