| Message ID | 20230718160833.36397-2-quic_nkela@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add qcom hvc/shmem transport | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, 18 Jul 2023 09:08:32 -0700, Nikunj Kela wrote: > Introduce compatible "qcom,scmi-hvc-shmem" for SCMI > transport channel for Qualcomm virtual platforms. > The compatible mandates a shared memory channel. > > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/firmware/arm,scmi.example.dts:194.31-32 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/firmware/arm,scmi.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2 make: *** [Makefile:234: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230718160833.36397-2-quic_nkela@quicinc.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 18/07/2023 18:08, Nikunj Kela wrote: > Introduce compatible "qcom,scmi-hvc-shmem" for SCMI > transport channel for Qualcomm virtual platforms. > The compatible mandates a shared memory channel. > > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index b138f3d23df8..605b1e997a85 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -45,6 +45,9 @@ properties: > - description: SCMI compliant firmware with OP-TEE transport > items: > - const: linaro,scmi-optee > + - description: SCMI compliant firmware with Qualcomm hvc/shmem transport > + items: > + - const: qcom,scmi-hvc-shmem > > interrupts: > description: > @@ -321,6 +324,16 @@ else: > required: > - linaro,optee-channel-id > > + else: > + if: > + properties: > + compatible: > + contains: > + const: qcom,scmi-hvc-shmem > + then: > + required: > + - shmem Unfortunately this pattern if-else-if-else-if-else does not scale well. Please convert all entries first to allOf:if:then,if:then,if:then (in new patch), and then add new if:then: > + > examples: > - | > firmware { > @@ -444,6 +457,62 @@ examples: > }; > }; > > + - | > + firmware { > + scmi_dpu { No underscores in node names. Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "qcom,scmi-hvc-shmem"; > + shmem = <&shmem_dpu>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + scmi_pd_dpu: protocol@11 { > + reg = <0x11>; > + #power-domain-cells = <1>; > + }; > + }; > + Add only one example, but then only if it differs significantly. I see no differences - except compatible - so maybe no point of examples. > + scmi_gpu { > + compatible = "qcom,scmi-hvc-shmem"; > + shmem = <&shmem_gpu>; This example for sure is not needed - you duplicate above. > + > + interrupts = <GIC_SPI 931 IRQ_TYPE_EDGE_RISING>; > + interrupt-names = "a2p"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + scmi_pd_gpu: protocol@11 { > + reg = <0x11>; > + #power-domain-cells = <1>; > + }; > + }; > + }; > + > + soc { > + #address-cells = <1>; > + #size-cells = <1>; > + > + sram@95c00000 { > + compatible = "mmio-sram"; > + reg = <0x95c00000 0x10000>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + shmem_dpu: scmi-sram-dpu@95c00000 { > + compatible = "arm,scmi-shmem"; > + reg = <0x95c00000 0x3f0>; > + }; How does these differ from existing example? Best regards, Krzysztof
On 7/18/2023 11:12 AM, Krzysztof Kozlowski wrote: > On 18/07/2023 18:08, Nikunj Kela wrote: >> Introduce compatible "qcom,scmi-hvc-shmem" for SCMI >> transport channel for Qualcomm virtual platforms. >> The compatible mandates a shared memory channel. >> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >> --- >> .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> index b138f3d23df8..605b1e997a85 100644 >> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> @@ -45,6 +45,9 @@ properties: >> - description: SCMI compliant firmware with OP-TEE transport >> items: >> - const: linaro,scmi-optee >> + - description: SCMI compliant firmware with Qualcomm hvc/shmem transport >> + items: >> + - const: qcom,scmi-hvc-shmem >> >> interrupts: >> description: >> @@ -321,6 +324,16 @@ else: >> required: >> - linaro,optee-channel-id >> >> + else: >> + if: >> + properties: >> + compatible: >> + contains: >> + const: qcom,scmi-hvc-shmem >> + then: >> + required: >> + - shmem > Unfortunately this pattern if-else-if-else-if-else does not scale well. > Please convert all entries first to allOf:if:then,if:then,if:then (in > new patch), and then add new if:then: Ok! > >> + >> examples: >> - | >> firmware { >> @@ -444,6 +457,62 @@ examples: >> }; >> }; >> >> + - | >> + firmware { >> + scmi_dpu { > No underscores in node names. > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation ACK! > > > >> + compatible = "qcom,scmi-hvc-shmem"; >> + shmem = <&shmem_dpu>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + scmi_pd_dpu: protocol@11 { >> + reg = <0x11>; >> + #power-domain-cells = <1>; >> + }; >> + }; >> + > Add only one example, but then only if it differs significantly. I see > no differences - except compatible - so maybe no point of examples. Other than the compatible, it also doesn't require smc-id, we read it from shmem region. Will remove examples. > > >> + scmi_gpu { >> + compatible = "qcom,scmi-hvc-shmem"; >> + shmem = <&shmem_gpu>; > This example for sure is not needed - you duplicate above. Right, will remove this example. > >> + >> + interrupts = <GIC_SPI 931 IRQ_TYPE_EDGE_RISING>; >> + interrupt-names = "a2p"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + scmi_pd_gpu: protocol@11 { >> + reg = <0x11>; >> + #power-domain-cells = <1>; >> + }; >> + }; >> + }; >> + >> + soc { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + sram@95c00000 { >> + compatible = "mmio-sram"; >> + reg = <0x95c00000 0x10000>; >> + >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + shmem_dpu: scmi-sram-dpu@95c00000 { >> + compatible = "arm,scmi-shmem"; >> + reg = <0x95c00000 0x3f0>; >> + }; > How does these differ from existing example? It doesn't. Will remove the example as suggested. Thanks > > Best regards, > Krzysztof >
On Tue, Jul 18, 2023 at 09:08:32AM -0700, Nikunj Kela wrote: > Introduce compatible "qcom,scmi-hvc-shmem" for SCMI > transport channel for Qualcomm virtual platforms. > The compatible mandates a shared memory channel. > > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index b138f3d23df8..605b1e997a85 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -45,6 +45,9 @@ properties: > - description: SCMI compliant firmware with OP-TEE transport > items: > - const: linaro,scmi-optee > + - description: SCMI compliant firmware with Qualcomm hvc/shmem transport > + items: > + - const: qcom,scmi-hvc-shmem > > interrupts: > description: > @@ -321,6 +324,16 @@ else: > required: > - linaro,optee-channel-id > > + else: > + if: > + properties: > + compatible: > + contains: > + const: qcom,scmi-hvc-shmem > + then: > + required: > + - shmem > + Since this was done after we merged the support recently for extension of SMC/HVC, I need the reason(s) why this can't be used and based on the response this is new change so it can't be because there is a product already supporting this. So for now, NACK until I get the response for these.
On 7/19/2023 3:39 AM, Sudeep Holla wrote: > On Tue, Jul 18, 2023 at 09:08:32AM -0700, Nikunj Kela wrote: >> Introduce compatible "qcom,scmi-hvc-shmem" for SCMI >> transport channel for Qualcomm virtual platforms. >> The compatible mandates a shared memory channel. >> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >> --- >> .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> index b138f3d23df8..605b1e997a85 100644 >> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml >> @@ -45,6 +45,9 @@ properties: >> - description: SCMI compliant firmware with OP-TEE transport >> items: >> - const: linaro,scmi-optee >> + - description: SCMI compliant firmware with Qualcomm hvc/shmem transport >> + items: >> + - const: qcom,scmi-hvc-shmem >> >> interrupts: >> description: >> @@ -321,6 +324,16 @@ else: >> required: >> - linaro,optee-channel-id >> >> + else: >> + if: >> + properties: >> + compatible: >> + contains: >> + const: qcom,scmi-hvc-shmem >> + then: >> + required: >> + - shmem >> + > Since this was done after we merged the support recently for extension of > SMC/HVC, I need the reason(s) why this can't be used and based on the response > this is new change so it can't be because there is a product already > supporting this. So for now, NACK until I get the response for these. Our hypervisor deals with objects and uses object-ids to identify them. The hvc doorbell object is independent of the shmem object created by hypervisor. Hypervisor treats them independently. With the patch you mentioned, hypervisor need to create an association between the doorbell object and shmem object which will make it SCMI specific change in hypervisor. Hypervisor doesn't really care if doorbell is for SCMI or for other purposes hence we are adding this new driver so it can work with our hypervisor ABIs specification.
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index b138f3d23df8..605b1e997a85 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -45,6 +45,9 @@ properties: - description: SCMI compliant firmware with OP-TEE transport items: - const: linaro,scmi-optee + - description: SCMI compliant firmware with Qualcomm hvc/shmem transport + items: + - const: qcom,scmi-hvc-shmem interrupts: description: @@ -321,6 +324,16 @@ else: required: - linaro,optee-channel-id + else: + if: + properties: + compatible: + contains: + const: qcom,scmi-hvc-shmem + then: + required: + - shmem + examples: - | firmware { @@ -444,6 +457,62 @@ examples: }; }; + - | + firmware { + scmi_dpu { + compatible = "qcom,scmi-hvc-shmem"; + shmem = <&shmem_dpu>; + + #address-cells = <1>; + #size-cells = <0>; + + scmi_pd_dpu: protocol@11 { + reg = <0x11>; + #power-domain-cells = <1>; + }; + }; + + scmi_gpu { + compatible = "qcom,scmi-hvc-shmem"; + shmem = <&shmem_gpu>; + + interrupts = <GIC_SPI 931 IRQ_TYPE_EDGE_RISING>; + interrupt-names = "a2p"; + + #address-cells = <1>; + #size-cells = <0>; + + scmi_pd_gpu: protocol@11 { + reg = <0x11>; + #power-domain-cells = <1>; + }; + }; + }; + + soc { + #address-cells = <1>; + #size-cells = <1>; + + sram@95c00000 { + compatible = "mmio-sram"; + reg = <0x95c00000 0x10000>; + + #address-cells = <1>; + #size-cells = <1>; + ranges; + + shmem_dpu: scmi-sram-dpu@95c00000 { + compatible = "arm,scmi-shmem"; + reg = <0x95c00000 0x3f0>; + }; + + shmem_gpu: scmi-sram-gpu@95c00400 { + compatible = "arm,scmi-shmem"; + reg = <0x95c00400 0x3f0>; + }; + }; + }; + - | firmware { scmi {
Introduce compatible "qcom,scmi-hvc-shmem" for SCMI transport channel for Qualcomm virtual platforms. The compatible mandates a shared memory channel. Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> --- .../bindings/firmware/arm,scmi.yaml | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+)