lkml.org 
[lkml]   [2022]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 2/5] dt-bindings: interconnect: Add Qualcomm SM6350 NoC support
From
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

\
 
 \ /
  Last update: 2022-05-20 16:25    [W:0.181 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site