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

[Bhargav Joshi]

Hi,

On Thu, Sep 3, 2026 at 8:47 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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);

This driver logic relies on the reg entry count to determine the port
instance. This is a software implementation detail and not hardware
detail. I believe it is bad idea to rely on the number of reg entries to
get instance, It's better to use of_alias_get_id() to get instance of port.

>From a hardware perspective, eth0 requires 2 reg entries because MDIO
node occupies the 0x800 to 0x8FF.

>
> 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.
>
To resolve this I can either:

Use nodename to hardcode exact address for eth0 and eth1 something like
  if:
      properties:
        $nodename:
          pattern: "^ethernet@4a100000$"
  then:
      properties:
        reg:
          minItems: 2
or
Enforce 2 reg for both eth0 and eth1, For eth1, even though there is no
MDIO, the base+0x800 to 0x8FF is simply unused, so describing it as
split regions is still accurate. But this will require a driver fix
for getting port
instance.

> 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.

Yes, I will fix this in the next version.

Best regards,
Bhargav