Re: [PATCH v4 03/20] soc: dt-bindings: qcom: add gpr bindings

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks Rob for the review,

On 13/08/2021 23:32, Rob Herring wrote:
> On Mon, Aug 09, 2021 at 12:23:22PM +0100, Srinivas Kandagatla wrote:
>> Qualcomm Generic Packet router aka GPR is the IPC mechanism found
>> in AudioReach next generation signal processing framework to perform
>> command and response messages between various processors.
>>
>> GPR has concepts of static and dynamic port, all static services like
>> APM (Audio Processing Manager), PRM (Proxy resource manager) have
>> fixed port numbers where as dynamic services like graphs have dynamic
>> port numbers which are allocated at runtime. All GPR packet messages
>> will have source and destination domain and port along with opcode
>> and payload.
>>
>> This support is added using existing APR driver to reuse most of
>> the code.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   .../bindings/soc/qcom/qcom,apr.yaml           | 92 ++++++++++++++++++-
>>   include/dt-bindings/soc/qcom,gpr.h            | 18 ++++
>>   2 files changed, 105 insertions(+), 5 deletions(-)
>>   create mode 100644 include/dt-bindings/soc/qcom,gpr.h
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,apr.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.yaml
>> index 12650f7084f4..59d8b4dce8b5 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,apr.yaml
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.yaml
>> @@ -4,14 +4,14 @@
>>   $id: "http://devicetree.org/schemas/soc/qcom/qcom,apr.yaml#"
>>   $schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>   
>> -title: Qualcomm APR (Asynchronous Packet Router) binding
>> +title: Qualcomm APR/GPR (Asynchronous/Generic Packet Router) binding
>>   
>>   maintainers:
>>     - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>   
>>   description: |
>> -  This binding describes the Qualcomm APR, APR is a IPC protocol for
>> -  communication between Application processor and QDSP. APR is mainly
>> +  This binding describes the Qualcomm APR/GPR, APR/GPR is a IPC protocol for
>> +  communication between Application processor and QDSP. APR/GPR is mainly
>>     used for audio/voice services on the QDSP.
>>   
>>   properties:
>> @@ -19,6 +19,7 @@ properties:
>>       enum:
>>         - qcom,apr
>>         - qcom,apr-v2
>> +      - qcom,gpr
>>   
>>     qcom,apr-domain:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>> @@ -33,13 +34,22 @@ properties:
>>           6 = Modem2 Domain
>>           7 = Application Processor2 Domain
>>   
>> +  qcom,gpr-domain:
> 
> When the next flavor comes out, we'll have qcom,foo-domain?

Am happy to generalize this to qcom,domain and make the qcom,apr-domain 
deprecated, if that is the direction you suggest?

> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
>> +    description:
>> +      Selects the processor domain for gpr
>> +        1 = Modem Domain
>> +        2 = Audio DSP Domain
>> +        3 = Application Processor Domain
>> +
>>     '#address-cells':
>>       const: 1
>>   
>>     '#size-cells':
>>       const: 0
>>   
>> -#APR Services
>> +#APR/GPR Services
>>   patternProperties:
>>     "^apr-service@[0-9a-e]$":
>>       type: object
>> @@ -86,9 +96,66 @@ patternProperties:
>>   
>>       additionalProperties: false
>>   
>> +  "^gpr-service@[0-9a-e]$":
> 
> And foo-service@...
> 
> Do you (the driver) care what the node name is?

not really, we can name it as service@

> 
>> +    type: object
>> +    description:
>> +      GPR node's client devices use subnodes for desired static port services.
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - qcom,q6apm
>> +          - qcom,q6prm
>> +
>> +      reg:
>> +        enum: [1, 2, 3, 4]
>> +        description:
>> +          GPR Service ID
>> +            1 = Audio Process Manager Service
>> +            2 = Proxy Resource Manager Service.
> 
> Looks like both reg and compatible encode what the service is.

yes, this is inline with what has been done with APR bindings.

> 
>> +            3 = AMDB Service.
>> +            4 = Voice processing manager.
>> +
>> +      qcom,protection-domain:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        description: protection domain service name and path for apr service
>> +          has dependency on.
>> +        items:
>> +          - const: avs/audio
>> +          - const: msm/adsp/audio_pd
> 
> Why are we redefining the same property? You've combined the binding but
> are still sharing almost nothing...

I agree, Its possible to remove these redefinition, I will try to clean 
this up properly in next spin.

--srini
> 
>> +
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>> +    additionalProperties: false
>> +
>>   required:
>>     - compatible
>> -  - qcom,apr-domain
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,apr-v2
>> +              - qcom,apr
>> +    then:
>> +      required:
>> +        - qcom,apr-domain
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,gpr
>> +    then:
>> +      required:
>> +        - qcom,gpr-domain
>>   
>>   additionalProperties: false
>>   
>> @@ -125,3 +192,18 @@ examples:
>>             qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
>>           };
>>       };
>> +
>> +  - |
>> +    #include <dt-bindings/soc/qcom,gpr.h>
>> +    gpr {
>> +        compatible = "qcom,gpr";
>> +        qcom,gpr-domain = <GPR_DOMAIN_ID_ADSP>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        gpr-service@1 {
>> +          compatible = "qcom,q6apm";
>> +          reg = <GPR_APM_MODULE_IID>;
>> +          qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
>> +        };
>> +    };
>> diff --git a/include/dt-bindings/soc/qcom,gpr.h b/include/dt-bindings/soc/qcom,gpr.h
>> new file mode 100644
>> index 000000000000..1c68906e079c
>> --- /dev/null
>> +++ b/include/dt-bindings/soc/qcom,gpr.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __DT_BINDINGS_QCOM_GPR_H
>> +#define __DT_BINDINGS_QCOM_GPR_H
>> +
>> +/* DOMAINS */
>> +
>> +#define GPR_DOMAIN_ID_MODEM	1
>> +#define GPR_DOMAIN_ID_ADSP	2
>> +#define GPR_DOMAIN_ID_APPS	3
>> +
>> +/* Static Services */
>> +
>> +#define GPR_APM_MODULE_IID		1
>> +#define GPR_PRM_MODULE_IID		2
>> +#define GPR_AMDB_MODULE_IID		3
>> +#define GPR_VCPM_MODULE_IID		4
>> +
>> +#endif /* __DT_BINDINGS_QCOM_GPR_H */
>> -- 
>> 2.21.0
>>
>>