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 <[email protected]>
> ---
> 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 <[email protected]>
> + - Odelu Kukatla <[email protected]>
Is this valid email address?
> +
> +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?
> +
> + qcom,bcm-voters:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
Please implement my previous comments.
> + maxItems: 1
> + description: |
No need for |
> + 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?
> + description: |
Ditto.
> + 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 <[email protected]>
> +
> +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?
> +
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +patternProperties:
This goes after "properties".
> + '^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
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + - Odelu Kukatla <[email protected]>
>
> 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/[email protected]/
[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 <[email protected]>
> > +
> > +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