diff mbox series

[v4,2/4] dt-bindings: soc: spacemit: Add spacemit,k1-syscon

Message ID 20250103215636.19967-4-heylenay@4d2.org (mailing list archive)
State New
Headers show
Series Add clock controller support for SpacemiT K1 | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Haylen Chu Jan. 3, 2025, 9:56 p.m. UTC
Add documentation to describe Spacemit K1 system controller registers.

Signed-off-by: Haylen Chu <heylenay@4d2.org>
---
 .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml

Comments

Krzysztof Kozlowski Jan. 4, 2025, 10:07 a.m. UTC | #1
On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
> Add documentation to describe Spacemit K1 system controller registers.
> 
> Signed-off-by: Haylen Chu <heylenay@4d2.org>
> ---
>  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> new file mode 100644
> index 000000000000..79c4a74ff30e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Spacemit K1 SoC System Controller
> +
> +maintainers:
> +  - Haylen Chu <heylenay@4d2.org>
> +
> +description:
> +  The Spacemit K1 SoC system controller provides access to shared register files
> +  for related SoC modules, such as clock controller and reset controller.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - spacemit,k1-apbc-syscon
> +          - spacemit,k1-apbs-syscon
> +          - spacemit,k1-apmu-syscon
> +          - spacemit,k1-mpmu-syscon
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
> +    type: object

So now we see the full picture and it leads to questions.

1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
correct combination?

2. Why having this split in the first place? Please confirm that clock
controller is really, really a separate device and its child in
datasheet. IOW, fake child for your Linux is a no-go. Fake child while
devices are independent is another no-go.

Actual answer for 1+2 above would be to fold the child into parent,
assuming clock controller split is fake in terms of datasheet.

If it is real device, then allOf:if:then: narrowing the compatibles of
child might not be worth the complexity.

3. Why using different naming, look:

spacemit,k1-XXXX-syscon
spacemit,k1-ccu-XXXX


Best regards,
Krzysztof
Haylen Chu Feb. 11, 2025, 5:15 a.m. UTC | #2
On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
> > Add documentation to describe Spacemit K1 system controller registers.
> > 
> > Signed-off-by: Haylen Chu <heylenay@4d2.org>
> > ---
> >  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > new file mode 100644
> > index 000000000000..79c4a74ff30e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Spacemit K1 SoC System Controller
> > +
> > +maintainers:
> > +  - Haylen Chu <heylenay@4d2.org>
> > +
> > +description:
> > +  The Spacemit K1 SoC system controller provides access to shared register files
> > +  for related SoC modules, such as clock controller and reset controller.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - spacemit,k1-apbc-syscon
> > +          - spacemit,k1-apbs-syscon
> > +          - spacemit,k1-apmu-syscon
> > +          - spacemit,k1-mpmu-syscon
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-controller:
> > +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
> > +    type: object
> 
> So now we see the full picture and it leads to questions.
> 
> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
> correct combination?
> 
> 2. Why having this split in the first place? Please confirm that clock
> controller is really, really a separate device and its child in
> datasheet. IOW, fake child for your Linux is a no-go. Fake child while
> devices are independent is another no-go.

These syscons are introduced because the clock controllers share
registers with reset controllers. Folding them into the parents results
in devicetree nodes act as both reset and clock controllers, like what
has been done for Rockchip SoCs. Such folding isn't practical for the
MPMU region either, since watchdog and other misc bits (e.g. PLL lock
status) locates in it.

If you're more comfortable with reset and clock controllers folded
together and eliminating most of these syscons, I'm willing to make the
change.

> Actual answer for 1+2 above would be to fold the child into parent,
> assuming clock controller split is fake in terms of datasheet.
> 
> If it is real device, then allOf:if:then: narrowing the compatibles of
> child might not be worth the complexity.
> 
> 3. Why using different naming, look:
> 
> spacemit,k1-XXXX-syscon
> spacemit,k1-ccu-XXXX

I didn't consider about consistency when naming them. Talked to Yixun,
I'll unify them as spacemit,k1-syscon-* and spacemit,k1-ccu-*, keeping
synchronized with other K1 peripherals.

> 
> Best regards,
> Krzysztof
> 

Thanks,
Haylen Chu
Krzysztof Kozlowski Feb. 11, 2025, 8:03 a.m. UTC | #3
On 11/02/2025 06:15, Haylen Chu wrote:
> On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote:
>> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
>>> Add documentation to describe Spacemit K1 system controller registers.
>>>
>>> Signed-off-by: Haylen Chu <heylenay@4d2.org>
>>> ---
>>>  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
>>>  1 file changed, 52 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>> new file mode 100644
>>> index 000000000000..79c4a74ff30e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Spacemit K1 SoC System Controller
>>> +
>>> +maintainers:
>>> +  - Haylen Chu <heylenay@4d2.org>
>>> +
>>> +description:
>>> +  The Spacemit K1 SoC system controller provides access to shared register files
>>> +  for related SoC modules, such as clock controller and reset controller.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - spacemit,k1-apbc-syscon
>>> +          - spacemit,k1-apbs-syscon
>>> +          - spacemit,k1-apmu-syscon
>>> +          - spacemit,k1-mpmu-syscon
>>> +      - const: syscon
>>> +      - const: simple-mfd
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-controller:
>>> +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
>>> +    type: object
>>
>> So now we see the full picture and it leads to questions.
>>
>> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
>> correct combination?
>>
>> 2. Why having this split in the first place? Please confirm that clock
>> controller is really, really a separate device and its child in
>> datasheet. IOW, fake child for your Linux is a no-go. Fake child while
>> devices are independent is another no-go.
> 
> These syscons are introduced because the clock controllers share
> registers with reset controllers. Folding them into the parents results

So a fake split...

> in devicetree nodes act as both reset and clock controllers, like what

Which is correct hardware representation, isn't it?

> has been done for Rockchip SoCs. Such folding isn't practical for the
> MPMU region either, since watchdog and other misc bits (e.g. PLL lock
> status) locates in it.

Hm? Why? You have a device which is reset and clock controller, so why
one device node is not practical? Other vendors do not have problem with
this.

> 
> If you're more comfortable with reset and clock controllers folded
> together and eliminating most of these syscons, I'm willing to make the
> change.

This is expected.



Best regards,
Krzysztof
Haylen Chu Feb. 13, 2025, 11:14 a.m. UTC | #4
On Tue, Feb 11, 2025 at 09:03:20AM +0100, Krzysztof Kozlowski wrote:
> On 11/02/2025 06:15, Haylen Chu wrote:
> > On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote:
> >> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
> >>> Add documentation to describe Spacemit K1 system controller registers.
> >>>
> >>> Signed-off-by: Haylen Chu <heylenay@4d2.org>
> >>> ---
> >>>  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
> >>>  1 file changed, 52 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> >>> new file mode 100644
> >>> index 000000000000..79c4a74ff30e
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> >>> @@ -0,0 +1,52 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Spacemit K1 SoC System Controller
> >>> +
> >>> +maintainers:
> >>> +  - Haylen Chu <heylenay@4d2.org>
> >>> +
> >>> +description:
> >>> +  The Spacemit K1 SoC system controller provides access to shared register files
> >>> +  for related SoC modules, such as clock controller and reset controller.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - spacemit,k1-apbc-syscon
> >>> +          - spacemit,k1-apbs-syscon
> >>> +          - spacemit,k1-apmu-syscon
> >>> +          - spacemit,k1-mpmu-syscon
> >>> +      - const: syscon
> >>> +      - const: simple-mfd
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clock-controller:
> >>> +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
> >>> +    type: object
> >>
> >> So now we see the full picture and it leads to questions.
> >>
> >> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
> >> correct combination?
> >>
> >> 2. Why having this split in the first place? Please confirm that clock
> >> controller is really, really a separate device and its child in
> >> datasheet. IOW, fake child for your Linux is a no-go. Fake child while
> >> devices are independent is another no-go.
> > 
> > These syscons are introduced because the clock controllers share
> > registers with reset controllers. Folding them into the parents results
> 
> So a fake split...
> 
> > in devicetree nodes act as both reset and clock controllers, like what
> 
> Which is correct hardware representation, isn't it?
> 
> > has been done for Rockchip SoCs. Such folding isn't practical for the
> > MPMU region either, since watchdog and other misc bits (e.g. PLL lock
> > status) locates in it.

I have to correct that the watchdog doesn't stay in the MPMU region, I
misremembered it.

> Hm? Why? You have a device which is reset and clock controller, so why
> one device node is not practical? Other vendors do not have problem with
> this.

Merging reset and clock controllers together is fine to me. What I want
to mention is that APMU and MPMU, abbreviated from Application/Main Power
Management Unit, contain not only clock/reset-related registers but also
power management ones[1]. Additionally, the PLL lock status bits locate
at MPMU, split from the PLL configuration registers as you've already
seen in the binding of spacemit,k1-ccu-apbs where I refer to it with a
phandle.

Since reset/clock and power management registers interleave in the MMIO
region, do you think syscons are acceptable in this situation or it
should be handled in another way? The reset and clock controllers could
still be folded together as they share the same registers. The device
tree will look like,

	syscon_mpmu: system-controller@d4050000 {
		compatible = "spacemit,mpmu-syscon", "syscon", "simple-mfd";
		reg = <0xd4050000 0x10000>;

		cru_mpmu: clock-controller {
			compatible = "spacemit,k1-cru-mpmu";
			#clock-cells = <1>;
			#reset-cells = <1>;
		};

		power_mpmu: power-controller {
			compatible = "spacemit,k1-powerdomain-mpmu";
			/* ... */
			#power-domain-cells = <0>;
		};
	};

For the other two clock controllers (APBS and APBC), syscons are really
unnecessary and it's simple to fold them.

> > 
> > If you're more comfortable with reset and clock controllers folded
> > together and eliminating most of these syscons, I'm willing to make the
> > change.
> 
> This is expected.

Thanks for the explanation.

> 
> 
> Best regards,
> Krzysztof

Best regards,
Haylen Chu

[1]: https://developer.spacemit.com/documentation?token=T7TnwVZz1iPBk1kKwAPc6lyKnNb#part958
Krzysztof Kozlowski Feb. 13, 2025, 6:07 p.m. UTC | #5
On 13/02/2025 12:14, Haylen Chu wrote:
> On Tue, Feb 11, 2025 at 09:03:20AM +0100, Krzysztof Kozlowski wrote:
>> On 11/02/2025 06:15, Haylen Chu wrote:
>>> On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote:
>>>> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
>>>>> Add documentation to describe Spacemit K1 system controller registers.
>>>>>
>>>>> Signed-off-by: Haylen Chu <heylenay@4d2.org>
>>>>> ---
>>>>>  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
>>>>>  1 file changed, 52 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..79c4a74ff30e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
>>>>> @@ -0,0 +1,52 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Spacemit K1 SoC System Controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Haylen Chu <heylenay@4d2.org>
>>>>> +
>>>>> +description:
>>>>> +  The Spacemit K1 SoC system controller provides access to shared register files
>>>>> +  for related SoC modules, such as clock controller and reset controller.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - spacemit,k1-apbc-syscon
>>>>> +          - spacemit,k1-apbs-syscon
>>>>> +          - spacemit,k1-apmu-syscon
>>>>> +          - spacemit,k1-mpmu-syscon
>>>>> +      - const: syscon
>>>>> +      - const: simple-mfd
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-controller:
>>>>> +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
>>>>> +    type: object
>>>>
>>>> So now we see the full picture and it leads to questions.
>>>>
>>>> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
>>>> correct combination?
>>>>
>>>> 2. Why having this split in the first place? Please confirm that clock
>>>> controller is really, really a separate device and its child in
>>>> datasheet. IOW, fake child for your Linux is a no-go. Fake child while
>>>> devices are independent is another no-go.
>>>
>>> These syscons are introduced because the clock controllers share
>>> registers with reset controllers. Folding them into the parents results
>>
>> So a fake split...
>>
>>> in devicetree nodes act as both reset and clock controllers, like what
>>
>> Which is correct hardware representation, isn't it?
>>
>>> has been done for Rockchip SoCs. Such folding isn't practical for the
>>> MPMU region either, since watchdog and other misc bits (e.g. PLL lock
>>> status) locates in it.
> 
> I have to correct that the watchdog doesn't stay in the MPMU region, I
> misremembered it.
> 
>> Hm? Why? You have a device which is reset and clock controller, so why
>> one device node is not practical? Other vendors do not have problem with
>> this.
> 
> Merging reset and clock controllers together is fine to me. What I want
> to mention is that APMU and MPMU, abbreviated from Application/Main Power
> Management Unit, contain not only clock/reset-related registers but also
> power management ones[1]. Additionally, the PLL lock status bits locate
> at MPMU, split from the PLL configuration registers as you've already
> seen in the binding of spacemit,k1-ccu-apbs where I refer to it with a
> phandle.

You need to define what is the device here. Don't create fake nodes just
for your drivers. If registers are interleaved and manual says "this is
block APMU/MPMU" then you have one device, so one node with 'reg'.

If subblocks are re-usable hardware (unlikely) or at least
separate/distinguishable, you could have children. If subblocks are
re-usable but addresses are interleaved, then children should not have
'reg'. If children do not have any resources as an effect, this is
strong indication these are not re-usable, separate subblocks.

> 
> Since reset/clock and power management registers interleave in the MMIO
> region, do you think syscons are acceptable in this situation or it
> should be handled in another way? The reset and clock controllers could
> still be folded together as they share the same registers. The device
> tree will look like,
> 
> 	syscon_mpmu: system-controller@d4050000 {
> 		compatible = "spacemit,mpmu-syscon", "syscon", "simple-mfd";
> 		reg = <0xd4050000 0x10000>;
> 
> 		cru_mpmu: clock-controller {
> 			compatible = "spacemit,k1-cru-mpmu";
> 			#clock-cells = <1>;
> 			#reset-cells = <1>;
> 		};
> 
> 		power_mpmu: power-controller {
> 			compatible = "spacemit,k1-powerdomain-mpmu";
> 			/* ... */
> 			#power-domain-cells = <0>;
> 		};

Based on above, I do not see any need for children device nodes. It's
fake split to match driver design.


> 	};
> 
> For the other two clock controllers (APBS and APBC), syscons are really
> unnecessary and it's simple to fold them.


I don't follow. Do we talk about children or syscon compatible?


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
new file mode 100644
index 000000000000..79c4a74ff30e
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Spacemit K1 SoC System Controller
+
+maintainers:
+  - Haylen Chu <heylenay@4d2.org>
+
+description:
+  The Spacemit K1 SoC system controller provides access to shared register files
+  for related SoC modules, such as clock controller and reset controller.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - spacemit,k1-apbc-syscon
+          - spacemit,k1-apbs-syscon
+          - spacemit,k1-apmu-syscon
+          - spacemit,k1-mpmu-syscon
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  clock-controller:
+    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
+    type: object
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    system-controller@d4050000 {
+        compatible = "spacemit,k1-mpmu-syscon", "syscon", "simple-mfd";
+        reg = <0xd4050000 0x209c>;
+
+        clock-controller {
+            compatible = "spacemit,k1-ccu-mpmu";
+            clocks = <&osc_32k>, <&vctcxo_1m>, <&vctcxo_3m>, <&vctcxo_24m>;
+            clock-names = "osc", "vctcxo_1m", "vctcxo_3m", "vctcxo_24m";
+            #clock-cells = <1>;
+        };
+    };