Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Michal Wilczynski]

On 1/31/25 16:39, Matt Coster wrote:
> On 28/01/2025 19:48, 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 | 178 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 190 insertions(+)
>>  create mode 100644 drivers/reset/reset-th1520.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b4e21d814481..d71b8c68ae48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20352,6 +20352,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..48afbc9f1cdd
>> --- /dev/null
>> +++ b/drivers/reset/reset-th1520.c
>> @@ -0,0 +1,178 @@
>> +// 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(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;
>> +	struct mutex gpu_seq_lock;  /* protects gpu assert/deassert sequence */
>> +};
>> +
>> +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,
>> +				  struct mutex *gpu_seq_lock)
>> +{
>> +	int val;
>> +
>> +	mutex_lock(gpu_seq_lock);
>> +
>> +	/* 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);
> 
> Do you know what this resets? From our side, the GPU only has a single
> reset line (which I assume to be GPU_RESET).

This is clock generator reset, as described in the manual 5.4.2.6.1
GPU_RST_CFG. It does reside in the same register as the GPU reset line.

I think this is required because the MEM clock gate is somehow broken
and marked as 'reserved' in manual, so instead as a workaround, since we
can't reliably enable the 'mem' clock it's a good idea to reset the
whole CLKGEN of the GPU.

> 
>> +
>> +	/*
>> +	 * 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);
> 
> I don't love that this procedure appears in the platform reset driver.
> I appreciate it may not be clear from the SoC TRM, but this is the
> standard reset procedure for all IMG Rogue GPUs. The currently
> supported TI SoC handles this in silicon, when power up/down requests
> are sent so we never needed to encode it in the driver before.
> 
> Strictly speaking, the 32 cycle delay is required between power and
> clocks being enabled and the reset line being deasserted. If nothing
> here touches power or clocks (which I don't think it should), the delay
> could potentially be lifted to the GPU driver.

Yeah you're making excellent points here, I think it would be a good    
idea to place the delay in the GPU driver, since this is specific to the
whole family of the GPU's not the SoC itself.

> 
> Is it expected that if a device exposes a reset in devicetree that it
> can be cleanly reset without interaction with the device driver itself?
> I.E. in this case, is it required that the reset driver alone can cleanly
> reset the GPU?

I'm not sure what the community as a whole thinks about that, so maybe
someone else can answer this, but I would code SoC specific stuff in the
reset driver for the SoC, and the GPU specific stuff (like the delay) in
the GPU driver code. I wasn't sure whether the delay was specific to the
SoC or the GPU so I've put it here.

> 
> Cheers,
> Matt
> 
>> +
>> +	/* rst gpu */
>> +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
>> +
>> +	mutex_unlock(gpu_seq_lock);
>> +}
>> +
>> +static void th1520_rst_gpu_disable(struct regmap *reg,
>> +				   struct mutex *gpu_seq_lock)
>> +{
>> +	mutex_lock(gpu_seq_lock);
>> +
>> +	regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
>> +
>> +	mutex_unlock(gpu_seq_lock);
>> +}
>> +
>> +static int th1520_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
>> +
>> +	switch (id) {
>> +	case TH1520_RESET_ID_GPU:
>> +		th1520_rst_gpu_disable(priv->map, &priv->gpu_seq_lock);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int th1520_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
>> +
>> +	switch (id) {
>> +	case TH1520_RESET_ID_GPU:
>> +		th1520_rst_gpu_enable(priv->map, &priv->gpu_seq_lock);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
>> +			      const struct of_phandle_args *reset_spec)
>> +{
>> +	unsigned int index = reset_spec->args[0];
>> +
>> +	/* currently, only GPU reset is implemented in this driver */
>> +	if (index == TH1520_RESET_ID_GPU)
>> +		return index;
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +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;
>> +
>> +	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);
>> +
>> +	mutex_init(&priv->gpu_seq_lock);
>> +
>> +	priv->rcdev.owner = THIS_MODULE;
>> +	priv->rcdev.nr_resets = 1;
>> +	priv->rcdev.ops = &th1520_reset_ops;
>> +	priv->rcdev.of_node = dev->of_node;
>> +	priv->rcdev.of_xlate = th1520_reset_xlate;
>> +	priv->rcdev.of_reset_n_cells = 1;
>> +
>> +	return devm_reset_controller_register(dev, &priv->rcdev);
>> +}
>> +
>> +static const struct of_device_id th1520_reset_match[] = {
>> +	{ .compatible = "thead,th1520-reset" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, th1520_reset_match);
>> +
>> +static struct platform_driver th1520_reset_driver = {
>> +	.driver = {
>> +		.name = "th1520-reset",
>> +		.of_match_table = th1520_reset_match,
>> +	},
>> +	.probe = th1520_reset_probe,
>> +};
>> +module_platform_driver(th1520_reset_driver);
>> +
>> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller");
>> +MODULE_LICENSE("GPL");
>