[PATCH net-next V2 00/16] net: fec: cleanup and fixes

[PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Troy Kisky]

V2 is a rebase on top of johannes endian-safe patch and
this set is only the next 16 patches.
The testing for this series was done on a nitrogen6x.
The base commit was
commit f5461c27631672b9e95282812ee521c53f502eca
    Merge branch 'dsa-pass-bridge-to-drivers'

Testing showed no change in performance.
Testing used imx_v6_v7_defconfig + CONFIG_MICREL_PHY.
The processor was running at 996Mhz.
The following commands were used to get the transfer rates.

On an x86 ubunto system,
iperf -s -i.5 -u


On a nitrogen6x board, running via SD Card.
I first stopped some background processes

stop cron
stop upstart-file-bridge
stop upstart-socket-bridge
stop upstart-udev-bridge
stop rsyslog
stop dbus
killall dhclient
echo performance >/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

taskset 0x2 iperf -c 192.168.0.201 -u -t60 -b500M -r

There is a branch available on github with this series, and the rest of
my fec patches, for those who would like to test it.
https://github.com:boundarydevices/linux-imx6.git branch net-next_master



Troy Kisky (16):
SDCard
 TX/RX
379/402	Before any patches
376/402  net: fec: only check queue 0 if RXF_0/TXF_0 interrupt is set
379/399  net: fec: pass rxq to fec_enet_rx_queue instead of queue_id
379/401  net: fec: pass txq to fec_enet_tx_queue instead of queue_id
389/409  net: fec: reduce interrupts
393/411  net: fec: split off napi routine with 3 queues
386/415  net: fec: don't clear all rx queue bits when just one is being checked
387/412  net: fec: set cbd_sc without relying on previous value
385/415  net: fec: eliminate calls to fec_enet_get_prevdesc
389/414  net: fec: move restart test for efficiency
386/412  net: fec: clear cbd_sc after transmission to help with debugging
387/415  net: fec: dump all tx queues in fec_dump
380/418  net: fec: detect tx int lost
385/417  net: fec: print more debug info in fec_timeout
388/417  net: fec: create subroutine reset_tx_queue
389/418  net: fec: call dma_unmap_single on mapped tx buffers at restart
381/414  net: fec: don't set cbd_bufaddr unless no mapping error

 drivers/net/ethernet/freescale/fec.h      |   7 +-
 drivers/net/ethernet/freescale/fec_main.c | 420 ++++++++++++++++--------------
 2 files changed, 224 insertions(+), 203 deletions(-)

-- 
2.5.0

[PATCH net-next V2 01/16] net: fec: only check queue 0 if RXF_0/TXF_0 interrupt is set

[Troy Kisky]

Before queue 0 was always checked if any queue caused an interrupt.
It is better to just mark queue 0 if queue 0 has caused an interrupt.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index bad0ba2..dbac975 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1534,14 +1534,14 @@ fec_enet_collect_events(struct fec_enet_private *fep, uint int_events)
 	if (int_events == 0)
 		return false;
 
-	if (int_events & FEC_ENET_RXF)
+	if (int_events & FEC_ENET_RXF_0)
 		fep->work_rx |= (1 << 2);
 	if (int_events & FEC_ENET_RXF_1)
 		fep->work_rx |= (1 << 0);
 	if (int_events & FEC_ENET_RXF_2)
 		fep->work_rx |= (1 << 1);
 
-	if (int_events & FEC_ENET_TXF)
+	if (int_events & FEC_ENET_TXF_0)
 		fep->work_tx |= (1 << 2);
 	if (int_events & FEC_ENET_TXF_1)
 		fep->work_tx |= (1 << 0);
-- 
2.5.0

RE: [PATCH net-next V2 01/16] net: fec: only check queue 0 if RXF_0/TXF_0 interrupt is set

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 01/16] net: fec: only check queue 0 if
> RXF_0/TXF_0 interrupt is set
> 
> Before queue 0 was always checked if any queue caused an interrupt.
> It is better to just mark queue 0 if queue 0 has caused an interrupt.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index bad0ba2..dbac975 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1534,14 +1534,14 @@ fec_enet_collect_events(struct fec_enet_private
> *fep, uint int_events)
>  	if (int_events == 0)
>  		return false;
> 
> -	if (int_events & FEC_ENET_RXF)
> +	if (int_events & FEC_ENET_RXF_0)
>  		fep->work_rx |= (1 << 2);
>  	if (int_events & FEC_ENET_RXF_1)
>  		fep->work_rx |= (1 << 0);
>  	if (int_events & FEC_ENET_RXF_2)
>  		fep->work_rx |= (1 << 1);
> 
> -	if (int_events & FEC_ENET_TXF)
> +	if (int_events & FEC_ENET_TXF_0)
>  		fep->work_tx |= (1 << 2);
>  	if (int_events & FEC_ENET_TXF_1)
>  		fep->work_tx |= (1 << 0);
> --
> 2.5.0

Acked-by: Fugang Duan <Fugang.duan@nxp.com>

[PATCH net-next V2 02/16] net: fec: pass rxq to fec_enet_rx_queue instead of queue_id

[Troy Kisky]

The queue_id is the qid member of struct bufdesc_prop.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index dbac975..9619b9e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1328,11 +1328,9 @@ static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb,
  * not been given to the system, we just set the empty indicator,
  * effectively tossing the packet.
  */
-static int
-fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
+static int fec_rxq(struct net_device *ndev, struct fec_enet_private *fep,
+		   struct fec_enet_priv_rx_q *rxq, int budget)
 {
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct fec_enet_priv_rx_q *rxq;
 	struct bufdesc *bdp;
 	unsigned short status;
 	struct  sk_buff *skb_new = NULL;
@@ -1350,8 +1348,6 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 #ifdef CONFIG_M532x
 	flush_cache_all();
 #endif
-	queue_id = FEC_ENET_GET_QUQUE(queue_id);
-	rxq = fep->rx_queue[queue_id];
 
 	/* First, grab all of the stats for the incoming packet.
 	 * These get messed up if we get called due to a busy condition.
@@ -1519,11 +1515,12 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	int     pkt_received = 0;
 	u16	queue_id;
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct fec_enet_priv_rx_q *rxq;
 
 	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS) {
 		clear_bit(queue_id, &fep->work_rx);
-		pkt_received += fec_enet_rx_queue(ndev,
-					budget - pkt_received, queue_id);
+		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
+		pkt_received += fec_rxq(ndev, fep, rxq, budget - pkt_received);
 	}
 	return pkt_received;
 }
-- 
2.5.0

Re: [PATCH net-next V2 02/16] net: fec: pass rxq to fec_enet_rx_queue instead of queue_id

[Zhi Li]

On Wed, Feb 24, 2016 at 6:36 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> The queue_id is the qid member of struct bufdesc_prop.
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index dbac975..9619b9e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1328,11 +1328,9 @@ static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb,
>   * not been given to the system, we just set the empty indicator,
>   * effectively tossing the packet.
>   */
> -static int
> -fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
> +static int fec_rxq(struct net_device *ndev, struct fec_enet_private *fep,
> +                  struct fec_enet_priv_rx_q *rxq, int budget)

feq is not necessary.
you can get it from ndev.

best regards
Frank Li

>  {
> -       struct fec_enet_private *fep = netdev_priv(ndev);
> -       struct fec_enet_priv_rx_q *rxq;
>         struct bufdesc *bdp;
>         unsigned short status;
>         struct  sk_buff *skb_new = NULL;
> @@ -1350,8 +1348,6 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  #ifdef CONFIG_M532x
>         flush_cache_all();
>  #endif
> -       queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -       rxq = fep->rx_queue[queue_id];
>
>         /* First, grab all of the stats for the incoming packet.
>          * These get messed up if we get called due to a busy condition.
> @@ -1519,11 +1515,12 @@ fec_enet_rx(struct net_device *ndev, int budget)
>         int     pkt_received = 0;
>         u16     queue_id;
>         struct fec_enet_private *fep = netdev_priv(ndev);
> +       struct fec_enet_priv_rx_q *rxq;
>
>         for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS) {
>                 clear_bit(queue_id, &fep->work_rx);
> -               pkt_received += fec_enet_rx_queue(ndev,
> -                                       budget - pkt_received, queue_id);
> +               rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> +               pkt_received += fec_rxq(ndev, fep, rxq, budget - pkt_received);
>         }
>         return pkt_received;
>  }
> --
> 2.5.0
>

RE: [PATCH net-next V2 02/16] net: fec: pass rxq to fec_enet_rx_queue instead of queue_id

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 02/16] net: fec: pass rxq to fec_enet_rx_queue
> instead of queue_id
> 
> The queue_id is the qid member of struct bufdesc_prop.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index dbac975..9619b9e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1328,11 +1328,9 @@ static bool fec_enet_copybreak(struct net_device
> *ndev, struct sk_buff **skb,
>   * not been given to the system, we just set the empty indicator,
>   * effectively tossing the packet.
>   */
> -static int
> -fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
> +static int fec_rxq(struct net_device *ndev, struct fec_enet_private *fep,
> +		   struct fec_enet_priv_rx_q *rxq, int budget)
>  {
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct fec_enet_priv_rx_q *rxq;
>  	struct bufdesc *bdp;
>  	unsigned short status;
>  	struct  sk_buff *skb_new = NULL;
> @@ -1350,8 +1348,6 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)  #ifdef CONFIG_M532x
>  	flush_cache_all();
>  #endif
> -	queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -	rxq = fep->rx_queue[queue_id];
> 
>  	/* First, grab all of the stats for the incoming packet.
>  	 * These get messed up if we get called due to a busy condition.
> @@ -1519,11 +1515,12 @@ fec_enet_rx(struct net_device *ndev, int budget)
>  	int     pkt_received = 0;
>  	u16	queue_id;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct fec_enet_priv_rx_q *rxq;
> 
>  	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS)
> {
>  		clear_bit(queue_id, &fep->work_rx);
> -		pkt_received += fec_enet_rx_queue(ndev,
> -					budget - pkt_received, queue_id);
> +		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> +		pkt_received += fec_rxq(ndev, fep, rxq, budget -
> pkt_received);
>  	}
>  	return pkt_received;
>  }
> --
> 2.5.0

This is one clean up patch that you introduce struct bufdesc_prop.

Acked-by: Fugang Duan <fugang.duan@nxp.com>

[PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

queue_id is the qid member of struct bufdesc_prop.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9619b9e..c517194 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep, unsigned ts,
 	hwtstamps->hwtstamp = ns_to_ktime(ns);
 }
 
-static void
-fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
+static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
+		    struct fec_enet_priv_tx_q *txq)
 {
-	struct	fec_enet_private *fep;
 	struct bufdesc *bdp;
 	unsigned short status;
 	struct	sk_buff	*skb;
-	struct fec_enet_priv_tx_q *txq;
 	struct netdev_queue *nq;
 	int	index = 0;
 	int	entries_free;
 
-	fep = netdev_priv(ndev);
-
-	queue_id = FEC_ENET_GET_QUQUE(queue_id);
-
-	txq = fep->tx_queue[queue_id];
 	/* get next bdp of dirty_tx */
-	nq = netdev_get_tx_queue(ndev, queue_id);
+	nq = netdev_get_tx_queue(ndev, txq->bd.qid);
 	bdp = txq->dirty_tx;
 
 	/* get next bdp of dirty_tx */
@@ -1268,11 +1261,13 @@ static void
 fec_enet_tx(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct fec_enet_priv_tx_q *txq;
 	u16 queue_id;
 	/* First process class A queue, then Class B and Best Effort queue */
 	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS) {
 		clear_bit(queue_id, &fep->work_tx);
-		fec_enet_tx_queue(ndev, queue_id);
+		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
+		fec_txq(ndev, fep, txq);
 	}
 	return;
 }
-- 
2.5.0

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Zhi Li]

On Wed, Feb 24, 2016 at 6:36 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> queue_id is the qid member of struct bufdesc_prop.
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 9619b9e..c517194 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep, unsigned ts,
>         hwtstamps->hwtstamp = ns_to_ktime(ns);
>  }
>
> -static void
> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> +                   struct fec_enet_priv_tx_q *txq)

you can get fep from ndev.

best regards
Frank Li

>  {
> -       struct  fec_enet_private *fep;
>         struct bufdesc *bdp;
>         unsigned short status;
>         struct  sk_buff *skb;
> -       struct fec_enet_priv_tx_q *txq;
>         struct netdev_queue *nq;
>         int     index = 0;
>         int     entries_free;
>
> -       fep = netdev_priv(ndev);
> -
> -       queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -
> -       txq = fep->tx_queue[queue_id];
>         /* get next bdp of dirty_tx */
> -       nq = netdev_get_tx_queue(ndev, queue_id);
> +       nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>         bdp = txq->dirty_tx;
>
>         /* get next bdp of dirty_tx */
> @@ -1268,11 +1261,13 @@ static void
>  fec_enet_tx(struct net_device *ndev)
>  {
>         struct fec_enet_private *fep = netdev_priv(ndev);
> +       struct fec_enet_priv_tx_q *txq;
>         u16 queue_id;
>         /* First process class A queue, then Class B and Best Effort queue */
>         for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS) {
>                 clear_bit(queue_id, &fep->work_tx);
> -               fec_enet_tx_queue(ndev, queue_id);
> +               txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> +               fec_txq(ndev, fep, txq);
>         }
>         return;
>  }
> --
> 2.5.0
>

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/1/2016 2:06 PM, Zhi Li wrote:
> On Wed, Feb 24, 2016 at 6:36 PM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
>> queue_id is the qid member of struct bufdesc_prop.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 9619b9e..c517194 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep, unsigned ts,
>>         hwtstamps->hwtstamp = ns_to_ktime(ns);
>>  }
>>
>> -static void
>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>> +                   struct fec_enet_priv_tx_q *txq)
> 
> you can get fep from ndev.
> 

True, but fec_txq/fec_rxq is called in a loop. Why not pass it, rather than
look it up again?

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Zhi Li]

On Tue, Mar 1, 2016 at 3:51 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> True, but fec_txq/fec_rxq is called in a loop.

netdev_priv(ndev) is that pointer move.
dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN)

Modem compiler can handle it greatly.

You can't get any valuable performance gain by that.

The main time of CPU is call dma_map_xxx.

best regards
Frank Li

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/1/2016 3:26 PM, Zhi Li wrote:
> On Tue, Mar 1, 2016 at 3:51 PM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
>> True, but fec_txq/fec_rxq is called in a loop.
> 
> netdev_priv(ndev) is that pointer move.
> dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN)
> 
> Modem compiler can handle it greatly.
> 
> You can't get any valuable performance gain by that.
> 
> The main time of CPU is call dma_map_xxx.
> 
> best regards
> Frank Li
> 

Ok, I'll revert that part.

RE: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com>  Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue
> instead of queue_id
> 
> queue_id is the qid member of struct bufdesc_prop.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 9619b9e..c517194 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep,
> unsigned ts,
>  	hwtstamps->hwtstamp = ns_to_ktime(ns);  }
> 
> -static void
> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> +		    struct fec_enet_priv_tx_q *txq)
>  {
> -	struct	fec_enet_private *fep;
>  	struct bufdesc *bdp;
>  	unsigned short status;
>  	struct	sk_buff	*skb;
> -	struct fec_enet_priv_tx_q *txq;
>  	struct netdev_queue *nq;
>  	int	index = 0;
>  	int	entries_free;
> 
> -	fep = netdev_priv(ndev);
> -
> -	queue_id = FEC_ENET_GET_QUQUE(queue_id);
> -
> -	txq = fep->tx_queue[queue_id];
>  	/* get next bdp of dirty_tx */
> -	nq = netdev_get_tx_queue(ndev, queue_id);
> +	nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>  	bdp = txq->dirty_tx;
> 
>  	/* get next bdp of dirty_tx */
> @@ -1268,11 +1261,13 @@ static void
>  fec_enet_tx(struct net_device *ndev)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct fec_enet_priv_tx_q *txq;
>  	u16 queue_id;
>  	/* First process class A queue, then Class B and Best Effort queue */
>  	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
> {
>  		clear_bit(queue_id, &fep->work_tx);
> -		fec_enet_tx_queue(ndev, queue_id);
> +		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> +		fec_txq(ndev, fep, txq);
>  	}
>  	return;
>  }
> --
> 2.5.0

The patch should merge with patch#1.

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/2/2016 8:16 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com>  Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue
>> instead of queue_id
>>
>> queue_id is the qid member of struct bufdesc_prop.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 9619b9e..c517194 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private *fep,
>> unsigned ts,
>>  	hwtstamps->hwtstamp = ns_to_ktime(ns);  }
>>
>> -static void
>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>> +		    struct fec_enet_priv_tx_q *txq)
>>  {
>> -	struct	fec_enet_private *fep;
>>  	struct bufdesc *bdp;
>>  	unsigned short status;
>>  	struct	sk_buff	*skb;
>> -	struct fec_enet_priv_tx_q *txq;
>>  	struct netdev_queue *nq;
>>  	int	index = 0;
>>  	int	entries_free;
>>
>> -	fep = netdev_priv(ndev);
>> -
>> -	queue_id = FEC_ENET_GET_QUQUE(queue_id);
>> -
>> -	txq = fep->tx_queue[queue_id];
>>  	/* get next bdp of dirty_tx */
>> -	nq = netdev_get_tx_queue(ndev, queue_id);
>> +	nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>>  	bdp = txq->dirty_tx;
>>
>>  	/* get next bdp of dirty_tx */
>> @@ -1268,11 +1261,13 @@ static void
>>  fec_enet_tx(struct net_device *ndev)
>>  {
>>  	struct fec_enet_private *fep = netdev_priv(ndev);
>> +	struct fec_enet_priv_tx_q *txq;
>>  	u16 queue_id;
>>  	/* First process class A queue, then Class B and Best Effort queue */
>>  	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
>> {
>>  		clear_bit(queue_id, &fep->work_tx);
>> -		fec_enet_tx_queue(ndev, queue_id);
>> +		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>> +		fec_txq(ndev, fep, txq);
>>  	}
>>  	return;
>>  }
>> --
>> 2.5.0
> 
> The patch should merge with patch#1.
> 


Why ?  That would only hide the change in patch #1.

RE: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, March 03, 2016 12:14 AM
> To: Fugang Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org;
> davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de
> Subject: Re: [PATCH net-next V2 03/16] net: fec: pass txq to
> fec_enet_tx_queue instead of queue_id
> 
> On 3/2/2016 8:16 AM, Fugang Duan wrote:
> > From: Troy Kisky <troy.kisky@boundarydevices.com>  Sent: Thursday,
> > February 25, 2016 8:37 AM
> >> To: netdev@vger.kernel.org; davem@davemloft.net;
> b38611@freescale.com
> >> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de;
> >> andrew@lunn.ch; tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> >> kernel@lists.infradead.org; laci@boundarydevices.com;
> >> shawnguo@kernel.org; johannes@sipsolutions.net;
> >> stillcompiling@gmail.com; sergei.shtylyov@cogentembedded.com;
> >> arnd@arndb.de; Troy Kisky <troy.kisky@boundarydevices.com>
> >> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to
> >> fec_enet_tx_queue instead of queue_id
> >>
> >> queue_id is the qid member of struct bufdesc_prop.
> >>
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
> >>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 9619b9e..c517194 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private
> >> *fep, unsigned ts,
> >>  	hwtstamps->hwtstamp = ns_to_ktime(ns);  }
> >>
> >> -static void
> >> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> >> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
> >> +		    struct fec_enet_priv_tx_q *txq)
> >>  {
> >> -	struct	fec_enet_private *fep;
> >>  	struct bufdesc *bdp;
> >>  	unsigned short status;
> >>  	struct	sk_buff	*skb;
> >> -	struct fec_enet_priv_tx_q *txq;
> >>  	struct netdev_queue *nq;
> >>  	int	index = 0;
> >>  	int	entries_free;
> >>
> >> -	fep = netdev_priv(ndev);
> >> -
> >> -	queue_id = FEC_ENET_GET_QUQUE(queue_id);
> >> -
> >> -	txq = fep->tx_queue[queue_id];
> >>  	/* get next bdp of dirty_tx */
> >> -	nq = netdev_get_tx_queue(ndev, queue_id);
> >> +	nq = netdev_get_tx_queue(ndev, txq->bd.qid);
> >>  	bdp = txq->dirty_tx;
> >>
> >>  	/* get next bdp of dirty_tx */
> >> @@ -1268,11 +1261,13 @@ static void
> >>  fec_enet_tx(struct net_device *ndev)  {
> >>  	struct fec_enet_private *fep = netdev_priv(ndev);
> >> +	struct fec_enet_priv_tx_q *txq;
> >>  	u16 queue_id;
> >>  	/* First process class A queue, then Class B and Best Effort queue */
> >>  	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
> {
> >>  		clear_bit(queue_id, &fep->work_tx);
> >> -		fec_enet_tx_queue(ndev, queue_id);
> >> +		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> >> +		fec_txq(ndev, fep, txq);
> >>  	}
> >>  	return;
> >>  }
> >> --
> >> 2.5.0
> >
> > The patch should merge with patch#1.
> >
> 
> 
> Why ?  That would only hide the change in patch #1.

Hi Troy Kisky,

Sorry, I mean patch#2 net: fec: pass rxq to fec_enet_rx_queue instead of queue_id.  It is not necessary to separate them.

Regards,
Andy

Re: [PATCH net-next V2 03/16] net: fec: pass txq to fec_enet_tx_queue instead of queue_id

[Troy Kisky]

On 3/4/2016 12:41 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, March 03, 2016 12:14 AM
>> To: Fugang Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org;
>> davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de
>> Subject: Re: [PATCH net-next V2 03/16] net: fec: pass txq to
>> fec_enet_tx_queue instead of queue_id
>>
>> On 3/2/2016 8:16 AM, Fugang Duan wrote:
>>> From: Troy Kisky <troy.kisky@boundarydevices.com>  Sent: Thursday,
>>> February 25, 2016 8:37 AM
>>>> To: netdev@vger.kernel.org; davem@davemloft.net;
>> b38611@freescale.com
>>>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de;
>>>> andrew@lunn.ch; tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>>>> kernel@lists.infradead.org; laci@boundarydevices.com;
>>>> shawnguo@kernel.org; johannes@sipsolutions.net;
>>>> stillcompiling@gmail.com; sergei.shtylyov@cogentembedded.com;
>>>> arnd@arndb.de; Troy Kisky <troy.kisky@boundarydevices.com>
>>>> Subject: [PATCH net-next V2 03/16] net: fec: pass txq to
>>>> fec_enet_tx_queue instead of queue_id
>>>>
>>>> queue_id is the qid member of struct bufdesc_prop.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>> ---
>>>>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++-----------
>>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 9619b9e..c517194 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1156,25 +1156,18 @@ fec_enet_hwtstamp(struct fec_enet_private
>>>> *fep, unsigned ts,
>>>>  	hwtstamps->hwtstamp = ns_to_ktime(ns);  }
>>>>
>>>> -static void
>>>> -fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>>> +static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
>>>> +		    struct fec_enet_priv_tx_q *txq)
>>>>  {
>>>> -	struct	fec_enet_private *fep;
>>>>  	struct bufdesc *bdp;
>>>>  	unsigned short status;
>>>>  	struct	sk_buff	*skb;
>>>> -	struct fec_enet_priv_tx_q *txq;
>>>>  	struct netdev_queue *nq;
>>>>  	int	index = 0;
>>>>  	int	entries_free;
>>>>
>>>> -	fep = netdev_priv(ndev);
>>>> -
>>>> -	queue_id = FEC_ENET_GET_QUQUE(queue_id);
>>>> -
>>>> -	txq = fep->tx_queue[queue_id];
>>>>  	/* get next bdp of dirty_tx */
>>>> -	nq = netdev_get_tx_queue(ndev, queue_id);
>>>> +	nq = netdev_get_tx_queue(ndev, txq->bd.qid);
>>>>  	bdp = txq->dirty_tx;
>>>>
>>>>  	/* get next bdp of dirty_tx */
>>>> @@ -1268,11 +1261,13 @@ static void
>>>>  fec_enet_tx(struct net_device *ndev)  {
>>>>  	struct fec_enet_private *fep = netdev_priv(ndev);
>>>> +	struct fec_enet_priv_tx_q *txq;
>>>>  	u16 queue_id;
>>>>  	/* First process class A queue, then Class B and Best Effort queue */
>>>>  	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
>> {
>>>>  		clear_bit(queue_id, &fep->work_tx);
>>>> -		fec_enet_tx_queue(ndev, queue_id);
>>>> +		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>>>> +		fec_txq(ndev, fep, txq);
>>>>  	}
>>>>  	return;
>>>>  }
>>>> --
>>>> 2.5.0
>>>
>>> The patch should merge with patch#1.
>>>
>>
>>
>> Why ?  That would only hide the change in patch #1.
> 
> Hi Troy Kisky,
> 
> Sorry, I mean patch#2 net: fec: pass rxq to fec_enet_rx_queue instead of queue_id.  It is not necessary to separate them.
> 



I'll happily squash them together, that is easy, and I will.
I don't agree they should be, but it is just not at all important to me.


Sincere thanks for reviewing this series though.

Troy

[PATCH net-next V2 04/16] net: fec: reduce interrupts

[Troy Kisky]

By clearing the NAPI interrupts in the NAPI routine
and not in the interrupt handler, we can reduce the
number of interrupts. We also don't need any status
variables as the registers are still valid.

Also, notice that if budget pkts are received, the
next call to fec_enet_rx_napi will now continue to
receive the previously pending packets.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec.h      |   6 +-
 drivers/net/ethernet/freescale/fec_main.c | 127 ++++++++++++------------------
 2 files changed, 50 insertions(+), 83 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 195122e..001200b 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -505,11 +505,7 @@ struct fec_enet_private {
 
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
-
-	unsigned long work_tx;
-	unsigned long work_rx;
-	unsigned long work_ts;
-	unsigned long work_mdio;
+	uint	events;
 
 	struct	platform_device *pdev;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c517194..4a218b9 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -70,8 +70,6 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
 
 #define DRIVER_NAME	"fec"
 
-#define FEC_ENET_GET_QUQUE(_x) ((_x == 0) ? 1 : ((_x == 1) ? 2 : 0))
-
 /* Pause frame feild and FIFO threshold */
 #define FEC_ENET_FCE	(1 << 5)
 #define FEC_ENET_RSEM_V	0x84
@@ -1257,21 +1255,6 @@ static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
 		writel(0, txq->bd.reg_desc_active);
 }
 
-static void
-fec_enet_tx(struct net_device *ndev)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct fec_enet_priv_tx_q *txq;
-	u16 queue_id;
-	/* First process class A queue, then Class B and Best Effort queue */
-	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS) {
-		clear_bit(queue_id, &fep->work_tx);
-		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
-		fec_txq(ndev, fep, txq);
-	}
-	return;
-}
-
 static int
 fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct sk_buff *skb)
 {
@@ -1504,92 +1487,80 @@ rx_processing_done:
 	return pkt_received;
 }
 
-static int
-fec_enet_rx(struct net_device *ndev, int budget)
-{
-	int     pkt_received = 0;
-	u16	queue_id;
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct fec_enet_priv_rx_q *rxq;
-
-	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS) {
-		clear_bit(queue_id, &fep->work_rx);
-		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
-		pkt_received += fec_rxq(ndev, fep, rxq, budget - pkt_received);
-	}
-	return pkt_received;
-}
-
-static bool
-fec_enet_collect_events(struct fec_enet_private *fep, uint int_events)
-{
-	if (int_events == 0)
-		return false;
-
-	if (int_events & FEC_ENET_RXF_0)
-		fep->work_rx |= (1 << 2);
-	if (int_events & FEC_ENET_RXF_1)
-		fep->work_rx |= (1 << 0);
-	if (int_events & FEC_ENET_RXF_2)
-		fep->work_rx |= (1 << 1);
-
-	if (int_events & FEC_ENET_TXF_0)
-		fep->work_tx |= (1 << 2);
-	if (int_events & FEC_ENET_TXF_1)
-		fep->work_tx |= (1 << 0);
-	if (int_events & FEC_ENET_TXF_2)
-		fep->work_tx |= (1 << 1);
-
-	return true;
-}
-
 static irqreturn_t
 fec_enet_interrupt(int irq, void *dev_id)
 {
 	struct net_device *ndev = dev_id;
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	uint int_events;
-	irqreturn_t ret = IRQ_NONE;
+	uint eir = readl(fep->hwp + FEC_IEVENT);
+	uint int_events = eir & readl(fep->hwp + FEC_IMASK);
 
-	int_events = readl(fep->hwp + FEC_IEVENT);
-	writel(int_events, fep->hwp + FEC_IEVENT);
-	fec_enet_collect_events(fep, int_events);
-
-	if ((fep->work_tx || fep->work_rx) && fep->link) {
-		ret = IRQ_HANDLED;
+	if (!int_events)
+		return IRQ_NONE;
 
+	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
 		if (napi_schedule_prep(&fep->napi)) {
 			/* Disable the NAPI interrupts */
 			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
 			__napi_schedule(&fep->napi);
+			int_events &= ~(FEC_ENET_RXF | FEC_ENET_TXF);
+			if (!int_events)
+				return IRQ_HANDLED;
+		} else {
+			fep->events |= int_events;
+			pr_info("%s: couldn't schedule NAPI\n", __func__);
 		}
 	}
 
-	if (int_events & FEC_ENET_MII) {
-		ret = IRQ_HANDLED;
+	writel(int_events, fep->hwp + FEC_IEVENT);
+	if (int_events & FEC_ENET_MII)
 		complete(&fep->mdio_done);
-	}
 
-	if (fep->ptp_clock)
+	if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
 		fec_ptp_check_pps_event(fep);
-
-	return ret;
+	return IRQ_HANDLED;
 }
 
 static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
 {
 	struct net_device *ndev = napi->dev;
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	int pkts;
-
-	pkts = fec_enet_rx(ndev, budget);
+	int pkts = 0;
+	uint events;
 
-	fec_enet_tx(ndev);
+	do {
+		events = readl(fep->hwp + FEC_IEVENT);
+		if (fep->events) {
+			events |= fep->events;
+			fep->events = 0;
+		}
+		events &= FEC_ENET_RXF | FEC_ENET_TXF;
+		if (!events) {
+			if (budget) {
+				napi_complete(napi);
+				writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
+			}
+			return pkts;
+		}
 
-	if (pkts < budget) {
-		napi_complete(napi);
-		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
-	}
+		writel(events, fep->hwp + FEC_IEVENT);
+		if (events & FEC_ENET_RXF_1)
+			pkts += fec_rxq(ndev, fep, fep->rx_queue[1],
+					budget - pkts);
+		if (events & FEC_ENET_RXF_2)
+			pkts += fec_rxq(ndev, fep, fep->rx_queue[2],
+					budget - pkts);
+		if (events & FEC_ENET_RXF_0)
+			pkts += fec_rxq(ndev, fep, fep->rx_queue[0],
+					budget - pkts);
+		if (events & FEC_ENET_TXF_1)
+			fec_txq(ndev, fep, fep->tx_queue[1]);
+		if (events & FEC_ENET_TXF_2)
+			fec_txq(ndev, fep, fep->tx_queue[2]);
+		if (events & FEC_ENET_TXF_0)
+			fec_txq(ndev, fep, fep->tx_queue[0]);
+	} while (pkts < budget);
+	fep->events |= events & FEC_ENET_RXF;	/* save for next callback */
 	return pkts;
 }
 
-- 
2.5.0

RE: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 04/16] net: fec: reduce interrupts
> 
> By clearing the NAPI interrupts in the NAPI routine and not in the interrupt
> handler, we can reduce the number of interrupts. We also don't need any
> status variables as the registers are still valid.
> 
> Also, notice that if budget pkts are received, the next call to fec_enet_rx_napi
> will now continue to receive the previously pending packets.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |   6 +-
>  drivers/net/ethernet/freescale/fec_main.c | 127 ++++++++++++----------------
> --
>  2 files changed, 50 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 195122e..001200b 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -505,11 +505,7 @@ struct fec_enet_private {
> 
>  	unsigned int total_tx_ring_size;
>  	unsigned int total_rx_ring_size;
> -
> -	unsigned long work_tx;
> -	unsigned long work_rx;
> -	unsigned long work_ts;
> -	unsigned long work_mdio;
> +	uint	events;
> 
>  	struct	platform_device *pdev;
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c517194..4a218b9 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -70,8 +70,6 @@ static void fec_enet_itr_coal_init(struct net_device
> *ndev);
> 
>  #define DRIVER_NAME	"fec"
> 
> -#define FEC_ENET_GET_QUQUE(_x) ((_x == 0) ? 1 : ((_x == 1) ? 2 : 0))
> -
>  /* Pause frame feild and FIFO threshold */
>  #define FEC_ENET_FCE	(1 << 5)
>  #define FEC_ENET_RSEM_V	0x84
> @@ -1257,21 +1255,6 @@ static void fec_txq(struct net_device *ndev, struct
> fec_enet_private *fep,
>  		writel(0, txq->bd.reg_desc_active);
>  }
> 
> -static void
> -fec_enet_tx(struct net_device *ndev)
> -{
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct fec_enet_priv_tx_q *txq;
> -	u16 queue_id;
> -	/* First process class A queue, then Class B and Best Effort queue */
> -	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
> {
> -		clear_bit(queue_id, &fep->work_tx);
> -		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> -		fec_txq(ndev, fep, txq);
> -	}
> -	return;
> -}
> -
>  static int
>  fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct
> sk_buff *skb)  { @@ -1504,92 +1487,80 @@ rx_processing_done:
>  	return pkt_received;
>  }
> 
> -static int
> -fec_enet_rx(struct net_device *ndev, int budget) -{
> -	int     pkt_received = 0;
> -	u16	queue_id;
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct fec_enet_priv_rx_q *rxq;
> -
> -	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS)
> {
> -		clear_bit(queue_id, &fep->work_rx);
> -		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> -		pkt_received += fec_rxq(ndev, fep, rxq, budget -
> pkt_received);
> -	}
> -	return pkt_received;
> -}
> -
> -static bool
> -fec_enet_collect_events(struct fec_enet_private *fep, uint int_events) -{
> -	if (int_events == 0)
> -		return false;
> -
> -	if (int_events & FEC_ENET_RXF_0)
> -		fep->work_rx |= (1 << 2);
> -	if (int_events & FEC_ENET_RXF_1)
> -		fep->work_rx |= (1 << 0);
> -	if (int_events & FEC_ENET_RXF_2)
> -		fep->work_rx |= (1 << 1);
> -
> -	if (int_events & FEC_ENET_TXF_0)
> -		fep->work_tx |= (1 << 2);
> -	if (int_events & FEC_ENET_TXF_1)
> -		fep->work_tx |= (1 << 0);
> -	if (int_events & FEC_ENET_TXF_2)
> -		fep->work_tx |= (1 << 1);
> -
> -	return true;
> -}
> -
>  static irqreturn_t
>  fec_enet_interrupt(int irq, void *dev_id)  {
>  	struct net_device *ndev = dev_id;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	uint int_events;
> -	irqreturn_t ret = IRQ_NONE;
> +	uint eir = readl(fep->hwp + FEC_IEVENT);
> +	uint int_events = eir & readl(fep->hwp + FEC_IMASK);
> 
> -	int_events = readl(fep->hwp + FEC_IEVENT);
> -	writel(int_events, fep->hwp + FEC_IEVENT);
> -	fec_enet_collect_events(fep, int_events);
> -
> -	if ((fep->work_tx || fep->work_rx) && fep->link) {
> -		ret = IRQ_HANDLED;
> +	if (!int_events)
> +		return IRQ_NONE;
> 
> +	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
>  		if (napi_schedule_prep(&fep->napi)) {
>  			/* Disable the NAPI interrupts */
>  			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
>  			__napi_schedule(&fep->napi);
> +			int_events &= ~(FEC_ENET_RXF | FEC_ENET_TXF);
> +			if (!int_events)
> +				return IRQ_HANDLED;
> +		} else {
> +			fep->events |= int_events;
> +			pr_info("%s: couldn't schedule NAPI\n", __func__);
>  		}
>  	}
> 
> -	if (int_events & FEC_ENET_MII) {
> -		ret = IRQ_HANDLED;
> +	writel(int_events, fep->hwp + FEC_IEVENT);
> +	if (int_events & FEC_ENET_MII)
>  		complete(&fep->mdio_done);
> -	}
> 
> -	if (fep->ptp_clock)
> +	if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
>  		fec_ptp_check_pps_event(fep);
> -
This is error in here.  FEC compare timer event is not TS timer.


> -	return ret;
> +	return IRQ_HANDLED;
>  }
> 
>  static int fec_enet_rx_napi(struct napi_struct *napi, int budget)  {
>  	struct net_device *ndev = napi->dev;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	int pkts;
> -
> -	pkts = fec_enet_rx(ndev, budget);
> +	int pkts = 0;
> +	uint events;
> 
> -	fec_enet_tx(ndev);
> +	do {
> +		events = readl(fep->hwp + FEC_IEVENT);
> +		if (fep->events) {
> +			events |= fep->events;
> +			fep->events = 0;
> +		}
> +		events &= FEC_ENET_RXF | FEC_ENET_TXF;
> +		if (!events) {
> +			if (budget) {
> +				napi_complete(napi);
> +				writel(FEC_DEFAULT_IMASK, fep->hwp +
> FEC_IMASK);
> +			}
> +			return pkts;
> +		}
> 
> -	if (pkts < budget) {
> -		napi_complete(napi);
> -		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> -	}
> +		writel(events, fep->hwp + FEC_IEVENT);
> +		if (events & FEC_ENET_RXF_1)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[1],
> +					budget - pkts);
> +		if (events & FEC_ENET_RXF_2)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[2],
> +					budget - pkts);
> +		if (events & FEC_ENET_RXF_0)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[0],
> +					budget - pkts);
> +		if (events & FEC_ENET_TXF_1)
> +			fec_txq(ndev, fep, fep->tx_queue[1]);
> +		if (events & FEC_ENET_TXF_2)
> +			fec_txq(ndev, fep, fep->tx_queue[2]);
> +		if (events & FEC_ENET_TXF_0)
> +			fec_txq(ndev, fep, fep->tx_queue[0]);
> +	} while (pkts < budget);
> +	fep->events |= events & FEC_ENET_RXF;	/* save for next callback
> */
>  	return pkts;
>  }
> 
> --
> 2.5.0

Re: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Troy Kisky]

On 3/2/2016 8:13 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 04/16] net: fec: reduce interrupts
>>
>> By clearing the NAPI interrupts in the NAPI routine and not in the interrupt
>> handler, we can reduce the number of interrupts. We also don't need any
>> status variables as the registers are still valid.
>>
>> Also, notice that if budget pkts are received, the next call to fec_enet_rx_napi
>> will now continue to receive the previously pending packets.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec.h      |   6 +-
>>  drivers/net/ethernet/freescale/fec_main.c | 127 ++++++++++++----------------
>> --
>>  2 files changed, 50 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec.h
>> b/drivers/net/ethernet/freescale/fec.h
>> index 195122e..001200b 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -505,11 +505,7 @@ struct fec_enet_private {
>>
>>  	unsigned int total_tx_ring_size;
>>  	unsigned int total_rx_ring_size;
>> -
>> -	unsigned long work_tx;
>> -	unsigned long work_rx;
>> -	unsigned long work_ts;
>> -	unsigned long work_mdio;
>> +	uint	events;
>>
>>  	struct	platform_device *pdev;
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index c517194..4a218b9 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -70,8 +70,6 @@ static void fec_enet_itr_coal_init(struct net_device
>> *ndev);
>>
>>  #define DRIVER_NAME	"fec"
>>
>> -#define FEC_ENET_GET_QUQUE(_x) ((_x == 0) ? 1 : ((_x == 1) ? 2 : 0))
>> -
>>  /* Pause frame feild and FIFO threshold */
>>  #define FEC_ENET_FCE	(1 << 5)
>>  #define FEC_ENET_RSEM_V	0x84
>> @@ -1257,21 +1255,6 @@ static void fec_txq(struct net_device *ndev, struct
>> fec_enet_private *fep,
>>  		writel(0, txq->bd.reg_desc_active);
>>  }
>>
>> -static void
>> -fec_enet_tx(struct net_device *ndev)
>> -{
>> -	struct fec_enet_private *fep = netdev_priv(ndev);
>> -	struct fec_enet_priv_tx_q *txq;
>> -	u16 queue_id;
>> -	/* First process class A queue, then Class B and Best Effort queue */
>> -	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
>> {
>> -		clear_bit(queue_id, &fep->work_tx);
>> -		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>> -		fec_txq(ndev, fep, txq);
>> -	}
>> -	return;
>> -}
>> -
>>  static int
>>  fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct
>> sk_buff *skb)  { @@ -1504,92 +1487,80 @@ rx_processing_done:
>>  	return pkt_received;
>>  }
>>
>> -static int
>> -fec_enet_rx(struct net_device *ndev, int budget) -{
>> -	int     pkt_received = 0;
>> -	u16	queue_id;
>> -	struct fec_enet_private *fep = netdev_priv(ndev);
>> -	struct fec_enet_priv_rx_q *rxq;
>> -
>> -	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS)
>> {
>> -		clear_bit(queue_id, &fep->work_rx);
>> -		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>> -		pkt_received += fec_rxq(ndev, fep, rxq, budget -
>> pkt_received);
>> -	}
>> -	return pkt_received;
>> -}
>> -
>> -static bool
>> -fec_enet_collect_events(struct fec_enet_private *fep, uint int_events) -{
>> -	if (int_events == 0)
>> -		return false;
>> -
>> -	if (int_events & FEC_ENET_RXF_0)
>> -		fep->work_rx |= (1 << 2);
>> -	if (int_events & FEC_ENET_RXF_1)
>> -		fep->work_rx |= (1 << 0);
>> -	if (int_events & FEC_ENET_RXF_2)
>> -		fep->work_rx |= (1 << 1);
>> -
>> -	if (int_events & FEC_ENET_TXF_0)
>> -		fep->work_tx |= (1 << 2);
>> -	if (int_events & FEC_ENET_TXF_1)
>> -		fep->work_tx |= (1 << 0);
>> -	if (int_events & FEC_ENET_TXF_2)
>> -		fep->work_tx |= (1 << 1);
>> -
>> -	return true;
>> -}
>> -
>>  static irqreturn_t
>>  fec_enet_interrupt(int irq, void *dev_id)  {
>>  	struct net_device *ndev = dev_id;
>>  	struct fec_enet_private *fep = netdev_priv(ndev);
>> -	uint int_events;
>> -	irqreturn_t ret = IRQ_NONE;
>> +	uint eir = readl(fep->hwp + FEC_IEVENT);
>> +	uint int_events = eir & readl(fep->hwp + FEC_IMASK);
>>
>> -	int_events = readl(fep->hwp + FEC_IEVENT);
>> -	writel(int_events, fep->hwp + FEC_IEVENT);
>> -	fec_enet_collect_events(fep, int_events);
>> -
>> -	if ((fep->work_tx || fep->work_rx) && fep->link) {
>> -		ret = IRQ_HANDLED;
>> +	if (!int_events)
>> +		return IRQ_NONE;
>>
>> +	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
>>  		if (napi_schedule_prep(&fep->napi)) {
>>  			/* Disable the NAPI interrupts */
>>  			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
>>  			__napi_schedule(&fep->napi);
>> +			int_events &= ~(FEC_ENET_RXF | FEC_ENET_TXF);
>> +			if (!int_events)
>> +				return IRQ_HANDLED;
>> +		} else {
>> +			fep->events |= int_events;
>> +			pr_info("%s: couldn't schedule NAPI\n", __func__);
>>  		}
>>  	}
>>
>> -	if (int_events & FEC_ENET_MII) {
>> -		ret = IRQ_HANDLED;
>> +	writel(int_events, fep->hwp + FEC_IEVENT);
>> +	if (int_events & FEC_ENET_MII)
>>  		complete(&fep->mdio_done);
>> -	}
>>
>> -	if (fep->ptp_clock)
>> +	if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
>>  		fec_ptp_check_pps_event(fep);
>> -
> This is error in here.  FEC compare timer event is not TS timer.
> 
> 


So when should fec_ptp_check_pps_event be called ? On every interrupt ?

If napi is active, it is going to be delayed for a very long time.
Should it be moved to the napi routine?

Re: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Zhi Li]

On Wed, Mar 2, 2016 at 10:12 AM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> it is going to be delayed for a very long time.
> Should it be moved to the napi routine?

No!

irq can generate.  Compared irq enable is not controlled by EIMR, but TCSR.

Original code is correct.

best regards
Frank Li

Re: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Zhi Li]

On Wed, Mar 2, 2016 at 10:12 AM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> On 3/2/2016 8:13 AM, Fugang Duan wrote:
>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>>> <troy.kisky@boundarydevices.com>
>>> Subject: [PATCH net-next V2 04/16] net: fec: reduce interrupts
>>>
>>> By clearing the NAPI interrupts in the NAPI routine and not in the interrupt
>>> handler, we can reduce the number of interrupts. We also don't need any
>>> status variables as the registers are still valid.
>>>
>>> Also, notice that if budget pkts are received, the next call to fec_enet_rx_napi
>>> will now continue to receive the previously pending packets.
>>>
>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>> ---
>>>  drivers/net/ethernet/freescale/fec.h      |   6 +-
>>>  drivers/net/ethernet/freescale/fec_main.c | 127 ++++++++++++----------------
>>> --
>>>  2 files changed, 50 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>> b/drivers/net/ethernet/freescale/fec.h
>>> index 195122e..001200b 100644
>>> --- a/drivers/net/ethernet/freescale/fec.h
>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>> @@ -505,11 +505,7 @@ struct fec_enet_private {
>>>
>>>      unsigned int total_tx_ring_size;
>>>      unsigned int total_rx_ring_size;
>>> -
>>> -    unsigned long work_tx;
>>> -    unsigned long work_rx;
>>> -    unsigned long work_ts;
>>> -    unsigned long work_mdio;
>>> +    uint    events;
>>>
>>>      struct  platform_device *pdev;
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index c517194..4a218b9 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -70,8 +70,6 @@ static void fec_enet_itr_coal_init(struct net_device
>>> *ndev);
>>>
>>>  #define DRIVER_NAME "fec"
>>>
>>> -#define FEC_ENET_GET_QUQUE(_x) ((_x == 0) ? 1 : ((_x == 1) ? 2 : 0))
>>> -
>>>  /* Pause frame feild and FIFO threshold */
>>>  #define FEC_ENET_FCE        (1 << 5)
>>>  #define FEC_ENET_RSEM_V     0x84
>>> @@ -1257,21 +1255,6 @@ static void fec_txq(struct net_device *ndev, struct
>>> fec_enet_private *fep,
>>>              writel(0, txq->bd.reg_desc_active);
>>>  }
>>>
>>> -static void
>>> -fec_enet_tx(struct net_device *ndev)
>>> -{
>>> -    struct fec_enet_private *fep = netdev_priv(ndev);
>>> -    struct fec_enet_priv_tx_q *txq;
>>> -    u16 queue_id;
>>> -    /* First process class A queue, then Class B and Best Effort queue */
>>> -    for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
>>> {
>>> -            clear_bit(queue_id, &fep->work_tx);
>>> -            txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>>> -            fec_txq(ndev, fep, txq);
>>> -    }
>>> -    return;
>>> -}
>>> -
>>>  static int
>>>  fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct
>>> sk_buff *skb)  { @@ -1504,92 +1487,80 @@ rx_processing_done:
>>>      return pkt_received;
>>>  }
>>>
>>> -static int
>>> -fec_enet_rx(struct net_device *ndev, int budget) -{
>>> -    int     pkt_received = 0;
>>> -    u16     queue_id;
>>> -    struct fec_enet_private *fep = netdev_priv(ndev);
>>> -    struct fec_enet_priv_rx_q *rxq;
>>> -
>>> -    for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS)
>>> {
>>> -            clear_bit(queue_id, &fep->work_rx);
>>> -            rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
>>> -            pkt_received += fec_rxq(ndev, fep, rxq, budget -
>>> pkt_received);
>>> -    }
>>> -    return pkt_received;
>>> -}
>>> -
>>> -static bool
>>> -fec_enet_collect_events(struct fec_enet_private *fep, uint int_events) -{
>>> -    if (int_events == 0)
>>> -            return false;
>>> -
>>> -    if (int_events & FEC_ENET_RXF_0)
>>> -            fep->work_rx |= (1 << 2);
>>> -    if (int_events & FEC_ENET_RXF_1)
>>> -            fep->work_rx |= (1 << 0);
>>> -    if (int_events & FEC_ENET_RXF_2)
>>> -            fep->work_rx |= (1 << 1);
>>> -
>>> -    if (int_events & FEC_ENET_TXF_0)
>>> -            fep->work_tx |= (1 << 2);
>>> -    if (int_events & FEC_ENET_TXF_1)
>>> -            fep->work_tx |= (1 << 0);
>>> -    if (int_events & FEC_ENET_TXF_2)
>>> -            fep->work_tx |= (1 << 1);
>>> -
>>> -    return true;
>>> -}
>>> -
>>>  static irqreturn_t
>>>  fec_enet_interrupt(int irq, void *dev_id)  {
>>>      struct net_device *ndev = dev_id;
>>>      struct fec_enet_private *fep = netdev_priv(ndev);
>>> -    uint int_events;
>>> -    irqreturn_t ret = IRQ_NONE;
>>> +    uint eir = readl(fep->hwp + FEC_IEVENT);
>>> +    uint int_events = eir & readl(fep->hwp + FEC_IMASK);
>>>
>>> -    int_events = readl(fep->hwp + FEC_IEVENT);
>>> -    writel(int_events, fep->hwp + FEC_IEVENT);
>>> -    fec_enet_collect_events(fep, int_events);
>>> -
>>> -    if ((fep->work_tx || fep->work_rx) && fep->link) {
>>> -            ret = IRQ_HANDLED;
>>> +    if (!int_events)
>>> +            return IRQ_NONE;
>>>
>>> +    if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
>>>              if (napi_schedule_prep(&fep->napi)) {
>>>                      /* Disable the NAPI interrupts */
>>>                      writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
>>>                      __napi_schedule(&fep->napi);
>>> +                    int_events &= ~(FEC_ENET_RXF | FEC_ENET_TXF);
>>> +                    if (!int_events)
>>> +                            return IRQ_HANDLED;
>>> +            } else {
>>> +                    fep->events |= int_events;
>>> +                    pr_info("%s: couldn't schedule NAPI\n", __func__);
>>>              }
>>>      }
>>>
>>> -    if (int_events & FEC_ENET_MII) {
>>> -            ret = IRQ_HANDLED;
>>> +    writel(int_events, fep->hwp + FEC_IEVENT);
>>> +    if (int_events & FEC_ENET_MII)
>>>              complete(&fep->mdio_done);
>>> -    }
>>>
>>> -    if (fep->ptp_clock)
>>> +    if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
>>>              fec_ptp_check_pps_event(fep);
>>> -
>> This is error in here.  FEC compare timer event is not TS timer.
>>
>>
>
>
> So when should fec_ptp_check_pps_event be called ? On every interrupt ?

Compare event is not showed in EIR register.  Need check TCSR, please
see below code.

uint fec_ptp_check_pps_event(struct fec_enet_private *fep)
{
xx
 val = readl(fep->hwp + FEC_TCSR(channel));
        if (val & FEC_T_TF_MASK) {
}
>
> If napi is active, it is going to be delayed for a very long time.
> Should it be moved to the napi routine?
>
>
>
>

Re: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Troy Kisky]

On 3/2/2016 9:47 AM, Zhi Li wrote:
> On Wed, Mar 2, 2016 at 10:12 AM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
>> On 3/2/2016 8:13 AM, Fugang Duan wrote:
>>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>>>>
>>>> -    if (fep->ptp_clock)
>>>> +    if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
>>>>              fec_ptp_check_pps_event(fep);
>>>> -
>>> This is error in here.  FEC compare timer event is not TS timer.
>>>
>>>
>>
>>
>> So when should fec_ptp_check_pps_event be called ? On every interrupt ?
> 
> Compare event is not showed in EIR register.  Need check TCSR, please
> see below code.
> 
> uint fec_ptp_check_pps_event(struct fec_enet_private *fep)
> {
> xx
>  val = readl(fep->hwp + FEC_TCSR(channel));
>         if (val & FEC_T_TF_MASK) {
> }


So, should FEC_ENET_TS_TIMER be removed from FEC_DEFAULT_IMASK, since
the interrupt routine never checks it ?

Re: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Zhi Li]

On Wed, Mar 2, 2016 at 4:32 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> FEC_ENET_TS_TIMER

I think so. TS_TIMER should never be triggered.

best regards
Frank Li

RE: [PATCH net-next V2 04/16] net: fec: reduce interrupts

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 04/16] net: fec: reduce interrupts
> 
> By clearing the NAPI interrupts in the NAPI routine and not in the interrupt
> handler, we can reduce the number of interrupts. We also don't need any
> status variables as the registers are still valid.
> 
> Also, notice that if budget pkts are received, the next call to fec_enet_rx_napi
> will now continue to receive the previously pending packets.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |   6 +-
>  drivers/net/ethernet/freescale/fec_main.c | 127 ++++++++++++----------------
> --
>  2 files changed, 50 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 195122e..001200b 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -505,11 +505,7 @@ struct fec_enet_private {
> 
>  	unsigned int total_tx_ring_size;
>  	unsigned int total_rx_ring_size;
> -
> -	unsigned long work_tx;
> -	unsigned long work_rx;
> -	unsigned long work_ts;
> -	unsigned long work_mdio;
> +	uint	events;
> 
>  	struct	platform_device *pdev;
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c517194..4a218b9 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -70,8 +70,6 @@ static void fec_enet_itr_coal_init(struct net_device
> *ndev);
> 
>  #define DRIVER_NAME	"fec"
> 
> -#define FEC_ENET_GET_QUQUE(_x) ((_x == 0) ? 1 : ((_x == 1) ? 2 : 0))
> -
>  /* Pause frame feild and FIFO threshold */
>  #define FEC_ENET_FCE	(1 << 5)
>  #define FEC_ENET_RSEM_V	0x84
> @@ -1257,21 +1255,6 @@ static void fec_txq(struct net_device *ndev, struct
> fec_enet_private *fep,
>  		writel(0, txq->bd.reg_desc_active);
>  }
> 
> -static void
> -fec_enet_tx(struct net_device *ndev)
> -{
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct fec_enet_priv_tx_q *txq;
> -	u16 queue_id;
> -	/* First process class A queue, then Class B and Best Effort queue */
> -	for_each_set_bit(queue_id, &fep->work_tx, FEC_ENET_MAX_TX_QS)
> {
> -		clear_bit(queue_id, &fep->work_tx);
> -		txq = fep->tx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> -		fec_txq(ndev, fep, txq);
> -	}
> -	return;
> -}
> -
>  static int
>  fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct
> sk_buff *skb)  { @@ -1504,92 +1487,80 @@ rx_processing_done:
>  	return pkt_received;
>  }
> 
> -static int
> -fec_enet_rx(struct net_device *ndev, int budget) -{
> -	int     pkt_received = 0;
> -	u16	queue_id;
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct fec_enet_priv_rx_q *rxq;
> -
> -	for_each_set_bit(queue_id, &fep->work_rx, FEC_ENET_MAX_RX_QS)
> {
> -		clear_bit(queue_id, &fep->work_rx);
> -		rxq = fep->rx_queue[FEC_ENET_GET_QUQUE(queue_id)];
> -		pkt_received += fec_rxq(ndev, fep, rxq, budget -
> pkt_received);
> -	}
> -	return pkt_received;
> -}
> -
> -static bool
> -fec_enet_collect_events(struct fec_enet_private *fep, uint int_events) -{
> -	if (int_events == 0)
> -		return false;
> -
> -	if (int_events & FEC_ENET_RXF_0)
> -		fep->work_rx |= (1 << 2);
> -	if (int_events & FEC_ENET_RXF_1)
> -		fep->work_rx |= (1 << 0);
> -	if (int_events & FEC_ENET_RXF_2)
> -		fep->work_rx |= (1 << 1);
> -
> -	if (int_events & FEC_ENET_TXF_0)
> -		fep->work_tx |= (1 << 2);
> -	if (int_events & FEC_ENET_TXF_1)
> -		fep->work_tx |= (1 << 0);
> -	if (int_events & FEC_ENET_TXF_2)
> -		fep->work_tx |= (1 << 1);
> -
> -	return true;
> -}
> -
>  static irqreturn_t
>  fec_enet_interrupt(int irq, void *dev_id)  {
>  	struct net_device *ndev = dev_id;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	uint int_events;
> -	irqreturn_t ret = IRQ_NONE;
> +	uint eir = readl(fep->hwp + FEC_IEVENT);
> +	uint int_events = eir & readl(fep->hwp + FEC_IMASK);
> 
> -	int_events = readl(fep->hwp + FEC_IEVENT);
> -	writel(int_events, fep->hwp + FEC_IEVENT);
> -	fec_enet_collect_events(fep, int_events);
> -
> -	if ((fep->work_tx || fep->work_rx) && fep->link) {
> -		ret = IRQ_HANDLED;
> +	if (!int_events)
> +		return IRQ_NONE;
> 
> +	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
>  		if (napi_schedule_prep(&fep->napi)) {
>  			/* Disable the NAPI interrupts */
>  			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
>  			__napi_schedule(&fep->napi);
> +			int_events &= ~(FEC_ENET_RXF | FEC_ENET_TXF);
> +			if (!int_events)
> +				return IRQ_HANDLED;
> +		} else {
> +			fep->events |= int_events;
> +			pr_info("%s: couldn't schedule NAPI\n", __func__);
>  		}
>  	}
> 
> -	if (int_events & FEC_ENET_MII) {
> -		ret = IRQ_HANDLED;
> +	writel(int_events, fep->hwp + FEC_IEVENT);
> +	if (int_events & FEC_ENET_MII)
>  		complete(&fep->mdio_done);
> -	}
> 
> -	if (fep->ptp_clock)
> +	if ((int_events & FEC_ENET_TS_TIMER) && fep->ptp_clock)
>  		fec_ptp_check_pps_event(fep);
> -
> -	return ret;
> +	return IRQ_HANDLED;
>  }
> 
>  static int fec_enet_rx_napi(struct napi_struct *napi, int budget)  {
>  	struct net_device *ndev = napi->dev;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	int pkts;
> -
> -	pkts = fec_enet_rx(ndev, budget);
> +	int pkts = 0;
> +	uint events;
> 
> -	fec_enet_tx(ndev);
> +	do {
> +		events = readl(fep->hwp + FEC_IEVENT);
> +		if (fep->events) {
> +			events |= fep->events;
> +			fep->events = 0;
> +		}
> +		events &= FEC_ENET_RXF | FEC_ENET_TXF;
> +		if (!events) {
> +			if (budget) {
> +				napi_complete(napi);
> +				writel(FEC_DEFAULT_IMASK, fep->hwp +
> FEC_IMASK);
> +			}
> +			return pkts;
> +		}
> 
> -	if (pkts < budget) {
> -		napi_complete(napi);
> -		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> -	}
> +		writel(events, fep->hwp + FEC_IEVENT);
> +		if (events & FEC_ENET_RXF_1)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[1],
> +					budget - pkts);
> +		if (events & FEC_ENET_RXF_2)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[2],
> +					budget - pkts);
> +		if (events & FEC_ENET_RXF_0)
> +			pkts += fec_rxq(ndev, fep, fep->rx_queue[0],
> +					budget - pkts);
> +		if (events & FEC_ENET_TXF_1)
> +			fec_txq(ndev, fep, fep->tx_queue[1]);
> +		if (events & FEC_ENET_TXF_2)
> +			fec_txq(ndev, fep, fep->tx_queue[2]);
> +		if (events & FEC_ENET_TXF_0)
> +			fec_txq(ndev, fep, fep->tx_queue[0]);
> +	} while (pkts < budget);
> +	fep->events |= events & FEC_ENET_RXF;	/* save for next callback
Here seems ugly, if the first time fep->events is FEC_ENET_RXF, suppose FEC_IEVENT  is zero when call the second napi callback,  fec_rxq() will be called again after "events |= fep->events;", it is not necessary.
If you clear FEC_ENET_TXF, FEC_ENET_RXF bit in ENET_EIMR, there have no TXF, RXF interrupt to cpu. So I don't think the patch can reduce interrupt numbers.

I don't like the patch.  I suggest to separate TX and RX into two NAPI context, for tx path call .netif_tx_napi_add() to init one NAPI context. 

Regards,
Andy

> */
>  	return pkts;
>  }
> 
> --
> 2.5.0

[PATCH net-next V2 05/16] net: fec: split off napi routine with 3 queues

[Troy Kisky]

If we only have 1 tx/rx queue, we need not check
the other queues.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 39 +++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4a218b9..610cf6c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1521,7 +1521,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
+static int fec_enet_napi_q3(struct napi_struct *napi, int budget)
 {
 	struct net_device *ndev = napi->dev;
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -1564,6 +1564,39 @@ static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
 	return pkts;
 }
 
+static int fec_enet_napi_q1(struct napi_struct *napi, int budget)
+{
+	struct net_device *ndev = napi->dev;
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	int pkts = 0;
+	uint events;
+
+	do {
+		events = readl(fep->hwp + FEC_IEVENT);
+		if (fep->events) {
+			events |= fep->events;
+			fep->events = 0;
+		}
+		events &= FEC_ENET_RXF_0 | FEC_ENET_TXF_0;
+		if (!events) {
+			if (budget) {
+				napi_complete(napi);
+				writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
+			}
+			return pkts;
+		}
+
+		writel(events, fep->hwp + FEC_IEVENT);
+		if (events & FEC_ENET_RXF_0)
+			pkts += fec_rxq(ndev, fep, fep->rx_queue[0],
+					budget - pkts);
+		if (events & FEC_ENET_TXF_0)
+			fec_txq(ndev, fep, fep->tx_queue[0]);
+	} while (pkts < budget);
+	fep->events |= FEC_ENET_RXF_0;	/* save for next callback */
+	return pkts;
+}
+
 /* ------------------------------------------------------------------------- */
 static void fec_get_mac(struct net_device *ndev)
 {
@@ -3123,7 +3156,9 @@ static int fec_enet_init(struct net_device *ndev)
 	ndev->ethtool_ops = &fec_enet_ethtool_ops;
 
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
-	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, NAPI_POLL_WEIGHT);
+	netif_napi_add(ndev, &fep->napi, (fep->num_rx_queues |
+		       fep->num_tx_queues) == 1 ? fec_enet_napi_q1 :
+		       fec_enet_napi_q3, NAPI_POLL_WEIGHT);
 
 	if (fep->quirks & FEC_QUIRK_HAS_VLAN)
 		/* enable hw VLAN support */
-- 
2.5.0

[PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when just one is being checked

[Troy Kisky]

FEC_ENET_RXF is 3 separate bits, we only check one queue
at a time. So, when the last queue is being checked, it is
bad to remove the interrupt on the 1st queue.

Also, since this is now done in the napi
routine and not the interrupt, it is not needed.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 610cf6c..791f385 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1338,8 +1338,6 @@ static int fec_rxq(struct net_device *ndev, struct fec_enet_private *fep,
 			break;
 		pkt_received++;
 
-		writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT);
-
 		/* Check for errors. */
 		status ^= BD_ENET_RX_LAST;
 		if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO |
-- 
2.5.0

RE: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when just one is being checked

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com>Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when
> just one is being checked
> 
> FEC_ENET_RXF is 3 separate bits, we only check one queue at a time. So, when
> the last queue is being checked, it is bad to remove the interrupt on the 1st
> queue.
> 
> Also, since this is now done in the napi routine and not the interrupt, it is not
> needed.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 610cf6c..791f385 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1338,8 +1338,6 @@ static int fec_rxq(struct net_device *ndev, struct
> fec_enet_private *fep,
>  			break;
>  		pkt_received++;
> 
> -		writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT);
> -

We should clear the related rx queue ievent, not remove the code.
Pls see commit: db3421c114cf that was submitted by Russell King.

No ack the patch.

>  		/* Check for errors. */
>  		status ^= BD_ENET_RX_LAST;
>  		if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH |
> BD_ENET_RX_NO |
> --
> 2.5.0

Re: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when just one is being checked

[Troy Kisky]

On 3/4/2016 2:11 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com>Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when
>> just one is being checked
>>
>> FEC_ENET_RXF is 3 separate bits, we only check one queue at a time. So, when
>> the last queue is being checked, it is bad to remove the interrupt on the 1st
>> queue.
>>
>> Also, since this is now done in the napi routine and not the interrupt, it is not
>> needed.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 610cf6c..791f385 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1338,8 +1338,6 @@ static int fec_rxq(struct net_device *ndev, struct
>> fec_enet_private *fep,
>>  			break;
>>  		pkt_received++;
>>
>> -		writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT);
>> -
> 
> We should clear the related rx queue ievent, not remove the code.
> Pls see commit: db3421c114cf that was submitted by Russell King.
> 
> No ack the patch.


This is now done in patch #4 "net: fec: reduce interrupts" and you could argue
that it should be squashed into that patch. But I like separating changes
as much as possible.


Russell, this patch and patch #4 will likely need your ack before it will be applied.
Can you take a look please?

http://www.spinics.net/lists/netdev/msg361927.html


Thanks
Troy

Re: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when just one is being checked

[Russell King - ARM Linux]

On Fri, Mar 04, 2016 at 09:18:19AM -0700, Troy Kisky wrote:
> On 3/4/2016 2:11 AM, Fugang Duan wrote:
> > From: Troy Kisky <troy.kisky@boundarydevices.com>Sent: Thursday, February 25, 2016 8:37 AM
> >> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> >> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> >> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> >> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> >> johannes@sipsolutions.net; stillcompiling@gmail.com;
> >> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> >> <troy.kisky@boundarydevices.com>
> >> Subject: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when
> >> just one is being checked
> >>
> >> FEC_ENET_RXF is 3 separate bits, we only check one queue at a time. So, when
> >> the last queue is being checked, it is bad to remove the interrupt on the 1st
> >> queue.
> >>
> >> Also, since this is now done in the napi routine and not the interrupt, it is not
> >> needed.
> >>
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_main.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 610cf6c..791f385 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -1338,8 +1338,6 @@ static int fec_rxq(struct net_device *ndev, struct
> >> fec_enet_private *fep,
> >>  			break;
> >>  		pkt_received++;
> >>
> >> -		writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT);
> >> -
> > 
> > We should clear the related rx queue ievent, not remove the code.
> > Pls see commit: db3421c114cf that was submitted by Russell King.
> > 
> > No ack the patch.
> 
> 
> This is now done in patch #4 "net: fec: reduce interrupts" and you could argue
> that it should be squashed into that patch. But I like separating changes
> as much as possible.
> 
> 
> Russell, this patch and patch #4 will likely need your ack before it will be applied.
> Can you take a look please?

I stopped caring about the FEC ethernet driver about 18 months ago,
after I ended up dropping a significant pile of fixes on the floor
through the huge number of conflicts and the shere effort of
constantly trying to move them forward.

My patch series tend to be large because I put concentrated effort
into something for a month, which then gives a problem if conflicts
come up later and the series has to be effectively rewritten from
scratch.  It was after the second or third time of facing an almost
total rewrite that happened that I just gave up.

I've toyed with the idea of forking the driver, but I wouldn't have
time to maintain such a thing.  So, right now I just put up with all
the bad quirks, and reset/power cycle the boards when things go wrong.
Right now, I just disable runtime PM support on the FEC to get
stability here. :)

Sorry, but I can't be of more help.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when just one is being checked

[Troy Kisky]

On 3/4/2016 9:38 AM, Russell King - ARM Linux wrote:
> On Fri, Mar 04, 2016 at 09:18:19AM -0700, Troy Kisky wrote:
>> On 3/4/2016 2:11 AM, Fugang Duan wrote:
>>> From: Troy Kisky <troy.kisky@boundarydevices.com>Sent: Thursday, February 25, 2016 8:37 AM
>>>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>>>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>>>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>>>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>>>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>>>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>>>> <troy.kisky@boundarydevices.com>
>>>> Subject: [PATCH net-next V2 06/16] net: fec: don't clear all rx queue bits when
>>>> just one is being checked
>>>>
>>>> FEC_ENET_RXF is 3 separate bits, we only check one queue at a time. So, when
>>>> the last queue is being checked, it is bad to remove the interrupt on the 1st
>>>> queue.
>>>>
>>>> Also, since this is now done in the napi routine and not the interrupt, it is not
>>>> needed.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>> ---
>>>>  drivers/net/ethernet/freescale/fec_main.c | 2 --
>>>>  1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 610cf6c..791f385 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1338,8 +1338,6 @@ static int fec_rxq(struct net_device *ndev, struct
>>>> fec_enet_private *fep,
>>>>  			break;
>>>>  		pkt_received++;
>>>>
>>>> -		writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT);
>>>> -
>>>
>>> We should clear the related rx queue ievent, not remove the code.
>>> Pls see commit: db3421c114cf that was submitted by Russell King.
>>>
>>> No ack the patch.
>>
>>
>> This is now done in patch #4 "net: fec: reduce interrupts" and you could argue
>> that it should be squashed into that patch. But I like separating changes
>> as much as possible.
>>
>>
>> Russell, this patch and patch #4 will likely need your ack before it will be applied.
>> Can you take a look please?
> 
> I stopped caring about the FEC ethernet driver about 18 months ago,
> after I ended up dropping a significant pile of fixes on the floor
> through the huge number of conflicts and the shere effort of
> constantly trying to move them forward.
> 
> My patch series tend to be large because I put concentrated effort
> into something for a month, which then gives a problem if conflicts
> come up later and the series has to be effectively rewritten from
> scratch.  It was after the second or third time of facing an almost
> total rewrite that happened that I just gave up.
> 
> I've toyed with the idea of forking the driver, but I wouldn't have
> time to maintain such a thing.  So, right now I just put up with all
> the bad quirks, and reset/power cycle the boards when things go wrong.
> Right now, I just disable runtime PM support on the FEC to get
> stability here. :)
> 
> Sorry, but I can't be of more help.
> 


I can sympathize, I've been almost ready to post my patches numerous times
when a huge patch set would hit, and conflict everywhere. Including once
about 18 months ago :)

That's why I got trigger happy, and first posted my too large set before
net-next was opened. It didn't help though, there was already a conflict
in net.


Troy

[PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on previous value

[Troy Kisky]

Relying on the wrap bit to stay valid once initialized
when the controller also writes to this byte seems
undesirable since we can easily know what the value
should be.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++----------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 791f385..6ceb5f9 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -340,9 +340,8 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
 		ebdp = (struct bufdesc_ex *)bdp;
 
-		status = fec16_to_cpu(bdp->cbd_sc);
-		status &= ~BD_ENET_TX_STATS;
-		status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
+		status = BD_ENET_TX_TC | BD_ENET_TX_READY |
+				((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
 		frag_len = skb_shinfo(skb)->frags[frag].size;
 
 		/* Handle the last BD specially */
@@ -436,8 +435,6 @@ static int fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
 	/* Fill in a Tx ring entry */
 	bdp = txq->bd.cur;
 	last_bdp = bdp;
-	status = fec16_to_cpu(bdp->cbd_sc);
-	status &= ~BD_ENET_TX_STATS;
 
 	/* Set buffer length and buffer pointer */
 	bufaddr = skb->data;
@@ -462,6 +459,8 @@ static int fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
 		return NETDEV_TX_OK;
 	}
 
+	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
+			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
 	if (nr_frags) {
 		last_bdp = fec_enet_txq_submit_frag_skb(txq, skb, ndev);
 		if (IS_ERR(last_bdp)) {
@@ -512,7 +511,6 @@ static int fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
 	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
 	 * it's the last BD of the frame, and to put the CRC on the end.
 	 */
-	status |= (BD_ENET_TX_READY | BD_ENET_TX_TC);
 	bdp->cbd_sc = cpu_to_fec16(status);
 
 	/* If this was the last BD in the ring, start at the beginning again. */
@@ -544,11 +542,6 @@ fec_enet_txq_put_data_tso(struct fec_enet_priv_tx_q *txq, struct sk_buff *skb,
 	unsigned int estatus = 0;
 	dma_addr_t addr;
 
-	status = fec16_to_cpu(bdp->cbd_sc);
-	status &= ~BD_ENET_TX_STATS;
-
-	status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
-
 	if (((unsigned long) data) & fep->tx_align ||
 		fep->quirks & FEC_QUIRK_SWAP_FRAME) {
 		memcpy(txq->tx_bounce[index], data, size);
@@ -578,15 +571,16 @@ fec_enet_txq_put_data_tso(struct fec_enet_priv_tx_q *txq, struct sk_buff *skb,
 		ebdp->cbd_esc = cpu_to_fec32(estatus);
 	}
 
+	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
+			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
 	/* Handle the last BD specially */
 	if (last_tcp)
-		status |= (BD_ENET_TX_LAST | BD_ENET_TX_TC);
+		status |= BD_ENET_TX_LAST;
 	if (is_last) {
 		status |= BD_ENET_TX_INTR;
 		if (fep->bufdesc_ex)
 			ebdp->cbd_esc |= cpu_to_fec32(BD_ENET_TX_INT);
 	}
-
 	bdp->cbd_sc = cpu_to_fec16(status);
 
 	return 0;
@@ -602,13 +596,8 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq,
 	struct bufdesc_ex *ebdp = container_of(bdp, struct bufdesc_ex, desc);
 	void *bufaddr;
 	unsigned long dmabuf;
-	unsigned short status;
 	unsigned int estatus = 0;
 
-	status = fec16_to_cpu(bdp->cbd_sc);
-	status &= ~BD_ENET_TX_STATS;
-	status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
-
 	bufaddr = txq->tso_hdrs + index * TSO_HEADER_SIZE;
 	dmabuf = txq->tso_hdrs_dma + index * TSO_HEADER_SIZE;
 	if (((unsigned long)bufaddr) & fep->tx_align ||
@@ -641,8 +630,8 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq,
 		ebdp->cbd_esc = cpu_to_fec32(estatus);
 	}
 
-	bdp->cbd_sc = cpu_to_fec16(status);
-
+	bdp->cbd_sc = cpu_to_fec16(BD_ENET_TX_TC | BD_ENET_TX_READY |
+			((bdp == txq->bd.last) ? BD_SC_WRAP : 0));
 	return 0;
 }
 
@@ -1453,12 +1442,6 @@ static int fec_rxq(struct net_device *ndev, struct fec_enet_private *fep,
 		}
 
 rx_processing_done:
-		/* Clear the status flags for this buffer */
-		status &= ~BD_ENET_RX_STATS;
-
-		/* Mark the buffer empty */
-		status |= BD_ENET_RX_EMPTY;
-
 		if (fep->bufdesc_ex) {
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 
@@ -1470,7 +1453,8 @@ rx_processing_done:
 		 * performed before transferring ownership.
 		 */
 		wmb();
-		bdp->cbd_sc = cpu_to_fec16(status);
+		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY |
+				((bdp == rxq->bd.last) ? BD_SC_WRAP : 0));
 
 		/* Update BD pointer to next entry */
 		bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
-- 
2.5.0

RE: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on previous value

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on
> previous value
> 
> Relying on the wrap bit to stay valid once initialized when the controller also
> writes to this byte seems undesirable since we can easily know what the value
> should be.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++----------------------
>  1 file changed, 11 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 791f385..6ceb5f9 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -340,9 +340,8 @@ fec_enet_txq_submit_frag_skb(struct
> fec_enet_priv_tx_q *txq,
>  		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>  		ebdp = (struct bufdesc_ex *)bdp;
> 
> -		status = fec16_to_cpu(bdp->cbd_sc);
> -		status &= ~BD_ENET_TX_STATS;
> -		status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
> +		status = BD_ENET_TX_TC | BD_ENET_TX_READY |
> +				((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
>  		frag_len = skb_shinfo(skb)->frags[frag].size;
> 
>  		/* Handle the last BD specially */
> @@ -436,8 +435,6 @@ static int fec_enet_txq_submit_skb(struct
> fec_enet_priv_tx_q *txq,
>  	/* Fill in a Tx ring entry */
>  	bdp = txq->bd.cur;
>  	last_bdp = bdp;
> -	status = fec16_to_cpu(bdp->cbd_sc);
> -	status &= ~BD_ENET_TX_STATS;
> 
>  	/* Set buffer length and buffer pointer */
>  	bufaddr = skb->data;
> @@ -462,6 +459,8 @@ static int fec_enet_txq_submit_skb(struct
> fec_enet_priv_tx_q *txq,
>  		return NETDEV_TX_OK;
>  	}
> 
> +	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
> +			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
>  	if (nr_frags) {
>  		last_bdp = fec_enet_txq_submit_frag_skb(txq, skb, ndev);
>  		if (IS_ERR(last_bdp)) {
> @@ -512,7 +511,6 @@ static int fec_enet_txq_submit_skb(struct
> fec_enet_priv_tx_q *txq,
>  	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
>  	 * it's the last BD of the frame, and to put the CRC on the end.
>  	 */
> -	status |= (BD_ENET_TX_READY | BD_ENET_TX_TC);

This is completely error.  We have to prepare all BDs for frag skb, and then enable "READY" and "TC" bit for the first BD, otherwise uDMA copy un-correct data to fifo.

<snip>

Re: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on previous value

[Troy Kisky]

On 3/4/2016 2:29 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on
>> previous value
>>
>> Relying on the wrap bit to stay valid once initialized when the controller also
>> writes to this byte seems undesirable since we can easily know what the value
>> should be.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++----------------------
>>  1 file changed, 11 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 791f385..6ceb5f9 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -340,9 +340,8 @@ fec_enet_txq_submit_frag_skb(struct
>> fec_enet_priv_tx_q *txq,
>>  		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>>  		ebdp = (struct bufdesc_ex *)bdp;
>>
>> -		status = fec16_to_cpu(bdp->cbd_sc);
>> -		status &= ~BD_ENET_TX_STATS;
>> -		status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
>> +		status = BD_ENET_TX_TC | BD_ENET_TX_READY |
>> +				((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
>>  		frag_len = skb_shinfo(skb)->frags[frag].size;
>>
>>  		/* Handle the last BD specially */
>> @@ -436,8 +435,6 @@ static int fec_enet_txq_submit_skb(struct
>> fec_enet_priv_tx_q *txq,
>>  	/* Fill in a Tx ring entry */
>>  	bdp = txq->bd.cur;
>>  	last_bdp = bdp;
>> -	status = fec16_to_cpu(bdp->cbd_sc);
>> -	status &= ~BD_ENET_TX_STATS;
>>
>>  	/* Set buffer length and buffer pointer */
>>  	bufaddr = skb->data;
>> @@ -462,6 +459,8 @@ static int fec_enet_txq_submit_skb(struct
>> fec_enet_priv_tx_q *txq,
>>  		return NETDEV_TX_OK;
>>  	}
>>
>> +	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
>> +			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
>>  	if (nr_frags) {
>>  		last_bdp = fec_enet_txq_submit_frag_skb(txq, skb, ndev);
>>  		if (IS_ERR(last_bdp)) {
>> @@ -512,7 +511,6 @@ static int fec_enet_txq_submit_skb(struct
>> fec_enet_priv_tx_q *txq,
>>  	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
>>  	 * it's the last BD of the frame, and to put the CRC on the end.
>>  	 */
>> -	status |= (BD_ENET_TX_READY | BD_ENET_TX_TC);
> 
> This is completely error.  We have to prepare all BDs for frag skb, and then enable "READY" and "TC" bit for the first BD, otherwise uDMA copy un-correct data to fifo.
> 




I don't follow. Please read patch again.


Thanks
Troy

RE: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on previous value

[Fugang Duan]

 From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Saturday, March 05, 2016 12:08 AM
> To: Fugang Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org;
> davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de
> Subject: Re: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on
> previous value
> 
> On 3/4/2016 2:29 AM, Fugang Duan wrote:
> > From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday,
> > February 25, 2016 8:37 AM
> >> To: netdev@vger.kernel.org; davem@davemloft.net;
> b38611@freescale.com
> >> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de;
> >> andrew@lunn.ch; tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> >> kernel@lists.infradead.org; laci@boundarydevices.com;
> >> shawnguo@kernel.org; johannes@sipsolutions.net;
> >> stillcompiling@gmail.com; sergei.shtylyov@cogentembedded.com;
> >> arnd@arndb.de; Troy Kisky <troy.kisky@boundarydevices.com>
> >> Subject: [PATCH net-next V2 07/16] net: fec: set cbd_sc without
> >> relying on previous value
> >>
> >> Relying on the wrap bit to stay valid once initialized when the
> >> controller also writes to this byte seems undesirable since we can
> >> easily know what the value should be.
> >>
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_main.c | 38
> >> +++++++++----------------------
> >>  1 file changed, 11 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 791f385..6ceb5f9 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -340,9 +340,8 @@ fec_enet_txq_submit_frag_skb(struct
> >> fec_enet_priv_tx_q *txq,
> >>  		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> >>  		ebdp = (struct bufdesc_ex *)bdp;
> >>
> >> -		status = fec16_to_cpu(bdp->cbd_sc);
> >> -		status &= ~BD_ENET_TX_STATS;
> >> -		status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
> >> +		status = BD_ENET_TX_TC | BD_ENET_TX_READY |
> >> +				((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
> >>  		frag_len = skb_shinfo(skb)->frags[frag].size;
> >>
> >>  		/* Handle the last BD specially */ @@ -436,8 +435,6 @@ static
> int
> >> fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
> >>  	/* Fill in a Tx ring entry */
> >>  	bdp = txq->bd.cur;
> >>  	last_bdp = bdp;
> >> -	status = fec16_to_cpu(bdp->cbd_sc);
> >> -	status &= ~BD_ENET_TX_STATS;
> >>
> >>  	/* Set buffer length and buffer pointer */
> >>  	bufaddr = skb->data;
> >> @@ -462,6 +459,8 @@ static int fec_enet_txq_submit_skb(struct
> >> fec_enet_priv_tx_q *txq,
> >>  		return NETDEV_TX_OK;
> >>  	}
> >>
> >> +	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
> >> +			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
> >>  	if (nr_frags) {
> >>  		last_bdp = fec_enet_txq_submit_frag_skb(txq, skb, ndev);
> >>  		if (IS_ERR(last_bdp)) {
> >> @@ -512,7 +511,6 @@ static int fec_enet_txq_submit_skb(struct
> >> fec_enet_priv_tx_q *txq,
> >>  	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
> >>  	 * it's the last BD of the frame, and to put the CRC on the end.
> >>  	 */
> >> -	status |= (BD_ENET_TX_READY | BD_ENET_TX_TC);
> >
> > This is completely error.  We have to prepare all BDs for frag skb, and then
> enable "READY" and "TC" bit for the first BD, otherwise uDMA copy un-correct
> data to fifo.
> >
> 
> 
> 
> 
> I don't follow. Please read patch again.
> 
Understand, sorry, I take one mistake.  The patch is fine for me.

[PATCH net-next V2 08/16] net: fec: eliminate calls to fec_enet_get_prevdesc

[Troy Kisky]

This shrinks the code a little.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 37 +++++++++----------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 6ceb5f9..b5ed287 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -758,6 +758,7 @@ static void fec_enet_bd_init(struct net_device *dev)
 	struct bufdesc *bdp;
 	unsigned int i;
 	unsigned int q;
+	unsigned status;
 
 	for (q = 0; q < fep->num_rx_queues; q++) {
 		/* Initialize the receive buffer descriptors. */
@@ -765,19 +766,13 @@ static void fec_enet_bd_init(struct net_device *dev)
 		bdp = rxq->bd.base;
 
 		for (i = 0; i < rxq->bd.ring_size; i++) {
-
 			/* Initialize the BD for every fragment in the page. */
-			if (bdp->cbd_bufaddr)
-				bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY);
-			else
-				bdp->cbd_sc = cpu_to_fec16(0);
+			status = bdp->cbd_bufaddr ? BD_ENET_RX_EMPTY : 0;
+			if (bdp == rxq->bd.last)
+				status |= BD_SC_WRAP;
+			bdp->cbd_sc = cpu_to_fec16(status);
 			bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
 		}
-
-		/* Set the last buffer to wrap */
-		bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
-		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
-
 		rxq->bd.cur = rxq->bd.base;
 	}
 
@@ -789,18 +784,16 @@ static void fec_enet_bd_init(struct net_device *dev)
 
 		for (i = 0; i < txq->bd.ring_size; i++) {
 			/* Initialize the BD for every fragment in the page. */
-			bdp->cbd_sc = cpu_to_fec16(0);
 			if (txq->tx_skbuff[i]) {
 				dev_kfree_skb_any(txq->tx_skbuff[i]);
 				txq->tx_skbuff[i] = NULL;
 			}
 			bdp->cbd_bufaddr = cpu_to_fec32(0);
+			bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
+					BD_SC_WRAP : 0);
 			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
 		}
-
-		/* Set the last buffer to wrap */
 		bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
-		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
 		txq->dirty_tx = bdp;
 	}
 }
@@ -2717,19 +2710,16 @@ fec_enet_alloc_rxq_buffers(struct net_device *ndev, unsigned int queue)
 		}
 
 		rxq->rx_skbuff[i] = skb;
-		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY);
 
 		if (fep->bufdesc_ex) {
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 			ebdp->cbd_esc = cpu_to_fec32(BD_ENET_RX_INT);
 		}
+		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY |
+				((bdp == rxq->bd.last) ? BD_SC_WRAP : 0));
 
 		bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
 	}
-
-	/* Set the last buffer to wrap. */
-	bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
-	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
 	return 0;
 
  err_alloc:
@@ -2752,21 +2742,16 @@ fec_enet_alloc_txq_buffers(struct net_device *ndev, unsigned int queue)
 		if (!txq->tx_bounce[i])
 			goto err_alloc;
 
-		bdp->cbd_sc = cpu_to_fec16(0);
 		bdp->cbd_bufaddr = cpu_to_fec32(0);
 
 		if (fep->bufdesc_ex) {
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 			ebdp->cbd_esc = cpu_to_fec32(BD_ENET_TX_INT);
 		}
-
+		bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
+				BD_SC_WRAP : 0);
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
 	}
-
-	/* Set the last buffer to wrap. */
-	bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
-	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
-
 	return 0;
 
  err_alloc:
-- 
2.5.0

RE: [PATCH net-next V2 08/16] net: fec: eliminate calls to fec_enet_get_prevdesc

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 08/16] net: fec: eliminate calls to
> fec_enet_get_prevdesc
> 
> This shrinks the code a little.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 37 +++++++++----------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 6ceb5f9..b5ed287 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -758,6 +758,7 @@ static void fec_enet_bd_init(struct net_device *dev)
>  	struct bufdesc *bdp;
>  	unsigned int i;
>  	unsigned int q;
> +	unsigned status;

Should be unsigned int status;

> 
>  	for (q = 0; q < fep->num_rx_queues; q++) {
>  		/* Initialize the receive buffer descriptors. */ @@ -765,19
> +766,13 @@ static void fec_enet_bd_init(struct net_device *dev)
>  		bdp = rxq->bd.base;
> 
>  		for (i = 0; i < rxq->bd.ring_size; i++) {
> -
>  			/* Initialize the BD for every fragment in the page. */
> -			if (bdp->cbd_bufaddr)
> -				bdp->cbd_sc =
> cpu_to_fec16(BD_ENET_RX_EMPTY);
> -			else
> -				bdp->cbd_sc = cpu_to_fec16(0);
> +			status = bdp->cbd_bufaddr ? BD_ENET_RX_EMPTY : 0;
> +			if (bdp == rxq->bd.last)
> +				status |= BD_SC_WRAP;
> +			bdp->cbd_sc = cpu_to_fec16(status);
>  			bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
>  		}
> -
> -		/* Set the last buffer to wrap */
> -		bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
> -		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> -
>  		rxq->bd.cur = rxq->bd.base;
>  	}
> 
> @@ -789,18 +784,16 @@ static void fec_enet_bd_init(struct net_device *dev)
> 
>  		for (i = 0; i < txq->bd.ring_size; i++) {
>  			/* Initialize the BD for every fragment in the page. */
> -			bdp->cbd_sc = cpu_to_fec16(0);
>  			if (txq->tx_skbuff[i]) {
>  				dev_kfree_skb_any(txq->tx_skbuff[i]);
>  				txq->tx_skbuff[i] = NULL;
>  			}
>  			bdp->cbd_bufaddr = cpu_to_fec32(0);
> +			bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
> +					BD_SC_WRAP : 0);
>  			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>  		}
> -
> -		/* Set the last buffer to wrap */
>  		bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
> -		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
>  		txq->dirty_tx = bdp;
>  	}
>  }
> @@ -2717,19 +2710,16 @@ fec_enet_alloc_rxq_buffers(struct net_device
> *ndev, unsigned int queue)
>  		}
> 
>  		rxq->rx_skbuff[i] = skb;
> -		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY);
> 
>  		if (fep->bufdesc_ex) {
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>  			ebdp->cbd_esc = cpu_to_fec32(BD_ENET_RX_INT);
>  		}
> +		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY |
> +				((bdp == rxq->bd.last) ? BD_SC_WRAP : 0));
> 
>  		bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
>  	}
> -
> -	/* Set the last buffer to wrap. */
> -	bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
> -	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
>  	return 0;
> 
>   err_alloc:
> @@ -2752,21 +2742,16 @@ fec_enet_alloc_txq_buffers(struct net_device
> *ndev, unsigned int queue)
>  		if (!txq->tx_bounce[i])
>  			goto err_alloc;
> 
> -		bdp->cbd_sc = cpu_to_fec16(0);
>  		bdp->cbd_bufaddr = cpu_to_fec32(0);
> 
>  		if (fep->bufdesc_ex) {
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>  			ebdp->cbd_esc = cpu_to_fec32(BD_ENET_TX_INT);
>  		}
> -
> +		bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
> +				BD_SC_WRAP : 0);
>  		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>  	}
> -
> -	/* Set the last buffer to wrap. */
> -	bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
> -	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> -
>  	return 0;
> 
>   err_alloc:
> --
> 2.5.0

Re: [PATCH net-next V2 08/16] net: fec: eliminate calls to fec_enet_get_prevdesc

[Troy Kisky]

On 3/4/2016 2:33 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 08/16] net: fec: eliminate calls to
>> fec_enet_get_prevdesc
>>
>> This shrinks the code a little.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 37 +++++++++----------------------
>>  1 file changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 6ceb5f9..b5ed287 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -758,6 +758,7 @@ static void fec_enet_bd_init(struct net_device *dev)
>>  	struct bufdesc *bdp;
>>  	unsigned int i;
>>  	unsigned int q;
>> +	unsigned status;
> 
> Should be unsigned int status;
> 


Fine



Thanks

[PATCH net-next V2 09/16] net: fec: move restart test for efficiency

[Troy Kisky]

This saves 1 comparison.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b5ed287..1902897 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1157,8 +1157,13 @@ static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
 		/* Order the load of bd.cur and cbd_sc */
 		rmb();
 		status = fec16_to_cpu(READ_ONCE(bdp->cbd_sc));
-		if (status & BD_ENET_TX_READY)
+		if (status & BD_ENET_TX_READY) {
+			if (!readl(txq->bd.reg_desc_active)) {
+				/* ERR006358 has hit, restart tx */
+				writel(0, txq->bd.reg_desc_active);
+			}
 			break;
+		}
 
 		index = fec_enet_get_bd_index(bdp, &txq->bd);
 
@@ -1230,11 +1235,6 @@ static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
 				netif_tx_wake_queue(nq);
 		}
 	}
-
-	/* ERR006538: Keep the transmitter going */
-	if (bdp != txq->bd.cur &&
-	    readl(txq->bd.reg_desc_active) == 0)
-		writel(0, txq->bd.reg_desc_active);
 }
 
 static int
-- 
2.5.0

[PATCH net-next V2 10/16] net: fec: clear cbd_sc after transmission to help with debugging

[Troy Kisky]

When the tx queue is dumped, it is easier to see that this
entry is idle.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1902897..96a0394 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1164,6 +1164,8 @@ static void fec_txq(struct net_device *ndev, struct fec_enet_private *fep,
 			}
 			break;
 		}
+		bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
+					   BD_SC_WRAP : 0);
 
 		index = fec_enet_get_bd_index(bdp, &txq->bd);
 
-- 
2.5.0

[PATCH net-next V2 11/16] net: fec: dump all tx queues in fec_dump

[Troy Kisky]

Dump all tx queues, not just queue 0.
Also, disable fec interrupts first.
The interrupts will be reenabled in fec_restart.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 40 +++++++++++++++++--------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 96a0394..9b24669 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -271,28 +271,32 @@ static void swap_buffer2(void *dst_buf, void *src_buf, int len)
 static void fec_dump(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct bufdesc *bdp;
-	struct fec_enet_priv_tx_q *txq;
-	int index = 0;
+	int i;
 
+	/* Disable all FEC interrupts */
+	writel(0, fep->hwp + FEC_IMASK);
 	netdev_info(ndev, "TX ring dump\n");
 	pr_info("Nr     SC     addr       len  SKB\n");
 
-	txq = fep->tx_queue[0];
-	bdp = txq->bd.base;
-
-	do {
-		pr_info("%3u %c%c 0x%04x 0x%08x %4u %p\n",
-			index,
-			bdp == txq->bd.cur ? 'S' : ' ',
-			bdp == txq->dirty_tx ? 'H' : ' ',
-			fec16_to_cpu(bdp->cbd_sc),
-			fec32_to_cpu(bdp->cbd_bufaddr),
-			fec16_to_cpu(bdp->cbd_datlen),
-			txq->tx_skbuff[index]);
-		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
-		index++;
-	} while (bdp != txq->bd.base);
+	for (i = 0; i < fep->num_tx_queues; i++) {
+		struct fec_enet_priv_tx_q *txq = fep->tx_queue[i];
+		struct bufdesc *bdp = txq->bd.base;
+		int index = 0;
+
+		pr_info("tx queue %d\n", i);
+		do {
+			pr_info("%3u %c%c 0x%04x 0x%08x %4u %p\n",
+				index,
+				bdp == txq->bd.cur ? 'S' : ' ',
+				bdp == txq->dirty_tx ? 'H' : ' ',
+				fec16_to_cpu(bdp->cbd_sc),
+				fec32_to_cpu(bdp->cbd_bufaddr),
+				fec16_to_cpu(bdp->cbd_datlen),
+				txq->tx_skbuff[index]);
+			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
+			index++;
+		} while (bdp != txq->bd.base);
+	}
 }
 
 static inline bool is_ipv4_pkt(struct sk_buff *skb)
-- 
2.5.0

[PATCH net-next V2 12/16] net: fec: detect tx int lost

[Troy Kisky]

If a tx int is lost, no need to reset
the fec. Just mark the event and call napi_schedule.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 38 ++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9b24669..afd4060 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1094,14 +1094,50 @@ fec_stop(struct net_device *ndev)
 	}
 }
 
+static const uint txint_flags[] = {
+	FEC_ENET_TXF_0, FEC_ENET_TXF_1, FEC_ENET_TXF_2
+};
 
 static void
 fec_timeout(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct bufdesc *bdp;
+	unsigned short status;
+	int i;
+	uint events = 0;
 
-	fec_dump(ndev);
+	for (i = 0; i < fep->num_tx_queues; i++) {
+		struct fec_enet_priv_tx_q *txq = fep->tx_queue[i];
+		int index;
+		struct sk_buff *skb = NULL;
 
+		bdp = txq->dirty_tx;
+		while (1) {
+			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
+			if (bdp == txq->bd.cur)
+				break;
+			index = fec_enet_get_bd_index(bdp, &txq->bd);
+			skb = txq->tx_skbuff[index];
+			if (skb) {
+				status = fec16_to_cpu(bdp->cbd_sc);
+				if ((status & BD_ENET_TX_READY) == 0)
+					events |= txint_flags[i];
+				break;
+			}
+		}
+	}
+	if (events) {
+		fep->events |= events;
+		/* Disable the RX/TX interrupt */
+		writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
+		napi_schedule(&fep->napi);
+		netif_wake_queue(fep->netdev);
+		pr_err("%s: tx int lost\n", __func__);
+		return;
+	}
+
+	fec_dump(ndev);
 	ndev->stats.tx_errors++;
 
 	schedule_work(&fep->tx_timeout_work);
-- 
2.5.0

[PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Troy Kisky]

Print the current interrupt flags and mask and
the interrupt state during the last interrupt in
fec_timeout.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec.h      | 1 +
 drivers/net/ethernet/freescale/fec_main.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 001200b..615cca1 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -506,6 +506,7 @@ struct fec_enet_private {
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
 	uint	events;
+	uint	last_ievents;
 
 	struct	platform_device *pdev;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index afd4060..9a3136b 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
 	int i;
 	uint events = 0;
 
+	pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
+	       readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
+
 	for (i = 0; i < fep->num_tx_queues; i++) {
 		struct fec_enet_priv_tx_q *txq = fep->tx_queue[i];
 		int index;
@@ -1514,6 +1517,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 
 	if (!int_events)
 		return IRQ_NONE;
+	fep->last_ievents = int_events;
 
 	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
 		if (napi_schedule_prep(&fep->napi)) {
-- 
2.5.0

RE: [PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Fugang Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
> johannes@sipsolutions.net; stillcompiling@gmail.com;
> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
> <troy.kisky@boundarydevices.com>
> Subject: [PATCH net-next V2 13/16] net: fec: print more debug info in
> fec_timeout
> 
> Print the current interrupt flags and mask and the interrupt state during the last
> interrupt in fec_timeout.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      | 1 +
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 001200b..615cca1 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -506,6 +506,7 @@ struct fec_enet_private {
>  	unsigned int total_tx_ring_size;
>  	unsigned int total_rx_ring_size;
>  	uint	events;
> +	uint	last_ievents;
> 
>  	struct	platform_device *pdev;
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index afd4060..9a3136b 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
>  	int i;
>  	uint events = 0;
> 
> +	pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
> +	       readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
> +
pr_err() -> netdev_err()

I don't want the debug patch enter to the driver.

>  	for (i = 0; i < fep->num_tx_queues; i++) {
>  		struct fec_enet_priv_tx_q *txq = fep->tx_queue[i];
>  		int index;
> @@ -1514,6 +1517,7 @@ fec_enet_interrupt(int irq, void *dev_id)
> 
>  	if (!int_events)
>  		return IRQ_NONE;
> +	fep->last_ievents = int_events;
> 
>  	if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
>  		if (napi_schedule_prep(&fep->napi)) {
> --
> 2.5.0

Re: [PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Troy Kisky]

On 3/4/2016 3:06 AM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
>> To: netdev@vger.kernel.org; davem@davemloft.net; b38611@freescale.com
>> Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
>> tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
>> kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
>> johannes@sipsolutions.net; stillcompiling@gmail.com;
>> sergei.shtylyov@cogentembedded.com; arnd@arndb.de; Troy Kisky
>> <troy.kisky@boundarydevices.com>
>> Subject: [PATCH net-next V2 13/16] net: fec: print more debug info in
>> fec_timeout
>>
>> Print the current interrupt flags and mask and the interrupt state during the last
>> interrupt in fec_timeout.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/net/ethernet/freescale/fec.h      | 1 +
>>  drivers/net/ethernet/freescale/fec_main.c | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec.h
>> b/drivers/net/ethernet/freescale/fec.h
>> index 001200b..615cca1 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -506,6 +506,7 @@ struct fec_enet_private {
>>  	unsigned int total_tx_ring_size;
>>  	unsigned int total_rx_ring_size;
>>  	uint	events;
>> +	uint	last_ievents;
>>
>>  	struct	platform_device *pdev;
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index afd4060..9a3136b 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
>>  	int i;
>>  	uint events = 0;
>>
>> +	pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
>> +	       readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
>> +
> pr_err() -> netdev_err()
> 


Sounds good

Re: [PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Joe Perches]

On Fri, 2016-03-04 at 09:05 -0700, Troy Kisky wrote:
> On 3/4/2016 3:06 AM, Fugang Duan wrote:
> > From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
[]
> > > Print the current interrupt flags and mask and the interrupt state during the last
> > > interrupt in fec_timeout.
[]
> > > diff --git a/drivers/net/ethernet/freescale/fec_main.c
[]
> > > @@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
> > >  	int i;
> > >  	uint events = 0;
> > > 
> > > +	pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
> > > +	       readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
> > > +
> > pr_err() -> netdev_err()
> Sounds good

This seems like debugging information rather than
an error a user can do anything with and if there's
a timeout, how likely is it that the hardware is
hosed and this would  repetitively and unnecessarily
fill up logs?

So maybe netdev_dbg and net_ratelimit() too.

	if (net_ratelimit()
		netdev_<level>(etc...)

Re: [PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Troy Kisky]

On 3/4/2016 10:35 AM, Joe Perches wrote:
> On Fri, 2016-03-04 at 09:05 -0700, Troy Kisky wrote:
>> On 3/4/2016 3:06 AM, Fugang Duan wrote:
>>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, February 25, 2016 8:37 AM
> []
>>>> Print the current interrupt flags and mask and the interrupt state during the last
>>>> interrupt in fec_timeout.
> []
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> []
>>>> @@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
>>>>  	int i;
>>>>  	uint events = 0;
>>>>
>>>> +	pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
>>>> +	       readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
>>>> +
>>> pr_err() -> netdev_err()
>> Sounds good
> 
> This seems like debugging information rather than
> an error a user can do anything with and if there's
> a timeout, how likely is it that the hardware is
> hosed and this would  repetitively and unnecessarily
> fill up logs?
> 
> So maybe netdev_dbg and net_ratelimit() too.
> 
> 	if (net_ratelimit()
> 		netdev_<level>(etc...)
> 
> 

This patch hasn't been helpful to me in quite a while. I'll just drop it.
I know where to get it if I need it again.


Thanks
Troy

[PATCH net-next V2 14/16] net: fec: create subroutine reset_tx_queue

[Troy Kisky]

This creates one place to release any queued
tx skbs.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++----------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9a3136b..8711196 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -752,12 +752,33 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	return NETDEV_TX_OK;
 }
 
+static void reset_tx_queue(struct fec_enet_private *fep,
+			   struct fec_enet_priv_tx_q *txq)
+{
+	struct bufdesc *bdp = txq->bd.base;
+	unsigned int i;
+
+	txq->bd.cur = bdp;
+	for (i = 0; i < txq->bd.ring_size; i++) {
+		/* Initialize the BD for every fragment in the page. */
+		if (txq->tx_skbuff[i]) {
+			dev_kfree_skb_any(txq->tx_skbuff[i]);
+			txq->tx_skbuff[i] = NULL;
+		}
+		bdp->cbd_bufaddr = cpu_to_fec32(0);
+		bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
+					   BD_SC_WRAP : 0);
+		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
+	}
+	bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
+	txq->dirty_tx = bdp;
+}
+
 /* Init RX & TX buffer descriptors
  */
 static void fec_enet_bd_init(struct net_device *dev)
 {
 	struct fec_enet_private *fep = netdev_priv(dev);
-	struct fec_enet_priv_tx_q *txq;
 	struct fec_enet_priv_rx_q *rxq;
 	struct bufdesc *bdp;
 	unsigned int i;
@@ -780,26 +801,8 @@ static void fec_enet_bd_init(struct net_device *dev)
 		rxq->bd.cur = rxq->bd.base;
 	}
 
-	for (q = 0; q < fep->num_tx_queues; q++) {
-		/* ...and the same for transmit */
-		txq = fep->tx_queue[q];
-		bdp = txq->bd.base;
-		txq->bd.cur = bdp;
-
-		for (i = 0; i < txq->bd.ring_size; i++) {
-			/* Initialize the BD for every fragment in the page. */
-			if (txq->tx_skbuff[i]) {
-				dev_kfree_skb_any(txq->tx_skbuff[i]);
-				txq->tx_skbuff[i] = NULL;
-			}
-			bdp->cbd_bufaddr = cpu_to_fec32(0);
-			bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
-					BD_SC_WRAP : 0);
-			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
-		}
-		bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
-		txq->dirty_tx = bdp;
-	}
+	for (q = 0; q < fep->num_tx_queues; q++)
+		reset_tx_queue(fep, fep->tx_queue[q]);
 }
 
 static void fec_enet_active_rxring(struct net_device *ndev)
@@ -2652,13 +2655,10 @@ static void fec_enet_free_buffers(struct net_device *ndev)
 
 	for (q = 0; q < fep->num_tx_queues; q++) {
 		txq = fep->tx_queue[q];
-		bdp = txq->bd.base;
+		reset_tx_queue(fep, txq);
 		for (i = 0; i < txq->bd.ring_size; i++) {
 			kfree(txq->tx_bounce[i]);
 			txq->tx_bounce[i] = NULL;
-			skb = txq->tx_skbuff[i];
-			txq->tx_skbuff[i] = NULL;
-			dev_kfree_skb(skb);
 		}
 	}
 }
-- 
2.5.0

[PATCH net-next V2 15/16] net: fec: call dma_unmap_single on mapped tx buffers at restart

[Troy Kisky]

Make sure any pending tx buffers are unmapped when the
fec is restarted.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 8711196..1d71b2e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -404,6 +404,7 @@ dma_mapping_error:
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
 		dma_unmap_single(&fep->pdev->dev, fec32_to_cpu(bdp->cbd_bufaddr),
 				 fec16_to_cpu(bdp->cbd_datlen), DMA_TO_DEVICE);
+		bdp->cbd_bufaddr = cpu_to_fec32(0);
 	}
 	return ERR_PTR(-ENOMEM);
 }
@@ -761,11 +762,18 @@ static void reset_tx_queue(struct fec_enet_private *fep,
 	txq->bd.cur = bdp;
 	for (i = 0; i < txq->bd.ring_size; i++) {
 		/* Initialize the BD for every fragment in the page. */
+		if (bdp->cbd_bufaddr) {
+			if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
+				dma_unmap_single(&fep->pdev->dev,
+						 fec32_to_cpu(bdp->cbd_bufaddr),
+						 fec16_to_cpu(bdp->cbd_datlen),
+						 DMA_TO_DEVICE);
+			bdp->cbd_bufaddr = cpu_to_fec32(0);
+		}
 		if (txq->tx_skbuff[i]) {
 			dev_kfree_skb_any(txq->tx_skbuff[i]);
 			txq->tx_skbuff[i] = NULL;
 		}
-		bdp->cbd_bufaddr = cpu_to_fec32(0);
 		bdp->cbd_sc = cpu_to_fec16((bdp == txq->bd.last) ?
 					   BD_SC_WRAP : 0);
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
@@ -2647,6 +2655,7 @@ static void fec_enet_free_buffers(struct net_device *ndev)
 						 fec32_to_cpu(bdp->cbd_bufaddr),
 						 FEC_ENET_RX_FRSIZE - fep->rx_align,
 						 DMA_FROM_DEVICE);
+				bdp->cbd_bufaddr = cpu_to_fec32(0);
 				dev_kfree_skb(skb);
 			}
 			bdp = fec_enet_get_nextdesc(bdp, &rxq->bd);
-- 
2.5.0

[PATCH net-next V2 16/16] net: fec: don't set cbd_bufaddr unless no mapping error

[Troy Kisky]

Not assigning cbd_bufaddr on error will prevent trying to
unmap the error in case the FEC is reset.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1d71b2e..c443a96 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -600,7 +600,7 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq,
 	int hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 	struct bufdesc_ex *ebdp = container_of(bdp, struct bufdesc_ex, desc);
 	void *bufaddr;
-	unsigned long dmabuf;
+	dma_addr_t dmabuf;
 	unsigned int estatus = 0;
 
 	bufaddr = txq->tso_hdrs + index * TSO_HEADER_SIZE;
@@ -1298,17 +1298,21 @@ fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct sk_buff
 {
 	struct  fec_enet_private *fep = netdev_priv(ndev);
 	int off;
+	dma_addr_t dmabuf;
 
 	off = ((unsigned long)skb->data) & fep->rx_align;
 	if (off)
 		skb_reserve(skb, fep->rx_align + 1 - off);
 
-	bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));
-	if (dma_mapping_error(&fep->pdev->dev, fec32_to_cpu(bdp->cbd_bufaddr))) {
+	dmabuf = dma_map_single(&fep->pdev->dev, skb->data,
+				FEC_ENET_RX_FRSIZE - fep->rx_align,
+				DMA_FROM_DEVICE);
+	if (dma_mapping_error(&fep->pdev->dev, dmabuf)) {
 		if (net_ratelimit())
 			netdev_err(ndev, "Rx DMA memory map failed\n");
 		return -ENOMEM;
 	}
+	bdp->cbd_bufaddr = cpu_to_fec32(dmabuf);
 
 	return 0;
 }
-- 
2.5.0

Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Joshua Clayton]

Hello Troy,
I'm replying here instead of to a particular commit because several of
the commit messages seem inadequate.

The first line summaries all look good.

The descriptions should each also include the "user visible impact" of
the patch and the justification for it (i.e. why you made the change).

For instance, patch 3 doesn't include either what will change
 (nothing, I'm guessing?) or why we now pass in the structures
instead of a queue_id. 

You've also got a few (e.g. patch 9, patch 14) where the substance
of the patch is in the summary,

but missing from the message.

These kind of descriptions are very hard to review since the expression
is split between the subject of the email and the body of the email, which
are not close
together in some email programs.

Better to reiterate or elaborate on the summary in the message.
In patch 9, for instance, it would be more clear to say:

Move restart test to earlier in fec_txq() which saves one comparison. 

On Wednesday, February 24, 2016 05:36:43 PM Troy Kisky wrote:
> V2 is a rebase on top of johannes endian-safe patch and
> this set is only the next 16 patches.
> The testing for this series was done on a nitrogen6x.
> The base commit was
> commit f5461c27631672b9e95282812ee521c53f502eca
>     Merge branch 'dsa-pass-bridge-to-drivers'
> 
> Testing showed no change in performance.
> Testing used imx_v6_v7_defconfig + CONFIG_MICREL_PHY.
> The processor was running at 996Mhz.
> The following commands were used to get the transfer rates.
> 
> On an x86 ubunto system,
> iperf -s -i.5 -u
> 
> 
> On a nitrogen6x board, running via SD Card.
> I first stopped some background processes
> 
> stop cron
> stop upstart-file-bridge
> stop upstart-socket-bridge
> stop upstart-udev-bridge
> stop rsyslog
> stop dbus
> killall dhclient
> echo performance >/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> 
> taskset 0x2 iperf -c 192.168.0.201 -u -t60 -b500M -r
> 
> There is a branch available on github with this series, and the rest of
> my fec patches, for those who would like to test it.
> https://github.com:boundarydevices/linux-imx6.git branch net-next_master
> 
> 
> 
> Troy Kisky (16):
> SDCard
>  TX/RX
> 379/402	Before any patches
> 376/402  net: fec: only check queue 0 if RXF_0/TXF_0 interrupt is set
> 379/399  net: fec: pass rxq to fec_enet_rx_queue instead of queue_id
> 379/401  net: fec: pass txq to fec_enet_tx_queue instead of queue_id
> 389/409  net: fec: reduce interrupts
> 393/411  net: fec: split off napi routine with 3 queues
> 386/415  net: fec: don't clear all rx queue bits when just one is being checked
> 387/412  net: fec: set cbd_sc without relying on previous value
> 385/415  net: fec: eliminate calls to fec_enet_get_prevdesc
> 389/414  net: fec: move restart test for efficiency
> 386/412  net: fec: clear cbd_sc after transmission to help with debugging
> 387/415  net: fec: dump all tx queues in fec_dump
> 380/418  net: fec: detect tx int lost
> 385/417  net: fec: print more debug info in fec_timeout
> 388/417  net: fec: create subroutine reset_tx_queue
> 389/418  net: fec: call dma_unmap_single on mapped tx buffers at restart
> 381/414  net: fec: don't set cbd_bufaddr unless no mapping error
> 
>  drivers/net/ethernet/freescale/fec.h      |   7 +-
>  drivers/net/ethernet/freescale/fec_main.c | 420 ++++++++++++++++--------------
>  2 files changed, 224 insertions(+), 203 deletions(-)
> 
> 

Hope this helps,
Joshua

P.S I'm a little confused, as I came looking for a v3 of the first 8 patches
and found these instead. I'll try to give your first 8 a look when they show up.

Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Troy Kisky]

On 2/24/2016 7:52 PM, Joshua Clayton wrote:
> Hello Troy,
> I'm replying here instead of to a particular commit because several of
> the commit messages seem inadequate.
> 
> The first line summaries all look good.
> 
> The descriptions should each also include the "user visible impact" of
> the patch and the justification for it (i.e. why you made the change).
> 
> For instance, patch 3 doesn't include either what will change
>  (nothing, I'm guessing?) or why we now pass in the structures
> instead of a queue_id. 

I can add to the commit message, that this is in preparation for
patch 4 which depends on it. Or I could squash patches 2/3/4
together, but I think it is easier to review smaller patches.


> 
> You've also got a few (e.g. patch 9, patch 14) where the substance
> of the patch is in the summary,
> 
> but missing from the message.
> 
> These kind of descriptions are very hard to review since the expression
> is split between the subject of the email and the body of the email, which
> are not close
> together in some email programs.
> 
> Better to reiterate or elaborate on the summary in the message.
> In patch 9, for instance, it would be more clear to say:
> 
> Move restart test to earlier in fec_txq() which saves one comparison. 


I can do this. And change patch 14 to read


Create subroutine reset_tx_queue to have one place
to release any queued tx skbs.

Any other commit messages you'd like to improve?


> P.S I'm a little confused, as I came looking for a v3 of the first 8 patches
> and found these instead. I'll try to give your first 8 a look when they show up.

The 1st 8 patches have already been applied. I added a patch to address your review there
at the end of the series. So, that patch will show up in my next set.


Thanks for the review

Troy

Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Joshua Clayton]

On Thu, 25 Feb 2016 09:05:34 -0700
Troy Kisky <troy.kisky@boundarydevices.com> wrote:

> On 2/24/2016 7:52 PM, Joshua Clayton wrote:
> > Hello Troy,
> > I'm replying here instead of to a particular commit because several
> > of the commit messages seem inadequate.
> > 
> > The first line summaries all look good.
> > 
> > The descriptions should each also include the "user visible impact"
> > of the patch and the justification for it (i.e. why you made the
> > change).
> > 
> > For instance, patch 3 doesn't include either what will change
> >  (nothing, I'm guessing?) or why we now pass in the structures
> > instead of a queue_id. 
> 
> I can add to the commit message, that this is in preparation for
> patch 4 which depends on it. Or I could squash patches 2/3/4
> together, but I think it is easier to review smaller patches.
> 

I agree that the smaller patches are better. Mentioning that a future
patch depends on the cleanup, (or the specific structure that is
depended on) is good.

> 
> > 
> > You've also got a few (e.g. patch 9, patch 14) where the substance
> > of the patch is in the summary,
> > 
> > but missing from the message.
> > 
> > These kind of descriptions are very hard to review since the
> > expression is split between the subject of the email and the body
> > of the email, which are not close
> > together in some email programs.
> > 
> > Better to reiterate or elaborate on the summary in the message.
> > In patch 9, for instance, it would be more clear to say:
> > 
> > Move restart test to earlier in fec_txq() which saves one
> > comparison. 
> 
> 
> I can do this. And change patch 14 to read
> 
Ok.

> 
> Create subroutine reset_tx_queue to have one place
> to release any queued tx skbs.
> 
That looks like a good message.

> Any other commit messages you'd like to improve?
> 
I'm trying to give guidance in keeping with
Documentation/SubmittingPatches
What I might rather suggest is to do a quick once over for
each commit message to make sure they are each in harmony
with that document.

You can do it with git rebase --interactive, or directly in the
patches.

> 
> > P.S I'm a little confused, as I came looking for a v3 of the first
> > 8 patches and found these instead. I'll try to give your first 8 a
> > look when they show up.
> 
> The 1st 8 patches have already been applied. I added a patch to
> address your review there at the end of the series. So, that patch
> will show up in my next set.
> 
Heh. I didn't see that. I'm used to maintainers waiting for comments to
be responded to before merging changes. That however is not your fault.
Thanks for considering my suggestion anyway.

> 
> Thanks for the review
> 
> Troy

You are welcome. Thanks for upstreaming these improvements.

Joshua

Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Holger Schurig]

Hi Troy,

what is the general aim of your patches?  Stability?  Speed?  Cleanup?

Re: [PATCH net-next V2 00/16] net: fec: cleanup and fixes

[Troy Kisky]

On 2/25/2016 1:39 AM, Holger Schurig wrote:
> Hi Troy,
> 
> what is the general aim of your patches?  Stability?  Speed?  Cleanup?
> 

1. Stability
2. performance
3. easier to read
4. more debug info


The 2nd goal is very hard to measure. It seems function alignment changes swamp
most any other improvements. I think that if the same measurement that I did were done
with a different compiler, you would see different patches increased/decreased
the BPS. But at least the overall trend on the patch set is positive. And
each individual patch has been tested. I would like someone to test on a machine
with 3 queues though.


If you have a more accurate way to measure performance, please let me know.
Also, if you know why freescale's bsp has so much better performance
that would be a very welcome patch.

Troy