There are additional SpacemiT syscon CCUs whose registers control both
clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined
previously, these will (initially) support only resets. They do not
incorporate power domain functionality.
Previously the clock properties were required for all compatible nodes.
Make that requirement only apply to the three existing CCUs (APBC, APMU,
and MPMU), so that the new reset-only CCUs can go without specifying them.
Define the index values for resets associated with all SpacemiT K1
syscon nodes, including those with clocks already defined, as well as
the new ones (without clocks).
Signed-off-by: Alex Elder <elder@riscstar.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
.../soc/spacemit/spacemit,k1-syscon.yaml | 29 +++-
.../dt-bindings/clock/spacemit,k1-syscon.h | 128 ++++++++++++++++++
2 files changed, 150 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
index 30aaf49da03d3..133a391ee68cd 100644
--- a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
@@ -19,6 +19,9 @@ properties:
- spacemit,k1-syscon-apbc
- spacemit,k1-syscon-apmu
- spacemit,k1-syscon-mpmu
+ - spacemit,k1-syscon-rcpu
+ - spacemit,k1-syscon-rcpu2
+ - spacemit,k1-syscon-apbc2
reg:
maxItems: 1
@@ -47,9 +50,6 @@ properties:
required:
- compatible
- reg
- - clocks
- - clock-names
- - "#clock-cells"
- "#reset-cells"
allOf:
@@ -57,13 +57,28 @@ allOf:
properties:
compatible:
contains:
- const: spacemit,k1-syscon-apbc
+ enum:
+ - spacemit,k1-syscon-apmu
+ - spacemit,k1-syscon-mpmu
then:
- properties:
- "#power-domain-cells": false
- else:
required:
- "#power-domain-cells"
+ else:
+ properties:
+ "#power-domain-cells": false
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - spacemit,k1-syscon-apbc
+ - spacemit,k1-syscon-apmu
+ - spacemit,k1-syscon-mpmu
+ then:
+ required:
+ - clocks
+ - clock-names
+ - "#clock-cells"
additionalProperties: false
diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h
index 35968ae982466..f5965dda3b905 100644
--- a/include/dt-bindings/clock/spacemit,k1-syscon.h
+++ b/include/dt-bindings/clock/spacemit,k1-syscon.h
@@ -78,6 +78,9 @@
#define CLK_APB 31
#define CLK_WDT_BUS 32
+/* MPMU resets */
+#define RESET_WDT 0
+
/* APBC clocks */
#define CLK_UART0 0
#define CLK_UART2 1
@@ -180,6 +183,59 @@
#define CLK_TSEN_BUS 98
#define CLK_IPC_AP2AUD_BUS 99
+/* APBC resets */
+#define RESET_UART0 0
+#define RESET_UART2 1
+#define RESET_UART3 2
+#define RESET_UART4 3
+#define RESET_UART5 4
+#define RESET_UART6 5
+#define RESET_UART7 6
+#define RESET_UART8 7
+#define RESET_UART9 8
+#define RESET_GPIO 9
+#define RESET_PWM0 10
+#define RESET_PWM1 11
+#define RESET_PWM2 12
+#define RESET_PWM3 13
+#define RESET_PWM4 14
+#define RESET_PWM5 15
+#define RESET_PWM6 16
+#define RESET_PWM7 17
+#define RESET_PWM8 18
+#define RESET_PWM9 19
+#define RESET_PWM10 20
+#define RESET_PWM11 21
+#define RESET_PWM12 22
+#define RESET_PWM13 23
+#define RESET_PWM14 24
+#define RESET_PWM15 25
+#define RESET_PWM16 26
+#define RESET_PWM17 27
+#define RESET_PWM18 28
+#define RESET_PWM19 29
+#define RESET_SSP3 30
+#define RESET_RTC 31
+#define RESET_TWSI0 32
+#define RESET_TWSI1 33
+#define RESET_TWSI2 34
+#define RESET_TWSI4 35
+#define RESET_TWSI5 36
+#define RESET_TWSI6 37
+#define RESET_TWSI7 38
+#define RESET_TWSI8 39
+#define RESET_TIMERS1 40
+#define RESET_TIMERS2 41
+#define RESET_AIB 42
+#define RESET_ONEWIRE 43
+#define RESET_SSPA0 44
+#define RESET_SSPA1 45
+#define RESET_DRO 46
+#define RESET_IR 47
+#define RESET_TSEN 48
+#define RESET_IPC_AP2AUD 49
+#define RESET_CAN0 50
+
/* APMU clocks */
#define CLK_CCI550 0
#define CLK_CPU_C0_HI 1
@@ -244,4 +300,76 @@
#define CLK_V2D 60
#define CLK_EMMC_BUS 61
+/* APMU resets */
+#define RESET_CCIC_4X 0
+#define RESET_CCIC1_PHY 1
+#define RESET_SDH_AXI 2
+#define RESET_SDH0 3
+#define RESET_SDH1 4
+#define RESET_SDH2 5
+#define RESET_USBP1_AXI 6
+#define RESET_USB_AXI 7
+#define RESET_USB3_0 8
+#define RESET_QSPI 9
+#define RESET_QSPI_BUS 10
+#define RESET_DMA 11
+#define RESET_AES 12
+#define RESET_VPU 13
+#define RESET_GPU 14
+#define RESET_EMMC 15
+#define RESET_EMMC_X 16
+#define RESET_AUDIO 17
+#define RESET_HDMI 18
+#define RESET_PCIE0 19
+#define RESET_PCIE1 20
+#define RESET_PCIE2 21
+#define RESET_EMAC0 22
+#define RESET_EMAC1 23
+#define RESET_JPG 24
+#define RESET_CCIC2PHY 25
+#define RESET_CCIC3PHY 26
+#define RESET_CSI 27
+#define RESET_ISP_CPP 28
+#define RESET_ISP_BUS 29
+#define RESET_ISP 30
+#define RESET_ISP_CI 31
+#define RESET_DPU_MCLK 32
+#define RESET_DPU_ESC 33
+#define RESET_DPU_HCLK 34
+#define RESET_DPU_SPIBUS 35
+#define RESET_DPU_SPI_HBUS 36
+#define RESET_V2D 37
+#define RESET_MIPI 38
+#define RESET_MC 39
+
+/* RCPU resets */
+#define RESET_RCPU_SSP0 0
+#define RESET_RCPU_I2C0 1
+#define RESET_RCPU_UART1 2
+#define RESET_RCPU_IR 3
+#define RESET_RCPU_CAN 4
+#define RESET_RCPU_UART0 5
+#define RESET_RCPU_HDMI_AUDIO 6
+
+/* RCPU2 resets */
+#define RESET_RCPU2_PWM0 0
+#define RESET_RCPU2_PWM1 1
+#define RESET_RCPU2_PWM2 2
+#define RESET_RCPU2_PWM3 3
+#define RESET_RCPU2_PWM4 4
+#define RESET_RCPU2_PWM5 5
+#define RESET_RCPU2_PWM6 6
+#define RESET_RCPU2_PWM7 7
+#define RESET_RCPU2_PWM8 8
+#define RESET_RCPU2_PWM9 9
+
+/* APBC2 resets */
+#define RESET_APBC2_UART1 0
+#define RESET_APBC2_SSP2 1
+#define RESET_APBC2_TWSI3 2
+#define RESET_APBC2_RTC 3
+#define RESET_APBC2_TIMERS0 4
+#define RESET_APBC2_KPC 5
+#define RESET_APBC2_GPIO 6
+
#endif /* _DT_BINDINGS_SPACEMIT_CCU_H_ */
--
2.45.2hi Alex, On 16:06 Tue 06 May , Alex Elder wrote: > There are additional SpacemiT syscon CCUs whose registers control both > clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined > previously, these will (initially) support only resets. They do not > incorporate power domain functionality. > > Previously the clock properties were required for all compatible nodes. > Make that requirement only apply to the three existing CCUs (APBC, APMU, > and MPMU), so that the new reset-only CCUs can go without specifying them. > > Define the index values for resets associated with all SpacemiT K1 > syscon nodes, including those with clocks already defined, as well as > the new ones (without clocks). > > Signed-off-by: Alex Elder <elder@riscstar.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../soc/spacemit/spacemit,k1-syscon.yaml | 29 +++- > .../dt-bindings/clock/spacemit,k1-syscon.h | 128 ++++++++++++++++++ > 2 files changed, 150 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > index 30aaf49da03d3..133a391ee68cd 100644 > --- a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > @@ -19,6 +19,9 @@ properties: > - spacemit,k1-syscon-apbc > - spacemit,k1-syscon-apmu > - spacemit,k1-syscon-mpmu > + - spacemit,k1-syscon-rcpu > + - spacemit,k1-syscon-rcpu2 > + - spacemit,k1-syscon-apbc2 > > reg: > maxItems: 1 > @@ -47,9 +50,6 @@ properties: > required: > - compatible > - reg > - - clocks > - - clock-names > - - "#clock-cells" > - "#reset-cells" > > allOf: > @@ -57,13 +57,28 @@ allOf: > properties: > compatible: > contains: > - const: spacemit,k1-syscon-apbc > + enum: > + - spacemit,k1-syscon-apmu > + - spacemit,k1-syscon-mpmu > then: > - properties: > - "#power-domain-cells": false > - else: > required: > - "#power-domain-cells" > + else: > + properties: > + "#power-domain-cells": false > + - if: > + properties: > + compatible: > + contains: > + enum: > + - spacemit,k1-syscon-apbc > + - spacemit,k1-syscon-apmu > + - spacemit,k1-syscon-mpmu > + then: > + required: > + - clocks > + - clock-names > + - "#clock-cells" > > additionalProperties: false > > diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h > index 35968ae982466..f5965dda3b905 100644 > --- a/include/dt-bindings/clock/spacemit,k1-syscon.h > +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h would it be better to move all reset definition to its dedicated dir? which like: include/dt-bindings/reset/spacemit,k1-syscon.h? > @@ -78,6 +78,9 @@ > #define CLK_APB 31 > #define CLK_WDT_BUS 32 > > +/* MPMU resets */ > +#define RESET_WDT 0 > + > /* APBC clocks */ > #define CLK_UART0 0 > #define CLK_UART2 1 > @@ -180,6 +183,59 @@ > #define CLK_TSEN_BUS 98 > #define CLK_IPC_AP2AUD_BUS 99 > > +/* APBC resets */ > +#define RESET_UART0 0 > +#define RESET_UART2 1 > +#define RESET_UART3 2 > +#define RESET_UART4 3 > +#define RESET_UART5 4 > +#define RESET_UART6 5 > +#define RESET_UART7 6 > +#define RESET_UART8 7 > +#define RESET_UART9 8 > +#define RESET_GPIO 9 > +#define RESET_PWM0 10 > +#define RESET_PWM1 11 > +#define RESET_PWM2 12 > +#define RESET_PWM3 13 > +#define RESET_PWM4 14 > +#define RESET_PWM5 15 > +#define RESET_PWM6 16 > +#define RESET_PWM7 17 > +#define RESET_PWM8 18 > +#define RESET_PWM9 19 > +#define RESET_PWM10 20 > +#define RESET_PWM11 21 > +#define RESET_PWM12 22 > +#define RESET_PWM13 23 > +#define RESET_PWM14 24 > +#define RESET_PWM15 25 > +#define RESET_PWM16 26 > +#define RESET_PWM17 27 > +#define RESET_PWM18 28 > +#define RESET_PWM19 29 > +#define RESET_SSP3 30 > +#define RESET_RTC 31 > +#define RESET_TWSI0 32 > +#define RESET_TWSI1 33 > +#define RESET_TWSI2 34 > +#define RESET_TWSI4 35 > +#define RESET_TWSI5 36 > +#define RESET_TWSI6 37 > +#define RESET_TWSI7 38 > +#define RESET_TWSI8 39 > +#define RESET_TIMERS1 40 > +#define RESET_TIMERS2 41 > +#define RESET_AIB 42 > +#define RESET_ONEWIRE 43 > +#define RESET_SSPA0 44 > +#define RESET_SSPA1 45 > +#define RESET_DRO 46 > +#define RESET_IR 47 > +#define RESET_TSEN 48 > +#define RESET_IPC_AP2AUD 49 > +#define RESET_CAN0 50 > + > /* APMU clocks */ > #define CLK_CCI550 0 > #define CLK_CPU_C0_HI 1 > @@ -244,4 +300,76 @@ > #define CLK_V2D 60 > #define CLK_EMMC_BUS 61 > > +/* APMU resets */ > +#define RESET_CCIC_4X 0 > +#define RESET_CCIC1_PHY 1 > +#define RESET_SDH_AXI 2 > +#define RESET_SDH0 3 > +#define RESET_SDH1 4 > +#define RESET_SDH2 5 > +#define RESET_USBP1_AXI 6 > +#define RESET_USB_AXI 7 > +#define RESET_USB3_0 8 > +#define RESET_QSPI 9 > +#define RESET_QSPI_BUS 10 > +#define RESET_DMA 11 > +#define RESET_AES 12 > +#define RESET_VPU 13 > +#define RESET_GPU 14 > +#define RESET_EMMC 15 > +#define RESET_EMMC_X 16 > +#define RESET_AUDIO 17 > +#define RESET_HDMI 18 > +#define RESET_PCIE0 19 > +#define RESET_PCIE1 20 > +#define RESET_PCIE2 21 > +#define RESET_EMAC0 22 > +#define RESET_EMAC1 23 > +#define RESET_JPG 24 > +#define RESET_CCIC2PHY 25 > +#define RESET_CCIC3PHY 26 > +#define RESET_CSI 27 > +#define RESET_ISP_CPP 28 > +#define RESET_ISP_BUS 29 > +#define RESET_ISP 30 > +#define RESET_ISP_CI 31 > +#define RESET_DPU_MCLK 32 > +#define RESET_DPU_ESC 33 > +#define RESET_DPU_HCLK 34 > +#define RESET_DPU_SPIBUS 35 > +#define RESET_DPU_SPI_HBUS 36 > +#define RESET_V2D 37 > +#define RESET_MIPI 38 > +#define RESET_MC 39 > + > +/* RCPU resets */ > +#define RESET_RCPU_SSP0 0 > +#define RESET_RCPU_I2C0 1 > +#define RESET_RCPU_UART1 2 > +#define RESET_RCPU_IR 3 > +#define RESET_RCPU_CAN 4 > +#define RESET_RCPU_UART0 5 > +#define RESET_RCPU_HDMI_AUDIO 6 > + > +/* RCPU2 resets */ > +#define RESET_RCPU2_PWM0 0 > +#define RESET_RCPU2_PWM1 1 > +#define RESET_RCPU2_PWM2 2 > +#define RESET_RCPU2_PWM3 3 > +#define RESET_RCPU2_PWM4 4 > +#define RESET_RCPU2_PWM5 5 > +#define RESET_RCPU2_PWM6 6 > +#define RESET_RCPU2_PWM7 7 > +#define RESET_RCPU2_PWM8 8 > +#define RESET_RCPU2_PWM9 9 > + > +/* APBC2 resets */ > +#define RESET_APBC2_UART1 0 > +#define RESET_APBC2_SSP2 1 > +#define RESET_APBC2_TWSI3 2 > +#define RESET_APBC2_RTC 3 > +#define RESET_APBC2_TIMERS0 4 > +#define RESET_APBC2_KPC 5 > +#define RESET_APBC2_GPIO 6 > + > #endif /* _DT_BINDINGS_SPACEMIT_CCU_H_ */ > -- > 2.45.2 > -- Yixun Lan (dlan)
On 08/05/2025 00:35, Yixun Lan wrote: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - spacemit,k1-syscon-apbc >> + - spacemit,k1-syscon-apmu >> + - spacemit,k1-syscon-mpmu >> + then: >> + required: >> + - clocks >> + - clock-names >> + - "#clock-cells" >> >> additionalProperties: false >> >> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h >> index 35968ae982466..f5965dda3b905 100644 >> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h >> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h > would it be better to move all reset definition to its dedicated dir? > which like: include/dt-bindings/reset/spacemit,k1-syscon.h? Please kindly trim the replies from unnecessary context. It makes it much easier to find new content. I don't get why such comments are appearing so late - at v6. There was nothing from you about this in v1, v2 and v3, which finally got reviewed. I just feel people wait for maintainers to review and only after they will add their 2 cents of nitpicks or even some more important things potentially invalidating the review. Lesson for me: do not review people's work before it reaches v10, right? Best regards, Krzysztof
On 5/8/25 7:02 AM, Krzysztof Kozlowski wrote: > On 08/05/2025 00:35, Yixun Lan wrote: >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - spacemit,k1-syscon-apbc >>> + - spacemit,k1-syscon-apmu >>> + - spacemit,k1-syscon-mpmu >>> + then: >>> + required: >>> + - clocks >>> + - clock-names >>> + - "#clock-cells" >>> >>> additionalProperties: false >>> >>> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h >>> index 35968ae982466..f5965dda3b905 100644 >>> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h >>> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h >> would it be better to move all reset definition to its dedicated dir? >> which like: include/dt-bindings/reset/spacemit,k1-syscon.h? > > Please kindly trim the replies from unnecessary context. It makes it > much easier to find new content. > > > I don't get why such comments are appearing so late - at v6. There was > nothing from you about this in v1, v2 and v3, which finally got reviewed. Stephen Boyd said "please rework this to use the auxiliary driver framework" on version 5 of the series; it was otherwise "done" at that point. Doing this meant there was a much clearer separation of the clock definitions from the reset definitions. And Yixun's suggestion came from viewing things in that context. Given the rework, I considered sending this as v1 of a new series but did not. > I just feel people wait for maintainers to review and only after they > will add their 2 cents of nitpicks or even some more important things > potentially invalidating the review. Lesson for me: do not review > people's work before it reaches v10, right? That's not what happened here--or at least, it's not as simple as that. Your quick review was very much appreciated. Yixun: Krzysztof was satisfied with things the way they're defined here. Do you feel strongly I should make your suggested change? Or are you OK with me just keeping things defined this way for the next version? I'd like this question resolved before I send the next version. Thank you. -Alex > Best regards, > Krzysztof
Hi Alex, On 07:17 Thu 08 May , Alex Elder wrote: > On 5/8/25 7:02 AM, Krzysztof Kozlowski wrote: > > On 08/05/2025 00:35, Yixun Lan wrote: > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + enum: > >>> + - spacemit,k1-syscon-apbc > >>> + - spacemit,k1-syscon-apmu > >>> + - spacemit,k1-syscon-mpmu > >>> + then: > >>> + required: > >>> + - clocks > >>> + - clock-names > >>> + - "#clock-cells" > >>> > >>> additionalProperties: false > >>> > >>> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h > >>> index 35968ae982466..f5965dda3b905 100644 > >>> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h > >>> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h > >> would it be better to move all reset definition to its dedicated dir? > >> which like: include/dt-bindings/reset/spacemit,k1-syscon.h? > > > > Please kindly trim the replies from unnecessary context. It makes it > > much easier to find new content. > > > > > > I don't get why such comments are appearing so late - at v6. There was > > nothing from you about this in v1, v2 and v3, which finally got reviewed. > > Stephen Boyd said "please rework this to use the auxiliary driver > framework" on version 5 of the series; it was otherwise "done" at > that point. > > Doing this meant there was a much clearer separation of the clock > definitions from the reset definitions. And Yixun's suggestion > came from viewing things in that context. > > Given the rework, I considered sending this as v1 of a new series > but did not. > > > I just feel people wait for maintainers to review and only after they > > will add their 2 cents of nitpicks or even some more important things > > potentially invalidating the review. Lesson for me: do not review > > people's work before it reaches v10, right? > > That's not what happened here--or at least, it's not as simple > as that. Your quick review was very much appreciated. > > Yixun: Krzysztof was satisfied with things the way they're > defined here. Do you feel strongly I should make your suggested > change? Or are you OK with me just keeping things defined this > way for the next version? I'd like this question resolved before > I send the next version. > I was fine with squashing all definitions in one file for old version, but now, a new reset driver is introduced, I think it is deemed an independent header file? all newly added macros are related to reset. -- Yixun Lan (dlan)
On 08/05/2025 14:17, Alex Elder wrote: > On 5/8/25 7:02 AM, Krzysztof Kozlowski wrote: >> On 08/05/2025 00:35, Yixun Lan wrote: >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - spacemit,k1-syscon-apbc >>>> + - spacemit,k1-syscon-apmu >>>> + - spacemit,k1-syscon-mpmu >>>> + then: >>>> + required: >>>> + - clocks >>>> + - clock-names >>>> + - "#clock-cells" >>>> >>>> additionalProperties: false >>>> >>>> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h >>>> index 35968ae982466..f5965dda3b905 100644 >>>> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h >>>> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h >>> would it be better to move all reset definition to its dedicated dir? >>> which like: include/dt-bindings/reset/spacemit,k1-syscon.h? >> >> Please kindly trim the replies from unnecessary context. It makes it >> much easier to find new content. >> >> >> I don't get why such comments are appearing so late - at v6. There was >> nothing from you about this in v1, v2 and v3, which finally got reviewed. > > Stephen Boyd said "please rework this to use the auxiliary driver > framework" on version 5 of the series; it was otherwise "done" at > that point. Stephen is a subsystem maintainer so his comments are fine or acceptable to be late. > > Doing this meant there was a much clearer separation of the clock > definitions from the reset definitions. And Yixun's suggestion > came from viewing things in that context. Weren't they applicable to v1 as well? How bindings could change with change to auxiliary bus/driver? > > Given the rework, I considered sending this as v1 of a new series > but did not. Sorry but no. Bindings headers at v1 are exactly the same or almost the same as now: https://lore.kernel.org/lkml/20250321151831.623575-2-elder@riscstar.com/ so this idea could have been given at v1, v2, v3, v4, v5 (that would be late).... but at some point this is just unnecessary late nitpicking. So what then? Imagine that you prepare v7 and some other person gives you different comment about bindings or bindings headers. Then you prepare v8. And then someone comes with one more, different comment, because that person did not bother to review between v1-v8 (that imaginary future v8), so you need to prepare v9. I don't think this process is correct. Best regards, Krzysztof
On 5/8/25 7:36 AM, Krzysztof Kozlowski wrote: > On 08/05/2025 14:17, Alex Elder wrote: >> On 5/8/25 7:02 AM, Krzysztof Kozlowski wrote: >>> On 08/05/2025 00:35, Yixun Lan wrote: >>>>> + - if: >>>>> + properties: >>>>> + compatible: >>>>> + contains: >>>>> + enum: >>>>> + - spacemit,k1-syscon-apbc >>>>> + - spacemit,k1-syscon-apmu >>>>> + - spacemit,k1-syscon-mpmu >>>>> + then: >>>>> + required: >>>>> + - clocks >>>>> + - clock-names >>>>> + - "#clock-cells" >>>>> >>>>> additionalProperties: false >>>>> >>>>> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h >>>>> index 35968ae982466..f5965dda3b905 100644 >>>>> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h >>>>> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h >>>> would it be better to move all reset definition to its dedicated dir? >>>> which like: include/dt-bindings/reset/spacemit,k1-syscon.h? >>> >>> Please kindly trim the replies from unnecessary context. It makes it >>> much easier to find new content. >>> >>> >>> I don't get why such comments are appearing so late - at v6. There was >>> nothing from you about this in v1, v2 and v3, which finally got reviewed. >> >> Stephen Boyd said "please rework this to use the auxiliary driver >> framework" on version 5 of the series; it was otherwise "done" at >> that point. > > Stephen is a subsystem maintainer so his comments are fine or acceptable > to be late. > >> >> Doing this meant there was a much clearer separation of the clock >> definitions from the reset definitions. And Yixun's suggestion >> came from viewing things in that context. > > Weren't they applicable to v1 as well? How bindings could change with > change to auxiliary bus/driver? > >> >> Given the rework, I considered sending this as v1 of a new series >> but did not. > > Sorry but no. Bindings headers at v1 are exactly the same or almost the > same as now: > > https://lore.kernel.org/lkml/20250321151831.623575-2-elder@riscstar.com/ > > so this idea could have been given at v1, v2, v3, v4, v5 (that would be > late).... but at some point this is just unnecessary late nitpicking. > > So what then? Imagine that you prepare v7 and some other person gives > you different comment about bindings or bindings headers. Then you > prepare v8. And then someone comes with one more, different comment, > because that person did not bother to review between v1-v8 (that > imaginary future v8), so you need to prepare v9. > > I don't think this process is correct. We'll stick with the original binding definition. -Alex > > Best regards, > Krzysztof
On 5/7/25 5:35 PM, Yixun Lan wrote: > hi Alex, > > On 16:06 Tue 06 May , Alex Elder wrote: >> There are additional SpacemiT syscon CCUs whose registers control both >> clocks and resets: RCPU, RCPU2, and APBC2. Unlike those defined >> previously, these will (initially) support only resets. They do not >> incorporate power domain functionality. >> >> Previously the clock properties were required for all compatible nodes. >> Make that requirement only apply to the three existing CCUs (APBC, APMU, >> and MPMU), so that the new reset-only CCUs can go without specifying them. >> >> Define the index values for resets associated with all SpacemiT K1 >> syscon nodes, including those with clocks already defined, as well as >> the new ones (without clocks). >> >> Signed-off-by: Alex Elder <elder@riscstar.com> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> .../soc/spacemit/spacemit,k1-syscon.yaml | 29 +++- >> .../dt-bindings/clock/spacemit,k1-syscon.h | 128 ++++++++++++++++++ >> 2 files changed, 150 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >> index 30aaf49da03d3..133a391ee68cd 100644 >> --- a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >> @@ -19,6 +19,9 @@ properties: >> - spacemit,k1-syscon-apbc >> - spacemit,k1-syscon-apmu >> - spacemit,k1-syscon-mpmu >> + - spacemit,k1-syscon-rcpu >> + - spacemit,k1-syscon-rcpu2 >> + - spacemit,k1-syscon-apbc2 >> >> reg: >> maxItems: 1 >> @@ -47,9 +50,6 @@ properties: >> required: >> - compatible >> - reg >> - - clocks >> - - clock-names >> - - "#clock-cells" >> - "#reset-cells" >> >> allOf: >> @@ -57,13 +57,28 @@ allOf: >> properties: >> compatible: >> contains: >> - const: spacemit,k1-syscon-apbc >> + enum: >> + - spacemit,k1-syscon-apmu >> + - spacemit,k1-syscon-mpmu >> then: >> - properties: >> - "#power-domain-cells": false >> - else: >> required: >> - "#power-domain-cells" >> + else: >> + properties: >> + "#power-domain-cells": false >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - spacemit,k1-syscon-apbc >> + - spacemit,k1-syscon-apmu >> + - spacemit,k1-syscon-mpmu >> + then: >> + required: >> + - clocks >> + - clock-names >> + - "#clock-cells" >> >> additionalProperties: false >> >> diff --git a/include/dt-bindings/clock/spacemit,k1-syscon.h b/include/dt-bindings/clock/spacemit,k1-syscon.h >> index 35968ae982466..f5965dda3b905 100644 >> --- a/include/dt-bindings/clock/spacemit,k1-syscon.h >> +++ b/include/dt-bindings/clock/spacemit,k1-syscon.h > would it be better to move all reset definition to its dedicated dir? > which like: include/dt-bindings/reset/spacemit,k1-syscon.h? That's fine with me. I should have thought of that. Krzysztof, I'll drop your Reviewed-by if I make that change, but if you say I can before I post v7 I will keep it. I'll wait a bit more before I update so others can comment (on this or anything else). Thanks. -Alex > >> @@ -78,6 +78,9 @@ >> #define CLK_APB 31 >> #define CLK_WDT_BUS 32 >> >> +/* MPMU resets */ >> +#define RESET_WDT 0 >> + >> /* APBC clocks */ >> #define CLK_UART0 0 >> #define CLK_UART2 1 >> @@ -180,6 +183,59 @@ >> #define CLK_TSEN_BUS 98 >> #define CLK_IPC_AP2AUD_BUS 99 >> >> +/* APBC resets */ >> +#define RESET_UART0 0 >> +#define RESET_UART2 1 >> +#define RESET_UART3 2 >> +#define RESET_UART4 3 >> +#define RESET_UART5 4 >> +#define RESET_UART6 5 >> +#define RESET_UART7 6 >> +#define RESET_UART8 7 >> +#define RESET_UART9 8 >> +#define RESET_GPIO 9 >> +#define RESET_PWM0 10 >> +#define RESET_PWM1 11 >> +#define RESET_PWM2 12 >> +#define RESET_PWM3 13 >> +#define RESET_PWM4 14 >> +#define RESET_PWM5 15 >> +#define RESET_PWM6 16 >> +#define RESET_PWM7 17 >> +#define RESET_PWM8 18 >> +#define RESET_PWM9 19 >> +#define RESET_PWM10 20 >> +#define RESET_PWM11 21 >> +#define RESET_PWM12 22 >> +#define RESET_PWM13 23 >> +#define RESET_PWM14 24 >> +#define RESET_PWM15 25 >> +#define RESET_PWM16 26 >> +#define RESET_PWM17 27 >> +#define RESET_PWM18 28 >> +#define RESET_PWM19 29 >> +#define RESET_SSP3 30 >> +#define RESET_RTC 31 >> +#define RESET_TWSI0 32 >> +#define RESET_TWSI1 33 >> +#define RESET_TWSI2 34 >> +#define RESET_TWSI4 35 >> +#define RESET_TWSI5 36 >> +#define RESET_TWSI6 37 >> +#define RESET_TWSI7 38 >> +#define RESET_TWSI8 39 >> +#define RESET_TIMERS1 40 >> +#define RESET_TIMERS2 41 >> +#define RESET_AIB 42 >> +#define RESET_ONEWIRE 43 >> +#define RESET_SSPA0 44 >> +#define RESET_SSPA1 45 >> +#define RESET_DRO 46 >> +#define RESET_IR 47 >> +#define RESET_TSEN 48 >> +#define RESET_IPC_AP2AUD 49 >> +#define RESET_CAN0 50 >> + >> /* APMU clocks */ >> #define CLK_CCI550 0 >> #define CLK_CPU_C0_HI 1 >> @@ -244,4 +300,76 @@ >> #define CLK_V2D 60 >> #define CLK_EMMC_BUS 61 >> >> +/* APMU resets */ >> +#define RESET_CCIC_4X 0 >> +#define RESET_CCIC1_PHY 1 >> +#define RESET_SDH_AXI 2 >> +#define RESET_SDH0 3 >> +#define RESET_SDH1 4 >> +#define RESET_SDH2 5 >> +#define RESET_USBP1_AXI 6 >> +#define RESET_USB_AXI 7 >> +#define RESET_USB3_0 8 >> +#define RESET_QSPI 9 >> +#define RESET_QSPI_BUS 10 >> +#define RESET_DMA 11 >> +#define RESET_AES 12 >> +#define RESET_VPU 13 >> +#define RESET_GPU 14 >> +#define RESET_EMMC 15 >> +#define RESET_EMMC_X 16 >> +#define RESET_AUDIO 17 >> +#define RESET_HDMI 18 >> +#define RESET_PCIE0 19 >> +#define RESET_PCIE1 20 >> +#define RESET_PCIE2 21 >> +#define RESET_EMAC0 22 >> +#define RESET_EMAC1 23 >> +#define RESET_JPG 24 >> +#define RESET_CCIC2PHY 25 >> +#define RESET_CCIC3PHY 26 >> +#define RESET_CSI 27 >> +#define RESET_ISP_CPP 28 >> +#define RESET_ISP_BUS 29 >> +#define RESET_ISP 30 >> +#define RESET_ISP_CI 31 >> +#define RESET_DPU_MCLK 32 >> +#define RESET_DPU_ESC 33 >> +#define RESET_DPU_HCLK 34 >> +#define RESET_DPU_SPIBUS 35 >> +#define RESET_DPU_SPI_HBUS 36 >> +#define RESET_V2D 37 >> +#define RESET_MIPI 38 >> +#define RESET_MC 39 >> + >> +/* RCPU resets */ >> +#define RESET_RCPU_SSP0 0 >> +#define RESET_RCPU_I2C0 1 >> +#define RESET_RCPU_UART1 2 >> +#define RESET_RCPU_IR 3 >> +#define RESET_RCPU_CAN 4 >> +#define RESET_RCPU_UART0 5 >> +#define RESET_RCPU_HDMI_AUDIO 6 >> + >> +/* RCPU2 resets */ >> +#define RESET_RCPU2_PWM0 0 >> +#define RESET_RCPU2_PWM1 1 >> +#define RESET_RCPU2_PWM2 2 >> +#define RESET_RCPU2_PWM3 3 >> +#define RESET_RCPU2_PWM4 4 >> +#define RESET_RCPU2_PWM5 5 >> +#define RESET_RCPU2_PWM6 6 >> +#define RESET_RCPU2_PWM7 7 >> +#define RESET_RCPU2_PWM8 8 >> +#define RESET_RCPU2_PWM9 9 >> + >> +/* APBC2 resets */ >> +#define RESET_APBC2_UART1 0 >> +#define RESET_APBC2_SSP2 1 >> +#define RESET_APBC2_TWSI3 2 >> +#define RESET_APBC2_RTC 3 >> +#define RESET_APBC2_TIMERS0 4 >> +#define RESET_APBC2_KPC 5 >> +#define RESET_APBC2_GPIO 6 >> + >> #endif /* _DT_BINDINGS_SPACEMIT_CCU_H_ */ >> -- >> 2.45.2 >> >
© 2016 - 2025 Red Hat, Inc.