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