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 <[email protected]>
---
.../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
+
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 {
--
2.17.1
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 <[email protected]>
> ---
> .../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/[email protected]
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 <[email protected]>
> ---
> .../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 <[email protected]>
>> ---
>> .../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 <[email protected]>
> ---
> .../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.
--
Regards,
Sudeep
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 <[email protected]>
>> ---
>> .../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.