RE: [PATCH net-next 2/5] net: ethernet: oa_tc6: Allow custom mii_bus

[Regus, Ciprian]

> > > This all seems pretty invasive and ugly. Please could you think what
> > > happens if instead of passing in an mdiobus, you pass a phydev. Is the
> > > change to the core simpler and cleaner?
> > >
> > > Andrew
> >
> 
> > Kind of agree. Initially we were thinking about changing the
> > existing code (Microchip's vendor code) to alloc mii_bus so that
> > code would be same across multiple vendors. Either way, it would be
> > invasive changes. So, we decide to go with minimal change to other
> > vendor's code.
> 
> That would be wrong. The standard defines this, so it should be in the
> core. Anything which the standard defines should be in the core, so
> that drivers for hardware which actually follow the standard are
> minimal. Also, we try to keep workarounds for broken hardware out of
> the core, hide it in the driver. That is not always possible, but the
> aim should be to make the core clean. We don't want to penalise
> vendors which got the implementation correct because of vendors who
> got is wrong.
> 
> > Trying to understand your suggestion. Are you suggesting to move
> > entire mii_bus allocation/APIs implementation to vendor side and
> > keep only phy dev usage in oa_tc6.c?
> 
> No. I'm thinking maybe extend oa_tc6_init, similar to what you
> did. Add a quirks flag, and define TC6_QUIRK_BROKEN_PHY. And allow a
> phydev to be passed as well.
> 
> If the quirk is set, don't call oa_tc6_mdiobus_register() or
> phy_find_first(), nor oa_tc6_mdiobus_unregister().
>

The issue I can see with this approach is that we should have already registered
the mii_bus and read a valid PHY id from the device, before passing the phy_device to
to oa_tc_init(). Scanning the mdio bus requires OA TC6 SPI transfers (reading registers 0xFF02
and 0xFF03), while oa_tc6 has not yet initialized. For the ADIN1140 driver this is not an issue,
because we can return cached values for the PHY id, as you suggested. However, that limits
the usefulness of the BROKEN_PHY flag, because every new driver that cannot use the default
init sequence in oa_tc6 (and wants to set the BROKEN_PHY flag) has to fit this specific case.

I think the approach which involves the least amount of changes in the core would be for oa_tc6
to skip the oa_tc6_phy_init() and oa_tc6_phy_exit() if the BROKEN_PHY quirk flag is set and
leave it to the drivers using oa_tc6 to handle the mii_bus alloc/register/unregister/free and
phy_connect()/disconnect().

These would be the only changes in the core's phy handling path (besides adding the flag itself):

@@ -585,10 +586,13 @@ static int oa_tc6_phy_init(struct oa_tc6 *tc6)
 {
        int ret;
 
+       if (tc6->quirk_flags & OA_TC6_BROKEN_PHY)
+               return 0;
+
        ret = oa_tc6_check_phy_reg_direct_access_capability(tc6);
        if (ret) {
                netdev_err(tc6->netdev,
                          "Direct PHY register access is not supported by the MAC-PHY\n");
                return ret;
        }
...
}

static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
{
+       if (tc6->quirk_flags & OA_TC6_BROKEN_PHY)
+               return;
+
        phy_disconnect(tc6->phydev);
        oa_tc6_mdiobus_unregister(tc6);
}

> You probably want to start with a patch which breaks oa_tc6_phy_init()
> into two, since you still need the phy_connect_direct() and
> phy_attached_info(). Then add the quirk, and lastly your driver making
> use of the quirk.
> 
> The quirks flag could also be used for devices which have MMD 30
> mapped into a vendor reserved MMS.
> 
> 	Andrew