| Message ID | 20250128194816.2185326-10-m.wilczynski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable drm/imagination BXM-4-64 Support for LicheePi 4A | expand |
On Di, 2025-01-28 at 20:48 +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 | 178 +++++++++++++++++++++++++++++++++++ > 4 files changed, 190 insertions(+) > create mode 100644 drivers/reset/reset-th1520.c > [...] > 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 @@ [...] > +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; It is customary to return -EINVAL for unsupported resets. Further, you don't have to implement a custom .of_xlate at all. With nr_resets being set to 1, and because TH1520_RESET_ID_GPU happens to be 0, the of_reset_simple_xlate() default implementation will do exactly the same. [...] > +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; > A. > + priv->rcdev.of_xlate = th1520_reset_xlate; > + priv->rcdev.of_reset_n_cells = 1; You could just drop these two lines again. With that, Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
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@samsung.com> > --- > 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@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(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). > + > + /* > + * 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. 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? 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@samsung.com>"); > +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller"); > +MODULE_LICENSE("GPL");
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@samsung.com> >> --- >> 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@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(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@samsung.com>"); >> +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller"); >> +MODULE_LICENSE("GPL"); >
On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote: > > 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@samsung.com> > > > --- > > > 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/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 @@ [...] > > > +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); BIT(2) is not documented, but cleared here. > > > + > > > + /* 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. If this is a workaround for broken gating of the "mem" clock, would it be possible (and reasonable) to make this a separate reset control that is handled by the clock driver? ... > > > + > > > + /* > > > + * 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. ... This could be expressed as a delay between clk_prepare_enable() and reset_control_deassert() in the GPU driver then. > 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? No, the "resets" property should just describe the physical connection(s) between reset controller and the device. It is fine for the device driver to manually assert the reset, enable clocks and power, delay, and then deassert the reset, if that is the device specific reset procedure. > 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. I agree. regards Philipp
On Mo, 2025-02-10 at 19:17 +0100, Michal Wilczynski wrote: > On 2/4/25 18:18, Philipp Zabel wrote: > > On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote: [...] > > > 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. > > > > If this is a workaround for broken gating of the "mem" clock, would it > > be possible (and reasonable) to make this a separate reset control that > > is handled by the clock driver? ... > > Thank you for the detailed feedback, Philipp. > > After further consideration, I believe keeping the current reset driver > implementation would be preferable to moving the CLKGEN reset handling > to the clock driver. While it's technically possible to implement this > in the clock driver, I have concerns about the added complexity: > > 1. We'd need to expose the CLKGEN reset separately in the reset driver I'd expect this to simplify the reset driver. > 2. The clock driver's dt-bindings would need modification to add an > optional resets property If it describes the hardware correctly, that should be fine. > 3. We'd need custom clk_ops for all three clock gates (including a dummy > 'mem' gate) > 4. Each clock gate's .enable operation would need to handle CLKGEN reset > deassertion I accept these arguments, as I have no good feeling for how much complexity this would actually add. In my mind it shouldn't be much: the GPU clocks could all share the same refcounted implementation. The first clock to get enabled would ungate both GPU_CORE and GPU_CFG_ACLK gates and deassert GPU_SW_CLKGEN_RST, all in one place. The remaining enable(s) would be no-ops. Would that work? Whether a separate "dummy" MEM clock for the DT bindings is added or not would not make a difference. > While the clock framework could theoretically handle this, there's no > clean way to express the requirement that the CLKGEN reset should only > be deasserted after all clocks in the group are enabled. We could > implement this explicitly, but it would make the code more complex and > harder to understand. Doing this in the clock driver would have the advantage of clk_enabled GPU clocks actually staying physically enabled, without the reset driver disabling them via GPU_SW_CLKGEN_RST from the outside. > The current solution in the reset driver is simpler and clearer - it > treats this as what it really is: a TH1520-specific reset sequence. Yes. What this also is: a workaround for a SoC specific defect in the clock tree. I think it belongs in the clock driver because of this. [...] > Regarding the delay between clock enable and reset deassert - for SoCs > like BPI-F3 with a single reset line, implementing this in the GPU > consumer driver makes perfect sense. However, for the T-HEAD SoC, moving > the delay there would actually complicate things since we need to manage > both the CLKGEN and GPU reset lines in a specific sequence. Having this > handled entirely within the reset driver keeps the implementation > cleaner. You could delay in both places, it's just a microsecond after all. Whether the workaround is implemented in the reset driver or in the clock driver, I wouldn't want the GPU driver to have to carry a special case for TH1520. > Does this reasoning align with your thoughts? I'm happy to explore the > clock driver approach further if you still see significant advantages to > that solution. I won't object to carry this in the reset driver if the clock implementation turns out to be unreasonably complex, but I currently don't expect that to be the case. Please give it a shot. regards Philipp
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@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(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); + + /* + * 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); + + 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@samsung.com>"); +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller"); +MODULE_LICENSE("GPL");
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 | 178 +++++++++++++++++++++++++++++++++++ 4 files changed, 190 insertions(+) create mode 100644 drivers/reset/reset-th1520.c