Re: [PATCH 1/8] dt-bindings: interconnect: add Qualcomm MSM8998 NoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: interconnect: qcom,msm8998: require clocks for clocked NoCs
- [Medium] dt-bindings: interconnect: qcom,msm8998: disallow reg on mnoc-ahb child
- [Low] dt-bindings: interconnect: qcom,msm8998: require compatible on mnoc-ahb
--

commit 2b1f68f1df36a65ddbb7510a13783218c56291b7
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

dt-bindings: interconnect: add Qualcomm MSM8998 NoC

This patch adds the YAML device tree binding schema and header definitions
for the eight Qualcomm MSM8998 Network-On-Chip interconnect providers.

> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998.yaml
> new file mode 100644
> index 0000000000000..0a05dc620cfd4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998.yaml
> @@ -0,0 +1,192 @@

[ ... ]

> +  - if:
> +      properties:
> +        compatible:
> +          const: qcom,msm8998-mnoc
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: CPU-NoC High-performance Bus Clock
> +            - description: MMSS NoC AXI Clock
> +
> +        clock-names:
> +          items:
> +            - const: iface
> +            - const: mnoc_bus
> +
> +        interconnect-mnoc-ahb:
> +          type: object
> +          additionalProperties: true
> +
> +          properties:
> +            compatible:
> +              const: qcom,msm8998-mnoc-ahb

[Severity: Low]
Should compatible be listed under required for the interconnect-mnoc-ahb
subschema here?

Because interconnect-mnoc-ahb sets additionalProperties: true and relies on
the top-level schema matching on compatible to validate the child node, an
interconnect-mnoc-ahb child node that omits compatible will pass validation
without its properties being checked, and will fail to bind a driver when
populated by devm_of_platform_populate() in qnoc_probe().

> +
> +  - if:
> +      properties:
> +        compatible:
> +          const: qcom,msm8998-mnoc-ahb
> +
> +    then:
> +      description:
> +        The MMSS AHB NoC shares its register space with the MMSS NoC, so it is
> +        described as a child of the latter and has no register range of its own.
> +
> +      properties:
> +        clocks:
> +          items:
> +            - description: MMSS AHB Clock
> +
> +        clock-names:
> +          const: bus
> +
> +      required:
> +        - clocks
> +        - clock-names
> +
> +    else:
> +      required:
> +        - reg

[Severity: Medium]
Should the then branch for qcom,msm8998-mnoc-ahb also set reg: false under
properties?

Since reg is defined at the top level of the schema, unevaluatedProperties:
false still allows reg to be present on a qcom,msm8998-mnoc-ahb child node.

In drivers/interconnect/qcom/icc-rpm.c:qnoc_probe(), the child device only
inherits its parent's regmap when IORESOURCE_MEM is absent:

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	if (!res) {
		/* Try parent's regmap */
		qp->regmap = dev_get_regmap(dev->parent, NULL);
		if (qp->regmap)
			goto regmap_done;
		return -ENODEV;
	}

If a child node specifies reg, schema validation passes, but qnoc_probe()
attempts to claim and ioremap the memory resource instead of inheriting the
parent's regmap.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - qcom,msm8998-bimc
> +            - qcom,msm8998-gnoc
> +
> +    then:
> +      properties:
> +        clocks: false
> +        clock-names: false

[Severity: Medium]
Should clocks and clock-names be required for all compatibles other than
qcom,msm8998-bimc and qcom,msm8998-gnoc (for instance, in an else branch
here instead of only under qcom,msm8998-mnoc-ahb)?

Currently, required: [clocks, clock-names] is only specified for
qcom,msm8998-mnoc-ahb. Nodes using qcom,msm8998-a1noc, qcom,msm8998-a2noc,
qcom,msm8998-cnoc, qcom,msm8998-mnoc, or qcom,msm8998-snoc without clocks or
clock-names will pass schema validation, but fail probe in
drivers/interconnect/qcom/icc-rpm.c:qnoc_probe():

	ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
	if (ret)
		goto err_disable_unprepare_clk;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-msm8998-icc-v1-0-c2da3934b6c8@oss.qualcomm.com?part=1