| Message ID | 20200201074625.8698-4-jeremy.linton@arm.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add ACPI bindings to the genet | expand |
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@arm.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. 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? Thanks Andrew
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@arm.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. 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.
Hi, 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@arm.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. > > 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? I though I should clarify the direct question here about ACPI. ACPI does have the ability to do what you describe, but it a more rigorous way. If you look at the ACPI GenericSerialBus abstraction you will see how ACPI would likely handle this situation. I've been considering making a similar comment in that large fwnode patch set posted the other day.
> I though I should clarify the direct question here about ACPI. ACPI does > have the ability to do what you describe, but it a more rigorous way. If you > look at the ACPI GenericSerialBus abstraction you will see how ACPI would > likely handle this situation. I've been considering making a similar comment > in that large fwnode patch set posted the other day. I know ~0 about ACPI. But it does not seem unreasonable to describe an MDIO bus in the same way as an i2c bus, or an spi bus. Each can have devices on it, at specific addresses. Each needs common properties like interrupts, and each needs bus specific properties like SPI polarity. And you need pointers to these devices, so that other subsystems can use them. So maybe the correct way to describe this is to use ACPI GenericSerialBus? What the kernel community really seems to miss is a "Rob Herring" for ACPI. Andrew
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@arm.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.
Hi, On 2/2/20 7:15 PM, Andrew Lunn wrote: >> I though I should clarify the direct question here about ACPI. ACPI does >> have the ability to do what you describe, but it a more rigorous way. If you >> look at the ACPI GenericSerialBus abstraction you will see how ACPI would >> likely handle this situation. I've been considering making a similar comment >> in that large fwnode patch set posted the other day. I should have been a lot more specific here, but I didn't want to write a book. > > I know ~0 about ACPI. But it does not seem unreasonable to describe an > MDIO bus in the same way as an i2c bus, or an spi bus. Each can have > devices on it, at specific addresses. Each needs common properties > like interrupts, and each needs bus specific properties like SPI > polarity. And you need pointers to these devices, so that other > subsystems can use them. > > So maybe the correct way to describe this is to use ACPI > GenericSerialBus? AFAIK, not as the specification stands today. First its not defined for MDIO (see 6-240 in acpi 6.3) , and secondly because its intended to be used from AML (one of the examples IIRC is to read battery vendor info). That implies to me, that the ACPI standards body's would also have to add some additional methods which configure and return state about the phys. AKA some of the linux phy_() functions would just redirect to AML equivalents the same way there are AML battery functions for returning status/etc.
> 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. Yes. And i'm also interested in this in a generic way. Ethernet switches often have MDIO busses, with lots of PHYs on them. Some might argue that switches are out of scope for ACPI, but people have shown interest in getting ACPI working on Espressobin, http://espressobin.net/ which has a marvell Switch on it. There are also some broadcom SoCs with generic PHYs, not ethernet PHYs as devices on MDIO busses. And we have people submitting patches to other drivers at the moment, which just seem to stuff DT properties into ACPI tables. At least in networking, ACPI seems to be a wild west, anything goes, no documentation, no standardisation, nobody has an ACPI maintainer role over the whole kernel who cares about it. Andrew
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; + } + } else { + phydev = dev->phydev; + } phydev->dev_flags = phy_flags; ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup, @@ -455,9 +477,12 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) /* Retain this platform_device pointer for later cleanup */ priv->mii_pdev = ppdev; ppdev->dev.parent = &pdev->dev; - ppdev->dev.of_node = bcmgenet_mii_of_find_mdio(priv); - if (pdata) + if (dn) + ppdev->dev.of_node = bcmgenet_mii_of_find_mdio(priv); + else if (pdata) bcmgenet_mii_pdata_init(priv, &ppd); + else + ppd.phy_mask = ~0; ret = platform_device_add_resources(ppdev, &res, 1); if (ret) @@ -586,10 +611,13 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) static int bcmgenet_mii_bus_init(struct bcmgenet_priv *priv) { - struct device_node *dn = priv->pdev->dev.of_node; + struct device *kdev = &priv->pdev->dev; + struct device_node *dn = kdev->of_node; if (dn) return bcmgenet_mii_of_init(priv); + else if (has_acpi_companion(kdev)) + return bcmgenet_phy_interface_init(priv); else return bcmgenet_mii_pd_init(priv); }
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@arm.com> --- drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-)