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

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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

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                 | 41 +++++++++++++-------------
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>

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
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

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                 | 41 +++++++++++++-------------
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

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                 | 41 +++++++++++++-------------
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

[PATCH net v2 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>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560326/

 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 42fdfd4..d69cb3f 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.
@@ -2271,15 +2270,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.5

[PATCH net v2 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>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560326/

 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 42fdfd4..d69cb3f 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.
@@ -2271,15 +2270,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.5

[PATCH net v2 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>
Acked-by: Rob Herring <robh@kernel.org>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---
If would like this for -stable it should apply cleanly as far back
as 4.5. It failes on 4.4 due to some context differences, but can be
applied with 'git am -C2'. Or, I can produce a separate patch against
linux-4.4.y if preferred.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] https://patchwork.ozlabs.org/patch/560324/

 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 d69cb3f..3c81413 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 {
 		phy_attached_info(slave->phy);
 
 		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);
@@ -2067,15 +2074,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.5

[PATCH net v2 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>
Acked-by: Rob Herring <robh@kernel.org>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---
If would like this for -stable it should apply cleanly as far back
as 4.5. It failes on 4.4 due to some context differences, but can be
applied with 'git am -C2'. Or, I can produce a separate patch against
linux-4.4.y if preferred.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] https://patchwork.ozlabs.org/patch/560324/

 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 d69cb3f..3c81413 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 {
 		phy_attached_info(slave->phy);
 
 		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);
@@ -2067,15 +2074,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.5

[PATCH net v2 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>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---

Changes since v1 [1]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560327/


 drivers/net/ethernet/ti/cpsw.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3c81413..245f919 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,30 +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->mdio.bus->id,
-				 phy_dev->mdio.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.5

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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>

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-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
Tested-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

Changes since v1 [1]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560327/


 drivers/net/ethernet/ti/cpsw.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3c81413..245f919 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,30 +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->mdio.bus->id,
-				 phy_dev->mdio.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.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v2 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>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
---

Changes since v1 [1]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560327/


 drivers/net/ethernet/ti/cpsw.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3c81413..245f919 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,30 +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->mdio.bus->id,
-				 phy_dev->mdio.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.5

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

[Mugunthan V N]

On Thursday 21 April 2016 11:20 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 

Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

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

[Mugunthan V N]

On Thursday 21 April 2016 11:20 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 

Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

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

[Mugunthan V N]

On Thursday 21 April 2016 11:20 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 

Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

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

[Mugunthan V N]

On Thursday 21 April 2016 11:20 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 

Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

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

[Grygorii Strashko]

On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
> 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>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> I would suggest this for -stable. It should apply cleanly as far back
> as 4.4.
>
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560326/

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

>
>   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 42fdfd4..d69cb3f 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.
> @@ -2271,15 +2270,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 {
>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
> 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>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> I would suggest this for -stable. It should apply cleanly as far back
> as 4.4.
>
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560326/

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

>
>   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 42fdfd4..d69cb3f 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.
> @@ -2271,15 +2270,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 {
>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
> 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>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> I would suggest this for -stable. It should apply cleanly as far back
> as 4.4.
>
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560326/

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

>
>   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 42fdfd4..d69cb3f 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.
> @@ -2271,15 +2270,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 {
>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:26 PM, 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>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> If would like this for -stable it should apply cleanly as far back
> as 4.5. It failes on 4.4 due to some context differences, but can be
> applied with 'git am -C2'. Or, I can produce a separate patch against
> linux-4.4.y if preferred.
> 
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
> - Added Acked-by from Rob Herring for the binding change
> 
> [1] https://patchwork.ozlabs.org/patch/560324/
> 
>   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

May be the "phy_id" can be marked as deprecated? (while here)
The recommended property now is "phy-handle".

>   - 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 d69cb3f..3c81413 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);

Unfortunately,  there are some inconsistency between legacy and FDT API :(
of_phy_connect() will return valid phy_device or NULL, but phy_connect()
can return valid phy_device or ERR_PTR().



>   		slave->phy = NULL;
>   	} else {
>   		phy_attached_info(slave->phy);
>   
>   		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);
> @@ -2067,15 +2074,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;
> 


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:26 PM, 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>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> If would like this for -stable it should apply cleanly as far back
> as 4.5. It failes on 4.4 due to some context differences, but can be
> applied with 'git am -C2'. Or, I can produce a separate patch against
> linux-4.4.y if preferred.
> 
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
> - Added Acked-by from Rob Herring for the binding change
> 
> [1] https://patchwork.ozlabs.org/patch/560324/
> 
>   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

May be the "phy_id" can be marked as deprecated? (while here)
The recommended property now is "phy-handle".

>   - 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 d69cb3f..3c81413 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);

Unfortunately,  there are some inconsistency between legacy and FDT API :(
of_phy_connect() will return valid phy_device or NULL, but phy_connect()
can return valid phy_device or ERR_PTR().



>   		slave->phy = NULL;
>   	} else {
>   		phy_attached_info(slave->phy);
>   
>   		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);
> @@ -2067,15 +2074,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;
> 


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:26 PM, 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>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> If would like this for -stable it should apply cleanly as far back
> as 4.5. It failes on 4.4 due to some context differences, but can be
> applied with 'git am -C2'. Or, I can produce a separate patch against
> linux-4.4.y if preferred.
> 
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
> - Added Acked-by from Rob Herring for the binding change
> 
> [1] https://patchwork.ozlabs.org/patch/560324/
> 
>   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

May be the "phy_id" can be marked as deprecated? (while here)
The recommended property now is "phy-handle".

>   - 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 d69cb3f..3c81413 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);

Unfortunately,  there are some inconsistency between legacy and FDT API :(
of_phy_connect() will return valid phy_device or NULL, but phy_connect()
can return valid phy_device or ERR_PTR().



>   		slave->phy = NULL;
>   	} else {
>   		phy_attached_info(slave->phy);
>   
>   		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);
> @@ -2067,15 +2074,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;
> 


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:26 PM, 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>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> If would like this for -stable it should apply cleanly as far back
> as 4.5. It failes on 4.4 due to some context differences, but can be
> applied with 'git am -C2'. Or, I can produce a separate patch against
> linux-4.4.y if preferred.
> 
> Changes since v1 [1]:
> - Rebased (no conflicts)
> - Added Tested-by from Nicolas Chauvet
> - Added Acked-by from Rob Herring for the binding change
> 
> [1] https://patchwork.ozlabs.org/patch/560324/
> 
>   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

May be the "phy_id" can be marked as deprecated? (while here)
The recommended property now is "phy-handle".

>   - 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 d69cb3f..3c81413 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);

Unfortunately,  there are some inconsistency between legacy and FDT API :(
of_phy_connect() will return valid phy_device or NULL, but phy_connect()
can return valid phy_device or ERR_PTR().



>   		slave->phy = NULL;
>   	} else {
>   		phy_attached_info(slave->phy);
>   
>   		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);
> @@ -2067,15 +2074,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;
> 


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:41 PM, David Rivshin (Allworx) wrote:
> 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>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
>
> Changes since v1 [1]:
> - Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560327/
>

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

>
>   drivers/net/ethernet/ti/cpsw.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3c81413..245f919 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2038,30 +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->mdio.bus->id,
> -				 phy_dev->mdio.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);
>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/21/2016 09:41 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>
> 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-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> Changes since v1 [1]:
> - Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560327/
>

Reviewed-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

>
>   drivers/net/ethernet/ti/cpsw.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3c81413..245f919 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2038,30 +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->mdio.bus->id,
> -				 phy_dev->mdio.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);
>


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 04/21/2016 09:41 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>
> 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-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> Changes since v1 [1]:
> - Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560327/
>

Reviewed-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

>
>   drivers/net/ethernet/ti/cpsw.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3c81413..245f919 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2038,30 +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->mdio.bus->id,
> -				 phy_dev->mdio.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);
>


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 04/21/2016 09:41 PM, David Rivshin (Allworx) wrote:
> 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>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
>
> Changes since v1 [1]:
> - Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
> - Added Tested-by from Nicolas Chauvet
>
> [1] https://patchwork.ozlabs.org/patch/560327/
>

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

>
>   drivers/net/ethernet/ti/cpsw.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3c81413..245f919 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2038,30 +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->mdio.bus->id,
> -				 phy_dev->mdio.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);
>


-- 
regards,
-grygorii

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

[Andrew Goodbody]

> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 
> Changes since v1 [2]:
> - Rebased
> - Added Tested-by from Nicolas Chauvet on all patches
> - Added Acked-by from Rob Herring for the binding change in patch 2 [3]
> 
> [1] http://www.spinics.net/lists/netdev/msg357890.html
> [2] http://www.spinics.net/lists/netdev/msg357772.html
> [3] http://www.spinics.net/lists/netdev/msg358254.html
> 
> 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                 | 41 +++++++++++++-------------
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 
> --
> 2.5.5

Tested on hardware with 2 PHYs but not dual_emac mode, DT has phy-handle entries.

Tested-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>

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

[Andrew Goodbody]

> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 
> Changes since v1 [2]:
> - Rebased
> - Added Tested-by from Nicolas Chauvet on all patches
> - Added Acked-by from Rob Herring for the binding change in patch 2 [3]
> 
> [1] http://www.spinics.net/lists/netdev/msg357890.html
> [2] http://www.spinics.net/lists/netdev/msg357772.html
> [3] http://www.spinics.net/lists/netdev/msg358254.html
> 
> 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                 | 41 +++++++++++++-------------
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 
> --
> 2.5.5

Tested on hardware with 2 PHYs but not dual_emac mode, DT has phy-handle entries.

Tested-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>

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

[Andrew Goodbody]

> From: David Rivshin <drivshin@allworx.com>
> 
> 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
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 
> Changes since v1 [2]:
> - Rebased
> - Added Tested-by from Nicolas Chauvet on all patches
> - Added Acked-by from Rob Herring for the binding change in patch 2 [3]
> 
> [1] http://www.spinics.net/lists/netdev/msg357890.html
> [2] http://www.spinics.net/lists/netdev/msg357772.html
> [3] http://www.spinics.net/lists/netdev/msg358254.html
> 
> 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                 | 41 +++++++++++++-------------
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 
> --
> 2.5.5

Tested on hardware with 2 PHYs but not dual_emac mode, DT has phy-handle entries.

Tested-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>

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

[David Rivshin (Allworx)]

On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/21/2016 09:26 PM, 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>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > If would like this for -stable it should apply cleanly as far back
> > as 4.5. It failes on 4.4 due to some context differences, but can be
> > applied with 'git am -C2'. Or, I can produce a separate patch against
> > linux-4.4.y if preferred.
> > 
> > Changes since v1 [1]:
> > - Rebased (no conflicts)
> > - Added Tested-by from Nicolas Chauvet
> > - Added Acked-by from Rob Herring for the binding change
> > 
> > [1] https://patchwork.ozlabs.org/patch/560324/
> > 
> >   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  
> 
> May be the "phy_id" can be marked as deprecated? (while here)
> The recommended property now is "phy-handle".

I can certainly do that. Perhaps something like this?
 - phy_id		: Specifies slave phy id (deprecated, use phy-handle)

Rob, would you have any issues with bundling that?

> 
> >   - 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 d69cb3f..3c81413 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);  
> 
> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> can return valid phy_device or ERR_PTR().

Good catch, I hadn't noticed that. It looks like that's actually a more 
serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end 
up dereferencing it and pagefaulting.

How about moving the IS_ERR() check into the phy_connect() case like this:
	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))
			slave->phy = NULL;
	}
	if (!slave->phy) {
		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);  
	} else {

Since you say the phy_id case is deprecated anyways, I'm not too concerned
about not printing the error code returned by phy_connect() in that case
(especially since it never did so in the past). That lets us still avoid
duplicating the dev_err() itself.


> 
> 
> 
> >   		slave->phy = NULL;
> >   	} else {
> >   		phy_attached_info(slave->phy);
> >   
> >   		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);
> > @@ -2067,15 +2074,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;
> >   
> 
> 

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

[David Rivshin (Allworx)]

On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/21/2016 09:26 PM, 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>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > If would like this for -stable it should apply cleanly as far back
> > as 4.5. It failes on 4.4 due to some context differences, but can be
> > applied with 'git am -C2'. Or, I can produce a separate patch against
> > linux-4.4.y if preferred.
> > 
> > Changes since v1 [1]:
> > - Rebased (no conflicts)
> > - Added Tested-by from Nicolas Chauvet
> > - Added Acked-by from Rob Herring for the binding change
> > 
> > [1] https://patchwork.ozlabs.org/patch/560324/
> > 
> >   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  
> 
> May be the "phy_id" can be marked as deprecated? (while here)
> The recommended property now is "phy-handle".

I can certainly do that. Perhaps something like this?
 - phy_id		: Specifies slave phy id (deprecated, use phy-handle)

Rob, would you have any issues with bundling that?

> 
> >   - 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 d69cb3f..3c81413 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);  
> 
> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> can return valid phy_device or ERR_PTR().

Good catch, I hadn't noticed that. It looks like that's actually a more 
serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end 
up dereferencing it and pagefaulting.

How about moving the IS_ERR() check into the phy_connect() case like this:
	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))
			slave->phy = NULL;
	}
	if (!slave->phy) {
		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);  
	} else {

Since you say the phy_id case is deprecated anyways, I'm not too concerned
about not printing the error code returned by phy_connect() in that case
(especially since it never did so in the past). That lets us still avoid
duplicating the dev_err() itself.


> 
> 
> 
> >   		slave->phy = NULL;
> >   	} else {
> >   		phy_attached_info(slave->phy);
> >   
> >   		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);
> > @@ -2067,15 +2074,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;
> >   
> 
> 

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

[David Rivshin (Allworx)]

On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/21/2016 09:26 PM, 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>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > If would like this for -stable it should apply cleanly as far back
> > as 4.5. It failes on 4.4 due to some context differences, but can be
> > applied with 'git am -C2'. Or, I can produce a separate patch against
> > linux-4.4.y if preferred.
> > 
> > Changes since v1 [1]:
> > - Rebased (no conflicts)
> > - Added Tested-by from Nicolas Chauvet
> > - Added Acked-by from Rob Herring for the binding change
> > 
> > [1] https://patchwork.ozlabs.org/patch/560324/
> > 
> >   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  
> 
> May be the "phy_id" can be marked as deprecated? (while here)
> The recommended property now is "phy-handle".

I can certainly do that. Perhaps something like this?
 - phy_id		: Specifies slave phy id (deprecated, use phy-handle)

Rob, would you have any issues with bundling that?

> 
> >   - 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 d69cb3f..3c81413 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);  
> 
> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> can return valid phy_device or ERR_PTR().

Good catch, I hadn't noticed that. It looks like that's actually a more 
serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end 
up dereferencing it and pagefaulting.

How about moving the IS_ERR() check into the phy_connect() case like this:
	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))
			slave->phy = NULL;
	}
	if (!slave->phy) {
		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);  
	} else {

Since you say the phy_id case is deprecated anyways, I'm not too concerned
about not printing the error code returned by phy_connect() in that case
(especially since it never did so in the past). That lets us still avoid
duplicating the dev_err() itself.


> 
> 
> 
> >   		slave->phy = NULL;
> >   	} else {
> >   		phy_attached_info(slave->phy);
> >   
> >   		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);
> > @@ -2067,15 +2074,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;
> >   
> 
> 

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

[Grygorii Strashko]

On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> 
>> On 04/21/2016 09:26 PM, 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.

I think, commit message need to be updated.
You not only fix log messages - you also fix the issue with 
of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
 

slave_data->phy_if = of_get_phy_mode(slave_node); 
^ see below
>>>
>>> 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>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> If would like this for -stable it should apply cleanly as far back
>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>> linux-4.4.y if preferred.
>>>
>>> Changes since v1 [1]:
>>> - Rebased (no conflicts)
>>> - Added Tested-by from Nicolas Chauvet
>>> - Added Acked-by from Rob Herring for the binding change
>>>
>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>
>>>    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
>>
>> May be the "phy_id" can be marked as deprecated? (while here)
>> The recommended property now is "phy-handle".
> 
> I can certainly do that. Perhaps something like this?
>   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> 
> Rob, would you have any issues with bundling that?
> 
>>
>>>    - 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 d69cb3f..3c81413 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);
>>
>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>> can return valid phy_device or ERR_PTR().
> 
> Good catch, I hadn't noticed that. It looks like that's actually a more
> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> up dereferencing it and pagefaulting.
> 
> How about moving the IS_ERR() check into the phy_connect() case like this:
> 	if (slave->data->phy_node) {
> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> 				 &cpsw_adjust_link, 0, slave->data->phy_if);

[1]

> 	} else {
> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> 				 &cpsw_adjust_link, slave->data->phy_if);
> 		if (IS_ERR(slave->phy))
> 			slave->phy = NULL;
[2]
> 	}
> 	if (!slave->phy) {
> 		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);
> 	} else {
> 
> Since you say the phy_id case is deprecated anyways, I'm not too concerned
> about not printing the error code returned by phy_connect() in that case
> (especially since it never did so in the past). That lets us still avoid
> duplicating the dev_err() itself.

I'm not worry too much about duplicating dev_err() - it's always good to know
the reason of failure.

So, may be for of_phy_connect() [1]:
 dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
	slave->data->phy_node->full_name,
 	slave->slave_num);

and for phy_connect() [2]:
  dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
  	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));

Mugunthan, any comments?

> 
> 
>>
>>
>>
>>>    		slave->phy = NULL;
>>>    	} else {
>>>    		phy_attached_info(slave->phy);
>>>    
>>>    		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);
>>> @@ -2067,15 +2074,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);

Your change will allow the code to reach this point in case of phy-handle.

>>>    		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;
>>>    
>>
>>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> 
>> On 04/21/2016 09:26 PM, 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.

I think, commit message need to be updated.
You not only fix log messages - you also fix the issue with 
of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
 

slave_data->phy_if = of_get_phy_mode(slave_node); 
^ see below
>>>
>>> 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>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> If would like this for -stable it should apply cleanly as far back
>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>> linux-4.4.y if preferred.
>>>
>>> Changes since v1 [1]:
>>> - Rebased (no conflicts)
>>> - Added Tested-by from Nicolas Chauvet
>>> - Added Acked-by from Rob Herring for the binding change
>>>
>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>
>>>    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
>>
>> May be the "phy_id" can be marked as deprecated? (while here)
>> The recommended property now is "phy-handle".
> 
> I can certainly do that. Perhaps something like this?
>   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> 
> Rob, would you have any issues with bundling that?
> 
>>
>>>    - 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 d69cb3f..3c81413 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);
>>
>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>> can return valid phy_device or ERR_PTR().
> 
> Good catch, I hadn't noticed that. It looks like that's actually a more
> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> up dereferencing it and pagefaulting.
> 
> How about moving the IS_ERR() check into the phy_connect() case like this:
> 	if (slave->data->phy_node) {
> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> 				 &cpsw_adjust_link, 0, slave->data->phy_if);

[1]

> 	} else {
> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> 				 &cpsw_adjust_link, slave->data->phy_if);
> 		if (IS_ERR(slave->phy))
> 			slave->phy = NULL;
[2]
> 	}
> 	if (!slave->phy) {
> 		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);
> 	} else {
> 
> Since you say the phy_id case is deprecated anyways, I'm not too concerned
> about not printing the error code returned by phy_connect() in that case
> (especially since it never did so in the past). That lets us still avoid
> duplicating the dev_err() itself.

I'm not worry too much about duplicating dev_err() - it's always good to know
the reason of failure.

So, may be for of_phy_connect() [1]:
 dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
	slave->data->phy_node->full_name,
 	slave->slave_num);

and for phy_connect() [2]:
  dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
  	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));

Mugunthan, any comments?

> 
> 
>>
>>
>>
>>>    		slave->phy = NULL;
>>>    	} else {
>>>    		phy_attached_info(slave->phy);
>>>    
>>>    		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);
>>> @@ -2067,15 +2074,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);

Your change will allow the code to reach this point in case of phy-handle.

>>>    		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;
>>>    
>>
>>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> 
>> On 04/21/2016 09:26 PM, 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.

I think, commit message need to be updated.
You not only fix log messages - you also fix the issue with 
of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
 

slave_data->phy_if = of_get_phy_mode(slave_node); 
^ see below
>>>
>>> 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>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> If would like this for -stable it should apply cleanly as far back
>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>> linux-4.4.y if preferred.
>>>
>>> Changes since v1 [1]:
>>> - Rebased (no conflicts)
>>> - Added Tested-by from Nicolas Chauvet
>>> - Added Acked-by from Rob Herring for the binding change
>>>
>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>
>>>    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
>>
>> May be the "phy_id" can be marked as deprecated? (while here)
>> The recommended property now is "phy-handle".
> 
> I can certainly do that. Perhaps something like this?
>   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> 
> Rob, would you have any issues with bundling that?
> 
>>
>>>    - 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 d69cb3f..3c81413 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);
>>
>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>> can return valid phy_device or ERR_PTR().
> 
> Good catch, I hadn't noticed that. It looks like that's actually a more
> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> up dereferencing it and pagefaulting.
> 
> How about moving the IS_ERR() check into the phy_connect() case like this:
> 	if (slave->data->phy_node) {
> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> 				 &cpsw_adjust_link, 0, slave->data->phy_if);

[1]

> 	} else {
> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> 				 &cpsw_adjust_link, slave->data->phy_if);
> 		if (IS_ERR(slave->phy))
> 			slave->phy = NULL;
[2]
> 	}
> 	if (!slave->phy) {
> 		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);
> 	} else {
> 
> Since you say the phy_id case is deprecated anyways, I'm not too concerned
> about not printing the error code returned by phy_connect() in that case
> (especially since it never did so in the past). That lets us still avoid
> duplicating the dev_err() itself.

I'm not worry too much about duplicating dev_err() - it's always good to know
the reason of failure.

So, may be for of_phy_connect() [1]:
 dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
	slave->data->phy_node->full_name,
 	slave->slave_num);

and for phy_connect() [2]:
  dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
  	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));

Mugunthan, any comments?

> 
> 
>>
>>
>>
>>>    		slave->phy = NULL;
>>>    	} else {
>>>    		phy_attached_info(slave->phy);
>>>    
>>>    		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);
>>> @@ -2067,15 +2074,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);

Your change will allow the code to reach this point in case of phy-handle.

>>>    		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;
>>>    
>>
>>


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
>> 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>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> ---
>> I would suggest this for -stable. It should apply cleanly as far back
>> as 4.4.
>>
>> Changes since v1 [1]:
>> - Rebased (no conflicts)
>> - Added Tested-by from Nicolas Chauvet
>>
>> [1] https://patchwork.ozlabs.org/patch/560326/
> 
> Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

In my opinion, it will be good to have this patch merged as part of -rc cycle, since
it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.


> 
>>
>>   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 42fdfd4..d69cb3f 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;
>>   }
>>

[..]

-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
>> From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>>
>> 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-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>> Tested-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> I would suggest this for -stable. It should apply cleanly as far back
>> as 4.4.
>>
>> Changes since v1 [1]:
>> - Rebased (no conflicts)
>> - Added Tested-by from Nicolas Chauvet
>>
>> [1] https://patchwork.ozlabs.org/patch/560326/
> 
> Reviewed-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

In my opinion, it will be good to have this patch merged as part of -rc cycle, since
it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.


> 
>>
>>   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 42fdfd4..d69cb3f 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;
>>   }
>>

[..]

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
>> From: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>>
>> 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-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
>> Tested-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> I would suggest this for -stable. It should apply cleanly as far back
>> as 4.4.
>>
>> Changes since v1 [1]:
>> - Rebased (no conflicts)
>> - Added Tested-by from Nicolas Chauvet
>>
>> [1] https://patchwork.ozlabs.org/patch/560326/
> 
> Reviewed-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

In my opinion, it will be good to have this patch merged as part of -rc cycle, since
it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.


> 
>>
>>   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 42fdfd4..d69cb3f 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;
>>   }
>>

[..]

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
>> 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>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> ---
>> I would suggest this for -stable. It should apply cleanly as far back
>> as 4.4.
>>
>> Changes since v1 [1]:
>> - Rebased (no conflicts)
>> - Added Tested-by from Nicolas Chauvet
>>
>> [1] https://patchwork.ozlabs.org/patch/560326/
> 
> Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

In my opinion, it will be good to have this patch merged as part of -rc cycle, since
it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.


> 
>>
>>   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 42fdfd4..d69cb3f 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;
>>   }
>>

[..]

-- 
regards,
-grygorii

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

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> >   
> >> On 04/21/2016 09:26 PM, 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.  
> 
> I think, commit message need to be updated.
> You not only fix log messages - you also fix the issue with 
> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.

You are correct, and that is probably the more important fix compared
to the error messages.

Because the content is becoming less coherent, what I may do is split 
this patch into 3 small patches:
A) devicetree binding documentation changes
B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
   related error message
C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
   returns NULL, and related error message 

Does that sound reasonable?

>  
> 
> slave_data->phy_if = of_get_phy_mode(slave_node); 
> ^ see below
> >>>
> >>> 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>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> If would like this for -stable it should apply cleanly as far back
> >>> as 4.5. It failes on 4.4 due to some context differences, but can be
> >>> applied with 'git am -C2'. Or, I can produce a separate patch against
> >>> linux-4.4.y if preferred.
> >>>
> >>> Changes since v1 [1]:
> >>> - Rebased (no conflicts)
> >>> - Added Tested-by from Nicolas Chauvet
> >>> - Added Acked-by from Rob Herring for the binding change
> >>>
> >>> [1] https://patchwork.ozlabs.org/patch/560324/
> >>>
> >>>    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  
> >>
> >> May be the "phy_id" can be marked as deprecated? (while here)
> >> The recommended property now is "phy-handle".  
> > 
> > I can certainly do that. Perhaps something like this?
> >   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> > 
> > Rob, would you have any issues with bundling that?
> >   
> >>  
> >>>    - 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 d69cb3f..3c81413 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);  
> >>
> >> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> >> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> >> can return valid phy_device or ERR_PTR().  
> > 
> > Good catch, I hadn't noticed that. It looks like that's actually a more
> > serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> > up dereferencing it and pagefaulting.
> > 
> > How about moving the IS_ERR() check into the phy_connect() case like this:
> > 	if (slave->data->phy_node) {
> > 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> > 				 &cpsw_adjust_link, 0, slave->data->phy_if);  
> 
> [1]
> 
> > 	} else {
> > 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > 				 &cpsw_adjust_link, slave->data->phy_if);
> > 		if (IS_ERR(slave->phy))
> > 			slave->phy = NULL;  
> [2]
> > 	}
> > 	if (!slave->phy) {
> > 		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);
> > 	} else {
> > 
> > Since you say the phy_id case is deprecated anyways, I'm not too concerned
> > about not printing the error code returned by phy_connect() in that case
> > (especially since it never did so in the past). That lets us still avoid
> > duplicating the dev_err() itself.  
> 
> I'm not worry too much about duplicating dev_err() - it's always good to know
> the reason of failure.
> 
> So, may be for of_phy_connect() [1]:
>  dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
> 	slave->data->phy_node->full_name,
>  	slave->slave_num);
> 
> and for phy_connect() [2]:
>   dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>   	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
> 
> Mugunthan, any comments?

If that's the preference, then I can incorporate that into V3.

> 
> > 
> >   
> >>
> >>
> >>  
> >>>    		slave->phy = NULL;
> >>>    	} else {
> >>>    		phy_attached_info(slave->phy);
> >>>    
> >>>    		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);
> >>> @@ -2067,15 +2074,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);  
> 
> Your change will allow the code to reach this point in case of phy-handle.
> 
> >>>    		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;
> >>>      
> >>
> >>  
> 
> 

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

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> >   
> >> On 04/21/2016 09:26 PM, 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.  
> 
> I think, commit message need to be updated.
> You not only fix log messages - you also fix the issue with 
> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.

You are correct, and that is probably the more important fix compared
to the error messages.

Because the content is becoming less coherent, what I may do is split 
this patch into 3 small patches:
A) devicetree binding documentation changes
B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
   related error message
C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
   returns NULL, and related error message 

Does that sound reasonable?

>  
> 
> slave_data->phy_if = of_get_phy_mode(slave_node); 
> ^ see below
> >>>
> >>> 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>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> If would like this for -stable it should apply cleanly as far back
> >>> as 4.5. It failes on 4.4 due to some context differences, but can be
> >>> applied with 'git am -C2'. Or, I can produce a separate patch against
> >>> linux-4.4.y if preferred.
> >>>
> >>> Changes since v1 [1]:
> >>> - Rebased (no conflicts)
> >>> - Added Tested-by from Nicolas Chauvet
> >>> - Added Acked-by from Rob Herring for the binding change
> >>>
> >>> [1] https://patchwork.ozlabs.org/patch/560324/
> >>>
> >>>    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  
> >>
> >> May be the "phy_id" can be marked as deprecated? (while here)
> >> The recommended property now is "phy-handle".  
> > 
> > I can certainly do that. Perhaps something like this?
> >   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> > 
> > Rob, would you have any issues with bundling that?
> >   
> >>  
> >>>    - 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 d69cb3f..3c81413 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);  
> >>
> >> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> >> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> >> can return valid phy_device or ERR_PTR().  
> > 
> > Good catch, I hadn't noticed that. It looks like that's actually a more
> > serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> > up dereferencing it and pagefaulting.
> > 
> > How about moving the IS_ERR() check into the phy_connect() case like this:
> > 	if (slave->data->phy_node) {
> > 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> > 				 &cpsw_adjust_link, 0, slave->data->phy_if);  
> 
> [1]
> 
> > 	} else {
> > 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > 				 &cpsw_adjust_link, slave->data->phy_if);
> > 		if (IS_ERR(slave->phy))
> > 			slave->phy = NULL;  
> [2]
> > 	}
> > 	if (!slave->phy) {
> > 		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);
> > 	} else {
> > 
> > Since you say the phy_id case is deprecated anyways, I'm not too concerned
> > about not printing the error code returned by phy_connect() in that case
> > (especially since it never did so in the past). That lets us still avoid
> > duplicating the dev_err() itself.  
> 
> I'm not worry too much about duplicating dev_err() - it's always good to know
> the reason of failure.
> 
> So, may be for of_phy_connect() [1]:
>  dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
> 	slave->data->phy_node->full_name,
>  	slave->slave_num);
> 
> and for phy_connect() [2]:
>   dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>   	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
> 
> Mugunthan, any comments?

If that's the preference, then I can incorporate that into V3.

> 
> > 
> >   
> >>
> >>
> >>  
> >>>    		slave->phy = NULL;
> >>>    	} else {
> >>>    		phy_attached_info(slave->phy);
> >>>    
> >>>    		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);
> >>> @@ -2067,15 +2074,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);  
> 
> Your change will allow the code to reach this point in case of phy-handle.
> 
> >>>    		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;
> >>>      
> >>
> >>  
> 
> 

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

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> >   
> >> On 04/21/2016 09:26 PM, 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.  
> 
> I think, commit message need to be updated.
> You not only fix log messages - you also fix the issue with 
> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.

You are correct, and that is probably the more important fix compared
to the error messages.

Because the content is becoming less coherent, what I may do is split 
this patch into 3 small patches:
A) devicetree binding documentation changes
B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
   related error message
C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
   returns NULL, and related error message 

Does that sound reasonable?

>  
> 
> slave_data->phy_if = of_get_phy_mode(slave_node); 
> ^ see below
> >>>
> >>> 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>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> If would like this for -stable it should apply cleanly as far back
> >>> as 4.5. It failes on 4.4 due to some context differences, but can be
> >>> applied with 'git am -C2'. Or, I can produce a separate patch against
> >>> linux-4.4.y if preferred.
> >>>
> >>> Changes since v1 [1]:
> >>> - Rebased (no conflicts)
> >>> - Added Tested-by from Nicolas Chauvet
> >>> - Added Acked-by from Rob Herring for the binding change
> >>>
> >>> [1] https://patchwork.ozlabs.org/patch/560324/
> >>>
> >>>    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  
> >>
> >> May be the "phy_id" can be marked as deprecated? (while here)
> >> The recommended property now is "phy-handle".  
> > 
> > I can certainly do that. Perhaps something like this?
> >   - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
> > 
> > Rob, would you have any issues with bundling that?
> >   
> >>  
> >>>    - 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 d69cb3f..3c81413 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);  
> >>
> >> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> >> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> >> can return valid phy_device or ERR_PTR().  
> > 
> > Good catch, I hadn't noticed that. It looks like that's actually a more
> > serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
> > up dereferencing it and pagefaulting.
> > 
> > How about moving the IS_ERR() check into the phy_connect() case like this:
> > 	if (slave->data->phy_node) {
> > 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> > 				 &cpsw_adjust_link, 0, slave->data->phy_if);  
> 
> [1]
> 
> > 	} else {
> > 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > 				 &cpsw_adjust_link, slave->data->phy_if);
> > 		if (IS_ERR(slave->phy))
> > 			slave->phy = NULL;  
> [2]
> > 	}
> > 	if (!slave->phy) {
> > 		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);
> > 	} else {
> > 
> > Since you say the phy_id case is deprecated anyways, I'm not too concerned
> > about not printing the error code returned by phy_connect() in that case
> > (especially since it never did so in the past). That lets us still avoid
> > duplicating the dev_err() itself.  
> 
> I'm not worry too much about duplicating dev_err() - it's always good to know
> the reason of failure.
> 
> So, may be for of_phy_connect() [1]:
>  dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
> 	slave->data->phy_node->full_name,
>  	slave->slave_num);
> 
> and for phy_connect() [2]:
>   dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>   	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
> 
> Mugunthan, any comments?

If that's the preference, then I can incorporate that into V3.

> 
> > 
> >   
> >>
> >>
> >>  
> >>>    		slave->phy = NULL;
> >>>    	} else {
> >>>    		phy_attached_info(slave->phy);
> >>>    
> >>>    		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);
> >>> @@ -2067,15 +2074,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);  
> 
> Your change will allow the code to reach this point in case of phy-handle.
> 
> >>>    		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;
> >>>      
> >>
> >>  
> 
> 

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

[Grygorii Strashko]

On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
>>> On Fri, 22 Apr 2016 16:03:34 +0300
>>> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, 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.
>>
>> I think, commit message need to be updated.
>> You not only fix log messages - you also fix the issue with
>> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
>
> You are correct, and that is probably the more important fix compared
> to the error messages.
>
> Because the content is becoming less coherent, what I may do is split
> this patch into 3 small patches:
> A) devicetree binding documentation changes
> B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
>     related error message
> C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
>     returns NULL, and related error message
>
> Does that sound reasonable?

Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.

>
>>
>>
>> slave_data->phy_if = of_get_phy_mode(slave_node);
>> ^ see below
>>>>>
>>>>> 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>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> If would like this for -stable it should apply cleanly as far back
>>>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>>>> linux-4.4.y if preferred.
>>>>>
>>>>> Changes since v1 [1]:
>>>>> - Rebased (no conflicts)
>>>>> - Added Tested-by from Nicolas Chauvet
>>>>> - Added Acked-by from Rob Herring for the binding change
>>>>>
>>>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>>>
>>>>>     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
>>>>
>>>> May be the "phy_id" can be marked as deprecated? (while here)
>>>> The recommended property now is "phy-handle".
>>>
>>> I can certainly do that. Perhaps something like this?
>>>    - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
>>>
>>> Rob, would you have any issues with bundling that?
>>>
>>>>
>>>>>     - 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 d69cb3f..3c81413 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);
>>>>
>>>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>>>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>>>> can return valid phy_device or ERR_PTR().
>>>
>>> Good catch, I hadn't noticed that. It looks like that's actually a more
>>> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
>>> up dereferencing it and pagefaulting.
>>>
>>> How about moving the IS_ERR() check into the phy_connect() case like this:
>>> 	if (slave->data->phy_node) {
>>> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
>>> 				 &cpsw_adjust_link, 0, slave->data->phy_if);
>>
>> [1]
>>
>>> 	} else {
>>> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
>>> 				 &cpsw_adjust_link, slave->data->phy_if);
>>> 		if (IS_ERR(slave->phy))
>>> 			slave->phy = NULL;
>> [2]
>>> 	}
>>> 	if (!slave->phy) {
>>> 		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);
>>> 	} else {
>>>
>>> Since you say the phy_id case is deprecated anyways, I'm not too concerned
>>> about not printing the error code returned by phy_connect() in that case
>>> (especially since it never did so in the past). That lets us still avoid
>>> duplicating the dev_err() itself.
>>
>> I'm not worry too much about duplicating dev_err() - it's always good to know
>> the reason of failure.
>>
>> So, may be for of_phy_connect() [1]:
>>   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
>> 	slave->data->phy_node->full_name,
>>   	slave->slave_num);
>>
>> and for phy_connect() [2]:
>>    dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>>    	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
>>
>> Mugunthan, any comments?
>
> If that's the preference, then I can incorporate that into V3.
>

Yes, please, if no other comments.


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
>>> On Fri, 22 Apr 2016 16:03:34 +0300
>>> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, 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.
>>
>> I think, commit message need to be updated.
>> You not only fix log messages - you also fix the issue with
>> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
>
> You are correct, and that is probably the more important fix compared
> to the error messages.
>
> Because the content is becoming less coherent, what I may do is split
> this patch into 3 small patches:
> A) devicetree binding documentation changes
> B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
>     related error message
> C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
>     returns NULL, and related error message
>
> Does that sound reasonable?

Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.

>
>>
>>
>> slave_data->phy_if = of_get_phy_mode(slave_node);
>> ^ see below
>>>>>
>>>>> 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>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> If would like this for -stable it should apply cleanly as far back
>>>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>>>> linux-4.4.y if preferred.
>>>>>
>>>>> Changes since v1 [1]:
>>>>> - Rebased (no conflicts)
>>>>> - Added Tested-by from Nicolas Chauvet
>>>>> - Added Acked-by from Rob Herring for the binding change
>>>>>
>>>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>>>
>>>>>     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
>>>>
>>>> May be the "phy_id" can be marked as deprecated? (while here)
>>>> The recommended property now is "phy-handle".
>>>
>>> I can certainly do that. Perhaps something like this?
>>>    - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
>>>
>>> Rob, would you have any issues with bundling that?
>>>
>>>>
>>>>>     - 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 d69cb3f..3c81413 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);
>>>>
>>>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>>>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>>>> can return valid phy_device or ERR_PTR().
>>>
>>> Good catch, I hadn't noticed that. It looks like that's actually a more
>>> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
>>> up dereferencing it and pagefaulting.
>>>
>>> How about moving the IS_ERR() check into the phy_connect() case like this:
>>> 	if (slave->data->phy_node) {
>>> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
>>> 				 &cpsw_adjust_link, 0, slave->data->phy_if);
>>
>> [1]
>>
>>> 	} else {
>>> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
>>> 				 &cpsw_adjust_link, slave->data->phy_if);
>>> 		if (IS_ERR(slave->phy))
>>> 			slave->phy = NULL;
>> [2]
>>> 	}
>>> 	if (!slave->phy) {
>>> 		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);
>>> 	} else {
>>>
>>> Since you say the phy_id case is deprecated anyways, I'm not too concerned
>>> about not printing the error code returned by phy_connect() in that case
>>> (especially since it never did so in the past). That lets us still avoid
>>> duplicating the dev_err() itself.
>>
>> I'm not worry too much about duplicating dev_err() - it's always good to know
>> the reason of failure.
>>
>> So, may be for of_phy_connect() [1]:
>>   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
>> 	slave->data->phy_node->full_name,
>>   	slave->slave_num);
>>
>> and for phy_connect() [2]:
>>    dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>>    	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
>>
>> Mugunthan, any comments?
>
> If that's the preference, then I can incorporate that into V3.
>

Yes, please, if no other comments.


-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
>>> On Fri, 22 Apr 2016 16:03:34 +0300
>>> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, 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.
>>
>> I think, commit message need to be updated.
>> You not only fix log messages - you also fix the issue with
>> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.
>
> You are correct, and that is probably the more important fix compared
> to the error messages.
>
> Because the content is becoming less coherent, what I may do is split
> this patch into 3 small patches:
> A) devicetree binding documentation changes
> B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
>     related error message
> C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
>     returns NULL, and related error message
>
> Does that sound reasonable?

Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.

>
>>
>>
>> slave_data->phy_if = of_get_phy_mode(slave_node);
>> ^ see below
>>>>>
>>>>> 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>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> If would like this for -stable it should apply cleanly as far back
>>>>> as 4.5. It failes on 4.4 due to some context differences, but can be
>>>>> applied with 'git am -C2'. Or, I can produce a separate patch against
>>>>> linux-4.4.y if preferred.
>>>>>
>>>>> Changes since v1 [1]:
>>>>> - Rebased (no conflicts)
>>>>> - Added Tested-by from Nicolas Chauvet
>>>>> - Added Acked-by from Rob Herring for the binding change
>>>>>
>>>>> [1] https://patchwork.ozlabs.org/patch/560324/
>>>>>
>>>>>     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
>>>>
>>>> May be the "phy_id" can be marked as deprecated? (while here)
>>>> The recommended property now is "phy-handle".
>>>
>>> I can certainly do that. Perhaps something like this?
>>>    - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
>>>
>>> Rob, would you have any issues with bundling that?
>>>
>>>>
>>>>>     - 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 d69cb3f..3c81413 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);
>>>>
>>>> Unfortunately,  there are some inconsistency between legacy and FDT API :(
>>>> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
>>>> can return valid phy_device or ERR_PTR().
>>>
>>> Good catch, I hadn't noticed that. It looks like that's actually a more
>>> serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end
>>> up dereferencing it and pagefaulting.
>>>
>>> How about moving the IS_ERR() check into the phy_connect() case like this:
>>> 	if (slave->data->phy_node) {
>>> 		slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
>>> 				 &cpsw_adjust_link, 0, slave->data->phy_if);
>>
>> [1]
>>
>>> 	} else {
>>> 		slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
>>> 				 &cpsw_adjust_link, slave->data->phy_if);
>>> 		if (IS_ERR(slave->phy))
>>> 			slave->phy = NULL;
>> [2]
>>> 	}
>>> 	if (!slave->phy) {
>>> 		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);
>>> 	} else {
>>>
>>> Since you say the phy_id case is deprecated anyways, I'm not too concerned
>>> about not printing the error code returned by phy_connect() in that case
>>> (especially since it never did so in the past). That lets us still avoid
>>> duplicating the dev_err() itself.
>>
>> I'm not worry too much about duplicating dev_err() - it's always good to know
>> the reason of failure.
>>
>> So, may be for of_phy_connect() [1]:
>>   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
>> 	slave->data->phy_node->full_name,
>>   	slave->slave_num);
>>
>> and for phy_connect() [2]:
>>    dev_err(priv->dev, "phy %s not found on slave %d, err %d\n",
>>    	slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy));
>>
>> Mugunthan, any comments?
>
> If that's the preference, then I can incorporate that into V3.
>

Yes, please, if no other comments.


-- 
regards,
-grygorii

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

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:14:07 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> > On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:  
> >> 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>
> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >> ---
> >> I would suggest this for -stable. It should apply cleanly as far back
> >> as 4.4.
> >>
> >> Changes since v1 [1]:
> >> - Rebased (no conflicts)
> >> - Added Tested-by from Nicolas Chauvet
> >>
> >> [1] https://patchwork.ozlabs.org/patch/560326/  
> > 
> > Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>  
> 
> In my opinion, it will be good to have this patch merged as part of -rc cycle, since
> it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.

Dave, 
If you'd like to take just this first patch while enhancements for patch 2
are worked out, I'd have no problem with that. I would then just submit the 
rest of the series separately. If I don't see that you've taken this by the
time I have a V3 ready I'll include it again, but it will be unchanged and 
you can still take it separately if you wish. Or I can resubmit this patch
separately if you prefer.

(FYI, I tried to send this multiple times last night, but gmail has not 
been my friend lately. I ended up having to trim the CC list substantially
just to get this out at all; apologies for that. Suggestions for other 
email providers that are usable for patch submissions are welcome.)

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

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:14:07 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> > On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:  
> >> 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>
> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >> ---
> >> I would suggest this for -stable. It should apply cleanly as far back
> >> as 4.4.
> >>
> >> Changes since v1 [1]:
> >> - Rebased (no conflicts)
> >> - Added Tested-by from Nicolas Chauvet
> >>
> >> [1] https://patchwork.ozlabs.org/patch/560326/  
> > 
> > Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>  
> 
> In my opinion, it will be good to have this patch merged as part of -rc cycle, since
> it will fix "NULL pointer dereference" issue with current LKML as reported by Andrew Goodbody.

Dave, 
If you'd like to take just this first patch while enhancements for patch 2
are worked out, I'd have no problem with that. I would then just submit the 
rest of the series separately. If I don't see that you've taken this by the
time I have a V3 ready I'll include it again, but it will be unchanged and 
you can still take it separately if you wish. Or I can resubmit this patch
separately if you prefer.

(FYI, I tried to send this multiple times last night, but gmail has not 
been my friend lately. I ended up having to trim the CC list substantially
just to get this out at all; apologies for that. Suggestions for other 
email providers that are usable for patch submissions are welcome.)