Add a schema for the venus video encoder/decoder on the qcm2290.
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
.../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++
1 file changed, 127 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
new file mode 100644
index 000000000000..0371f8dd91a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
@@ -0,0 +1,127 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm QCM2290 Venus video encode and decode accelerators
+
+maintainers:
+ - Vikash Garodia <quic_vgarodia@quicinc.com>
+
+description:
+ The Venus AR50_LITE IP is a video encode and decode accelerator present
+ on Qualcomm platforms
+
+allOf:
+ - $ref: qcom,venus-common.yaml#
+
+properties:
+ compatible:
+ const: qcom,qcm2290-venus
+
+ power-domains:
+ maxItems: 3
+
+ power-domain-names:
+ items:
+ - const: venus
+ - const: vcodec0
+ - const: cx
+
+ clocks:
+ maxItems: 6
+
+ clock-names:
+ items:
+ - const: core
+ - const: iface
+ - const: bus
+ - const: throttle
+ - const: vcodec0_core
+ - const: vcodec0_bus
+
+ iommus:
+ minItems: 1
+ maxItems: 5
+
+ interconnects:
+ maxItems: 2
+
+ interconnect-names:
+ items:
+ - const: video-mem
+ - const: cpu-cfg
+
+ operating-points-v2: true
+ opp-table:
+ type: object
+
+required:
+ - compatible
+ - power-domain-names
+ - iommus
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/qcom,gcc-qcm2290.h>
+ #include <dt-bindings/interconnect/qcom,qcm2290.h>
+ #include <dt-bindings/interconnect/qcom,rpm-icc.h>
+ #include <dt-bindings/power/qcom-rpmpd.h>
+
+ venus: video-codec@5a00000 {
+ compatible = "qcom,qcm2290-venus";
+ reg = <0x5a00000 0xf0000>;
+ interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+
+ power-domains = <&gcc GCC_VENUS_GDSC>,
+ <&gcc GCC_VCODEC0_GDSC>,
+ <&rpmpd QCM2290_VDDCX>;
+ power-domain-names = "venus",
+ "vcodec0",
+ "cx";
+ operating-points-v2 = <&venus_opp_table>;
+
+ clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
+ <&gcc GCC_VIDEO_AHB_CLK>,
+ <&gcc GCC_VENUS_CTL_AXI_CLK>,
+ <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
+ <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
+ <&gcc GCC_VCODEC0_AXI_CLK>;
+ clock-names = "core",
+ "iface",
+ "bus",
+ "throttle",
+ "vcodec0_core",
+ "vcodec0_bus";
+
+ memory-region = <&pil_video_mem>;
+ iommus = <&apps_smmu 0x860 0x0>,
+ <&apps_smmu 0x880 0x0>,
+ <&apps_smmu 0x861 0x04>,
+ <&apps_smmu 0x863 0x0>,
+ <&apps_smmu 0x804 0xe0>;
+
+ interconnects = <&mmnrt_virt MASTER_VIDEO_P0 RPM_ALWAYS_TAG
+ &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>,
+ <&bimc MASTER_APPSS_PROC RPM_ACTIVE_TAG
+ &config_noc SLAVE_VENUS_CFG RPM_ACTIVE_TAG>;
+ interconnect-names = "video-mem",
+ "cpu-cfg";
+
+ venus_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-133333333 {
+ opp-hz = /bits/ 64 <133333333>;
+ required-opps = <&rpmpd_opp_low_svs>;
+ };
+ opp-240000000 {
+ opp-hz = /bits/ 64 <240000000>;
+ required-opps = <&rpmpd_opp_svs>;
+ };
+ };
+ };
--
2.34.1On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote:
> Add a schema for the venus video encoder/decoder on the qcm2290.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++
> 1 file changed, 127 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
> new file mode 100644
> index 000000000000..0371f8dd91a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
> @@ -0,0 +1,127 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QCM2290 Venus video encode and decode accelerators
> +
> +maintainers:
> + - Vikash Garodia <quic_vgarodia@quicinc.com>
Shouldn't you be on this list ? If you upstream a file I think you
should list yourself as responsible for its glory or its mess.
> +
> +description:
> + The Venus AR50_LITE IP is a video encode and decode accelerator present
> + on Qualcomm platforms
> +
> +allOf:
> + - $ref: qcom,venus-common.yaml#
> +
> +properties:
> + compatible:
> + const: qcom,qcm2290-venus
> +
> + power-domains:
> + maxItems: 3
> +
> + power-domain-names:
> + items:
> + - const: venus
> + - const: vcodec0
> + - const: cx
> +
> + clocks:
> + maxItems: 6
> +
> + clock-names:
> + items:
> + - const: core
> + - const: iface
> + - const: bus
> + - const: throttle
> + - const: vcodec0_core
> + - const: vcodec0_bus
> +
> + iommus:
> + minItems: 1
> + maxItems: 5
I'm confused to see this is still here
https://lore.kernel.org/linux-media/zk5cmielm4urfm22yszmjmwvi4mqvdsfthlonq6mij7rkijcsp@7evb3ejxuaj7/
I think Dima is right, what's the problem with declaring maxItems: 5 here ?
> + interconnects:
> + maxItems: 2
> +
> + interconnect-names:
> + items:
> + - const: video-mem
> + - const: cpu-cfg
> +
> + operating-points-v2: true
> + opp-table:
> + type: object
> +
> +required:
> + - compatible
> + - power-domain-names
> + - iommus
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/qcom,gcc-qcm2290.h>
> + #include <dt-bindings/interconnect/qcom,qcm2290.h>
> + #include <dt-bindings/interconnect/qcom,rpm-icc.h>
> + #include <dt-bindings/power/qcom-rpmpd.h>
Alphabetise includes for preference.
> +
> + venus: video-codec@5a00000 {
> + compatible = "qcom,qcm2290-venus";
> + reg = <0x5a00000 0xf0000>;
> + interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
> +
> + power-domains = <&gcc GCC_VENUS_GDSC>,
> + <&gcc GCC_VCODEC0_GDSC>,
> + <&rpmpd QCM2290_VDDCX>;
> + power-domain-names = "venus",
> + "vcodec0",
> + "cx";
> + operating-points-v2 = <&venus_opp_table>;
> +
> + clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
> + <&gcc GCC_VIDEO_AHB_CLK>,
> + <&gcc GCC_VENUS_CTL_AXI_CLK>,
> + <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
> + <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
> + <&gcc GCC_VCODEC0_AXI_CLK>;
> + clock-names = "core",
> + "iface",
> + "bus",
> + "throttle",
> + "vcodec0_core",
> + "vcodec0_bus";
> +
> + memory-region = <&pil_video_mem>;
> + iommus = <&apps_smmu 0x860 0x0>,
> + <&apps_smmu 0x880 0x0>,
> + <&apps_smmu 0x861 0x04>,
> + <&apps_smmu 0x863 0x0>,
> + <&apps_smmu 0x804 0xe0>;
You're listing five iommus.
I understand there's some disagreement about whether or not to list all
of the potential use-cases but, TBH I don't think those are good arguments.
Unless there's some technical prohibition I can't think of listing all
five maxItems:5 .. let's just do that.
> +
> + interconnects = <&mmnrt_virt MASTER_VIDEO_P0 RPM_ALWAYS_TAG
> + &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>,
> + <&bimc MASTER_APPSS_PROC RPM_ACTIVE_TAG
> + &config_noc SLAVE_VENUS_CFG RPM_ACTIVE_TAG>;
> + interconnect-names = "video-mem",
> + "cpu-cfg";
> +
> + venus_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-133333333 {
> + opp-hz = /bits/ 64 <133333333>;
> + required-opps = <&rpmpd_opp_low_svs>;
> + };
> + opp-240000000 {
> + opp-hz = /bits/ 64 <240000000>;
> + required-opps = <&rpmpd_opp_svs>;
> + };
> + };
> + };
---
bod
On 17/07/25 00:22:53, Bryan O'Donoghue wrote:
> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote:
> > Add a schema for the venus video encoder/decoder on the qcm2290.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > ---
> > .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
> > new file mode 100644
> > index 000000000000..0371f8dd91a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml
> > @@ -0,0 +1,127 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm QCM2290 Venus video encode and decode accelerators
> > +
> > +maintainers:
> > + - Vikash Garodia <quic_vgarodia@quicinc.com>
>
> Shouldn't you be on this list ? If you upstream a file I think you should
> list yourself as responsible for its glory or its mess.
happy to do it. The MAINTAINER's file covered all the files named
schemas/media/*venus* so my understanding was that I shouldn't.
if you/Vikash/Dikshita could confirm I will do on v8. thanks!.
>
> > +
> > +description:
> > + The Venus AR50_LITE IP is a video encode and decode accelerator present
> > + on Qualcomm platforms
> > +
> > +allOf:
> > + - $ref: qcom,venus-common.yaml#
> > +
> > +properties:
> > + compatible:
> > + const: qcom,qcm2290-venus
> > +
> > + power-domains:
> > + maxItems: 3
> > +
> > + power-domain-names:
> > + items:
> > + - const: venus
> > + - const: vcodec0
> > + - const: cx
> > +
> > + clocks:
> > + maxItems: 6
> > +
> > + clock-names:
> > + items:
> > + - const: core
> > + - const: iface
> > + - const: bus
> > + - const: throttle
> > + - const: vcodec0_core
> > + - const: vcodec0_bus
> > +
> > + iommus:
> > + minItems: 1
> > + maxItems: 5
>
> I'm confused to see this is still here
>
> https://lore.kernel.org/linux-media/zk5cmielm4urfm22yszmjmwvi4mqvdsfthlonq6mij7rkijcsp@7evb3ejxuaj7/
um, I didnt see this review comment - sorry about it Dmitry, I wished I
had. Right, there are 5 SIDs.
>
> I think Dima is right, what's the problem with declaring maxItems: 5
> here ?
none. will fix.
>
>
> > + interconnects:
> > + maxItems: 2
> > +
> > + interconnect-names:
> > + items:
> > + - const: video-mem
> > + - const: cpu-cfg
> > +
> > + operating-points-v2: true
> > + opp-table:
> > + type: object
> > +
> > +required:
> > + - compatible
> > + - power-domain-names
> > + - iommus
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/qcom,gcc-qcm2290.h>
> > + #include <dt-bindings/interconnect/qcom,qcm2290.h>
> > + #include <dt-bindings/interconnect/qcom,rpm-icc.h>
> > + #include <dt-bindings/power/qcom-rpmpd.h>
>
> Alphabetise includes for preference.
um this was auto-generated by the build tool. I'll fix.
>
> > +
> > + venus: video-codec@5a00000 {
> > + compatible = "qcom,qcm2290-venus";
> > + reg = <0x5a00000 0xf0000>;
> > + interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > + power-domains = <&gcc GCC_VENUS_GDSC>,
> > + <&gcc GCC_VCODEC0_GDSC>,
> > + <&rpmpd QCM2290_VDDCX>;
> > + power-domain-names = "venus",
> > + "vcodec0",
> > + "cx";
> > + operating-points-v2 = <&venus_opp_table>;
> > +
> > + clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
> > + <&gcc GCC_VIDEO_AHB_CLK>,
> > + <&gcc GCC_VENUS_CTL_AXI_CLK>,
> > + <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
> > + <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
> > + <&gcc GCC_VCODEC0_AXI_CLK>;
> > + clock-names = "core",
> > + "iface",
> > + "bus",
> > + "throttle",
> > + "vcodec0_core",
> > + "vcodec0_bus";
> > +
> > + memory-region = <&pil_video_mem>;
> > + iommus = <&apps_smmu 0x860 0x0>,
> > + <&apps_smmu 0x880 0x0>,
> > + <&apps_smmu 0x861 0x04>,
> > + <&apps_smmu 0x863 0x0>,
> > + <&apps_smmu 0x804 0xe0>;
>
> You're listing five iommus.
>
> I understand there's some disagreement about whether or not to list all of
> the potential use-cases but, TBH I don't think those are good arguments.
>
> Unless there's some technical prohibition I can't think of listing all five
> maxItems:5 .. let's just do that.
since the device tree should describe hardware and not policy, and the
driver seems to be able to ignore the unused SIDs I think this is the
right thing to do.
once secure buffer support is enabled, I think migration to a child node
model will be needed but AFAIK (please let me know if I am wrong!), this
approach will maintain future compatibility.
This sort of thing is where we are heading to:
video-codec@5a00000 {
compatible = "qcom,qcm2290-venus";
reg = <0x0 0x5a00000 0x0 0xf0000>;
iommus = <&apps_smmu 0x860 0x0>, <&apps_smmu 0x880 0x0>;
secure-buffers {
iommus = <&apps_smmu 0x861 0x04>,
<&apps_smmu 0x863 0x0>,
<&apps_smmu 0x804 0xe0>;
memory-region = <&pil_video_mem>;
};
};
>
> > +
> > + interconnects = <&mmnrt_virt MASTER_VIDEO_P0 RPM_ALWAYS_TAG
> > + &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>,
> > + <&bimc MASTER_APPSS_PROC RPM_ACTIVE_TAG
> > + &config_noc SLAVE_VENUS_CFG RPM_ACTIVE_TAG>;
> > + interconnect-names = "video-mem",
> > + "cpu-cfg";
> > +
> > + venus_opp_table: opp-table {
> > + compatible = "operating-points-v2";
> > +
> > + opp-133333333 {
> > + opp-hz = /bits/ 64 <133333333>;
> > + required-opps = <&rpmpd_opp_low_svs>;
> > + };
> > + opp-240000000 {
> > + opp-hz = /bits/ 64 <240000000>;
> > + required-opps = <&rpmpd_opp_svs>;
> > + };
> > + };
> > + };
> ---
> bod
On 17/07/2025 08:35, Jorge Ramirez wrote: > On 17/07/25 00:22:53, Bryan O'Donoghue wrote: >> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: >>> Add a schema for the venus video encoder/decoder on the qcm2290. >>> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> --- >>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ >>> 1 file changed, 127 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>> new file mode 100644 >>> index 000000000000..0371f8dd91a3 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>> @@ -0,0 +1,127 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators >>> + >>> +maintainers: >>> + - Vikash Garodia <quic_vgarodia@quicinc.com> >> >> Shouldn't you be on this list ? If you upstream a file I think you should >> list yourself as responsible for its glory or its mess. > > happy to do it. The MAINTAINER's file covered all the files named This should be the person(s) interested and caring about this hardware, which means: 1. Subsystem maintainers: no 2. Driver maintainers: usually yes 3. Author(s) of new hardware support: usually yes > schemas/media/*venus* so my understanding was that I shouldn't. I cannot comment why people decided to go one way or another in other code, but it as well could be just incorrect choice thinking only people in MAINTAINERS care about hardware. ... >>> + >>> + memory-region = <&pil_video_mem>; >>> + iommus = <&apps_smmu 0x860 0x0>, >>> + <&apps_smmu 0x880 0x0>, >>> + <&apps_smmu 0x861 0x04>, >>> + <&apps_smmu 0x863 0x0>, >>> + <&apps_smmu 0x804 0xe0>; >> >> You're listing five iommus. >> >> I understand there's some disagreement about whether or not to list all of >> the potential use-cases but, TBH I don't think those are good arguments. >> >> Unless there's some technical prohibition I can't think of listing all five >> maxItems:5 .. let's just do that. > > since the device tree should describe hardware and not policy, and the > driver seems to be able to ignore the unused SIDs I think this is the > right thing to do. It was never about the driver but about whether you should describe in DTS for non-secure world the entries which are secure world. The answer in general is that you can and there will be benefits (e.g. sharing DTS with secure world implementations). Best regards, Krzysztof
On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: > On 17/07/2025 08:35, Jorge Ramirez wrote: > > On 17/07/25 00:22:53, Bryan O'Donoghue wrote: > >> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: > >>> Add a schema for the venus video encoder/decoder on the qcm2290. > >>> > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> > >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > >>> --- > >>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ > >>> 1 file changed, 127 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > >>> new file mode 100644 > >>> index 000000000000..0371f8dd91a3 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > >>> @@ -0,0 +1,127 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators > >>> + > >>> +maintainers: > >>> + - Vikash Garodia <quic_vgarodia@quicinc.com> > >> > >> Shouldn't you be on this list ? If you upstream a file I think you should > >> list yourself as responsible for its glory or its mess. > > > > happy to do it. The MAINTAINER's file covered all the files named > > This should be the person(s) interested and caring about this hardware, > which means: > 1. Subsystem maintainers: no > 2. Driver maintainers: usually yes > 3. Author(s) of new hardware support: usually yes perfect, will do > > > schemas/media/*venus* so my understanding was that I shouldn't. > > I cannot comment why people decided to go one way or another in other > code, but it as well could be just incorrect choice thinking only people > in MAINTAINERS care about hardware. > > ... > > >>> + > >>> + memory-region = <&pil_video_mem>; > >>> + iommus = <&apps_smmu 0x860 0x0>, > >>> + <&apps_smmu 0x880 0x0>, > >>> + <&apps_smmu 0x861 0x04>, > >>> + <&apps_smmu 0x863 0x0>, > >>> + <&apps_smmu 0x804 0xe0>; > >> > >> You're listing five iommus. > >> > >> I understand there's some disagreement about whether or not to list all of > >> the potential use-cases but, TBH I don't think those are good arguments. > >> > >> Unless there's some technical prohibition I can't think of listing all five > >> maxItems:5 .. let's just do that. > > > > since the device tree should describe hardware and not policy, and the > > driver seems to be able to ignore the unused SIDs I think this is the > > right thing to do. > > > It was never about the driver but about whether you should describe in > DTS for non-secure world the entries which are secure world. The answer > in general is that you can and there will be benefits (e.g. sharing DTS > with secure world implementations). all right, sounds good then, thanks
On 17/07/25 13:16:31, Jorge Ramirez wrote: > On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: > > On 17/07/2025 08:35, Jorge Ramirez wrote: > > > On 17/07/25 00:22:53, Bryan O'Donoghue wrote: > > >> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: > > >>> Add a schema for the venus video encoder/decoder on the qcm2290. > > >>> > > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> > > >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > >>> --- > > >>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ > > >>> 1 file changed, 127 insertions(+) > > >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > >>> > > >>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > >>> new file mode 100644 > > >>> index 000000000000..0371f8dd91a3 > > >>> --- /dev/null > > >>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > >>> @@ -0,0 +1,127 @@ > > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > >>> +%YAML 1.2 > > >>> +--- > > >>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# > > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > >>> + > > >>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators > > >>> + > > >>> +maintainers: > > >>> + - Vikash Garodia <quic_vgarodia@quicinc.com> > > >> > > >> Shouldn't you be on this list ? If you upstream a file I think you should > > >> list yourself as responsible for its glory or its mess. > > > > > > happy to do it. The MAINTAINER's file covered all the files named > > > > This should be the person(s) interested and caring about this hardware, > > which means: > > 1. Subsystem maintainers: no > > 2. Driver maintainers: usually yes > > 3. Author(s) of new hardware support: usually yes > > perfect, will do > > > > > > schemas/media/*venus* so my understanding was that I shouldn't. > > > > I cannot comment why people decided to go one way or another in other > > code, but it as well could be just incorrect choice thinking only people > > in MAINTAINERS care about hardware. > > > > ... > > > > >>> + > > >>> + memory-region = <&pil_video_mem>; > > >>> + iommus = <&apps_smmu 0x860 0x0>, > > >>> + <&apps_smmu 0x880 0x0>, > > >>> + <&apps_smmu 0x861 0x04>, > > >>> + <&apps_smmu 0x863 0x0>, > > >>> + <&apps_smmu 0x804 0xe0>; > > >> > > >> You're listing five iommus. > > >> > > >> I understand there's some disagreement about whether or not to list all of > > >> the potential use-cases but, TBH I don't think those are good arguments. > > >> > > >> Unless there's some technical prohibition I can't think of listing all five > > >> maxItems:5 .. let's just do that. > > > > > > since the device tree should describe hardware and not policy, and the > > > driver seems to be able to ignore the unused SIDs I think this is the > > > right thing to do. > > > > > > It was never about the driver but about whether you should describe in > > DTS for non-secure world the entries which are secure world. The answer > > in general is that you can and there will be benefits (e.g. sharing DTS > > with secure world implementations). > > all right, sounds good then, thanks Not sure if I’ve shared this before, but following an internal discussion, I think it’s worth highlighting a functional dependency in the current kernel: - the driver only works if the first two IOMMUs in the list — the non-secure ones — are placed at the beginning. Reordering them breaks functionality, which introduces unexpected fragility. Regardless, this seems like a valid concern to me — a driver shouldn't rely on the order of phandles — and I just wanted to make sure you're aware of it before I post a v8 (likely sometime next week or the following, as I’ll be taking a short break soon). Do you consider this serious enough to be called out in the commit message, or is this kind of behavior accepted as-is - ie, do you know if the DT binding for iommus rely on the order?
On 17/07/2025 19:00, Jorge Ramirez wrote: > On 17/07/25 13:16:31, Jorge Ramirez wrote: >> On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: >>> On 17/07/2025 08:35, Jorge Ramirez wrote: >>>> On 17/07/25 00:22:53, Bryan O'Donoghue wrote: >>>>> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: >>>>>> Add a schema for the venus video encoder/decoder on the qcm2290. >>>>>> >>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> >>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>>>> --- >>>>>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ >>>>>> 1 file changed, 127 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>> new file mode 100644 >>>>>> index 000000000000..0371f8dd91a3 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>> @@ -0,0 +1,127 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>> + >>>>>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators >>>>>> + >>>>>> +maintainers: >>>>>> + - Vikash Garodia <quic_vgarodia@quicinc.com> >>>>> >>>>> Shouldn't you be on this list ? If you upstream a file I think you should >>>>> list yourself as responsible for its glory or its mess. >>>> >>>> happy to do it. The MAINTAINER's file covered all the files named >>> >>> This should be the person(s) interested and caring about this hardware, >>> which means: >>> 1. Subsystem maintainers: no >>> 2. Driver maintainers: usually yes >>> 3. Author(s) of new hardware support: usually yes >> >> perfect, will do >> >>> >>>> schemas/media/*venus* so my understanding was that I shouldn't. >>> >>> I cannot comment why people decided to go one way or another in other >>> code, but it as well could be just incorrect choice thinking only people >>> in MAINTAINERS care about hardware. >>> >>> ... >>> >>>>>> + >>>>>> + memory-region = <&pil_video_mem>; >>>>>> + iommus = <&apps_smmu 0x860 0x0>, >>>>>> + <&apps_smmu 0x880 0x0>, >>>>>> + <&apps_smmu 0x861 0x04>, >>>>>> + <&apps_smmu 0x863 0x0>, >>>>>> + <&apps_smmu 0x804 0xe0>; >>>>> >>>>> You're listing five iommus. >>>>> >>>>> I understand there's some disagreement about whether or not to list all of >>>>> the potential use-cases but, TBH I don't think those are good arguments. >>>>> >>>>> Unless there's some technical prohibition I can't think of listing all five >>>>> maxItems:5 .. let's just do that. >>>> >>>> since the device tree should describe hardware and not policy, and the >>>> driver seems to be able to ignore the unused SIDs I think this is the >>>> right thing to do. >>> >>> >>> It was never about the driver but about whether you should describe in >>> DTS for non-secure world the entries which are secure world. The answer >>> in general is that you can and there will be benefits (e.g. sharing DTS >>> with secure world implementations). >> >> all right, sounds good then, thanks > > Not sure if I’ve shared this before, but following an internal > discussion, I think it’s worth highlighting a functional dependency in > the current kernel: > > - the driver only works if the first two IOMMUs in the list — the > non-secure ones — are placed at the beginning. Reordering them breaks > functionality, which introduces unexpected fragility. > > Regardless, this seems like a valid concern to me — a driver shouldn't > rely on the order of phandles — and I just wanted to make sure you're > aware of it before I post a v8 (likely sometime next week or the > following, as I’ll be taking a short break soon). Hm? Order of lists is strictly defined. That's actually an overlook that we never do it for iommus, but the core rule stays. Best regards, Krzysztof
On 7/18/25 8:27 AM, Krzysztof Kozlowski wrote: > On 17/07/2025 19:00, Jorge Ramirez wrote: >> On 17/07/25 13:16:31, Jorge Ramirez wrote: >>> On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: >>>> On 17/07/2025 08:35, Jorge Ramirez wrote: >>>>> On 17/07/25 00:22:53, Bryan O'Donoghue wrote: >>>>>> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: >>>>>>> Add a schema for the venus video encoder/decoder on the qcm2290. >>>>>>> >>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> >>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>>>>> --- >>>>>>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ >>>>>>> 1 file changed, 127 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>> new file mode 100644 >>>>>>> index 000000000000..0371f8dd91a3 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>> @@ -0,0 +1,127 @@ >>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>> +%YAML 1.2 >>>>>>> +--- >>>>>>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>> + >>>>>>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators >>>>>>> + >>>>>>> +maintainers: >>>>>>> + - Vikash Garodia <quic_vgarodia@quicinc.com> >>>>>> >>>>>> Shouldn't you be on this list ? If you upstream a file I think you should >>>>>> list yourself as responsible for its glory or its mess. >>>>> >>>>> happy to do it. The MAINTAINER's file covered all the files named >>>> >>>> This should be the person(s) interested and caring about this hardware, >>>> which means: >>>> 1. Subsystem maintainers: no >>>> 2. Driver maintainers: usually yes >>>> 3. Author(s) of new hardware support: usually yes >>> >>> perfect, will do >>> >>>> >>>>> schemas/media/*venus* so my understanding was that I shouldn't. >>>> >>>> I cannot comment why people decided to go one way or another in other >>>> code, but it as well could be just incorrect choice thinking only people >>>> in MAINTAINERS care about hardware. >>>> >>>> ... >>>> >>>>>>> + >>>>>>> + memory-region = <&pil_video_mem>; >>>>>>> + iommus = <&apps_smmu 0x860 0x0>, >>>>>>> + <&apps_smmu 0x880 0x0>, >>>>>>> + <&apps_smmu 0x861 0x04>, >>>>>>> + <&apps_smmu 0x863 0x0>, >>>>>>> + <&apps_smmu 0x804 0xe0>; >>>>>> >>>>>> You're listing five iommus. >>>>>> >>>>>> I understand there's some disagreement about whether or not to list all of >>>>>> the potential use-cases but, TBH I don't think those are good arguments. >>>>>> >>>>>> Unless there's some technical prohibition I can't think of listing all five >>>>>> maxItems:5 .. let's just do that. >>>>> >>>>> since the device tree should describe hardware and not policy, and the >>>>> driver seems to be able to ignore the unused SIDs I think this is the >>>>> right thing to do. >>>> >>>> >>>> It was never about the driver but about whether you should describe in >>>> DTS for non-secure world the entries which are secure world. The answer >>>> in general is that you can and there will be benefits (e.g. sharing DTS >>>> with secure world implementations). >>> >>> all right, sounds good then, thanks >> >> Not sure if I’ve shared this before, but following an internal >> discussion, I think it’s worth highlighting a functional dependency in >> the current kernel: >> >> - the driver only works if the first two IOMMUs in the list — the >> non-secure ones — are placed at the beginning. Reordering them breaks >> functionality, which introduces unexpected fragility. >> >> Regardless, this seems like a valid concern to me — a driver shouldn't >> rely on the order of phandles — and I just wanted to make sure you're >> aware of it before I post a v8 (likely sometime next week or the >> following, as I’ll be taking a short break soon). > > > Hm? Order of lists is strictly defined. That's actually an overlook that > we never do it for iommus, but the core rule stays. (FWIW "items:" is an ordered list, "enum:" is unordered) Konrad
On 18/07/2025 12:02, Konrad Dybcio wrote: > On 7/18/25 8:27 AM, Krzysztof Kozlowski wrote: >> On 17/07/2025 19:00, Jorge Ramirez wrote: >>> On 17/07/25 13:16:31, Jorge Ramirez wrote: >>>> On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: >>>>> On 17/07/2025 08:35, Jorge Ramirez wrote: >>>>>> On 17/07/25 00:22:53, Bryan O'Donoghue wrote: >>>>>>> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: >>>>>>>> Add a schema for the venus video encoder/decoder on the qcm2290. >>>>>>>> >>>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> >>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>>>>>> --- >>>>>>>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ >>>>>>>> 1 file changed, 127 insertions(+) >>>>>>>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..0371f8dd91a3 >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>> @@ -0,0 +1,127 @@ >>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>>> +%YAML 1.2 >>>>>>>> +--- >>>>>>>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>> + >>>>>>>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators >>>>>>>> + >>>>>>>> +maintainers: >>>>>>>> + - Vikash Garodia <quic_vgarodia@quicinc.com> >>>>>>> >>>>>>> Shouldn't you be on this list ? If you upstream a file I think you should >>>>>>> list yourself as responsible for its glory or its mess. >>>>>> >>>>>> happy to do it. The MAINTAINER's file covered all the files named >>>>> >>>>> This should be the person(s) interested and caring about this hardware, >>>>> which means: >>>>> 1. Subsystem maintainers: no >>>>> 2. Driver maintainers: usually yes >>>>> 3. Author(s) of new hardware support: usually yes >>>> >>>> perfect, will do >>>> >>>>> >>>>>> schemas/media/*venus* so my understanding was that I shouldn't. >>>>> >>>>> I cannot comment why people decided to go one way or another in other >>>>> code, but it as well could be just incorrect choice thinking only people >>>>> in MAINTAINERS care about hardware. >>>>> >>>>> ... >>>>> >>>>>>>> + >>>>>>>> + memory-region = <&pil_video_mem>; >>>>>>>> + iommus = <&apps_smmu 0x860 0x0>, >>>>>>>> + <&apps_smmu 0x880 0x0>, >>>>>>>> + <&apps_smmu 0x861 0x04>, >>>>>>>> + <&apps_smmu 0x863 0x0>, >>>>>>>> + <&apps_smmu 0x804 0xe0>; >>>>>>> >>>>>>> You're listing five iommus. >>>>>>> >>>>>>> I understand there's some disagreement about whether or not to list all of >>>>>>> the potential use-cases but, TBH I don't think those are good arguments. >>>>>>> >>>>>>> Unless there's some technical prohibition I can't think of listing all five >>>>>>> maxItems:5 .. let's just do that. >>>>>> >>>>>> since the device tree should describe hardware and not policy, and the >>>>>> driver seems to be able to ignore the unused SIDs I think this is the >>>>>> right thing to do. >>>>> >>>>> >>>>> It was never about the driver but about whether you should describe in >>>>> DTS for non-secure world the entries which are secure world. The answer >>>>> in general is that you can and there will be benefits (e.g. sharing DTS >>>>> with secure world implementations). >>>> >>>> all right, sounds good then, thanks >>> >>> Not sure if I’ve shared this before, but following an internal >>> discussion, I think it’s worth highlighting a functional dependency in >>> the current kernel: >>> >>> - the driver only works if the first two IOMMUs in the list — the >>> non-secure ones — are placed at the beginning. Reordering them breaks >>> functionality, which introduces unexpected fragility. >>> >>> Regardless, this seems like a valid concern to me — a driver shouldn't >>> rely on the order of phandles — and I just wanted to make sure you're >>> aware of it before I post a v8 (likely sometime next week or the >>> following, as I’ll be taking a short break soon). >> >> >> Hm? Order of lists is strictly defined. That's actually an overlook that >> we never do it for iommus, but the core rule stays. > > (FWIW "items:" is an ordered list, "enum:" is unordered) enum is not a list, but enumeration, meaning one item of multiple values. Best regards, Krzysztof
On 7/18/25 12:04 PM, Krzysztof Kozlowski wrote: > On 18/07/2025 12:02, Konrad Dybcio wrote: >> On 7/18/25 8:27 AM, Krzysztof Kozlowski wrote: >>> On 17/07/2025 19:00, Jorge Ramirez wrote: >>>> On 17/07/25 13:16:31, Jorge Ramirez wrote: >>>>> On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: >>>>>> On 17/07/2025 08:35, Jorge Ramirez wrote: >>>>>>> On 17/07/25 00:22:53, Bryan O'Donoghue wrote: >>>>>>>> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: >>>>>>>>> Add a schema for the venus video encoder/decoder on the qcm2290. >>>>>>>>> >>>>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> >>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>>>>>>> --- >>>>>>>>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ >>>>>>>>> 1 file changed, 127 insertions(+) >>>>>>>>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>>> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..0371f8dd91a3 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml >>>>>>>>> @@ -0,0 +1,127 @@ >>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>>>> +%YAML 1.2 >>>>>>>>> +--- >>>>>>>>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# >>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>>> + >>>>>>>>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators >>>>>>>>> + >>>>>>>>> +maintainers: >>>>>>>>> + - Vikash Garodia <quic_vgarodia@quicinc.com> >>>>>>>> >>>>>>>> Shouldn't you be on this list ? If you upstream a file I think you should >>>>>>>> list yourself as responsible for its glory or its mess. >>>>>>> >>>>>>> happy to do it. The MAINTAINER's file covered all the files named >>>>>> >>>>>> This should be the person(s) interested and caring about this hardware, >>>>>> which means: >>>>>> 1. Subsystem maintainers: no >>>>>> 2. Driver maintainers: usually yes >>>>>> 3. Author(s) of new hardware support: usually yes >>>>> >>>>> perfect, will do >>>>> >>>>>> >>>>>>> schemas/media/*venus* so my understanding was that I shouldn't. >>>>>> >>>>>> I cannot comment why people decided to go one way or another in other >>>>>> code, but it as well could be just incorrect choice thinking only people >>>>>> in MAINTAINERS care about hardware. >>>>>> >>>>>> ... >>>>>> >>>>>>>>> + >>>>>>>>> + memory-region = <&pil_video_mem>; >>>>>>>>> + iommus = <&apps_smmu 0x860 0x0>, >>>>>>>>> + <&apps_smmu 0x880 0x0>, >>>>>>>>> + <&apps_smmu 0x861 0x04>, >>>>>>>>> + <&apps_smmu 0x863 0x0>, >>>>>>>>> + <&apps_smmu 0x804 0xe0>; >>>>>>>> >>>>>>>> You're listing five iommus. >>>>>>>> >>>>>>>> I understand there's some disagreement about whether or not to list all of >>>>>>>> the potential use-cases but, TBH I don't think those are good arguments. >>>>>>>> >>>>>>>> Unless there's some technical prohibition I can't think of listing all five >>>>>>>> maxItems:5 .. let's just do that. >>>>>>> >>>>>>> since the device tree should describe hardware and not policy, and the >>>>>>> driver seems to be able to ignore the unused SIDs I think this is the >>>>>>> right thing to do. >>>>>> >>>>>> >>>>>> It was never about the driver but about whether you should describe in >>>>>> DTS for non-secure world the entries which are secure world. The answer >>>>>> in general is that you can and there will be benefits (e.g. sharing DTS >>>>>> with secure world implementations). >>>>> >>>>> all right, sounds good then, thanks >>>> >>>> Not sure if I’ve shared this before, but following an internal >>>> discussion, I think it’s worth highlighting a functional dependency in >>>> the current kernel: >>>> >>>> - the driver only works if the first two IOMMUs in the list — the >>>> non-secure ones — are placed at the beginning. Reordering them breaks >>>> functionality, which introduces unexpected fragility. >>>> >>>> Regardless, this seems like a valid concern to me — a driver shouldn't >>>> rely on the order of phandles — and I just wanted to make sure you're >>>> aware of it before I post a v8 (likely sometime next week or the >>>> following, as I’ll be taking a short break soon). >>> >>> >>> Hm? Order of lists is strictly defined. That's actually an overlook that >>> we never do it for iommus, but the core rule stays. >> >> (FWIW "items:" is an ordered list, "enum:" is unordered) > > enum is not a list, but enumeration, meaning one item of multiple values. Right, need more caffeine Konrad
On 18/07/2025 11:04, Krzysztof Kozlowski wrote: > On 18/07/2025 12:02, Konrad Dybcio wrote: >> On 7/18/25 8:27 AM, Krzysztof Kozlowski wrote: >>> On 17/07/2025 19:00, Jorge Ramirez wrote: >>>> On 17/07/25 13:16:31, Jorge Ramirez wrote: >>>> Not sure if I’ve shared this before, but following an internal >>>> discussion, I think it’s worth highlighting a functional dependency in >>>> the current kernel: >>>> >>>> - the driver only works if the first two IOMMUs in the list — the >>>> non-secure ones — are placed at the beginning. Reordering them breaks >>>> functionality, which introduces unexpected fragility. >>>> >>>> Regardless, this seems like a valid concern to me — a driver shouldn't >>>> rely on the order of phandles — and I just wanted to make sure you're >>>> aware of it before I post a v8 (likely sometime next week or the >>>> following, as I’ll be taking a short break soon). >>> >>> >>> Hm? Order of lists is strictly defined. That's actually an overlook that >>> we never do it for iommus, but the core rule stays. >> >> (FWIW "items:" is an ordered list, "enum:" is unordered) > > enum is not a list, but enumeration, meaning one item of multiple values. > > Best regards, > Krzysztof As Krzysztof says the ordering is strict. I think the right-thing-to-do is to document in the commit log the dependency. The final three entries are secure entries and the ordering is important. --- bod
On 18/07/25 11:21:07, Bryan O'Donoghue wrote: > On 18/07/2025 11:04, Krzysztof Kozlowski wrote: > > On 18/07/2025 12:02, Konrad Dybcio wrote: > > > On 7/18/25 8:27 AM, Krzysztof Kozlowski wrote: > > > > On 17/07/2025 19:00, Jorge Ramirez wrote: > > > > > On 17/07/25 13:16:31, Jorge Ramirez wrote: > > > > > > Not sure if I’ve shared this before, but following an internal > > > > > discussion, I think it’s worth highlighting a functional dependency in > > > > > the current kernel: > > > > > > > > > > - the driver only works if the first two IOMMUs in the list — the > > > > > non-secure ones — are placed at the beginning. Reordering them breaks > > > > > functionality, which introduces unexpected fragility. > > > > > > > > > > Regardless, this seems like a valid concern to me — a driver shouldn't > > > > > rely on the order of phandles — and I just wanted to make sure you're > > > > > aware of it before I post a v8 (likely sometime next week or the > > > > > following, as I’ll be taking a short break soon). > > > > > > > > > > > > Hm? Order of lists is strictly defined. That's actually an overlook that > > > > we never do it for iommus, but the core rule stays. > > > > > > (FWIW "items:" is an ordered list, "enum:" is unordered) > > > > enum is not a list, but enumeration, meaning one item of multiple values. > > > > Best regards, > > Krzysztof > > As Krzysztof says the ordering is strict. > > I think the right-thing-to-do is to document in the commit log the > dependency. > > The final three entries are secure entries and the ordering is important. sure, will do that > > --- > bod
On 17/07/25 19:00:22, Jorge Ramirez wrote: > On 17/07/25 13:16:31, Jorge Ramirez wrote: > > On 17/07/25 08:45:17, Krzysztof Kozlowski wrote: > > > On 17/07/2025 08:35, Jorge Ramirez wrote: > > > > On 17/07/25 00:22:53, Bryan O'Donoghue wrote: > > > >> On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: > > > >>> Add a schema for the venus video encoder/decoder on the qcm2290. > > > >>> > > > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> > > > >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > >>> --- > > > >>> .../bindings/media/qcom,qcm2290-venus.yaml | 127 ++++++++++++++++++ > > > >>> 1 file changed, 127 insertions(+) > > > >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > > >>> > > > >>> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > > >>> new file mode 100644 > > > >>> index 000000000000..0371f8dd91a3 > > > >>> --- /dev/null > > > >>> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml > > > >>> @@ -0,0 +1,127 @@ > > > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > >>> +%YAML 1.2 > > > >>> +--- > > > >>> +$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml# > > > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > >>> + > > > >>> +title: Qualcomm QCM2290 Venus video encode and decode accelerators > > > >>> + > > > >>> +maintainers: > > > >>> + - Vikash Garodia <quic_vgarodia@quicinc.com> > > > >> > > > >> Shouldn't you be on this list ? If you upstream a file I think you should > > > >> list yourself as responsible for its glory or its mess. > > > > > > > > happy to do it. The MAINTAINER's file covered all the files named > > > > > > This should be the person(s) interested and caring about this hardware, > > > which means: > > > 1. Subsystem maintainers: no > > > 2. Driver maintainers: usually yes > > > 3. Author(s) of new hardware support: usually yes > > > > perfect, will do > > > > > > > > > schemas/media/*venus* so my understanding was that I shouldn't. > > > > > > I cannot comment why people decided to go one way or another in other > > > code, but it as well could be just incorrect choice thinking only people > > > in MAINTAINERS care about hardware. > > > > > > ... > > > > > > >>> + > > > >>> + memory-region = <&pil_video_mem>; > > > >>> + iommus = <&apps_smmu 0x860 0x0>, > > > >>> + <&apps_smmu 0x880 0x0>, > > > >>> + <&apps_smmu 0x861 0x04>, > > > >>> + <&apps_smmu 0x863 0x0>, > > > >>> + <&apps_smmu 0x804 0xe0>; > > > >> > > > >> You're listing five iommus. > > > >> > > > >> I understand there's some disagreement about whether or not to list all of > > > >> the potential use-cases but, TBH I don't think those are good arguments. > > > >> > > > >> Unless there's some technical prohibition I can't think of listing all five > > > >> maxItems:5 .. let's just do that. > > > > > > > > since the device tree should describe hardware and not policy, and the > > > > driver seems to be able to ignore the unused SIDs I think this is the > > > > right thing to do. > > > > > > > > > It was never about the driver but about whether you should describe in > > > DTS for non-secure world the entries which are secure world. The answer > > > in general is that you can and there will be benefits (e.g. sharing DTS > > > with secure world implementations). > > > > all right, sounds good then, thanks > > Not sure if I’ve shared this before, but following an internal > discussion, I think it’s worth highlighting a functional dependency in > the current kernel: > > - the driver only works if the first two IOMMUs in the list — the > non-secure ones — are placed at the beginning. Reordering them breaks > functionality, which introduces unexpected fragility. by "the driver only works" I mean the firmware will fail to load otherwise (should have been clear about the failure mode). > > Regardless, this seems like a valid concern to me — a driver shouldn't > rely on the order of phandles — and I just wanted to make sure you're > aware of it before I post a v8 (likely sometime next week or the > following, as I’ll be taking a short break soon). > > Do you consider this serious enough to be called out in the commit > message, or is this kind of behavior accepted as-is - ie, do you know if > the DT binding for iommus rely on the order? >
© 2016 - 2025 Red Hat, Inc.