RE: [RFC PATCH 4/8] xilinx: tsn: Add Ethernet MAC (EMAC) and MDIO support to the TSN driver

["Neeli, Srinivas" <srinivas.neeli@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, February 19, 2026 10:36 PM
> To: Neeli, Srinivas <srinivas.neeli@amd.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Simek,
> Michal <michal.simek@amd.com>; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; richardcochran@gmail.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [RFC PATCH 4/8] xilinx: tsn: Add Ethernet MAC (EMAC) and MDIO
> support to the TSN driver
>
> > +static int tsn_mdio_read(struct mii_bus *bus, int phy_id, int reg) {
> > +   u32 rc;
> > +   int ret;
> > +   struct tsn_emac *emac = bus->priv;
> > +   struct tsn_priv *common = emac->common;
> > +
> > +   scoped_guard(mutex, &common->mdio_lock) {
>
> What is this mutex protecting?
>

The mutex serializes access to the MDIO controller register block. The MDIO core already serializes bus accesses via bus->mdio_lock,
so the additional driver level mutex is redundant.
I will remove the extra mutex from the driver and rely on the MDIO core locking instead.

> > +           tsn_mdio_mdc_enable(emac);
>
> It is unusual to stop MDC. I suspect some PHYs will not like this.
> What is your reason for doing this.
>
The intention is to reduce power consumption by gating  the clock when it is not required.

> > +/**
> > + * tsn_mdio_setup - Setup MDIO bus for TSN EMAC
> > + * @emac: Pointer to TSN EMAC structure
> > + * @mac_np: Device tree node for MAC
> > + *
> > + * This function initializes the MDIO bus for the TSN EMAC interface.
> > + * It allocates an MII bus structure, configures MDIO timing, finds
> > + * the MDIO device tree node, and registers the MDIO bus with the kernel.
> > + *
> > + * Return: 0 on success, negative error code on failure  */ int
> > +tsn_mdio_setup(struct tsn_emac *emac, struct device_node *mac_np) {
> > +   struct tsn_priv *common = emac->common;
> > +   struct device_node *mdio_node;
> > +   struct mii_bus *bus;
> > +   int ret;
> > +
> > +   bus = mdiobus_alloc();
> > +   if (!bus)
> > +           return -ENOMEM;
> > +
> > +   snprintf(bus->id, MII_BUS_ID_SIZE, "tsn-mac-%.8llx",
> > +            (unsigned long long)emac->regs_start);
> > +
> > +   bus->priv = emac;
> > +   bus->name = "Xilinx TSN Ethernet MDIO";
> > +   bus->read = tsn_mdio_read;
> > +   bus->write = tsn_mdio_write;
> > +   bus->parent = common->dev;
> > +   emac->mii_bus = bus;
> > +
> > +   mdio_node = of_get_child_by_name(mac_np, "mdio");
> > +   if (!mdio_node) {
> > +           dev_err(common->dev, "MAC%d: missing 'mdio' child
> node\n",
> > +                   emac->emac_num);
> > +           ret = -ENODEV;
> > +           goto unregister;
> > +   }
>
> I forget, does the binding have a required: for the MDIO node?
>

MDIO node is not mandatory in binding. I will update the driver to treat the mdio chaild node as optional.

> > +   ret = tsn_mdio_enable(emac);
> > +   if (ret < 0)
> > +           goto unregister;
> > +   ret = of_mdiobus_register(bus, mdio_node);
>
> Having a node in DT is generally optional. You can pass NULL to
> of_mdiobus_register() and it will do the right thing.
>
>       Andrew
Yes, agreed. I will update the implementation to pass NULL to of_mdiobus_register() when the MDIO DT node is absent, instead of failing probe.

Thanks
Neeli Srinivas