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

Krzysztof Kozlowski
2025-02-13T19:14:00Z

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