| Message ID | 20211028183527.3050-3-semen.protsenko@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | watchdog: s3c2410: Add Exynos850 support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | success |
On 28/10/2021 20:35, Sam Protsenko wrote: > Exynos850 SoC has two CPU clusters: > - cluster 0: contains CPUs #0, #1, #2, #3 > - cluster 1: contains CPUs #4, #5, #6, #7 > > Each cluster has its own dedicater watchdog timer. Those WDT instances > are controlled using different bits in PMU registers, so there should be > two different compatible strings (for each cluster), to tell the driver > which bits to use for each WDT instance. > > Also on Exynos850 the peripheral clock and the source clock are two > different clocks. Provide a way to specify two clocks in watchdog device > tree node. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > .../devicetree/bindings/watchdog/samsung-wdt.yaml | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > index 93cd77a6e92c..19c7f7767559 100644 > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > @@ -22,16 +22,24 @@ properties: > - samsung,exynos5250-wdt # for Exynos5250 > - samsung,exynos5420-wdt # for Exynos5420 > - samsung,exynos7-wdt # for Exynos7 > + - samsung,exynos850-cl0-wdt # for Exynos850 (CPU cluster 0) > + - samsung,exynos850-cl1-wdt # for Exynos850 (CPU cluster 1) I would prefer to have one compatible and additional u32 property pointing to cluster index. The driver would use this property to adjust the PMU register offsets or bits. Why? Because if next time you have three clusters, you will need to make three compatibles for something which differs by only two register offsets. Both watchdog instances (or three in some unspecified future) are here the same, they just control different blocks, therefore should accept some parameter instead of making them different compatibles. > > reg: > maxItems: 1 > > clocks: > - maxItems: 1 > + minItems: 1 > + items: > + - description: Peripheral clock used for register interface; if it's the > + only clock, it's also a source clock > + - description: Source clock (optional) > > clock-names: > + minItems: 1 > items: > - const: watchdog > + - const: watchdog_src Don't you require src clock on Exynos850? Best regards, Krzysztof
On Fri, 29 Oct 2021 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 28/10/2021 20:35, Sam Protsenko wrote: > > Exynos850 SoC has two CPU clusters: > > - cluster 0: contains CPUs #0, #1, #2, #3 > > - cluster 1: contains CPUs #4, #5, #6, #7 > > > > Each cluster has its own dedicater watchdog timer. Those WDT instances > > are controlled using different bits in PMU registers, so there should be > > two different compatible strings (for each cluster), to tell the driver > > which bits to use for each WDT instance. > > > > Also on Exynos850 the peripheral clock and the source clock are two > > different clocks. Provide a way to specify two clocks in watchdog device > > tree node. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > .../devicetree/bindings/watchdog/samsung-wdt.yaml | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > > index 93cd77a6e92c..19c7f7767559 100644 > > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > > @@ -22,16 +22,24 @@ properties: > > - samsung,exynos5250-wdt # for Exynos5250 > > - samsung,exynos5420-wdt # for Exynos5420 > > - samsung,exynos7-wdt # for Exynos7 > > + - samsung,exynos850-cl0-wdt # for Exynos850 (CPU cluster 0) > > + - samsung,exynos850-cl1-wdt # for Exynos850 (CPU cluster 1) > > I would prefer to have one compatible and additional u32 property > pointing to cluster index. The driver would use this property to adjust > the PMU register offsets or bits. > > Why? Because if next time you have three clusters, you will need to make > three compatibles for something which differs by only two register > offsets. Both watchdog instances (or three in some unspecified future) > are here the same, they just control different blocks, therefore should > accept some parameter instead of making them different compatibles. > Agreed. I considered both cases, both looked ugly to me. But having one compatible is probably better in the end, although it'll require some additional code in the driver. Anyway, will be done in v2, will send it soon. > > > > reg: > > maxItems: 1 > > > > clocks: > > - maxItems: 1 > > + minItems: 1 > > + items: > > + - description: Peripheral clock used for register interface; if it's the > > + only clock, it's also a source clock > > + - description: Source clock (optional) > > > > clock-names: > > + minItems: 1 > > items: > > - const: watchdog > > + - const: watchdog_src > > Don't you require src clock on Exynos850? > Will be addressed in v2 properly, thanks. > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml index 93cd77a6e92c..19c7f7767559 100644 --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml @@ -22,16 +22,24 @@ properties: - samsung,exynos5250-wdt # for Exynos5250 - samsung,exynos5420-wdt # for Exynos5420 - samsung,exynos7-wdt # for Exynos7 + - samsung,exynos850-cl0-wdt # for Exynos850 (CPU cluster 0) + - samsung,exynos850-cl1-wdt # for Exynos850 (CPU cluster 1) reg: maxItems: 1 clocks: - maxItems: 1 + minItems: 1 + items: + - description: Peripheral clock used for register interface; if it's the + only clock, it's also a source clock + - description: Source clock (optional) clock-names: + minItems: 1 items: - const: watchdog + - const: watchdog_src interrupts: maxItems: 1 @@ -40,7 +48,7 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: Phandle to the PMU system controller node (in case of Exynos5250, - Exynos5420 and Exynos7). + Exynos5420, Exynos7 and Exynos850). required: - compatible @@ -59,6 +67,8 @@ allOf: - samsung,exynos5250-wdt - samsung,exynos5420-wdt - samsung,exynos7-wdt + - samsung,exynos850-cl0-wdt + - samsung,exynos850-cl1-wdt then: required: - samsung,syscon-phandle
Exynos850 SoC has two CPU clusters: - cluster 0: contains CPUs #0, #1, #2, #3 - cluster 1: contains CPUs #4, #5, #6, #7 Each cluster has its own dedicater watchdog timer. Those WDT instances are controlled using different bits in PMU registers, so there should be two different compatible strings (for each cluster), to tell the driver which bits to use for each WDT instance. Also on Exynos850 the peripheral clock and the source clock are two different clocks. Provide a way to specify two clocks in watchdog device tree node. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- .../devicetree/bindings/watchdog/samsung-wdt.yaml | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)