On Fr, 2026-05-29 at 00:53 +0300, Stefan Dösinger wrote:

> ```
This register space controls core devices: PLLs, the AHB bus, a lot of
timers, the USB controller, the Cortex M0 processor that boots the board
and a few other devices. For some reason the LTE coprocessor is also
partially controlled by it. The main application processor and DDR
memory are not found here though.

> 
The register to reboot the board is also found here.

> 
Signed-off-by: Stefan Dösinger <stefandoesinger@gmail.com>

> 
---

> 
Patch changlog:

> 
v2:
*) Add code to set up PLLs
*) Merge top and matrix controllers into one device
*) Bugfixes pointed out by Sashiko
---
 MAINTAINERS                      |   1 +
 drivers/clk/Kconfig              |   1 +
 drivers/clk/Makefile             |   1 +
 drivers/clk/zte/Kconfig          |  18 +
 drivers/clk/zte/Makefile         |   5 +
 drivers/clk/zte/clk-zx297520v3.c | 775 +++++++++++++++++++++++++++++++++++++++
 drivers/clk/zte/pll.c            | 450 +++++++++++++++++++++++
 drivers/clk/zte/pll.h            |  23 ++
 8 files changed, 1274 insertions(+)

> 
```

[...]

> ```
diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
new file mode 100644
index 000000000000..986042dd4caf
--- /dev/null
+++ b/drivers/clk/zte/clk-zx297520v3.c
@@ -0,0 +1,775 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Stefan Dösinger
+ */
+#include <dt-bindings/clock/zte,zx297520v3-clk.h>
+#include <linux/reset-controller.h>
+#include <linux/platform_device.h>
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/reboot.h>
+#include <linux/iopoll.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#include "pll.h"
+
+/* All LSP and some Matrix registers contain both resets and clock gates, so access to them needs
+ * to be synchronized between the reset and clock callbacks.
+ */
+static DEFINE_SPINLOCK(reg_lock);
+
+struct zx29_reset_reg {
+	void __iomem *reg;
+	u32 mask, wait_mask;
+};
+
+struct zx29_clk_controller {
+	struct clk_hw_onecell_data *clocks;
+	struct reset_controller_dev rcdev;
+	struct zx29_reset_reg resets[];
+};
+
+static int __zx297520v3_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct zx29_clk_controller *data = container_of(rcdev, struct zx29_clk_controller,
+							    rcdev);
+	u32 val;
+
+	val = readl(data->resets[id].reg);
+	val &= ~data->resets[id].mask;
+	writel(val, data->resets[id].reg);
+
+	return 0;
+}
+
+static int zx297520v3_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	unsigned long flags;
+	int res;
+
+	spin_lock_irqsave(&reg_lock, flags);
+	res = __zx297520v3_rst_assert(rcdev, id);
+	spin_unlock_irqrestore(&reg_lock, flags);
+
+	return res;
+}
+
+static int __zx297520v3_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct zx29_clk_controller *data = container_of(rcdev, struct zx29_clk_controller,
+							    rcdev);
+	u32 val;
+
+	val = readl(data->resets[id].reg);
+	val |= data->resets[id].mask;
+	writel(val, data->resets[id].reg);
```


I'd move the spinlock in here ...


> ```
+	/* This is a special case used only by USB reset */
+	if (data->resets[id].wait_mask) {
+		return readl_poll_timeout(data->resets[id].reg + 4, val,
+					  val & data->resets[id].wait_mask, 1, 100);
```


... because this might sleep.


> ```
+	}
+
+	return 0;
+}
+
+static int zx297520v3_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	unsigned long flags;
+	int res;
+
+	spin_lock_irqsave(&reg_lock, flags);
+	res = __zx297520v3_rst_deassert(rcdev, id);
+	spin_unlock_irqrestore(&reg_lock, flags);
+
+	return res;
+}
+
+static int zx297520v3_rst_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	unsigned long flags;
+	int res;
+
+	spin_lock_irqsave(&reg_lock, flags);
+
+	res = __zx297520v3_rst_assert(rcdev, id);
+	if (res)
+		goto unlock;
+	udelay(100);
```


Is this delay long enough for all potential users of reset_control_reset()? Are there actually any at all?

Why delay under the global spinlock?  
You could use fsleep() instead and only lock the read-modify-write cycles.


> ```
+	res = __zx297520v3_rst_deassert(rcdev, id);
+
+unlock:
+	spin_unlock_irqrestore(&reg_lock, flags);
+	return res;
+}
+
+static int zx297520v3_rst_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct zx29_clk_controller *data = container_of(rcdev, struct zx29_clk_controller,
+							    rcdev);
+	u32 val;
+
+	val = readl(data->resets[id].reg);
+
+	return val & data->resets[id].mask;
```


This will return a negative value for bit BIT(31), I'd wrap this with a double negation.

regards  
Philipp