Re: [RFC V3 1/4] dt-bindings: firmware: Document bindings for ARM SCMI QCOM Vendor Protocol

[Sibi Sankar <quic_sibis@quicinc.com>]

On 7/9/24 22:02, Cristian Marussi wrote:
> On Wed, Jul 03, 2024 at 12:44:37AM +0530, Sibi Sankar wrote:
>> Document the various memory buses that can be monitored and scaled by the
>> memory latency governor hosted by the ARM SCMI QCOM Vendor protocol v1.0.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

Hey Christian,

Thanks for taking time to review the series :)

>> ---
> 
> Hi Sibi,
> 
> this series got a bit neglected...my bad...a few comments down below.
> 
>>
>> Adding a reg property in scmi-memlat.yaml seems incorrect/superfluous
>> but without it I see the following errors:
>>
>> Err Logs:
>> protocol@80: 'reg' does not match any of the regexes: '^memory-[0-9]$', 'pinctrl-[0-9]+'
>> protocol@80: Unevaluated properties are not allowed ('memory-0', 'memory-1', 'memory-2' were unexpected)
>>
>> v2:
>> * Drop container dvfs memlat container node. [Rob]
>> * Move scmi-memlat.yaml to protocol level given that a lot of vendors might end up
>>    using the same protocol number. [Rob]
>> * Replace qcom,cpulist with the standard "cpus" property. [Rob]
>> * Fix up compute-type/ipm-ceil required. [Rob]
>>
> 
> ...so there has been a lot of work around Vendor protos recently (as you
> have seen) and especially around the way we define the DT bindings to have
> multiple vendor protocols coexist with the same overlapping numbers.
> (the code-level coexistence is already in place as you've seen...)
> 
> I think some sort of agreement on HOW to render this in the bindings
> side was reached around a series from NXP...not sure if I am missing something
> here but this commit from Peng/NXP (if you have not seen it already):
> https://lore.kernel.org/linux-arm-kernel/20240621-imx95-bbm-misc-v2-v5-2-b85a6bf778cb@nxp.com/
> 
> ...it is a good example of how you can define your vendor specific part in
> a vendor specific binding files and then just add a single $ref line in
> the core binding Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> (and that has been successfully reviewd...)
> 
> Moreover, in that same series from Peng/NXP you could have a look at
> https://lore.kernel.org/linux-arm-kernel/20240621-imx95-bbm-misc-v2-v5-1-b85a6bf778cb@nxp.com/
> 
> that adds the Documentation for their Vendor protocols.
> Beside the final location in the tree for such docs, which is a detail
> we can settle later on our side too, I think that patch is a good example
> of the kind of vendor-protos Documentation Sudeep is expecting.
> 
> 
>>   .../bindings/firmware/arm,scmi.yaml           |  15 ++
>>   .../bindings/soc/qcom/qcom,scmi-memlat.yaml   | 242 ++++++++++++++++++
>>   include/dt-bindings/soc/qcom,scmi-vendor.h    |  22 ++
>>   3 files changed, 279 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>>   create mode 100644 include/dt-bindings/soc/qcom,scmi-vendor.h
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 4d823f3b1f0e..a4022682e5ca 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -284,6 +284,21 @@ properties:
>>       required:
>>         - reg
>>   
>> +  protocol@80:
>> +    type: object
>> +    allOf:
>> +      - $ref: '#/$defs/protocol-node'
>> +      - $ref: /schemas/soc/qcom/qcom,scmi-memlat.yaml#
>> +
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        const: 0x80
>> +
>> +    required:
>> +      - reg
>> +
> 
> ..here you should be able to just plant your $ref without redefining the
> protocol@80
> 
>>   additionalProperties: false
>>   
>>   $defs:
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>> new file mode 100644
>> index 000000000000..915a6bf5697f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,scmi-memlat.yaml
>> @@ -0,0 +1,242 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,scmi-memlat.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SCMI Memory Bus nodes
>> +
>> +maintainers:
>> +  - Sibi Sankar <quic_sibis@quicinc.com>
>> +
>> +description:
>> +  This binding describes the various memory buses that can be monitored and scaled
>> +  by memory latency governor running on the CPU Control Processor (SCMI controller).
>> +
> 
> ...and instead here you will define your protocols, compliant with the
> main protocol-node def and any specific vendor sub-properties that you
> should need....
> 
> ...the above example from NXP is probably more clear than any attempt of mine
> to explain this :P

ack, will adhere to the same in the next re-spin.

> 
> Thanks,
> Cristian
>