Re: [RFC v3 08/18] reset: thead: Add TH1520 reset controller driver

Philipp Zabel
2025-01-21T08:40:51Z

On Mo, 2025-01-20 at 18:21 +0100, Michal Wilczynski wrote:
> Introduce reset controller driver for the T-HEAD TH1520 SoC. The
> controller manages hardware reset lines for various SoC subsystems, such
> as the GPU.

This statement is confusing, given the implementation only handles a
single (GPU) reset control.

> By exposing these resets via the Linux reset subsystem,
> drivers can request and control hardware resets to reliably initialize
> or recover key components.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 1 +
> drivers/reset/Kconfig | 10 +++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-th1520.c | 144 +++++++++++++++++++++++++++++++++++
> 4 files changed, 156 insertions(+)
> create mode 100644 drivers/reset/reset-th1520.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b6e894500ef..18382a356b12 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20197,6 +20197,7 @@ F: drivers/mailbox/mailbox-th1520.c
> F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> F: drivers/pinctrl/pinctrl-th1520.c
> F: drivers/pmdomain/thead/
> +F: drivers/reset/reset-th1520.c
> F: include/dt-bindings/clock/thead,th1520-clk-ap.h
> F: include/dt-bindings/firmware/thead,th1520-aon.h
> F: include/linux/firmware/thead/thead,th1520-aon.h
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 5b3abb6db248..fa0943c3d1de 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -272,6 +272,16 @@ config RESET_SUNXI
> help
> This enables the reset driver for Allwinner SoCs.
>
> +config RESET_TH1520
> + tristate "T-HEAD 1520 reset controller"
> + depends on ARCH_THEAD || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + This driver provides support for the T-HEAD TH1520 SoC reset controller,
> + which manages hardware reset lines for SoC components such as the GPU.
> + Enable this option if you need to control hardware resets on TH1520-based
> + systems.
> +
> config RESET_TI_SCI
> tristate "TI System Control Interface (TI-SCI) reset driver"
> depends on TI_SCI_PROTOCOL || (COMPILE_TEST && TI_SCI_PROTOCOL=n)
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 677c4d1e2632..d6c2774407ae 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> +obj-$(CONFIG_RESET_TH1520) += reset-th1520.o
> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> obj-$(CONFIG_RESET_TI_TPS380X) += reset-tps380x.o
> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> new file mode 100644
> index 000000000000..e4278f49c62f
> --- /dev/null
> +++ b/drivers/reset/reset-th1520.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Samsung Electronics Co., Ltd.
> + * Author: Michal Wilczynski <m.wilczynski@samsung.com>
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +
> + /* register offset in VOSYS_REGMAP */
> +#define TH1520_GPU_RST_CFG 0x0
> +#define TH1520_GPU_RST_CFG_MASK GENMASK(2, 0)
> +
> +/* register values */
> +#define TH1520_GPU_SW_GPU_RST BIT(0)
> +#define TH1520_GPU_SW_CLKGEN_RST BIT(1)
> +
> +struct th1520_reset_priv {
> + struct reset_controller_dev rcdev;
> + struct regmap *map;
> +};
> +
> +static inline struct th1520_reset_priv *
> +to_th1520_reset(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct th1520_reset_priv, rcdev);
> +}
> +
> +static void th1520_rst_gpu_enable(struct regmap *reg)
> +{
> + int val;
> +
> + /* if the GPU is not in a reset state it, put it into one */
> + regmap_read(reg, TH1520_GPU_RST_CFG, &val);
> + if (val)
> + regmap_update_bits(reg, TH1520_GPU_RST_CFG,
> + TH1520_GPU_RST_CFG_MASK, 0x0);
> +
> + /* rst gpu clkgen */
> + regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
> +
> + /*
> + * According to the hardware manual, a delay of at least 32 clock
> + * cycles is required between de-asserting the clkgen reset and
> + * de-asserting the GPU reset. Assuming a worst-case scenario with
> + * a very high GPU clock frequency, a delay of 1 microsecond is
> + * sufficient to ensure this requirement is met across all
> + * feasible GPU clock speeds.
> + */
> + udelay(1);
> +
> + /* rst gpu */
> + regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);

This sequence of TH1520_GPU_RST_CFG register accesses should be
protected by a lock.

[...]
> +static int th1520_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> +
> + th1520_rst_gpu_disable(priv->map);
> +
> + return 0;
> +}
> +
> +static int th1520_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> +
> + th1520_rst_gpu_enable(priv->map);
> +
> + return 0;
> +}
> +
> +static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
> + const struct of_phandle_args *reset_spec)
> +{
> + return 0;
> +}

These all explicitly handle only a single reset control, which is in
conflict with the commit message of this patch and the dt-binding
patch. Will more reset controls be added to this driver in the future?

regards
Philipp