Re: [PATCH v5 12/21] reset: thead: Add TH1520 reset controller driver

Philipp Zabel
2025-02-19T15:02:26Z

On Mi, 2025-02-19 at 15:02 +0100, Michal Wilczynski wrote:
> Add reset controller driver for the T-HEAD TH1520 SoC that manages
> hardware reset lines for various subsystems. The driver currently
> implements support for GPU reset control, with infrastructure in place
> to extend support for NPU and Watchdog Timer resets in future updates.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 1 +
> drivers/reset/Kconfig | 10 +++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-th1520.c | 141 +++++++++++++++++++++++++++++++++++
> 4 files changed, 153 insertions(+)
> create mode 100644 drivers/reset/reset-th1520.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 819686e98214..e4a0a83b4c11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20425,6 +20425,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/power/thead,th1520-power.h
> F: include/dt-bindings/reset/thead,th1520-reset.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..d6816c86ba95
> --- /dev/null
> +++ b/drivers/reset/reset-th1520.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Samsung Electronics Co., Ltd.
> + * Author: Michal Wilczynski <m.wilczynski@samsung.com>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/reset/thead,th1520-reset.h>
> +
> + /* register offset in VOSYS_REGMAP */
> +#define TH1520_GPU_RST_CFG 0x0
> +#define TH1520_GPU_RST_CFG_MASK GENMASK(1, 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;
> +};
> +
> +struct th1520_reset_map {
> + u32 bit;
> + u32 reg;
> +};
> +
> +static const struct th1520_reset_map th1520_resets[] = {
> + [TH1520_RESET_ID_GPU] = {
> + .bit = TH1520_GPU_SW_GPU_RST,
> + .reg = TH1520_GPU_RST_CFG,
> + },
> + [TH1520_RESET_ID_GPU_CLKGEN] = {
> + .bit = TH1520_GPU_SW_CLKGEN_RST,
> + .reg = TH1520_GPU_RST_CFG,
> + }

I expect the NPU and WDT resets will be added to this list later?

> +};
> +
> +static inline struct th1520_reset_priv *
> +to_th1520_reset(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct th1520_reset_priv, rcdev);
> +}
> +
> +static int th1520_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> + const struct th1520_reset_map *reset;
> +
> + if (id >= ARRAY_SIZE(th1520_resets))
> + return -EINVAL;

This check is not necessary. The core will have checked this in
of_reset_simple_xlate() before returning the reset control.

> +
> + reset = &th1520_resets[id];
> +
> + return regmap_update_bits(priv->map, reset->reg, reset->bit, 0);
> +}
> +
> +static int th1520_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> + const struct th1520_reset_map *reset;
> +
> + if (id >= ARRAY_SIZE(th1520_resets))
> + return -EINVAL;

This check is not necessary.

> +
> + reset = &th1520_resets[id];
> +
> + return regmap_update_bits(priv->map, reset->reg, reset->bit,
> + reset->bit);
> +}
> +
> +static const struct reset_control_ops th1520_reset_ops = {
> + .assert = th1520_reset_assert,
> + .deassert = th1520_reset_deassert,
> +};
> +
> +static const struct regmap_config th1520_reset_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> +};
> +
> +static int th1520_reset_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct th1520_reset_priv *priv;
> + void __iomem *base;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + priv->map = devm_regmap_init_mmio(dev, base,
> + &th1520_reset_regmap_config);
> + if (IS_ERR(priv->map))
> + return PTR_ERR(priv->map);
> +
> + /* Initialize GPU resets to asserted state */
> + ret = regmap_update_bits(priv->map, TH1520_GPU_RST_CFG,
> + TH1520_GPU_RST_CFG_MASK, 0);
> + if (ret)
> + return ret;
> +
> + priv->rcdev.owner = THIS_MODULE;
> + priv->rcdev.nr_resets = 2;

Better use ARRAY_SIZE(th1520_resets) here, this will simplify adding
further resets in the future. With that,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp