| Message ID | 20250530-apr_14_for_sending-v3-1-83d5744d997c@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add TH1520 GPU support with power sequencing | expand |
On Fri, May 30, 2025 at 12:24 AM Michal Wilczynski <m.wilczynski@samsung.com> wrote: > > Introduce device tree bindings for a new power sequencer provider > dedicated to the T-HEAD TH1520 SoC's GPU. > > The thead,th1520-gpu-pwrseq compatible designates a node that will > manage the complex power-up and power-down sequence for the GPU. This > sequencer requires a handle to the GPU's clock generator reset line > (gpu-clkgen), which is specified in its device tree node. > > This binding will be used by a new pwrseq driver to abstract the > SoC specific power management details from the generic GPU driver. > > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> > --- > .../bindings/power/thead,th1520-pwrseq.yaml | 42 ++++++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 43 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/thead,th1520-pwrseq.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: T-HEAD TH1520 GPU Power Sequencer > + > +maintainers: > + - Michal Wilczynski <m.wilczynski@samsung.com> > + > +description: | > + This binding describes the power sequencer for the T-HEAD TH1520 GPU. > + This sequencer handles the specific power-up and power-down sequences > + required by the GPU, including managing clocks and resets from both the > + sequencer and the GPU device itself. > + > +properties: > + compatible: > + const: thead,th1520-gpu-pwrseq > + Before I review the rest: is this actually a physical device that takes care of the power sequencing? Some kind of a power management unit for the GPU? If so, I bet it's not called "power sequencer" so let's use its actual name as per the datasheet? Bart
On 6/2/25 16:46, Bartosz Golaszewski wrote: > On Fri, May 30, 2025 at 12:24 AM Michal Wilczynski > <m.wilczynski@samsung.com> wrote: >> >> Introduce device tree bindings for a new power sequencer provider >> dedicated to the T-HEAD TH1520 SoC's GPU. >> >> The thead,th1520-gpu-pwrseq compatible designates a node that will >> manage the complex power-up and power-down sequence for the GPU. This >> sequencer requires a handle to the GPU's clock generator reset line >> (gpu-clkgen), which is specified in its device tree node. >> >> This binding will be used by a new pwrseq driver to abstract the >> SoC specific power management details from the generic GPU driver. >> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> >> --- >> .../bindings/power/thead,th1520-pwrseq.yaml | 42 ++++++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 43 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml >> new file mode 100644 >> index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml >> @@ -0,0 +1,42 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: https://protect2.fireeye.com/v1/url?k=55ca3a77-34b7d20f-55cbb138-74fe485fffb1-4da99284aaf5bdf2&q=1&e=085ffc69-21ad-4abd-9147-970a308c8818&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpower%2Fthead%2Cth1520-pwrseq.yaml%23 >> +$schema: https://protect2.fireeye.com/v1/url?k=8e9b901c-efe67864-8e9a1b53-74fe485fffb1-c964471a6655716e&q=1&e=085ffc69-21ad-4abd-9147-970a308c8818&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >> + >> +title: T-HEAD TH1520 GPU Power Sequencer >> + >> +maintainers: >> + - Michal Wilczynski <m.wilczynski@samsung.com> >> + >> +description: | >> + This binding describes the power sequencer for the T-HEAD TH1520 GPU. >> + This sequencer handles the specific power-up and power-down sequences >> + required by the GPU, including managing clocks and resets from both the >> + sequencer and the GPU device itself. >> + >> +properties: >> + compatible: >> + const: thead,th1520-gpu-pwrseq >> + > > Before I review the rest: is this actually a physical device that > takes care of the power sequencing? Some kind of a power management > unit for the GPU? If so, I bet it's not called "power sequencer" so > let's use its actual name as per the datasheet? Hi Bart, Thanks for your feedback. The hardware block responsible for powering up the components in the TH1520 SoC datasheet is called AON (Always On). However, we already have a DT node named aon that serves as a power domain provider (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml). Following the discussion [1] about needing a separate DT node for the power sequencing capabilities of this AON block, and thinking further about it, I think the binding should be more generic. The AON block can manage power sequences for more than just the GPU (e.g. NPU, AUDIO, DSP). The compatible string could be updated like so: "thead,th1520-aon-pwrseq" And the description: " This binding describes the hardware capabilities within the Always-On (AON) block of the T-HEAD TH1520 SoC responsible for controlling and sequencing the power supply to various integrated peripherals, such as the GPU, NPU, Audio, and DSP. " The exact power architecture of the SoC is described in the chapter 6.3.2 (Power Architecture) [2]. The "VDEC/NPU/VENC/GPU/DSP Power Up/ Power Down" is described in chapter 6.4.2.3. [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/ [2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf > > Bart > Best regards,
On Mon, Jun 02, 2025 at 10:29:13PM GMT, Michal Wilczynski wrote: > >> +description: | > >> + This binding describes the power sequencer for the T-HEAD TH1520 GPU. > >> + This sequencer handles the specific power-up and power-down sequences > >> + required by the GPU, including managing clocks and resets from both the > >> + sequencer and the GPU device itself. > >> + > >> +properties: > >> + compatible: > >> + const: thead,th1520-gpu-pwrseq > >> + > > > > Before I review the rest: is this actually a physical device that > > takes care of the power sequencing? Some kind of a power management > > unit for the GPU? If so, I bet it's not called "power sequencer" so > > let's use its actual name as per the datasheet? > > Hi Bart, > Thanks for your feedback. > > The hardware block responsible for powering up the components in the > TH1520 SoC datasheet is called AON (Always On). However, we already have > a DT node named aon that serves as a power domain provider > (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml). So no. One device, one device node (sometimes with cildren nodes). You do not get another device node just because someone wrote incomplete binding or because driver looks differently. > > Following the discussion [1] about needing a separate DT node for the > power sequencing capabilities of this AON block, and thinking further > about it, I think the binding should be more generic. The AON block can > manage power sequences for more than just the GPU (e.g. NPU, AUDIO, > DSP). > > The compatible string could be updated like so: > "thead,th1520-aon-pwrseq" Should not be separate node, you already have one for AON. Best regards, Krzysztof
On Tue, Jun 3, 2025 at 3:19 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Jun 02, 2025 at 10:29:13PM GMT, Michal Wilczynski wrote: > > >> +description: | > > >> + This binding describes the power sequencer for the T-HEAD TH1520 GPU. > > >> + This sequencer handles the specific power-up and power-down sequences > > >> + required by the GPU, including managing clocks and resets from both the > > >> + sequencer and the GPU device itself. > > >> + > > >> +properties: > > >> + compatible: > > >> + const: thead,th1520-gpu-pwrseq > > >> + > > > > > > Before I review the rest: is this actually a physical device that > > > takes care of the power sequencing? Some kind of a power management > > > unit for the GPU? If so, I bet it's not called "power sequencer" so > > > let's use its actual name as per the datasheet? > > > > Hi Bart, > > Thanks for your feedback. > > > > The hardware block responsible for powering up the components in the > > TH1520 SoC datasheet is called AON (Always On). However, we already have > > a DT node named aon that serves as a power domain provider > > (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml). > > So no. One device, one device node (sometimes with cildren nodes). You > do not get another device node just because someone wrote incomplete > binding or because driver looks differently. > > > > > Following the discussion [1] about needing a separate DT node for the > > power sequencing capabilities of this AON block, and thinking further > > about it, I think the binding should be more generic. The AON block can > > manage power sequences for more than just the GPU (e.g. NPU, AUDIO, > > DSP). > > > > The compatible string could be updated like so: > > "thead,th1520-aon-pwrseq" > > Should not be separate node, you already have one for AON. > Agreed. And as far as implementation goes, you can have the same driver be a PM domain AND pwrseq provider. It just has to bind to the device node that represents an actual component, not a made-up "convenience" node. Bartosz
On Tue, Jun 3, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > The compatible string could be updated like so: > > > "thead,th1520-aon-pwrseq" > > > > Should not be separate node, you already have one for AON. > > > > Agreed. And as far as implementation goes, you can have the same > driver be a PM domain AND pwrseq provider. It just has to bind to the > device node that represents an actual component, not a made-up > "convenience" node. > I'm seeing that there's already a main driver under drivers/pmdomain/thead/th1520-pm-domains.c and a "logical sub-driver" under drivers/firmware/thead,th1520-aon.c which exposes th1520_aon_init() called by the former. Maybe just follow that pattern, add a module under drivers/power/sequencing/ called pwrseq-th1520-pwrseq.c and call its init function from the pm-domains module? Bart
On 6/3/25 16:49, Bartosz Golaszewski wrote: > On Tue, Jun 3, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>> >>>> The compatible string could be updated like so: >>>> "thead,th1520-aon-pwrseq" >>> >>> Should not be separate node, you already have one for AON. >>> >> >> Agreed. And as far as implementation goes, you can have the same >> driver be a PM domain AND pwrseq provider. It just has to bind to the >> device node that represents an actual component, not a made-up >> "convenience" node. >> > > I'm seeing that there's already a main driver under > drivers/pmdomain/thead/th1520-pm-domains.c and a "logical sub-driver" > under drivers/firmware/thead,th1520-aon.c which exposes > th1520_aon_init() called by the former. Maybe just follow that > pattern, add a module under drivers/power/sequencing/ called > pwrseq-th1520-pwrseq.c and call its init function from the pm-domains > module? Right, sorry I haven't noticed this and responded to previous message. Thanks for the direction ! > > Bart > Best regards,
On Tue, Jun 3, 2025 at 8:24 PM Michal Wilczynski <m.wilczynski@samsung.com> wrote: > > > > Agreed. And as far as implementation goes, you can have the same > > driver be a PM domain AND pwrseq provider. It just has to bind to the > > device node that represents an actual component, not a made-up > > "convenience" node. > > Sure - this can be done using existing AON node. > > To keep the pwrseq code organized in drivers/power/sequencing/, a > similar approach to our th1520-pd driver interfacing with the AON > firmware library (drivers/firmware/thead,th1520-aon.c) could work. > > The idea would be to treat code like pwrseq-thead-aon.c (changed from a > current pwrseq-thead-gpu.c) as a library. It would export its necessary > functions (e.g., for specific sequence init/deinit steps) using > EXPORT_SYMBOL_GPL. The main AON driver would then call these to provide > the pwrseq functionality. > > This will introduce a compile-time dependency, as expected. > > An alternative would be to keep the driver in drivers/power/sequencing/ > as a platform driver and start it up using, for example, an auxiliary > bus. This is similar to what the JH7110 clock driver > (drivers/clk/starfive/clk-starfive-jh7110-sys.c) is doing with a reset > driver. This could offer a cleaner separation of roles if that's > preferred. > > Please let me know which way would be preferred. I forgot the auxiliary bus is a thing. Yeah, definitely use that, it's more elegant than a library function IMO. Bart
diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml new file mode 100644 index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3 --- /dev/null +++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/thead,th1520-pwrseq.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: T-HEAD TH1520 GPU Power Sequencer + +maintainers: + - Michal Wilczynski <m.wilczynski@samsung.com> + +description: | + This binding describes the power sequencer for the T-HEAD TH1520 GPU. + This sequencer handles the specific power-up and power-down sequences + required by the GPU, including managing clocks and resets from both the + sequencer and the GPU device itself. + +properties: + compatible: + const: thead,th1520-gpu-pwrseq + + resets: + description: A phandle to the GPU clock generator reset. + maxItems: 1 + + reset-names: + const: gpu-clkgen + +required: + - compatible + - resets + - reset-names + +additionalProperties: false + +examples: + - | + gpu_pwrseq: pwrseq { + compatible = "thead,th1520-gpu-pwrseq"; + resets = <&rst 0>; + reset-names = "gpu-clkgen"; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 0d59a5910e632350a4d72a761c6c5ce1d3a1bc34..78e3067df1152929de638244b03264733d08556e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21355,6 +21355,7 @@ F: Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml +F: Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml F: Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml F: arch/riscv/boot/dts/thead/ F: drivers/clk/thead/clk-th1520-ap.c
Introduce device tree bindings for a new power sequencer provider dedicated to the T-HEAD TH1520 SoC's GPU. The thead,th1520-gpu-pwrseq compatible designates a node that will manage the complex power-up and power-down sequence for the GPU. This sequencer requires a handle to the GPU's clock generator reset line (gpu-clkgen), which is specified in its device tree node. This binding will be used by a new pwrseq driver to abstract the SoC specific power management details from the generic GPU driver. Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> --- .../bindings/power/thead,th1520-pwrseq.yaml | 42 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 43 insertions(+)