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 <[email protected]>
---
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;
+ }
+ } 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);
}
--
2.24.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>>> ---
>>> 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
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