Re: [v3] dt-bindings: net: ti,davinci-emac: Convert to DT schema

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The new schema lets `ti,dm816-emac` nodes carry either 1 or 2
  `reg` entries with no further constraint, while the driver derives…
- [Low] The schema makes `clocks` required (justified in the commit
  message by 'driver probe fails without it') but leaves `power-domains`…

--

dt-bindings: net: ti,davinci-emac: Convert to DT schema

This converts the DaVinci EMAC text binding to YAML, documents clocks,
clock-names, syscon, power-domains and ti,hwmods, and adds per-compatible
conditionals restricting reg, syscon and clock-names.  clocks becomes a
required property and ti,hwmods is marked deprecated.

> diff --git a/Documentation/devicetree/bindings/net/ti,davinci-emac.yaml b/Documentation/devicetree/bindings/net/ti,davinci-emac.yaml
> new file mode 100644
> index 0000000000000..f0c539d1bac79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,davinci-emac.yaml
> @@ -0,0 +1,127 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - ti,davinci-dm6467-emac
> +      - ti,am3517-emac
> +      - ti,dm816-emac
> +
> +  reg:
> +    minItems: 1
> +    items:
> +      - description: EMAC core
> +      - description: EMAC control module

[Severity: Medium]

For ti,dm816-emac this leaves reg free to be either 1 or 2 entries, but the
driver derives hardware semantics from the entry count.  In
davinci_emac_probe() the presence of the second region is the only signal
used, both for the control module base:

	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	if (res_ctrl) {
		priv->ctrl_base =
			devm_ioremap_resource(&pdev->dev, res_ctrl);
		...
	} else {
		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
	}

and for the SCM eFuse MAC-ID instance:

	rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1, priv->mac_addr);

which flows into ti_cm_get_macid()/cpsw_am33xx_cm_get_macid() and selects the
register pair at offset + slave * 8.

Given that, can a schema-valid DM816x node land on the wrong hardware?  Two
cases seem possible:

A single-reg EMAC0 node with a 0x800 core window (as in dm816x.dtsi, where
MDIO at 0x4a100800 is a separate node) plus ti,davinci-ctrl-mod-reg-offset =
<0x900> would make ctrl_base point 0x900 past the end of the ioremap.

An EMAC1 node written with two reg entries would be passed instance 0 and get
EMAC0's factory MAC address, putting two interfaces on the wire with the same
address.

In-tree, arch/arm/boot/dts/ti/omap/dm816x.dtsi happens to pair these
correctly (eth0 has two reg entries, eth1 has one), but nothing here requires
that pairing.  Would it make sense to constrain this per instance, or at
least document that the presence of the second reg entry also selects the
MAC-ID instance?  The commit message line:

    - Allow 1-2 reg entries for DM816.

reads as though the choice is free.

[ ... ]

> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - ti,davinci-ctrl-reg-offset
> +  - ti,davinci-ctrl-mod-reg-offset
> +  - ti,davinci-ctrl-ram-offset
> +  - ti,davinci-ctrl-ram-size
> +  - clocks

[Severity: Low]

clocks is required here, but power-domains stays optional.  For
ti,davinci-dm6467-emac, is clocks alone enough to describe an enabled module?
davinci_emac_probe() only queries the rate and immediately drops the clock:

	emac_clk = devm_clk_get(&pdev->dev, NULL);
	if (IS_ERR(emac_clk)) { ... }
	emac_bus_frequency = clk_get_rate(emac_clk);
	devm_clk_put(&pdev->dev, emac_clk);

There is no clk_prepare_enable() in probe, so the LPSC gate and local reset
appear to be handled by the genpd reached through power-domains.  In
drivers/clk/davinci/psc.c each LPSC registers a domain with
GENPD_FLAG_PM_CLK and davinci_psc_genpd_attach_dev() adds the LPSC clock to
the device's pm_clk list:

	lpsc->pm_domain.attach_dev = davinci_psc_genpd_attach_dev;
	lpsc->pm_domain.flags = GENPD_FLAG_PM_CLK;
	pm_genpd_init(&lpsc->pm_domain, NULL, is_on);

That matches da850.dtsi, the only in-tree user of this compatible, which
carries both clocks = <&psc1 5> and power-domains = <&psc1 5>, and it matches
the example added below.

> +allOf:
> +  - $ref: ethernet-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,davinci-dm6467-emac
> +    then:
> +      properties:
> +        syscon: false
> +        clock-names: false
> +        reg:
> +          maxItems: 1

Since this conditional already narrows reg, syscon and clock-names for
ti,davinci-dm6467-emac, would adding power-domains to a required list here be
appropriate?  Without it, a conforming node describes a module that is never
ungated, while probe continues and later register access in emac_dev_open()
and the cpdma setup targets a gated, reset-asserted module.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901-ti-davinci-emac-v3-1-db41e32f3f6d%40gmail.com