| Message ID | 20250402154942.3645283-2-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add the NXP S32 Watchdog | expand |
On 02/04/2025 17:49, Daniel Lezcano wrote: > Describe the Software Watchdog Timer available on the S32G platforms. > > Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> > Cc: Thomas Fossati <thomas.fossati@linaro.org> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Wed, 02 Apr 2025 17:49:39 +0200, Daniel Lezcano wrote: > Describe the Software Watchdog Timer available on the S32G platforms. > > Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> > Cc: Thomas Fossati <thomas.fossati@linaro.org> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > .../bindings/watchdog/nxp,s32g2-swt.yaml | 42 +++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml:17:7: [warning] wrong indentation: expected 4 but found 6 (indentation) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml: ignoring, error in schema: properties: compatible /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml: properties:compatible: [{'const': 'nxp,s32g2-swt'}, {'items': [{'const': 'nxp,s32g3-swt'}, {'const': 'nxp,s32g2-swt'}]}] is not of type 'object', 'boolean' from schema $id: http://json-schema.org/draft-07/schema# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml: properties:compatible: [{'const': 'nxp,s32g2-swt'}, {'items': [{'const': 'nxp,s32g3-swt'}, {'const': 'nxp,s32g2-swt'}]}] is not of type 'object', 'boolean' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.example.dts:18.29-23.11: Warning (unit_address_format): /example-0/watchdog@0x40100000: unit name should not have leading "0x" Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.example.dtb: /example-0/watchdog@0x40100000: failed to match any schema with compatible: ['nxp,s32g2-swt'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250402154942.3645283-2-daniel.lezcano@linaro.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 02/04/2025 17:56, Krzysztof Kozlowski wrote: > On 02/04/2025 17:49, Daniel Lezcano wrote: >> Describe the Software Watchdog Timer available on the S32G platforms. >> >> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> >> Cc: Thomas Fossati <thomas.fossati@linaro.org> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Best regards, > Krzysztof I ran the make dt_binding_check but inadvertently removed the "oneOf:" after :/
On 4/2/2025 6:49 PM, Daniel Lezcano wrote: [ ... ] > +examples: > + - | > + watchdog@0x40100000 { > + compatible = "nxp,s32g2-swt"; > + reg = <0x40100000 0x1000>; > + clocks = <&clks 0x3a>; > + timeout-sec = <10>; > + }; The S32G reference manual specifies two clocks for the SWT module: one for the registers and another for the counter itself. Shouldn't both clocks be represented in the bindings?
On 03/04/2025 01:01, Daniel Lezcano wrote: > On 02/04/2025 17:56, Krzysztof Kozlowski wrote: >> On 02/04/2025 17:49, Daniel Lezcano wrote: >>> Describe the Software Watchdog Timer available on the S32G platforms. >>> >>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> >>> Cc: Thomas Fossati <thomas.fossati@linaro.org> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> Best regards, >> Krzysztof > > I ran the make dt_binding_check but inadvertently removed the "oneOf:" > after :/ Other errors were also not fixed even though you got report on them on 29th of March. Best regards, Krzysztof
On 03/04/2025 08:19, Ghennadi Procopciuc wrote: > On 4/2/2025 6:49 PM, Daniel Lezcano wrote: > [ ... ] >> +examples: >> + - | >> + watchdog@0x40100000 { >> + compatible = "nxp,s32g2-swt"; >> + reg = <0x40100000 0x1000>; >> + clocks = <&clks 0x3a>; >> + timeout-sec = <10>; >> + }; > > The S32G reference manual specifies two clocks for the SWT module: one > for the registers and another for the counter itself. Shouldn't both > clocks be represented in the bindings? AFAICS, there are two clocks as described in the documentation for the s32g2 page 846, section 23.7.3.3 SWT clocking. The module and the register clock are fed by the XBAR_DIV3_CLK which is an system clock always-on. The counter is fed by the FIRC_CLK which described as "FIRC_CLK is the default clock for the entire system at power-up." From my understanding, we should not describe the XBAR_DIV3_CLK as it is a system clock. And the FIRC_CLK is only there to get the clock rate in the driver.
On 03/04/2025 16:16, Krzysztof Kozlowski wrote: > On 03/04/2025 01:01, Daniel Lezcano wrote: >> On 02/04/2025 17:56, Krzysztof Kozlowski wrote: >>> On 02/04/2025 17:49, Daniel Lezcano wrote: >>>> Describe the Software Watchdog Timer available on the S32G platforms. >>>> >>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> >>>> Cc: Thomas Fossati <thomas.fossati@linaro.org> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> Best regards, >>> Krzysztof >> >> I ran the make dt_binding_check but inadvertently removed the "oneOf:" >> after :/ > Other errors were also not fixed even though you got report on them on > 29th of March. Which ones ? All errors were coming from the oneOf removal, after re-adding it they are all gone
On 03/04/2025 17:33, Daniel Lezcano wrote: > On 03/04/2025 16:16, Krzysztof Kozlowski wrote: >> On 03/04/2025 01:01, Daniel Lezcano wrote: >>> On 02/04/2025 17:56, Krzysztof Kozlowski wrote: >>>> On 02/04/2025 17:49, Daniel Lezcano wrote: >>>>> Describe the Software Watchdog Timer available on the S32G platforms. >>>>> >>>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> >>>>> Cc: Thomas Fossati <thomas.fossati@linaro.org> >>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> >>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> >>>> Best regards, >>>> Krzysztof >>> >>> I ran the make dt_binding_check but inadvertently removed the "oneOf:" >>> after :/ >> Other errors were also not fixed even though you got report on them on >> 29th of March. > > Which ones ? > > All errors were coming from the oneOf removal, after re-adding it they > are all gone The same as from previous patchset - wrong unit address, maybe more. > Best regards, Krzysztof
On 03/04/2025 17:35, Krzysztof Kozlowski wrote: > On 03/04/2025 17:33, Daniel Lezcano wrote: >> On 03/04/2025 16:16, Krzysztof Kozlowski wrote: >>> On 03/04/2025 01:01, Daniel Lezcano wrote: >>>> On 02/04/2025 17:56, Krzysztof Kozlowski wrote: >>>>> On 02/04/2025 17:49, Daniel Lezcano wrote: >>>>>> Describe the Software Watchdog Timer available on the S32G platforms. >>>>>> >>>>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> >>>>>> Cc: Thomas Fossati <thomas.fossati@linaro.org> >>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>> >>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>> >>>>> Best regards, >>>>> Krzysztof >>>> >>>> I ran the make dt_binding_check but inadvertently removed the "oneOf:" >>>> after :/ >>> Other errors were also not fixed even though you got report on them on >>> 29th of March. >> >> Which ones ? >> >> All errors were coming from the oneOf removal, after re-adding it they >> are all gone > > > The same as from previous patchset - wrong unit address, maybe more. Indeed, my parser was not up-to-date and did not reported the error. Thanks for pointing this out -- Daniel
On 4/3/2025 6:10 PM, Daniel Lezcano wrote: > On 03/04/2025 08:19, Ghennadi Procopciuc wrote: >> On 4/2/2025 6:49 PM, Daniel Lezcano wrote: >> [ ... ] >>> +examples: >>> + - | >>> + watchdog@0x40100000 { >>> + compatible = "nxp,s32g2-swt"; >>> + reg = <0x40100000 0x1000>; >>> + clocks = <&clks 0x3a>; >>> + timeout-sec = <10>; >>> + }; >> >> The S32G reference manual specifies two clocks for the SWT module: one >> for the registers and another for the counter itself. Shouldn't both >> clocks be represented in the bindings? > > AFAICS, there are two clocks as described in the documentation for the > s32g2 page 846, section 23.7.3.3 SWT clocking. This diagram illustrates the module clocks and their connections to the S32GS system clocks. From the module's perspective, there are three clocks: MODULE_CLOCK, REG_INTF, and COUNTER_CLOCK. Specifically, on S32G2 SoCs, the first two are connected to XBAR_DIV3_CLK, while the counter clock is linked to FIRC_CLK. Based on my understanding of the device tree, this configuration should be listed as follows: clocks = <&clks XBAR_DIV3_CLK>, <&clks XBAR_DIV3_CLK>, <&clks FIRC_CLK>; clock-names = "module", "reg", "counter"; Configuring it this way allows flexibility to reuse the same clocking scheme for other SoCs where the integration is performed differently. It is possible that the 'module' and 'reg' clocks could be linked to two distinct system clocks. > > The module and the register clock are fed by the XBAR_DIV3_CLK which is > an system clock always-on. XBAR_DIV3_CLK is not an always-on clock, meaning it is not available during suspend, if that is what you mean by always-on. The SIRC can be considered the only always-on clock on this device. > > The counter is fed by the FIRC_CLK which described as "FIRC_CLK is the > default clock for the entire system at power-up." > > From my understanding, we should not describe the XBAR_DIV3_CLK as it is > a system clock. > > And the FIRC_CLK is only there to get the clock rate in the driver. >
On 04/04/2025 08:35, Ghennadi Procopciuc wrote: > On 4/3/2025 6:10 PM, Daniel Lezcano wrote: >> On 03/04/2025 08:19, Ghennadi Procopciuc wrote: >>> On 4/2/2025 6:49 PM, Daniel Lezcano wrote: >>> [ ... ] >>>> +examples: >>>> + - | >>>> + watchdog@0x40100000 { >>>> + compatible = "nxp,s32g2-swt"; >>>> + reg = <0x40100000 0x1000>; >>>> + clocks = <&clks 0x3a>; >>>> + timeout-sec = <10>; >>>> + }; >>> >>> The S32G reference manual specifies two clocks for the SWT module: one >>> for the registers and another for the counter itself. Shouldn't both >>> clocks be represented in the bindings? >> >> AFAICS, there are two clocks as described in the documentation for the >> s32g2 page 846, section 23.7.3.3 SWT clocking. > > This diagram illustrates the module clocks and their connections to the > S32GS system clocks. From the module's perspective, there are three > clocks: MODULE_CLOCK, REG_INTF, and COUNTER_CLOCK. Specifically, on > S32G2 SoCs, the first two are connected to XBAR_DIV3_CLK, while the > counter clock is linked to FIRC_CLK. Based on my understanding of the > device tree, this configuration should be listed as follows: > > clocks = <&clks XBAR_DIV3_CLK>, <&clks XBAR_DIV3_CLK>, <&clks FIRC_CLK>; > clock-names = "module", "reg", "counter"; > > Configuring it this way allows flexibility to reuse the same clocking > scheme for other SoCs where the integration is performed differently. It > is possible that the 'module' and 'reg' clocks could be linked to two > distinct system clocks. That is something we can handle when the other SoC will be in the process of being upstream, no ? I don't see how that can help with the current hardware we are describing. What is the benefit ? I would prefer to stick to what is needed today >> The module and the register clock are fed by the XBAR_DIV3_CLK which is >> an system clock always-on. > > XBAR_DIV3_CLK is not an always-on clock, meaning it is not available > during suspend, if that is what you mean by always-on. The SIRC can be > considered the only always-on clock on this device. > >> >> The counter is fed by the FIRC_CLK which described as "FIRC_CLK is the >> default clock for the entire system at power-up." >> >> From my understanding, we should not describe the XBAR_DIV3_CLK as it is >> a system clock. >> >> And the FIRC_CLK is only there to get the clock rate in the driver. >> >
On 4/6/2025 11:33 PM, Daniel Lezcano wrote: > On 04/04/2025 08:35, Ghennadi Procopciuc wrote: >> On 4/3/2025 6:10 PM, Daniel Lezcano wrote: >>> On 03/04/2025 08:19, Ghennadi Procopciuc wrote: >>>> On 4/2/2025 6:49 PM, Daniel Lezcano wrote: >>>> [ ... ] >>>>> +examples: >>>>> + - | >>>>> + watchdog@0x40100000 { >>>>> + compatible = "nxp,s32g2-swt"; >>>>> + reg = <0x40100000 0x1000>; >>>>> + clocks = <&clks 0x3a>; >>>>> + timeout-sec = <10>; >>>>> + }; >>>> >>>> The S32G reference manual specifies two clocks for the SWT module: one >>>> for the registers and another for the counter itself. Shouldn't both >>>> clocks be represented in the bindings? >>> >>> AFAICS, there are two clocks as described in the documentation for the >>> s32g2 page 846, section 23.7.3.3 SWT clocking. >> >> This diagram illustrates the module clocks and their connections to the >> S32GS system clocks. From the module's perspective, there are three >> clocks: MODULE_CLOCK, REG_INTF, and COUNTER_CLOCK. Specifically, on >> S32G2 SoCs, the first two are connected to XBAR_DIV3_CLK, while the >> counter clock is linked to FIRC_CLK. Based on my understanding of the >> device tree, this configuration should be listed as follows: >> >> clocks = <&clks XBAR_DIV3_CLK>, <&clks XBAR_DIV3_CLK>, <&clks FIRC_CLK>; >> clock-names = "module", "reg", "counter"; >> >> Configuring it this way allows flexibility to reuse the same clocking >> scheme for other SoCs where the integration is performed differently. It >> is possible that the 'module' and 'reg' clocks could be linked to two >> distinct system clocks. > > That is something we can handle when the other SoC will be in the > process of being upstream, no ? > > I don't see how that can help with the current hardware we are > describing. What is the benefit ? > The benefit is that the binding accurately describes the hardware module, and no changes are expected in this area over time, even if different integrations of the module are performed on other SoCs. Otherwise, the binding will contain a different set of clocks depending on the SoC integration, even though the exact same hardware IP is used. > I would prefer to stick to what is needed today > >>> The module and the register clock are fed by the XBAR_DIV3_CLK which is >>> an system clock always-on. >> >> XBAR_DIV3_CLK is not an always-on clock, meaning it is not available >> during suspend, if that is what you mean by always-on. The SIRC can be >> considered the only always-on clock on this device. >> >>> >>> The counter is fed by the FIRC_CLK which described as "FIRC_CLK is the >>> default clock for the entire system at power-up." >>> >>> From my understanding, we should not describe the XBAR_DIV3_CLK as >>> it is >>> a system clock. >>> >>> And the FIRC_CLK is only there to get the clock rate in the driver. >>> >> > >
diff --git a/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml b/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml new file mode 100644 index 000000000000..d5f2513c4e8f --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/nxp,s32g2-swt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP Software Watchdog Timer (SWT) + +maintainers: + - Daniel Lezcano <daniel.lezcano@kernel.org> + +allOf: + - $ref: watchdog.yaml# + +properties: + compatible: + - const: nxp,s32g2-swt + - items: + - const: nxp,s32g3-swt + - const: nxp,s32g2-swt + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + +required: + - compatible + - reg + - clocks + +unevaluatedProperties: false + +examples: + - | + watchdog@0x40100000 { + compatible = "nxp,s32g2-swt"; + reg = <0x40100000 0x1000>; + clocks = <&clks 0x3a>; + timeout-sec = <10>; + };
Describe the Software Watchdog Timer available on the S32G platforms. Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com> Cc: Thomas Fossati <thomas.fossati@linaro.org> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- .../bindings/watchdog/nxp,s32g2-swt.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g2-swt.yaml