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

[Krzysztof Kozlowski]

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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxx>
>>>>> +  - Odelu Kukatla <okukatla@xxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx/
>>> [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