Re: [PATCH 07/18] reset: zhihe: add A210 support

[Philipp Zabel <p.zabel@pengutronix.de>]

On Do, 2026-10-22 at 03:53 +0800, Han Gao via B4 Relay wrote:
> From: Zhiguo Zhu <zhiguo.zhu@linux.alibaba.com>
> 
> Add support for A210 reset banks and linked reset signals. Check
> register readback to detect rejected writes.

Please explain why the linked reset mechanism is required. To me it
looks like these should be separate resets.

> Signed-off-by: Zhiguo Zhu <zhiguo.zhu@linux.alibaba.com>
> Co-developed-by: Han Gao <gaohan@iscas.ac.cn>
> Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
> ---
>  drivers/reset/Kconfig      |  11 +
>  drivers/reset/Makefile     |   1 +
>  drivers/reset/reset-a210.c | 589 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 601 insertions(+)
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 3168ab1eea88..039e8a4046dc 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -428,6 +428,17 @@ config RESET_UNIPHIER_GLUE
>  	  on UniPhier SoCs. Say Y if you want to control reset signals
>  	  provided by the glue layer.
>  
> +config RESET_ZHIHE_A210
> +	tristate "ZhiHe A210 reset controller"
> +	depends on OF && (ARCH_ZHIHE || COMPILE_TEST)
> +	default ARCH_ZHIHE
> +	help
> +	  Build the reset controller driver for the ZhiHe A210 SoC.
> +
> +	  This driver provides reset control for the various hardware
> +	  blocks in the A210 SoC. Say Y if you are building a kernel
> +	  for the ZhiHe A210 platform.
> +
>  config RESET_ZYNQ
>  	bool "ZYNQ Reset Driver" if COMPILE_TEST
>  	default ARCH_ZYNQ
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 45ad17062462..3620e94e4539 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -52,5 +52,6 @@ obj-$(CONFIG_RESET_TI_TPS380X) += reset-tps380x.o
>  obj-$(CONFIG_RESET_TN48M_CPLD) += reset-tn48m.o
>  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
>  obj-$(CONFIG_RESET_UNIPHIER_GLUE) += reset-uniphier-glue.o
> +obj-$(CONFIG_RESET_ZHIHE_A210) += reset-a210.o
>  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
>  obj-$(CONFIG_RESET_ZYNQMP) += reset-zynqmp.o
> diff --git a/drivers/reset/reset-a210.c b/drivers/reset/reset-a210.c
> new file mode 100644
> index 000000000000..38bad400d6b8
> --- /dev/null
> +++ b/drivers/reset/reset-a210.c
> @@ -0,0 +1,589 @@
[...]
> +static int a210_reset_update(struct reset_controller_dev *rcdev,
> +			     unsigned long id, bool assert)
> +{
> +	struct a210_reset *rst = to_a210_reset(rcdev);
> +	const struct a210_rst_signal *signal;
> +	unsigned long flags;
> +	u32 value;
> +	int ret = 0;
> +
> +	if (id >= rst->data->num_signals)
> +		return -EINVAL;

Unnecessary, the core already checks id < rcdev->num_resets before
creating the reset control.

> +
> +	spin_lock_irqsave(&rst->lock, flags);

If you use guard() here ...

> +	do {
> +		signal = &rst->data->signals[id];
> +		value = readl(rst->base + signal->offset);
> +		if (assert)
> +			value &= ~signal->mask;
> +		else
> +			value |= signal->mask;
> +		writel(value, rst->base + signal->offset);
> +		/* Firmware can lock individual reset controls against AP writes. */
> +		if ((readl(rst->base + signal->offset) & signal->mask) !=
> +		    (value & signal->mask)) {
> +			ret = -EIO;
> +			break;

... this could just return -EIO;

> +		}
> +		id = signal->nextid;
> +	} while (id);

Is this loop a workaround for the UART driver not handling two resets?

Or do the two linked reset bits really have to be asserted, one
directly after the other, without any other write to the same register
in-between and without setting them at the same time? If so, why?

> +	spin_unlock_irqrestore(&rst->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int a210_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	return a210_reset_update(rcdev, id, true);
> +}
> +
> +static int a210_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	return a210_reset_update(rcdev, id, false);
> +}
> +
> +static int a210_reset_status(struct reset_controller_dev *rcdev, unsigned long id)

Do you actually need this for any consumer?

> +{
> +	struct a210_reset *rst = to_a210_reset(rcdev);
> +	const struct a210_rst_signal *signal;
> +	unsigned long flags;
> +	bool asserted;
> +
> +	if (id >= rst->data->num_signals)
> +		return -EINVAL;
> +
> +	/* The logical reset is asserted if any linked signal is asserted. */

What is the "logical reset" in this context?

When there are multiple reset lines that can be in different states,
I'd expect this to return an error.

> +	spin_lock_irqsave(&rst->lock, flags);
> +	do {
> +		signal = &rst->data->signals[id];
> +		asserted = (readl(rst->base + signal->offset) & signal->mask) !=
> +			   signal->mask;
> +		if (asserted)
> +			break;
> +		id = signal->nextid;
> +	} while (id);
> +	spin_unlock_irqrestore(&rst->lock, flags);
> +
> +	return asserted;
> +}

regards
Philipp