[PATCHv3,1/5] net: dsa: Support internal phy on 'cpu' port

This adds support for enabling the internal PHY for a 'cpu' port.
It has been tested on GE B850v3,  B650v3 and B450v3, which have a
built-in MV88E6240 switch connected to a PCIe based network card.
Without this patch the link does not come up and no traffic can be
routed through the switch.

The PHY interface, that is being used on the above test systems is
part of the MV88E6240 and since mv88e6xxx driver resets the chip
during probe, it is definitely disabled without this patch.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 net/dsa/port.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

On 01/15/2018 11:37 AM, Sebastian Reichel wrote:
> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3,  B650v3 and B450v3, which have a
> built-in MV88E6240 switch connected to a PCIe based network card.
> Without this patch the link does not come up and no traffic can be
> routed through the switch.
> 
> The PHY interface, that is being used on the above test systems is
> part of the MV88E6240 and since mv88e6xxx driver resets the chip
> during probe, it is definitely disabled without this patch.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  net/dsa/port.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index bb4be2679904..011ecd9b1be7 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -273,6 +273,55 @@ int dsa_port_vlan_del(struct dsa_port *dp,
>  	return 0;
>  }
>  
> +static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
> +{
> +	struct device_node *port_dn = dp->dn;
> +	struct device_node *phy_dn;
> +	struct dsa_switch *ds = dp->ds;
> +	struct phy_device *phydev;
> +	int port = dp->index;
> +	int err = 0;

I don't have a better solution to offer yet, but this is really making
the number of special cases within DSA much worse, thus impending the
on-going conversion to PHYLINK...

Having PHYs with no network devices is clearly a hack so maybe we should
re-think this whole design paradigm in DSA for which we don't have
net_device being created for CPU and DSA links, and maybe come up with
an actual net_device and just not expose it to the default namespace.

Anyway...

> +
> +	phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
> +	if (!phy_dn)
> +		return 0;
> +
> +	phydev = of_phy_find_device(phy_dn);
> +	if (!phydev) {
> +		err = -EPROBE_DEFER;
> +		goto err_put_of;
> +	}
> +
> +	if (enable) {
> +		err = genphy_config_init(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +
> +		err = genphy_resume(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +
> +		err = genphy_read_status(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +	} else {
> +		err = genphy_suspend(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +	}
> +
> +	if (ds->ops->adjust_link)
> +		ds->ops->adjust_link(ds, port, phydev);
> +
> +	dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev));
> +
> +err_put_dev:
> +	put_device(&phydev->mdio.dev);
> +err_put_of:
> +	of_node_put(phy_dn);
> +	return err;
> +}
> +
>  int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  {
>  	struct device_node *dn = dp->dn;
> @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  			ds->ops->adjust_link(ds, port, phydev);
>  
>  		put_device(&phydev->mdio.dev);
> +	} else {
> +		err = dsa_port_setup_phy_of(dp, true);
> +		if (err)
> +			return err;
>  	}
>  
>  	return 0;
> @@ -316,4 +369,6 @@ void dsa_port_fixed_link_unregister_of(struct dsa_port *dp)
>  
>  	if (of_phy_is_fixed_link(dn))
>  		of_phy_deregister_fixed_link(dn);
> +	else
> +		dsa_port_setup_phy_of(dp, false);
>  }
>

>  int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  {
>  	struct device_node *dn = dp->dn;
> @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  			ds->ops->adjust_link(ds, port, phydev);
>  
>  		put_device(&phydev->mdio.dev);
> +	} else {
> +		err = dsa_port_setup_phy_of(dp, true);
> +		if (err)
> +			return err;

Hi Sebastian

First off, i tend to agree with Florian. I'm not sure how reliable
this is. There is normally a state machine moving the PHY between
different states. But in order to do that, i think you need a netdev.
Have you tried multiple down/up of the other MAC/PHY? Does it always
work?

But, at the moment, we don't have much better.

What i don't like is having this code inside
dsa_port_fixed_link_register_of(). This has nothing to do with a fixed
link. Please export functions from port.c and call them directly from
dsa_port_setup() and dsa_port_teardown().

	 Andrew

Hi,

On Mon, Jan 15, 2018 at 11:57:18PM +0100, Andrew Lunn wrote:
> >  int dsa_port_fixed_link_register_of(struct dsa_port *dp)
> >  {
> >  	struct device_node *dn = dp->dn;
> > @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp)
> >  			ds->ops->adjust_link(ds, port, phydev);
> >  
> >  		put_device(&phydev->mdio.dev);
> > +	} else {
> > +		err = dsa_port_setup_phy_of(dp, true);
> > +		if (err)
> > +			return err;
> 
> Hi Sebastian
> 
> First off, i tend to agree with Florian. I'm not sure how reliable
> this is. There is normally a state machine moving the PHY between
> different states. But in order to do that, i think you need a netdev.
> Have you tried multiple down/up of the other MAC/PHY? Does it always
> work?

I tested multiple down/up transitions and everything works as
expected.

> But, at the moment, we don't have much better.
> 
> What i don't like is having this code inside
> dsa_port_fixed_link_register_of(). This has nothing to do with a fixed
> link. Please export functions from port.c and call them directly from
> dsa_port_setup() and dsa_port_teardown().

I just sent PATCHv4 implementing this change.

-- Sebastian