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

[Sibi Sankar]

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@xxxxxxxxxxx>

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@xxxxxxx/
>
> ...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@xxxxxxx/
>
> 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@xxxxxxxxxxx>
> > +
> > +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