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

[Rob Herring <robh@kernel.org>]

On Fri, Sep 04, 2026 at 01:13:54AM +0530, Bhargav Joshi wrote:
> Hi,
> 
> On Thu, Sep 3, 2026 at 8:47 PM <netdev-bot+sashiko@kernel.org> 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.

Just leave it as-is. This is for ancient h/w.

Rob