Re: [PATCH RFC v3 3/5] clk: zte: Introduce a driver for zx297520v3 top clocks and resets.
From: Philipp Zabel
Date: Wed Jun 03 2026 - 05:17:20 EST
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@xxxxxxxxx>
>
---
>
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(®_lock, flags);
+ res = __zx297520v3_rst_assert(rcdev, id);
+ spin_unlock_irqrestore(®_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(®_lock, flags);
+ res = __zx297520v3_rst_deassert(rcdev, id);
+ spin_unlock_irqrestore(®_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(®_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(®_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