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