[PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

This series is based on the tip of the net tree.

The first patch fixes a bug that makes dual_emac mode break if
either slave uses the phy-handle property in the devicetree.

The second patch fixes some cosmetic problems with error messages,
and also makes the binding documentation more explicit.

The third patch cleans up the fixed-link case to work like
the now-fixed phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode
Note that I don't have a board which would uses a phy-handle property,
though I have used hacked devicetrees to exercise the code paths.
Testing by anyone who has real hardware using phy-handle or dual_emac
with fixed-link would be appreciated.

David Rivshin (3):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
    config
  drivers: net: cpsw: fix error messages when using phy-handle DT
    property
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
 drivers/net/ethernet/ti/cpsw.c                 | 40 +++++++++++++-------------
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 23 insertions(+), 22 deletions(-)

-- 
2.5.0

[PATCH 1/3] drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac config

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

Commit 9e42f715264ff158478fa30eaed847f6e131366b ("drivers: net: cpsw: add
phy-handle parsing") saved the "phy-handle" phandle into a new cpsw_priv
field. However, phy connections are per-slave, so the phy_node field should
be in cpsw_slave_data rather than cpsw_priv.

This would go unnoticed in a single emac configuration. But in dual_emac
mode, the last "phy-handle" property parsed for either slave would be used
by both of them, causing them both to refer to the same phy_device.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin <drivshin@allworx.com>
---
You may want to consider this for 4.3-stable. It manages to apply
on top of v4.3.3 with 'git am -C1', or I can produce a separate
patch against v4.3.3 if preferred.

 drivers/net/ethernet/ti/cpsw.c | 13 ++++++-------
 drivers/net/ethernet/ti/cpsw.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3b489ca..8ad0ed8 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, u32 val, u32 offset)
 	__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
 	spinlock_t			lock;
 	struct platform_device		*pdev;
 	struct net_device		*ndev;
-	struct device_node		*phy_node;
 	struct napi_struct		napi_rx;
 	struct napi_struct		napi_tx;
 	struct device			*dev;
 	struct cpsw_platform_data	data;
 	struct cpsw_ss_regs __iomem	*regs;
 	struct cpsw_wr_regs __iomem	*wr_regs;
 	u8 __iomem			*hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 
 	if (priv->data.dual_emac)
 		cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
 	else
 		cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
 				   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-	if (priv->phy_node)
-		slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+	if (slave->data->phy_node)
+		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 				 &cpsw_adjust_link, 0, slave->data->phy_if);
 	else
 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 				 &cpsw_adjust_link, slave->data->phy_if);
 	if (IS_ERR(slave->phy)) {
 		dev_err(priv->dev, "phy %s not found on slave %d\n",
 			slave->data->phy_id, slave->slave_num);
@@ -1936,20 +1935,19 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
 
 	slave->data	= data;
 	slave->regs	= regs + slave_reg_ofs;
 	slave->sliver	= regs + sliver_reg_ofs;
 	slave->port_vlan = data->dual_emac_res_vlan;
 }
 
-static int cpsw_probe_dt(struct cpsw_priv *priv,
+static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			 struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *slave_node;
-	struct cpsw_platform_data *data = &priv->data;
 	int i = 0, ret;
 	u32 prop;
 
 	if (!node)
 		return -EINVAL;
 
 	if (of_property_read_u32(node, "slaves", &prop)) {
@@ -2029,15 +2027,16 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
 		int lenp;
 		const __be32 *parp;
 
 		/* This is no slave child node, continue */
 		if (strcmp(slave_node->name, "slave"))
 			continue;
 
-		priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
+		slave_data->phy_node = of_parse_phandle(slave_node,
+							"phy-handle", 0);
 		parp = of_get_property(slave_node, "phy_id", &lenp);
 		if (of_phy_is_fixed_link(slave_node)) {
 			struct device_node *phy_node;
 			struct phy_device *phy_dev;
 
 			/* In the case of a fixed PHY, the DT node associated
 			 * to the PHY is the Ethernet MAC DT node.
@@ -2270,15 +2269,15 @@ static int cpsw_probe(struct platform_device *pdev)
 	 * This may be required here for child devices.
 	 */
 	pm_runtime_enable(&pdev->dev);
 
 	/* Select default pin state */
 	pinctrl_pm_select_default_state(&pdev->dev);
 
-	if (cpsw_probe_dt(priv, pdev)) {
+	if (cpsw_probe_dt(&priv->data, pdev)) {
 		dev_err(&pdev->dev, "cpsw: platform data missing\n");
 		ret = -ENODEV;
 		goto clean_runtime_disable_ret;
 	}
 	data = &priv->data;
 
 	if (is_valid_ether_addr(data->slave_data[0].mac_addr)) {
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 442a703..e50afd1 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -14,14 +14,15 @@
 #ifndef __CPSW_H__
 #define __CPSW_H__
 
 #include <linux/if_ether.h>
 #include <linux/phy.h>
 
 struct cpsw_slave_data {
+	struct device_node *phy_node;
 	char		phy_id[MII_BUS_ID_SIZE];
 	int		phy_if;
 	u8		mac_addr[ETH_ALEN];
 	u16		dual_emac_res_vlan;	/* Reserved VLAN for DualEMAC */
 };
 
 struct cpsw_platform_data {
-- 
2.5.0

[PATCH 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. However if phy-handle was specified, an
error message would complain about the lack of phy_id or fixed-link.

Also, if phy-handle was specified and the subsequent of_phy_connect()
failed, the error message still referenced slaved->data->phy_id, which
would be empty. Instead, use the name of the device_node as a useful
identifier.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin <drivshin@allworx.com>
---
This would require some adjustments to backport to 4.3-stable due to
other changes in this area. Let me know if you want a version of this
against v4.3.3.

 Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
 drivers/net/ethernet/ti/cpsw.c                 | 17 +++++++++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..3033c0f 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -46,16 +46,16 @@ Optional properties:
 - dual_emac_res_vlan	: Specifies VID to be used to segregate the ports
 - mac-address		: See ethernet.txt file in the same directory
 - phy_id		: Specifies slave phy id
 - phy-handle		: See ethernet.txt file in the same directory
 
 Slave sub-nodes:
 - fixed-link		: See fixed-link.txt file in the same directory
-			  Either the property phy_id, or the sub-node
-			  fixed-link can be specified
+
+Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
 Future plan is to migrate hwmod data base contents into device tree
 blob so that, all the required data will be used from device tree dts
 file.
 
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 8ad0ed8..f9029e7 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 	if (slave->data->phy_node)
 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 				 &cpsw_adjust_link, 0, slave->data->phy_if);
 	else
 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 				 &cpsw_adjust_link, slave->data->phy_if);
 	if (IS_ERR(slave->phy)) {
-		dev_err(priv->dev, "phy %s not found on slave %d\n",
-			slave->data->phy_id, slave->slave_num);
+		dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+			slave->data->phy_node ?
+				slave->data->phy_node->full_name :
+				slave->data->phy_id,
+			slave->slave_num);
 		slave->phy = NULL;
 	} else {
 		dev_info(priv->dev, "phy found : id is : 0x%x\n",
 			 slave->phy->phy_id);
 		phy_start(slave->phy);
 
 		/* Configure GMII_SEL register */
@@ -2030,15 +2033,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 		/* This is no slave child node, continue */
 		if (strcmp(slave_node->name, "slave"))
 			continue;
 
 		slave_data->phy_node = of_parse_phandle(slave_node,
 							"phy-handle", 0);
 		parp = of_get_property(slave_node, "phy_id", &lenp);
-		if (of_phy_is_fixed_link(slave_node)) {
+		if (slave_data->phy_node) {
+			dev_dbg(&pdev->dev,
+				"slave[%d] using phy-handle=\"%s\"\n",
+				i, slave_data->phy_node->full_name);
+		} else if (of_phy_is_fixed_link(slave_node)) {
 			struct device_node *phy_node;
 			struct phy_device *phy_dev;
 
 			/* In the case of a fixed PHY, the DT node associated
 			 * to the PHY is the Ethernet MAC DT node.
 			 */
 			ret = of_phy_register_fixed_link(slave_node);
@@ -2066,15 +2073,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			if (!mdio) {
 				dev_err(&pdev->dev, "Missing mdio platform device\n");
 				return -EINVAL;
 			}
 			snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
 				 PHY_ID_FMT, mdio->name, phyid);
 		} else {
-			dev_err(&pdev->dev, "No slave[%d] phy_id or fixed-link property\n", i);
+			dev_err(&pdev->dev,
+				"No slave[%d] phy_id, phy-handle, or fixed-link property\n",
+				i);
 			goto no_phy_slave;
 		}
 		slave_data->phy_if = of_get_phy_mode(slave_node);
 		if (slave_data->phy_if < 0) {
 			dev_err(&pdev->dev, "Missing or malformed slave[%d] phy-mode property\n",
 				i);
 			return slave_data->phy_if;
-- 
2.5.0

[PATCH 3/3] drivers: net: cpsw: use of_phy_connect() in fixed-link case

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

If a fixed-link DT subnode is used, the phy_device was looked up so
that a PHY ID string could be constructed and passed to phy_connect().
This is not necessary, as the device_node can be passed directly to
of_phy_connect() instead. This reuses the same codepath as if the
phy-handle DT property was used.

Signed-off-by: David Rivshin <drivshin@allworx.com>
---
 drivers/net/ethernet/ti/cpsw.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index f9029e7..94b818c 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,29 +2038,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 							"phy-handle", 0);
 		parp = of_get_property(slave_node, "phy_id", &lenp);
 		if (slave_data->phy_node) {
 			dev_dbg(&pdev->dev,
 				"slave[%d] using phy-handle=\"%s\"\n",
 				i, slave_data->phy_node->full_name);
 		} else if (of_phy_is_fixed_link(slave_node)) {
-			struct device_node *phy_node;
-			struct phy_device *phy_dev;
-
 			/* In the case of a fixed PHY, the DT node associated
 			 * to the PHY is the Ethernet MAC DT node.
 			 */
 			ret = of_phy_register_fixed_link(slave_node);
 			if (ret)
 				return ret;
-			phy_node = of_node_get(slave_node);
-			phy_dev = of_phy_find_device(phy_node);
-			if (!phy_dev)
-				return -ENODEV;
-			snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-				 PHY_ID_FMT, phy_dev->bus->id, phy_dev->addr);
+			slave_data->phy_node = of_node_get(slave_node);
 		} else if (parp) {
 			u32 phyid;
 			struct device_node *mdio_node;
 			struct platform_device *mdio;
 
 			if (lenp != (sizeof(__be32) * 2)) {
 				dev_err(&pdev->dev, "Invalid slave[%d] phy_id property\n", i);
-- 
2.5.0

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Miller]

From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
Date: Tue, 22 Dec 2015 19:36:31 -0500

> Testing by anyone who has real hardware using phy-handle or dual_emac
> with fixed-link would be appreciated.

I'm going to wait for such testing before applying this series.

Thanks.

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[Markus Brunner]

On Wednesday 23 December 2015 12:04:25 David Miller wrote:
> From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
> Date: Tue, 22 Dec 2015 19:36:31 -0500
> 
> > Testing by anyone who has real hardware using phy-handle or dual_emac
> > with fixed-link would be appreciated.
> 
> I'm going to wait for such testing before applying this series.
> 
> Thanks.

Successfully tested the following 3 configurations.
1. emac0 with phy_id and emac1 with fixed phy
2. emac0 with phy-handle and emac1 with fixed phy
3. emac0 with fixed phy and emac1 with fixed phy


Below are the affected DT nodes, with one cpsw_emac0 node choosen for each 
test.

&cpsw_emac0 {
	phy_id = <&davinci_mdio>, <5>;
	phy-mode = "mii";
	dual_emac_res_vlan = <4093>;
};

&cpsw_emac0 {
	phy-mode = "mii";
	dual_emac_res_vlan = <4093>;
	phy-handle = <&phy0>;
};

&cpsw_emac0 {
	phy-mode = "mii";
	dual_emac_res_vlan = <4093>;
	fixed-link {
		speed = <100>;
		full-duplex;
	};
};

&cpsw_emac1 {
	dual_emac_res_vlan = <4094>;
	phy-mode = "mii";
	fixed-link {
		speed = <100>;
		full-duplex;
	};
};

&davinci_mdio {
	status = "okay";
	phy0: ethernet-phy@0 {
		reg = <5>;
	};
};

ps: I don't have access to the board for the next 2 weeks.

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

On Wed, 23 Dec 2015 19:35:37 +0100
Markus Brunner <systemprogrammierung.brunner@gmail.com> wrote:

> On Wednesday 23 December 2015 12:04:25 David Miller wrote:
> > From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
> > Date: Tue, 22 Dec 2015 19:36:31 -0500
> > 
> > > Testing by anyone who has real hardware using phy-handle or
> > > dual_emac with fixed-link would be appreciated.
> > 
> > I'm going to wait for such testing before applying this series.
> > 
> > Thanks.
> 
> Successfully tested the following 3 configurations.
> 1. emac0 with phy_id and emac1 with fixed phy
> 2. emac0 with phy-handle and emac1 with fixed phy
> 3. emac0 with fixed phy and emac1 with fixed phy

Great, thanks for testing. Using the same technique
for the phy-handle case as you, I also just tested:
 - (EVMSK) dual emac, phy-handle property in both slaves

I think that covers all the interesting cases.

Dave,
 I actually just received a note off-list reporting a problem  
with this series on the dm8148-t410 board. So please hold off  
applying this series for now. If it turns out to be a real  
problem I'll have a v2. 

[...]

> &davinci_mdio {
> 	status = "okay";
> 	phy0: ethernet-phy@0 {
> 		reg = <5>;
> 	};
> };

I was unaware that the davinci-mdio driver creates PHY devices
from child nodes. The davinci-mdio.txt binding documentation 
makes no mention of that. By comparison the emac_rockchip.txt 
file does talk about it.

Now that I take a closer look at the code, it looks like that 
capability was added in commit 0a0ea0687281 ("net: davinci_mdio: 
allow to create phys from dt"), but it didn't update the binding.

Grygorii, was that just an oversight, or capability that's not
supposed to be used? 

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

On Wed, 23 Dec 2015 22:20:58 +0100
Nicolas Chauvet <kwizart@gmail.com> wrote:

> 2015-12-23 1:36 GMT+01:00 David Rivshin (Allworx) <
> drivshin.allworx@gmail.com>:
> 
> > From: David Rivshin <drivshin@allworx.com>
> >
> > This series is based on the tip of the net tree.
> >
> > The first patch fixes a bug that makes dual_emac mode break if
> > either slave uses the phy-handle property in the devicetree.
> >
> > The second patch fixes some cosmetic problems with error messages,
> > and also makes the binding documentation more explicit.
> >
> > The third patch cleans up the fixed-link case to work like
> > the now-fixed phy-handle case.
> >
> > I have tested on the following hardware configurations:
> >  - (EVMSK) dual emac, phy_id property in both slaves
> >  - (BeagleBoneBlack) single emac, phy_id property
> >  - (custom) single emac, fixed-link subnode
> > Note that I don't have a board which would uses a phy-handle
> > property, though I have used hacked devicetrees to exercise the
> > code paths. Testing by anyone who has real hardware using
> > phy-handle or dual_emac with fixed-link would be appreciated.
> >
> > David Rivshin (3):
> >   drivers: net: cpsw: fix parsing of phy-handle DT property in
> > dual_emac config
> >   drivers: net: cpsw: fix error messages when using phy-handle DT
> >     property
> >   drivers: net: cpsw: use of_phy_connect() in fixed-link case
> >
> >  Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
> >  drivers/net/ethernet/ti/cpsw.c                 | 40
> > +++++++++++++-------------
> >  drivers/net/ethernet/ti/cpsw.h                 |  1 +
> >  3 files changed, 23 insertions(+), 22 deletions(-)
> >
> >
> This serie failed with me on dm8418 hp-t410 on top of linux-next from
> today whereas the same patch level and same methods without this
> serie is working fine.
> I wasn't able to ping anything on a minimal rootfs with static ip set
> from cmdline from kernel.
> 
> Sorry for  the lack of details, feel free to add me to any other
> revision of the patch if any.
> I will be able to do more testing next month.

(Sorry for the duplicate, doing a reply-all this time. Not sure why it 
looked like a non-list email the first time)

Thanks for testing. I found the dm8148-t410.dts devicetree in the
kernel source, and it uses the phy_id for both slaves, just like the
EVMSK board I tested. So I can't think of an obvious reason for the
problem. 
Would you be able to send the 'dmesg' log from right after bootup? 
I'm hoping there is an error message in there with some clue.

Just to verify, is your linux-next tree up-to-date? This series needs
to be applied is based on another recent series of 3 patches to cpsw.c. 
Although I doubt it would apply cleanly at all without those.

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

On Thu, 24 Dec 2015 00:34:49 +0100
Nicolas Chauvet <kwizart@gmail.com> wrote:

> 2015-12-23 22:54 GMT+01:00 David Rivshin (Allworx) <
> drivshin.allworx@gmail.com>:
> 
> > On Wed, 23 Dec 2015 22:20:58 +0100
> > Nicolas Chauvet <kwizart@gmail.com> wrote:
> >
> > > 2015-12-23 1:36 GMT+01:00 David Rivshin (Allworx) <
> > > drivshin.allworx@gmail.com>:
> > >
> > > > From: David Rivshin <drivshin@allworx.com>
> > > >
> > > > This series is based on the tip of the net tree.
> > > >
> > > > The first patch fixes a bug that makes dual_emac mode break if
> > > > either slave uses the phy-handle property in the devicetree.
> > > >
> > > > The second patch fixes some cosmetic problems with error
> > > > messages, and also makes the binding documentation more
> > > > explicit.
> > > >
> > > > The third patch cleans up the fixed-link case to work like
> > > > the now-fixed phy-handle case.
> > > >
> > > > I have tested on the following hardware configurations:
> > > >  - (EVMSK) dual emac, phy_id property in both slaves
> > > >  - (BeagleBoneBlack) single emac, phy_id property
> > > >  - (custom) single emac, fixed-link subnode
> > > > Note that I don't have a board which would uses a phy-handle
> > > > property, though I have used hacked devicetrees to exercise the
> > > > code paths. Testing by anyone who has real hardware using
> > > > phy-handle or dual_emac with fixed-link would be appreciated.
> > > >
> > > > David Rivshin (3):
> > > >   drivers: net: cpsw: fix parsing of phy-handle DT property in
> > > > dual_emac config
> > > >   drivers: net: cpsw: fix error messages when using phy-handle
> > > > DT property
> > > >   drivers: net: cpsw: use of_phy_connect() in fixed-link case
> > > >
> > > >  Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
> > > >  drivers/net/ethernet/ti/cpsw.c                 | 40
> > > > +++++++++++++-------------
> > > >  drivers/net/ethernet/ti/cpsw.h                 |  1 +
> > > >  3 files changed, 23 insertions(+), 22 deletions(-)
> > > >
> > > >
> > > This serie failed with me on dm8418 hp-t410 on top of linux-next
> > > from today whereas the same patch level and same methods without
> > > this serie is working fine.
> > > I wasn't able to ping anything on a minimal rootfs with static ip
> > > set from cmdline from kernel.
> > >
> > > Sorry for  the lack of details, feel free to add me to any other
> > > revision of the patch if any.
> > > I will be able to do more testing next month.
> >
> > (Sorry for the duplicate, doing a reply-all this time. Not sure why
> > it looked like a non-list email the first time)
> >
> My bad, I've replied twice, but only last on-list.
> 
> 
> > Thanks for testing. I found the dm8148-t410.dts devicetree in the
> > kernel source, and it uses the phy_id for both slaves, just like the
> > EVMSK board I tested. So I can't think of an obvious reason for the
> > problem.
> > Would you be able to send the 'dmesg' log from right after bootup?
> > I'm hoping there is an error message in there with some clue.
> >
> here is the full dmesg output with this serie applied to linux-next:
> http://ur1.ca/ocvs6
> 
> [    2.281524] davinci_mdio 4a100800.mdio: davinci mdio revision 1.6
> [    2.287909] davinci_mdio 4a100800.mdio: detected phy mask fffffffe
> [    2.302663] Atheros 8031 ethernet 4a100800.mdio:00: GPIO lookup for
> consumer reset
> [    2.302686] Atheros 8031 ethernet 4a100800.mdio:00: using lookup
> tables for GPIO lookup
> [    2.302732] Atheros 8031 ethernet 4a100800.mdio:00: lookup for GPIO
> reset failed
> [    2.302860] libphy: 4a100800.mdio: probed
> [    2.307063] davinci_mdio 4a100800.mdio: phy[0]: device
> 4a100800.mdio:00, driver Atheros 8031 ethernet
> [    2.317945] cpsw 4a100000.ethernet: Detected MACID =
> 00:18:32:60:8e:38
> 
> * 19:06 < nchauvet> btw with this dmesg, I've tried to apply this
> serie: http://marc.info/?l=linux-omap&m=145032865615589&w=2, but it
> doesn't seem to work for me (I cannot ping my gateway anymore)

That particular email was about a v1 patch, which was then replaced
by a 3-patch series for v2:
http://marc.info/?l=linux-netdev&m=145032497014944&w=2
That is already in linux-next, and below you show that you have it.

> So I've remembered that I've double checked the Ethernet wire, but I
> agree it can also be another random issue.

If it works without this series, but fails with it, that is strong
evidence that something in this series either is a bug or exposes
one.

> > Just to verify, is your linux-next tree up-to-date? This series
> > needs to be applied is based on another recent series of 3 patches
> > to cpsw.c. Although I doubt it would apply cleanly at all without
> > those.
> 
> From my linux-next base, related to cpsw, I have:
> git log --oneline cpsw*
> a6b257c Merge remote-tracking branch 'net-next/master'
> dfc0a6d drivers: net: cpsw: increment reference count on fixed-link
> PHY node 
> f1eea5c drivers: net: cpsw: fix RMII/RGMII mode when used
> with fixed-link PHY
> 1873c58 ethernet:ti:cpsw: fix phy identification with multiple slaves
> on fixed-phy
> f188b95 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> 0db19b8 net: cpsw: Fix ethernet regression for dm814
> 
> Theses patches don't lead to any issue on the t410 device.

Thanks for verifying. 1873c58, f1eea5, and cdfc0a6d are the previous
patches I referred to.

> Comparing a dmesg output where the driver work: it doesn't show any
> difference from the quoted lines:
> http://paste.fedoraproject.org/304248/45086839/

Thanks for the additional info. I also don't see any red flags in the 
dmesg output. 

When I looked closer at the dm8148-t410 devicetree, I see that it's 
actually more like the BeagleBones than an EVMSK. It specifies two 
slaves, but is not in dual_emac mode. Other than using RGMII mode 
instead of MII, the emac configuration in the DT looks the same to
me. Further, your dmesg shows there is only one actual PHY, same as
a BeagleBone. I assume that's historical, as specifying slaves=<1> 
used to crash (fixed by 1973db0df7c3b in v4.2).

Despite the similarities, I have been unable to reproduce a failure 
on a BeagleBone-Black running linux-next plus this series. Things
that I've checked in dmesg output look similar to yours, so I'm 
currently out of ideas I can check myself. If I could ask you for
some more assistance in debugging:

Are you using the devicetree from the linux-next source, or is it 
modified? If it's modified, can you send it?

Could you check the phy_interface mode used? I know that being wrong
would generate such symptoms. Quickest way to check that (and related
bits) is probably:
   grep -H . /sys/bus/mdio_bus/devices/*/phy*
Based on the devicetree in the kernel source and your dmesg output, 
I expect there to be a single PHY with phy_interface=rgmii and
phy_id=0x4dd074. If any of those aspects don't match what sysfs 
reports, then that's likely to be the cause.

Failing that, I would next try to apply the 3 patches in this series
one at a time (or bisect them) to identify the specific culprit. 

Thanks.

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[Markus Brunner]

On Wednesday 23 December 2015 20:18:16 David Rivshin wrote:
> On Thu, 24 Dec 2015 00:34:49 +0100
> 
> Nicolas Chauvet <kwizart@gmail.com> wrote:
> > 2015-12-23 22:54 GMT+01:00 David Rivshin (Allworx) <
> > 
> > here is the full dmesg output with this serie applied to linux-next:
> > http://ur1.ca/ocvs6
Linux version 4.4.0-rc6-next-20151223-00029-g858e234 (kwizart@nchauvet-
desktop) (gcc version 5.2.1 20150716 (Red Hat Cross 5.2.1-3) (GCC) ) #15 SMP 
Wed Dec 23 18:29:57 CET 2015

I will refer to this as 0029

> > Comparing a dmesg output where the driver work: it doesn't show any
> > difference from the quoted lines:
> > http://paste.fedoraproject.org/304248/45086839/
Linux version 4.4.0-rc6-next-20151223-00023-g71d15d9 (kwizart@nchauvet-
desktop) (gcc version 5.2.1 20150716 (Red Hat Cross 5.2.1-3) (GCC) ) #12 SMP 
Wed Dec 23 10:36:46 CET 2015

I will refer to this as 0023

I made a "grep fail" on both dmesg and 0023 seems to lack edma, mmc and usb. I 
wonder how ethernet could have worked. There seems also to be a problem with 
the clocks.

fails that are specific to 0023:

ti_dt_clocks_register: failed to lookup clock node sysclk5_ck
ti_dt_clocks_register: failed to lookup clock node sysclk8_ck
platform 47400000.usb: clk_get for pll260dcoclkldo failed
platform 48060000.mmc: clk_get for sysclk8_ck failed
platform 481d8000.mmc: clk_get for sysclk8_ck failed
platform 47810000.mmc: clk_get for sysclk8_ck failed
edma 49000000.edma: pm_runtime_get_sync() failed
edma: probe of 49000000.edma failed with error -22
musb-dsps: probe of 47401400.usb failed with error -16
musb-dsps: probe of 47401c00.usb failed with error -16
cppi41-dma-engine: probe of 47400000.dma-controller failed with error -16
omap_hsmmc: probe of 48060000.mmc failed with error -2
omap_hsmmc: probe of 481d8000.mmc failed with error -2
omap_hsmmc: probe of 47810000.mmc failed with error -2

Re: [PATCH 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[Rob Herring]

On Tue, Dec 22, 2015 at 07:36:33PM -0500, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> and only one need be specified. However if phy-handle was specified, an
> error message would complain about the lack of phy_id or fixed-link.
> 
> Also, if phy-handle was specified and the subsequent of_phy_connect()
> failed, the error message still referenced slaved->data->phy_id, which
> would be empty. Instead, use the name of the device_node as a useful
> identifier.
> 
> Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> Signed-off-by: David Rivshin <drivshin@allworx.com>
> ---
> This would require some adjustments to backport to 4.3-stable due to
> other changes in this area. Let me know if you want a version of this
> against v4.3.3.
> 
>  Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--

For the binding:

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

On Wed, 23 Dec 2015 20:18:16 -0500
"David Rivshin (Allworx)" <drivshin.allworx@gmail.com> wrote:

> On Thu, 24 Dec 2015 00:34:49 +0100
> Nicolas Chauvet <kwizart@gmail.com> wrote:
> 
> > 2015-12-23 22:54 GMT+01:00 David Rivshin (Allworx) <  
> > drivshin.allworx@gmail.com>:  
> >   
> > > On Wed, 23 Dec 2015 22:20:58 +0100
> > > Nicolas Chauvet <kwizart@gmail.com> wrote:
> > >  
> > > > 2015-12-23 1:36 GMT+01:00 David Rivshin (Allworx) <  
> > > > drivshin.allworx@gmail.com>:  
> > > >  
> > > > > From: David Rivshin <drivshin@allworx.com>
> > > > >
> > > > > This series is based on the tip of the net tree.
> > > > >
> > > > > The first patch fixes a bug that makes dual_emac mode break if
> > > > > either slave uses the phy-handle property in the devicetree.
> > > > >
> > > > > The second patch fixes some cosmetic problems with error
> > > > > messages, and also makes the binding documentation more
> > > > > explicit.
> > > > >
> > > > > The third patch cleans up the fixed-link case to work like
> > > > > the now-fixed phy-handle case.
> > > > >
> > > > > I have tested on the following hardware configurations:
> > > > >  - (EVMSK) dual emac, phy_id property in both slaves
> > > > >  - (BeagleBoneBlack) single emac, phy_id property
> > > > >  - (custom) single emac, fixed-link subnode
> > > > > Note that I don't have a board which would uses a phy-handle
> > > > > property, though I have used hacked devicetrees to exercise
> > > > > the code paths. Testing by anyone who has real hardware using
> > > > > phy-handle or dual_emac with fixed-link would be appreciated.
> > > > >
> > > > > David Rivshin (3):
> > > > >   drivers: net: cpsw: fix parsing of phy-handle DT property in
> > > > > dual_emac config
> > > > >   drivers: net: cpsw: fix error messages when using phy-handle
> > > > > DT property
> > > > >   drivers: net: cpsw: use of_phy_connect() in fixed-link case
> > > > >
> > > > >  Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
> > > > >  drivers/net/ethernet/ti/cpsw.c                 | 40
> > > > > +++++++++++++-------------
> > > > >  drivers/net/ethernet/ti/cpsw.h                 |  1 +
> > > > >  3 files changed, 23 insertions(+), 22 deletions(-)
> > > > >
> > > > >  
> > > > This serie failed with me on dm8418 hp-t410 on top of linux-next
> > > > from today whereas the same patch level and same methods without
> > > > this serie is working fine.
> > > > I wasn't able to ping anything on a minimal rootfs with static
> > > > ip set from cmdline from kernel.
> > > >
> > > > Sorry for  the lack of details, feel free to add me to any other
> > > > revision of the patch if any.
> > > > I will be able to do more testing next month.  
> > >
> > > (Sorry for the duplicate, doing a reply-all this time. Not sure
> > > why it looked like a non-list email the first time)
> > >  
> > My bad, I've replied twice, but only last on-list.
> > 
> >   
> > > Thanks for testing. I found the dm8148-t410.dts devicetree in the
> > > kernel source, and it uses the phy_id for both slaves, just like
> > > the EVMSK board I tested. So I can't think of an obvious reason
> > > for the problem.
> > > Would you be able to send the 'dmesg' log from right after bootup?
> > > I'm hoping there is an error message in there with some clue.
> > >  
> > here is the full dmesg output with this serie applied to linux-next:
> > http://ur1.ca/ocvs6
> > 
> > [    2.281524] davinci_mdio 4a100800.mdio: davinci mdio revision 1.6
> > [    2.287909] davinci_mdio 4a100800.mdio: detected phy mask
> > fffffffe [    2.302663] Atheros 8031 ethernet 4a100800.mdio:00:
> > GPIO lookup for consumer reset
> > [    2.302686] Atheros 8031 ethernet 4a100800.mdio:00: using lookup
> > tables for GPIO lookup
> > [    2.302732] Atheros 8031 ethernet 4a100800.mdio:00: lookup for
> > GPIO reset failed
> > [    2.302860] libphy: 4a100800.mdio: probed
> > [    2.307063] davinci_mdio 4a100800.mdio: phy[0]: device
> > 4a100800.mdio:00, driver Atheros 8031 ethernet
> > [    2.317945] cpsw 4a100000.ethernet: Detected MACID =
> > 00:18:32:60:8e:38
> > 
> > * 19:06 < nchauvet> btw with this dmesg, I've tried to apply this
> > serie: http://marc.info/?l=linux-omap&m=145032865615589&w=2, but it
> > doesn't seem to work for me (I cannot ping my gateway anymore)  
> 
> That particular email was about a v1 patch, which was then replaced
> by a 3-patch series for v2:
> http://marc.info/?l=linux-netdev&m=145032497014944&w=2
> That is already in linux-next, and below you show that you have it.
> 
> > So I've remembered that I've double checked the Ethernet wire, but I
> > agree it can also be another random issue.  
> 
> If it works without this series, but fails with it, that is strong
> evidence that something in this series either is a bug or exposes
> one.
> 
> > > Just to verify, is your linux-next tree up-to-date? This series
> > > needs to be applied is based on another recent series of 3 patches
> > > to cpsw.c. Although I doubt it would apply cleanly at all without
> > > those.  
> > 
> > From my linux-next base, related to cpsw, I have:
> > git log --oneline cpsw*
> > a6b257c Merge remote-tracking branch 'net-next/master'
> > dfc0a6d drivers: net: cpsw: increment reference count on fixed-link
> > PHY node 
> > f1eea5c drivers: net: cpsw: fix RMII/RGMII mode when used
> > with fixed-link PHY
> > 1873c58 ethernet:ti:cpsw: fix phy identification with multiple
> > slaves on fixed-phy
> > f188b95 Merge
> > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net 0db19b8
> > net: cpsw: Fix ethernet regression for dm814
> > 
> > Theses patches don't lead to any issue on the t410 device.  
> 
> Thanks for verifying. 1873c58, f1eea5, and cdfc0a6d are the previous
> patches I referred to.
> 
> > Comparing a dmesg output where the driver work: it doesn't show any
> > difference from the quoted lines:
> > http://paste.fedoraproject.org/304248/45086839/  
> 
> Thanks for the additional info. I also don't see any red flags in the 
> dmesg output. 
> 
> When I looked closer at the dm8148-t410 devicetree, I see that it's 
> actually more like the BeagleBones than an EVMSK. It specifies two 
> slaves, but is not in dual_emac mode. Other than using RGMII mode 
> instead of MII, the emac configuration in the DT looks the same to
> me. Further, your dmesg shows there is only one actual PHY, same as
> a BeagleBone. I assume that's historical, as specifying slaves=<1> 
> used to crash (fixed by 1973db0df7c3b in v4.2).
> 
> Despite the similarities, I have been unable to reproduce a failure 
> on a BeagleBone-Black running linux-next plus this series. Things
> that I've checked in dmesg output look similar to yours, so I'm 
> currently out of ideas I can check myself. If I could ask you for
> some more assistance in debugging:
> 
> Are you using the devicetree from the linux-next source, or is it 
> modified? If it's modified, can you send it?
> 
> Could you check the phy_interface mode used? I know that being wrong
> would generate such symptoms. Quickest way to check that (and related
> bits) is probably:
>    grep -H . /sys/bus/mdio_bus/devices/*/phy*
> Based on the devicetree in the kernel source and your dmesg output, 
> I expect there to be a single PHY with phy_interface=rgmii and
> phy_id=0x4dd074. If any of those aspects don't match what sysfs 
> reports, then that's likely to be the cause.
> 
> Failing that, I would next try to apply the 3 patches in this series
> one at a time (or bisect them) to identify the specific culprit. 
> 
> Thanks.

Nicolas,

Did you have a chance to try this series again? I've retested 
on a BeagleBone-Black against both current net and linux-next, 
and I still can't reproduce see the failure you reported. I'm
hoping that was just the result of some unrelated and since-
resolved problem. 

Note that the 3rd patch has a merge conflict now, but it is 
trivially resolved by taking the (deletion of) lines from this 
series. If you'd like me to send a post-merge version, just 
let me know.

Also, I have just CCed you on a related patch that adds a 
dev_warn() which catches the case I mentioned at the end of my
previous email. I would suggest applying that as well, just to
remove some guesswork.

Thanks.