diff mbox series

[v2,2/5] dt-bindings: interconnect: Add Qualcomm SM6350 NoC support

Message ID 20220520070318.48521-3-luca.weiss@fairphone.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series Add interconnect support for SM6350 | expand

Commit Message

Luca Weiss May 20, 2022, 7:03 a.m. UTC
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@fairphone.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

Comments

Krzysztof Kozlowski May 20, 2022, 10:31 a.m. UTC | #1
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@fairphone.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@linaro.org>
> +  - Odelu Kukatla <okukatla@codeaurora.org>

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 <luca.weiss@fairphone.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?

> +
> +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
Luca Weiss May 20, 2022, 12:04 p.m. UTC | #2
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@fairphone.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@linaro.org>
> > +  - Odelu Kukatla <okukatla@codeaurora.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@fairphone.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
Krzysztof Kozlowski May 20, 2022, 2:24 p.m. UTC | #3
On 20/05/2022 14:04, Luca Weiss wrote:
> 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@fairphone.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@linaro.org>
>>> +  - Odelu Kukatla <okukatla@codeaurora.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

Hm, indeed driver supports variable values. It's fine then.

> 
>>
>>> +
>>> +  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?

Let's go with maxItems:1, for both fields.

> 
>>
>>> +      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

The bindings example uses apps and disp, so here would be only "apps".

>>> +
>>> +  '#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.

Yes, and the $ref defines it as [1, 2], so initially I thought this
should be narrowed. However it seems 1 or 2 are still valid for all of
Qcom interconnects, so your "true" is correct.


Best regards,
Krzysztof
Luca Weiss May 23, 2022, 2:32 p.m. UTC | #4
Hi Krzysztof,

On Fri May 20, 2022 at 4:24 PM CEST, Krzysztof Kozlowski wrote:
> On 20/05/2022 14:04, Luca Weiss wrote:
> > 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@fairphone.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@linaro.org>
> >>> +  - Odelu Kukatla <okukatla@codeaurora.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
>
> Hm, indeed driver supports variable values. It's fine then.
>
> > 
> >>
> >>> +
> >>> +  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?
>
> Let's go with maxItems:1, for both fields.

Do you mean adjusting the example using:

  qcom,bcm-voter-names = "apps", "disp";
  qcom,bcm-voters = <&apps_bcm_voter>, <&disp_bcm_voter>;

in qcom,rpmh.yaml then? Otherwise validation fails with maxItems: 1

>
> > 
> >>
> >>> +      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
>
> The bindings example uses apps and disp, so here would be only "apps".

Here also the above, allow only "apps" for now in the binding and remove
"disp" from example?

Regards
Luca

>
> >>> +
> >>> +  '#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.
>
> Yes, and the $ref defines it as [1, 2], so initially I thought this
> should be narrowed. However it seems 1 or 2 are still valid for all of
> Qcom interconnects, so your "true" is correct.
>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski May 24, 2022, 5:30 p.m. UTC | #5
On 23/05/2022 16:32, Luca Weiss wrote:
> Hi Krzysztof,
> 
> On Fri May 20, 2022 at 4:24 PM CEST, Krzysztof Kozlowski wrote:
>> On 20/05/2022 14:04, Luca Weiss wrote:
>>> 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@fairphone.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@linaro.org>
>>>>> +  - Odelu Kukatla <okukatla@codeaurora.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
>>
>> Hm, indeed driver supports variable values. It's fine then.
>>
>>>
>>>>
>>>>> +
>>>>> +  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?
>>
>> Let's go with maxItems:1, for both fields.
> 
> Do you mean adjusting the example using:
> 
>   qcom,bcm-voter-names = "apps", "disp";
>   qcom,bcm-voters = <&apps_bcm_voter>, <&disp_bcm_voter>;
> 
> in qcom,rpmh.yaml then? Otherwise validation fails with maxItems: 1
> 
>>
>>>
>>>>
>>>>> +      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
>>
>> The bindings example uses apps and disp, so here would be only "apps".
> 
> Here also the above, allow only "apps" for now in the binding and remove
> "disp" from example?

I actually don't know what is the proper value, so choose a reasonable
constraint matching existing sources. Since example uses two of them,
then maybe "maxItems:2"?


Best regards,
Krzysztof
diff mbox series

Patch

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@linaro.org>
+  - Odelu Kukatla <okukatla@codeaurora.org>
+
+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 ]
+
+  qcom,bcm-voters:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      maxItems: 1
+    description: |
+      List of phandles to qcom,bcm-voter nodes that are required by
+      this interconnect to send RPMh commands.
+
+  qcom,bcm-voter-names:
+    description: |
+      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@fairphone.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
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+patternProperties:
+  '^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
+
+    required:
+      - compatible
+
+    unevaluatedProperties: false
+
+examples:
+  - |
+    config_noc: interconnect@1500000 {
+        compatible = "qcom,sm6350-config-noc";
+        reg = <0x01500000 0x28000>;
+        #interconnect-cells = <2>;
+        qcom,bcm-voters = <&apps_bcm_voter>;
+    };
+
+    system_noc: interconnect@1620000 {
+        compatible = "qcom,sm6350-system-noc";
+        reg = <0x01620000 0x17080>;
+        #interconnect-cells = <2>;
+        qcom,bcm-voters = <&apps_bcm_voter>;
+
+        clk_virt: interconnect-clk-virt {
+            compatible = "qcom,sm6350-clk-virt";
+            #interconnect-cells = <2>;
+            qcom,bcm-voters = <&apps_bcm_voter>;
+        };
+    };
diff --git a/include/dt-bindings/interconnect/qcom,sm6350.h b/include/dt-bindings/interconnect/qcom,sm6350.h
new file mode 100644
index 000000000000..e662cede9aaa
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,sm6350.h
@@ -0,0 +1,148 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
+/*
+ * Qualcomm SM6350 interconnect IDs
+ *
+ * Copyright (C) 2022 Luca Weiss <luca.weiss@fairphone.com>
+ */
+
+#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_SM6350_H
+#define __DT_BINDINGS_INTERCONNECT_QCOM_SM6350_H
+
+#define MASTER_A1NOC_CFG		0
+#define MASTER_QUP_0			1
+#define MASTER_EMMC			2
+#define MASTER_UFS_MEM			3
+#define A1NOC_SNOC_SLV			4
+#define SLAVE_SERVICE_A1NOC		5
+
+#define MASTER_A2NOC_CFG		0
+#define MASTER_QDSS_BAM			1
+#define MASTER_QUP_1			2
+#define MASTER_CRYPTO_CORE_0		3
+#define MASTER_IPA			4
+#define MASTER_QDSS_ETR			5
+#define MASTER_SDCC_2			6
+#define MASTER_USB3			7
+#define A2NOC_SNOC_SLV			8
+#define SLAVE_SERVICE_A2NOC		9
+
+#define MASTER_CAMNOC_HF0_UNCOMP	0
+#define MASTER_CAMNOC_ICP_UNCOMP	1
+#define MASTER_CAMNOC_SF_UNCOMP		2
+#define MASTER_QUP_CORE_0		3
+#define MASTER_QUP_CORE_1		4
+#define MASTER_LLCC			5
+#define SLAVE_CAMNOC_UNCOMP		6
+#define SLAVE_QUP_CORE_0		7
+#define SLAVE_QUP_CORE_1		8
+#define SLAVE_EBI_CH0			9
+
+#define MASTER_NPU			0
+#define MASTER_NPU_PROC			1
+#define SLAVE_CDSP_GEM_NOC		2
+
+#define SNOC_CNOC_MAS			0
+#define MASTER_QDSS_DAP			1
+#define SLAVE_A1NOC_CFG			2
+#define SLAVE_A2NOC_CFG			3
+#define SLAVE_AHB2PHY			4
+#define SLAVE_AHB2PHY_2			5
+#define SLAVE_AOSS			6
+#define SLAVE_BOOT_ROM			7
+#define SLAVE_CAMERA_CFG		8
+#define SLAVE_CAMERA_NRT_THROTTLE_CFG	9
+#define SLAVE_CAMERA_RT_THROTTLE_CFG	10
+#define SLAVE_CLK_CTL			11
+#define SLAVE_RBCPR_CX_CFG		12
+#define SLAVE_RBCPR_MX_CFG		13
+#define SLAVE_CRYPTO_0_CFG		14
+#define SLAVE_DCC_CFG			15
+#define SLAVE_CNOC_DDRSS		16
+#define SLAVE_DISPLAY_CFG		17
+#define SLAVE_DISPLAY_THROTTLE_CFG	18
+#define SLAVE_EMMC_CFG			19
+#define SLAVE_GLM			20
+#define SLAVE_GRAPHICS_3D_CFG		21
+#define SLAVE_IMEM_CFG			22
+#define SLAVE_IPA_CFG			23
+#define SLAVE_CNOC_MNOC_CFG		24
+#define SLAVE_CNOC_MSS			25
+#define SLAVE_NPU_CFG			26
+#define SLAVE_PDM			27
+#define SLAVE_PIMEM_CFG			28
+#define SLAVE_PRNG			29
+#define SLAVE_QDSS_CFG			30
+#define SLAVE_QM_CFG			31
+#define SLAVE_QM_MPU_CFG		32
+#define SLAVE_QUP_0			33
+#define SLAVE_QUP_1			34
+#define SLAVE_SDCC_2			35
+#define SLAVE_SECURITY			36
+#define SLAVE_SNOC_CFG			37
+#define SLAVE_TCSR			38
+#define SLAVE_UFS_MEM_CFG		39
+#define SLAVE_USB3			40
+#define SLAVE_VENUS_CFG			41
+#define SLAVE_VENUS_THROTTLE_CFG	42
+#define SLAVE_VSENSE_CTRL_CFG		43
+#define SLAVE_SERVICE_CNOC		44
+
+#define MASTER_CNOC_DC_NOC		0
+#define SLAVE_GEM_NOC_CFG		1
+#define SLAVE_LLCC_CFG			2
+
+#define MASTER_AMPSS_M0			0
+#define MASTER_SYS_TCU			1
+#define MASTER_GEM_NOC_CFG		2
+#define MASTER_COMPUTE_NOC		3
+#define MASTER_MNOC_HF_MEM_NOC		4
+#define MASTER_MNOC_SF_MEM_NOC		5
+#define MASTER_SNOC_GC_MEM_NOC		6
+#define MASTER_SNOC_SF_MEM_NOC		7
+#define MASTER_GRAPHICS_3D		8
+#define SLAVE_MCDMA_MS_MPU_CFG		9
+#define SLAVE_MSS_PROC_MS_MPU_CFG	10
+#define SLAVE_GEM_NOC_SNOC		11
+#define SLAVE_LLCC			12
+#define SLAVE_SERVICE_GEM_NOC		13
+
+#define MASTER_CNOC_MNOC_CFG		0
+#define MASTER_VIDEO_P0			1
+#define MASTER_VIDEO_PROC		2
+#define MASTER_CAMNOC_HF		3
+#define MASTER_CAMNOC_ICP		4
+#define MASTER_CAMNOC_SF		5
+#define MASTER_MDP_PORT0		6
+#define SLAVE_MNOC_HF_MEM_NOC		7
+#define SLAVE_MNOC_SF_MEM_NOC		8
+#define SLAVE_SERVICE_MNOC		9
+
+#define MASTER_NPU_SYS			0
+#define MASTER_NPU_NOC_CFG		1
+#define SLAVE_NPU_CAL_DP0		2
+#define SLAVE_NPU_CP			3
+#define SLAVE_NPU_INT_DMA_BWMON_CFG	4
+#define SLAVE_NPU_DPM			5
+#define SLAVE_ISENSE_CFG		6
+#define SLAVE_NPU_LLM_CFG		7
+#define SLAVE_NPU_TCM			8
+#define SLAVE_NPU_COMPUTE_NOC		9
+#define SLAVE_SERVICE_NPU_NOC		10
+
+#define MASTER_SNOC_CFG			0
+#define A1NOC_SNOC_MAS			1
+#define A2NOC_SNOC_MAS			2
+#define MASTER_GEM_NOC_SNOC		3
+#define MASTER_PIMEM			4
+#define MASTER_GIC			5
+#define SLAVE_APPSS			6
+#define SNOC_CNOC_SLV			7
+#define SLAVE_SNOC_GEM_NOC_GC		8
+#define SLAVE_SNOC_GEM_NOC_SF		9
+#define SLAVE_OCIMEM			10
+#define SLAVE_PIMEM			11
+#define SLAVE_SERVICE_SNOC		12
+#define SLAVE_QDSS_STM			13
+#define SLAVE_TCU			14
+
+#endif