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

[Haylen Chu <heylenay@4d2.org>]

On Thu, Feb 13, 2025 at 07:07:55PM +0100, Krzysztof Kozlowski wrote:
> 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.

Okay, I'll make APMU/MPMU act as a whole device without split children
and drop bindings for the childern (spacemit,k1-ccu-mpmu) in the next
revision. Do I get the point?

> > 	};
> > 
> > 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?

APBS region contains only clock (PLL) bits and APBC region contains only
reset and clock bits, so I was thinking about dropping the syscon nodes
and changing their compatible to spacemit,k1-plls and
spacemit,k1-cru-apbc.

In summary, my plan is,

- For MPMU, APMU and APBC region, keep the binding in soc/spacemit.
  They'll be reset, clock and power controllers, with compatible
  "spacemit,k1-syscon-*".
- For APBS region, write a new binding clock/spacemit,k1-plls, as it
  contains only PLL-related bits. It acts as clock controller.
- All split children will be eliminated, there'll be only four device
  nodes, one for each region, matching the datasheet.
- Put all clock-related binding definition of SpacemiT K1 in
  dt-bindings/clock/spacemit,k1-ccu.h

Is it fine for you?

> 
> Best regards,
> Krzysztof

Thanks,
Haylen Chu