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

[Michal Wilczynski]

On 2/19/25 16:01, Philipp Zabel wrote:
> 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@xxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxx>
>> + */
>> +
>> +#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?

Thanks! Yes, the NPU and WDT resets will be added later. For now, we’re
hoping to merge the current patchset as it has already grown quite a
bit.

I'll be back from my time off at the beginning of March and will re-send
the series with fixes for any remaining issues.

> 
>> +};
>> +
>> +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@xxxxxxxxxxxxxx>
> 
> regards
> Philipp
>