From: David Rivshin <[email protected]>
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
From: David Rivshin <[email protected]>
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 <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
---
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
From: David Rivshin <[email protected]>
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
---
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
From: David Rivshin <[email protected]>
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 <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
---
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
On Thursday 21 April 2016 11:20 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <[email protected]>
>
> 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 <[email protected]>
Regards
Mugunthan V N
On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> ---
> 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 <[email protected]>
>
> 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
On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> ---
> 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
On 04/21/2016 09:41 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> ---
>
> 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 <[email protected]>
>
> 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
> From: David Rivshin <[email protected]>
>
> 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 <[email protected]>
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <[email protected]> wrote:
> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <[email protected]>
> >
> > 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 <[email protected]>
> > Acked-by: Rob Herring <[email protected]>
> > Tested-by: Nicolas Chauvet <[email protected]>
> > ---
> > 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;
> >
>
>
On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <[email protected]> wrote:
>
>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>> From: David Rivshin <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Acked-by: Rob Herring <[email protected]>
>>> Tested-by: Nicolas Chauvet <[email protected]>
>>> ---
>>> 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
On 04/22/2016 04:03 PM, Grygorii Strashko wrote:
> On 04/21/2016 09:19 PM, David Rivshin (Allworx) wrote:
>> From: David Rivshin <[email protected]>
>>
>> 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 <[email protected]>
>> Tested-by: Nicolas Chauvet <[email protected]>
>> ---
>> 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 <[email protected]>
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
On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <[email protected]> wrote:
> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <[email protected]> wrote:
> >
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> >>> From: David Rivshin <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> Acked-by: Rob Herring <[email protected]>
> >>> Tested-by: Nicolas Chauvet <[email protected]>
> >>> ---
> >>> 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;
> >>>
> >>
> >>
>
>
On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <[email protected]> wrote:
>
>> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
>>> On Fri, 22 Apr 2016 16:03:34 +0300
>>> Grygorii Strashko <[email protected]> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>>>> From: David Rivshin <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Acked-by: Rob Herring <[email protected]>
>>>>> Tested-by: Nicolas Chauvet <[email protected]>
>>>>> ---
>>>>> 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