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

[Krzysztof Kozlowski <krzk@kernel.org>]

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?

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