linux-kernel - Re: [PATCH v6 5/6] reset: spacemit: define three more CCUs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65bc21e7-d936-492a-b095-4c1469932623@riscstar.com>
Date: Thu, 8 May 2025 07:29:46 -0500
From: Alex Elder <elder@...cstar.com>
To: Philipp Zabel <p.zabel@...gutronix.de>, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, mturquette@...libre.com,
 sboyd@...nel.org, paul.walmsley@...ive.com, palmer@...belt.com,
 aou@...s.berkeley.edu, alex@...ti.fr, dlan@...too.org
Cc: heylenay@....org, inochiama@...look.com, guodong@...cstar.com,
 devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 5/6] reset: spacemit: define three more CCUs

On 5/8/25 4:11 AM, Philipp Zabel wrote:
> On Di, 2025-05-06 at 16:06 -0500, Alex Elder wrote:
>> Three more CCUs on the SpacemiT K1 SoC implement only resets, not clocks.
>> Define these resets so they can be used.
>>
>> Signed-off-by: Alex Elder <elder@...cstar.com>
>> ---
>>   drivers/clk/spacemit/ccu-k1.c | 24 ++++++++++++++++
>>   drivers/reset/spacemit/k1.c   | 54 +++++++++++++++++++++++++++++++++++
>>   include/soc/spacemit/ccu_k1.h | 30 +++++++++++++++++++
>>   3 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
>> index 6b1845e899e5f..bddc83aff23cd 100644
>> --- a/drivers/clk/spacemit/ccu-k1.c
>> +++ b/drivers/clk/spacemit/ccu-k1.c
>> @@ -939,6 +939,18 @@ static const struct spacemit_ccu_data k1_ccu_apmu_data = {
>>   	.num		= ARRAY_SIZE(k1_ccu_apmu_hws),
>>   };
>>   
>> +static const struct spacemit_ccu_data k1_ccu_rcpu_data = {
> 
> The /* No clocks in the RCPU CCU */ comment belongs here, instead of in
> the reset driver.
> 
> I wonder though, if these units have no clocks, why are they called
> CCUs? It doesn't make much sense to me to add their compatibles to the
> ccu-k1 driver only to load the reset aux driver. Why not just add a
> platform driver next to the aux driver in reset-spacemit.ko for these
> three?

In fact, the particular registers involved *do* have fields
to control some clocks, but there is currently no use for
them, so support for them isn't implemented.  The reset
controls in those registers are needed now, however.

I actually first implemented this as a small, separate reset
driver, only to learn late in the process that these other
clocks might be needed someday.

> 
>> +	.reset_name	= "rcpu-reset",
>> +};
>> +
>> +static const struct spacemit_ccu_data k1_ccu_rcpu2_data = {
>> +	.reset_name	= "rcpu2-reset",
>> +};
>> +
>> +static const struct spacemit_ccu_data k1_ccu_apbc2_data = {
>> +	.reset_name	= "apbc2-reset",
>> +};
>> +
>>   static int spacemit_ccu_register(struct device *dev,
>>   				 struct regmap *regmap,
>>   				 struct regmap *lock_regmap,
>> @@ -1106,6 +1118,18 @@ static const struct of_device_id of_k1_ccu_match[] = {
>>   		.compatible	= "spacemit,k1-syscon-apmu",
>>   		.data		= &k1_ccu_apmu_data,
>>   	},
>> +	{
>> +		.compatible	= "spacemit,k1-syscon-rcpu",
>> +		.data		= &k1_ccu_rcpu_data,
>> +	},
>> +	{
>> +		.compatible	= "spacemit,k1-syscon-rcpu2",
>> +		.data		= &k1_ccu_rcpu2_data,
>> +	},
>> +	{
>> +		.compatible	= "spacemit,k1-syscon-apbc2",
>> +		.data		= &k1_ccu_apbc2_data,
>> +	},
>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(of, of_k1_ccu_match);
>> diff --git a/drivers/reset/spacemit/k1.c b/drivers/reset/spacemit/k1.c
>> index 19a34f151b214..27434a1928261 100644
>> --- a/drivers/reset/spacemit/k1.c
>> +++ b/drivers/reset/spacemit/k1.c
>> @@ -137,6 +137,57 @@ static const struct ccu_reset_controller_data k1_apmu_reset_data = {
>>   	.count		= ARRAY_SIZE(apmu_resets),
>>   };
>>   
>> +static const struct ccu_reset_data rcpu_resets[] = {
>> +	[RESET_RCPU_SSP0]	= RESET_DATA(RCPU_SSP0_CLK_RST,	0, BIT(0)),
>> +	[RESET_RCPU_I2C0]	= RESET_DATA(RCPU_I2C0_CLK_RST,	0, BIT(0)),
>> +	[RESET_RCPU_UART1]	= RESET_DATA(RCPU_UART1_CLK_RST, 0, BIT(0)),
>> +	[RESET_RCPU_IR]		= RESET_DATA(RCPU_CAN_CLK_RST,	0, BIT(0)),
>> +	[RESET_RCPU_CAN]	= RESET_DATA(RCPU_IR_CLK_RST,	0, BIT(0)),
>> +	[RESET_RCPU_UART0]	= RESET_DATA(RCPU_UART0_CLK_RST, 0, BIT(0)),
>> +	[RESET_RCPU_HDMI_AUDIO]	= RESET_DATA(AUDIO_HDMI_CLK_CTRL, 0, BIT(0)),
>> +};
>> +
>> +static const struct ccu_reset_controller_data k1_rcpu_reset_data = {
>> +	/* No clocks in the RCPU CCU */
> 
> This information is not useful in the reset driver.

Yes, I think this was just copy-pasted from the previous code.
This won't be there in the next version.

					-Alex

> regards
> Philipp