| Message ID | 20220520070318.48521-3-luca.weiss@fairphone.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add interconnect support for SM6350 | expand |
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? > + > +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? > + > + qcom,bcm-voters: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + items: Please implement my previous comments. > + maxItems: 1 > + description: | No need for | > + 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? > + description: | Ditto. > + Names for each of the qcom,bcm-voters specified. > + > +required: > + - '#interconnect-cells' > + - qcom,bcm-voters > + > +additionalProperties: true > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml > new file mode 100644 > index 000000000000..89fe17c31b8f > --- /dev/null > +++ b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interconnect/qcom,sm6350-rpmh.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm SM6350 RPMh Network-On-Chip Interconnect > + > +maintainers: > + - Luca Weiss <luca.weiss@fairphone.com> > + > +description: | > + Qualcomm RPMh-based interconnect provider on SM6350. > + > +allOf: > + - $ref: qcom,rpmh-common.yaml# > + > +properties: > + compatible: > + enum: > + - qcom,sm6350-aggre1-noc > + - qcom,sm6350-aggre2-noc > + - qcom,sm6350-config-noc > + - qcom,sm6350-dc-noc > + - qcom,sm6350-gem-noc > + - qcom,sm6350-mmss-noc > + - qcom,sm6350-npu-noc > + - qcom,sm6350-system-noc > + > + reg: > + maxItems: 1 > + > + '#interconnect-cells': true Since you defined it as enum in rpmh-common, you really expect here different values? > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: false > + > +patternProperties: This goes after "properties". > + '^interconnect-[a-z0-9\-]+$': > + type: object > + description: > + The interconnect providers do not have a separate QoS register space, > + but share parent's space. > + $ref: qcom,rpmh-common.yaml# > + > + properties: > + compatible: > + enum: > + - qcom,sm6350-clk-virt > + - qcom,sm6350-compute-noc > + > + '#interconnect-cells': true Same problem. > + > + required: > + - compatible > + > + unevaluatedProperties: false > + Best regards, Krzysztof
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 > > > + > > + 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? > > > + 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 > > > + description: | > > Ditto. ack > > > + Names for each of the qcom,bcm-voters specified. > > + > > +required: > > + - '#interconnect-cells' > > + - qcom,bcm-voters > > + > > +additionalProperties: true > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml > > new file mode 100644 > > index 000000000000..89fe17c31b8f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml > > @@ -0,0 +1,82 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/interconnect/qcom,sm6350-rpmh.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm SM6350 RPMh Network-On-Chip Interconnect > > + > > +maintainers: > > + - Luca Weiss <luca.weiss@fairphone.com> > > + > > +description: | > > + Qualcomm RPMh-based interconnect provider on SM6350. > > + > > +allOf: > > + - $ref: qcom,rpmh-common.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - qcom,sm6350-aggre1-noc > > + - qcom,sm6350-aggre2-noc > > + - qcom,sm6350-config-noc > > + - qcom,sm6350-dc-noc > > + - qcom,sm6350-gem-noc > > + - qcom,sm6350-mmss-noc > > + - qcom,sm6350-npu-noc > > + - qcom,sm6350-system-noc > > + > > + reg: > > + maxItems: 1 > > + > > + '#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. > > > + > > +required: > > + - compatible > > + - reg > > + > > +unevaluatedProperties: false > > + > > +patternProperties: > > This goes after "properties". So above required & unevaluatedProperties? Will update. Regards Luca > > > + '^interconnect-[a-z0-9\-]+$': > > + type: object > > + description: > > + The interconnect providers do not have a separate QoS register space, > > + but share parent's space. > > + $ref: qcom,rpmh-common.yaml# > > + > > + properties: > > + compatible: > > + enum: > > + - qcom,sm6350-clk-virt > > + - qcom,sm6350-compute-noc > > + > > + '#interconnect-cells': true > > Same problem. > > > + > > + required: > > + - compatible > > + > > + unevaluatedProperties: false > > + > > > Best regards, > Krzysztof
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
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@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. 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 <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. > > 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
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> + +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 ] + + qcom,bcm-voters: + $ref: /schemas/types.yaml#/definitions/phandle-array + items: + maxItems: 1 + description: | + List of phandles to qcom,bcm-voter nodes that are required by + this interconnect to send RPMh commands. + + qcom,bcm-voter-names: + description: | + Names for each of the qcom,bcm-voters specified. + +required: + - '#interconnect-cells' + - qcom,bcm-voters + +additionalProperties: true diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml new file mode 100644 index 000000000000..89fe17c31b8f --- /dev/null +++ b/Documentation/devicetree/bindings/interconnect/qcom,sm6350-rpmh.yaml @@ -0,0 +1,82 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interconnect/qcom,sm6350-rpmh.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm SM6350 RPMh Network-On-Chip Interconnect + +maintainers: + - Luca Weiss <luca.weiss@fairphone.com> + +description: | + Qualcomm RPMh-based interconnect provider on SM6350. + +allOf: + - $ref: qcom,rpmh-common.yaml# + +properties: + compatible: + enum: + - qcom,sm6350-aggre1-noc + - qcom,sm6350-aggre2-noc + - qcom,sm6350-config-noc + - qcom,sm6350-dc-noc + - qcom,sm6350-gem-noc + - qcom,sm6350-mmss-noc + - qcom,sm6350-npu-noc + - qcom,sm6350-system-noc + + reg: + maxItems: 1 + + '#interconnect-cells': true + +required: + - compatible + - reg + +unevaluatedProperties: false + +patternProperties: + '^interconnect-[a-z0-9\-]+$': + type: object + description: + The interconnect providers do not have a separate QoS register space, + but share parent's space. + $ref: qcom,rpmh-common.yaml# + + properties: + compatible: + enum: + - qcom,sm6350-clk-virt + - qcom,sm6350-compute-noc + + '#interconnect-cells': true + + required: + - compatible + + unevaluatedProperties: false + +examples: + - | + config_noc: interconnect@1500000 { + compatible = "qcom,sm6350-config-noc"; + reg = <0x01500000 0x28000>; + #interconnect-cells = <2>; + qcom,bcm-voters = <&apps_bcm_voter>; + }; + + system_noc: interconnect@1620000 { + compatible = "qcom,sm6350-system-noc"; + reg = <0x01620000 0x17080>; + #interconnect-cells = <2>; + qcom,bcm-voters = <&apps_bcm_voter>; + + clk_virt: interconnect-clk-virt { + compatible = "qcom,sm6350-clk-virt"; + #interconnect-cells = <2>; + qcom,bcm-voters = <&apps_bcm_voter>; + }; + }; diff --git a/include/dt-bindings/interconnect/qcom,sm6350.h b/include/dt-bindings/interconnect/qcom,sm6350.h new file mode 100644 index 000000000000..e662cede9aaa --- /dev/null +++ b/include/dt-bindings/interconnect/qcom,sm6350.h @@ -0,0 +1,148 @@ +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */ +/* + * Qualcomm SM6350 interconnect IDs + * + * Copyright (C) 2022 Luca Weiss <luca.weiss@fairphone.com> + */ + +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_SM6350_H +#define __DT_BINDINGS_INTERCONNECT_QCOM_SM6350_H + +#define MASTER_A1NOC_CFG 0 +#define MASTER_QUP_0 1 +#define MASTER_EMMC 2 +#define MASTER_UFS_MEM 3 +#define A1NOC_SNOC_SLV 4 +#define SLAVE_SERVICE_A1NOC 5 + +#define MASTER_A2NOC_CFG 0 +#define MASTER_QDSS_BAM 1 +#define MASTER_QUP_1 2 +#define MASTER_CRYPTO_CORE_0 3 +#define MASTER_IPA 4 +#define MASTER_QDSS_ETR 5 +#define MASTER_SDCC_2 6 +#define MASTER_USB3 7 +#define A2NOC_SNOC_SLV 8 +#define SLAVE_SERVICE_A2NOC 9 + +#define MASTER_CAMNOC_HF0_UNCOMP 0 +#define MASTER_CAMNOC_ICP_UNCOMP 1 +#define MASTER_CAMNOC_SF_UNCOMP 2 +#define MASTER_QUP_CORE_0 3 +#define MASTER_QUP_CORE_1 4 +#define MASTER_LLCC 5 +#define SLAVE_CAMNOC_UNCOMP 6 +#define SLAVE_QUP_CORE_0 7 +#define SLAVE_QUP_CORE_1 8 +#define SLAVE_EBI_CH0 9 + +#define MASTER_NPU 0 +#define MASTER_NPU_PROC 1 +#define SLAVE_CDSP_GEM_NOC 2 + +#define SNOC_CNOC_MAS 0 +#define MASTER_QDSS_DAP 1 +#define SLAVE_A1NOC_CFG 2 +#define SLAVE_A2NOC_CFG 3 +#define SLAVE_AHB2PHY 4 +#define SLAVE_AHB2PHY_2 5 +#define SLAVE_AOSS 6 +#define SLAVE_BOOT_ROM 7 +#define SLAVE_CAMERA_CFG 8 +#define SLAVE_CAMERA_NRT_THROTTLE_CFG 9 +#define SLAVE_CAMERA_RT_THROTTLE_CFG 10 +#define SLAVE_CLK_CTL 11 +#define SLAVE_RBCPR_CX_CFG 12 +#define SLAVE_RBCPR_MX_CFG 13 +#define SLAVE_CRYPTO_0_CFG 14 +#define SLAVE_DCC_CFG 15 +#define SLAVE_CNOC_DDRSS 16 +#define SLAVE_DISPLAY_CFG 17 +#define SLAVE_DISPLAY_THROTTLE_CFG 18 +#define SLAVE_EMMC_CFG 19 +#define SLAVE_GLM 20 +#define SLAVE_GRAPHICS_3D_CFG 21 +#define SLAVE_IMEM_CFG 22 +#define SLAVE_IPA_CFG 23 +#define SLAVE_CNOC_MNOC_CFG 24 +#define SLAVE_CNOC_MSS 25 +#define SLAVE_NPU_CFG 26 +#define SLAVE_PDM 27 +#define SLAVE_PIMEM_CFG 28 +#define SLAVE_PRNG 29 +#define SLAVE_QDSS_CFG 30 +#define SLAVE_QM_CFG 31 +#define SLAVE_QM_MPU_CFG 32 +#define SLAVE_QUP_0 33 +#define SLAVE_QUP_1 34 +#define SLAVE_SDCC_2 35 +#define SLAVE_SECURITY 36 +#define SLAVE_SNOC_CFG 37 +#define SLAVE_TCSR 38 +#define SLAVE_UFS_MEM_CFG 39 +#define SLAVE_USB3 40 +#define SLAVE_VENUS_CFG 41 +#define SLAVE_VENUS_THROTTLE_CFG 42 +#define SLAVE_VSENSE_CTRL_CFG 43 +#define SLAVE_SERVICE_CNOC 44 + +#define MASTER_CNOC_DC_NOC 0 +#define SLAVE_GEM_NOC_CFG 1 +#define SLAVE_LLCC_CFG 2 + +#define MASTER_AMPSS_M0 0 +#define MASTER_SYS_TCU 1 +#define MASTER_GEM_NOC_CFG 2 +#define MASTER_COMPUTE_NOC 3 +#define MASTER_MNOC_HF_MEM_NOC 4 +#define MASTER_MNOC_SF_MEM_NOC 5 +#define MASTER_SNOC_GC_MEM_NOC 6 +#define MASTER_SNOC_SF_MEM_NOC 7 +#define MASTER_GRAPHICS_3D 8 +#define SLAVE_MCDMA_MS_MPU_CFG 9 +#define SLAVE_MSS_PROC_MS_MPU_CFG 10 +#define SLAVE_GEM_NOC_SNOC 11 +#define SLAVE_LLCC 12 +#define SLAVE_SERVICE_GEM_NOC 13 + +#define MASTER_CNOC_MNOC_CFG 0 +#define MASTER_VIDEO_P0 1 +#define MASTER_VIDEO_PROC 2 +#define MASTER_CAMNOC_HF 3 +#define MASTER_CAMNOC_ICP 4 +#define MASTER_CAMNOC_SF 5 +#define MASTER_MDP_PORT0 6 +#define SLAVE_MNOC_HF_MEM_NOC 7 +#define SLAVE_MNOC_SF_MEM_NOC 8 +#define SLAVE_SERVICE_MNOC 9 + +#define MASTER_NPU_SYS 0 +#define MASTER_NPU_NOC_CFG 1 +#define SLAVE_NPU_CAL_DP0 2 +#define SLAVE_NPU_CP 3 +#define SLAVE_NPU_INT_DMA_BWMON_CFG 4 +#define SLAVE_NPU_DPM 5 +#define SLAVE_ISENSE_CFG 6 +#define SLAVE_NPU_LLM_CFG 7 +#define SLAVE_NPU_TCM 8 +#define SLAVE_NPU_COMPUTE_NOC 9 +#define SLAVE_SERVICE_NPU_NOC 10 + +#define MASTER_SNOC_CFG 0 +#define A1NOC_SNOC_MAS 1 +#define A2NOC_SNOC_MAS 2 +#define MASTER_GEM_NOC_SNOC 3 +#define MASTER_PIMEM 4 +#define MASTER_GIC 5 +#define SLAVE_APPSS 6 +#define SNOC_CNOC_SLV 7 +#define SLAVE_SNOC_GEM_NOC_GC 8 +#define SLAVE_SNOC_GEM_NOC_SF 9 +#define SLAVE_OCIMEM 10 +#define SLAVE_PIMEM 11 +#define SLAVE_SERVICE_SNOC 12 +#define SLAVE_QDSS_STM 13 +#define SLAVE_TCU 14 + +#endif
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