Message-Id: <CK4KPEWM9165.2LR9ZUG2GGK6Q@otso>
Date:   Fri, 20 May 2022 14:04:28 +0200
From:   "Luca Weiss" <luca.weiss@...rphone.com>
To:     "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>,
        <linux-arm-msm@...r.kernel.org>
Cc:     <~postmarketos/upstreaming@...ts.sr.ht>,
        <phone-devel@...r.kernel.org>, "Andy Gross" <agross@...nel.org>,
        "Bjorn Andersson" <bjorn.andersson@...aro.org>,
        "Georgi Djakov" <djakov@...nel.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        "Odelu Kukatla" <okukatla@...eaurora.org>,
        <linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/5] dt-bindings: interconnect: Add Qualcomm SM6350
 NoC support

Hi Krzysztof,

Thanks for the review!

On Fri May 20, 2022 at 12:31 PM CEST, Krzysztof Kozlowski wrote:
> On 20/05/2022 09:03, Luca Weiss wrote:
> > Add bindings for Qualcomm SM6350 Network-On-Chip interconnect devices.
> > 
> > As SM6350 has two pairs of NoCs sharing the same reg, allow this in the
> > binding documentation, as was done for qcm2290.
> > 
> > Because the main qcom,rpmh.yaml file is getting too complicated for our
> > use cases, create a new qcom,rpmh-common.yaml and a separate
> > qcom,sm6350-rpmh.yaml that defines our new bindings.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@...rphone.com>
> > ---
> > Changes since v1:
> > * Split sm6350 into separate yaml with new rpmh-common.yaml
> > 
> >  .../interconnect/qcom,rpmh-common.yaml        |  41 +++++
> >  .../interconnect/qcom,sm6350-rpmh.yaml        |  82 ++++++++++
> >  .../dt-bindings/interconnect/qcom,sm6350.h    | 148 ++++++++++++++++++
> >  3 files changed, 271 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,rpmh-common.yaml
> >  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml
> >  create mode 100644 include/dt-bindings/interconnect/qcom,sm6350.h
> > 
> > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh-common.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh-common.yaml
> > new file mode 100644
> > index 000000000000..6121eea3e87d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh-common.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interconnect/qcom,rpmh-common.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm RPMh Network-On-Chip Interconnect
> > +
> > +maintainers:
> > +  - Georgi Djakov <georgi.djakov@...aro.org>
> > +  - Odelu Kukatla <okukatla@...eaurora.org>
>
> Is this valid email address?

Will put Georgi and Bjorn as maintainers, as per your other email.

>
> > +
> > +description: |
> > +   RPMh interconnect providers support system bandwidth requirements through
> > +   RPMh hardware accelerators known as Bus Clock Manager (BCM). The provider is
> > +   able to communicate with the BCM through the Resource State Coordinator (RSC)
> > +   associated with each execution environment. Provider nodes must point to at
> > +   least one RPMh device child node pertaining to their RSC and each provider
> > +   can map to multiple RPMh resources.
> > +
> > +properties:
> > +  '#interconnect-cells':
> > +    enum: [ 1, 2 ]
>
> Why this is an enum?

As a start, just adding that the definitions are copied from
qcom,rpmh.yaml so it's not my invention :) Of course that doesn't mean
that it should be improved where possible!

Either value is supported by the driver (and used upstream). But perhaps
it can use a description to define what the 'parameters' mean.

The second (optional) parameters "is to support different bandwidth
configurations that are toggled by RPMh, depending on the power state of
the CPU."[0]

A commit message for sc7180 calls it the "tag information" and "The
consumers can specify the path tag as an additional argument to the
endpoints."[1]

Not sure how to properly describe the first property, I guess the
interconnect endpoint? Maybe Georgi can help here.


[0] https://lore.kernel.org/linux-arm-msm/b079a211-d387-7958-bbe2-c41cac00d269@kernel.org/
[1] https://git.kernel.org/torvalds/c/e23b122

>
> > +
> > +  qcom,bcm-voters:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
>
> Please implement my previous comments.

Sorry, I looked over the comment in v1.

As far as I can tell in current code only 1 item is used.

If the second parameter of_bcm_voter_get would be used as non-NULL then
qcom,bcm-voter-names gets looked up and the N-th value in qcom,bcm-voters
used. But currently qcom,bcm-voter-names is not actively used so only
one gets used.

Do you have a recommendation what to put here? A synthetic limit like
32 just to have a number there?

>
> > +      maxItems: 1
> > +    description: |
>
> No need for |

ack

>
> > +      List of phandles to qcom,bcm-voter nodes that are required by
> > +      this interconnect to send RPMh commands.
> > +
> > +  qcom,bcm-voter-names:
>
> What names do you expect here?

Currently unused in mainline but newer downstream kernels[2] use "hlos"
as first parameter, and e.g. "disp" as second one that goes to a
qcom,bcm-voter that's a child of disp_rsc. Not sure exactly what that
does.

[2] https://github.com/atomsand/android_kernel_qcom_devicetree/blob/a6d50810116e8314d64eb63b8862c207b974e0c7/qcom/waipio.dtsi#L1701-L1793

>
> > +    description: |
>
> Ditto.

ack

>
> > +      Names for each of the qcom,bcm-voters specified.
> > +
> > +required:
> > +  - '#interconnect-cells'
> > +  - qcom,bcm-voters
> > +
> > +additionalProperties: true
> > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml
> > new file mode 100644
> > index 000000000000..89fe17c31b8f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml
> > @@ -0,0 +1,82 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interconnect/qcom,sm6350-rpmh.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm SM6350 RPMh Network-On-Chip Interconnect
> > +
> > +maintainers:
> > +  - Luca Weiss <luca.weiss@...rphone.com>
> > +
> > +description: |
> > +  Qualcomm RPMh-based interconnect provider on SM6350.
> > +
> > +allOf:
> > +  - $ref: qcom,rpmh-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,sm6350-aggre1-noc
> > +      - qcom,sm6350-aggre2-noc
> > +      - qcom,sm6350-config-noc
> > +      - qcom,sm6350-dc-noc
> > +      - qcom,sm6350-gem-noc
> > +      - qcom,sm6350-mmss-noc
> > +      - qcom,sm6350-npu-noc
> > +      - qcom,sm6350-system-noc
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#interconnect-cells': true
>
> Since you defined it as enum in rpmh-common, you really expect here
> different values?

Doesn't ": true" here just mean we want the value from the allOf: -
$ref?
But we could in theory make interconnect-cells only accept <2> for
sm6350.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +patternProperties:
>
> This goes after "properties".

So above required & unevaluatedProperties? Will update.

Regards
Luca

>
> > +  '^interconnect-[a-z0-9\-]+$':
> > +    type: object
> > +    description:
> > +      The interconnect providers do not have a separate QoS register space,
> > +      but share parent's space.
> > +    $ref: qcom,rpmh-common.yaml#
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - qcom,sm6350-clk-virt
> > +          - qcom,sm6350-compute-noc
> > +
> > +      '#interconnect-cells': true
>
> Same problem.
>
> > +
> > +    required:
> > +      - compatible
> > +
> > +    unevaluatedProperties: false
> > +
>
>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists