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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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