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/Kconfig b/drivers/reset/Kconfig
index 99f6f9784e686..b1f1af50ca10b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -354,6 +354,7 @@ source "drivers/reset/amlogic/Kconfig"
 source "drivers/reset/starfive/Kconfig"
 source "drivers/reset/sti/Kconfig"
 source "drivers/reset/hisilicon/Kconfig"
+source "drivers/reset/spacemit/Kconfig"
 source "drivers/reset/tegra/Kconfig"
 
 endif
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 31f9904d13f9c..6c19fd875ff13 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -2,6 +2,7 @@
 obj-y += core.o
 obj-y += amlogic/
 obj-y += hisilicon/
+obj-y += spacemit/
 obj-y += starfive/
 obj-y += sti/
 obj-y += tegra/
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
+
+reset_spacemit-y				:= core.o
+
+reset_spacemit-$(CONFIG_RESET_SPACEMIT_K1)	+= k1.o
diff --git a/drivers/reset/spacemit/core.c b/drivers/reset/spacemit/core.c
new file mode 100644
index 0000000000000..5cd981eb3f097
--- /dev/null
+++ b/drivers/reset/spacemit/core.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* SpacemiT reset driver core */
+
+#include <linux/container_of.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+#include <linux/types.h>
+
+#include "core.h"
+
+static int spacemit_reset_update(struct reset_controller_dev *rcdev,
+				 unsigned long id, bool assert)
+{
+	struct ccu_reset_controller *controller;
+	const struct ccu_reset_data *data;
+	u32 mask;
+	u32 val;
+
+	controller = container_of(rcdev, struct ccu_reset_controller, rcdev);
+	data = &controller->data->reset_data[id];
+	mask = data->assert_mask | data->deassert_mask;
+	val = assert ? data->assert_mask : data->deassert_mask;
+
+	return regmap_update_bits(controller->regmap, data->offset, mask, val);
+}
+
+static int spacemit_reset_assert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	return spacemit_reset_update(rcdev, id, true);
+}
+
+static int spacemit_reset_deassert(struct reset_controller_dev *rcdev,
+				   unsigned long id)
+{
+	return spacemit_reset_update(rcdev, id, false);
+}
+
+static const struct reset_control_ops spacemit_reset_control_ops = {
+	.assert		= spacemit_reset_assert,
+	.deassert	= spacemit_reset_deassert,
+};
+
+/**
+ * spacemit_reset_controller_register() - register a reset controller
+ * @dev:	Device that's registering the controller
+ * @controller:	SpacemiT CCU reset controller structure
+ */
+int spacemit_reset_controller_register(struct device *dev,
+				       struct ccu_reset_controller *controller)
+{
+	struct reset_controller_dev *rcdev = &controller->rcdev;
+
+	rcdev->ops = &spacemit_reset_control_ops;
+	rcdev->owner = THIS_MODULE;
+	rcdev->of_node = dev->of_node;
+	rcdev->nr_resets = controller->data->count;
+
+	return devm_reset_controller_register(dev, &controller->rcdev);
+}
diff --git a/drivers/reset/spacemit/core.h b/drivers/reset/spacemit/core.h
new file mode 100644
index 0000000000000..a71f835ccb779
--- /dev/null
+++ b/drivers/reset/spacemit/core.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* SpacemiT reset driver core */
+
+#ifndef __RESET_SPACEMIT_CORE_H__
+#define __RESET_SPACEMIT_CORE_H__
+
+#include <linux/device.h>
+#include <linux/reset-controller.h>
+#include <linux/types.h>
+
+struct ccu_reset_data {
+	u32 offset;
+	u32 assert_mask;
+	u32 deassert_mask;
+};
+
+#define RESET_DATA(_offset, _assert_mask, _deassert_mask)	\
+	{							\
+		.offset		= (_offset),			\
+		.assert_mask	= (_assert_mask),		\
+		.deassert_mask	= (_deassert_mask),		\
+	}
+
+struct ccu_reset_controller_data {
+	const struct ccu_reset_data *reset_data;	/* array */
+	size_t count;
+};
+
+struct ccu_reset_controller {
+	struct reset_controller_dev rcdev;
+	const struct ccu_reset_controller_data *data;
+	struct regmap *regmap;
+};
+
+extern int spacemit_reset_controller_register(struct device *dev,
+			      struct ccu_reset_controller *controller);
+
+#endif /* __RESET_SPACEMIT_CORE_H__ */
diff --git a/drivers/reset/spacemit/k1.c b/drivers/reset/spacemit/k1.c
new file mode 100644
index 0000000000000..19a34f151b214
--- /dev/null
+++ b/drivers/reset/spacemit/k1.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* SpacemiT reset driver data for the K1 SoC */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bits.h>
+#include <linux/container_of.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/reset-controller.h>
+#include <linux/types.h>
+
+#include <soc/spacemit/ccu_k1.h>
+#include <dt-bindings/clock/spacemit,k1-syscon.h>
+
+#include "core.h"
+
+static const struct ccu_reset_data mpmu_resets[] = {
+	[RESET_WDT]	= RESET_DATA(MPMU_WDTPCR,		BIT(2), 0),
+};
+
+static const struct ccu_reset_controller_data k1_mpmu_reset_data = {
+	.reset_data	= mpmu_resets,
+	.count		= ARRAY_SIZE(mpmu_resets),
+};
+
+static const struct ccu_reset_data apbc_resets[] = {
+	[RESET_UART0]	= RESET_DATA(APBC_UART1_CLK_RST,	BIT(2),	0),
+	[RESET_UART2]	= RESET_DATA(APBC_UART2_CLK_RST,	BIT(2), 0),
+	[RESET_GPIO]	= RESET_DATA(APBC_GPIO_CLK_RST,		BIT(2), 0),
+	[RESET_PWM0]	= RESET_DATA(APBC_PWM0_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM1]	= RESET_DATA(APBC_PWM1_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM2]	= RESET_DATA(APBC_PWM2_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM3]	= RESET_DATA(APBC_PWM3_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM4]	= RESET_DATA(APBC_PWM4_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM5]	= RESET_DATA(APBC_PWM5_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM6]	= RESET_DATA(APBC_PWM6_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM7]	= RESET_DATA(APBC_PWM7_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM8]	= RESET_DATA(APBC_PWM8_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM9]	= RESET_DATA(APBC_PWM9_CLK_RST,		BIT(2), BIT(0)),
+	[RESET_PWM10]	= RESET_DATA(APBC_PWM10_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM11]	= RESET_DATA(APBC_PWM11_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM12]	= RESET_DATA(APBC_PWM12_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM13]	= RESET_DATA(APBC_PWM13_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM14]	= RESET_DATA(APBC_PWM14_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM15]	= RESET_DATA(APBC_PWM15_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM16]	= RESET_DATA(APBC_PWM16_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM17]	= RESET_DATA(APBC_PWM17_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM18]	= RESET_DATA(APBC_PWM18_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_PWM19]	= RESET_DATA(APBC_PWM19_CLK_RST,	BIT(2), BIT(0)),
+	[RESET_SSP3]	= RESET_DATA(APBC_SSP3_CLK_RST,		BIT(2), 0),
+	[RESET_UART3]	= RESET_DATA(APBC_UART3_CLK_RST,	BIT(2), 0),
+	[RESET_RTC]	= RESET_DATA(APBC_RTC_CLK_RST,		BIT(2), 0),
+	[RESET_TWSI0]	= RESET_DATA(APBC_TWSI0_CLK_RST,	BIT(2), 0),
+	[RESET_TIMERS1]	= RESET_DATA(APBC_TIMERS1_CLK_RST,	BIT(2), 0),
+	[RESET_AIB]	= RESET_DATA(APBC_AIB_CLK_RST,		BIT(2), 0),
+	[RESET_TIMERS2]	= RESET_DATA(APBC_TIMERS2_CLK_RST,	BIT(2), 0),
+	[RESET_ONEWIRE]	= RESET_DATA(APBC_ONEWIRE_CLK_RST,	BIT(2), 0),
+	[RESET_SSPA0]	= RESET_DATA(APBC_SSPA0_CLK_RST,	BIT(2), 0),
+	[RESET_SSPA1]	= RESET_DATA(APBC_SSPA1_CLK_RST,	BIT(2), 0),
+	[RESET_DRO]	= RESET_DATA(APBC_DRO_CLK_RST,		BIT(2), 0),
+	[RESET_IR]	= RESET_DATA(APBC_IR_CLK_RST,		BIT(2), 0),
+	[RESET_TWSI1]	= RESET_DATA(APBC_TWSI1_CLK_RST,	BIT(2), 0),
+	[RESET_TSEN]	= RESET_DATA(APBC_TSEN_CLK_RST,		BIT(2), 0),
+	[RESET_TWSI2]	= RESET_DATA(APBC_TWSI2_CLK_RST,	BIT(2), 0),
+	[RESET_TWSI4]	= RESET_DATA(APBC_TWSI4_CLK_RST,	BIT(2), 0),
+	[RESET_TWSI5]	= RESET_DATA(APBC_TWSI5_CLK_RST,	BIT(2), 0),
+	[RESET_TWSI6]	= RESET_DATA(APBC_TWSI6_CLK_RST,	BIT(2), 0),
+	[RESET_TWSI7]	= RESET_DATA(APBC_TWSI7_CLK_RST,	BIT(2), 0),
+	[RESET_TWSI8]	= RESET_DATA(APBC_TWSI8_CLK_RST,	BIT(2), 0),
+	[RESET_IPC_AP2AUD] = RESET_DATA(APBC_IPC_AP2AUD_CLK_RST, BIT(2), 0),
+	[RESET_UART4]	= RESET_DATA(APBC_UART4_CLK_RST,	BIT(2), 0),
+	[RESET_UART5]	= RESET_DATA(APBC_UART5_CLK_RST,	BIT(2), 0),
+	[RESET_UART6]	= RESET_DATA(APBC_UART6_CLK_RST,	BIT(2), 0),
+	[RESET_UART7]	= RESET_DATA(APBC_UART7_CLK_RST,	BIT(2), 0),
+	[RESET_UART8]	= RESET_DATA(APBC_UART8_CLK_RST,	BIT(2), 0),
+	[RESET_UART9]	= RESET_DATA(APBC_UART9_CLK_RST,	BIT(2), 0),
+	[RESET_CAN0]	= RESET_DATA(APBC_CAN0_CLK_RST,		BIT(2), 0),
+};
+
+static const struct ccu_reset_controller_data k1_apbc_reset_data = {
+	.reset_data	= apbc_resets,
+	.count		= ARRAY_SIZE(apbc_resets),
+};
+
+static const struct ccu_reset_data apmu_resets[] = {
+	[RESET_CCIC_4X]	= RESET_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_CCIC1_PHY] = RESET_DATA(APMU_CCIC_CLK_RES_CTRL,	0, BIT(2)),
+	[RESET_SDH_AXI]	= RESET_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_SDH0]	= RESET_DATA(APMU_SDH0_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_SDH1]	= RESET_DATA(APMU_SDH1_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_SDH2]	= RESET_DATA(APMU_SDH2_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_USBP1_AXI] = RESET_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(4)),
+	[RESET_USB_AXI]	= RESET_DATA(APMU_USB_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_USB3_0]	= RESET_DATA(APMU_USB_CLK_RES_CTRL,	0,
+				      BIT(11) | BIT(10) | BIT(9)),
+	[RESET_QSPI]	= RESET_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_QSPI_BUS] = RESET_DATA(APMU_QSPI_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_DMA]	= RESET_DATA(APMU_DMA_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_AES]	= RESET_DATA(APMU_AES_CLK_RES_CTRL,	0, BIT(4)),
+	[RESET_VPU]	= RESET_DATA(APMU_VPU_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_GPU]	= RESET_DATA(APMU_GPU_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_EMMC]	= RESET_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_EMMC_X]	= RESET_DATA(APMU_PMUA_EM_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_AUDIO]	= RESET_DATA(APMU_AUDIO_CLK_RES_CTRL,	0,
+				   BIT(3) | BIT(2) | BIT(0)),
+	[RESET_HDMI]	= RESET_DATA(APMU_HDMI_CLK_RES_CTRL,	0, BIT(9)),
+	[RESET_PCIE0]	= RESET_DATA(APMU_PCIE_CLK_RES_CTRL_0,	BIT(8),
+				   BIT(5) | BIT(4) | BIT(3)),
+	[RESET_PCIE1]	= RESET_DATA(APMU_PCIE_CLK_RES_CTRL_1,	BIT(8),
+				   BIT(5) | BIT(4) | BIT(3)),
+	[RESET_PCIE2]	= RESET_DATA(APMU_PCIE_CLK_RES_CTRL_2,	BIT(8),
+				   BIT(5) | BIT(4) | BIT(3)),
+	[RESET_EMAC0]	= RESET_DATA(APMU_EMAC0_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_EMAC1]	= RESET_DATA(APMU_EMAC1_CLK_RES_CTRL,	0, BIT(1)),
+	[RESET_JPG]	= RESET_DATA(APMU_JPG_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_CCIC2PHY] = RESET_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL, 0, BIT(2)),
+	[RESET_CCIC3PHY] = RESET_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL, 0, BIT(29)),
+	[RESET_CSI]	= RESET_DATA(APMU_CSI_CCIC2_CLK_RES_CTRL, 0, BIT(1)),
+	[RESET_ISP]	= RESET_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(0)),
+	[RESET_ISP_CPP]	= RESET_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(27)),
+	[RESET_ISP_BUS]	= RESET_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(3)),
+	[RESET_ISP_CI]	= RESET_DATA(APMU_ISP_CLK_RES_CTRL,	0, BIT(16)),
+	[RESET_DPU_MCLK] = RESET_DATA(APMU_LCD_CLK_RES_CTRL2,	0, BIT(9)),
+	[RESET_DPU_ESC]	= RESET_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(3)),
+	[RESET_DPU_HCLK] = RESET_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(4)),
+	[RESET_DPU_SPIBUS] = RESET_DATA(APMU_LCD_SPI_CLK_RES_CTRL, 0, BIT(4)),
+	[RESET_DPU_SPI_HBUS] = RESET_DATA(APMU_LCD_SPI_CLK_RES_CTRL, 0, BIT(2)),
+	[RESET_V2D]	= RESET_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(27)),
+	[RESET_MIPI]	= RESET_DATA(APMU_LCD_CLK_RES_CTRL1,	0, BIT(15)),
+	[RESET_MC]	= RESET_DATA(APMU_PMUA_MC_CTRL,		0, BIT(0)),
+};
+
+static const struct ccu_reset_controller_data k1_apmu_reset_data = {
+	.reset_data	= apmu_resets,
+	.count		= ARRAY_SIZE(apmu_resets),
+};
+
+static int spacemit_k1_reset_probe(struct auxiliary_device *adev,
+				   const struct auxiliary_device_id *id)
+{
+	struct spacemit_ccu_adev *rdev = to_spacemit_ccu_adev(adev);
+	const void *data = (void *)id->driver_data;
+	struct ccu_reset_controller *controller;
+	struct device *dev = &adev->dev;
+
+	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
+	if (!controller)
+		return -ENODEV;
+	controller->data = data;
+	controller->regmap = rdev->regmap;
+
+	return spacemit_reset_controller_register(dev, controller);
+}
+
+#define K1_AUX_DEV_ID(_unit) \
+	{ \
+		.name = "spacemit_ccu_k1." #_unit "-reset", \
+		.driver_data = (kernel_ulong_t)&k1_ ## _unit ## _reset_data, \
+	}
+
+static const struct auxiliary_device_id spacemit_k1_reset_ids[] = {
+	K1_AUX_DEV_ID(mpmu),
+	K1_AUX_DEV_ID(apbc),
+	K1_AUX_DEV_ID(apmu),
+	{ },
+};
+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

Hi Alex,

On Di, 2025-05-06 at 16:06 -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.

Are you confident that future SpacemiT CCUs will be sufficiently
similar for this split to make sense? This feels like it could be a
premature abstraction to me.

> 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/Kconfig b/drivers/reset/Kconfig
> index 99f6f9784e686..b1f1af50ca10b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -354,6 +354,7 @@ source "drivers/reset/amlogic/Kconfig"
>  source "drivers/reset/starfive/Kconfig"
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> +source "drivers/reset/spacemit/Kconfig"
>  source "drivers/reset/tegra/Kconfig"
>  
>  endif
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 31f9904d13f9c..6c19fd875ff13 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -2,6 +2,7 @@
>  obj-y += core.o
>  obj-y += amlogic/
>  obj-y += hisilicon/
> +obj-y += spacemit/
>  obj-y += starfive/
>  obj-y += sti/
>  obj-y += tegra/
> 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.

Does the size of the K1 specific parts even warrant this per-SoC
Kconfig option? I suggest to only have a user visible tristate
RESET_SPACEMIT option.

> 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
> +
> +reset_spacemit-y				:= core.o
> +
> +reset_spacemit-$(CONFIG_RESET_SPACEMIT_K1)	+= k1.o
> diff --git a/drivers/reset/spacemit/core.c b/drivers/reset/spacemit/core.c
> new file mode 100644
> index 0000000000000..5cd981eb3f097
> --- /dev/null
> +++ b/drivers/reset/spacemit/core.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* SpacemiT reset driver core */
> +
> +#include <linux/container_of.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +#include <linux/types.h>
> +
> +#include "core.h"
> +
> +static int spacemit_reset_update(struct reset_controller_dev *rcdev,
> +				 unsigned long id, bool assert)
> +{
> +	struct ccu_reset_controller *controller;
> +	const struct ccu_reset_data *data;
> +	u32 mask;
> +	u32 val;
> +
> +	controller = container_of(rcdev, struct ccu_reset_controller, rcdev);
> +	data = &controller->data->reset_data[id];
> +	mask = data->assert_mask | data->deassert_mask;
> +	val = assert ? data->assert_mask : data->deassert_mask;
> +
> +	return regmap_update_bits(controller->regmap, data->offset, mask, val);
> +}
> +
> +static int spacemit_reset_assert(struct reset_controller_dev *rcdev,
> +				 unsigned long id)
> +{
> +	return spacemit_reset_update(rcdev, id, true);
> +}
> +
> +static int spacemit_reset_deassert(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	return spacemit_reset_update(rcdev, id, false);
> +}
> +
> +static const struct reset_control_ops spacemit_reset_control_ops = {
> +	.assert		= spacemit_reset_assert,
> +	.deassert	= spacemit_reset_deassert,
> +};
> +
> +/**
> + * spacemit_reset_controller_register() - register a reset controller
> + * @dev:	Device that's registering the controller
> + * @controller:	SpacemiT CCU reset controller structure
> + */
> +int spacemit_reset_controller_register(struct device *dev,
> +				       struct ccu_reset_controller *controller)
> +{
> +	struct reset_controller_dev *rcdev = &controller->rcdev;
> +
> +	rcdev->ops = &spacemit_reset_control_ops;
> +	rcdev->owner = THIS_MODULE;
> +	rcdev->of_node = dev->of_node;
> +	rcdev->nr_resets = controller->data->count;
> +
> +	return devm_reset_controller_register(dev, &controller->rcdev);
> +}
> diff --git a/drivers/reset/spacemit/core.h b/drivers/reset/spacemit/core.h
> new file mode 100644
> index 0000000000000..a71f835ccb779
> --- /dev/null
> +++ b/drivers/reset/spacemit/core.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* SpacemiT reset driver core */
> +
> +#ifndef __RESET_SPACEMIT_CORE_H__
> +#define __RESET_SPACEMIT_CORE_H__
> +
> +#include <linux/device.h>

This include can be replaced by forward declarations for struct device
and struct regmap.

> +#include <linux/reset-controller.h>
> +#include <linux/types.h>
> +
> +struct ccu_reset_data {
> +	u32 offset;
> +	u32 assert_mask;
> +	u32 deassert_mask;
> +};
> +
> +#define RESET_DATA(_offset, _assert_mask, _deassert_mask)	\
> +	{							\
> +		.offset		= (_offset),			\
> +		.assert_mask	= (_assert_mask),		\
> +		.deassert_mask	= (_deassert_mask),		\
> +	}
> +
> +struct ccu_reset_controller_data {
> +	const struct ccu_reset_data *reset_data;	/* array */
> +	size_t count;
> +};
> +
> +struct ccu_reset_controller {
> +	struct reset_controller_dev rcdev;
> +	const struct ccu_reset_controller_data *data;
> +	struct regmap *regmap;
> +};
> +
> +extern int spacemit_reset_controller_register(struct device *dev,
> +			      struct ccu_reset_controller *controller);

Drop the extern keyword.

> +
> +#endif /* __RESET_SPACEMIT_CORE_H__ */
> diff --git a/drivers/reset/spacemit/k1.c b/drivers/reset/spacemit/k1.c
> new file mode 100644
> index 0000000000000..19a34f151b214
> --- /dev/null
> +++ b/drivers/reset/spacemit/k1.c
> @@ -0,0 +1,177 @@
[...]
> +static int spacemit_k1_reset_probe(struct auxiliary_device *adev,
> +				   const struct auxiliary_device_id *id)
> +{
> +	struct spacemit_ccu_adev *rdev = to_spacemit_ccu_adev(adev);
> +	const void *data = (void *)id->driver_data;
> +	struct ccu_reset_controller *controller;
> +	struct device *dev = &adev->dev;
> +
> +	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
> +	if (!controller)
> +		return -ENODEV;

-ENOMEM, this is a failed memory allocation.

regards
Philipp

On 5/8/25 4:01 AM, Philipp Zabel wrote:
> Hi Alex,
> 
> On Di, 2025-05-06 at 16:06 -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.
> 
> Are you confident that future SpacemiT CCUs will be sufficiently
> similar for this split to make sense? This feels like it could be a
> premature abstraction to me.

Honestly, no.  I decided to do it this way to mirror how
the clock driver is structured, but I agree with you.  The
code is pretty simple currently, and it could all go in a
single file.  Splitting it later would be easy if needed.

I'll do that for the next version.

>> 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/Kconfig b/drivers/reset/Kconfig
>> index 99f6f9784e686..b1f1af50ca10b 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -354,6 +354,7 @@ source "drivers/reset/amlogic/Kconfig"
>>   source "drivers/reset/starfive/Kconfig"
>>   source "drivers/reset/sti/Kconfig"
>>   source "drivers/reset/hisilicon/Kconfig"
>> +source "drivers/reset/spacemit/Kconfig"
>>   source "drivers/reset/tegra/Kconfig"
>>   
>>   endif
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 31f9904d13f9c..6c19fd875ff13 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -2,6 +2,7 @@
>>   obj-y += core.o
>>   obj-y += amlogic/
>>   obj-y += hisilicon/
>> +obj-y += spacemit/
>>   obj-y += starfive/
>>   obj-y += sti/
>>   obj-y += tegra/
>> 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.
> 
> Does the size of the K1 specific parts even warrant this per-SoC
> Kconfig option? I suggest to only have a user visible tristate
> RESET_SPACEMIT option.

I don't know if the size warrants it, it was more about structuring
config options to match the separation of the reset definitions.
Along with consolidating into a single source file, I'll just use
a single config option.

> 
>> 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
>> +
>> +reset_spacemit-y				:= core.o
>> +
>> +reset_spacemit-$(CONFIG_RESET_SPACEMIT_K1)	+= k1.o
>> diff --git a/drivers/reset/spacemit/core.c b/drivers/reset/spacemit/core.c
>> new file mode 100644
>> index 0000000000000..5cd981eb3f097
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/core.c
>> @@ -0,0 +1,61 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/* SpacemiT reset driver core */
>> +
>> +#include <linux/container_of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/types.h>
>> +
>> +#include "core.h"
>> +
>> +static int spacemit_reset_update(struct reset_controller_dev *rcdev,
>> +				 unsigned long id, bool assert)
>> +{
>> +	struct ccu_reset_controller *controller;
>> +	const struct ccu_reset_data *data;
>> +	u32 mask;
>> +	u32 val;
>> +
>> +	controller = container_of(rcdev, struct ccu_reset_controller, rcdev);
>> +	data = &controller->data->reset_data[id];
>> +	mask = data->assert_mask | data->deassert_mask;
>> +	val = assert ? data->assert_mask : data->deassert_mask;
>> +
>> +	return regmap_update_bits(controller->regmap, data->offset, mask, val);
>> +}
>> +
>> +static int spacemit_reset_assert(struct reset_controller_dev *rcdev,
>> +				 unsigned long id)
>> +{
>> +	return spacemit_reset_update(rcdev, id, true);
>> +}
>> +
>> +static int spacemit_reset_deassert(struct reset_controller_dev *rcdev,
>> +				   unsigned long id)
>> +{
>> +	return spacemit_reset_update(rcdev, id, false);
>> +}
>> +
>> +static const struct reset_control_ops spacemit_reset_control_ops = {
>> +	.assert		= spacemit_reset_assert,
>> +	.deassert	= spacemit_reset_deassert,
>> +};
>> +
>> +/**
>> + * spacemit_reset_controller_register() - register a reset controller
>> + * @dev:	Device that's registering the controller
>> + * @controller:	SpacemiT CCU reset controller structure
>> + */
>> +int spacemit_reset_controller_register(struct device *dev,
>> +				       struct ccu_reset_controller *controller)
>> +{
>> +	struct reset_controller_dev *rcdev = &controller->rcdev;
>> +
>> +	rcdev->ops = &spacemit_reset_control_ops;
>> +	rcdev->owner = THIS_MODULE;
>> +	rcdev->of_node = dev->of_node;
>> +	rcdev->nr_resets = controller->data->count;
>> +
>> +	return devm_reset_controller_register(dev, &controller->rcdev);
>> +}
>> diff --git a/drivers/reset/spacemit/core.h b/drivers/reset/spacemit/core.h
>> new file mode 100644
>> index 0000000000000..a71f835ccb779
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/core.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/* SpacemiT reset driver core */
>> +
>> +#ifndef __RESET_SPACEMIT_CORE_H__
>> +#define __RESET_SPACEMIT_CORE_H__
>> +
>> +#include <linux/device.h>
> 
> This include can be replaced by forward declarations for struct device
> and struct regmap.

You're right, but I think this will be moot with a single source file.
Anyway, I'll fix this.

>> +#include <linux/reset-controller.h>
>> +#include <linux/types.h>
>> +
>> +struct ccu_reset_data {
>> +	u32 offset;
>> +	u32 assert_mask;
>> +	u32 deassert_mask;
>> +};
>> +
>> +#define RESET_DATA(_offset, _assert_mask, _deassert_mask)	\
>> +	{							\
>> +		.offset		= (_offset),			\
>> +		.assert_mask	= (_assert_mask),		\
>> +		.deassert_mask	= (_deassert_mask),		\
>> +	}
>> +
>> +struct ccu_reset_controller_data {
>> +	const struct ccu_reset_data *reset_data;	/* array */
>> +	size_t count;
>> +};
>> +
>> +struct ccu_reset_controller {
>> +	struct reset_controller_dev rcdev;
>> +	const struct ccu_reset_controller_data *data;
>> +	struct regmap *regmap;
>> +};
>> +
>> +extern int spacemit_reset_controller_register(struct device *dev,
>> +			      struct ccu_reset_controller *controller);
> 
> Drop the extern keyword.

OK, but here too I think there will be no external declaration in
the next version.

>> +
>> +#endif /* __RESET_SPACEMIT_CORE_H__ */
>> diff --git a/drivers/reset/spacemit/k1.c b/drivers/reset/spacemit/k1.c
>> new file mode 100644
>> index 0000000000000..19a34f151b214
>> --- /dev/null
>> +++ b/drivers/reset/spacemit/k1.c
>> @@ -0,0 +1,177 @@
> [...]
>> +static int spacemit_k1_reset_probe(struct auxiliary_device *adev,
>> +				   const struct auxiliary_device_id *id)
>> +{
>> +	struct spacemit_ccu_adev *rdev = to_spacemit_ccu_adev(adev);
>> +	const void *data = (void *)id->driver_data;
>> +	struct ccu_reset_controller *controller;
>> +	struct device *dev = &adev->dev;
>> +
>> +	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
>> +	if (!controller)
>> +		return -ENODEV;
> 
> -ENOMEM, this is a failed memory allocation.

Yes, you're correct, I'll fix this.  Thank you very much
for your review.

					-Alex

> regards
> Philipp

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,

        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.

> +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,

	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

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

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