| Message ID | 20220712124421.3129206-1-stephan.gerhold@kernkonzept.com |
|---|---|
| Headers | show |
| Series | remoteproc: qcom_q6v5_mss: Add MSM8909 | expand |
On Tue, 12 Jul 2022 14:44:17 +0200, Stephan Gerhold wrote: > qcom,q6v5.txt covers multiple SoCs with quite different binding > requirements. Converting this into one DT schema would require > several if statements, making the DT schema overall harder to > read and understand. > > To avoid this, follow the example of SC7180/SC7280 and split > "qcom,msm8916-mss-pil" (and the equivalent deprecated "qcom,q6v5-pil" > compatible) into a separate DT schema. The schema is somewhat based > on the one for SC7180/SC7280 but adjusted for the old platforms. > > Compared to the old plain text bindings, add missing documentation for > the "bam-dmux" subnode and recommend one particular approach to specify > the MBA/MPSS "memory-region" (the other one is marked as deprecated). > > Cc: Sireesh Kodali <sireeshkodali1@gmail.com> > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- > Like Sibi's patch series for SC7180/SC7820 [1] this is somewhat related > to Sireesh's series that converts all of qcom,q6v5.txt [2] (with a lot > of if statements). However, this series focuses on MSM8916/MSM8974 (or > actually MSM8909) only. > > [1]: https://lore.kernel.org/linux-arm-msm/1657020721-24939-1-git-send-email-quic_sibis@quicinc.com/ > [2]: https://lore.kernel.org/linux-arm-msm/20220511161602.117772-7-sireeshkodali1@gmail.com/ > --- > .../remoteproc/qcom,msm8916-mss-pil.yaml | 246 ++++++++++++++++++ > .../bindings/remoteproc/qcom,q6v5.txt | 19 -- > 2 files changed, 246 insertions(+), 19 deletions(-) > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/qcom,smd-edge.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.example.dtb: remoteproc@4080000: smd-edge: False schema does not allow {'interrupts': [[0, 25, 1]], 'qcom,smd-edge': [[0]], 'qcom,ipc': [[4294967295, 8, 12]], 'qcom,remote-pid': [[1]], 'label': ['hexagon']} From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 12/07/2022 14:44, Stephan Gerhold wrote: > MSM8916 was originally using the "qcom,q6v5-pil" compatible for the > MSS remoteproc. Later it was decided to use SoC-specific compatibles > instead, so "qcom,msm8916-mss-pil" is now the preferred compatible. > > Commit 60a05ed059a0 ("arm64: dts: qcom: msm8916: Add MSM8916-specific > compatibles to SCM/MSS") updated the MSM8916 device tree to make use of > the new compatible but still kept the old "qcom,q6v5-pil" as fallback. > > This is inconsistent with other SoCs and conflicts with the description > in the binding documentation (which says that only one compatible should > be present). Also, it has no functional advantage since older kernels > could not handle this DT anyway (e.g. "power-domains" in the MSS node is > only supported by kernels that also support "qcom,msm8916-mss-pil"). > > Make this consistent with other SoCs by using only the > "qcom,msm8916-mss-pil" compatible. > > Fixes: 60a05ed059a0 ("arm64: dts: qcom: msm8916: Add MSM8916-specific compatibles to SCM/MSS") > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 12/07/2022 14:44, Stephan Gerhold wrote: > qcom,q6v5.txt covers multiple SoCs with quite different binding > requirements. Converting this into one DT schema would require > several if statements, making the DT schema overall harder to > read and understand. > > To avoid this, follow the example of SC7180/SC7280 and split > "qcom,msm8916-mss-pil" (and the equivalent deprecated "qcom,q6v5-pil" > compatible) into a separate DT schema. The schema is somewhat based > on the one for SC7180/SC7280 but adjusted for the old platforms. > > Compared to the old plain text bindings, add missing documentation for > the "bam-dmux" subnode and recommend one particular approach to specify > the MBA/MPSS "memory-region" (the other one is marked as deprecated). > > Cc: Sireesh Kodali <sireeshkodali1@gmail.com> > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- > Like Sibi's patch series for SC7180/SC7820 [1] this is somewhat related > to Sireesh's series that converts all of qcom,q6v5.txt [2] (with a lot > of if statements). However, this series focuses on MSM8916/MSM8974 (or > actually MSM8909) only. > Thank you for your patch. There is something to discuss/improve. > [1]: https://lore.kernel.org/linux-arm-msm/1657020721-24939-1-git-send-email-quic_sibis@quicinc.com/ > [2]: https://lore.kernel.org/linux-arm-msm/20220511161602.117772-7-sireeshkodali1@gmail.com/ > --- > .../remoteproc/qcom,msm8916-mss-pil.yaml | 246 ++++++++++++++++++ > .../bindings/remoteproc/qcom,q6v5.txt | 19 -- > 2 files changed, 246 insertions(+), 19 deletions(-) > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > new file mode 100644 > index 000000000000..3968348dc982 > --- /dev/null > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > @@ -0,0 +1,246 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/remoteproc/qcom,msm8916-mss-pil.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm MSM8916 MSS Peripheral Image Loader (and similar) > + > +maintainers: > + - Stephan Gerhold <stephan@gerhold.net> > + > +description: > + This document describes the hardware for a component that loads and boots > + firmware on the Qualcomm MSM8916 Modem Hexagon Core (and similar). > + > +properties: > + compatible: > + oneOf: > + - enum: > + - qcom,msm8916-mss-pil > + > + - const: qcom,q6v5-pil > + description: Deprecated, prefer using qcom,msm8916-mss-pil > + deprecated: true The last compatible does not seem applicable here. Aren't you moving only MSM8916 to new schema? > + > + reg: > + items: > + - description: MSS QDSP6 registers > + - description: RMB registers > + > + reg-names: > + items: > + - const: qdsp6 > + - const: rmb > + > + interrupts: > + items: > + - description: Watchdog interrupt > + - description: Fatal interrupt > + - description: Ready interrupt > + - description: Handover interrupt > + - description: Stop acknowledge interrupt > + > + interrupt-names: > + items: > + - const: wdog > + - const: fatal > + - const: ready > + - const: handover > + - const: stop-ack > + > + clocks: > + items: > + - description: Configuration interface (AXI) clock > + - description: Configuration bus (AHB) clock > + - description: Boot ROM (AHB) clock > + - description: XO proxy clock (control handed over after startup) > + > + clock-names: > + items: > + - const: iface > + - const: bus > + - const: mem > + - const: xo > + > + power-domains: > + items: > + - description: CX proxy power domain (control handed over after startup) > + - description: MX proxy power domain (control handed over after startup) > + > + power-domain-names: > + items: > + - const: cx > + - const: mx > + > + pll-supply: > + description: PLL proxy supply (control handed over after startup) > + > + resets: > + items: > + - description: MSS restart control > + > + reset-names: > + items: > + - const: mss_restart > + > + qcom,smem-states: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: States used by the AP to signal the Hexagon core > + items: > + - description: Stop modem > + > + qcom,smem-state-names: > + description: Names of the states used by the AP to signal the Hexagon core > + items: > + - const: stop > + > + qcom,halt-regs: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: > + Halt registers are used to halt transactions of various sub-components > + within MSS. > + items: > + - items: > + - description: phandle to TCSR syscon region > + - description: offset to the Q6 halt register > + - description: offset to the modem halt register > + - description: offset to the nc halt register > + > + memory-region: > + items: > + - description: MBA reserved region > + - description: MPSS reserved region > + > + firmware-name: > + $ref: /schemas/types.yaml#/definitions/string-array > + items: > + - description: Name of MBA firmware > + - description: Name of modem firmware > + > + bam-dmux: > + $ref: /schemas/net/qcom,bam-dmux.yaml# > + description: > + Qualcomm BAM Data Multiplexer (provides network interface to the modem) > + > + smd-edge: > + $ref: qcom,smd-edge.yaml# > + description: > + Qualcomm SMD subnode which represents communication edge, channels > + and devices related to the DSP. > + > + properties: > + label: > + enum: > + - modem > + - hexagon > + > + # Deprecated properties > + cx-supply: > + description: CX power domain regulator supply (prefer using power-domains) > + deprecated: true Blank line, here and in other places between top-level properties. > + mx-supply: > + description: MX power domain regulator supply (prefer using power-domains) > + deprecated: true > + mba: > + type: object > + description: > + MBA reserved region (prefer using memory-region with two items) > + properties: > + memory-region: true > + required: > + - memory-region > + deprecated: true > + mpss: > + type: object > + description: > + MPSS reserved region (prefer using memory-region with two items) > + properties: > + memory-region: true > + required: > + - memory-region > + deprecated: true > + > +required: > + - compatible > + - reg > + - reg-names > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + - pll-supply > + - resets > + - reset-names > + - qcom,halt-regs > + - qcom,smem-states > + - qcom,smem-state-names > + - smd-edge > + > +# Fallbacks for deprecated properties > +allOf: > + - oneOf: > + - required: > + - memory-region > + - required: > + - mba > + - mpss > + deprecated: true Not sure if this is correct syntax. > + - oneOf: > + - required: > + - power-domains > + - power-domain-names > + - required: > + - cx-supply > + - mx-supply > + deprecated: true > + > +additionalProperties: false Best regards, Krzysztof
On 12/07/2022 14:44, Stephan Gerhold wrote: > The "qcom,msm8974-mss-pil" binding is still similar enough to MSM8916 > to be covered by the same DT schema. The only difference is the > additional "mss-supply", which can be easily handled using a single > if statement. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- > Note: I generated this patch on top of Sibi's series [1] to avoid > conflicts later (I expect it will be picked up first). > > [1]: https://lore.kernel.org/linux-arm-msm/1657020721-24939-1-git-send-email-quic_sibis@quicinc.com/ > --- > .../remoteproc/qcom,msm8916-mss-pil.yaml | 16 ++++++++++++++++ > .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 16 ---------------- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > index 3968348dc982..ca7146551ba9 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml > @@ -18,6 +18,7 @@ properties: > oneOf: > - enum: > - qcom,msm8916-mss-pil > + - qcom,msm8974-mss-pil > > - const: qcom,q6v5-pil > description: Deprecated, prefer using qcom,msm8916-mss-pil > @@ -76,6 +77,9 @@ properties: > pll-supply: > description: PLL proxy supply (control handed over after startup) > > + mss-supply: > + description: MSS power domain supply (only valid for qcom,msm8974-mss-pil) > + > resets: > items: > - description: MSS restart control > @@ -177,6 +181,18 @@ required: > - qcom,smem-state-names > - smd-edge > > +# mss-supply is only valid (and required) for MSM8974 > +if: Put it under allOf. This makes it prepared for growing. > + properties: > + compatible: > + const: qcom,msm8974-mss-pil > +then: > + required: > + - mss-supply > +else: > + properties: > + mss-supply: false > + Best regards, Krzysztof
On Thu, Jul 14, 2022 at 11:50:30AM +0200, Krzysztof Kozlowski wrote: > On 12/07/2022 14:44, Stephan Gerhold wrote: > > [...] > > +properties: > > + compatible: > > + oneOf: > > + - enum: > > + - qcom,msm8916-mss-pil > > + > > + - const: qcom,q6v5-pil > > + description: Deprecated, prefer using qcom,msm8916-mss-pil > > + deprecated: true > > The last compatible does not seem applicable here. Aren't you moving > only MSM8916 to new schema? > "qcom,q6v5-pil" is exactly the same as "qcom,msm8916-mss-pil". It's just a deprecated quite unfortunately chosen old name for the same thing. :) See these lines in the driver: { .compatible = "qcom,q6v5-pil", .data = &msm8916_mss}, { .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss}, > > [...] > > + # Deprecated properties > > + cx-supply: > > + description: CX power domain regulator supply (prefer using power-domains) > > + deprecated: true > > Blank line, here and in other places between top-level properties. > Ack, will change this in v2. > > [...] > > +# Fallbacks for deprecated properties > > +allOf: > > + - oneOf: > > + - required: > > + - memory-region > > + - required: > > + - mba > > + - mpss > > + deprecated: true > > Not sure if this is correct syntax. > Yeah I was not sure either but at least dt_binding_check does not complain about this. :-) Maybe Rob has an opinion if this makes sense or not? Thanks, Stephan
On 14/07/2022 20:48, Stephan Gerhold wrote: > On Thu, Jul 14, 2022 at 11:50:30AM +0200, Krzysztof Kozlowski wrote: >> On 12/07/2022 14:44, Stephan Gerhold wrote: >>> [...] >>> +properties: >>> + compatible: >>> + oneOf: >>> + - enum: >>> + - qcom,msm8916-mss-pil >>> + >>> + - const: qcom,q6v5-pil >>> + description: Deprecated, prefer using qcom,msm8916-mss-pil >>> + deprecated: true >> >> The last compatible does not seem applicable here. Aren't you moving >> only MSM8916 to new schema? >> > > "qcom,q6v5-pil" is exactly the same as "qcom,msm8916-mss-pil". It's just > a deprecated quite unfortunately chosen old name for the same thing. :) > > See these lines in the driver: > > { .compatible = "qcom,q6v5-pil", .data = &msm8916_mss}, > { .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss}, Yeah, but previous bindings were not mentioning it alone, so this would not be a direct conversion. Best regards, Krzysztof
On Fri, Jul 15, 2022 at 08:33:53AM +0200, Krzysztof Kozlowski wrote: > On 14/07/2022 20:48, Stephan Gerhold wrote: > > On Thu, Jul 14, 2022 at 11:50:30AM +0200, Krzysztof Kozlowski wrote: > >> On 12/07/2022 14:44, Stephan Gerhold wrote: > >>> [...] > >>> +properties: > >>> + compatible: > >>> + oneOf: > >>> + - enum: > >>> + - qcom,msm8916-mss-pil > >>> + > >>> + - const: qcom,q6v5-pil > >>> + description: Deprecated, prefer using qcom,msm8916-mss-pil > >>> + deprecated: true > >> > >> The last compatible does not seem applicable here. Aren't you moving > >> only MSM8916 to new schema? > >> > > > > "qcom,q6v5-pil" is exactly the same as "qcom,msm8916-mss-pil". It's just > > a deprecated quite unfortunately chosen old name for the same thing. :) > > > > See these lines in the driver: > > > > { .compatible = "qcom,q6v5-pil", .data = &msm8916_mss}, > > { .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss}, > > Yeah, but previous bindings were not mentioning it alone, so this would > not be a direct conversion. > Sorry, I'm not sure I understand you correctly: What do you mean with "the previous bindings were not mentioning it alone"? "qcom,q6v5-pil" was listed as standalone compatible just like all the other compatibles: - compatible: Usage: required Value type: <string> Definition: must be one of: "qcom,q6v5-pil", <---- "qcom,ipq8074-wcss-pil" "qcom,qcs404-wcss-pil" "qcom,msm8916-mss-pil", <---- "qcom,msm8974-mss-pil" "qcom,msm8996-mss-pil" "qcom,msm8998-mss-pil" "qcom,sc7180-mss-pil" "qcom,sc7280-mss-pil" "qcom,sdm845-mss-pil" The only non-conversion steps I did was to mark some of the redundant bindings as deprecated (e.g. "memory-region" with 2 items vs "mba" and "mpss" subnode, "qcom,msm8916-mss-pil" vs "qcom,q6v5-pil"). I can put the deprecations in a separate patch if that clarifies the situation. Thanks, Stephan
On 15/07/2022 10:00, Stephan Gerhold wrote: >> > > Sorry, I'm not sure I understand you correctly: What do you mean with > "the previous bindings were not mentioning it alone"? "qcom,q6v5-pil" > was listed as standalone compatible just like all the other compatibles: > > - compatible: > Usage: required > Value type: <string> > Definition: must be one of: > "qcom,q6v5-pil", <---- You're right, sorry for confusion. Best regards, Krzysztof