Message-ID: <976a2029-c0c0-4093-a3cd-71e1524db032@kernel.org>
Date: Tue, 25 Feb 2025 09:12:40 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Haylen Chu <heylenay@....org>, Alex Elder <elder@...cstar.com>
Cc: Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Haylen Chu <heylenay@...look.com>,
 Yixun Lan <dlan@...too.org>, linux-riscv@...ts.infradead.org,
 linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Inochi Amaoto <inochiama@...look.com>,
 Chen Wang <unicornxdotw@...mail.com>, Jisheng Zhang <jszhang@...nel.org>,
 Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>,
 Guodong Xu <guodong@...cstar.com>
Subject: Re: [PATCH v4 2/4] dt-bindings: soc: spacemit: Add spacemit,k1-syscon

On 24/02/2025 11:17, Haylen Chu wrote:
> 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,

Ack

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

Ack

> 
>>>
>>> 	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, it looks good.



Best regards,
Krzysztof

Powered by blists - more mailing lists