Message ID | 20250120172111.3492708-1-m.wilczynski@samsung.com |
---|---|
Headers | show |
Series | Enable drm/imagination BXM-4-64 Support for LicheePi 4A | expand |
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
On Mon, Jan 20, 2025 at 06:21:03PM +0100, Michal Wilczynski wrote: > Many RISC-V boards featuring Imagination Technologies GPUs require a > reset line to be de-asserted as part of the GPU power-up sequence. To > support this, add a 'resets' property (and corresponding 'reset-names') > to the GPU device tree bindings. This ensures the GPU can be properly > initialized on these platforms. > > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> > --- > Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 1/21/25 10:47, Krzysztof Kozlowski wrote: > On Mon, Jan 20, 2025 at 06:20:54PM +0100, Michal Wilczynski wrote: >> properties: >> compatible: >> - const: thead,th1520-clk-ap >> + enum: >> + - thead,th1520-clk-ap >> + - thead,th1520-clk-vo >> >> reg: >> maxItems: 1 >> >> clocks: >> items: >> - - description: main oscillator (24MHz) >> + - description: main oscillator (24MHz) or CLK_VIDEO_PLL > > thead,th1520-clk-ap gets also VIDEO_PLL? Aren't both serving the same > purpose from these devices point of view? Bindings are telling what this > device is expecting. Since thead,th1520-clk-ap configures PLL clocks it takes the oscillator 24MHz as an input, so no. The VO subsystem takes as an input VIDEO_PLL that's configured by the AP. I could do something like this if this needs to be formally expressed in the schema: if: properties: compatible: contains: const: thead,th1520-clk-ap then: properties: clocks: description: main oscillator (24MHz) if: properties: compatible: contains: const: thead,th1520-clk-vo then: properties: clocks: description: VIDEO_PLL (derived from AP) for the VO clock controller. > >> >> "#clock-cells": >> const: 1 >> @@ -51,3 +54,10 @@ examples: >> clocks = <&osc>; >> #clock-cells = <1>; >> }; >> + >> + clock-controller@ff010000 { >> + compatible = "thead,th1520-clk-vo"; > > Difference in one property does not justify new example. If there is > goign to be resend, just drop. > > >> + reg = <0xff010000 0x1000>; >> + clocks = <&clk CLK_VIDEO_PLL>; >> + #clock-cells = <1>; > > Best regards, > Krzysztof > >
On 1/21/25 10:56, Krzysztof Kozlowski wrote: > On Mon, Jan 20, 2025 at 06:20:57PM +0100, Michal Wilczynski wrote: >> +#include <linux/firmware/thead/thead,th1520-aon.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/mailbox_controller.h> >> +#include <linux/mod_devicetable.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> + >> +#include <dt-bindings/firmware/thead,th1520-aon.h> > > How/where do you use this header? Indeed, it's used by the power-domain driver, so it's not needed here. > >> + >> +#define MAX_RX_TIMEOUT (msecs_to_jiffies(3000)) >> +#define MAX_TX_TIMEOUT 500 >> + >> +struct th1520_aon_chan { >> + struct platform_device *pd; >> + struct mbox_chan *ch; >> + struct th1520_aon_rpc_ack_common ack_msg; >> + struct mbox_client cl; >> + struct completion done; >> + >> + /* make sure only one RPC is perfomed at a time */ >> + struct mutex transaction_lock; >> +}; >> + > > ... > >> +static int th1520_aon_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct th1520_aon_chan *aon_chan; >> + struct mbox_client *cl; >> + int ret; >> + struct platform_device_info pdevinfo = { >> + .name = "th1520-pd", >> + .id = PLATFORM_DEVID_AUTO, >> + .parent = dev, >> + }; >> + >> + aon_chan = devm_kzalloc(dev, sizeof(*aon_chan), GFP_KERNEL); >> + if (!aon_chan) >> + return -ENOMEM; >> + >> + cl = &aon_chan->cl; >> + cl->dev = dev; >> + cl->tx_block = true; >> + cl->tx_tout = MAX_TX_TIMEOUT; >> + cl->rx_callback = th1520_aon_rx_callback; >> + >> + aon_chan->ch = mbox_request_channel_byname(cl, "aon"); >> + if (IS_ERR(aon_chan->ch)) { >> + ret = PTR_ERR(aon_chan->ch); > > Drop, pointless. The entire point of using dev_err_probe is to make it > simple. > >> + return dev_err_probe(dev, ret, "Failed to request aon mbox chan\n"); >> + } >> + >> + mutex_init(&aon_chan->transaction_lock); >> + init_completion(&aon_chan->done); >> + >> + platform_set_drvdata(pdev, aon_chan); >> + >> + aon_chan->pd = platform_device_register_full(&pdevinfo); >> + ret = PTR_ERR_OR_ZERO(aon_chan->pd); >> + if (ret) { >> + dev_err(dev, "Failed to register child device 'th1520-pd': %d\n", ret); >> + goto free_mbox_chan; >> + } >> + >> + ret = devm_of_platform_populate(dev); >> + if (ret) >> + goto unregister_pd; >> + >> + return 0; >> + >> +unregister_pd: >> + platform_device_unregister(aon_chan->pd); >> +free_mbox_chan: >> + mbox_free_channel(aon_chan->ch); >> + >> + return ret; >> +} >> + >> +static void th1520_aon_remove(struct platform_device *pdev) >> +{ >> + struct th1520_aon_chan *aon_chan = platform_get_drvdata(pdev); >> + >> + platform_device_unregister(aon_chan->pd); >> + mbox_free_channel(aon_chan->ch); >> +} >> + >> +static const struct of_device_id th1520_aon_match[] = { >> + { .compatible = "thead,th1520-aon" }, >> + { /* Sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, th1520_aon_match); >> + >> +static struct platform_driver th1520_aon_driver = { >> + .driver = { >> + .name = "th1520-aon", >> + .of_match_table = th1520_aon_match, >> + }, >> + .probe = th1520_aon_probe, >> + .remove = th1520_aon_remove, >> +}; >> +module_platform_driver(th1520_aon_driver); >> + >> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>"); >> +MODULE_DESCRIPTION("T-HEAD TH1520 Always-On firmware driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/firmware/thead/thead,th1520-aon.h b/include/linux/firmware/thead/thead,th1520-aon.h >> new file mode 100644 >> index 000000000000..3daa17c01d17 >> --- /dev/null >> +++ b/include/linux/firmware/thead/thead,th1520-aon.h >> @@ -0,0 +1,186 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (C) 2021 Alibaba Group Holding Limited. >> + */ >> + >> +#ifndef _THEAD_AON_H >> +#define _THEAD_AON_H >> + >> +#include <linux/device.h> >> +#include <linux/types.h> >> + >> +#define AON_RPC_MSG_MAGIC (0xef) >> +#define TH1520_AON_RPC_VERSION 2 >> +#define TH1520_AON_RPC_MSG_NUM 7 >> + >> +extern struct th1520_aon_chan *aon_chan; > > Drop all externs. > > > Best regards, > Krzysztof > >
On 1/21/25 09:35, Philipp Zabel wrote: > On Mo, 2025-01-20 at 18:21 +0100, Michal Wilczynski wrote: >> Add a YAML schema for the T-HEAD TH1520 SoC reset controller. This >> controller manages resets for subsystems such as the GPU within the >> TH1520 SoC. > > This mentions "resets", plural, but the #reset-cells = <0> below and > the driver implementation look like there only is a single reset > control (for the GPU). > >> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> >> --- >> .../bindings/reset/thead,th1520-reset.yaml | 44 +++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 45 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml >> >> diff --git a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml >> new file mode 100644 >> index 000000000000..c15a80e00935 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml >> @@ -0,0 +1,44 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: https://protect2.fireeye.com/v1/url?k=9a1e91c0-fb9584d9-9a1f1a8f-74fe485cbfec-4ac5a7f48f7ed305&q=1&e=57e2ad34-940c-48d4-b365-a5719457bd20&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Freset%2Fthead%2Cth1520-reset.yaml%23 >> +$schema: https://protect2.fireeye.com/v1/url?k=40dd1447-2156015e-40dc9f08-74fe485cbfec-5ae5fe2734d49263&q=1&e=57e2ad34-940c-48d4-b365-a5719457bd20&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >> + >> +title: T-HEAD TH1520 SoC Reset Controller >> + >> +description: >> + The T-HEAD TH1520 reset controller is a hardware block that asserts/deasserts >> + resets for SoC subsystems. > > Again, plural. Yeah should be singular sorry. > >> + >> +maintainers: >> + - Michal Wilczynski <m.wilczynski@samsung.com> >> + >> +properties: >> + compatible: >> + enum: >> + - thead,th1520-reset >> + >> + reg: >> + maxItems: 1 >> + >> + "#reset-cells": >> + const: 0 > > Should this be "const: 1" instead? Right now I'm not planning to extend by more resets, I've thought about this during the discussion on v2 of this patchset. At this point I just can't see more interesting resets to have. Vendor kernel implements WDT and NPU. I don't think NPU driver will be upstream anytime soon. That would leave WDT reset potentially. > > regards > Philipp > >