| Message ID | 20220422183012.444674-5-j.neuschaefer@gmx.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Nuvoton WPCM450 clock and reset driver | expand |
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
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
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
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 --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 */
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