[PATCH 0/8] net: stmmac: misc patchs

[PATCH 0/8] net: stmmac: misc patchs

[Corentin Labbe]

Hello

This is a follow up of my previous stmmac serie which address some comment
done in v2.

Corentin Labbe (8):
  net: stmmac: remove useless parenthesis
  net: stmmac: likely is useless in occasional function
  net: stmmac: use SPEED_UNKNOWN/DUPLEX_UNKNOWN
  net: stmmac: set speed at SPEED_UNKNOWN in case of broken speed
  net: stmmac: run stmmac_hw_fix_mac_speed when speed is valid
  net: stmmac: split the stmmac_adjust_link 10/100 case
  net: stmmac: reduce indentation by adding a continue
  net: stmmac: invert the logic for dumping regs

 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 18 ++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 40 ++++++-----
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  | 82 +++++++++++-----------
 3 files changed, 71 insertions(+), 69 deletions(-)

-- 
2.10.2

[PATCH 1/8] net: stmmac: remove useless parenthesis

[Corentin Labbe]

This patch remove some useless parenthesis.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7251871..ee1dbf4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -716,15 +716,15 @@ static void stmmac_adjust_link(struct net_device *dev)
 			new_state = 1;
 			switch (phydev->speed) {
 			case 1000:
-				if (likely((priv->plat->has_gmac) ||
-					   (priv->plat->has_gmac4)))
+				if (likely(priv->plat->has_gmac ||
+					   priv->plat->has_gmac4))
 					ctrl &= ~priv->hw->link.port;
 				stmmac_hw_fix_mac_speed(priv);
 				break;
 			case 100:
 			case 10:
-				if (likely((priv->plat->has_gmac) ||
-					   (priv->plat->has_gmac4))) {
+				if (likely(priv->plat->has_gmac ||
+					   priv->plat->has_gmac4)) {
 					ctrl |= priv->hw->link.port;
 					if (phydev->speed == SPEED_100) {
 						ctrl |= priv->hw->link.speed;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index d9893cf..a695773 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -257,7 +257,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 			 * If an IRQ was provided to be assigned after
 			 * the bus probe, do it here.
 			 */
-			if ((!mdio_bus_data->irqs) &&
+			if (!mdio_bus_data->irqs &&
 			    (mdio_bus_data->probed_phy_irq > 0)) {
 				new_bus->irq[addr] =
 					mdio_bus_data->probed_phy_irq;
-- 
2.10.2

[PATCH 2/8] net: stmmac: likely is useless in occasional function

[Corentin Labbe]

The stmmac_adjust_link() function is called too rarely for having
likely() macros being useful.
Just remove likely annotation in it.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ee1dbf4..511c47c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -716,15 +716,15 @@ static void stmmac_adjust_link(struct net_device *dev)
 			new_state = 1;
 			switch (phydev->speed) {
 			case 1000:
-				if (likely(priv->plat->has_gmac ||
-					   priv->plat->has_gmac4))
+				if (priv->plat->has_gmac ||
+				    priv->plat->has_gmac4)
 					ctrl &= ~priv->hw->link.port;
 				stmmac_hw_fix_mac_speed(priv);
 				break;
 			case 100:
 			case 10:
-				if (likely(priv->plat->has_gmac ||
-					   priv->plat->has_gmac4)) {
+				if (priv->plat->has_gmac ||
+				    priv->plat->has_gmac4) {
 					ctrl |= priv->hw->link.port;
 					if (phydev->speed == SPEED_100) {
 						ctrl |= priv->hw->link.speed;
-- 
2.10.2

[PATCH 3/8] net: stmmac: use SPEED_UNKNOWN/DUPLEX_UNKNOWN

[Corentin Labbe]

It is better to use DUPLEX_UNKNOWN instead of just "-1".
Using 0 for an invalid speed is bad since 0 is a valid value for speed.
So this patch replace 0 by SPEED_UNKNOWN.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 511c47c..a87071d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -754,8 +754,8 @@ static void stmmac_adjust_link(struct net_device *dev)
 	} else if (priv->oldlink) {
 		new_state = 1;
 		priv->oldlink = 0;
-		priv->speed = 0;
-		priv->oldduplex = -1;
+		priv->speed = SPEED_UNKNOWN;
+		priv->oldduplex = DUPLEX_UNKNOWN;
 	}
 
 	if (new_state && netif_msg_link(priv))
@@ -817,8 +817,8 @@ static int stmmac_init_phy(struct net_device *dev)
 	int interface = priv->plat->interface;
 	int max_speed = priv->plat->max_speed;
 	priv->oldlink = 0;
-	priv->speed = 0;
-	priv->oldduplex = -1;
+	priv->speed = SPEED_UNKNOWN;
+	priv->oldduplex = DUPLEX_UNKNOWN;
 
 	if (priv->plat->phy_node) {
 		phydev = of_phy_connect(dev, priv->plat->phy_node,
@@ -3434,8 +3434,8 @@ int stmmac_suspend(struct device *dev)
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	priv->oldlink = 0;
-	priv->speed = 0;
-	priv->oldduplex = -1;
+	priv->speed = SPEED_UNKNOWN;
+	priv->oldduplex = DUPLEX_UNKNOWN;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(stmmac_suspend);
-- 
2.10.2

[PATCH 4/8] net: stmmac: set speed at SPEED_UNKNOWN in case of broken speed

[Corentin Labbe]

In case of invalid speed given, stmmac_adjust_link() still record it as
current speed.
This patch modify the default case to set speed as SPEED_UNKNOWN if not
10/100/1000.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a87071d..f7664b9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -739,6 +739,7 @@ static void stmmac_adjust_link(struct net_device *dev)
 			default:
 				netif_warn(priv, link, priv->dev,
 					   "broken speed: %d\n", phydev->speed);
+				phydev->speed = SPEED_UNKNOWN;
 				break;
 			}
 
-- 
2.10.2

[PATCH 5/8] net: stmmac: run stmmac_hw_fix_mac_speed when speed is valid

[Corentin Labbe]

This patch mutualise a bit by running stmmac_hw_fix_mac_speed() after
the switch in case of valid speed.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f7664b9..bebe810 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -719,7 +719,6 @@ static void stmmac_adjust_link(struct net_device *dev)
 				if (priv->plat->has_gmac ||
 				    priv->plat->has_gmac4)
 					ctrl &= ~priv->hw->link.port;
-				stmmac_hw_fix_mac_speed(priv);
 				break;
 			case 100:
 			case 10:
@@ -734,7 +733,6 @@ static void stmmac_adjust_link(struct net_device *dev)
 				} else {
 					ctrl &= ~priv->hw->link.port;
 				}
-				stmmac_hw_fix_mac_speed(priv);
 				break;
 			default:
 				netif_warn(priv, link, priv->dev,
@@ -742,7 +740,8 @@ static void stmmac_adjust_link(struct net_device *dev)
 				phydev->speed = SPEED_UNKNOWN;
 				break;
 			}
-
+			if (phydev->speed != SPEED_UNKNOWN)
+				stmmac_hw_fix_mac_speed(priv);
 			priv->speed = phydev->speed;
 		}
 
-- 
2.10.2

[PATCH 6/8] net: stmmac: split the stmmac_adjust_link 10/100 case

[Corentin Labbe]

The 10/100 case have too many ifcase.
This patch split it for removing an if.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bebe810..3cbe096 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -721,15 +721,19 @@ static void stmmac_adjust_link(struct net_device *dev)
 					ctrl &= ~priv->hw->link.port;
 				break;
 			case 100:
+				if (priv->plat->has_gmac ||
+				    priv->plat->has_gmac4) {
+					ctrl |= priv->hw->link.port;
+					ctrl |= priv->hw->link.speed;
+				} else {
+					ctrl &= ~priv->hw->link.port;
+				}
+				break;
 			case 10:
 				if (priv->plat->has_gmac ||
 				    priv->plat->has_gmac4) {
 					ctrl |= priv->hw->link.port;
-					if (phydev->speed == SPEED_100) {
-						ctrl |= priv->hw->link.speed;
-					} else {
-						ctrl &= ~(priv->hw->link.speed);
-					}
+					ctrl &= ~(priv->hw->link.speed);
 				} else {
 					ctrl &= ~priv->hw->link.port;
 				}
-- 
2.10.2

[PATCH 7/8] net: stmmac: reduce indentation by adding a continue

[Corentin Labbe]

As suggested by Joe Perches, replacing the "if phydev" logic permit to
reduce indentation in the for loop.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 82 +++++++++++------------
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a695773..db157a4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -247,50 +247,48 @@ int stmmac_mdio_register(struct net_device *ndev)
 	found = 0;
 	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
 		struct phy_device *phydev = mdiobus_get_phy(new_bus, addr);
+		int act = 0;
+		char irq_num[4];
+		char *irq_str;
+
+		if (!phydev)
+			continue;
+
+		/*
+		 * If an IRQ was provided to be assigned after
+		 * the bus probe, do it here.
+		 */
+		if (!mdio_bus_data->irqs &&
+		    (mdio_bus_data->probed_phy_irq > 0)) {
+			new_bus->irq[addr] = mdio_bus_data->probed_phy_irq;
+			phydev->irq = mdio_bus_data->probed_phy_irq;
+		}
 
-		if (phydev) {
-			int act = 0;
-			char irq_num[4];
-			char *irq_str;
-
-			/*
-			 * If an IRQ was provided to be assigned after
-			 * the bus probe, do it here.
-			 */
-			if (!mdio_bus_data->irqs &&
-			    (mdio_bus_data->probed_phy_irq > 0)) {
-				new_bus->irq[addr] =
-					mdio_bus_data->probed_phy_irq;
-				phydev->irq = mdio_bus_data->probed_phy_irq;
-			}
-
-			/*
-			 * If we're going to bind the MAC to this PHY bus,
-			 * and no PHY number was provided to the MAC,
-			 * use the one probed here.
-			 */
-			if (priv->plat->phy_addr == -1)
-				priv->plat->phy_addr = addr;
-
-			act = (priv->plat->phy_addr == addr);
-			switch (phydev->irq) {
-			case PHY_POLL:
-				irq_str = "POLL";
-				break;
-			case PHY_IGNORE_INTERRUPT:
-				irq_str = "IGNORE";
-				break;
-			default:
-				sprintf(irq_num, "%d", phydev->irq);
-				irq_str = irq_num;
-				break;
-			}
-			netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
-				    phydev->phy_id, addr,
-				    irq_str, phydev_name(phydev),
-				    act ? " active" : "");
-			found = 1;
+		/*
+		 * If we're going to bind the MAC to this PHY bus,
+		 * and no PHY number was provided to the MAC,
+		 * use the one probed here.
+		 */
+		if (priv->plat->phy_addr == -1)
+			priv->plat->phy_addr = addr;
+
+		act = (priv->plat->phy_addr == addr);
+		switch (phydev->irq) {
+		case PHY_POLL:
+			irq_str = "POLL";
+			break;
+		case PHY_IGNORE_INTERRUPT:
+			irq_str = "IGNORE";
+			break;
+		default:
+			sprintf(irq_num, "%d", phydev->irq);
+			irq_str = irq_num;
+			break;
 		}
+		netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
+			    phydev->phy_id, addr, irq_str, phydev_name(phydev),
+			    act ? " active" : "");
+		found = 1;
 	}
 
 	if (!found && !mdio_node) {
-- 
2.10.2

[PATCH 8/8] net: stmmac: invert the logic for dumping regs

[Corentin Labbe]

It is easier to follow the logic by removing the not operator

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index aab895d..5ff6bc4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -442,24 +442,24 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
 
 	memset(reg_space, 0x0, REG_SPACE_SIZE);
 
-	if (!(priv->plat->has_gmac || priv->plat->has_gmac4)) {
+	if (priv->plat->has_gmac || priv->plat->has_gmac4) {
 		/* MAC registers */
-		for (i = 0; i < 12; i++)
+		for (i = 0; i < 55; i++)
 			reg_space[i] = readl(priv->ioaddr + (i * 4));
 		/* DMA registers */
-		for (i = 0; i < 9; i++)
-			reg_space[i + 12] =
+		for (i = 0; i < 22; i++)
+			reg_space[i + 55] =
 			    readl(priv->ioaddr + (DMA_BUS_MODE + (i * 4)));
-		reg_space[22] = readl(priv->ioaddr + DMA_CUR_TX_BUF_ADDR);
-		reg_space[23] = readl(priv->ioaddr + DMA_CUR_RX_BUF_ADDR);
 	} else {
 		/* MAC registers */
-		for (i = 0; i < 55; i++)
+		for (i = 0; i < 12; i++)
 			reg_space[i] = readl(priv->ioaddr + (i * 4));
 		/* DMA registers */
-		for (i = 0; i < 22; i++)
-			reg_space[i + 55] =
+		for (i = 0; i < 9; i++)
+			reg_space[i + 12] =
 			    readl(priv->ioaddr + (DMA_BUS_MODE + (i * 4)));
+		reg_space[22] = readl(priv->ioaddr + DMA_CUR_TX_BUF_ADDR);
+		reg_space[23] = readl(priv->ioaddr + DMA_CUR_RX_BUF_ADDR);
 	}
 }
 
-- 
2.10.2

Re: [PATCH 0/8] net: stmmac: misc patchs

[Giuseppe CAVALLARO]

Hello Corentin

On 2/14/2017 8:54 PM, Corentin Labbe wrote:
> Hello
>
> This is a follow up of my previous stmmac serie which address some comment
> done in v2.

I wonder if you can resend all the patches as V3
please add the Acked-by and Reviewed-by. This will help
on final review and integration.

Thx for your support and effort

Peppe

>
> Corentin Labbe (8):
>   net: stmmac: remove useless parenthesis
>   net: stmmac: likely is useless in occasional function
>   net: stmmac: use SPEED_UNKNOWN/DUPLEX_UNKNOWN
>   net: stmmac: set speed at SPEED_UNKNOWN in case of broken speed
>   net: stmmac: run stmmac_hw_fix_mac_speed when speed is valid
>   net: stmmac: split the stmmac_adjust_link 10/100 case
>   net: stmmac: reduce indentation by adding a continue
>   net: stmmac: invert the logic for dumping regs
>
>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 18 ++---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 40 ++++++-----
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  | 82 +++++++++++-----------
>  3 files changed, 71 insertions(+), 69 deletions(-)
>

Re: [PATCH 0/8] net: stmmac: misc patchs

[Corentin Labbe]

On Wed, Feb 15, 2017 at 07:34:41AM +0100, Giuseppe CAVALLARO wrote:
> Hello Corentin
> 
> On 2/14/2017 8:54 PM, Corentin Labbe wrote:
> > Hello
> >
> > This is a follow up of my previous stmmac serie which address some comment
> > done in v2.
> 
> I wonder if you can resend all the patches as V3
> please add the Acked-by and Reviewed-by. This will help
> on final review and integration.
> 
> Thx for your support and effort
> 
> Peppe
> 

Hello

Since all patch in v2 already hit linux-next, I just want to be sure, I can add "Acked-by and Reviewed-by" to thoses 8 new patchs ?

Regards

> >
> > Corentin Labbe (8):
> >   net: stmmac: remove useless parenthesis
> >   net: stmmac: likely is useless in occasional function
> >   net: stmmac: use SPEED_UNKNOWN/DUPLEX_UNKNOWN
> >   net: stmmac: set speed at SPEED_UNKNOWN in case of broken speed
> >   net: stmmac: run stmmac_hw_fix_mac_speed when speed is valid
> >   net: stmmac: split the stmmac_adjust_link 10/100 case
> >   net: stmmac: reduce indentation by adding a continue
> >   net: stmmac: invert the logic for dumping regs
> >
> >  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 18 ++---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 40 ++++++-----
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  | 82 +++++++++++-----------
> >  3 files changed, 71 insertions(+), 69 deletions(-)
> >
> 

Re: [PATCH 0/8] net: stmmac: misc patchs

[Giuseppe CAVALLARO]

On 2/15/2017 8:39 AM, Corentin Labbe wrote:
> Since all patch in v2 already hit linux-next, I just want to be sure, I can add "Acked-by and Reviewed-by" to thoses 8 new patchs ?

Right I have just seen the V2 patches are in net-next.

pls consider, for this set, my

Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>


Regards
Peppe

>
> Regards
>
>>>
>>> Corentin Labbe (8):
>>>   net: stmmac: remove useless parenthesis
>>>   net: stmmac: likely is useless in occasional function
>>>   net: stmmac: use SPEED_UNKNOWN/DUPLEX_UNKNOWN
>>>   net: stmmac: set speed at SPEED_UNKNOWN in case of broken speed
>>>   net: stmmac: run stmmac_hw_fix_mac_speed when speed is valid
>>>   net: stmmac: split the stmmac_adjust_link 10/100 case
>>>   net: stmmac: reduce indentation by adding a continue
>>>   net: stmmac: invert the logic for dumping regs
>>>
>>>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 18 ++---
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 40 ++++++-----
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  | 82 +++++++++++-----------
>>>  3 files changed, 71 insertions(+), 69 deletions(-)
>>>
>>
>