[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <800ec5d6-44d5-22aa-cf0c-0e7c5de1feb3@gmail.com>
Date: Mon, 3 Feb 2020 12:55:01 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Jeremy Linton <jeremy.linton@....com>, Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, opendmb@...il.com, davem@...emloft.net,
bcm-kernel-feedback-list@...adcom.com,
linux-kernel@...r.kernel.org, wahrenst@....net,
hkallweit1@...il.com
Subject: Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery
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@....com>
>>> ---
>>> 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
Powered by blists - more mailing lists