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

[Florian Fainelli <f.fainelli@gmail.com>]

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