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

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

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

[1] http://patchwork.ozlabs.org/patch/613276/
[2] http://patchwork.ozlabs.org/patch/560327/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/529


 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 712bc6d..e2fcdf1 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2044,30 +2044,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (slave_data->phy_node) {
dev_dbg(>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(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

[PATCH net v3 4/5] dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. Make this clear in the binding doc.

Also mark the phy_id property as deprecated, as phy-handle should be
used instead.

Signed-off-by: David Rivshin 
---

Changes since v2 [1]:
- split from previous patch 2
- marked the phy_id property as deprecated [3]
- removed Rob Herring's Acked-by due to above change

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

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/494


 Documentation/devicetree/bindings/net/cpsw.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..0ae0649 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -41,21 +41,21 @@ Optional properties:
 Slave Properties:
 Required properties:
 - phy-mode : See ethernet.txt file in the same directory
 
 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_id   : Specifies slave phy id (deprecated, use 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.
 
-- 
2.5.5

[PATCH net v3 3/5] drivers: net: cpsw: don't ignore phy-mode if phy-handle is used

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

The phy-mode emac property was only being processed in the phy_id
or fixed-link cases. However if phy-handle was specified instead,
an error message would complain about the lack of phy_id or
fixed-link, and then jump past the of_get_phy_mode(). This would
result in the PHY mode defaulting to MII, regardless of what the
devicetree specified.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- split from previous patch 2
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- rewrote commit log to focus on the functional bug fixed, rather
  than the bogus error message

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

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63


 drivers/net/ethernet/ti/cpsw.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5903448..712bc6d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2039,15 +2039,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", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>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);
@@ -2076,15 +2080,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
if (!mdio) {
dev_err(>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(>dev, "No slave[%d] phy_id or fixed-link 
property\n", i);
+   dev_err(>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(>dev, "Missing or malformed slave[%d] 
phy-mode property\n",
i);
return slave_data->phy_if;
-- 
2.5.5

[PATCH net v3 2/5] drivers: net: cpsw: fix segfault in case of bad phy-handle

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

If an emac node has a phy-handle property that points to something
which is not a phy, then a segmentation fault will occur when the
interface is brought up. This is because while phy_connect() will
return ERR_PTR() on failure, of_phy_connect() will return NULL.
The common error check uses IS_ERR(), and so missed when
of_phy_connect() fails. The NULL pointer is then dereferenced.

Also, the common error message referenced slave->data->phy_id,
which would be empty in the case of phy-handle. Instead, use the
name of the device_node as a useful identifier. And in the phy_id
case add the error code for completeness.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.5, although there is a trivial conflict in 4.4. I can produce a
separate patch against linux-4.4.y if preferred.

Changes since v2:
- new patch, although fixing part of previous patch 2 [1]

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

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/


 drivers/net/ethernet/ti/cpsw.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ce0b0ca..5903448 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1143,33 +1143,42 @@ 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 (slave->data->phy_node)
+   if (slave->data->phy_node) {
slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
-   else
+   if (!slave->phy) {
+   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+   slave->data->phy_node->full_name,
+   slave->slave_num);
+   return;
+   }
+   } else {
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _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);
-   slave->phy = NULL;
-   } else {
-   phy_attached_info(slave->phy);
-
-   phy_start(slave->phy);
-
-   /* Configure GMII_SEL register */
-   cpsw_phy_sel(>pdev->dev, slave->phy->interface,
-slave->slave_num);
+   if (IS_ERR(slave->phy)) {
+   dev_err(priv->dev,
+   "phy \"%s\" not found on slave %d, err %ld\n",
+   slave->data->phy_id, slave->slave_num,
+   PTR_ERR(slave->phy));
+   slave->phy = NULL;
+   return;
+   }
}
+
+   phy_attached_info(slave->phy);
+
+   phy_start(slave->phy);
+
+   /* Configure GMII_SEL register */
+   cpsw_phy_sel(>pdev->dev, slave->phy->interface, slave->slave_num);
 }
 
 static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
 {
const int vlan = priv->data.default_vlan;
const int port = priv->host_port;
u32 reg;
-- 
2.5.5

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

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

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

[1] http://patchwork.ozlabs.org/patch/613237/
[2] http://patchwork.ozlabs.org/patch/560326/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/496


 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 bbb77cd..ce0b0ca 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,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _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 = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -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", );
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

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

2016-04-27 Thread David Rivshin (Allworx) 
From: David Rivshin 

This series fixes a number of related issues around using phy-handle
properties in cpsw emac nodes.

Patch 1 fixes a bug if more than one slave is used, and either
slave uses the phy-handle property in the devicetree.

Patch 2 fixes a NULL pointer dereference which can occur if a
phy-handle property is used and of_phy_connect() return NULL,
such as with a bad devicetree.

Patch 3 fixes an issue where the phy-mode property would be ignored
if a phy-handle property was used. This also fixes a bogus error
message that would be emitted.

Patch 4 fixes makes the binding documentation more explicit that
exactly one PHY property should be used, and also marks phy_id as
deprecated.

Patch 5 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
 - (EVMSK) a bad phy-handle property pointing to 
 - (EVMSK) phy_id property with incorrect PHY address
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Andrew Goodbody reported testing v2 on a board that doesn't use
dual_emac mode, but with 2 PHYs using phy-handle properties [1].

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

Markus Brunner reported testing v1 on the following [2]:
 - 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

[1] https://lkml.org/lkml/2016/4/22/537
[2] http://www.spinics.net/lists/netdev/msg357890.html

David Rivshin (5):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix segfault in case of bad phy-handle
  drivers: net: cpsw: don't ignore phy-mode if phy-handle is used
  dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  6 +--
 drivers/net/ethernet/ti/cpsw.c | 69 ++
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 41 insertions(+), 35 deletions(-)

-- 
2.5.5

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

2016-04-26 Thread David Rivshin (Allworx) 
On Mon, 25 Apr 2016 22:14:07 +0300
Grygorii Strashko <grygorii.stras...@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 <drivs...@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 <drivs...@allworx.com>
> >> Tested-by: Nicolas Chauvet <kwiz...@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.stras...@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 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

2016-04-25 Thread David Rivshin (Allworx) 
On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.stras...@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.stras...@ti.com> wrote:
> >   
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin <drivs...@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 <drivs...@allworx.com>
> >>> Acked-by: Rob Herring <r...@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwiz...@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 dat

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

2016-04-22 Thread David Rivshin (Allworx) 
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.stras...@ti.com> wrote:

> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivs...@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 <drivs...@allworx.com>
> > Acked-by: Rob Herring <r...@kernel.org>
> > Tested-by: Nicolas Chauvet <kwiz...@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,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> >  _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 legac

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

2016-04-21 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
Tested-by: Nicolas Chauvet 
---

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", );
if (slave_data->phy_node) {
dev_dbg(>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(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

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

2016-04-21 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
Acked-by: Rob Herring 
Tested-by: Nicolas Chauvet 
---
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,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _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", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>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(>dev,

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

2016-04-21 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
Tested-by: Nicolas Chauvet 
---
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,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _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 = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -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", );
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(>dev);
 
/* Select default pin state */
pinctrl_pm_select_default_state(>dev);
 
-   if (cpsw_probe_dt(priv, pdev)) {
+   if (cpsw_probe_dt(>data, pdev)) {
dev_err(>dev, "cpsw: platform data missing\n");
ret =

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

2016-04-21 Thread David Rivshin (Allworx) 
From: David Rivshin 

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

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

2016-04-20 Thread David Rivshin (Allworx) 
Sorry all for the noise. Gmail seems to be deciding that this outgoing 
mail is spammy, and starts blocking it part-way through. I've tried 
cutting down the CC list, but still no luck. If anyone knows how to get 
around this (while still having a reasonable patch submission), please 
let me know. 

For tonight, I guess I have no choice but to give up. I'll try again
tomorrow in hopes gmail becomes sane again.


On Wed, 20 Apr 2016 23:24:39 -0400
"David Rivshin (Allworx)" <drivshin.allw...@gmail.com> wrote:

> From: David Rivshin <drivs...@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(-)
>

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

2016-04-20 Thread David Rivshin (Allworx) 
From: David Rivshin 

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

2016-04-20 Thread David Rivshin (Allworx) 
From: David Rivshin 

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

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

2016-04-19 Thread David Rivshin (Allworx) 
On Tue, 19 Apr 2016 18:43:39 -0400 (EDT)
David Miller  wrote:

> From: Grygorii Strashko 
> Date: Tue, 19 Apr 2016 21:44:09 +0300
> 
> > May be you can send revert + your patch 1 (only fix for this issue).
> > 
> > Dave, Does that sound good to you?  
> 
> Sure.

OK, I will hopefully have that ready tomorrow evening.

Anything in particular I should mention in the revert commit message?
I didn't find a discussion on it, other than the earlier statement that 
"it breaks boot on many TI boards".

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

2016-04-19 Thread David Rivshin (Allworx) 
On Tue, 19 Apr 2016 18:44:41 +0300
Grygorii Strashko <grygorii.stras...@ti.com> wrote:

> On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
> > On Tue, 19 Apr 2016 17:41:07 +0300
> > Grygorii Strashko <grygorii.stras...@ti.com> wrote:
> >   
> >> Hi,
> >>
> >> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:  
> >>> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> >>> as below. Fix by maintaining a reference to each PHY node in slave
> >>> struct instead of a single reference in the priv struct which was
> >>> overwritten by the 2nd PHY.  
> >>
> >> David, Is it possible to drop prev version of this patch from linux-next
> >> - it breaks boot on many TI boards with -next.
> >>
> >>  
> >>>
> >>> [   17.870933] Unable to handle kernel NULL pointer dereference at 
> >>> virtual address 0180
> >>> [   17.879557] pgd = dc8bc000
> >>> [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> >>> [   17.889213] Internal error: Oops: 17 [#1] ARM
> >>> [   17.893838] Modules linked in:
> >>> [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> >>> 4.5.0-ge463dfb-dirty #11
> >>> [   17.904947] Hardware name: Cambrionix whippet
> >>> [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> >>> [   17.915339] PC is at phy_attached_print+0x18/0x8c
> >>> [   17.920339] LR is at phy_attached_info+0x14/0x18
> >>> [   17.925247] pc : []lr : []psr: 600f0113
> >>> [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> >>> [   17.937425] r10: dda7a400  r9 :   r8 : 
> >>> [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> >>> [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> >>> [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  
> >>> Segment none
> >>> [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> >>> [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> >>> [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> >>
> >> [...]
> >>  
> >>> [   18.323956] [] (inet_ioctl) from [] 
> >>> (sock_ioctl+0x15c/0x2d8)
> >>> [   18.331829] [] (sock_ioctl) from [] 
> >>> (do_vfs_ioctl+0x98/0x8d0)
> >>> [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> >>> [   18.345822] [] (do_vfs_ioctl) from [] 
> >>> (SyS_ioctl+0x74/0x84)
> >>> [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> >>> r6:dc8ab4c0 r5:dc8ab4c0
> >>> [   18.361924]  r4:
> >>> [   18.364653] [] (SyS_ioctl) from [] 
> >>> (ret_fast_syscall+0x0/0x3c)
> >>> [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 
> >>> r5:0011 r4:
> >>> [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> >>> [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> >>
> >> ^ Could you make it shorter and drop timestamps, pls?
> >>  
> >>>
> >>> Signed-off-by: Andrew Goodbody <andrew.goodb...@cambrionix.com>
> >>> ---
> >>>
> >>> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt 
> >>> so it
> >>>has data->slaves initialised first which is needed to calculate 
> >>> size
> >>>
> >>>drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >>>1 file changed, 15 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c 
> >>> b/drivers/net/ethernet/ti/cpsw.c
> >>> index 42fdfd4..e62909c 100644
> >>> --- a/drivers/net/ethernet/ti/cpsw.c
> >>> +++ b/drivers/net/ethernet/ti/cpsw.c
> >>> @@ -349,6 +349,7 @@ struct cpsw_slave {
> >>>   struct cpsw_slave_data  *data;
> >>>   struct phy_device   *phy;
> >>>   struct net_device   *ndev;
> >>> + struct device_node  *phy_node;
> >>>   u32 port_vlan;
> >>>   u32 open_stat;
> >>>};
> >>> @@ -367,7 +368,6 @@ struct cpsw_priv {
> >>>   spinlock_t  lock;
> &

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

2016-04-19 Thread David Rivshin (Allworx) 
On Tue, 19 Apr 2016 17:41:07 +0300
Grygorii Strashko  wrote:

> Hi,
> 
> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> > Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> > as below. Fix by maintaining a reference to each PHY node in slave
> > struct instead of a single reference in the priv struct which was
> > overwritten by the 2nd PHY.  
> 
> David, Is it possible to drop prev version of this patch from linux-next
> - it breaks boot on many TI boards with -next.
> 
> 
> > 
> > [   17.870933] Unable to handle kernel NULL pointer dereference at virtual 
> > address 0180
> > [   17.879557] pgd = dc8bc000
> > [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> > [   17.889213] Internal error: Oops: 17 [#1] ARM
> > [   17.893838] Modules linked in:
> > [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> > 4.5.0-ge463dfb-dirty #11
> > [   17.904947] Hardware name: Cambrionix whippet
> > [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> > [   17.915339] PC is at phy_attached_print+0x18/0x8c
> > [   17.920339] LR is at phy_attached_info+0x14/0x18
> > [   17.925247] pc : []lr : []psr: 600f0113
> > [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> > [   17.937425] r10: dda7a400  r9 :   r8 : 
> > [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> > [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> > [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment 
> > none
> > [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> > [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> > [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> 
> [...]
> 
> > [   18.323956] [] (inet_ioctl) from [] 
> > (sock_ioctl+0x15c/0x2d8)
> > [   18.331829] [] (sock_ioctl) from [] 
> > (do_vfs_ioctl+0x98/0x8d0)
> > [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> > [   18.345822] [] (do_vfs_ioctl) from [] 
> > (SyS_ioctl+0x74/0x84)
> > [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> > r6:dc8ab4c0 r5:dc8ab4c0
> > [   18.361924]  r4:
> > [   18.364653] [] (SyS_ioctl) from [] 
> > (ret_fast_syscall+0x0/0x3c)
> > [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011 
> > r4:
> > [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> > [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> 
> ^ Could you make it shorter and drop timestamps, pls?
> 
> > 
> > Signed-off-by: Andrew Goodbody 
> > ---
> > 
> > v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so 
> > it
> >   has data->slaves initialised first which is needed to calculate size
> > 
> >   drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >   1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 42fdfd4..e62909c 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -349,6 +349,7 @@ struct cpsw_slave {
> > struct cpsw_slave_data  *data;
> > struct phy_device   *phy;
> > struct net_device   *ndev;
> > +   struct device_node  *phy_node;
> > u32 port_vlan;
> > u32 open_stat;
> >   };
> > @@ -367,7 +368,6 @@ 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;
> > @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
> > struct cpsw_priv *priv)
> > 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->phy_node)
> > +   slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > struct device_node *node = pdev->dev.of_node;
> > struct device_node *slave_node;
> > struct cpsw_platform_data *data = >data;
> > -   int i = 0, ret;
> > +   int i, ret;
> > u32 prop;
> >   
> > if (!node)
> > @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > }
> > data->slaves = prop;
> >   
> > +   priv->slaves = devm_kzalloc(>dev,
> > +

[PATCH] drivers: net: cpsw-phy-sel: add dev_warn() for unsupported PHY mode

2016-02-12 Thread David Rivshin (Allworx) 
From: David Rivshin 

The cpsw-phy-sel driver supports only MII, RMII, and RGMII PHY modes,
and silently handled any other values as if MII was specified. In a
case where the PHY mode was incorrectly specified, or a bug elsewhere,
there would be no indication of a problem. If MII was the correct mode,
then this will go unnoticed, otherwise the symptom will be a failure
to transmit/receive data over the RMII/RGMII link.

Add a dev_warn() to make this condition obvious and provide a
breadcrumb to follow.

Cc: Mugunthan V N 
Signed-off-by: David Rivshin 
---

Dave,
 I'm not sure if you'd consider this net or net-next material, but it will
apply cleanly to either. Indeed, if you want it for -stable, it applies back
as far as 3.18.

 drivers/net/ethernet/ti/cpsw-phy-sel.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c 
b/drivers/net/ethernet/ti/cpsw-phy-sel.c
index e9cc61e..c3e85ac 100644
--- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
+++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
@@ -63,8 +63,12 @@ static void cpsw_gmii_sel_am3352(struct cpsw_phy_sel_priv 
*priv,
mode = AM33XX_GMII_SEL_MODE_RGMII;
break;
 
-   case PHY_INTERFACE_MODE_MII:
default:
+   dev_warn(priv->dev,
+"Unsupported PHY mode: \"%s\". Defaulting to MII.\n",
+   phy_modes(phy_mode));
+   /* fallthrough */
+   case PHY_INTERFACE_MODE_MII:
mode = AM33XX_GMII_SEL_MODE_MII;
break;
};
@@ -106,8 +110,12 @@ static void cpsw_gmii_sel_dra7xx(struct cpsw_phy_sel_priv 
*priv,
mode = AM33XX_GMII_SEL_MODE_RGMII;
break;
 
-   case PHY_INTERFACE_MODE_MII:
default:
+   dev_warn(priv->dev,
+"Unsupported PHY mode: \"%s\". Defaulting to MII.\n",
+   phy_modes(phy_mode));
+   /* fallthrough */
+   case PHY_INTERFACE_MODE_MII:
mode = AM33XX_GMII_SEL_MODE_MII;
break;
};
-- 
2.5.0

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

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

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

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

2015-12-23 Thread David Rivshin (Allworx) 
On Thu, 24 Dec 2015 00:34:49 +0100
Nicolas Chauvet <kwiz...@gmail.com> wrote:

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

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

> So I've remembered that I've double checked the Ethernet w

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

2015-12-23 Thread David Rivshin (Allworx) 
On Wed, 23 Dec 2015 19:35:37 +0100
Markus Brunner <systemprogrammierung.brun...@gmail.com> wrote:

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

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

I think that covers all the interesting cases.

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

[...]

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

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

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

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

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

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

2015-12-23 Thread David Rivshin (Allworx) 
On Wed, 23 Dec 2015 22:20:58 +0100
Nicolas Chauvet <kwiz...@gmail.com> wrote:

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

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

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

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

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

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

2015-12-22 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
---
 drivers/net/ethernet/ti/cpsw.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

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

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

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

2015-12-22 Thread David Rivshin (Allworx) 
From: David Rivshin 

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

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

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

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

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

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

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

-- 
2.5.0

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

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

2015-12-22 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
---
This would require some adjustments to backport to 4.3-stable due to
other changes in this area. Let me know if you want a version of this
against v4.3.3.

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

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

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

2015-12-22 Thread David Rivshin (Allworx) 
From: David Rivshin 

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 
---
You may want to consider this for 4.3-stable. It manages to apply
on top of v4.3.3 with 'git am -C1', or I can produce a separate
patch against v4.3.3 if preferred.

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

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3b489ca..8ad0ed8 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, 
u32 val, u32 offset)
__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
spinlock_t  lock;
struct platform_device  *pdev;
struct net_device   *ndev;
-   struct device_node  *phy_node;
struct napi_struct  napi_rx;
struct napi_struct  napi_tx;
struct device   *dev;
struct cpsw_platform_data   data;
struct cpsw_ss_regs __iomem *regs;
struct cpsw_wr_regs __iomem *wr_regs;
u8 __iomem  *hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (priv->phy_node)
-   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+   if (slave->data->phy_node)
+   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _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 = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -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", );
if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
@@ -2270,15 +2269,15 @@ static int cpsw_probe(struct platform_device *pdev)
 * This may be required here for child devices.
 */
pm_runtime_enable(>dev);
 
/* Select default pin state */
pinctrl_pm_select_default_state(>dev);
 
-   if (cpsw_probe_dt(priv, pdev)) {
+   if (cpsw_probe_dt(>data, pdev)) {
dev_err(>dev, "cpsw: platform data missing\n");
ret = -ENODEV;
goto clean_runtime_disable_ret;
}
data = >data;

Re: [PATCH v2 0/3] drivers: net: cpsw: Fix bugs in fixed-link PHY DT parsing

2015-12-18 Thread David Rivshin (Allworx) 
On Fri, 18 Dec 2015 11:20:21 +0100
Daniel Trautmann <dtrautm...@ibhsoftec.com> wrote:

> On Thu, Dec 17, 2015 at 03:45:08PM -0500, David Miller wrote:
> > From: "David Rivshin (Allworx)" <drivshin.allw...@gmail.com>
> > Date: Wed, 16 Dec 2015 23:02:08 -0500
> > 
> > > I have tested on the following hardware configurations:
> > >  - (EVMSK) dual emac with two real MDIO-connected phys using
> > > RGMII-TXID
> > >  - single emac with fixed-link using RGMII
> > > Testing of other CPSW emac configurations that folks may have
> > > would be appreciated.
> > 
> > I'm going to wait until some others give some feedback and testing
> > results on this one, thanks.
> 
> I can confirm that this patch is working on the following hardware
> setup:
>  - single emac using MII connected with fixed-link to Micrel KSZ8895
> Switch

Thanks for testing.

I see that Dave applied the series, but just for good measure I'll 
report that I also tested on:
 - (BeagleBone-Black) single emac, MII, real 100Mbit PHY

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

Re: pull-request: mac80211 2015-12-15

2015-12-17 Thread David Rivshin (Allworx) 
On Thu, 17 Dec 2015 12:04:48 -0500 (EST)
David Miller  wrote:

> From: Johannes Berg 
> Date: Thu, 17 Dec 2015 13:44:32 +0100
> 
> > On Wed, 2015-12-16 at 18:34 -0500, David Miller wrote:
> >> 
> >> Something about your text encoding kept this from ending up
> >> in patchwork for some reason.
> >> 
> > 
> > Hm. I don't see anything special with this, seems to just be plain
> > text 8bit transfer encoding.
> > 
> > Do you want me to watch out for things getting into patchwork in the
> > future?
> 
> Look in the quoted text of mine, see that underline thing after
> the ">>"?  Where are those coming from?  Those were all over the
> place in your pull request and tripped up patchwork's parser I
> guess.

I was curious and took a look. I suspect what you're seeing are the UTF-8 
<0xC2 0xA0> sequence, which translates to codepoint 0x00A0 "no-break space"
(same as in latin1). They seem to have been used in place of regular spaces 
for the purpose of indenting. The Content-Type charset in Johannes' emails 
is "UTF-8", so I think that's legal. Although I have no idea how Patchwork 
reacts to it. 


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

Re: [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY

2015-12-16 Thread David Rivshin (Allworx) 
On Wed, 16 Dec 2015 07:39:16 +0100
Markus Brunner  wrote:

> On Monday 14 December 2015 13:04:46 David Rivshin wrote:
> > On Sat, 12 Dec 2015 16:44:19 +0100
> ...
> > > Your patch works fine on my board, which uses MII and dual_emac
> > > with a fixed_phy and a real one.
> > 
> > Thanks for checking. The only dual_emac board I have available is
> > the EVMSK, which has two real PHYs. I'm not sure of the usual
> > etiquette (and Google was  unhelpful), should I add a Tested-by on
> > the next version?
> > 
> Yes you can. Documentation/SubmittingPatches has some notes about it.

Thanks, I didn't want to throw it on without permission. Although due 
to the non-trivial change I mention below, I figured that the previous 
testing wasn't totally valid anymore anyways, so I left it off the v2 
emails.

> > > I wanted to keep changes small and didn't spend too much thinking
> > > about already broken devicetrees. Since my patch is quite new, I
> > 
> > I'm honestly not sure it's an important consideration myself. Most
> > patches I've seen in this area for this or other drivers do not take
> > such behavior into account (e.g. the phy-handle parsing that went in
> > to cpsw in 4.3).
> > I would generally feel more comfortable with such a behavior tweak
> > (minor as it is) before 4.4 is released, to avoid ping-ponging the
> > behavior. But given how far along the cycle is, I'm not sure about
> > the chances of that.
> 
> Well I don't think compatibility for flawed DTs is such a high
> priority, especially if it is that unlikely that there are some
> affected. Keep the focus on the other _real_ problems you have
> encountered and fix those like you see fit. 

Since there's been no indication from anyone that being nice to such
broken DTs is desired, I decided to drop that aspect of the patch and 
leave the current 4.4-rc1..5 behavior. This also made it much more 
reasonable to chop up the patch into smaller pieces, which I think will 
be easier to review.

> > > don't see any problems with subtle changes like that. However you
> > > should update the documentation as well.
> > 
> > Your patch already updated .../bindings/net/cpsw.txt, which this
> > patch left alone. Are you referring to some other documentation,
> > or do you think I should update the binding documentation to state
> > that phy_id takes precedence over fixed-link? I figured that such
> > devicetrees were still officially malformed, so I thought the
> > existing text was appropriate.
> 
> "Either the properties phy_id and phy-mode, or the sub-node
> fixed-link can be specified" One flaw of my patch was to ignore the
> phy-mode for a fixed link. Do not mention the precedence of the
> phy_id, because it is an undefined behavior. Your patch should change
> it to: "Either the property phy_id, or the sub-node fixed-link can be
> specified"

Thanks for pointing that out. For some reason my brain skipped over the 
"and phy-mode" part. Fixed in v2.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] ethernet:ti:cpsw: fix phy identification with multiple slaves on fixed-phy

2015-12-16 Thread David Rivshin (Allworx) 
From: Pascal Speck (Iktek) 
Date: Fri, 04 Dec 2015 16:55:17 +0100

When using more than one slave with ti cpsw and fixed phy the pd->phy_id
will be always zero, but slave_data->phy_id must be unique. pd->phy_id
means a "phy hardware id" whereas slave_data->phy_id means an "unique id",
so we should use pd->addr which has the same unique meaning.

Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link PHY")
Signed-off-by: Pascal Speck 
---
This was originally submitted by Pascal Speck on December 4, but was not
picked up by patchwork. I suspect that is because the patch was mangled by
the mailer. The only changes I made were to manually fix the patch whitespace
and wrapping, and add the Fixes: tag.

 drivers/net/ethernet/ti/cpsw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 48b92c9..e3b220d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2047,7 +2047,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
if (!pd)
return -ENODEV;
snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-PHY_ID_FMT, pd->bus->id, pd->phy_id);
+PHY_ID_FMT, pd->bus->id, pd->addr);
goto no_phy_slave;
}
parp = of_get_property(slave_node, "phy_id", );
--
2.5.0

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

[PATCH v2 0/3] drivers: net: cpsw: Fix bugs in fixed-link PHY DT parsing

2015-12-16 Thread David Rivshin (Allworx) 
Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw:
Add support for fixed-link PHY") added initial fixed-link PHY support
for CPSW, but missed a few considerations.

This series is based on the tip of the net tree. The first two patches
fix user-visible errors in different hardware configurations. The third
patch is for an internal reference counting issue. They are logically
independent changes, but in the same function, so must be applied in
order to apply cleanly.

The first patch was originally submitted by Pascal Speck on December 4,
but was not picked up by patchwork. I suspect that is because the patch
was mangled by the mailer. I fixed the mangling and am including it in
this series, as I believe it is the correct change.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac with two real MDIO-connected phys using RGMII-TXID
 - single emac with fixed-link using RGMII
Testing of other CPSW emac configurations that folks may have would
be appreciated.


Changes from v1 [1]:
 - Split into 3 smaller patches.
 - Maintain 1f71e8c96fc6's preference for fixed-link over phy_id if
   they are both (incorrectly) specified in the slave node.
 - Update binding documentation to no longer say that phy_mode is also
   mutually exclusive with fixed-link.
 - Dropped unnecessary include of phy_fixed.h.

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

David Rivshin (2):
  drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY
  drivers: net: cpsw: increment reference count on fixed-link PHY node

Pascal Speck (Iktek) (1):
  ethernet:ti:cpsw: fix phy identification with multiple slaves on
fixed-phy

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

--
2.5.0

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

[PATCH v2 2/3] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY

2015-12-16 Thread David Rivshin (Allworx) 
From: David Rivshin 

Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw: Add
support for fixed-link PHY") did not parse the "phy-mode" property in
the case of a fixed-link PHY, leaving slave_data->phy_if with its default
of PHY_INTERFACE_MODE_NA(0). This later gets passed to phy_connect() in
cpsw_slave_open(), and eventually to cpsw_phy_sel() where it hits a default
case that configures the MAC for MII mode.

The user visible symptom is that while kernel log messages seem to indicate
that the interface is set up, there is no network communication. Eventually
a watchdog error occurs:
NETDEV WATCHDOG: eth0 (cpsw): transmit queue 0 timed out

Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link PHY")
Signed-off-by: David Rivshin 
---
Changes from v1 (https://patchwork.ozlabs.org/patch/554989/):
 - Maintain 1f71e8c96fc6's preference for fixed-link over phy_id if they
   are both (incorrectly) specified in the slave node.
 - Update binding documentation to no longer say that phy_mode is also
   mutually exclusive with fixed-link.
 - Dropped unnecessary include of phy_fixed.h.
 - Commit message tweaked.

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

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 9853f8e..28a4781 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -40,18 +40,18 @@ Optional properties:

 Slave Properties:
 Required properties:
-- phy_id   : Specifies slave phy id
 - phy-mode : See ethernet.txt file in the same directory

 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 properties phy_id and phy-mode,
- or the sub-node fixed-link can be specified
+ Either the property phy_id, or the sub-node
+ fixed-link can 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.
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index e3b220d..bc6d20d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2026,17 +2026,15 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
for_each_child_of_node(node, slave_node) {
struct cpsw_slave_data *slave_data = data->slave_data + i;
const void *mac_addr = NULL;
-   u32 phyid;
int lenp;
const __be32 *parp;
-   struct device_node *mdio_node;
-   struct platform_device *mdio;

/* 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);
+   parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
struct phy_device *pd;

@@ -2048,23 +2046,29 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
return -ENODEV;
snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
 PHY_ID_FMT, pd->bus->id, pd->addr);
+   } else if (parp) {
+   u32 phyid;
+   struct device_node *mdio_node;
+   struct platform_device *mdio;
+
+   if (lenp != (sizeof(__be32) * 2)) {
+   dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
+   goto no_phy_slave;
+   }
+   mdio_node = of_find_node_by_phandle(be32_to_cpup(parp));
+   phyid = be32_to_cpup(parp+1);
+   mdio = of_find_device_by_node(mdio_node);
+   of_node_put(mdio_node);
+   if (!mdio) {
+   dev_err(>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(>dev, "No slave[%d] phy_id or fixed-link

[PATCH v2 3/3] drivers: net: cpsw: increment reference count on fixed-link PHY node

2015-12-16 Thread David Rivshin (Allworx) 
From: David Rivshin 

When a fixed-link sub-node exists in a slave node, the slave node
is also the PHY node. Since this is a separate use of the slave node,
of_node_get() should be used to increment the reference count.

Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link PHY")
Signed-off-by: David Rivshin 
---
'pd' was renamed to 'phy_dev' to better fit the naming convention in the
function/file.

 drivers/net/ethernet/ti/cpsw.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index bc6d20d..3b489ca 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2036,16 +2036,21 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
-   struct phy_device *pd;
+   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;
-   pd = of_phy_find_device(slave_node);
-   if (!pd)
+   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, pd->bus->id, pd->addr);
+PHY_ID_FMT, phy_dev->bus->id, phy_dev->addr);
} else if (parp) {
u32 phyid;
struct device_node *mdio_node;
--
2.5.0

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

Re: [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY

2015-12-14 Thread David Rivshin (Allworx) 
On Sat, 12 Dec 2015 16:44:19 +0100
Markus Brunner  wrote:

> On Wednesday 09 December 2015 22:31:15 David Rivshin wrote:
> ...
> > This patch was originally developed in parallel with 1f71e8c96fc6 to
> > accomplish the same goal. When I replaced this patch with
> > 1f71e8c96fc6, I found that it did not work on my hardware (which
> > uses RGMII), so I am submitting this patch now as a bugfix. 
> 
> Your patch works fine on my board, which uses MII and dual_emac with
> a fixed_phy and a real one. 

Thanks for checking. The only dual_emac board I have available is the 
EVMSK, which has two real PHYs. I'm not sure of the usual etiquette 
(and Google was  unhelpful), should I add a Tested-by on the next 
version?

> > Besides fixing the bug mentioned in the commit log, there are a few
> > other differences to note:
> >  * If both "phy_id" and a "fixed-link" subnode are present this
> > patch will use the "phy_id" property. This should preserve current
> > behavior with existing devicetrees that might incorrectly have
> > both. This motivates the biggest difference in code organization
> > from 1f71e8c96fc6.
> >  * Some error messages have been tweaked to reflect the slightly
> > changed meanings of the checks.
> 
> I wanted to keep changes small and didn't spend too much thinking
> about already broken devicetrees. Since my patch is quite new, I

I'm honestly not sure it's an important consideration myself. Most
patches I've seen in this area for this or other drivers do not take 
such behavior into account (e.g. the phy-handle parsing that went in 
to cpsw in 4.3).
I would generally feel more comfortable with such a behavior tweak
(minor as it is) before 4.4 is released, to avoid ping-ponging the
behavior. But given how far along the cycle is, I'm not sure about 
the chances of that.

> don't see any problems with subtle changes like that. However you
> should update the documentation as well. 

Your patch already updated .../bindings/net/cpsw.txt, which this
patch left alone. Are you referring to some other documentation, 
or do you think I should update the binding documentation to state
that phy_id takes precedence over fixed-link? I figured that such
devicetrees were still officially malformed, so I thought the 
existing text was appropriate.

> >  * of_node_get() is called on slave_node before passing the result
> > to of_phy_find_device(). This increments the reference count, which
> > I believe is correct for this situation, but I'm not certain of
> > that.
> I think you are right. At least most other drivers calling
> of_phy_register_fixed_link(), call of_node_get afterwards. I can't
> remember where I got my "inspiration" for the patch. I made it almost
> a year ago and now 3 other guys tinker with the same feature? What a
> coincidence ;-)

Perhaps not a complete coincidence. I first wrote this patch a few 
months ago against 4.1. While I had always intended on submitting when 
time allowed, testing your patch gave me an extra push.

[...]

> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> The prototypes for of_*_fixed_link are in linux/of_mdio.h, which is
> already included.

You are correct; I forget why I had originally included that. I will 
remove it for v2.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY

2015-12-09 Thread David Rivshin (Allworx) 
Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw: Add
support for fixed-link PHY") used a 'goto no_phy_slave' to skip over the
processing of the mutually-exclusive "phy_id" property. Unfortunately that
also skipped the "phy-mode" property processing, leaving slave_data->phy_if
with its default of PHY_INTERFACE_MODE_NA(0). This later gets passed to
phy_connect() in cpsw_slave_open(), and eventually to cpsw_phy_sel() where
it hits a default case that configures the MAC for MII mode.

The user visible symptom is that while kernel log messages seem to indicate
that the interface is set up, there is no network communication. Eventually
a watchdog error occurs:
NETDEV WATCHDOG: eth0 (cpsw): transmit queue 0 timed out

Signed-off-by: David Rivshin (Allworx) <drivshin.allw...@gmail.com>
Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link PHY")
---

This patch was originally developed in parallel with 1f71e8c96fc6 to
accomplish the same goal. When I replaced this patch with 1f71e8c96fc6,
I found that it did not work on my hardware (which uses RGMII), so I
am submitting this patch now as a bugfix. I have rebased this to the
current head of the net tree, but because of the different order of the
logic (see explanation below) it ends up replacing 1f71e8c96fc6's changes
in cpsw.c. If a minimal patch is preferred, I can do that instead.

Besides fixing the bug mentioned in the commit log, there are a few other
differences to note:
 * If both "phy_id" and a "fixed-link" subnode are present this patch will
   use the "phy_id" property. This should preserve current behavior with
   existing devicetrees that might incorrectly have both. This motivates
   the biggest difference in code organization from 1f71e8c96fc6.
 * Some error messages have been tweaked to reflect the slightly changed
   meanings of the checks.
 * of_node_get() is called on slave_node before passing the result to
   of_phy_find_device(). This increments the reference count, which I
   believe is correct for this situation, but I'm not certain of that.
 * Uses the phy_device's 'addr' instead of 'phy_id' field when constructing
   slave_data->phy_id. Pascal Speck separately came to the same conclusion
   in another patch [1].
 * [2] pointed out that the 'lenp' sanity check was wrong, and since this
   patch re-arranged that check anyways I incorporated that fix as well.
Although the last three items could be fixed separately, it seemed like that
would be unnecessary churn since those same lines were already touched in
this patch. Let me know if its preferred to fix them separately.

This patch is also very similar to one Daniel Trautmann submitted [3], with
the biggest difference being that Daniel's patch uses the new priv->phy_node
field and of_node_get(). While this seems like a better path overall it
does not work if more than once CPSW slave is used, due to struct cpsw_priv
being shared with all the slaves. I am under the impression that phy_node
should really be in struct cpsw_slave instead, but I will leave that for
another patch.

Checkpatch does flag 3 issues I've decided to leave, but I'd be happy to
resolve them if desired:
WARNING: line over 80 characters (cpsw.c:2046):
+   dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
WARNING: line over 80 characters (cpsw.c:2077):
+   dev_err(>dev, "No slave[%d] phy_id or fixed-link 
property\n", i);
Both modifications of the same pre-existing messages that was already
over 80 characters. Seemed more readable to leave them as 1-liners.
CHECK: spaces preferred around that '+' (ctx:VxV) (cpsw.c:2051):
+   phyid = be32_to_cpup(parp+1);
ALso pre-existing, and far enough from the purpose of this patch that it
seemed gratuitous to change now.

[1] http://www.spinics.net/lists/netdev/msg355254.html
[2] http://www.spinics.net/lists/netdev/msg351703.html
[3] http://www.spinics.net/lists/netdev/msg351674.html

(Side note: This is my first patch submission, and any feedback on things 
to do differently in the future would be appreciated.)

 drivers/net/ethernet/ti/cpsw.c | 65 +-
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 48b92c9..ba8d086 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2026,45 +2027,57 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
for_each_child_of_node(node, slave_node) {
struct cpsw_slave_data *slave_data = data->slave_data + i;
const void *mac_addr = NULL;
-   u32 phyid;
int lenp;