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

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

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.

	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].

> 
> > access to all of the I/O address ranges.  And then within
> > the DT node for that device there is a sub-node for the
> 
> Uh, confusing. You said there is one device for all the clocks, so if
> there is one device so also one device node. No children.
> 
> Maybe you have more devices but none of you is explaining the hardware
> that way. Mixing talk about drivers is really not helping.
> 
>
> 
> 
> Best regards,
> Krzysztof

Best regards,
Haylen Chu

[1]: https://developer.spacemit.com/documentation?token=LzJyw97BCipK1dkUygrcbT0NnMg