Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery

[Florian Fainelli]

On 2/1/20 11:07 AM, Jeremy Linton wrote:
> Hi,
> 
> First thanks for looking at this!
> 
> On 2/1/20 9:25 AM, Andrew Lunn wrote:
>> On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:
>>> The unimac mdio driver falls back to scanning the
>>> entire bus if its given an appropriate mask. In ACPI
>>> mode we expect that the system is well behaved and
>>> conforms to recent versions of the specification.
>>>
>>> We then utilize phy_find_first(), and
>>> phy_connect_direct() to find and attach to the
>>> discovered phy during net_device open.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
>>> ---
>>> Â drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
>>> Â 1 file changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 2049f8218589..f3271975b375 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -5,7 +5,7 @@
>>> ÂÂ * Copyright (c) 2014-2017 Broadcom
>>> ÂÂ */
>>> Â -
>>> +#include <linux/acpi.h>
>>> Â #include <linux/types.h>
>>> Â #include <linux/delay.h>
>>> Â #include <linux/wait.h>
>>> @@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev,
>>> bool init)
>>> Â int bcmgenet_mii_probe(struct net_device *dev)
>>> Â {
>>> ÂÂÂÂÂ struct bcmgenet_priv *priv = netdev_priv(dev);
>>> -ÂÂÂ struct device_node *dn = priv->pdev->dev.of_node;
>>> +ÂÂÂ struct device *kdev = &priv->pdev->dev;
>>> +ÂÂÂ struct device_node *dn = kdev->of_node;
>>> +
>>> ÂÂÂÂÂ struct phy_device *phydev;
>>> ÂÂÂÂÂ u32 phy_flags = 0;
>>> ÂÂÂÂÂ int ret;
>>> @@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENODEV;
>>> ÂÂÂÂÂÂÂÂÂ }
>>> ÂÂÂÂÂ } else {
>>> -ÂÂÂÂÂÂÂ phydev = dev->phydev;
>>> +ÂÂÂÂÂÂÂ if (has_acpi_companion(kdev)) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ char mdio_bus_id[MII_BUS_ID_SIZE];
>>> +ÂÂÂÂÂÂÂÂÂÂÂ struct mii_bus *unimacbus;
>>> +
>>> +ÂÂÂÂÂÂÂÂÂÂÂ snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
>>> +
>>> +ÂÂÂÂÂÂÂÂÂÂÂ unimacbus = mdio_find_bus(mdio_bus_id);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (!unimacbus) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Unable to find mii\n");
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENODEV;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ }
>>> +ÂÂÂÂÂÂÂÂÂÂÂ phydev = phy_find_first(unimacbus);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ put_device(&unimacbus->dev);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (!phydev) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Unable to find PHY\n");
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENODEV;
>>
>> Hi Jeremy
>>
>> phy_find_first() is not recommended. Only use it if you have no other
>> option. If the hardware is more complex, two PHYs on one bus, you are
>> going to have a problem. So i suggest this is used only for PCI cards
>> where the hardware is very fixed, and there is only ever one MAC and
>> PHY on the PCI card. When you do have this split between MAC and MDIO
>> bus, each being independent devices, it is more likely that you do
>> have multiple PHYs on one shared MDIO bus.
> 
> Understood.
> 
>>
>> In the DT world, you use a phy-handle to point to the PHY node in the
>> device tree. Does ACPI have the same concept, a pointer to some other
>> device in ACPI?
> 
> There aren't a lot of good options here. ACPI is mostly a power mgmt
> abstraction and is directly silent on this topic. So while it can be
> quite descriptive like DT, frequently choosing to use a bunch of DT
> properties in ACPI _DSD methods is a mistake. Both for cross OS booting
> as well as long term support. Similar silence from SBSA, which attempts
> to setup some guide rails for situations like this. I think that is
> because there aren't any non-obsolete industry standards for NICs.

I suppose you have two options from here:

- try to set a good example and provide a representation of the devices
that is as comprehensive as possible and paves the way for others to
draw as a good (or not bad at least) example

- continue to do an ad-hoc solution since there is little to no interest
in any standardization (more likely, no need).

> 
> So, in an attempt to fall back on the idea that the hardware should be
> self describing, and it shouldn't be involving the system firmware in
> basic device specific introspection I've been trying to avoid the use of
> any DSD properties. In the majority of cases (including DT) these
> properties aren't being auto-detected by the firmware either, they are
> just being hard-coded into DT or DSDT tables.
> 
> Part of the arm standardization effort has been to clamp down on all the
> creative ways that these machines can be built. It seems a guide rail
> that says for this adapter it must have a MDIO bus per MAC for ACPI
> support as though it were on PCI isn't unreasonable. Another easily
> understood one, might be to assign the PHY's the the same order as the
> MAC's UIDs if there were a shared bus (less ideal without example
> hardware).
> 
> I'm not really sure what the right answer here is, but I like to avoid
> hardcoding DT properties in DSD unless there simply isn't an alternative.

I do not think we are asking to get properties hard coded, we are asking
to get a proper representation of these MDIO devices, including their
supserset that PHY devices are into ACPI, in a way that is usable by the
core MDIO layer without drivers cutting corners.
-- 
Florian