[PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[Petri Gynther]

Broadcom 7xxx MIPS-based STB platforms do not use device trees.
Modify bcmgenet driver so that it can be used on those platforms.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/net/ethernet/broadcom/Kconfig          |   1 -
 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  30 +++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  11 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 169 +++++++++++++++++++++----
 4 files changed, 179 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index c3e260c..888247a 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -62,7 +62,6 @@ config BCM63XX_ENET
 
 config BCMGENET
 	tristate "Broadcom GENET internal MAC support"
-	depends on OF
 	select MII
 	select PHYLIB
 	select FIXED_PHY if BCMGENET=y
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index fff2634..1ec7a32 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2475,6 +2475,7 @@ static const struct of_device_id bcmgenet_match[] = {
 
 static int bcmgenet_probe(struct platform_device *pdev)
 {
+	struct bcmgenet_platform_data *pd = pdev->dev.platform_data;
 	struct device_node *dn = pdev->dev.of_node;
 	const struct of_device_id *of_id;
 	struct bcmgenet_priv *priv;
@@ -2490,9 +2491,13 @@ static int bcmgenet_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	of_id = of_match_node(bcmgenet_match, dn);
-	if (!of_id)
-		return -EINVAL;
+	if (dn) {
+		of_id = of_match_node(bcmgenet_match, dn);
+		if (!of_id)
+			return -EINVAL;
+	} else {
+		of_id = NULL;
+	}
 
 	priv = netdev_priv(dev);
 	priv->irq0 = platform_get_irq(pdev, 0);
@@ -2504,11 +2509,15 @@ static int bcmgenet_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	macaddr = of_get_mac_address(dn);
-	if (!macaddr) {
-		dev_err(&pdev->dev, "can't find MAC address\n");
-		err = -EINVAL;
-		goto err;
+	if (dn) {
+		macaddr = of_get_mac_address(dn);
+		if (!macaddr) {
+			dev_err(&pdev->dev, "can't find MAC address\n");
+			err = -EINVAL;
+			goto err;
+		}
+	} else {
+		macaddr = pd->macaddr;
 	}
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -2548,7 +2557,10 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 	priv->pdev = pdev;
-	priv->version = (enum bcmgenet_version)of_id->data;
+	if (of_id)
+		priv->version = (enum bcmgenet_version)of_id->data;
+	else
+		priv->version = pd->genet_version;
 
 	priv->clk = devm_clk_get(&priv->pdev->dev, "enet");
 	if (IS_ERR(priv->clk))
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index dbf524e..5191e3f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -17,6 +17,17 @@
 #include <linux/if_vlan.h>
 #include <linux/phy.h>
 
+struct bcmgenet_platform_data {
+	void __iomem	*base_reg;
+	int		irq0;
+	int		irq1;
+	int		phy_type;
+	int		phy_addr;
+	int		phy_speed;
+	u8		macaddr[ETH_ALEN];
+	int		genet_version;
+};
+
 /* total number of Buffer Descriptors, same for Rx/Tx */
 #define TOTAL_DESC				256
 
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 9ff799a..e5ff913 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
 	phy_print_status(phydev);
 }
 
+static int bcmgenet_moca_fphy_update(struct net_device *dev,
+				     struct fixed_phy_status *status)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	struct phy_device *phydev = priv->phydev;
+
+	/*
+	 * MoCA daemon updates phydev->autoneg to reflect the link status.
+	 * Update MoCA fixed PHY status accordingly, so that the PHY state
+	 * machine becomes aware of the real link status.
+	 */
+	status->link = phydev->autoneg;
+	return 0;
+}
+
 void bcmgenet_mii_reset(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -311,22 +326,6 @@ static int bcmgenet_mii_probe(struct net_device *dev)
 	u32 phy_flags;
 	int ret;
 
-	if (priv->phydev) {
-		pr_info("PHY already attached\n");
-		return 0;
-	}
-
-	/* In the case of a fixed PHY, the DT node associated
-	 * to the PHY is the Ethernet MAC DT node.
-	 */
-	if (!priv->phy_dn && of_phy_is_fixed_link(dn)) {
-		ret = of_phy_register_fixed_link(dn);
-		if (ret)
-			return ret;
-
-		priv->phy_dn = of_node_get(dn);
-	}
-
 	/* Communicate the integrated PHY revision */
 	phy_flags = priv->gphy_rev;
 
@@ -336,11 +335,39 @@ static int bcmgenet_mii_probe(struct net_device *dev)
 	priv->old_duplex = -1;
 	priv->old_pause = -1;
 
-	phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup,
-				phy_flags, priv->phy_interface);
-	if (!phydev) {
-		pr_err("could not attach to PHY\n");
-		return -ENODEV;
+	if (dn) {
+		if (priv->phydev) {
+			pr_info("PHY already attached\n");
+			return 0;
+		}
+
+		/* In the case of a fixed PHY, the DT node associated
+		 * to the PHY is the Ethernet MAC DT node.
+		 */
+		if (!priv->phy_dn && of_phy_is_fixed_link(dn)) {
+			ret = of_phy_register_fixed_link(dn);
+			if (ret)
+				return ret;
+
+			priv->phy_dn = of_node_get(dn);
+		}
+
+		phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup,
+					phy_flags, priv->phy_interface);
+		if (!phydev) {
+			pr_err("could not attach to PHY\n");
+			return -ENODEV;
+		}
+	} else {
+		phydev = priv->phydev;
+		phydev->dev_flags = phy_flags;
+
+		ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,
+					 priv->phy_interface);
+		if (ret) {
+			pr_err("could not attach to PHY\n");
+			return -ENODEV;
+		}
 	}
 
 	priv->phydev = phydev;
@@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 	return 0;
 }
 
+static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
+{
+	struct device *kdev = &priv->pdev->dev;
+	struct bcmgenet_platform_data *pd = kdev->platform_data;
+	struct mii_bus *mdio = priv->mii_bus;
+	int phy_addr = pd->phy_addr;
+	struct phy_device *phydev;
+	int ret;
+	int i;
+
+	/* Disable automatic MDIO bus scan */
+	mdio->phy_mask = ~0;
+
+	/* Clear all the IRQ properties */
+	if (mdio->irq)
+		for (i = 0; i < PHY_MAX_ADDR; i++)
+			mdio->irq[i] = PHY_POLL;
+
+	/* Register the MDIO bus */
+	ret = mdiobus_register(mdio);
+	if (ret) {
+		dev_err(kdev, "failed to register MDIO bus\n");
+		return ret;
+	}
+
+	/*
+	 * bcmgenet_platform_data needs to pass a valid PHY address for
+	 * internal/external PHY or -1 for MoCA PHY.
+	 */
+	if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {
+		/*
+		 * 10/100/1000 Ethernet port with external or internal PHY.
+		 */
+		phydev = get_phy_device(mdio, phy_addr, false);
+		if (!phydev || IS_ERR(phydev)) {
+			dev_err(kdev, "failed to create PHY device\n");
+			mdiobus_unregister(mdio);
+			return 1;
+		}
+
+		phydev->irq = PHY_POLL;
+
+		ret = phy_device_register(phydev);
+		if (ret) {
+			dev_err(kdev, "failed to register PHY device\n");
+			phy_device_free(phydev);
+			mdiobus_unregister(mdio);
+			return 1;
+		}
+
+		priv->phydev = phydev;
+		priv->phy_interface = pd->phy_type;
+	} else {
+		/*
+		 * MoCA port with no MDIO-accessible PHY.
+		 * We need to use 1000/HD fixed PHY to represent the link layer.
+		 * MoCA daemon interacts with this PHY via ethtool.
+		 */
+		struct fixed_phy_status moca_fphy_status = {
+			.link = 0,
+			.duplex = 0,
+			.speed = 1000,
+			.pause = 0,
+			.asym_pause = 0,
+		};
+
+		phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
+		if (!phydev || IS_ERR(phydev)) {
+			dev_err(kdev, "failed to register fixed PHY device\n");
+			mdiobus_unregister(mdio);
+			return 1;
+		}
+
+		phydev->autoneg = AUTONEG_DISABLE;
+
+		ret = fixed_phy_set_link_update(phydev,
+						bcmgenet_moca_fphy_update);
+		if (ret) {
+			dev_err(kdev, "failed to set fixed PHY link update\n");
+		}
+
+		priv->phydev = phydev;
+		priv->phy_interface = PHY_INTERFACE_MODE_MOCA;
+	}
+
+	return 0;
+}
+
+static int bcmgenet_mii_bus_init(struct bcmgenet_priv *priv)
+{
+	struct device_node *dn = priv->pdev->dev.of_node;
+
+	if (dn)
+		return bcmgenet_mii_of_init(priv);
+	else
+		return bcmgenet_mii_pd_init(priv);
+}
+
 int bcmgenet_mii_init(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -446,7 +571,7 @@ int bcmgenet_mii_init(struct net_device *dev)
 	if (ret)
 		return ret;
 
-	ret = bcmgenet_mii_of_init(priv);
+	ret = bcmgenet_mii_bus_init(priv);
 	if (ret)
 		goto out_free;
 
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[Florian Fainelli]

On 10/10/2014 11:35 AM, Petri Gynther wrote:
> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
> Modify bcmgenet driver so that it can be used on those platforms.
> 
> Signed-off-by: Petri Gynther <pgynther@google.com>
> ---

[snip]

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index dbf524e..5191e3f 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -17,6 +17,17 @@
>  #include <linux/if_vlan.h>
>  #include <linux/phy.h>
>  
> +struct bcmgenet_platform_data {
> +	void __iomem	*base_reg;
> +	int		irq0;
> +	int		irq1;

Why would these members here? The platform device should provide those
as standard resources that the driver fetches using
platform_get_resource() and platform_get_irq().

> +	int		phy_type;
> +	int		phy_addr;
> +	int		phy_speed;
> +	u8		macaddr[ETH_ALEN];
> +	int		genet_version;
> +};

I would rather we put this in include/linux/platform_data/bcmgenet.h
where it belongs.

> +
>  /* total number of Buffer Descriptors, same for Rx/Tx */
>  #define TOTAL_DESC				256
>  
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 9ff799a..e5ff913 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
>  	phy_print_status(phydev);
>  }
>  
> +static int bcmgenet_moca_fphy_update(struct net_device *dev,
> +				     struct fixed_phy_status *status)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +	struct phy_device *phydev = priv->phydev;
> +
> +	/*
> +	 * MoCA daemon updates phydev->autoneg to reflect the link status.
> +	 * Update MoCA fixed PHY status accordingly, so that the PHY state
> +	 * machine becomes aware of the real link status.
> +	 */
> +	status->link = phydev->autoneg;
> +	return 0;
> +}

I don't want to see that in the upstream driver, please enable the link
interrupts like I suggested before and do not use the autoneg field at
all, which should require no MoCA daemon modifications.

[snip]

>  
>  	priv->phydev = phydev;
> @@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>  	return 0;
>  }
>  
> +static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
> +{
> +	struct device *kdev = &priv->pdev->dev;
> +	struct bcmgenet_platform_data *pd = kdev->platform_data;
> +	struct mii_bus *mdio = priv->mii_bus;
> +	int phy_addr = pd->phy_addr;
> +	struct phy_device *phydev;
> +	int ret;
> +	int i;
> +
> +	/* Disable automatic MDIO bus scan */
> +	mdio->phy_mask = ~0;
> +
> +	/* Clear all the IRQ properties */
> +	if (mdio->irq)
> +		for (i = 0; i < PHY_MAX_ADDR; i++)
> +			mdio->irq[i] = PHY_POLL;
> +
> +	/* Register the MDIO bus */
> +	ret = mdiobus_register(mdio);
> +	if (ret) {
> +		dev_err(kdev, "failed to register MDIO bus\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * bcmgenet_platform_data needs to pass a valid PHY address for
> +	 * internal/external PHY or -1 for MoCA PHY.
> +	 */
> +	if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {

We do too much low-level PHY device handling, and since you already have
the phy_type provided via platform_data, we can use that hint to do the
following:

1) an internal or external PHY with MDIO accesses should leave the bus
auto-probing on with the specified PHY address in the mdio bus phy_mask

2) a MoCA PHY or an external PHY with MDIO accesses disabled should use
the fixed-0 MII bus instead.

This would look like this:

if (pd->phy_type != PHY_INTERFACE_MODE_MOCA || pd->mdio_enabled)
	mdio->phy_mask = ~(1 << pd->phy_addr);

	...
	mdiobus_register()

	priv->phydev = mdio->bus->phy_map[pd->phy_addr];

	phydev->phy_flags |= mask;

if (pd->phy_type == PHY_INTERFACE_MODE_MOCA || !pd->mdio_enabled)
	priv->phydev = fixed_phy_register(...);

and in both cases, later on you do connect to the PHY device

I can cook a patch to illustrate what I think this could look like since
I realize using pseudo-code to explain might not be the best thing.

> +		/*
> +		 * 10/100/1000 Ethernet port with external or internal PHY.
> +		 */
> +		phydev = get_phy_device(mdio, phy_addr, false);
> +		if (!phydev || IS_ERR(phydev)) {
> +			dev_err(kdev, "failed to create PHY device\n");
> +			mdiobus_unregister(mdio);
> +			return 1;
> +		}
> +
> +		phydev->irq = PHY_POLL;
> +
> +		ret = phy_device_register(phydev);
> +		if (ret) {
> +			dev_err(kdev, "failed to register PHY device\n");
> +			phy_device_free(phydev);
> +			mdiobus_unregister(mdio);
> +			return 1;
> +		}
> +
> +		priv->phydev = phydev;
> +		priv->phy_interface = pd->phy_type;
> +	} else {
> +		/*
> +		 * MoCA port with no MDIO-accessible PHY.
> +		 * We need to use 1000/HD fixed PHY to represent the link layer.
> +		 * MoCA daemon interacts with this PHY via ethtool.
> +		 */
> +		struct fixed_phy_status moca_fphy_status = {
> +			.link = 0,
> +			.duplex = 0,

This should be DUPLEX_FULL here, the link between GENET and the MoCA
Ethernet convergence layer is full-duplex by nature (despite we report
the PHY being half-duplex, which is a mistake in the downstream driver),
the MoCA medium on the coaxial cable is half-duplex though, but that is
handled by the MoCA HW.

NB: I had issues in the past using a half-duplex link with the MoCA
ethernet convergence layer, causing various types of packet loss because
we use a simplified signaling internally in the hardware.

> +			.speed = 1000,
> +			.pause = 0,
> +			.asym_pause = 0,
> +		};
> +
> +		phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
> +		if (!phydev || IS_ERR(phydev)) {
> +			dev_err(kdev, "failed to register fixed PHY device\n");
> +			mdiobus_unregister(mdio);
> +			return 1;
> +		}
> +
> +		phydev->autoneg = AUTONEG_DISABLE;
> +
> +		ret = fixed_phy_set_link_update(phydev,
> +						bcmgenet_moca_fphy_update);
> +		if (ret) {
> +			dev_err(kdev, "failed to set fixed PHY link update\n");
> +		}

Should not we propagate this error to the caller?
--
Florian

Re: [PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[Petri Gynther]

Hi Florian,

On Fri, Oct 10, 2014 at 11:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/10/2014 11:35 AM, Petri Gynther wrote:
>> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
>> Modify bcmgenet driver so that it can be used on those platforms.
>>
>> Signed-off-by: Petri Gynther <pgynther@google.com>
>> ---
>
> [snip]
>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> index dbf524e..5191e3f 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> @@ -17,6 +17,17 @@
>>  #include <linux/if_vlan.h>
>>  #include <linux/phy.h>
>>
>> +struct bcmgenet_platform_data {
>> +     void __iomem    *base_reg;
>> +     int             irq0;
>> +     int             irq1;
>
> Why would these members here? The platform device should provide those
> as standard resources that the driver fetches using
> platform_get_resource() and platform_get_irq().
>

I modeled this on struct bcmemac_platform_data that was used in the
legacy BRCMSTB code.
include/linux/brcmstb/brcmstb.h:

struct bcmemac_platform_data {
        /* used by the BSP code only */
        uintptr_t               base_reg;
        int                     irq0;
        int                     irq1;

        int                     phy_type;
        int                     phy_id;
        int                     phy_speed;
        u8                      macaddr[ETH_ALEN];
};

The legacy BRCMSTB code stores all relevant GENET info in this struct
and then creates the resources from that info:

static void __init brcm_register_genet(int id, struct bcmemac_platform_data *pd)
{
        struct resource res[3];
        struct platform_device *pdev;

        memset(&res, 0, sizeof(res));
        res[0].start = BPHYSADDR(pd->base_reg);
        res[0].end = BPHYSADDR(pd->base_reg + 0x4fff);
        res[0].flags = IORESOURCE_MEM;

        res[1].start = res[1].end = pd->irq0;
        res[1].flags = IORESOURCE_IRQ;

        res[2].start = res[2].end = pd->irq1;
        res[2].flags = IORESOURCE_IRQ;

        brcm_alloc_macaddr(pd->macaddr);

        pdev = platform_device_alloc("bcmgenet", id);
        platform_device_add_resources(pdev, res, 3);
        platform_device_add_data(pdev, pd, sizeof(*pd));
        platform_device_add(pdev);
}

>> +     int             phy_type;
>> +     int             phy_addr;
>> +     int             phy_speed;
>> +     u8              macaddr[ETH_ALEN];
>> +     int             genet_version;
>> +};
>
> I would rather we put this in include/linux/platform_data/bcmgenet.h
> where it belongs.
>

I wasn't aware of the directory include/linux/platform_data/. Yes,
that's where this belongs.

>> +
>>  /* total number of Buffer Descriptors, same for Rx/Tx */
>>  #define TOTAL_DESC                           256
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 9ff799a..e5ff913 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
>>       phy_print_status(phydev);
>>  }
>>
>> +static int bcmgenet_moca_fphy_update(struct net_device *dev,
>> +                                  struct fixed_phy_status *status)
>> +{
>> +     struct bcmgenet_priv *priv = netdev_priv(dev);
>> +     struct phy_device *phydev = priv->phydev;
>> +
>> +     /*
>> +      * MoCA daemon updates phydev->autoneg to reflect the link status.
>> +      * Update MoCA fixed PHY status accordingly, so that the PHY state
>> +      * machine becomes aware of the real link status.
>> +      */
>> +     status->link = phydev->autoneg;
>> +     return 0;
>> +}
>
> I don't want to see that in the upstream driver, please enable the link
> interrupts like I suggested before and do not use the autoneg field at
> all, which should require no MoCA daemon modifications.
>

I added debug printk's to bcmgenet_isr0 to check on UMAC_IRQ_LINK_UP
and UMAC_IRQ_LINK_DOWN.
I am not getting those interrupts on eth1 (MoCA) port when coax is
removed/inserted.
But, they do work on eth0.

I'll modify init_umac() to enable those interrupts for MoCA port and retest.

> [snip]
>
>>
>>       priv->phydev = phydev;
>> @@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>>       return 0;
>>  }
>>
>> +static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>> +{
>> +     struct device *kdev = &priv->pdev->dev;
>> +     struct bcmgenet_platform_data *pd = kdev->platform_data;
>> +     struct mii_bus *mdio = priv->mii_bus;
>> +     int phy_addr = pd->phy_addr;
>> +     struct phy_device *phydev;
>> +     int ret;
>> +     int i;
>> +
>> +     /* Disable automatic MDIO bus scan */
>> +     mdio->phy_mask = ~0;
>> +
>> +     /* Clear all the IRQ properties */
>> +     if (mdio->irq)
>> +             for (i = 0; i < PHY_MAX_ADDR; i++)
>> +                     mdio->irq[i] = PHY_POLL;
>> +
>> +     /* Register the MDIO bus */
>> +     ret = mdiobus_register(mdio);
>> +     if (ret) {
>> +             dev_err(kdev, "failed to register MDIO bus\n");
>> +             return ret;
>> +     }
>> +
>> +     /*
>> +      * bcmgenet_platform_data needs to pass a valid PHY address for
>> +      * internal/external PHY or -1 for MoCA PHY.
>> +      */
>> +     if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {
>
> We do too much low-level PHY device handling, and since you already have
> the phy_type provided via platform_data, we can use that hint to do the
> following:
>
> 1) an internal or external PHY with MDIO accesses should leave the bus
> auto-probing on with the specified PHY address in the mdio bus phy_mask
>
> 2) a MoCA PHY or an external PHY with MDIO accesses disabled should use
> the fixed-0 MII bus instead.
>
> This would look like this:
>
> if (pd->phy_type != PHY_INTERFACE_MODE_MOCA || pd->mdio_enabled)
>         mdio->phy_mask = ~(1 << pd->phy_addr);
>
>         ...
>         mdiobus_register()
>
>         priv->phydev = mdio->bus->phy_map[pd->phy_addr];
>
>         phydev->phy_flags |= mask;
>
> if (pd->phy_type == PHY_INTERFACE_MODE_MOCA || !pd->mdio_enabled)
>         priv->phydev = fixed_phy_register(...);
>
> and in both cases, later on you do connect to the PHY device
>
> I can cook a patch to illustrate what I think this could look like since
> I realize using pseudo-code to explain might not be the best thing.
>

I think I understand what you mean. I'll make a change.

>> +             /*
>> +              * 10/100/1000 Ethernet port with external or internal PHY.
>> +              */
>> +             phydev = get_phy_device(mdio, phy_addr, false);
>> +             if (!phydev || IS_ERR(phydev)) {
>> +                     dev_err(kdev, "failed to create PHY device\n");
>> +                     mdiobus_unregister(mdio);
>> +                     return 1;
>> +             }
>> +
>> +             phydev->irq = PHY_POLL;
>> +
>> +             ret = phy_device_register(phydev);
>> +             if (ret) {
>> +                     dev_err(kdev, "failed to register PHY device\n");
>> +                     phy_device_free(phydev);
>> +                     mdiobus_unregister(mdio);
>> +                     return 1;
>> +             }
>> +
>> +             priv->phydev = phydev;
>> +             priv->phy_interface = pd->phy_type;
>> +     } else {
>> +             /*
>> +              * MoCA port with no MDIO-accessible PHY.
>> +              * We need to use 1000/HD fixed PHY to represent the link layer.
>> +              * MoCA daemon interacts with this PHY via ethtool.
>> +              */
>> +             struct fixed_phy_status moca_fphy_status = {
>> +                     .link = 0,
>> +                     .duplex = 0,
>
> This should be DUPLEX_FULL here, the link between GENET and the MoCA
> Ethernet convergence layer is full-duplex by nature (despite we report
> the PHY being half-duplex, which is a mistake in the downstream driver),
> the MoCA medium on the coaxial cable is half-duplex though, but that is
> handled by the MoCA HW.
>
> NB: I had issues in the past using a half-duplex link with the MoCA
> ethernet convergence layer, causing various types of packet loss because
> we use a simplified signaling internally in the hardware.
>

I picked this setting from 3.3 GENET driver. I'll test 1000/FULL on my
platform to see if it works.

>> +                     .speed = 1000,
>> +                     .pause = 0,
>> +                     .asym_pause = 0,
>> +             };
>> +
>> +             phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
>> +             if (!phydev || IS_ERR(phydev)) {
>> +                     dev_err(kdev, "failed to register fixed PHY device\n");
>> +                     mdiobus_unregister(mdio);
>> +                     return 1;
>> +             }
>> +
>> +             phydev->autoneg = AUTONEG_DISABLE;
>> +
>> +             ret = fixed_phy_set_link_update(phydev,
>> +                                             bcmgenet_moca_fphy_update);
>> +             if (ret) {
>> +                     dev_err(kdev, "failed to set fixed PHY link update\n");
>> +             }
>
> Should not we propagate this error to the caller?

Good catch. Yes.

> --
> Florian

Re: [PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[Petri Gynther]

Hi Florian,

Getting back to this (finally). I have made changes per your comments.
Sending a new patch shortly.

-- Petri

On Fri, Oct 10, 2014 at 12:46 PM, Petri Gynther <pgynther@google.com> wrote:
> Hi Florian,
>
> On Fri, Oct 10, 2014 at 11:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/10/2014 11:35 AM, Petri Gynther wrote:
>>> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
>>> Modify bcmgenet driver so that it can be used on those platforms.
>>>
>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>> ---
>>
>> [snip]
>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> index dbf524e..5191e3f 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> @@ -17,6 +17,17 @@
>>>  #include <linux/if_vlan.h>
>>>  #include <linux/phy.h>
>>>
>>> +struct bcmgenet_platform_data {
>>> +     void __iomem    *base_reg;
>>> +     int             irq0;
>>> +     int             irq1;
>>
>> Why would these members here? The platform device should provide those
>> as standard resources that the driver fetches using
>> platform_get_resource() and platform_get_irq().
>>
>
> I modeled this on struct bcmemac_platform_data that was used in the
> legacy BRCMSTB code.
> include/linux/brcmstb/brcmstb.h:
>
> struct bcmemac_platform_data {
>         /* used by the BSP code only */
>         uintptr_t               base_reg;
>         int                     irq0;
>         int                     irq1;
>
>         int                     phy_type;
>         int                     phy_id;
>         int                     phy_speed;
>         u8                      macaddr[ETH_ALEN];
> };
>
> The legacy BRCMSTB code stores all relevant GENET info in this struct
> and then creates the resources from that info:
>
> static void __init brcm_register_genet(int id, struct bcmemac_platform_data *pd)
> {
>         struct resource res[3];
>         struct platform_device *pdev;
>
>         memset(&res, 0, sizeof(res));
>         res[0].start = BPHYSADDR(pd->base_reg);
>         res[0].end = BPHYSADDR(pd->base_reg + 0x4fff);
>         res[0].flags = IORESOURCE_MEM;
>
>         res[1].start = res[1].end = pd->irq0;
>         res[1].flags = IORESOURCE_IRQ;
>
>         res[2].start = res[2].end = pd->irq1;
>         res[2].flags = IORESOURCE_IRQ;
>
>         brcm_alloc_macaddr(pd->macaddr);
>
>         pdev = platform_device_alloc("bcmgenet", id);
>         platform_device_add_resources(pdev, res, 3);
>         platform_device_add_data(pdev, pd, sizeof(*pd));
>         platform_device_add(pdev);
> }
>
>>> +     int             phy_type;
>>> +     int             phy_addr;
>>> +     int             phy_speed;
>>> +     u8              macaddr[ETH_ALEN];
>>> +     int             genet_version;
>>> +};
>>
>> I would rather we put this in include/linux/platform_data/bcmgenet.h
>> where it belongs.
>>
>
> I wasn't aware of the directory include/linux/platform_data/. Yes,
> that's where this belongs.
>
>>> +
>>>  /* total number of Buffer Descriptors, same for Rx/Tx */
>>>  #define TOTAL_DESC                           256
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 9ff799a..e5ff913 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
>>>       phy_print_status(phydev);
>>>  }
>>>
>>> +static int bcmgenet_moca_fphy_update(struct net_device *dev,
>>> +                                  struct fixed_phy_status *status)
>>> +{
>>> +     struct bcmgenet_priv *priv = netdev_priv(dev);
>>> +     struct phy_device *phydev = priv->phydev;
>>> +
>>> +     /*
>>> +      * MoCA daemon updates phydev->autoneg to reflect the link status.
>>> +      * Update MoCA fixed PHY status accordingly, so that the PHY state
>>> +      * machine becomes aware of the real link status.
>>> +      */
>>> +     status->link = phydev->autoneg;
>>> +     return 0;
>>> +}
>>
>> I don't want to see that in the upstream driver, please enable the link
>> interrupts like I suggested before and do not use the autoneg field at
>> all, which should require no MoCA daemon modifications.
>>
>
> I added debug printk's to bcmgenet_isr0 to check on UMAC_IRQ_LINK_UP
> and UMAC_IRQ_LINK_DOWN.
> I am not getting those interrupts on eth1 (MoCA) port when coax is
> removed/inserted.
> But, they do work on eth0.
>
> I'll modify init_umac() to enable those interrupts for MoCA port and retest.
>
>> [snip]
>>
>>>
>>>       priv->phydev = phydev;
>>> @@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>>>       return 0;
>>>  }
>>>
>>> +static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>> +{
>>> +     struct device *kdev = &priv->pdev->dev;
>>> +     struct bcmgenet_platform_data *pd = kdev->platform_data;
>>> +     struct mii_bus *mdio = priv->mii_bus;
>>> +     int phy_addr = pd->phy_addr;
>>> +     struct phy_device *phydev;
>>> +     int ret;
>>> +     int i;
>>> +
>>> +     /* Disable automatic MDIO bus scan */
>>> +     mdio->phy_mask = ~0;
>>> +
>>> +     /* Clear all the IRQ properties */
>>> +     if (mdio->irq)
>>> +             for (i = 0; i < PHY_MAX_ADDR; i++)
>>> +                     mdio->irq[i] = PHY_POLL;
>>> +
>>> +     /* Register the MDIO bus */
>>> +     ret = mdiobus_register(mdio);
>>> +     if (ret) {
>>> +             dev_err(kdev, "failed to register MDIO bus\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     /*
>>> +      * bcmgenet_platform_data needs to pass a valid PHY address for
>>> +      * internal/external PHY or -1 for MoCA PHY.
>>> +      */
>>> +     if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {
>>
>> We do too much low-level PHY device handling, and since you already have
>> the phy_type provided via platform_data, we can use that hint to do the
>> following:
>>
>> 1) an internal or external PHY with MDIO accesses should leave the bus
>> auto-probing on with the specified PHY address in the mdio bus phy_mask
>>
>> 2) a MoCA PHY or an external PHY with MDIO accesses disabled should use
>> the fixed-0 MII bus instead.
>>
>> This would look like this:
>>
>> if (pd->phy_type != PHY_INTERFACE_MODE_MOCA || pd->mdio_enabled)
>>         mdio->phy_mask = ~(1 << pd->phy_addr);
>>
>>         ...
>>         mdiobus_register()
>>
>>         priv->phydev = mdio->bus->phy_map[pd->phy_addr];
>>
>>         phydev->phy_flags |= mask;
>>
>> if (pd->phy_type == PHY_INTERFACE_MODE_MOCA || !pd->mdio_enabled)
>>         priv->phydev = fixed_phy_register(...);
>>
>> and in both cases, later on you do connect to the PHY device
>>
>> I can cook a patch to illustrate what I think this could look like since
>> I realize using pseudo-code to explain might not be the best thing.
>>
>
> I think I understand what you mean. I'll make a change.
>
>>> +             /*
>>> +              * 10/100/1000 Ethernet port with external or internal PHY.
>>> +              */
>>> +             phydev = get_phy_device(mdio, phy_addr, false);
>>> +             if (!phydev || IS_ERR(phydev)) {
>>> +                     dev_err(kdev, "failed to create PHY device\n");
>>> +                     mdiobus_unregister(mdio);
>>> +                     return 1;
>>> +             }
>>> +
>>> +             phydev->irq = PHY_POLL;
>>> +
>>> +             ret = phy_device_register(phydev);
>>> +             if (ret) {
>>> +                     dev_err(kdev, "failed to register PHY device\n");
>>> +                     phy_device_free(phydev);
>>> +                     mdiobus_unregister(mdio);
>>> +                     return 1;
>>> +             }
>>> +
>>> +             priv->phydev = phydev;
>>> +             priv->phy_interface = pd->phy_type;
>>> +     } else {
>>> +             /*
>>> +              * MoCA port with no MDIO-accessible PHY.
>>> +              * We need to use 1000/HD fixed PHY to represent the link layer.
>>> +              * MoCA daemon interacts with this PHY via ethtool.
>>> +              */
>>> +             struct fixed_phy_status moca_fphy_status = {
>>> +                     .link = 0,
>>> +                     .duplex = 0,
>>
>> This should be DUPLEX_FULL here, the link between GENET and the MoCA
>> Ethernet convergence layer is full-duplex by nature (despite we report
>> the PHY being half-duplex, which is a mistake in the downstream driver),
>> the MoCA medium on the coaxial cable is half-duplex though, but that is
>> handled by the MoCA HW.
>>
>> NB: I had issues in the past using a half-duplex link with the MoCA
>> ethernet convergence layer, causing various types of packet loss because
>> we use a simplified signaling internally in the hardware.
>>
>
> I picked this setting from 3.3 GENET driver. I'll test 1000/FULL on my
> platform to see if it works.
>
>>> +                     .speed = 1000,
>>> +                     .pause = 0,
>>> +                     .asym_pause = 0,
>>> +             };
>>> +
>>> +             phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
>>> +             if (!phydev || IS_ERR(phydev)) {
>>> +                     dev_err(kdev, "failed to register fixed PHY device\n");
>>> +                     mdiobus_unregister(mdio);
>>> +                     return 1;
>>> +             }
>>> +
>>> +             phydev->autoneg = AUTONEG_DISABLE;
>>> +
>>> +             ret = fixed_phy_set_link_update(phydev,
>>> +                                             bcmgenet_moca_fphy_update);
>>> +             if (ret) {
>>> +                     dev_err(kdev, "failed to set fixed PHY link update\n");
>>> +             }
>>
>> Should not we propagate this error to the caller?
>
> Good catch. Yes.
>
>> --
>> Florian

Re: [PATCH net-next] net: bcmgenet: enable driver to work without a device tree

[Florian Fainelli]

On 01/12/14 13:13, Petri Gynther wrote:
> Hi Florian,
> 
> Getting back to this (finally). I have made changes per your comments.
> Sending a new patch shortly.

Ok, I am still a little bit hesitant in accepting these changes
considering that Kevin spent some time making a BMIPS multiplatform
kernel to work on 7425, 7435 [1]. Let's see how your changes look like,
and we can decide by then.

https://lwn.net/Articles/622947/

> 
> -- Petri
> 
> On Fri, Oct 10, 2014 at 12:46 PM, Petri Gynther <pgynther@google.com> wrote:
>> Hi Florian,
>>
>> On Fri, Oct 10, 2014 at 11:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 10/10/2014 11:35 AM, Petri Gynther wrote:
>>>> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
>>>> Modify bcmgenet driver so that it can be used on those platforms.
>>>>
>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>> ---
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> index dbf524e..5191e3f 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> @@ -17,6 +17,17 @@
>>>>  #include <linux/if_vlan.h>
>>>>  #include <linux/phy.h>
>>>>
>>>> +struct bcmgenet_platform_data {
>>>> +     void __iomem    *base_reg;
>>>> +     int             irq0;
>>>> +     int             irq1;
>>>
>>> Why would these members here? The platform device should provide those
>>> as standard resources that the driver fetches using
>>> platform_get_resource() and platform_get_irq().
>>>
>>
>> I modeled this on struct bcmemac_platform_data that was used in the
>> legacy BRCMSTB code.
>> include/linux/brcmstb/brcmstb.h:
>>
>> struct bcmemac_platform_data {
>>         /* used by the BSP code only */
>>         uintptr_t               base_reg;
>>         int                     irq0;
>>         int                     irq1;
>>
>>         int                     phy_type;
>>         int                     phy_id;
>>         int                     phy_speed;
>>         u8                      macaddr[ETH_ALEN];
>> };
>>
>> The legacy BRCMSTB code stores all relevant GENET info in this struct
>> and then creates the resources from that info:
>>
>> static void __init brcm_register_genet(int id, struct bcmemac_platform_data *pd)
>> {
>>         struct resource res[3];
>>         struct platform_device *pdev;
>>
>>         memset(&res, 0, sizeof(res));
>>         res[0].start = BPHYSADDR(pd->base_reg);
>>         res[0].end = BPHYSADDR(pd->base_reg + 0x4fff);
>>         res[0].flags = IORESOURCE_MEM;
>>
>>         res[1].start = res[1].end = pd->irq0;
>>         res[1].flags = IORESOURCE_IRQ;
>>
>>         res[2].start = res[2].end = pd->irq1;
>>         res[2].flags = IORESOURCE_IRQ;
>>
>>         brcm_alloc_macaddr(pd->macaddr);
>>
>>         pdev = platform_device_alloc("bcmgenet", id);
>>         platform_device_add_resources(pdev, res, 3);
>>         platform_device_add_data(pdev, pd, sizeof(*pd));
>>         platform_device_add(pdev);
>> }
>>
>>>> +     int             phy_type;
>>>> +     int             phy_addr;
>>>> +     int             phy_speed;
>>>> +     u8              macaddr[ETH_ALEN];
>>>> +     int             genet_version;
>>>> +};
>>>
>>> I would rather we put this in include/linux/platform_data/bcmgenet.h
>>> where it belongs.
>>>
>>
>> I wasn't aware of the directory include/linux/platform_data/. Yes,
>> that's where this belongs.
>>
>>>> +
>>>>  /* total number of Buffer Descriptors, same for Rx/Tx */
>>>>  #define TOTAL_DESC                           256
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> index 9ff799a..e5ff913 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> @@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
>>>>       phy_print_status(phydev);
>>>>  }
>>>>
>>>> +static int bcmgenet_moca_fphy_update(struct net_device *dev,
>>>> +                                  struct fixed_phy_status *status)
>>>> +{
>>>> +     struct bcmgenet_priv *priv = netdev_priv(dev);
>>>> +     struct phy_device *phydev = priv->phydev;
>>>> +
>>>> +     /*
>>>> +      * MoCA daemon updates phydev->autoneg to reflect the link status.
>>>> +      * Update MoCA fixed PHY status accordingly, so that the PHY state
>>>> +      * machine becomes aware of the real link status.
>>>> +      */
>>>> +     status->link = phydev->autoneg;
>>>> +     return 0;
>>>> +}
>>>
>>> I don't want to see that in the upstream driver, please enable the link
>>> interrupts like I suggested before and do not use the autoneg field at
>>> all, which should require no MoCA daemon modifications.
>>>
>>
>> I added debug printk's to bcmgenet_isr0 to check on UMAC_IRQ_LINK_UP
>> and UMAC_IRQ_LINK_DOWN.
>> I am not getting those interrupts on eth1 (MoCA) port when coax is
>> removed/inserted.
>> But, they do work on eth0.
>>
>> I'll modify init_umac() to enable those interrupts for MoCA port and retest.
>>
>>> [snip]
>>>
>>>>
>>>>       priv->phydev = phydev;
>>>> @@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>>>>       return 0;
>>>>  }
>>>>
>>>> +static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>>> +{
>>>> +     struct device *kdev = &priv->pdev->dev;
>>>> +     struct bcmgenet_platform_data *pd = kdev->platform_data;
>>>> +     struct mii_bus *mdio = priv->mii_bus;
>>>> +     int phy_addr = pd->phy_addr;
>>>> +     struct phy_device *phydev;
>>>> +     int ret;
>>>> +     int i;
>>>> +
>>>> +     /* Disable automatic MDIO bus scan */
>>>> +     mdio->phy_mask = ~0;
>>>> +
>>>> +     /* Clear all the IRQ properties */
>>>> +     if (mdio->irq)
>>>> +             for (i = 0; i < PHY_MAX_ADDR; i++)
>>>> +                     mdio->irq[i] = PHY_POLL;
>>>> +
>>>> +     /* Register the MDIO bus */
>>>> +     ret = mdiobus_register(mdio);
>>>> +     if (ret) {
>>>> +             dev_err(kdev, "failed to register MDIO bus\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     /*
>>>> +      * bcmgenet_platform_data needs to pass a valid PHY address for
>>>> +      * internal/external PHY or -1 for MoCA PHY.
>>>> +      */
>>>> +     if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {
>>>
>>> We do too much low-level PHY device handling, and since you already have
>>> the phy_type provided via platform_data, we can use that hint to do the
>>> following:
>>>
>>> 1) an internal or external PHY with MDIO accesses should leave the bus
>>> auto-probing on with the specified PHY address in the mdio bus phy_mask
>>>
>>> 2) a MoCA PHY or an external PHY with MDIO accesses disabled should use
>>> the fixed-0 MII bus instead.
>>>
>>> This would look like this:
>>>
>>> if (pd->phy_type != PHY_INTERFACE_MODE_MOCA || pd->mdio_enabled)
>>>         mdio->phy_mask = ~(1 << pd->phy_addr);
>>>
>>>         ...
>>>         mdiobus_register()
>>>
>>>         priv->phydev = mdio->bus->phy_map[pd->phy_addr];
>>>
>>>         phydev->phy_flags |= mask;
>>>
>>> if (pd->phy_type == PHY_INTERFACE_MODE_MOCA || !pd->mdio_enabled)
>>>         priv->phydev = fixed_phy_register(...);
>>>
>>> and in both cases, later on you do connect to the PHY device
>>>
>>> I can cook a patch to illustrate what I think this could look like since
>>> I realize using pseudo-code to explain might not be the best thing.
>>>
>>
>> I think I understand what you mean. I'll make a change.
>>
>>>> +             /*
>>>> +              * 10/100/1000 Ethernet port with external or internal PHY.
>>>> +              */
>>>> +             phydev = get_phy_device(mdio, phy_addr, false);
>>>> +             if (!phydev || IS_ERR(phydev)) {
>>>> +                     dev_err(kdev, "failed to create PHY device\n");
>>>> +                     mdiobus_unregister(mdio);
>>>> +                     return 1;
>>>> +             }
>>>> +
>>>> +             phydev->irq = PHY_POLL;
>>>> +
>>>> +             ret = phy_device_register(phydev);
>>>> +             if (ret) {
>>>> +                     dev_err(kdev, "failed to register PHY device\n");
>>>> +                     phy_device_free(phydev);
>>>> +                     mdiobus_unregister(mdio);
>>>> +                     return 1;
>>>> +             }
>>>> +
>>>> +             priv->phydev = phydev;
>>>> +             priv->phy_interface = pd->phy_type;
>>>> +     } else {
>>>> +             /*
>>>> +              * MoCA port with no MDIO-accessible PHY.
>>>> +              * We need to use 1000/HD fixed PHY to represent the link layer.
>>>> +              * MoCA daemon interacts with this PHY via ethtool.
>>>> +              */
>>>> +             struct fixed_phy_status moca_fphy_status = {
>>>> +                     .link = 0,
>>>> +                     .duplex = 0,
>>>
>>> This should be DUPLEX_FULL here, the link between GENET and the MoCA
>>> Ethernet convergence layer is full-duplex by nature (despite we report
>>> the PHY being half-duplex, which is a mistake in the downstream driver),
>>> the MoCA medium on the coaxial cable is half-duplex though, but that is
>>> handled by the MoCA HW.
>>>
>>> NB: I had issues in the past using a half-duplex link with the MoCA
>>> ethernet convergence layer, causing various types of packet loss because
>>> we use a simplified signaling internally in the hardware.
>>>
>>
>> I picked this setting from 3.3 GENET driver. I'll test 1000/FULL on my
>> platform to see if it works.
>>
>>>> +                     .speed = 1000,
>>>> +                     .pause = 0,
>>>> +                     .asym_pause = 0,
>>>> +             };
>>>> +
>>>> +             phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
>>>> +             if (!phydev || IS_ERR(phydev)) {
>>>> +                     dev_err(kdev, "failed to register fixed PHY device\n");
>>>> +                     mdiobus_unregister(mdio);
>>>> +                     return 1;
>>>> +             }
>>>> +
>>>> +             phydev->autoneg = AUTONEG_DISABLE;
>>>> +
>>>> +             ret = fixed_phy_set_link_update(phydev,
>>>> +                                             bcmgenet_moca_fphy_update);
>>>> +             if (ret) {
>>>> +                     dev_err(kdev, "failed to set fixed PHY link update\n");
>>>> +             }
>>>
>>> Should not we propagate this error to the caller?
>>
>> Good catch. Yes.
>>
>>> --
>>> Florian