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

[Krzysztof Kozlowski <krzk@kernel.org>]

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