Re: [PATCH v2 2/2] dt-bindings: arm: omap-l3-noc: Convert to DT schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] dt-bindings: bus: ti,omap-l3-noc: split omap3-l3-smx into
  separate schema
- [Low] dt-bindings: bus: ti,omap-l3-noc: restrict ti,dra7-l3-noc reg to
  2 items
- [Low] dt-bindings: bus: ti,omap-l3-noc: fix property ordering in
  schema and example
--

commit 4bcc6989fce18ef0d819b57aac84df28a2100f89
Author: Bhargav Joshi <j.bhargav.u@gmail.com>

dt-bindings: arm: omap-l3-noc: Convert to DT schema

This commit converts the TI OMAP L3 Network On Chip (NoC) binding from text
to YAML DT schema format and moves it to Documentation/devicetree/bindings/bus/.

> diff --git a/Documentation/devicetree/bindings/bus/ti,omap-l3-noc.yaml b/Documentation/devicetree/bindings/bus/ti,omap-l3-noc.yaml
> new file mode 100644
> index 0000000000000..7dab336347b2c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/ti,omap-l3-noc.yaml

[ ... ]

> +  ti,hwmods:
> +    description: hwmod names for each NoC domain.
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    deprecated: true
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +patternProperties:
> +  "^.*@[0-9a-f]+$":
> +    type: object
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,omap3-l3-smx
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +        ti,hwmods:
> +          maxItems: 1
> +      required:
> +        - "#address-cells"
> +        - "#size-cells"
> +        - ranges
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,omap4-l3-noc
> +              - ti,omap5-l3-noc
> +    then:
> +      properties:
> +        reg:
> +          minItems: 3
> +        "#address-cells": false
> +        "#size-cells": false
> +        ranges: false
> +      patternProperties:
> +        "^.*@[0-9a-f]+$": false

[Severity: Low]
Should ti,omap3-l3-smx be split into a separate binding schema rather than
combined with the Arteris NoC variants?

On OMAP3, the SonicsMX interconnect (handled by drivers/bus/omap_l3_smx.c) is
modeled as a simple-bus parent node requiring #address-cells, #size-cells,
ranges, child device nodes, a single reg entry, and the legacy ti,hwmods
property.  The OMAP4/5, DRA7, and AM4372 NoC nodes (handled by
drivers/bus/omap_l3_noc.c) are leaf device nodes with multiple reg entries
and no child nodes.  Combining both into one schema requires declaring the
bus properties and patternProperties at the top level and repeating the same
exclusions across three separate allOf blocks, while still leaving ti,hwmods
allowed on the NoC compatibles.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,dra7-l3-noc
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +          maxItems: 3

[Severity: Low]
Does ti,dra7-l3-noc support 3 reg entries, or should this be fixed at 2 items
like ti,am4372-l3-noc?

In drivers/bus/omap_l3_noc.h, dra_l3_data defines module 1 as
L3_BASE_IS_SUBMODULE sharing module 0's base address:

drivers/bus/omap_l3_noc.h:
static const struct omap_l3 dra_l3_data = {
	.l3_base = { [1] = L3_BASE_IS_SUBMODULE },
	.l3_flagmux = dra_l3_flagmux,
	.num_modules = ARRAY_SIZE(dra_l3_flagmux),

drivers/bus/omap_l3_noc.c:omap_l3_probe() {
	...
		if (l3->l3_base[i] == L3_BASE_IS_SUBMODULE) {
			/* First entry cannot be submodule */
			BUG_ON(i == 0);
			l3->l3_base[i] = l3->l3_base[i - 1];
			continue;
		}
		res = platform_get_resource(pdev, IORESOURCE_MEM, res_idx);
	...
}

Because omap_l3_probe() skips platform_get_resource() and does not advance
res_idx for L3_BASE_IS_SUBMODULE, the driver only ever consumes 2 memory
resources on DRA7, and a 3rd reg entry would be ignored.

[ ... ]

> +examples:
> +  - |
> +    l3-noc@68000000 {
> +        compatible = "ti,omap3-l3-smx", "simple-bus";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +        reg = <0x68000000 0x10000>;
> +        interrupts = <9>, <10>;
> +        ti,hwmods = "l3_main";
> +    };

[Severity: Low]
This isn't a bug, but per Documentation/devicetree/bindings/dts-coding-style.rst,
reg should come right after compatible, followed by ranges, standard
properties (#address-cells, #size-cells, interrupts), and vendor-specific
properties (ti,hwmods) last.  In the top-level properties block above,
ti,hwmods is also placed before #address-cells, #size-cells, and ranges.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-ti-l3-noc-v2-0-509e8485a1f3@gmail.com?part=2