Re: [PATCH v6 4/6] reset: spacemit: add support for SpacemiT CCU resets
From: Alex Elder
Date: Thu May 08 2025 - 08:05:24 EST
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@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/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