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

[Alex Elder]

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