RE: [PATCH v1 net-next] net: phy: mdio_bus: make mdiobus_scan also cover PHY that only talks C45

[Voon, Weifeng]

> On 8/28/19 8:41 AM, Ong, Boon Leong wrote:
> >> On Tue, Aug 27, 2019 at 03:23:34PM +0000, Voon, Weifeng wrote:
> >>>>>> Make mdiobus_scan() to try harder to look for any PHY that only
> >>>> talks C45.
> >>>>> If you are not using Device Tree or ACPI, and you are letting the
> >>>>> MDIO bus be scanned, it sounds like there should be a way for you
> >>>>> to provide a hint as to which addresses should be scanned (that's
> >>>>> mii_bus::phy_mask) and possibly enhance that with a mask of
> >>>>> possible
> >>>>> C45 devices?
> >>>>
> >>>> Yes, i don't like this unconditional c45 scanning. A lot of MDIO
> >>>> bus drivers don't look for the MII_ADDR_C45. They are going to do a
> >>>> C22 transfer, and maybe not mask out the MII_ADDR_C45 from reg,
> >>>> causing an invalid register write. Bad things can then happen.
> >>>>
> >>>> With DT and ACPI, we have an explicit indication that C45 should be
> >>>> used, so we know on this platform C45 is safe to use. We need
> >>>> something similar when not using DT or ACPI.
> >>>>
> >>>> 	  Andrew
> >>>
> >>> Florian and Andrew,
> >>> The mdio c22 is using the start-of-frame ST=01 while mdio c45 is
> >>> using ST=00 as identifier. So mdio c22 device will not response to
> mdio c45 protocol.
> >>> As in IEEE 802.1ae-2002 Annex 45A.3 mention that:
> >>> " Even though the Clause 45 MDIO frames using the ST=00 frame code
> >>> will also be driven on to the Clause 22 MII Management interface,
> >>> the Clause 22 PHYs will ignore the frames. "
> >>>
> >>> Hence, I am not seeing any concern that the c45 scanning will mess
> >>> up with
> >>> c22 devices.
> >>
> >> Hi Voon
> >>
> >> Take for example mdio-hisi-femac.c
> >>
> >> static int hisi_femac_mdio_read(struct mii_bus *bus, int mii_id, int
> >> regnum) {
> >>        struct hisi_femac_mdio_data *data = bus->priv;
> >>        int ret;
> >>
> >>        ret = hisi_femac_mdio_wait_ready(data);
> >>        if (ret)
> >>                return ret;
> >>
> >>        writel((mii_id << BIT_PHY_ADDR_OFFSET) | regnum,
> >>               data->membase + MDIO_RWCTRL);
> >>
> >>
> >> There is no check here for MII_ADDR_C45. So it will perform a C22
> >> transfer. And regnum will still have MII_ADDR_C45 in it, so the
> >> writel() is going to set bit 30, since #define MII_ADDR_C45 (1<<30).
> >> What happens on this hardware under these conditions?
> >>
> >> You cannot unconditionally ask an MDIO driver to do a C45 transfer.
> >> Some drivers are going to do bad things.
> >
> > Andrew & Florian, thanks for your review on this patch and insights on
> it.
> > We will look into the implementation as suggested as follow.
> >
> > - for each bit clear in mii_bus::phy_mask, scan it as C22
> > - for each bit clear in mii_bus::phy_c45_mask, scan it as C45
> >
> > We will work on this and resubmit soonest.
> 
> Sounds good. If you do not need to scan the MDIO bus, another approach
> is to call get_phy_device() by passing the is_c45 boolean to true in
> order to connect directly to a C45 device for which you already know the
> address.
> 
> Assuming this is done for the stmmac PCI changes that you have submitted,
> and that those cards have a fixed set of addresses for their PHYs, maybe
> scanning the bus is overkill?
> --
> Florian

Good suggestion. And yes, we have a fix phy address too. But the MAC is free 
to pair with any PHY which might supports only mdio c22 or only mdio c45. 
We will consider it and resubmit soonest.