Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456731 > unrolled thread

[PATCH 0/3] net: ethernet: ti: cpsw: split driver data and per ndev data

Started byIvan Khoronzhuk <ivan.khoronzhuk@linaro.org>
First post2016-08-04 23:20 +0200
Last post2016-08-05 16:40 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] net: ethernet: ti: cpsw: split driver data and per ndev data Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2016-08-04 23:20 +0200
    [PATCH 2/3] net: ethernet: ti: cpsw: remove redundant check in napi poll Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2016-08-04 23:20 +0200
    [PATCH 1/3] net: ethernet: ti: cpsw: simplify submit routine Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2016-08-04 23:20 +0200
    Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and  private net data Grygorii Strashko <grygorii.strashko@ti.com> - 2016-08-05 14:20 +0200
      Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and  private net data Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2016-08-05 16:40 +0200

#1456731 — [PATCH 0/3] net: ethernet: ti: cpsw: split driver data and per ndev data

FromIvan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date2016-08-04 23:20 +0200
Subject[PATCH 0/3] net: ethernet: ti: cpsw: split driver data and per ndev data
Message-ID<s2ydA-6EW-23@gated-at.bofh.it>
In dual_emac mode the driver can handle 2 network devices. Each of them can use
its own private data and common data/resources. This patchset splits common driver
data/resources and private per net device data.

Doesn't have bad impact on performance.

Based on net-next/master

Ivan Khoronzhuk (3):
  net: ethernet: ti: cpsw: simplify submit routine
  net: ethernet: ti: cpsw: remove redundant check in napi poll
  net: ethernet: ti: cpsw: split common driver data and private net data

 drivers/net/ethernet/ti/cpsw.c | 797 +++++++++++++++++++----------------------
 1 file changed, 369 insertions(+), 428 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1456734 — [PATCH 2/3] net: ethernet: ti: cpsw: remove redundant check in napi poll

FromIvan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date2016-08-04 23:20 +0200
Subject[PATCH 2/3] net: ethernet: ti: cpsw: remove redundant check in napi poll
Message-ID<s2ydA-6EW-31@gated-at.bofh.it>
In reply to#1456731
No need to check number of handled packets, when in most cases (> 99%)
it's not 0. It can be 0 only in rarely cases, even in this case
it's not bad to print just 0.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 8972bf6..85ee9f5 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -793,9 +793,7 @@ static int cpsw_tx_poll(struct napi_struct *napi_tx, int budget)
 		}
 	}
 
-	if (num_tx)
-		cpsw_dbg(priv, intr, "poll %d tx pkts\n", num_tx);
-
+	cpsw_dbg(priv, intr, "poll %d tx pkts\n", num_tx);
 	return num_tx;
 }
 
@@ -814,9 +812,7 @@ static int cpsw_rx_poll(struct napi_struct *napi_rx, int budget)
 		}
 	}
 
-	if (num_rx)
-		cpsw_dbg(priv, intr, "poll %d rx pkts\n", num_rx);
-
+	cpsw_dbg(priv, intr, "poll %d rx pkts\n", num_rx);
 	return num_rx;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1456737 — [PATCH 1/3] net: ethernet: ti: cpsw: simplify submit routine

FromIvan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date2016-08-04 23:20 +0200
Subject[PATCH 1/3] net: ethernet: ti: cpsw: simplify submit routine
Message-ID<s2ydA-6EW-39@gated-at.bofh.it>
In reply to#1456731
As second net dev is created only in case of dual_emac mode, port
number can be figured out in simpler way. Also no need to pass
redundant ndev struct.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c51f346..8972bf6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1065,19 +1065,11 @@ static int cpsw_common_res_usage_state(struct cpsw_priv *priv)
 	return usage_count;
 }
 
-static inline int cpsw_tx_packet_submit(struct net_device *ndev,
-			struct cpsw_priv *priv, struct sk_buff *skb)
+static inline int cpsw_tx_packet_submit(struct cpsw_priv *priv,
+					struct sk_buff *skb)
 {
-	if (!priv->data.dual_emac)
-		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 0);
-
-	if (ndev == cpsw_get_slave_ndev(priv, 0))
-		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 1);
-	else
-		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 2);
+	return cpdma_chan_submit(priv->txch, skb, skb->data, skb->len,
+				 priv->emac_port + priv->data.dual_emac);
 }
 
 static inline void cpsw_add_dual_emac_def_ale_entries(
@@ -1406,7 +1398,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
 
 	skb_tx_timestamp(skb);
 
-	ret = cpsw_tx_packet_submit(ndev, priv, skb);
+	ret = cpsw_tx_packet_submit(priv, skb);
 	if (unlikely(ret != 0)) {
 		cpsw_err(priv, tx_err, "desc submit failed\n");
 		goto fail;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1457064 — Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data

FromGrygorii Strashko <grygorii.strashko@ti.com>
Date2016-08-05 14:20 +0200
SubjectRe: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data
Message-ID<s2Mgy-7oy-3@gated-at.bofh.it>
In reply to#1456731
On 08/05/2016 12:14 AM, Ivan Khoronzhuk wrote:
> Simplify driver by splitting common driver data and net dev
> private data. In case of dual_emac mode 2 networks devices
> are created, each of them contains its own private data.
> But 2 net devices share a bunch of h/w resources, that shouldn't
> be duplicated.
> This patch leads to the following:
> - no functional changes
> - reduce code size
> - reduce memory usage
> - reduce number of conversion to priv function
> - reduce number of arguments for some functions
> - increase code readability
> - create prerequisites to add multichannel support,
>   when channels are shared between net devices

Even if it sounds reasonable, I have to NACK this patch -
main reason below, but there are few more:
- could you pls split this change as it's too big and I'm pretty sure it's possible;
- could you pls avoid unrelated changes like variable reordering in structures

and thanks a lot for working on this.

> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 775 +++++++++++++++++++----------------------
>  1 file changed, 364 insertions(+), 411 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 85ee9f5..7a84515 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -141,8 +141,8 @@ do {								\
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
>  #define cpsw_slave_index(priv)				\
> -		((priv->data.dual_emac) ? priv->emac_port :	\
> -		priv->data.active_slave)
> +		((cpsw->data.dual_emac) ? priv->emac_port :	\
> +		cpsw->data.active_slave)
>  
>  static int debug_level;
>  module_param(debug_level, int, 0);
> @@ -364,29 +364,34 @@ static inline void slave_write(struct cpsw_slave *slave, u32 val, u32 offset)
>  }
>  
>  struct cpsw_priv {
> -	struct platform_device		*pdev;
>  	struct net_device		*ndev;
> -	struct napi_struct		napi_rx;
> -	struct napi_struct		napi_tx;
>  	struct device			*dev;
> +	u8				mac_addr[ETH_ALEN];
> +	bool				rx_pause;
> +	bool				tx_pause;
> +	u32				msg_enable;
> +	u32				emac_port;
> +};
> +
> +struct cpsw_common {
> +	struct net_device		*ndev; /* holds base ndev */
> +	struct platform_device		*pdev;
>  	struct cpsw_platform_data	data;
> +	struct napi_struct		napi_rx;
> +	struct napi_struct		napi_tx;
> +	struct cpdma_chan		*txch, *rxch;
> +	struct cpsw_slave		*slaves;
>  	struct cpsw_ss_regs __iomem	*regs;
>  	struct cpsw_wr_regs __iomem	*wr_regs;
>  	u8 __iomem			*hw_stats;
>  	struct cpsw_host_regs __iomem	*host_port_regs;
> -	u32				msg_enable;
> -	u32				version;
> -	u32				coal_intvl;
> -	u32				bus_freq_mhz;
> -	int				rx_packet_max;
>  	struct clk			*clk;
> -	u8				mac_addr[ETH_ALEN];
> -	struct cpsw_slave		*slaves;
>  	struct cpdma_ctlr		*dma;
> -	struct cpdma_chan		*txch, *rxch;
>  	struct cpsw_ale			*ale;
> -	bool				rx_pause;
> -	bool				tx_pause;
> +	int				rx_packet_max;
> +	u32				bus_freq_mhz;
> +	u32				version;
> +	u32				coal_intvl;
>  	bool				quirk_irq;
>  	bool				rx_irq_disabled;
>  	bool				tx_irq_disabled;
> @@ -394,9 +399,10 @@ struct cpsw_priv {
>  	u32 irqs_table[4];
>  	u32 num_irqs;
>  	struct cpts *cpts;
> -	u32 emac_port;
>  };



>  
> +static struct cpsw_common *cpsw;
> +

Sry, but NACK - no new static variables pls.

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1457132 — Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data

FromIvan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date2016-08-05 16:40 +0200
SubjectRe: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data
Message-ID<s2Os2-i5-7@gated-at.bofh.it>
In reply to#1457064

On 05.08.16 15:14, Grygorii Strashko wrote:
> On 08/05/2016 12:14 AM, Ivan Khoronzhuk wrote:
>> Simplify driver by splitting common driver data and net dev
>> private data. In case of dual_emac mode 2 networks devices
>> are created, each of them contains its own private data.
>> But 2 net devices share a bunch of h/w resources, that shouldn't
>> be duplicated.
>> This patch leads to the following:
>> - no functional changes
>> - reduce code size
>> - reduce memory usage
>> - reduce number of conversion to priv function
>> - reduce number of arguments for some functions
>> - increase code readability
>> - create prerequisites to add multichannel support,
>>   when channels are shared between net devices
>
> Even if it sounds reasonable, I have to NACK this patch -
> main reason below, but there are few more:
> - could you pls split this change as it's too big and I'm pretty sure it's possible;
> - could you pls avoid unrelated changes like variable reordering in structures
Ok. v2 will include it.

>
> and thanks a lot for working on this.
>
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>  drivers/net/ethernet/ti/cpsw.c | 775 +++++++++++++++++++----------------------
>>  1 file changed, 364 insertions(+), 411 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 85ee9f5..7a84515 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -141,8 +141,8 @@ do {								\
>>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>>
>>  #define cpsw_slave_index(priv)				\
>> -		((priv->data.dual_emac) ? priv->emac_port :	\
>> -		priv->data.active_slave)
>> +		((cpsw->data.dual_emac) ? priv->emac_port :	\
>> +		cpsw->data.active_slave)
>>
>>  static int debug_level;
>>  module_param(debug_level, int, 0);
>> @@ -364,29 +364,34 @@ static inline void slave_write(struct cpsw_slave *slave, u32 val, u32 offset)
>>  }
>>
>>  struct cpsw_priv {
>> -	struct platform_device		*pdev;
>>  	struct net_device		*ndev;
>> -	struct napi_struct		napi_rx;
>> -	struct napi_struct		napi_tx;
>>  	struct device			*dev;
>> +	u8				mac_addr[ETH_ALEN];
>> +	bool				rx_pause;
>> +	bool				tx_pause;
>> +	u32				msg_enable;
>> +	u32				emac_port;
>> +};
>> +
>> +struct cpsw_common {
>> +	struct net_device		*ndev; /* holds base ndev */
>> +	struct platform_device		*pdev;
>>  	struct cpsw_platform_data	data;
>> +	struct napi_struct		napi_rx;
>> +	struct napi_struct		napi_tx;
>> +	struct cpdma_chan		*txch, *rxch;
>> +	struct cpsw_slave		*slaves;
>>  	struct cpsw_ss_regs __iomem	*regs;
>>  	struct cpsw_wr_regs __iomem	*wr_regs;
>>  	u8 __iomem			*hw_stats;
>>  	struct cpsw_host_regs __iomem	*host_port_regs;
>> -	u32				msg_enable;
>> -	u32				version;
>> -	u32				coal_intvl;
>> -	u32				bus_freq_mhz;
>> -	int				rx_packet_max;
>>  	struct clk			*clk;
>> -	u8				mac_addr[ETH_ALEN];
>> -	struct cpsw_slave		*slaves;
>>  	struct cpdma_ctlr		*dma;
>> -	struct cpdma_chan		*txch, *rxch;
>>  	struct cpsw_ale			*ale;
>> -	bool				rx_pause;
>> -	bool				tx_pause;
>> +	int				rx_packet_max;
>> +	u32				bus_freq_mhz;
>> +	u32				version;
>> +	u32				coal_intvl;
>>  	bool				quirk_irq;
>>  	bool				rx_irq_disabled;
>>  	bool				tx_irq_disabled;
>> @@ -394,9 +399,10 @@ struct cpsw_priv {
>>  	u32 irqs_table[4];
>>  	u32 num_irqs;
>>  	struct cpts *cpts;
>> -	u32 emac_port;
>>  };
>
>
>
>>
>> +static struct cpsw_common *cpsw;
>> +
>
> Sry, but NACK - no new static variables pls.
Ok.

>

-- 
Regards,
Ivan Khoronzhuk

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web