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

[Haylen Chu <heylenay@4d2.org>]

On Thu, May 08, 2025 at 06:55:17AM -0500, Alex Elder wrote:
> 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.

...

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

The problem is that RESET_SPACEMIT could only evaluate to N or Y (it's a
bool entry), thus reset_spacemit.o will always be built into the kernel,
regardless whether RESET_SPACEMIT_K1 is set to Y or M.

In the clock driver, the platform config entry (SPACEMIT_CCU, bool type)
is used to hide SoC-specific entries instead of being really used in
Makefile.

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

This isn't necessary, but making both of them bool will definitely
simplify your work, although I'd like to see the reset driver able to
be built as module, just like the clock one :)

> > > +reset_spacemit-y				:= core.o
> > > +
> > > +reset_spacemit-$(CONFIG_RESET_SPACEMIT_K1)	+= k1.o

Regards,
Haylen Chu