This patch series allows the BCM GENET, as used on the RPi4,
to attach when booted in an ACPI enviroment. The DSDT entry to trigger
this is seen below. The second patch in the set retrieves the MAC
address from the umac registers rather than carrying it directly in
the DSDT. This of course requires the firmware to pre-program it, so
we continue to fall back to a random one if it appears to be garbage.
+ Device (ETH0)
+ {
+ Name (_HID, "BCM6E4E")
+ Name (_CID, "BCM6E4E")
+ Name (_UID, 0)
+ Name (_CCA, 0x0)
+ Method (_STA)
+ {
+ Return (0xf)
+ }
+ Method (_CRS, 0x0, Serialized)
+ {
+ Name (RBUF, ResourceTemplate ()
+ {
+ Memory32Fixed (ReadWrite, 0xFd580000, 0x10000, )
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBD }
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBE }
+ })
+ Return (RBUF)
+ }
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "phy-mode", "rgmii" },
+ }
+ })
+ }
+
Jeremy Linton (2):
net: bcmgenet: Initial bcmgenet ACPI support
net: bcmgenet: Fetch MAC address from the adapter
.../net/ethernet/broadcom/genet/bcmgenet.c | 64 ++++++++++++----
drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++-------
2 files changed, 98 insertions(+), 42 deletions(-)
--
2.24.1
The rpi4 is capable of booting in ACPI mode with the latest
edk2-platform commits. As such, it would be helpful if the genet
platform device were usable.
To achive this we convert some of the of_ calls to device_ and
add the ACPI id module table, and tweak the phy connection code
to use phy_connect() in the ACPI path.
Signed-off-by: Jeremy Linton <[email protected]>
---
.../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++--
drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++-------
2 files changed, 63 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 120fa05a39ff..c736700f829e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -7,6 +7,7 @@
#define pr_fmt(fmt) "bcmgenet: " fmt
+#include <linux/acpi.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sched.h>
@@ -3476,7 +3477,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
const struct bcmgenet_plat_data *pdata;
struct bcmgenet_priv *priv;
struct net_device *dev;
- const void *macaddr;
+ const void *macaddr = NULL;
unsigned int i;
int err = -EIO;
const char *phy_mode_str;
@@ -3510,7 +3511,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
if (dn)
macaddr = of_get_mac_address(dn);
- else
+ else if (pd)
macaddr = pd->mac_address;
priv->base = devm_platform_ioremap_resource(pdev, 0);
@@ -3555,8 +3556,9 @@ static int bcmgenet_probe(struct platform_device *pdev)
priv->dev = dev;
priv->pdev = pdev;
- if (of_id) {
- pdata = of_id->data;
+
+ pdata = device_get_match_data(&pdev->dev);
+ if (pdata) {
priv->version = pdata->version;
priv->dma_max_burst_length = pdata->dma_max_burst_length;
} else {
@@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
/* If this is an internal GPHY, power it on now, before UniMAC is
* brought out of reset as absolutely no UniMAC activity is allowed
*/
- if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) &&
+ if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) &&
!strcasecmp(phy_mode_str, "internal"))
bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
@@ -3768,6 +3770,12 @@ static int bcmgenet_suspend(struct device *d)
static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume);
+static const struct acpi_device_id genet_acpi_match[] = {
+ { "BCM6E4E", (kernel_ulong_t)&bcm2711_plat_data },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, genet_acpi_match);
+
static struct platform_driver bcmgenet_driver = {
.probe = bcmgenet_probe,
.remove = bcmgenet_remove,
@@ -3776,6 +3784,7 @@ static struct platform_driver bcmgenet_driver = {
.name = "bcmgenet",
.of_match_table = bcmgenet_match,
.pm = &bcmgenet_pm_ops,
+ .acpi_match_table = ACPI_PTR(genet_acpi_match),
},
};
module_platform_driver(bcmgenet_driver);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 6392a2530183..054be1eaa1ae 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>
@@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
return 0;
}
+static void bcmgenet_phy_name(char *phy_name, int mdid, int phid)
+{
+ char mdio_bus_id[MII_BUS_ID_SIZE];
+
+ snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+ UNIMAC_MDIO_DRV_NAME, mdid);
+ snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid);
+}
+
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;
@@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev)
pr_err("could not attach to PHY\n");
return -ENODEV;
}
+ } else if (has_acpi_companion(kdev)) {
+ char phy_name[MII_BUS_ID_SIZE + 3];
+
+ bcmgenet_phy_name(phy_name, priv->pdev->id, 1);
+ phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup,
+ priv->phy_interface);
+ if (!IS_ERR(phydev))
+ phydev->dev_flags = phy_flags;
+ else
+ return -ENODEV;
} else {
phydev = dev->phydev;
phydev->dev_flags = phy_flags;
@@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
ppd.wait_func = bcmgenet_mii_wait;
ppd.wait_func_data = priv;
ppd.bus_name = "bcmgenet MII bus";
+ ppd.phy_mask = ~0;
/* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
* and is 2 * 32-bits word long, 8 bytes total.
@@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
return ret;
}
+static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv)
+{
+ struct device *kdev = &priv->pdev->dev;
+
+ priv->phy_interface = device_get_phy_mode(kdev);
+ if (priv->phy_interface < 0)
+ priv->phy_interface = PHY_INTERFACE_MODE_RGMII;
+
+ /* We need to specifically look up whether this PHY interface is internal
+ * or not *before* we even try to probe the PHY driver over MDIO as we
+ * may have shut down the internal PHY for power saving purposes.
+ */
+ if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
+ priv->internal_phy = true;
+
+ return 0;
+}
+
static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
{
struct device_node *dn = priv->pdev->dev.of_node;
- struct device *kdev = &priv->pdev->dev;
struct phy_device *phydev;
- phy_interface_t phy_mode;
int ret;
/* Fetch the PHY phandle */
@@ -500,23 +538,10 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
}
/* Get the link mode */
- ret = of_get_phy_mode(dn, &phy_mode);
- if (ret) {
- dev_err(kdev, "invalid PHY mode property\n");
- return ret;
- }
-
- priv->phy_interface = phy_mode;
-
- /* We need to specifically look up whether this PHY interface is internal
- * or not *before* we even try to probe the PHY driver over MDIO as we
- * may have shut down the internal PHY for power saving purposes.
- */
- if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
- priv->internal_phy = true;
+ bcmgenet_mii_phy_init(priv);
/* Make sure we initialize MoCA PHYs with a link down */
- if (phy_mode == PHY_INTERFACE_MODE_MOCA) {
+ if (priv->phy_interface == PHY_INTERFACE_MODE_MOCA) {
phydev = of_phy_find_device(dn);
if (phydev) {
phydev->link = 0;
@@ -532,16 +557,10 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
struct device *kdev = &priv->pdev->dev;
struct bcmgenet_platform_data *pd = kdev->platform_data;
char phy_name[MII_BUS_ID_SIZE + 3];
- char mdio_bus_id[MII_BUS_ID_SIZE];
struct phy_device *phydev;
- snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
- UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
-
if (pd->phy_interface != PHY_INTERFACE_MODE_MOCA && pd->mdio_enabled) {
- snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT,
- mdio_bus_id, pd->phy_address);
-
+ bcmgenet_phy_name(phy_name, priv->pdev->id, pd->phy_address);
/*
* Internal or external PHY with MDIO access
*/
@@ -581,10 +600,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_mii_phy_init(priv);
else
return bcmgenet_mii_pd_init(priv);
}
--
2.24.1
ARM/ACPI machines should utilize self describing hardware
when possible. The MAC address on the BCM GENET can be
read from the adapter if a full featured firmware has already
programmed it. Lets try using the address already programmed,
if it appears to be valid.
Signed-off-by: Jeremy Linton <[email protected]>
---
.../net/ethernet/broadcom/genet/bcmgenet.c | 47 ++++++++++++++-----
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index c736700f829e..6782bb0a24bd 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2779,6 +2779,27 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv,
bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1);
}
+static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv,
+ unsigned char *addr)
+{
+ u32 addr_tmp;
+ bool acpi_mode = has_acpi_companion(&priv->pdev->dev);
+
+ /* UEFI/ACPI machines and possibly others will preprogram the MAC */
+ if (acpi_mode) {
+ addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0);
+ addr[0] = addr_tmp >> 24;
+ addr[1] = (addr_tmp >> 16) & 0xff;
+ addr[2] = (addr_tmp >> 8) & 0xff;
+ addr[3] = addr_tmp & 0xff;
+ addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1);
+ addr[4] = (addr_tmp >> 8) & 0xff;
+ addr[5] = addr_tmp & 0xff;
+ } else {
+ memset(addr, 0, ETH_ALEN);
+ }
+}
+
/* Returns a reusable dma control register value */
static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv)
{
@@ -3509,11 +3530,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
}
priv->wol_irq = platform_get_irq_optional(pdev, 2);
- if (dn)
- macaddr = of_get_mac_address(dn);
- else if (pd)
- macaddr = pd->mac_address;
-
priv->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(priv->base)) {
err = PTR_ERR(priv->base);
@@ -3524,12 +3540,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
SET_NETDEV_DEV(dev, &pdev->dev);
dev_set_drvdata(&pdev->dev, dev);
- if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
- dev_warn(&pdev->dev, "using random Ethernet MAC\n");
- eth_hw_addr_random(dev);
- } else {
- ether_addr_copy(dev->dev_addr, macaddr);
- }
dev->watchdog_timeo = 2 * HZ;
dev->ethtool_ops = &bcmgenet_ethtool_ops;
dev->netdev_ops = &bcmgenet_netdev_ops;
@@ -3601,6 +3611,21 @@ static int bcmgenet_probe(struct platform_device *pdev)
!strcasecmp(phy_mode_str, "internal"))
bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
+ if (dn)
+ macaddr = of_get_mac_address(dn);
+ else if (pd)
+ macaddr = pd->mac_address;
+
+ if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
+ bcmgenet_get_hw_addr(priv, dev->dev_addr);
+ if (!is_valid_ether_addr(dev->dev_addr)) {
+ dev_warn(&pdev->dev, "using random Ethernet MAC\n");
+ eth_hw_addr_random(dev);
+ }
+ } else {
+ ether_addr_copy(dev->dev_addr, macaddr);
+ }
+
reset_umac(priv);
err = bcmgenet_mii_init(dev);
--
2.24.1
On 1/22/20 10:08 PM, Jeremy Linton wrote:
> ARM/ACPI machines should utilize self describing hardware
> when possible. The MAC address on the BCM GENET can be
> read from the adapter if a full featured firmware has already
> programmed it. Lets try using the address already programmed,
> if it appears to be valid.
s/BCM GENET/BCMGENET/ or just GENET.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> .../net/ethernet/broadcom/genet/bcmgenet.c | 47 ++++++++++++++-----
> 1 file changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index c736700f829e..6782bb0a24bd 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2779,6 +2779,27 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv,
> bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1);
> }
>
> +static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv,
> + unsigned char *addr)
> +{
> + u32 addr_tmp;
> + bool acpi_mode = has_acpi_companion(&priv->pdev->dev);
> +
> + /* UEFI/ACPI machines and possibly others will preprogram the MAC */
> + if (acpi_mode) {
> + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0);
> + addr[0] = addr_tmp >> 24;
> + addr[1] = (addr_tmp >> 16) & 0xff;
> + addr[2] = (addr_tmp >> 8) & 0xff;
> + addr[3] = addr_tmp & 0xff;
> + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1);
> + addr[4] = (addr_tmp >> 8) & 0xff;
> + addr[5] = addr_tmp & 0xff;
> + } else {
> + memset(addr, 0, ETH_ALEN);
> + }
> +}
> +
> /* Returns a reusable dma control register value */
> static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv)
> {
> @@ -3509,11 +3530,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
> }
> priv->wol_irq = platform_get_irq_optional(pdev, 2);
>
> - if (dn)
> - macaddr = of_get_mac_address(dn);
> - else if (pd)
> - macaddr = pd->mac_address;
> -
> priv->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(priv->base)) {
> err = PTR_ERR(priv->base);
> @@ -3524,12 +3540,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
>
> SET_NETDEV_DEV(dev, &pdev->dev);
> dev_set_drvdata(&pdev->dev, dev);
> - if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
> - dev_warn(&pdev->dev, "using random Ethernet MAC\n");
> - eth_hw_addr_random(dev);
> - } else {
> - ether_addr_copy(dev->dev_addr, macaddr);
> - }
> dev->watchdog_timeo = 2 * HZ;
> dev->ethtool_ops = &bcmgenet_ethtool_ops;
> dev->netdev_ops = &bcmgenet_netdev_ops;
> @@ -3601,6 +3611,21 @@ static int bcmgenet_probe(struct platform_device *pdev)
> !strcasecmp(phy_mode_str, "internal"))
> bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
>
> + if (dn)
> + macaddr = of_get_mac_address(dn);
> + else if (pd)
> + macaddr = pd->mac_address;
I would be adding an:
else if (has_acpi_companion())
bcmgenet_get_hw_addr(priv, macaddr);
such that bcmgenet_get_hw_addr() does not have much ACPI specific
knowledge and get be used outside, with the caller knowing whether it is
appropriate.
You should also indicate in your commit message that you are moving the
fetching of the MAC address at a later point where the clocks are turned
on, to guarantee that the registers can be read.
With that fixed:
Acked-by: Florian Fainelli <[email protected]>
> +
> + if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
> + bcmgenet_get_hw_addr(priv, dev->dev_addr);
> + if (!is_valid_ether_addr(dev->dev_addr)) {
> + dev_warn(&pdev->dev, "using random Ethernet MAC\n");
> + eth_hw_addr_random(dev);
> + }
> + } else {
> + ether_addr_copy(dev->dev_addr, macaddr);
> + }
> +
> reset_umac(priv);
>
> err = bcmgenet_mii_init(dev);
>
--
Florian
On 1/22/20 10:08 PM, Jeremy Linton wrote:
> The rpi4 is capable of booting in ACPI mode with the latest
> edk2-platform commits. As such, it would be helpful if the genet
> platform device were usable.
>
> To achive this we convert some of the of_ calls to device_ and
> add the ACPI id module table, and tweak the phy connection code
> to use phy_connect() in the ACPI path.
This seems reasonable to me at first glance, although I would be
splitting the bcmgenet.c changes from the bcmmii.c for clarity.
There are some more specific comments below.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> .../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++--
> drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++-------
> 2 files changed, 63 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 120fa05a39ff..c736700f829e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -7,6 +7,7 @@
>
> #define pr_fmt(fmt) "bcmgenet: " fmt
>
> +#include <linux/acpi.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/sched.h>
> @@ -3476,7 +3477,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
> const struct bcmgenet_plat_data *pdata;
> struct bcmgenet_priv *priv;
> struct net_device *dev;
> - const void *macaddr;
> + const void *macaddr = NULL;
> unsigned int i;
> int err = -EIO;
> const char *phy_mode_str;
> @@ -3510,7 +3511,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>
> if (dn)
> macaddr = of_get_mac_address(dn);
> - else
> + else if (pd)
> macaddr = pd->mac_address;
>
> priv->base = devm_platform_ioremap_resource(pdev, 0);
> @@ -3555,8 +3556,9 @@ static int bcmgenet_probe(struct platform_device *pdev)
>
> priv->dev = dev;
> priv->pdev = pdev;
> - if (of_id) {
> - pdata = of_id->data;
> +
> + pdata = device_get_match_data(&pdev->dev);
> + if (pdata) {
> priv->version = pdata->version;
> priv->dma_max_burst_length = pdata->dma_max_burst_length;
> } else {
> @@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
> /* If this is an internal GPHY, power it on now, before UniMAC is
> * brought out of reset as absolutely no UniMAC activity is allowed
> */
> - if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) &&
> + if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) &&
> !strcasecmp(phy_mode_str, "internal"))
> bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
>
> @@ -3768,6 +3770,12 @@ static int bcmgenet_suspend(struct device *d)
>
> static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume);
>
> +static const struct acpi_device_id genet_acpi_match[] = {
> + { "BCM6E4E", (kernel_ulong_t)&bcm2711_plat_data },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, genet_acpi_match);
> +
> static struct platform_driver bcmgenet_driver = {
> .probe = bcmgenet_probe,
> .remove = bcmgenet_remove,
> @@ -3776,6 +3784,7 @@ static struct platform_driver bcmgenet_driver = {
> .name = "bcmgenet",
> .of_match_table = bcmgenet_match,
> .pm = &bcmgenet_pm_ops,
> + .acpi_match_table = ACPI_PTR(genet_acpi_match),
> },
> };
> module_platform_driver(bcmgenet_driver);
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 6392a2530183..054be1eaa1ae 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>
> @@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
> return 0;
> }
>
> +static void bcmgenet_phy_name(char *phy_name, int mdid, int phid)
> +{
> + char mdio_bus_id[MII_BUS_ID_SIZE];
> +
> + snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
> + UNIMAC_MDIO_DRV_NAME, mdid);
> + snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid);
> +}
> +
> 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;
> @@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev)
> pr_err("could not attach to PHY\n");
> return -ENODEV;
> }
> + } else if (has_acpi_companion(kdev)) {
> + char phy_name[MII_BUS_ID_SIZE + 3];
> +
> + bcmgenet_phy_name(phy_name, priv->pdev->id, 1);
There is no guarantee that 1 is valid other than for the current
Raspberry Pi 4 design that we have in the wild, would ACPI be used in
the future with other designs, this would likely be different. Can you
find a way to communicate that address via appropriate firmware properties?
> + phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup,
> + priv->phy_interface);
> + if (!IS_ERR(phydev))
> + phydev->dev_flags = phy_flags;
> + else
> + return -ENODEV;
> } else {
> phydev = dev->phydev;
> phydev->dev_flags = phy_flags;
> @@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
> ppd.wait_func = bcmgenet_mii_wait;
> ppd.wait_func_data = priv;
> ppd.bus_name = "bcmgenet MII bus";
> + ppd.phy_mask = ~0;
This is going to be breaking PHY scanning for the platform_data case, so
maybe something like:
if (acpi_has_companion())
ppd.phy_mask = ~BIT(acpi_phy_id);
or something like that?
>
> /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
> * and is 2 * 32-bits word long, 8 bytes total.
> @@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
> return ret;
> }
>
> +static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv)
> +{
Maybe name that phy_interface_init(), there is not strictly much PHY
initialization going on here, just property fetching and internal book
keeping.
--
Florian
Hi,
First, thanks for taking a look at this.
On 1/23/20 3:22 PM, Florian Fainelli wrote:
> On 1/22/20 10:08 PM, Jeremy Linton wrote:
>> The rpi4 is capable of booting in ACPI mode with the latest
>> edk2-platform commits. As such, it would be helpful if the genet
>> platform device were usable.
>>
>> To achive this we convert some of the of_ calls to device_ and
>> add the ACPI id module table, and tweak the phy connection code
>> to use phy_connect() in the ACPI path.
>
> This seems reasonable to me at first glance, although I would be
> splitting the bcmgenet.c changes from the bcmmii.c for clarity.
Sure.
>
> There are some more specific comments below.
>
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> .../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++--
>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++-------
>> 2 files changed, 63 insertions(+), 32 deletions(-)
>>
(trimming some)
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 6392a2530183..054be1eaa1ae 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>
>> @@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
>> return 0;
>> }
>>
>> +static void bcmgenet_phy_name(char *phy_name, int mdid, int phid)
>> +{
>> + char mdio_bus_id[MII_BUS_ID_SIZE];
>> +
>> + snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
>> + UNIMAC_MDIO_DRV_NAME, mdid);
>> + snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid);
>> +}
>> +
>> 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;
>> @@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev)
>> pr_err("could not attach to PHY\n");
>> return -ENODEV;
>> }
>> + } else if (has_acpi_companion(kdev)) {
>> + char phy_name[MII_BUS_ID_SIZE + 3];
>> +
>> + bcmgenet_phy_name(phy_name, priv->pdev->id, 1);
>
> There is no guarantee that 1 is valid other than for the current
> Raspberry Pi 4 design that we have in the wild, would ACPI be used in
> the future with other designs, this would likely be different. Can you
> find a way to communicate that address via appropriate firmware properties?
Your right, that "1" seems like it should be dynamic despite a large
number of these nic drivers hardcoding the phy address.
Another _DSD property could be added, but that likely just moves the
hardcoding from one place to another. Particularly, given that the
mdiobus_register() phy scanning is working correctly and the machine
knows what the phy address is.
AFAIK, The correct choice is something like phy_find_first(), but the
mii_bus structure is layered down in the unimac module's private data,
which we could retrieve, but that would be really ugly.
There is of_mdio_find_bus(), but that doesn't apply either. A generic
mdio_find_bus() that takes the bus name string, or maybe the parent
device and does the bus_find_device itself would be helpful.
Suggestions?
>
>> + phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup,
>> + priv->phy_interface);
>> + if (!IS_ERR(phydev))
>> + phydev->dev_flags = phy_flags;
>> + else
>> + return -ENODEV;
>> } else {
>> phydev = dev->phydev;
>> phydev->dev_flags = phy_flags;
>> @@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
>> ppd.wait_func = bcmgenet_mii_wait;
>> ppd.wait_func_data = priv;
>> ppd.bus_name = "bcmgenet MII bus";
>> + ppd.phy_mask = ~0;
>
> This is going to be breaking PHY scanning for the platform_data case, so
> maybe something like:
Does it? I thought it was being reset in bcmgenet_mii_pdata_init(). I
guess I don't fully understand the what happens in PHY_INTERFACE_MODE_MOCA.
>
> if (acpi_has_companion())
> ppd.phy_mask = ~BIT(acpi_phy_id);
>
> or something like that?
Sure, if nothing else I can wrap the mask in if (acpi) though, as I
mentioned above I've got some reservations about picking up the phy id
from the firmware unless its absolutely required.
>
>>
>> /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
>> * and is 2 * 32-bits word long, 8 bytes total.
>> @@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
>> return ret;
>> }
>>
>> +static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv)
>> +{
>
> Maybe name that phy_interface_init(), there is not strictly much PHY
> initialization going on here, just property fetching and internal book
> keeping.
>
Yup, sounds good.
Thanks, again!