Re: [PATCH v6 4/6] reset: spacemit: add support for SpacemiT CCU resets

[Alex Elder <elder@riscstar.com>]

On 5/8/25 12:38 AM, Haylen Chu wrote:
> On Tue, May 06, 2025 at 04:06:35PM -0500, Alex Elder wrote:
>> Implement reset support for SpacemiT CCUs.  The code is structured to
>> handle SpacemiT resets generically, while defining the set of specific
>> reset controllers and their resets in an SoC-specific source file.  A
>> SpacemiT reset controller device is an auxiliary device associated with
>> a clock controller (CCU).
>>
>> This initial patch defines the reset controllers for the MPMU, APBC, and
>> MPMU CCUs, which already defined clock controllers.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>>   drivers/reset/Kconfig           |   1 +
>>   drivers/reset/Makefile          |   1 +
>>   drivers/reset/spacemit/Kconfig  |  12 +++
>>   drivers/reset/spacemit/Makefile |   7 ++
>>   drivers/reset/spacemit/core.c   |  61 +++++++++++
>>   drivers/reset/spacemit/core.h   |  39 +++++++
>>   drivers/reset/spacemit/k1.c     | 177 ++++++++++++++++++++++++++++++++
>>   7 files changed, 298 insertions(+)
>>   create mode 100644 drivers/reset/spacemit/Kconfig
>>   create mode 100644 drivers/reset/spacemit/Makefile
>>   create mode 100644 drivers/reset/spacemit/core.c
>>   create mode 100644 drivers/reset/spacemit/core.h
>>   create mode 100644 drivers/reset/spacemit/k1.c
>>
> 
> ...
> 
>> diff --git a/drivers/reset/spacemit/Kconfig b/drivers/reset/spacemit/Kconfig
>> new file mode 100644
>> index 0000000000000..4ff3487a99eff
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/Kconfig
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config RESET_SPACEMIT
>> +	bool
>> +
>> +config RESET_SPACEMIT_K1
>> +	tristate "SpacemiT K1 reset driver"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	select RESET_SPACEMIT
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  This enables the reset controller driver for the SpacemiT K1 SoC.
> 
> With auxiliary bus introduced, Kconfig entries for both the reset and
> clock should select AUXILIARY_BUS, or building defconfig will fail with
> undefined references,

Wow, I really should have made this v1 of a new series.  You point
out several problems that came out of this using the auxiliary
device framework, which I should have tested more thoroughly.

Yes I will update this to select that, and will test it before
my next version.

> 
>          riscv64-unknown-linux-musl-ld: drivers/clk/spacemit/ccu-k1.o: in function `k1_ccu_probe':
>          ccu-k1.c:(.text+0x19c): undefined reference to `auxiliary_device_init'
>          riscv64-unknown-linux-musl-ld: ccu-k1.c:(.text+0x226): undefined reference to `__auxiliary_device_add'
>          riscv64-unknown-linux-musl-ld: drivers/reset/spacemit/k1.o: in function `spacemit_k1_reset_driver_init':
>          k1.c:(.init.text+0x1a): undefined reference to `__auxiliary_driver_register'
>          riscv64-unknown-linux-musl-ld: drivers/reset/spacemit/k1.o: in function `spacemit_k1_reset_driver_exit':
>          k1.c:(.exit.text+0x10): undefined reference to `auxiliary_driver_unregister'
> 
>> diff --git a/drivers/reset/spacemit/Makefile b/drivers/reset/spacemit/Makefile
>> new file mode 100644
>> index 0000000000000..3a064e9d5d6b4
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_RESET_SPACEMIT)			+= reset_spacemit.o
> 
> As RESET_SPACEMIT is defined as bool, the reset driver will never be
> compiled as a module... so either the RESET_SPACEMIT_K1 should be
> limited to bool as well or you could take an approach similar to the
> clock driver.

I'm not sure I understand this statement, at least in this
context.  This pattern is used to define a single module
"reset_spacemit.o" out of several source files.

But I think you're saying that RESET_SPACEMIT and
RESET_SPACEMIT_K1 should both be bool, or both be
tristate.  I will resolve that question before I
send the next version.

>> +reset_spacemit-y				:= core.o
>> +
>> +reset_spacemit-$(CONFIG_RESET_SPACEMIT_K1)	+= k1.o
> 
> ...
> 
>> new file mode 100644
>> index 0000000000000..19a34f151b214
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/k1.c
> 
> ...
> 
>> +MODULE_DEVICE_TABLE(auxiliary, spacemit_k1_reset_ids);
>> +
>> +#undef K1_AUX_DEV_ID
>> +
>> +static struct auxiliary_driver spacemit_k1_reset_driver = {
>> +	.probe          = spacemit_k1_reset_probe,
>> +	.id_table       = spacemit_k1_reset_ids,
>> +};
>> +module_auxiliary_driver(spacemit_k1_reset_driver);
>> -- 
>> 2.45.2
> 
> If you're willing to make the reset driver buildable as a module, please
> add MODULE_{LICENSE,DESCRIPTION} statements and possibly also
> MODULE_AUTHOR(), or modpost will complain,

OK, thank you.

					-Alex

> 
> 	ERROR: modpost: missing MODULE_LICENSE() in drivers/reset/spacemit/reset_spacemit.o
> 	WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/reset/spacemit/reset_spacemit.o
> 
> Best regards,
> Haylen Chu


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv