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

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

On Sat, Feb 22, 2025 at 12:50:13PM +0100, Krzysztof Kozlowski wrote:
> On 22/02/2025 11:48, Haylen Chu wrote:
> > On Sat, Feb 22, 2025 at 10:59:09AM +0100, Krzysztof Kozlowski wrote:
> >> On 22/02/2025 00:40, Alex Elder wrote:
> >>> I have a general proposal on how to represent this, but I'd
> >>> like to know whether it makes sense.  It might be what Krzysztof
> >>> is suggesting, but in any case, I hope this representation would
> >>> work, because it could simplify the code, and compartmentalizes
> >>> things.
> >>>
> >>> Part of what motivates this is that I've been looking at the
> >>> downstream reset code this week.  It contains a large number of
> >>> register offset definitions identical to what's used for the
> >>> clock driver.  The reset driver uses exactly the same registers
> >>> as the clock driver does.  Downstream they are separate drivers,
> >>> but the clock driver exports a shared spinlock for both drivers
> >>> to use.
> >>>
> >>> These really need to be incorporated into the same driver for
> >>> upstream.
> >>
> >> Why? First, it is not related to the topic here at all. You can design
> >> drivers as you wish and still nothing to do with discussion about binding.
> >> Second, different subsystems justify different drivers and Linux handles
> >> this well already. No need for custom spinlock - regmap already does it.
> >>
> >>
> >>>
> >>> The clock code defines four distinct "units" (a term I'll use
> >>> from here on; there might be a better name):
> >>>    MPMU  Main Power Management Unit
> >>>    APMU  Application Power Management Unit
> >>>    APBC  APB Clock
> >>>    APBS  APB Spare
> >>>
> >>> The reset code defines some of those, but doesn't use APBS.
> >>> It also defines three more:
> >>>    APBC2 Another APB Clock
> >>>    RCPU  Real-time CPU?
> >>>    RCPU2 Another Real-time CPU
> >>>
> >>> Each of these "units" has a distinct I/O memory region that
> >>> contains registers that manage the clocks and reset signals.
> >>
> >> So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu
> >> 1+2? But previous statements were saying these are intermixed?
> >>
> >> " I'll make APMU/MPMU act as a whole device"
> > 
> > My reply seems somehow misleading. The statement means I will merge the
> > children with the syscon into one devicetree node, which applies for
> > both APMU and MPMU. I wasn't going to say that APMU and MPMU are
> > intermixed.
> > 
> > As Alex said, all these units have their own distinct and separate MMIO
> > regions.
> > 
> >>>
> >>> I suggest a single "k1-clocks" device be created, which has
> >>
> >> For four devices? Or for one device?
> > 
> > By Alex's example, I think he means a device node taking all these
> > distinct MMIO regions as resource.
> 
> 
> You still do not answer about the hardware: how many devices is there?

In my understanding, the series covers four devices, APBC, APMU, MPMU
and APBS, each comes with its separate MMIO region and is clearly
described in the datasheet. I stated this in the later part of the
reply,

> > For APBC, MPMU, APBS and APMU, I'm pretty
> > sure they're standalone blocks with distinct and separate MMIO regions,
> > this could be confirmed by the address mapping[1].

Thus I don't agree on Alex's solution, since it creates fake devices not
mentioned by the datasheet (spacemit,k1-clocks and all its children in
the example devicetree).

> > 
> > 	clock {
> > 		compatible = "spacemit,k1-clocks";
> > 
> > 		reg = <0x0 0xc0880000 0x0 0x2050>,
> > 		      <0x0 0xc0888000 0x0 0x30>,
> > 		      <0x0 0xd4015000 0x0 0x1000>,
> > 		      <0x0 0xd4050000 0x0 0x209c>,
> > 		      <0x0 0xd4090000 0x0 0x1000>,
> > 		      <0x0 0xd4282800 0x0 0x400>,
> > 		      <0x0 0xf0610000 0x0 0x20>;
> > 		reg-names = "rcpu",
> > 			    "rcpu2",
> > 			    "apbc",
> > 			    "mpmu",
> > 			    "apbs",
> > 			    "apmu",
> > 			    "apbc2";
> > 
> > 		/* ... */
> > 	};
> > 
> >> No, it's again going to wrong direction. I already said:
> >>
> >> "You need to define what is the device here. Don't create fake nodes ust
> >> 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'."
> >>
> >> So what is the device here? Can you people actually answer?
> >>
> > 
> > I'm not sure about the apbc2, rcpu and rcpu2 regions; they aren't
> > related to the thread, either. For APBC, MPMU, APBS and APMU, I'm pretty
> > sure they're standalone blocks with distinct and separate MMIO regions,
> > this could be confirmed by the address mapping[1].
> 
> They were brought here to discuss for some reason. Long discussions,
> long emails, unrelated topics like hardware or different devices - all
> this is not making it easier for me to understand.
>
> Best regards,
> Krzysztof

By the way, I made a summary on the hardware covered by this series in
one of my earlier reply[1]. Could you please comment further on my
proposal[2] according it, or pointing out anything that's unclear or
missing? It will be helpful for things to improve.

Thanks,
Haylen Chu

[1]: https://lore.kernel.org/all/Z7m2oNXbwJ06KtLQ@ketchup/
[2]: https://lore.kernel.org/all/Z7BTVu10EKHMqOnJ@ketchup/