[PATCH 1/2] net: macb: remove gem_recv and reorder probe

[PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Steffen Trumtrar]

The function gem_recv implements a buffer "ring" that stops at filling level 10.
That means, when the driver is used as gem, it stops receiving after exactly
10 packets. Instead of implementing macb_recv twice, use it also for the gem
part. If there needs to be an extra recv function for the gigabit case, it can
be added than.
Also, first set the type of device (macb or gem) and then use functions that
use this info.

Tested on a Zynq7000.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/net/macb.c | 47 +++++++----------------------------------------
 1 file changed, 7 insertions(+), 40 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 8602437..a12eea7 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -169,37 +169,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
 	macb->rx_tail = new_tail;
 }
 
-static int gem_recv(struct eth_device *edev)
-{
-	struct macb_device *macb = edev->priv;
-	unsigned int rx_tail = macb->rx_tail;
-	void *buffer;
-	int length;
-	u32 status;
-
-	dev_dbg(macb->dev, "%s\n", __func__);
-
-	for (;;) {
-		barrier();
-		if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
-			return -1;
-
-		barrier();
-		status = macb->rx_ring[rx_tail].ctrl;
-		length = MACB_BFEXT(RX_FRMLEN, status);
-		if (status & MACB_BIT(RX_SOF)) {
-			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
-			net_receive(buffer, length);
-			macb->rx_ring[rx_tail].ctrl &= ~MACB_BIT(RX_USED);
-			barrier();
-		}
-		rx_tail++;
-		macb->rx_tail++;
-	}
-
-	return 0;
-}
-
 static int macb_recv(struct eth_device *edev)
 {
 	struct macb_device *macb = edev->priv;
@@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
 
 	macb->phy_flags = pdata->phy_flags;
 
-	macb_init_rx_buffer_size(macb, PKTSIZE);
-	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
-	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
-	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
-
 	macb->regs = dev_request_mem_region(dev, 0);
 
 	/*
@@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
 
 	clk_enable(macb->pclk);
 
-	if (macb_is_gem(macb))
-		edev->recv = gem_recv;
-	else
-		edev->recv = macb_recv;
 	macb->is_gem = read_is_gem(macb);
 
+	macb_init_rx_buffer_size(macb, PKTSIZE);
+	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
+	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
+	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
+
+	edev->recv = macb_recv;
+
 	macb_reset_hw(macb);
 	ncfgr = macb_mdc_clk_div(macb);
 	ncfgr |= MACB_BIT(PAE);		/* PAuse Enable */
-- 
1.8.2.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] net: macb: turn off endian_swp_pkt_en

[Steffen Trumtrar]

The core has a bit for swapping packet data endianism.
Reset default from Cadence is off. Xilinx however, that uses this core on the
Zynq SoCs, opted for on. Turn it off for all devices.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/net/macb.c | 1 +
 drivers/net/macb.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index a12eea7..005234e 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -277,6 +277,7 @@ static void macb_configure_dma(struct macb_device *bp)
 		dmacfg |= GEM_BF(FBLDO, 16);
 		dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
 		dmacfg |= GEM_BIT(DDRP);
+		dmacfg &= ~GEM_BIT(ENDIA);
 		gem_writel(bp, DMACFG, dmacfg);
 	}
 }
diff --git a/drivers/net/macb.h b/drivers/net/macb.h
index cadd561..1be9ff9 100644
--- a/drivers/net/macb.h
+++ b/drivers/net/macb.h
@@ -168,6 +168,8 @@
 /* Bitfields in DMACFG. */
 #define GEM_FBLDO_OFFSET			0
 #define GEM_FBLDO_SIZE				5
+#define GEM_ENDIA_OFFSET			7
+#define GEM_ENDIA_SIZE				1
 #define GEM_RXBMS_OFFSET			8
 #define GEM_RXBMS_SIZE				2
 #define GEM_TXPBMS_OFFSET			10
-- 
1.8.2.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> The function gem_recv implements a buffer "ring" that stops at filling level 10.
> That means, when the driver is used as gem, it stops receiving after exactly
> 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> part. If there needs to be an extra recv function for the gigabit case, it can
> be added than.
> Also, first set the type of device (macb or gem) and then use functions that
> use this info.
> 
> Tested on a Zynq7000.
NACK

on gem we can receive the packet in one buffer the gem_recv implement this
the macb can only receive splited buffer and then you have to reconstruct the
packet

Best Regards,
J.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  drivers/net/macb.c | 47 +++++++----------------------------------------
>  1 file changed, 7 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 8602437..a12eea7 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -169,37 +169,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
>  	macb->rx_tail = new_tail;
>  }
>  
> -static int gem_recv(struct eth_device *edev)
> -{
> -	struct macb_device *macb = edev->priv;
> -	unsigned int rx_tail = macb->rx_tail;
> -	void *buffer;
> -	int length;
> -	u32 status;
> -
> -	dev_dbg(macb->dev, "%s\n", __func__);
> -
> -	for (;;) {
> -		barrier();
> -		if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
> -			return -1;
> -
> -		barrier();
> -		status = macb->rx_ring[rx_tail].ctrl;
> -		length = MACB_BFEXT(RX_FRMLEN, status);
> -		if (status & MACB_BIT(RX_SOF)) {
> -			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
> -			net_receive(buffer, length);
> -			macb->rx_ring[rx_tail].ctrl &= ~MACB_BIT(RX_USED);
> -			barrier();
> -		}
> -		rx_tail++;
> -		macb->rx_tail++;
> -	}
> -
> -	return 0;
> -}
> -
>  static int macb_recv(struct eth_device *edev)
>  {
>  	struct macb_device *macb = edev->priv;
> @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
>  
>  	macb->phy_flags = pdata->phy_flags;
>  
> -	macb_init_rx_buffer_size(macb, PKTSIZE);
> -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> -
>  	macb->regs = dev_request_mem_region(dev, 0);
>  
>  	/*
> @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
>  
>  	clk_enable(macb->pclk);
>  
> -	if (macb_is_gem(macb))
> -		edev->recv = gem_recv;
> -	else
> -		edev->recv = macb_recv;
>  	macb->is_gem = read_is_gem(macb);
>  
> +	macb_init_rx_buffer_size(macb, PKTSIZE);
> +	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> +	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> +	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> +
> +	edev->recv = macb_recv;
> +
>  	macb_reset_hw(macb);
>  	ncfgr = macb_mdc_clk_div(macb);
>  	ncfgr |= MACB_BIT(PAE);		/* PAuse Enable */
> -- 
> 1.8.2.rc2
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] net: macb: turn off endian_swp_pkt_en

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> The core has a bit for swapping packet data endianism.
> Reset default from Cadence is off. Xilinx however, that uses this core on the
> Zynq SoCs, opted for on. Turn it off for all devices.

is this xilinx specifc?

on at91 and other we do not need it

Best Regards,
J.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  drivers/net/macb.c | 1 +
>  drivers/net/macb.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index a12eea7..005234e 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -277,6 +277,7 @@ static void macb_configure_dma(struct macb_device *bp)
>  		dmacfg |= GEM_BF(FBLDO, 16);
>  		dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
>  		dmacfg |= GEM_BIT(DDRP);
> +		dmacfg &= ~GEM_BIT(ENDIA);
>  		gem_writel(bp, DMACFG, dmacfg);
>  	}
>  }
> diff --git a/drivers/net/macb.h b/drivers/net/macb.h
> index cadd561..1be9ff9 100644
> --- a/drivers/net/macb.h
> +++ b/drivers/net/macb.h
> @@ -168,6 +168,8 @@
>  /* Bitfields in DMACFG. */
>  #define GEM_FBLDO_OFFSET			0
>  #define GEM_FBLDO_SIZE				5
> +#define GEM_ENDIA_OFFSET			7
> +#define GEM_ENDIA_SIZE				1
>  #define GEM_RXBMS_OFFSET			8
>  #define GEM_RXBMS_SIZE				2
>  #define GEM_TXPBMS_OFFSET			10
> -- 
> 1.8.2.rc2
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] net: macb: turn off endian_swp_pkt_en

[Steffen Trumtrar]

Hi!

On Wed, Mar 13, 2013 at 10:04:45AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > The core has a bit for swapping packet data endianism.
> > Reset default from Cadence is off. Xilinx however, that uses this core on the
> > Zynq SoCs, opted for on. Turn it off for all devices.
> 
> is this xilinx specifc?
> 
> on at91 and other we do not need it
> 

Well, as stated in the commit log, Cadence default is off. I guess, at91 does
not change this. So, where is the problem then forcing it to the sane default
from Cadence?

Regards,
str

> Best Regards,
> J.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  drivers/net/macb.c | 1 +
> >  drivers/net/macb.h | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index a12eea7..005234e 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -277,6 +277,7 @@ static void macb_configure_dma(struct macb_device *bp)
> >  		dmacfg |= GEM_BF(FBLDO, 16);
> >  		dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
> >  		dmacfg |= GEM_BIT(DDRP);
> > +		dmacfg &= ~GEM_BIT(ENDIA);
> >  		gem_writel(bp, DMACFG, dmacfg);
> >  	}
> >  }
> > diff --git a/drivers/net/macb.h b/drivers/net/macb.h
> > index cadd561..1be9ff9 100644
> > --- a/drivers/net/macb.h
> > +++ b/drivers/net/macb.h
> > @@ -168,6 +168,8 @@
> >  /* Bitfields in DMACFG. */
> >  #define GEM_FBLDO_OFFSET			0
> >  #define GEM_FBLDO_SIZE				5
> > +#define GEM_ENDIA_OFFSET			7
> > +#define GEM_ENDIA_SIZE				1
> >  #define GEM_RXBMS_OFFSET			8
> >  #define GEM_RXBMS_SIZE				2
> >  #define GEM_TXPBMS_OFFSET			10
> > -- 
> > 1.8.2.rc2
> > 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Steffen Trumtrar]

On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > The function gem_recv implements a buffer "ring" that stops at filling level 10.
> > That means, when the driver is used as gem, it stops receiving after exactly
> > 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> > part. If there needs to be an extra recv function for the gigabit case, it can
> > be added than.
> > Also, first set the type of device (macb or gem) and then use functions that
> > use this info.
> > 
> > Tested on a Zynq7000.
> NACK
> 
> on gem we can receive the packet in one buffer the gem_recv implement this
> the macb can only receive splited buffer and then you have to reconstruct the
> packet
> 

Okay. That is nice and all. But try receiving more than just 10 packets with
the current implementation.

Regards,
str

> Best Regards,
> J.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  drivers/net/macb.c | 47 +++++++----------------------------------------
> >  1 file changed, 7 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index 8602437..a12eea7 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -169,37 +169,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
> >  	macb->rx_tail = new_tail;
> >  }
> >  
> > -static int gem_recv(struct eth_device *edev)
> > -{
> > -	struct macb_device *macb = edev->priv;
> > -	unsigned int rx_tail = macb->rx_tail;
> > -	void *buffer;
> > -	int length;
> > -	u32 status;
> > -
> > -	dev_dbg(macb->dev, "%s\n", __func__);
> > -
> > -	for (;;) {
> > -		barrier();
> > -		if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
> > -			return -1;
> > -
> > -		barrier();
> > -		status = macb->rx_ring[rx_tail].ctrl;
> > -		length = MACB_BFEXT(RX_FRMLEN, status);
> > -		if (status & MACB_BIT(RX_SOF)) {
> > -			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
> > -			net_receive(buffer, length);
> > -			macb->rx_ring[rx_tail].ctrl &= ~MACB_BIT(RX_USED);
> > -			barrier();
> > -		}
> > -		rx_tail++;
> > -		macb->rx_tail++;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  static int macb_recv(struct eth_device *edev)
> >  {
> >  	struct macb_device *macb = edev->priv;
> > @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
> >  
> >  	macb->phy_flags = pdata->phy_flags;
> >  
> > -	macb_init_rx_buffer_size(macb, PKTSIZE);
> > -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > -
> >  	macb->regs = dev_request_mem_region(dev, 0);
> >  
> >  	/*
> > @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
> >  
> >  	clk_enable(macb->pclk);
> >  
> > -	if (macb_is_gem(macb))
> > -		edev->recv = gem_recv;
> > -	else
> > -		edev->recv = macb_recv;
> >  	macb->is_gem = read_is_gem(macb);
> >  
> > +	macb_init_rx_buffer_size(macb, PKTSIZE);
> > +	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > +	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > +	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > +
> > +	edev->recv = macb_recv;
> > +
> >  	macb_reset_hw(macb);
> >  	ncfgr = macb_mdc_clk_div(macb);
> >  	ncfgr |= MACB_BIT(PAE);		/* PAuse Enable */
> > -- 
> > 1.8.2.rc2
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Jean-Christophe PLAGNIOL-VILLARD]

On Mar 13, 2013, at 5:19 PM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:

> On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
>>> The function gem_recv implements a buffer "ring" that stops at filling level 10.
>>> That means, when the driver is used as gem, it stops receiving after exactly
>>> 10 packets. Instead of implementing macb_recv twice, use it also for the gem
>>> part. If there needs to be an extra recv function for the gigabit case, it can
>>> be added than.
>>> Also, first set the type of device (macb or gem) and then use functions that
>>> use this info.
>>> 
>>> Tested on a Zynq7000.
>> NACK
>> 
>> on gem we can receive the packet in one buffer the gem_recv implement this
>> the macb can only receive splited buffer and then you have to reconstruct the
>> packet
>> 
> 
> Okay. That is nice and all. But try receiving more than just 10 packets with
> the current implementation.

yes the drivers bufferize only 10 packets in the DMA which should be enough for barebox
if we want we can increase the dma buffer size by increasing RX_NB_PACKET

Best Regards,
J.

> 
> Regards,
> str
> 
>> Best Regards,
>> J.
>>> 
>>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>> ---
>>> drivers/net/macb.c | 47 +++++++----------------------------------------
>>> 1 file changed, 7 insertions(+), 40 deletions(-)
>>> 
>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>> index 8602437..a12eea7 100644
>>> --- a/drivers/net/macb.c
>>> +++ b/drivers/net/macb.c
>>> @@ -169,37 +169,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
>>> 	macb->rx_tail = new_tail;
>>> }
>>> 
>>> -static int gem_recv(struct eth_device *edev)
>>> -{
>>> -	struct macb_device *macb = edev->priv;
>>> -	unsigned int rx_tail = macb->rx_tail;
>>> -	void *buffer;
>>> -	int length;
>>> -	u32 status;
>>> -
>>> -	dev_dbg(macb->dev, "%s\n", __func__);
>>> -
>>> -	for (;;) {
>>> -		barrier();
>>> -		if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
>>> -			return -1;
>>> -
>>> -		barrier();
>>> -		status = macb->rx_ring[rx_tail].ctrl;
>>> -		length = MACB_BFEXT(RX_FRMLEN, status);
>>> -		if (status & MACB_BIT(RX_SOF)) {
>>> -			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
>>> -			net_receive(buffer, length);
>>> -			macb->rx_ring[rx_tail].ctrl &= ~MACB_BIT(RX_USED);
>>> -			barrier();
>>> -		}
>>> -		rx_tail++;
>>> -		macb->rx_tail++;
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> static int macb_recv(struct eth_device *edev)
>>> {
>>> 	struct macb_device *macb = edev->priv;
>>> @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
>>> 
>>> 	macb->phy_flags = pdata->phy_flags;
>>> 
>>> -	macb_init_rx_buffer_size(macb, PKTSIZE);
>>> -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
>>> -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
>>> -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
>>> -
>>> 	macb->regs = dev_request_mem_region(dev, 0);
>>> 
>>> 	/*
>>> @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
>>> 
>>> 	clk_enable(macb->pclk);
>>> 
>>> -	if (macb_is_gem(macb))
>>> -		edev->recv = gem_recv;
>>> -	else
>>> -		edev->recv = macb_recv;
>>> 	macb->is_gem = read_is_gem(macb);
>>> 
>>> +	macb_init_rx_buffer_size(macb, PKTSIZE);
>>> +	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
>>> +	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
>>> +	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
>>> +
>>> +	edev->recv = macb_recv;
>>> +
>>> 	macb_reset_hw(macb);
>>> 	ncfgr = macb_mdc_clk_div(macb);
>>> 	ncfgr |= MACB_BIT(PAE);		/* PAuse Enable */
>>> -- 
>>> 1.8.2.rc2
>>> 
>> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Jean-Christophe PLAGNIOL-VILLARD]

On Mar 13, 2013, at 5:32 PM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:

> 
> On Mar 13, 2013, at 5:19 PM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
>> On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
>>>> The function gem_recv implements a buffer "ring" that stops at filling level 10.
>>>> That means, when the driver is used as gem, it stops receiving after exactly
>>>> 10 packets. Instead of implementing macb_recv twice, use it also for the gem
>>>> part. If there needs to be an extra recv function for the gigabit case, it can
>>>> be added than.
>>>> Also, first set the type of device (macb or gem) and then use functions that
>>>> use this info.
>>>> 
>>>> Tested on a Zynq7000.
>>> NACK
>>> 
>>> on gem we can receive the packet in one buffer the gem_recv implement this
>>> the macb can only receive splited buffer and then you have to reconstruct the
>>> packet
>>> 
>> 
>> Okay. That is nice and all. But try receiving more than just 10 packets with
>> the current implementation.
> 
> yes the drivers bufferize only 10 packets in the DMA which should be enough for barebox
> if we want we can increase the dma buffer size by increasing RX_NB_PACKET

and this is valid for mach_recv and gem_recv btw

Best Regards,
J.
> 
> Best Regards,
> J.
> 
>> 
>> Regards,
>> str
>> 
>>> Best Regards,
>>> J.
>>>> 
>>>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>>>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>> ---
>>>> drivers/net/macb.c | 47 +++++++----------------------------------------
>>>> 1 file changed, 7 insertions(+), 40 deletions(-)
>>>> 
>>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>>> index 8602437..a12eea7 100644
>>>> --- a/drivers/net/macb.c
>>>> +++ b/drivers/net/macb.c
>>>> @@ -169,37 +169,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
>>>> 	macb->rx_tail = new_tail;
>>>> }
>>>> 
>>>> -static int gem_recv(struct eth_device *edev)
>>>> -{
>>>> -	struct macb_device *macb = edev->priv;
>>>> -	unsigned int rx_tail = macb->rx_tail;
>>>> -	void *buffer;
>>>> -	int length;
>>>> -	u32 status;
>>>> -
>>>> -	dev_dbg(macb->dev, "%s\n", __func__);
>>>> -
>>>> -	for (;;) {
>>>> -		barrier();
>>>> -		if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
>>>> -			return -1;
>>>> -
>>>> -		barrier();
>>>> -		status = macb->rx_ring[rx_tail].ctrl;
>>>> -		length = MACB_BFEXT(RX_FRMLEN, status);
>>>> -		if (status & MACB_BIT(RX_SOF)) {
>>>> -			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
>>>> -			net_receive(buffer, length);
>>>> -			macb->rx_ring[rx_tail].ctrl &= ~MACB_BIT(RX_USED);
>>>> -			barrier();
>>>> -		}
>>>> -		rx_tail++;
>>>> -		macb->rx_tail++;
>>>> -	}
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> static int macb_recv(struct eth_device *edev)
>>>> {
>>>> 	struct macb_device *macb = edev->priv;
>>>> @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
>>>> 
>>>> 	macb->phy_flags = pdata->phy_flags;
>>>> 
>>>> -	macb_init_rx_buffer_size(macb, PKTSIZE);
>>>> -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
>>>> -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
>>>> -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
>>>> -
>>>> 	macb->regs = dev_request_mem_region(dev, 0);
>>>> 
>>>> 	/*
>>>> @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
>>>> 
>>>> 	clk_enable(macb->pclk);
>>>> 
>>>> -	if (macb_is_gem(macb))
>>>> -		edev->recv = gem_recv;
>>>> -	else
>>>> -		edev->recv = macb_recv;
>>>> 	macb->is_gem = read_is_gem(macb);
>>>> 
>>>> +	macb_init_rx_buffer_size(macb, PKTSIZE);
>>>> +	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
>>>> +	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
>>>> +	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
>>>> +
>>>> +	edev->recv = macb_recv;
>>>> +
>>>> 	macb_reset_hw(macb);
>>>> 	ncfgr = macb_mdc_clk_div(macb);
>>>> 	ncfgr |= MACB_BIT(PAE);		/* PAuse Enable */
>>>> -- 
>>>> 1.8.2.rc2
>>>> 
>>> 
>> 
>> -- 
>> Pengutronix e.K.                           |                             |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Sascha Hauer]

On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > The function gem_recv implements a buffer "ring" that stops at filling level 10.
> > That means, when the driver is used as gem, it stops receiving after exactly
> > 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> > part. If there needs to be an extra recv function for the gigabit case, it can
> > be added than.
> > Also, first set the type of device (macb or gem) and then use functions that
> > use this info.
> > 
> > Tested on a Zynq7000.
> NACK
> 
> on gem we can receive the packet in one buffer the gem_recv implement this
> the macb can only receive splited buffer and then you have to reconstruct the
> packet

The gem received function was never used...

> >  static int macb_recv(struct eth_device *edev)
> >  {
> >  	struct macb_device *macb = edev->priv;
> > @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
> >  
> >  	macb->phy_flags = pdata->phy_flags;
> >  
> > -	macb_init_rx_buffer_size(macb, PKTSIZE);
> > -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > -
> >  	macb->regs = dev_request_mem_region(dev, 0);
> >  
> >  	/*
> > @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
> >  
> >  	clk_enable(macb->pclk);
> >  
> > -	if (macb_is_gem(macb))

... because macb_is_gem() is used here ...

> > -		edev->recv = gem_recv;
> > -	else
> > -		edev->recv = macb_recv;
> >  	macb->is_gem = read_is_gem(macb);

... but the variable is initialized here. Up to this point macb_is_gem()
will return 0.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:15 Wed 13 Mar     , Sascha Hauer wrote:
> On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > > The function gem_recv implements a buffer "ring" that stops at filling level 10.
> > > That means, when the driver is used as gem, it stops receiving after exactly
> > > 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> > > part. If there needs to be an extra recv function for the gigabit case, it can
> > > be added than.
> > > Also, first set the type of device (macb or gem) and then use functions that
> > > use this info.
> > > 
> > > Tested on a Zynq7000.
> > NACK
> > 
> > on gem we can receive the packet in one buffer the gem_recv implement this
> > the macb can only receive splited buffer and then you have to reconstruct the
> > packet
> 
> The gem received function was never used...
> 
> > >  static int macb_recv(struct eth_device *edev)
> > >  {
> > >  	struct macb_device *macb = edev->priv;
> > > @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
> > >  
> > >  	macb->phy_flags = pdata->phy_flags;
> > >  
> > > -	macb_init_rx_buffer_size(macb, PKTSIZE);
> > > -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > > -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > > -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > > -
> > >  	macb->regs = dev_request_mem_region(dev, 0);
> > >  
> > >  	/*
> > > @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
> > >  
> > >  	clk_enable(macb->pclk);
> > >  
> > > -	if (macb_is_gem(macb))
> 
> ... because macb_is_gem() is used here ...
> 
> > > -		edev->recv = gem_recv;
> > > -	else
> > > -		edev->recv = macb_recv;
> > >  	macb->is_gem = read_is_gem(macb);
> 
> ... but the variable is initialized here. Up to this point macb_is_gem()
> will return 0.
so this is a bug when the driver was merge to the mailine but here I does use
the gem_recv.

Best Regrards,
J.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Steffen Trumtrar]

On Wed, Mar 13, 2013 at 01:17:03PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:15 Wed 13 Mar     , Sascha Hauer wrote:
> > On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > > > The function gem_recv implements a buffer "ring" that stops at filling level 10.
> > > > That means, when the driver is used as gem, it stops receiving after exactly
> > > > 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> > > > part. If there needs to be an extra recv function for the gigabit case, it can
> > > > be added than.
> > > > Also, first set the type of device (macb or gem) and then use functions that
> > > > use this info.
> > > > 
> > > > Tested on a Zynq7000.
> > > NACK
> > > 
> > > on gem we can receive the packet in one buffer the gem_recv implement this
> > > the macb can only receive splited buffer and then you have to reconstruct the
> > > packet
> > 
> > The gem received function was never used...
> > 
> > > >  static int macb_recv(struct eth_device *edev)
> > > >  {
> > > >  	struct macb_device *macb = edev->priv;
> > > > @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
> > > >  
> > > >  	macb->phy_flags = pdata->phy_flags;
> > > >  
> > > > -	macb_init_rx_buffer_size(macb, PKTSIZE);
> > > > -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > > > -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > > > -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > > > -
> > > >  	macb->regs = dev_request_mem_region(dev, 0);
> > > >  
> > > >  	/*
> > > > @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
> > > >  
> > > >  	clk_enable(macb->pclk);
> > > >  
> > > > -	if (macb_is_gem(macb))
> > 
> > ... because macb_is_gem() is used here ...
> > 
> > > > -		edev->recv = gem_recv;
> > > > -	else
> > > > -		edev->recv = macb_recv;
> > > >  	macb->is_gem = read_is_gem(macb);
> > 
> > ... but the variable is initialized here. Up to this point macb_is_gem()
> > will return 0.
> so this is a bug when the driver was merge to the mailine but here I does use
> the gem_recv.
> 

What about the gem_recv function itself? It only increases rx_tail, but never
resets it. Therefore the buffer is full after 10 packets. Do you already have a
patch for that?

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] net: macb: remove gem_recv and reorder probe

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:08 Wed 13 Mar     , Steffen Trumtrar wrote:
> On Wed, Mar 13, 2013 at 01:17:03PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 11:15 Wed 13 Mar     , Sascha Hauer wrote:
> > > On Wed, Mar 13, 2013 at 10:03:36AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 09:48 Wed 13 Mar     , Steffen Trumtrar wrote:
> > > > > The function gem_recv implements a buffer "ring" that stops at filling level 10.
> > > > > That means, when the driver is used as gem, it stops receiving after exactly
> > > > > 10 packets. Instead of implementing macb_recv twice, use it also for the gem
> > > > > part. If there needs to be an extra recv function for the gigabit case, it can
> > > > > be added than.
> > > > > Also, first set the type of device (macb or gem) and then use functions that
> > > > > use this info.
> > > > > 
> > > > > Tested on a Zynq7000.
> > > > NACK
> > > > 
> > > > on gem we can receive the packet in one buffer the gem_recv implement this
> > > > the macb can only receive splited buffer and then you have to reconstruct the
> > > > packet
> > > 
> > > The gem received function was never used...
> > > 
> > > > >  static int macb_recv(struct eth_device *edev)
> > > > >  {
> > > > >  	struct macb_device *macb = edev->priv;
> > > > > @@ -619,11 +588,6 @@ static int macb_probe(struct device_d *dev)
> > > > >  
> > > > >  	macb->phy_flags = pdata->phy_flags;
> > > > >  
> > > > > -	macb_init_rx_buffer_size(macb, PKTSIZE);
> > > > > -	macb->rx_buffer = dma_alloc_coherent(macb->rx_buffer_size * macb->rx_ring_size);
> > > > > -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb));
> > > > > -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES);
> > > > > -
> > > > >  	macb->regs = dev_request_mem_region(dev, 0);
> > > > >  
> > > > >  	/*
> > > > > @@ -638,12 +602,15 @@ static int macb_probe(struct device_d *dev)
> > > > >  
> > > > >  	clk_enable(macb->pclk);
> > > > >  
> > > > > -	if (macb_is_gem(macb))
> > > 
> > > ... because macb_is_gem() is used here ...
> > > 
> > > > > -		edev->recv = gem_recv;
> > > > > -	else
> > > > > -		edev->recv = macb_recv;
> > > > >  	macb->is_gem = read_is_gem(macb);
> > > 
> > > ... but the variable is initialized here. Up to this point macb_is_gem()
> > > will return 0.
> > so this is a bug when the driver was merge to the mailine but here I does use
> > the gem_recv.
> > 
> 
> What about the gem_recv function itself? It only increases rx_tail, but never
> resets it. Therefore the buffer is full after 10 packets. Do you already have a
> patch for that?

yes I check I've the diff with my local tree

I send it on the top of your patch

Best Regards,
J.

> 
> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox