Message-ID: <14f73250-4255-6f4e-336a-9bf289539b75@gmail.com>
Date:   Wed, 6 Feb 2019 14:29:18 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Christian Lamparter <chunkeey@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy
 translation

On 2/6/19 1:57 PM, Christian Lamparter wrote:
> On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
>> On 2/5/19 2:12 PM, Christian Lamparter wrote:
>>> On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
>>>>> For now, I added the DT binding update to the patch as well.
>>>>> But if this is indeed the way to go, it'll get a separate patch.
>>>>
>>>> Hi Christian 
>>>>
>>>> You need to be careful with the DT binding. You need to keep backwards
>>>> compatible with it. An old DT blob needs to keep working. I don't
>>>> think this is true with this change.
>>>
>>> Do you mean because of the 
>>>
>>> -               switch0@0 {
>>> +               switch@10 {
>>>                         compatible = "qca,qca8337";
>>>                         #address-cells = <1>;
>>>                         #size-cells = <0>;
>>>  
>>> -                       reg = <0>;
>>> +                       reg = <0x10>;
>>>
>>> change?
>>>
>>> or because I removed the phy-handles?>
>>> The reg = <0x10>; will be necessary regardless. Because this
>>> is really a bug in the existing binding example and if it is
>>> copied it will prevent the qca8k driver from loading. 
>>> This is due to a resource conflict, because there will be 
>>> already a "phy_port1: phy@0" registered at reg = <0>;
>>> So this never worked would have worked.
>>
>> That part is fine, it is the removal of the phy-handle properties that
>> is possibly a problem, but in hindsight, I do not believe it will be a
>> compatibility issue. Lack of "phy-handle" property within the core DSA
>> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
>> instance, which you are not removing, you are just changing how the PHYs
>> map to port numbers.
>>
> Ok, thanks. 
> 
> I think I'm almost ready for v2. I have fully addressed the compatibility
> issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
> property on one of the ports was found or not. If there was no phy-handle the
> driver adds the slave-bus accessors to the ops which tells DSA to allocate
> the slave bus and allows the phys can be enumerated. If the phy-handles are
> found the driver will not have the accessors and DSA will not setup a
> redundant/fake bus and this prevents the second/double/duplicated discovery
> and enumeration of the same PHYs again.

The logic you have sounds a little too broad since it stops as soon as
one port is found with a 'phy-handle' property and assumes that the
parent MDIO bus from which qca8k itself is a child device, is the MDIO
bus to be used. There are possibly 3 cases:

1) All ports using internal/build-in PHYs. In that case, you can either
not specify a 'phy-handle' property and DSA assumes that they are part
of the switch's internal MDIO bus. You can also specify a 'phy-handle'
property that references the internal MDIO bus, although then we also
expect qca8k to register its internal MDIO bus (ala mv88e6xxx)

2) Some ports using internal PHYs, some using external PHYs. Similar
situation again, ports may, or may not specify a 'phy-handle' property,
so without a 'phy-handle' property that means the port connects to an
internal PHY, with a 'phy-handle' it could connect to either internal
PHY or external PHY

3) All ports using external PHYs, in that case, we must have a
'phy-handle' for each port to specify where and how they connect to
their external PHYs.

With respect to your patch, what I would do is register QCA8k's internal
MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
that bus, such that tell the DSA layer: look, here is the internal MDIO
bus, would you ever find a port that needs to use a PHY in there.

Then you can still scan each enabled port device, and for each of them,
populate ds->phys_mii_mask, thus telling DSA exacly which ports are
using an internal PHY because that would be the ports that do not have a
'phy-handle' property. Ports that have a 'phy-handle' property.

Hope this helps and is clear, if not, I can try to cook a patch for you
to try, though I don't have you hardware.

Tangential, since you are working on qca8k, it would be great to give
this driver some TLC and make sure that:

- bridge w/ and w/o VLAN filtering enabled works
- multicast snooping works etc.

Cheers

> 
> Cheers,
> Christian
> 
> Attached is a preview:
> 
> ---
> commit 96bc70b4d6e290c450b9af728d9ca0f6db877f13
> Author: Christian Lamparter <chunkeey@...il.com>
> Date:   Fri Feb 1 22:54:32 2019 +0100
> 
>     net: dsa: qca8k: extend slave-bus implementations
>     
>     This patch implements accessors for the QCA8337 MDIO access
>     through the MDIO_MASTER register, which makes it possible to
>     access the PHYs on slave-bus through the switch. In cases
>     where the switch ports are already mapped via external
>     "phy-phandles", the internal mdio-bus is disabled in order to
>     prevent a duplicated discovery and enumeration of the same
>     PHYs.
>     
>     Signed-off-by: Christian Lamparter <chunkeey@...il.com>
>     ---
>     
>     Changes from v2:
>      - Make it compatible with existing configurations
>     
>     Changes from v1:
>      - drop DT port <-> phy mapping
>      - added register definitions for the MDIO control register
>      - implemented new slave-mdio bus accessors
>      - DT-binding: fix switch's PSEUDO_PHY address. It's 0x10 not 0.
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a4b6cda38016..2f1b4b0a3507 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -613,19 +613,61 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
>  }
>  
>  static int
> -qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
> +qca8k_port_to_phy(int port)
> +{
> +	if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
> +		return -EINVAL;
> +
> +	return port - 1;
> +}
> +
> +static int
> +qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	u32 val, phy;
> +
> +	phy = qca8k_port_to_phy(port);
> +	if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
> +		return -EINVAL;
> +
> +	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> +	      QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> +	      QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
> +	      QCA8K_MDIO_MASTER_DATA(data);
>  
> -	return mdiobus_read(priv->bus, phy, regnum);
> +	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
> +
> +	return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> +		QCA8K_MDIO_MASTER_BUSY);
>  }
>  
> +
>  static int
> -qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
> +qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	u32 val, phy;
> +
> +	phy = qca8k_port_to_phy(port);
> +	if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
> +		return 0xffff;
> +
> +	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> +	      QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> +	      QCA8K_MDIO_MASTER_REG_ADDR(regnum);
> +
> +	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
>  
> -	return mdiobus_write(priv->bus, phy, regnum, val);
> +	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> +				  QCA8K_MDIO_MASTER_BUSY)) {
> +		return 0xffff;
> +	}
> +
> +	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
> +		QCA8K_MDIO_MASTER_DATA_MASK);
> +
> +	return val;
>  }
>  
>  static void
> @@ -868,8 +910,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.setup			= qca8k_setup,
>  	.adjust_link            = qca8k_adjust_link,
>  	.get_strings		= qca8k_get_strings,
> -	.phy_read		= qca8k_phy_read,
> -	.phy_write		= qca8k_phy_write,
>  	.get_ethtool_stats	= qca8k_get_ethtool_stats,
>  	.get_sset_count		= qca8k_get_sset_count,
>  	.get_mac_eee		= qca8k_get_mac_eee,
> @@ -884,6 +924,38 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_fdb_dump		= qca8k_port_fdb_dump,
>  };
>  
> +/* Special code to detect DeviceTrees that use the phy-handle
> + * to map external PHYs to the ports. Only if no phy-handle is
> + * detected the slave bus accessors are getting enabled.
> + */
> +static int qca8k_detect_slave_bus(struct qca8k_priv *priv)
> +{
> +	struct device_node *ports, *port;
> +	bool found = false;
> +
> +	ports = of_get_child_by_name(priv->dev->of_node, "ports");
> +	if (!ports) {
> +		dev_err(priv->dev, "no ports child node found.\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_available_child_of_node(ports, port) {
> +		if (of_property_read_bool(port, "phy-handle")) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		dev_info(priv->dev, "uses external mdio-bus.\n");
> +	} else {
> +		priv->ops.phy_read = qca8k_phy_read;
> +		priv->ops.phy_write = qca8k_phy_write;
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  qca8k_sw_probe(struct mdio_device *mdiodev)
>  {
> @@ -912,7 +984,12 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  		return -ENOMEM;
>  
>  	priv->ds->priv = priv;
> -	priv->ds->ops = &qca8k_switch_ops;
> +	priv->ops = qca8k_switch_ops;
> +	if (qca8k_detect_slave_bus(priv))
> +		return -EINVAL;
> +
> +	priv->ds->ops = &priv->ops;
> +
>  	mutex_init(&priv->reg_mutex);
>  	dev_set_drvdata(&mdiodev->dev, priv);
>  
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 613fe5c50236..38d06661f0a8 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -48,6 +48,18 @@
>  #define   QCA8K_MIB_FLUSH				BIT(24)
>  #define   QCA8K_MIB_CPU_KEEP				BIT(20)
>  #define   QCA8K_MIB_BUSY				BIT(17)
> +#define QCA8K_MDIO_MASTER_CTRL				0x3c
> +#define   QCA8K_MDIO_MASTER_BUSY			BIT(31)
> +#define   QCA8K_MDIO_MASTER_EN				BIT(30)
> +#define   QCA8K_MDIO_MASTER_READ			BIT(27)
> +#define   QCA8K_MDIO_MASTER_WRITE			0
> +#define   QCA8K_MDIO_MASTER_SUP_PRE			BIT(26)
> +#define   QCA8K_MDIO_MASTER_PHY_ADDR(x)			((x) << 21)
> +#define   QCA8K_MDIO_MASTER_REG_ADDR(x)			((x) << 16)
> +#define   QCA8K_MDIO_MASTER_DATA(x)			(x)
> +#define   QCA8K_MDIO_MASTER_DATA_MASK			GENMASK(15, 0)
> +#define   QCA8K_MDIO_MASTER_MAX_PORTS			5
> +#define   QCA8K_MDIO_MASTER_MAX_REG			32
>  #define QCA8K_GOL_MAC_ADDR0				0x60
>  #define QCA8K_GOL_MAC_ADDR1				0x64
>  #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
> @@ -168,6 +180,7 @@ struct qca8k_priv {
>  	struct dsa_switch *ds;
>  	struct mutex reg_mutex;
>  	struct device *dev;
> +	struct dsa_switch_ops ops;
>  };
>  
>  struct qca8k_mib_desc {
> 
> 
> 
> 
> 


-- 
Florian

Powered by blists - more mailing lists