On 2024-05-03 13:59:52 +0200, Andrew Lunn wrote:
> > > > +static int rtsn_mii_register(struct rtsn_private *priv)
> > > > +{
> > > > + struct platform_device *pdev = priv->pdev;
> > > > + struct device *dev = &pdev->dev;
> > > > + struct device_node *mdio_node;
> > > > + struct mii_bus *mii;
> > > > + int ret;
> > > > +
> > > > + mii = mdiobus_alloc();
> > > > + if (!mii)
> > > > + return -ENOMEM;
> > > > +
> > > > + mdio_node = of_get_child_by_name(dev->of_node, "mdio");
> > > > + if (!mdio_node) {
> > > > + ret = -ENODEV;
> > > > + goto out_free_bus;
> > > > + };
> > > > +
> > > > + mii->name = "rtsn_mii";
> > > > + sprintf(mii->id, "%s-%x", pdev->name, pdev->id);
> > > > + mii->priv = priv;
> > > > + mii->read = rtsn_mii_read;
> > > > + mii->write = rtsn_mii_write;
> > > > + mii->read_c45 = rtsn_mii_read_c45;
> > > > + mii->write_c45 = rtsn_mii_write_c45;
> > >
> > > Just leave these two empty, and the core will do C45 over C22 for you.
> >
> > Does this not require the bus to be created/allocated with an
> > implementation that support this, for example mdio_i2c_alloc() or
> > alloc_mdio_bitbang()? This bus is allocated with mdiobus_alloc() which
> > do not implement this. Removing the C45 functions here result in
> > __mdiobus_c45_read() returning -EOPNOTSUPP as bus->read_c45 is not set.
>
> phy_read_mmd():
> __phy_read_mmd():
> mmd_phy_read():
>
> So is is_c45 is true?
Not sure, I never get that far. The function __mdiobus_c45_read() is
called directly from of_mdiobus_register() call-chain.
The call chain is:
rtsn_open()
of_mdiobus_register() <- This fails and RTSN can't talk to the PHY
__of_mdiobus_register()
__of_mdiobus_parse_phys()
of_mdiobus_register_phy()
fwnode_mdiobus_register_phy() <- See [1]
get_phy_device()
get_phy_c45_ids()
mdiobus_c45_read()
__mdiobus_c45_read() <- Returns -EOPNOTSUPP [2]
1. Here is_c45 is set as it checks the compatible value is checked.
is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
2. The struct mii_bus have no read_c45() callback and returns
-EOPNOTSUPP.
>
> I would expect it to be false, so that it then uses
>
> mmd_phy_indirect(bus, phy_addr, devad, regnum);
> /* Write the data into MMD's selected register */
> return __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
Cool, I was not aware of that code-path I was only looking at the above
when trying to remove the implementation in RTSN driver.
>
> which is C45 over C22.
>
> If is_c45 is true, what is setting it true?
>
> Andrew
--
Kind Regards,
Niklas Söderlund
