[PATCH (net.git) 0/4 (v3)] stmmac fixes: EEE and chained mode

[PATCH (net.git) 0/4 (v3)] stmmac fixes: EEE and chained mode

[Giuseppe Cavallaro]

These patches are to fix some problem in the driver.

Mandatory changes are for EEE that needs to be disabled if not supported
and for the chain mode that is broken and the kernel panics if this mode
is enabled.

v3: removed a patch from my previous set that touched the stmmac_tx path
    that has not to be applied. Other patches for cleaning-up will be
    sent on top of net-next git repo.

Giuseppe Cavallaro (4):
  stmmac: disable at run-time the EEE if not supported
  stmmac: fix and better tune the default buffer sizes
  stmmac: dwmac-sti: fix broken STiD127 compatibility
  stmmac: fix chained mode

 drivers/net/ethernet/stmicro/stmmac/chain_mode.c   |    2 +-
 drivers/net/ethernet/stmicro/stmmac/common.h       |   20 ++---
 drivers/net/ethernet/stmicro/stmmac/ring_mode.c    |    9 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |   95 +++++++++++---------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |    2 +-
 5 files changed, 64 insertions(+), 64 deletions(-)

-- 
1.7.4.4

[PATCH (net.git) 1/4] stmmac: disable at run-time the EEE if not supported

[Giuseppe Cavallaro]

This patch is to disable the EEE (so HW and timers)
for example when the phy communicates that the EEE
can be supported anymore.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   23 +++++++++++++++++---
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 078ad0e..b418f94 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -286,10 +286,25 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
 
 	/* MAC core supports the EEE feature. */
 	if (priv->dma_cap.eee) {
+		int tx_lpi_timer = priv->tx_lpi_timer;
+
 		/* Check if the PHY supports EEE */
-		if (phy_init_eee(priv->phydev, 1))
+		if (phy_init_eee(priv->phydev, 1)) {
+			/* To manage at run-time if the EEE cannot be supported
+			 * anymore (for example because the lp caps have been
+			 * changed).
+			 * In that case the driver disable own timers.
+			 */
+			if (priv->eee_active) {
+				pr_debug("stmmac: disable EEE\n");
+				del_timer_sync(&priv->eee_ctrl_timer);
+				priv->hw->mac->set_eee_timer(priv->ioaddr, 0,
+							     tx_lpi_timer);
+			}
+			priv->eee_active = 0;
 			goto out;
-
+		}
+		/* Activate the EEE and start timers */
 		if (!priv->eee_active) {
 			priv->eee_active = 1;
 			init_timer(&priv->eee_ctrl_timer);
@@ -300,13 +315,13 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
 
 			priv->hw->mac->set_eee_timer(priv->ioaddr,
 						     STMMAC_DEFAULT_LIT_LS,
-						     priv->tx_lpi_timer);
+						     tx_lpi_timer);
 		} else
 			/* Set HW EEE according to the speed */
 			priv->hw->mac->set_eee_pls(priv->ioaddr,
 						   priv->phydev->link);
 
-		pr_info("stmmac: Energy-Efficient Ethernet initialized\n");
+		pr_debug("stmmac: Energy-Efficient Ethernet initialized\n");
 
 		ret = true;
 	}
-- 
1.7.4.4

[PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe Cavallaro]

This patch is to fix and tune the default buffer sizes.
It reduces the default bufsize used by the driver from
2048 to 1518 (taking into account the extra 4 bytes in case of VLAN).

Patch has been tested on both ARM and SH4 platform based.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b418f94..40d811d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -92,8 +92,12 @@ static int tc = TC_DEFAULT;
 module_param(tc, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(tc, "DMA threshold control value");
 
-#define DMA_BUFFER_SIZE	BUF_SIZE_4KiB
-static int buf_sz = DMA_BUFFER_SIZE;
+#ifdef STMMAC_VLAN_TAG_USED
+#define	DEFAULT_BUFSIZE	(VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
+#else
+#define	DEFAULT_BUFSIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
+#endif
+static int buf_sz = DEFAULT_BUFSIZE;
 module_param(buf_sz, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(buf_sz, "DMA buffer size");
 
@@ -136,8 +140,8 @@ static void stmmac_verify_args(void)
 		dma_rxsize = DMA_RX_SIZE;
 	if (unlikely(dma_txsize < 0))
 		dma_txsize = DMA_TX_SIZE;
-	if (unlikely((buf_sz < DMA_BUFFER_SIZE) || (buf_sz > BUF_SIZE_16KiB)))
-		buf_sz = DMA_BUFFER_SIZE;
+	if (unlikely((buf_sz < DEFAULT_BUFSIZE) || (buf_sz > BUF_SIZE_16KiB)))
+		buf_sz = DEFAULT_BUFSIZE;
 	if (unlikely(flow_ctrl > 1))
 		flow_ctrl = FLOW_AUTO;
 	else if (likely(flow_ctrl < 0))
@@ -901,10 +905,10 @@ static int stmmac_set_bfsize(int mtu, int bufsize)
 		ret = BUF_SIZE_8KiB;
 	else if (mtu >= BUF_SIZE_2KiB)
 		ret = BUF_SIZE_4KiB;
-	else if (mtu >= DMA_BUFFER_SIZE)
+	else if (mtu > DEFAULT_BUFSIZE)
 		ret = BUF_SIZE_2KiB;
 	else
-		ret = DMA_BUFFER_SIZE;
+		ret = DEFAULT_BUFSIZE;
 
 	return ret;
 }
-- 
1.7.4.4

[PATCH (net.git) 3/4] stmmac: dwmac-sti: fix broken STiD127 compatibility

[Giuseppe Cavallaro]

This is to fix the compatibility to the STiD127 SoC.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index c61bc72b..8fb32a8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -36,7 +36,7 @@ static const struct of_device_id stmmac_dt_ids[] = {
 #ifdef CONFIG_DWMAC_STI
 	{ .compatible = "st,stih415-dwmac", .data = &sti_gmac_data},
 	{ .compatible = "st,stih416-dwmac", .data = &sti_gmac_data},
-	{ .compatible = "st,stih127-dwmac", .data = &sti_gmac_data},
+	{ .compatible = "st,stid127-dwmac", .data = &sti_gmac_data},
 #endif
 	/* SoC specific glue layers should come before generic bindings */
 	{ .compatible = "st,spear600-gmac"},
-- 
1.7.4.4

[PATCH (net.git) 4/4] stmmac: fix chained mode

[Giuseppe Cavallaro]

This patch is to fix the chain mode that was broken
and generated a panic. This patch reviews the chain/ring
modes now shaing the same structure and taking care
about the pointers and callbacks.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/chain_mode.c  |    2 +-
 drivers/net/ethernet/stmicro/stmmac/common.h      |   20 ++-----
 drivers/net/ethernet/stmicro/stmmac/ring_mode.c   |    9 ++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   56 +++++++++------------
 4 files changed, 34 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
index 72d282b..c553f6b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
@@ -151,7 +151,7 @@ static void stmmac_clean_desc3(void *priv_ptr, struct dma_desc *p)
 					  sizeof(struct dma_desc)));
 }
 
-const struct stmmac_chain_mode_ops chain_mode_ops = {
+const struct stmmac_mode_ops chain_mode_ops = {
 	.init = stmmac_init_dma_chain,
 	.is_jumbo_frm = stmmac_is_jumbo_frm,
 	.jumbo_frm = stmmac_jumbo_frm,
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 7834a39..74610f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -419,20 +419,13 @@ struct mii_regs {
 	unsigned int data;	/* MII Data */
 };
 
-struct stmmac_ring_mode_ops {
-	unsigned int (*is_jumbo_frm) (int len, int ehn_desc);
-	unsigned int (*jumbo_frm) (void *priv, struct sk_buff *skb, int csum);
-	void (*refill_desc3) (void *priv, struct dma_desc *p);
-	void (*init_desc3) (struct dma_desc *p);
-	void (*clean_desc3) (void *priv, struct dma_desc *p);
-	int (*set_16kib_bfsize) (int mtu);
-};
-
-struct stmmac_chain_mode_ops {
+struct stmmac_mode_ops {
 	void (*init) (void *des, dma_addr_t phy_addr, unsigned int size,
 		      unsigned int extend_desc);
 	unsigned int (*is_jumbo_frm) (int len, int ehn_desc);
 	unsigned int (*jumbo_frm) (void *priv, struct sk_buff *skb, int csum);
+	int (*set_16kib_bfsize)(int mtu);
+	void (*init_desc3)(struct dma_desc *p);
 	void (*refill_desc3) (void *priv, struct dma_desc *p);
 	void (*clean_desc3) (void *priv, struct dma_desc *p);
 };
@@ -441,8 +434,7 @@ struct mac_device_info {
 	const struct stmmac_ops *mac;
 	const struct stmmac_desc_ops *desc;
 	const struct stmmac_dma_ops *dma;
-	const struct stmmac_ring_mode_ops *ring;
-	const struct stmmac_chain_mode_ops *chain;
+	const struct stmmac_mode_ops *mode;
 	const struct stmmac_hwtimestamp *ptp;
 	struct mii_regs mii;	/* MII register Addresses */
 	struct mac_link link;
@@ -460,7 +452,7 @@ void stmmac_get_mac_addr(void __iomem *ioaddr, unsigned char *addr,
 void stmmac_set_mac(void __iomem *ioaddr, bool enable);
 
 void dwmac_dma_flush_tx_fifo(void __iomem *ioaddr);
-extern const struct stmmac_ring_mode_ops ring_mode_ops;
-extern const struct stmmac_chain_mode_ops chain_mode_ops;
+extern const struct stmmac_mode_ops ring_mode_ops;
+extern const struct stmmac_mode_ops chain_mode_ops;
 
 #endif /* __COMMON_H__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
index a96c7c2..650a4be 100644
--- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
@@ -100,10 +100,9 @@ static void stmmac_refill_desc3(void *priv_ptr, struct dma_desc *p)
 {
 	struct stmmac_priv *priv = (struct stmmac_priv *)priv_ptr;
 
-	if (unlikely(priv->plat->has_gmac))
-		/* Fill DES3 in case of RING mode */
-		if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
-			p->des3 = p->des2 + BUF_SIZE_8KiB;
+	/* Fill DES3 in case of RING mode */
+	if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
+		p->des3 = p->des2 + BUF_SIZE_8KiB;
 }
 
 /* In ring mode we need to fill the desc3 because it is used as buffer */
@@ -126,7 +125,7 @@ static int stmmac_set_16kib_bfsize(int mtu)
 	return ret;
 }
 
-const struct stmmac_ring_mode_ops ring_mode_ops = {
+const struct stmmac_mode_ops ring_mode_ops = {
 	.is_jumbo_frm = stmmac_is_jumbo_frm,
 	.jumbo_frm = stmmac_jumbo_frm,
 	.refill_desc3 = stmmac_refill_desc3,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 40d811d..96afca4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -970,9 +970,9 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
 
 	p->des2 = priv->rx_skbuff_dma[i];
 
-	if ((priv->mode == STMMAC_RING_MODE) &&
+	if ((priv->hw->mode->init_desc3) &&
 	    (priv->dma_buf_sz == BUF_SIZE_16KiB))
-		priv->hw->ring->init_desc3(p);
+		priv->hw->mode->init_desc3(p);
 
 	return 0;
 }
@@ -1003,11 +1003,8 @@ static int init_dma_desc_rings(struct net_device *dev)
 	unsigned int bfsize = 0;
 	int ret = -ENOMEM;
 
-	/* Set the max buffer size according to the DESC mode
-	 * and the MTU. Note that RING mode allows 16KiB bsize.
-	 */
-	if (priv->mode == STMMAC_RING_MODE)
-		bfsize = priv->hw->ring->set_16kib_bfsize(dev->mtu);
+	if (priv->hw->mode->set_16kib_bfsize)
+		bfsize = priv->hw->mode->set_16kib_bfsize(dev->mtu);
 
 	if (bfsize < BUF_SIZE_16KiB)
 		bfsize = stmmac_set_bfsize(dev->mtu, priv->dma_buf_sz);
@@ -1048,15 +1045,15 @@ static int init_dma_desc_rings(struct net_device *dev)
 	/* Setup the chained descriptor addresses */
 	if (priv->mode == STMMAC_CHAIN_MODE) {
 		if (priv->extend_desc) {
-			priv->hw->chain->init(priv->dma_erx, priv->dma_rx_phy,
-					      rxsize, 1);
-			priv->hw->chain->init(priv->dma_etx, priv->dma_tx_phy,
-					      txsize, 1);
+			priv->hw->mode->init(priv->dma_erx, priv->dma_rx_phy,
+					     rxsize, 1);
+			priv->hw->mode->init(priv->dma_etx, priv->dma_tx_phy,
+					     txsize, 1);
 		} else {
-			priv->hw->chain->init(priv->dma_rx, priv->dma_rx_phy,
-					      rxsize, 0);
-			priv->hw->chain->init(priv->dma_tx, priv->dma_tx_phy,
-					      txsize, 0);
+			priv->hw->mode->init(priv->dma_rx, priv->dma_rx_phy,
+					     rxsize, 0);
+			priv->hw->mode->init(priv->dma_tx, priv->dma_tx_phy,
+					     txsize, 0);
 		}
 	}
 
@@ -1307,7 +1304,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 					 DMA_TO_DEVICE);
 			priv->tx_skbuff_dma[entry] = 0;
 		}
-		priv->hw->ring->clean_desc3(priv, p);
+		priv->hw->mode->clean_desc3(priv, p);
 
 		if (likely(skb != NULL)) {
 			dev_kfree_skb(skb);
@@ -1863,6 +1860,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	int nfrags = skb_shinfo(skb)->nr_frags;
 	struct dma_desc *desc, *first;
 	unsigned int nopaged_len = skb_headlen(skb);
+	unsigned int enh_desc = priv->plat->enh_desc;
 
 	if (unlikely(stmmac_tx_avail(priv) < nfrags + 1)) {
 		if (!netif_queue_stopped(dev)) {
@@ -1890,27 +1888,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	first = desc;
 
 	/* To program the descriptors according to the size of the frame */
-	if (priv->mode == STMMAC_RING_MODE) {
-		is_jumbo = priv->hw->ring->is_jumbo_frm(skb->len,
-							priv->plat->enh_desc);
-		if (unlikely(is_jumbo))
-			entry = priv->hw->ring->jumbo_frm(priv, skb,
-							  csum_insertion);
-	} else {
-		is_jumbo = priv->hw->chain->is_jumbo_frm(skb->len,
-							 priv->plat->enh_desc);
-		if (unlikely(is_jumbo))
-			entry = priv->hw->chain->jumbo_frm(priv, skb,
-							   csum_insertion);
-	}
+	if (enh_desc)
+		is_jumbo = priv->hw->mode->is_jumbo_frm(skb->len, enh_desc);
+
 	if (likely(!is_jumbo)) {
 		desc->des2 = dma_map_single(priv->device, skb->data,
 					    nopaged_len, DMA_TO_DEVICE);
 		priv->tx_skbuff_dma[entry] = desc->des2;
 		priv->hw->desc->prepare_tx_desc(desc, 1, nopaged_len,
 						csum_insertion, priv->mode);
-	} else
+	} else {
 		desc = first;
+		entry = priv->hw->mode->jumbo_frm(priv, skb, csum_insertion);
+	}
 
 	for (i = 0; i < nfrags; i++) {
 		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
@@ -2048,7 +2038,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
 
 			p->des2 = priv->rx_skbuff_dma[entry];
 
-			priv->hw->ring->refill_desc3(priv, p);
+			priv->hw->mode->refill_desc3(priv, p);
 
 			if (netif_msg_rx_status(priv))
 				pr_debug("\trefill entry #%d\n", entry);
@@ -2652,11 +2642,11 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 
 	/* To use the chained or ring mode */
 	if (chain_mode) {
-		priv->hw->chain = &chain_mode_ops;
+		priv->hw->mode = &chain_mode_ops;
 		pr_info(" Chain mode enabled\n");
 		priv->mode = STMMAC_CHAIN_MODE;
 	} else {
-		priv->hw->ring = &ring_mode_ops;
+		priv->hw->mode = &ring_mode_ops;
 		pr_info(" Ring mode enabled\n");
 		priv->mode = STMMAC_RING_MODE;
 	}
-- 
1.7.4.4

RE: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[David Laight]

From: Giuseppe Cavallaro
> This patch is to fix and tune the default buffer sizes.
> It reduces the default bufsize used by the driver from
> 2048 to 1518 (taking into account the extra 4 bytes in case of VLAN).
...
> -#define DMA_BUFFER_SIZE	BUF_SIZE_4KiB
> -static int buf_sz = DMA_BUFFER_SIZE;

Does this means that the old default was 4k, not the 2k in the
patch description.

> +#ifdef STMMAC_VLAN_TAG_USED
> +#define	DEFAULT_BUFSIZE	(VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
> +#else
> +#define	DEFAULT_BUFSIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
> +#endif
...
> +	if (unlikely((buf_sz < DEFAULT_BUFSIZE) || (buf_sz > BUF_SIZE_16KiB)))
> +		buf_sz = DEFAULT_BUFSIZE;

It doesn't seem right to me for the minimum buffer size to
depend on a compile-time option for VLAN.

Also (provided the hardware supports it) the rx buffers (are these
the ones being sized?) need to be aligned on a 4n+2 boundary in
order to avoid a realignment copy later on.
So I'm not sure that some of these sizes are right and/or optimal.

	David

Re: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe CAVALLARO]

On 2/27/2014 11:51 AM, David Laight wrote:
> From: Giuseppe Cavallaro
>> This patch is to fix and tune the default buffer sizes.
>> It reduces the default bufsize used by the driver from
>> 2048 to 1518 (taking into account the extra 4 bytes in case of VLAN).
> ...
>> -#define DMA_BUFFER_SIZE	BUF_SIZE_4KiB
>> -static int buf_sz = DMA_BUFFER_SIZE;
>
> Does this means that the old default was 4k, not the 2k in the
> patch description.

no pbl, I'll fix it in the patch subject.

>
>> +#ifdef STMMAC_VLAN_TAG_USED
>> +#define	DEFAULT_BUFSIZE	(VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
>> +#else
>> +#define	DEFAULT_BUFSIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
>> +#endif
> ...
>> +	if (unlikely((buf_sz < DEFAULT_BUFSIZE) || (buf_sz > BUF_SIZE_16KiB)))
>> +		buf_sz = DEFAULT_BUFSIZE;
>
> It doesn't seem right to me for the minimum buffer size to
> depend on a compile-time option for VLAN.

Hmm, I can have a default suitable for all the cases.
Indeed other drivers program buffers (dlink/sundance.c)
and do other settings according Koption like CONFIG_VLAN_8021Q.
It is not a problem to review and delete it.

> Also (provided the hardware supports it) the rx buffers (are these
> the ones being sized?) need to be aligned on a 4n+2 boundary in
> order to avoid a realignment copy later on.

This is true and indeed I had added the STMMAC_ALIGN to align all.
In the past to get the right alignment for SH4.

> So I'm not sure that some of these sizes are right and/or optimal.

What do you suggest?

Maybe, I can use a default for sure < 4KiB suitable to be used for VLAN
frames (it will be aligned later).

Peppe

>
> 	David
>
>
>
>
>

RE: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[David Laight]

From: Giuseppe CAVALLARO
...
> > Also (provided the hardware supports it) the rx buffers (are these
> > the ones being sized?) need to be aligned on a 4n+2 boundary in
> > order to avoid a realignment copy later on.
> 
> This is true and indeed I had added the STMMAC_ALIGN to align all.
> In the past to get the right alignment for SH4.
> 
> > So I'm not sure that some of these sizes are right and/or optimal.
> 
> What do you suggest?
> 
> Maybe, I can use a default for sure < 4KiB suitable to be used for VLAN
> frames (it will be aligned later).

Dunno... It rather depends on what the length is actually used for!
What you don’t want to be doing is adding 2 (for the 4n+2) and then
mallocing a 4096+2 byte buffer somewhere.

If the hardware does receive desegmentation, then you need to handle
the 64k+ receives somewhere.
If it doesn't then it doesn't matter if the hardware rx buffer size is
slightly too large (eg for VLAN or encapsulation full sized frames in PPoE).
1536 bytes for the memory buffer avoids cache line sharing (read to
offset 2).

The last ethernet driver I wrote from scratch (maybe 20 years ago) set
the rx-ring to point to an array of 512 byte buffers (last was shorter
to avoid an extra page) and did an aligned copy into the message buffer.
Only frames that crossed the ring end needed two copies.
ISTR making the copy be cache line aligned so that a special cache line
copy function could be used (I don't know if it ever was).
For that system the cost of the aligned data copies was less that the
complexity and cost of setting up the iommu.

	David

Re: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe CAVALLARO]

On 2/27/2014 2:03 PM, Giuseppe CAVALLARO wrote:
> On 2/27/2014 11:51 AM, David Laight wrote:
>> From: Giuseppe Cavallaro
>>> This patch is to fix and tune the default buffer sizes.
>>> It reduces the default bufsize used by the driver from
>>> 2048 to 1518 (taking into account the extra 4 bytes in case of VLAN).
>> ...
>>> -#define DMA_BUFFER_SIZE    BUF_SIZE_4KiB
>>> -static int buf_sz = DMA_BUFFER_SIZE;
>>
>> Does this means that the old default was 4k, not the 2k in the
>> patch description.
>
> no pbl, I'll fix it in the patch subject.
>
>>
>>> +#ifdef STMMAC_VLAN_TAG_USED
>>> +#define    DEFAULT_BUFSIZE    (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
>>> +#else
>>> +#define    DEFAULT_BUFSIZE    (ETH_FRAME_LEN + ETH_FCS_LEN)
>>> +#endif
>> ...
>>> +    if (unlikely((buf_sz < DEFAULT_BUFSIZE) || (buf_sz >
>>> BUF_SIZE_16KiB)))
>>> +        buf_sz = DEFAULT_BUFSIZE;
>>
>> It doesn't seem right to me for the minimum buffer size to
>> depend on a compile-time option for VLAN.
>
> Hmm, I can have a default suitable for all the cases.
> Indeed other drivers program buffers (dlink/sundance.c)
> and do other settings according Koption like CONFIG_VLAN_8021Q.
> It is not a problem to review and delete it.
>
>> Also (provided the hardware supports it) the rx buffers (are these
>> the ones being sized?) need to be aligned on a 4n+2 boundary in
>> order to avoid a realignment copy later on.
>
> This is true and indeed I had added the STMMAC_ALIGN to align all.
> In the past to get the right alignment for SH4.
>
>> So I'm not sure that some of these sizes are right and/or optimal.
>
> What do you suggest?
>
> Maybe, I can use a default for sure < 4KiB suitable to be used for VLAN
> frames (it will be aligned later).

I did some other check and indeed it is aligned to 2KiB so what do you
think if I use it instead of 4KiB? I do not remember why this was added
but it should be only set in case of we change the mtu and the driver
should take care about that.

if you agree, I can do further test on sh4 and arm and repost the patch

peppe



>
> Peppe
>
>>
>>     David
>>
>>
>>
>>
>>
>
>
>

Re: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe CAVALLARO]

Hi David

On 2/27/2014 2:31 PM, David Laight wrote:
> From: Giuseppe CAVALLARO
> ...
>>> Also (provided the hardware supports it) the rx buffers (are these
>>> the ones being sized?) need to be aligned on a 4n+2 boundary in
>>> order to avoid a realignment copy later on.
>>
>> This is true and indeed I had added the STMMAC_ALIGN to align all.
>> In the past to get the right alignment for SH4.
>>
>>> So I'm not sure that some of these sizes are right and/or optimal.
>>
>> What do you suggest?
>>
>> Maybe, I can use a default for sure < 4KiB suitable to be used for VLAN
>> frames (it will be aligned later).
>
> Dunno... It rather depends on what the length is actually used for!
> What you don’t want to be doing is adding 2 (for the 4n+2) and then
> mallocing a 4096+2 byte buffer somewhere.

thanks for your support.

What do you mean for 4n+2?

we reserve 2 more when allocate the skb.
I had seen that, When I used 2KiB as default this pushes us to allocate
from the next larger slab on old SH4 platforms and sometime forcing me
to increment the min_free_kbytes also when we work with the std
Ethernet MTU :-(.

So for sure 4KiB is really big and not good especially for embedded
system where this driver lives.

>
> If the hardware does receive desegmentation, then you need to handle
> the 64k+ receives somewhere.
> If it doesn't then it doesn't matter if the hardware rx buffer size is
> slightly too large (eg for VLAN or encapsulation full sized frames in PPoE).
> 1536 bytes for the memory buffer avoids cache line sharing (read to
> offset 2).

IIUC, so what you finally suggest is to use a default value w/o Koption
and 1536bytes is suitable for vlan etc. This is ok and can be managed
w/o breaking the compatibility with old mac where the rx hw buffer
are limited in size and where jumbo is not supported.

> The last ethernet driver I wrote from scratch (maybe 20 years ago) set
> the rx-ring to point to an array of 512 byte buffers (last was shorter
> to avoid an extra page) and did an aligned copy into the message buffer.
> Only frames that crossed the ring end needed two copies.
> ISTR making the copy be cache line aligned so that a special cache line
> copy function could be used (I don't know if it ever was).
> For that system the cost of the aligned data copies was less that the
> complexity and cost of setting up the iommu.

ok

peppe
>
> 	David
>
>
>
>
>

Re: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe CAVALLARO]

Hello David

Sorry, can you tell me what is the final your opinion about
this patch?

I think that it not good to keep 4KiB as default and your main concern
was about the default buffer size that depended on VLAN ifconfig.

So I could adopt 1536bytes as default and remove ifdef.

I'd like to resubmit these first patches that add some relevant fixes
in the driver especially for EEE and chained mode that doesn't work
after latest reworks.

Best Regards
Peppe

On 2/27/2014 2:54 PM, Giuseppe CAVALLARO wrote:
> Hi David
>
> On 2/27/2014 2:31 PM, David Laight wrote:
>> From: Giuseppe CAVALLARO
>> ...
>>>> Also (provided the hardware supports it) the rx buffers (are these
>>>> the ones being sized?) need to be aligned on a 4n+2 boundary in
>>>> order to avoid a realignment copy later on.
>>>
>>> This is true and indeed I had added the STMMAC_ALIGN to align all.
>>> In the past to get the right alignment for SH4.
>>>
>>>> So I'm not sure that some of these sizes are right and/or optimal.
>>>
>>> What do you suggest?
>>>
>>> Maybe, I can use a default for sure < 4KiB suitable to be used for VLAN
>>> frames (it will be aligned later).
>>
>> Dunno... It rather depends on what the length is actually used for!
>> What you don’t want to be doing is adding 2 (for the 4n+2) and then
>> mallocing a 4096+2 byte buffer somewhere.
>
> thanks for your support.
>
> What do you mean for 4n+2?
>
> we reserve 2 more when allocate the skb.
> I had seen that, When I used 2KiB as default this pushes us to allocate
> from the next larger slab on old SH4 platforms and sometime forcing me
> to increment the min_free_kbytes also when we work with the std
> Ethernet MTU :-(.
>
> So for sure 4KiB is really big and not good especially for embedded
> system where this driver lives.
>
>>
>> If the hardware does receive desegmentation, then you need to handle
>> the 64k+ receives somewhere.
>> If it doesn't then it doesn't matter if the hardware rx buffer size is
>> slightly too large (eg for VLAN or encapsulation full sized frames in
>> PPoE).
>> 1536 bytes for the memory buffer avoids cache line sharing (read to
>> offset 2).
>
> IIUC, so what you finally suggest is to use a default value w/o Koption
> and 1536bytes is suitable for vlan etc. This is ok and can be managed
> w/o breaking the compatibility with old mac where the rx hw buffer
> are limited in size and where jumbo is not supported.
>
>> The last ethernet driver I wrote from scratch (maybe 20 years ago) set
>> the rx-ring to point to an array of 512 byte buffers (last was shorter
>> to avoid an extra page) and did an aligned copy into the message buffer.
>> Only frames that crossed the ring end needed two copies.
>> ISTR making the copy be cache line aligned so that a special cache line
>> copy function could be used (I don't know if it ever was).
>> For that system the cost of the aligned data copies was less that the
>> complexity and cost of setting up the iommu.
>
> ok
>
> peppe
>>
>>     David
>>
>>
>>
>>
>>
>
>
>

RE: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[David Laight]

From: Giuseppe CAVALLARO 
> Hello David
> 
> Sorry, can you tell me what is the final your opinion about
> this patch?
> 
> I think that it not good to keep 4KiB as default and your main concern
> was about the default buffer size that depended on VLAN ifconfig.
> 
> So I could adopt 1536bytes as default and remove ifdef.

If the rx buffer size has to match the mtu then the minimum
size needs to be that for non-vlan packets.

OTOH if the rx buffer size only has to be 'long enough' and
a frame length check is done in software then you can always
use the vlan length and save reconfig is someone enables vlans.

My real concern was that you were setting the minimum buffer
size based on a vlan config option - which really didn't seem
right at all.

	David

Re: [PATCH (net.git) 2/4] stmmac: fix and better tune the default buffer sizes

[Giuseppe CAVALLARO]

On 3/5/2014 10:33 AM, David Laight wrote:
> From: Giuseppe CAVALLARO
>> Hello David
>>
>> Sorry, can you tell me what is the final your opinion about
>> this patch?
>>
>> I think that it not good to keep 4KiB as default and your main concern
>> was about the default buffer size that depended on VLAN ifconfig.
>>
>> So I could adopt 1536bytes as default and remove ifdef.
>
> If the rx buffer size has to match the mtu then the minimum
> size needs to be that for non-vlan packets.
>
> OTOH if the rx buffer size only has to be 'long enough' and
> a frame length check is done in software then you can always
> use the vlan length and save reconfig is someone enables vlans.

yes this is what I wanted to have.

> My real concern was that you were setting the minimum buffer
> size based on a vlan config option - which really didn't seem
> right at all.

ok, so I'll re-send a patch using as default 1536 and w/o surrounding
the code with ifdef (vlan).

thx
peppe

>
> 	David
>
>
>
>
>