| Message ID | 20241223125553.3527812-4-m.wilczynski@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/19] dt-bindings: clock: Add VO subsystem clocks and update address requirements | expand |
On Mon, 23 Dec 2024 13:55:37 +0100, Michal Wilczynski wrote: > Introduce a device tree binding for the T-HEAD TH1520 power domain > controller. This allows devices to specify their power domain > dependencies, thereby improving power management for subsystems such as > the GPU. The new YAML schema describes the power domain node for the > T-HEAD TH1520 SoC. > > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> > --- > .../bindings/power/thead,th1520-power.yaml | 42 +++++++++++++++++++ > MAINTAINERS | 1 + > .../dt-bindings/power/thead,th1520-power.h | 18 ++++++++ > 3 files changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml > create mode 100644 include/dt-bindings/power/thead,th1520-power.h > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: /example-0/firmware/aon: failed to match any schema with compatible: ['thead,th1520-aon'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241223125553.3527812-4-m.wilczynski@samsung.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 12/23/24 16:16, Rob Herring (Arm) wrote: > > On Mon, 23 Dec 2024 13:55:37 +0100, Michal Wilczynski wrote: >> Introduce a device tree binding for the T-HEAD TH1520 power domain >> controller. This allows devices to specify their power domain >> dependencies, thereby improving power management for subsystems such as >> the GPU. The new YAML schema describes the power domain node for the >> T-HEAD TH1520 SoC. >> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> >> --- >> .../bindings/power/thead,th1520-power.yaml | 42 +++++++++++++++++++ >> MAINTAINERS | 1 + >> .../dt-bindings/power/thead,th1520-power.h | 18 ++++++++ >> 3 files changed, 61 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml >> create mode 100644 include/dt-bindings/power/thead,th1520-power.h >> > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: /example-0/firmware/aon: failed to match any schema with compatible: ['thead,th1520-aon'] It is introduced in the next commit in this patch series.
On 23/12/2024 17:02, Michal Wilczynski wrote: >>> .../bindings/power/thead,th1520-power.yaml | 42 +++++++++++++++++++ >>> MAINTAINERS | 1 + >>> .../dt-bindings/power/thead,th1520-power.h | 18 ++++++++ >>> 3 files changed, 61 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml >>> create mode 100644 include/dt-bindings/power/thead,th1520-power.h >>> >> >> My bot found errors running 'make dt_binding_check' on your patch: >> >> yamllint warnings/errors: >> >> dtschema/dtc warnings/errors: >> Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: /example-0/firmware/aon: failed to match any schema with compatible: ['thead,th1520-aon'] > > It is introduced in the next commit in this patch series. Then your patchset is not bisectable. Reorder patches. Best regards, Krzysztof
On 23/12/2024 13:55, Michal Wilczynski wrote: > + compatible: > + const: thead,th1520-pd > + > + "#power-domain-cells": > + const: 1 > + > +additionalProperties: false > + > +required: > + - compatible > + - "#power-domain-cells" > + > +examples: > + - | > + firmware { Drop > + aon: aon { > + compatible = "thead,th1520-aon"; > + mboxes = <&mbox_910t 1>; > + mbox-names = "aon"; Drop aon node... but the main problem is you do not have any resources in your power-domain device node, assuming your binding is complete. This suggests that this is part of aon, not separate device. Fold the device node into its parent (so everything goes to AON). > + > + pd: power-domain { > + compatible = "thead,th1520-pd"; > + #power-domain-cells = <1>; > + }; > + }; > + }; Best regards, Krzysztof
On 12/23/24 17:09, Krzysztof Kozlowski wrote: > On 23/12/2024 13:55, Michal Wilczynski wrote: >> + compatible: >> + const: thead,th1520-pd >> + >> + "#power-domain-cells": >> + const: 1 >> + >> +additionalProperties: false >> + >> +required: >> + - compatible >> + - "#power-domain-cells" >> + >> +examples: >> + - | >> + firmware { > > Drop > >> + aon: aon { >> + compatible = "thead,th1520-aon"; >> + mboxes = <&mbox_910t 1>; >> + mbox-names = "aon"; > > Drop aon node... but the main problem is you do not have any resources > in your power-domain device node, assuming your binding is complete. > This suggests that this is part of aon, not separate device. Fold the > device node into its parent (so everything goes to AON). Merging everything to AON node would definitely work. I was looking at the other implementations of firmware protocols for example, and that's how I figured the current implementation: arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi soc { firmware: firmware { compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; mboxes = <&mailbox>; firmware_clocks: clocks { compatible = "raspberrypi,firmware-clocks"; #clock-cells = <1>; }; }; power: power { compatible = "raspberrypi,bcm2835-power"; firmware = <&firmware>; #power-domain-cells = <1>; }; }; This is fairly similar, as the firmware is passed as property, instead as in a parent-child relationship. Would you consider it more canonical ? I would be happy to merge everything to AON node, and merge the power-domain driver and AON driver together, but it seemed to me like those could use some separation, and since power-domain and the AON represent actual HW it's fine to represent them in the device tree. Thanks, Michał > >> + >> + pd: power-domain { >> + compatible = "thead,th1520-pd"; >> + #power-domain-cells = <1>; >> + }; >> + }; >> + }; > > Best regards, > Krzysztof >
On 24/12/2024 10:31, Michal Wilczynski wrote: > > > On 12/23/24 17:09, Krzysztof Kozlowski wrote: >> On 23/12/2024 13:55, Michal Wilczynski wrote: >>> + compatible: >>> + const: thead,th1520-pd >>> + >>> + "#power-domain-cells": >>> + const: 1 >>> + >>> +additionalProperties: false >>> + >>> +required: >>> + - compatible >>> + - "#power-domain-cells" >>> + >>> +examples: >>> + - | >>> + firmware { >> >> Drop >> >>> + aon: aon { >>> + compatible = "thead,th1520-aon"; >>> + mboxes = <&mbox_910t 1>; >>> + mbox-names = "aon"; >> >> Drop aon node... but the main problem is you do not have any resources >> in your power-domain device node, assuming your binding is complete. >> This suggests that this is part of aon, not separate device. Fold the >> device node into its parent (so everything goes to AON). > > Merging everything to AON node would definitely work. I was looking at > the other implementations of firmware protocols for example, and that's > how I figured the current implementation: > > arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi > soc { > firmware: firmware { > compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; > mboxes = <&mailbox>; That's a 10 year old binding. Not even reviewed initially by DT maintainers. > > firmware_clocks: clocks { > compatible = "raspberrypi,firmware-clocks"; > #clock-cells = <1>; > }; > }; > > power: power { > compatible = "raspberrypi,bcm2835-power"; > firmware = <&firmware>; > #power-domain-cells = <1>; > }; > }; > > This is fairly similar, as the firmware is passed as property, instead > as in a parent-child relationship. Would you consider it more canonical > ? I consider early Raspberry bindings as antipattern. Same as many other early approaches and that's not an accusation to their authors. Just rules changed and capacity of DT maintainers increased. > > I would be happy to merge everything to AON node, and merge the > power-domain driver and AON driver together, but it seemed to me like > those could use some separation, and since power-domain and the AON > represent actual HW it's fine to represent them in the device tree. > I said nothing about drivers and I did not suggest merging drivers. Device nodes should not be split because they have different functions. This is one single device with multiple functions. Just like hundreds of others, e.g. clock, power domain and reset controller - all in one. Lack of separate address space is the main aspect here indicating this is not a separate device. So merge the device nodes. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/power/thead,th1520-power.yaml b/Documentation/devicetree/bindings/power/thead,th1520-power.yaml new file mode 100644 index 000000000000..28ff86f2cb05 --- /dev/null +++ b/Documentation/devicetree/bindings/power/thead,th1520-power.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-power.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: T-HEAD TH1520 Power Domain + +description: | + The T-HEAD TH1520 SoC includes a power domain controller responsible for + managing the power states of various hardware domains such as the GPU. + +maintainers: + - Michal Wilczynski <m.wilczynski@samsung.com> + +properties: + compatible: + const: thead,th1520-pd + + "#power-domain-cells": + const: 1 + +additionalProperties: false + +required: + - compatible + - "#power-domain-cells" + +examples: + - | + firmware { + aon: aon { + compatible = "thead,th1520-aon"; + mboxes = <&mbox_910t 1>; + mbox-names = "aon"; + + pd: power-domain { + compatible = "thead,th1520-pd"; + #power-domain-cells = <1>; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 910305c11e8a..c0e9639ac210 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20192,6 +20192,7 @@ F: Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.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-power.yaml F: arch/riscv/boot/dts/thead/ F: drivers/clk/thead/clk-th1520-ap.c F: drivers/mailbox/mailbox-th1520.c diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h new file mode 100644 index 000000000000..45835f25010d --- /dev/null +++ b/include/dt-bindings/power/thead,th1520-power.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* + * Copyright (C) 2022 Alibaba Group Holding Limited. + * Copyright (c) 2024 Samsung Electronics Co., Ltd. + * Author: Michal Wilczynski <m.wilczynski@samsung.com> + */ + +#ifndef __DT_BINDINGS_POWER_TH1520_H +#define __DT_BINDINGS_POWER_TH1520_H + +#define TH1520_AON_VDEC_PD 1 +#define TH1520_AON_NPU_PD 2 +#define TH1520_AON_VENC_PD 3 +#define TH1520_AON_GPU_PD 4 +#define TH1520_AON_DSP0_PD 5 +#define TH1520_AON_DSP1_PD 6 + +#endif
Introduce a device tree binding for the T-HEAD TH1520 power domain controller. This allows devices to specify their power domain dependencies, thereby improving power management for subsystems such as the GPU. The new YAML schema describes the power domain node for the T-HEAD TH1520 SoC. Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> --- .../bindings/power/thead,th1520-power.yaml | 42 +++++++++++++++++++ MAINTAINERS | 1 + .../dt-bindings/power/thead,th1520-power.h | 18 ++++++++ 3 files changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml create mode 100644 include/dt-bindings/power/thead,th1520-power.h