diff mbox series

[4/7] dt-bindings: clock: Add Nuvoton WPCM450 clock/reset controller

Message ID 20220422183012.444674-5-j.neuschaefer@gmx.net
State Superseded
Headers show
Series Nuvoton WPCM450 clock and reset driver | expand

Commit Message

J. Neuschäfer April 22, 2022, 6:30 p.m. UTC
The Nuvoton WPCM450 SoC has a combined clock and reset controller.
Add a devicetree binding for it, as well as definitions for the bit
numbers used by it.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../bindings/clock/nuvoton,wpcm450-clk.yaml   | 74 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,wpcm450-clk.h   | 67 +++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,wpcm450-clk.h

--
2.35.1

Comments

Joel Stanley April 26, 2022, 8:35 a.m. UTC | #1
On Mon, 25 Apr 2022 at 07:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 22/04/2022 20:30, Jonathan Neuschäfer wrote:
> > The Nuvoton WPCM450 SoC has a combined clock and reset controller.
> > Add a devicetree binding for it, as well as definitions for the bit
> > numbers used by it.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
>
> Thank you for your patch. There is something to discuss/improve.
>
> >  .../bindings/clock/nuvoton,wpcm450-clk.yaml   | 74 +++++++++++++++++++
> >  .../dt-bindings/clock/nuvoton,wpcm450-clk.h   | 67 +++++++++++++++++
> >  2 files changed, 141 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
> >  create mode 100644 include/dt-bindings/clock/nuvoton,wpcm450-clk.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
> > new file mode 100644
> > index 0000000000000..0fffa8a68dee4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/nuvoton,wpcm450-clk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton WPCM450 clock controller binding
>
> s/binding//
>
> > +
> > +maintainers:
> > +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > +
> > +description:
> > +  This binding describes the clock controller of the Nuvoton WPCM450 SoC, which
> > +  supplies clocks and resets to the rest of the chip.
>
> s/This binding describes//
>
> Just describe the hardware.
>
> > +
> > +properties:
> > +  compatible:
> > +    const: nuvoton,wpcm450-clk
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Reference clock oscillator (should be 48 MHz)
> > +
> > +  clock-names:
> > +    items:
> > +      - const: refclk
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  '#reset-cells':
> > +    const: 1
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    refclk: clock-48mhz {
> > +      /* 48 MHz reference oscillator */
> > +      compatible = "fixed-clock";
> > +      clock-output-names = "refclk";
> > +      clock-frequency = <48000000>;
> > +      #clock-cells = <0>;
> > +    };
> > +
> > +    clk: clock-controller@b0000200 {
> > +      reg = <0xb0000200 0x100>;
> > +      compatible = "nuvoton,wpcm450-clk";
> > +      clocks = <&refclk>;
> > +      clock-names = "refclk";
> > +      #clock-cells = <1>;
> > +      #reset-cells = <1>;
> > +    };
> > +
> > +    serial@b8000000 {
> > +      compatible = "nuvoton,wpcm450-uart";
> > +      reg = <0xb8000000 0x20>;
> > +      reg-shift = <2>;
> > +      interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
> > +      clocks = <&clk WPCM450_CLK_UART0>;
> > +    };
>
> Skip the consumer example, it's obvious/trivial/duplicating.
>
> > diff --git a/include/dt-bindings/clock/nuvoton,wpcm450-clk.h b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
> > new file mode 100644
> > index 0000000000000..86e1c895921b7
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +#ifndef _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
> > +#define _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
> > +
> > +/* Clocks based on CLKEN bits */
> > +#define WPCM450_CLK_FIU            0
> > +#define WPCM450_CLK_XBUS           1
> > +#define WPCM450_CLK_KCS            2
> > +#define WPCM450_CLK_SHM            4
> > +#define WPCM450_CLK_USB1           5
> > +#define WPCM450_CLK_EMC0           6
> > +#define WPCM450_CLK_EMC1           7
> > +#define WPCM450_CLK_USB0           8
> > +#define WPCM450_CLK_PECI           9
> > +#define WPCM450_CLK_AES           10
> > +#define WPCM450_CLK_UART0         11
> > +#define WPCM450_CLK_UART1         12
> > +#define WPCM450_CLK_SMB2          13
> > +#define WPCM450_CLK_SMB3          14
> > +#define WPCM450_CLK_SMB4          15
> > +#define WPCM450_CLK_SMB5          16
> > +#define WPCM450_CLK_HUART         17
> > +#define WPCM450_CLK_PWM           18
> > +#define WPCM450_CLK_TIMER0        19
> > +#define WPCM450_CLK_TIMER1        20
> > +#define WPCM450_CLK_TIMER2        21
> > +#define WPCM450_CLK_TIMER3        22
> > +#define WPCM450_CLK_TIMER4        23
> > +#define WPCM450_CLK_MFT0          24
> > +#define WPCM450_CLK_MFT1          25
> > +#define WPCM450_CLK_WDT           26
> > +#define WPCM450_CLK_ADC           27
> > +#define WPCM450_CLK_SDIO          28
> > +#define WPCM450_CLK_SSPI          29
> > +#define WPCM450_CLK_SMB0          30
> > +#define WPCM450_CLK_SMB1          31
> > +
> > +/* Other clocks */
> > +#define WPCM450_CLK_USBPHY        32
> > +
> > +#define WPCM450_NUM_CLKS          33
> > +
> > +/* Resets based on IPSRST bits */
>
> All these defines should be in second header in dt-bindings/reset/...

I disagree. It makes more sense to keep the definitions together, and
it's all for the same hardware and driver.

>
> > +#define WPCM450_RESET_FIU          0
>
>
> Best regards,
> Krzysztof
J. Neuschäfer April 28, 2022, 8:48 a.m. UTC | #2
Hello,

On Sat, Apr 23, 2022 at 11:56:42AM +0200, Krzysztof Kozlowski wrote:
> On 22/04/2022 20:30, Jonathan Neuschäfer wrote:
> > The Nuvoton WPCM450 SoC has a combined clock and reset controller.
> > Add a devicetree binding for it, as well as definitions for the bit
> > numbers used by it.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> >  .../bindings/clock/nuvoton,wpcm450-clk.yaml   | 74 +++++++++++++++++++
> >  .../dt-bindings/clock/nuvoton,wpcm450-clk.h   | 67 +++++++++++++++++
> >  2 files changed, 141 insertions(+)
[...]
> > +title: Nuvoton WPCM450 clock controller binding
> 
> s/binding//

Will change.


> > +description:
> > +  This binding describes the clock controller of the Nuvoton WPCM450 SoC, which
> > +  supplies clocks and resets to the rest of the chip.
> 
> s/This binding describes//
> 
> Just describe the hardware.

Ok.


> > +    clk: clock-controller@b0000200 {
> > +      reg = <0xb0000200 0x100>;
> > +      compatible = "nuvoton,wpcm450-clk";
> > +      clocks = <&refclk>;
> > +      clock-names = "refclk";
> > +      #clock-cells = <1>;
> > +      #reset-cells = <1>;
> > +    };
> > +
> > +    serial@b8000000 {
> > +      compatible = "nuvoton,wpcm450-uart";
> > +      reg = <0xb8000000 0x20>;
> > +      reg-shift = <2>;
> > +      interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
> > +      clocks = <&clk WPCM450_CLK_UART0>;
> > +    };
> 
> Skip the consumer example, it's obvious/trivial/duplicating.

Ok.


> > +#ifndef _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
> > +#define _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
> > +
> > +/* Clocks based on CLKEN bits */
> > +#define WPCM450_CLK_FIU            0
[...]
> > +/* Other clocks */
> > +#define WPCM450_CLK_USBPHY        32
> > +
> > +#define WPCM450_NUM_CLKS          33
> > +
> > +/* Resets based on IPSRST bits */
> 
> All these defines should be in second header in dt-bindings/reset/...

(my reply is further down in the thread)
> 
> > +#define WPCM450_RESET_FIU          0



Thanks,
Jonathan
J. Neuschäfer April 28, 2022, 8:55 a.m. UTC | #3
On Tue, Apr 26, 2022 at 08:35:43AM +0000, Joel Stanley wrote:
> On Mon, 25 Apr 2022 at 07:59, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 22/04/2022 20:30, Jonathan Neuschäfer wrote:
> > > The Nuvoton WPCM450 SoC has a combined clock and reset controller.
> > > Add a devicetree binding for it, as well as definitions for the bit
> > > numbers used by it.
> > >
> > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > > ---
[...]
> > > +/* Other clocks */
> > > +#define WPCM450_CLK_USBPHY        32
> > > +
> > > +#define WPCM450_NUM_CLKS          33
> > > +
> > > +/* Resets based on IPSRST bits */
> >
> > All these defines should be in second header in dt-bindings/reset/...
> 
> I disagree. It makes more sense to keep the definitions together, and
> it's all for the same hardware and driver.

It's for the same hardware, DT node, and driver.

I could imagine splitting it into

	include/dt-bindings/clock/nuvoton,wpcm450-clk.h  and
	include/dt-bindings/reset/nuvoton,wpcm450-clk.h

if someone insists on it.

For convenience (being able to see all relevant definitions for
nuvoton,wpcm450-clk at once), I'd prefer to keep the definitions together.


Thanks,
Jonathan
Krzysztof Kozlowski April 28, 2022, 9:23 a.m. UTC | #4
On 28/04/2022 10:55, Jonathan Neuschäfer wrote:
>>>
>>> All these defines should be in second header in dt-bindings/reset/...
>>
>> I disagree. It makes more sense to keep the definitions together, and
>> it's all for the same hardware and driver.

These are bindings so the usage by same driver (Linux implementation)
matters less or even does not matter.

Driver can be split from one to several and you would need to include
clocks in your just-split-reset driver. Such driver split should not
affect bindings, therefore having the binding headers separate is
actually the most flexible.

> 
> It's for the same hardware, DT node, and driver.
> 
> I could imagine splitting it into
> 
> 	include/dt-bindings/clock/nuvoton,wpcm450-clk.h  and
> 	include/dt-bindings/reset/nuvoton,wpcm450-clk.h
> 
> if someone insists on it.
> 
> For convenience (being able to see all relevant definitions for
> nuvoton,wpcm450-clk at once), I'd prefer to keep the definitions together.

I don't insist. For some of the devices we split it, for some not.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
new file mode 100644
index 0000000000000..0fffa8a68dee4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
@@ -0,0 +1,74 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,wpcm450-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton WPCM450 clock controller binding
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description:
+  This binding describes the clock controller of the Nuvoton WPCM450 SoC, which
+  supplies clocks and resets to the rest of the chip.
+
+properties:
+  compatible:
+    const: nuvoton,wpcm450-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Reference clock oscillator (should be 48 MHz)
+
+  clock-names:
+    items:
+      - const: refclk
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    const: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    refclk: clock-48mhz {
+      /* 48 MHz reference oscillator */
+      compatible = "fixed-clock";
+      clock-output-names = "refclk";
+      clock-frequency = <48000000>;
+      #clock-cells = <0>;
+    };
+
+    clk: clock-controller@b0000200 {
+      reg = <0xb0000200 0x100>;
+      compatible = "nuvoton,wpcm450-clk";
+      clocks = <&refclk>;
+      clock-names = "refclk";
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+    };
+
+    serial@b8000000 {
+      compatible = "nuvoton,wpcm450-uart";
+      reg = <0xb8000000 0x20>;
+      reg-shift = <2>;
+      interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&clk WPCM450_CLK_UART0>;
+    };
diff --git a/include/dt-bindings/clock/nuvoton,wpcm450-clk.h b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
new file mode 100644
index 0000000000000..86e1c895921b7
--- /dev/null
+++ b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
@@ -0,0 +1,67 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
+#define _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
+
+/* Clocks based on CLKEN bits */
+#define WPCM450_CLK_FIU            0
+#define WPCM450_CLK_XBUS           1
+#define WPCM450_CLK_KCS            2
+#define WPCM450_CLK_SHM            4
+#define WPCM450_CLK_USB1           5
+#define WPCM450_CLK_EMC0           6
+#define WPCM450_CLK_EMC1           7
+#define WPCM450_CLK_USB0           8
+#define WPCM450_CLK_PECI           9
+#define WPCM450_CLK_AES           10
+#define WPCM450_CLK_UART0         11
+#define WPCM450_CLK_UART1         12
+#define WPCM450_CLK_SMB2          13
+#define WPCM450_CLK_SMB3          14
+#define WPCM450_CLK_SMB4          15
+#define WPCM450_CLK_SMB5          16
+#define WPCM450_CLK_HUART         17
+#define WPCM450_CLK_PWM           18
+#define WPCM450_CLK_TIMER0        19
+#define WPCM450_CLK_TIMER1        20
+#define WPCM450_CLK_TIMER2        21
+#define WPCM450_CLK_TIMER3        22
+#define WPCM450_CLK_TIMER4        23
+#define WPCM450_CLK_MFT0          24
+#define WPCM450_CLK_MFT1          25
+#define WPCM450_CLK_WDT           26
+#define WPCM450_CLK_ADC           27
+#define WPCM450_CLK_SDIO          28
+#define WPCM450_CLK_SSPI          29
+#define WPCM450_CLK_SMB0          30
+#define WPCM450_CLK_SMB1          31
+
+/* Other clocks */
+#define WPCM450_CLK_USBPHY        32
+
+#define WPCM450_NUM_CLKS          33
+
+/* Resets based on IPSRST bits */
+#define WPCM450_RESET_FIU          0
+#define WPCM450_RESET_EMC0         6
+#define WPCM450_RESET_EMC1         7
+#define WPCM450_RESET_USB0         8
+#define WPCM450_RESET_USB1         9
+#define WPCM450_RESET_AES_PECI    10
+#define WPCM450_RESET_UART        11
+#define WPCM450_RESET_MC          12
+#define WPCM450_RESET_SMB2        13
+#define WPCM450_RESET_SMB3        14
+#define WPCM450_RESET_SMB4        15
+#define WPCM450_RESET_SMB5        16
+#define WPCM450_RESET_PWM         18
+#define WPCM450_RESET_TIMER       19
+#define WPCM450_RESET_ADC         27
+#define WPCM450_RESET_SDIO        28
+#define WPCM450_RESET_SSPI        29
+#define WPCM450_RESET_SMB0        30
+#define WPCM450_RESET_SMB1        31
+
+#define WPCM450_NUM_RESETS        32
+
+#endif /* _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H */