[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.

Adjust it accordingly.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
by Otavio

 drivers/net/fec_mxc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 4cefda4..1a5105e 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 #define FEC_XFER_TIMEOUT	5000
 
+#define ARCH_DMA_MINALIGN_MX6SX	64
+
 #ifndef CONFIG_MII
 #error "CONFIG_MII has to be defined!"
 #endif
@@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
 	return 0;
 }
 
+static int dma_rx_align(void)
+{
+#ifdef CONFIG_MX6SX
+	return	ARCH_DMA_MINALIGN_MX6SX;
+#else
+	return ARCH_DMA_MINALIGN;
+#endif
+}
+
 /**
  * Initialize receive task's buffer descriptors
  * @param[in] fec all we know about the device yet
@@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize)
 	 * Reload the RX descriptors with default values and wipe
 	 * the RX buffers.
 	 */
-	size = roundup(dsize, ARCH_DMA_MINALIGN);
+	size = roundup(dsize, dma_rx_align());
 	for (i = 0; i < count; i++) {
 		data = (uint8_t *)fec->rbd_base[i].data_pointer;
 		memset(data, 0, dsize);
@@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec)
 	/* Allocate RX buffers. */
 
 	/* Maximum RX buffer size. */
-	size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN);
+	size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align());
 	for (i = 0; i < FEC_RBD_NUM; i++) {
-		data = memalign(ARCH_DMA_MINALIGN, size);
+		data = memalign(dma_rx_align(), size);
 		if (!data) {
 			printf("%s: error allocating rxbuf %d\n", __func__, i);
 			goto err_ring;
-- 
1.9.1

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Do not indicate an error when the buffer ready flag (FEC_TBD_READY) is set.

Without this change, mx6solox is not capable of doing TFTP transfers.

Succesfully tested on mx25, mx28, mx51, mx53, mx6q, mx6sl and mx6sx.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- None

 drivers/net/fec_mxc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 1a5105e..2699f5a 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -726,8 +726,6 @@ static int fec_send(struct eth_device *dev, void *packet, int length)
 		ret = -EINVAL;
 
 	invalidate_dcache_range(addr, addr + size);
-	if (readw(&fec->tbd_base[fec->tbd_index].status) & FEC_TBD_READY)
-		ret = -EINVAL;
 
 	debug("fec_send: status 0x%x index %d ret %i\n",
 			readw(&fec->tbd_base[fec->tbd_index].status),
-- 
1.9.1

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Otavio Salvador]

On Wed, Aug 20, 2014 at 6:24 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Do not indicate an error when the buffer ready flag (FEC_TBD_READY) is set.
>
> Without this change, mx6solox is not capable of doing TFTP transfers.
>
> Succesfully tested on mx25, mx28, mx51, mx53, mx6q, mx6sl and mx6sx.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

As you explicitly tested it in several SoC types it seems right
however Marek has explicitly add this code in:

    FEC: Rework the TX wait mechanism

    The mechanism waiting for transmission to finish in fec_send() now
    relies on the E-bit being cleared in the TX buffer descriptor. In
    case of data cache being on, this means invalidation of data cache
    above this TX buffer descriptor on each test for the E-bit being
    cleared.

    Apparently, there is another way to check if the transmission did
    complete. This is by checking the TDAR bit in the X_DES_ACTIVE
    register. Reading a register does not need any data cache invalidation,
    which is beneficial.

    Rework the sequence that wait for completion of the transmission so that
    the TDAR bit is tested first and afterwards check the E-bit being clear.
    This cuts down the number of cache invalidation calls to one.

May Marek recall why this was need?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Li Ye-B37916]

Hi Fabio,

    I feel the name "ARCH_DMA_MINALIGN_MX6SX" is not good here. Because the 64 bytes alignment is only required by the DMA engine in FEC controller, not a ARCH DMA value.

Best regards,
Ye Li

On 8/21/2014 5:24 AM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.
>
> Adjust it accordingly.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
> by Otavio
>
>  drivers/net/fec_mxc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 4cefda4..1a5105e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
>   */
>  #define FEC_XFER_TIMEOUT	5000
>  
> +#define ARCH_DMA_MINALIGN_MX6SX	64
> +
>  #ifndef CONFIG_MII
>  #error "CONFIG_MII has to be defined!"
>  #endif
> @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
>  	return 0;
>  }
>  
> +static int dma_rx_align(void)
> +{
> +#ifdef CONFIG_MX6SX
> +	return	ARCH_DMA_MINALIGN_MX6SX;
> +#else
> +	return ARCH_DMA_MINALIGN;
> +#endif
> +}
> +
>  /**
>   * Initialize receive task's buffer descriptors
>   * @param[in] fec all we know about the device yet
> @@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize)
>  	 * Reload the RX descriptors with default values and wipe
>  	 * the RX buffers.
>  	 */
> -	size = roundup(dsize, ARCH_DMA_MINALIGN);
> +	size = roundup(dsize, dma_rx_align());
>  	for (i = 0; i < count; i++) {
>  		data = (uint8_t *)fec->rbd_base[i].data_pointer;
>  		memset(data, 0, dsize);
> @@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec)
>  	/* Allocate RX buffers. */
>  
>  	/* Maximum RX buffer size. */
> -	size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN);
> +	size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align());
>  	for (i = 0; i < FEC_RBD_NUM; i++) {
> -		data = memalign(ARCH_DMA_MINALIGN, size);
> +		data = memalign(dma_rx_align(), size);
>  		if (!data) {
>  			printf("%s: error allocating rxbuf %d\n", __func__, i);
>  			goto err_ring;

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Marek Vasut]

On Wednesday, August 20, 2014 at 11:34:30 PM, Otavio Salvador wrote:
> On Wed, Aug 20, 2014 at 6:24 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Do not indicate an error when the buffer ready flag (FEC_TBD_READY) is
> > set.
> > 
> > Without this change, mx6solox is not capable of doing TFTP transfers.
> > 
> > Succesfully tested on mx25, mx28, mx51, mx53, mx6q, mx6sl and mx6sx.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> As you explicitly tested it in several SoC types it seems right
> however Marek has explicitly add this code in:
> 
>     FEC: Rework the TX wait mechanism
> 
>     The mechanism waiting for transmission to finish in fec_send() now
>     relies on the E-bit being cleared in the TX buffer descriptor. In
>     case of data cache being on, this means invalidation of data cache
>     above this TX buffer descriptor on each test for the E-bit being
>     cleared.
> 
>     Apparently, there is another way to check if the transmission did
>     complete. This is by checking the TDAR bit in the X_DES_ACTIVE
>     register. Reading a register does not need any data cache invalidation,
>     which is beneficial.
> 
>     Rework the sequence that wait for completion of the transmission so
> that the TDAR bit is tested first and afterwards check the E-bit being
> clear. This cuts down the number of cache invalidation calls to one.

It would come very helpful if you also provided the commit hash, so people can 
observe what was the contents of the patch. So let me fill it in:

Commit: 67449098a86be18cbdb27345bebe8da57e5d8899

> May Marek recall why this was need?

It's explained in the commit message above, quote:

'In case of data cache being on, this means invalidation of data cache above 
this TX buffer descriptor on each test for the E-bit being cleared.'

This means that it avoids the constant ping-pong between cache and DRAM. That is 
a loop consisting of:
- invalidating D-cache
- doing memory access to re-load the DMA descriptor from DRAM
- checking the E-bit in the DMA descriptor.
Instead, it reads the status from uncached register space of the FEC.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Marek Vasut]

On Wednesday, August 20, 2014 at 11:24:35 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.
> 
> Adjust it accordingly.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
> by Otavio
> 
>  drivers/net/fec_mxc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 4cefda4..1a5105e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
>   */
>  #define FEC_XFER_TIMEOUT	5000
> 
> +#define ARCH_DMA_MINALIGN_MX6SX	64
> +
>  #ifndef CONFIG_MII
>  #error "CONFIG_MII has to be defined!"
>  #endif
> @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
>  	return 0;
>  }
> 
> +static int dma_rx_align(void)
> +{
> +#ifdef CONFIG_MX6SX
> +	return	ARCH_DMA_MINALIGN_MX6SX;
> +#else
> +	return ARCH_DMA_MINALIGN;
> +#endif
> +}

This is not really an alignment, is it ? What is this really, cacheline length 
problem or what ? See RXDESC_PER_CACHELINE in the driver and make sure you don't 
break that mechanism as it would bite you ...

btw. you're using tab/space inconsistently past the 'return' keyword. Also, use 
the fec_ prefix in the functions in this driver consistently please.
[..]

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Marek Vasut]

On Wednesday, August 20, 2014 at 11:24:36 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Do not indicate an error when the buffer ready flag (FEC_TBD_READY) is set.
> 
> Without this change, mx6solox is not capable of doing TFTP transfers.
> 
> Succesfully tested on mx25, mx28, mx51, mx53, mx6q, mx6sl and mx6sx.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - None
> 
>  drivers/net/fec_mxc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 1a5105e..2699f5a 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -726,8 +726,6 @@ static int fec_send(struct eth_device *dev, void
> *packet, int length) ret = -EINVAL;
> 
>  	invalidate_dcache_range(addr, addr + size);
> -	if (readw(&fec->tbd_base[fec->tbd_index].status) & FEC_TBD_READY)
> -		ret = -EINVAL;

Uh, this means that if the buffer didn't complete for whatever reason, you will 
happily proceed and claim that this buffer you sent is really sent. You will 
never figure out that you need to re-send it. Sorry, but such a change cannot be
applied, since that just allows errors to creep in. Is there a bug in the MX6SX
or something so that it doesn't set this bit ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Ye Li]

The TDAR bit is set when the descriptors are all out from TX ring, but the descriptor properly is in transmitting not READY.  These are two signals, and in Ic simulation, we found the TDAR always clear prior than the READY bit of last BD. 
In mx6solox, we use a latest version of FEC IP. It looks the intrinsic behave of TDAR bit is changed in this FEC version, not any bug in mx6sx.

There are some solutions for this problem. Which would be acceptable?
1. Change the TDAR polling to checking the READY bit in BD. 
2. Add polling the READY bit of BD after the TDAR polling.
3. Add a delay after the TDAR polling.

Best regards,
Ye Li

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de] 
Sent: Thursday, August 21, 2014 11:53 AM
To: Fabio Estevam
Cc: joe.hershberger at gmail.com; sbabic at denx.de; u-boot at lists.denx.de; Li Ye-B37916; otavio at ossystems.com.br; Estevam Fabio-R49496
Subject: Re: [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

On Wednesday, August 20, 2014 at 11:24:36 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Do not indicate an error when the buffer ready flag (FEC_TBD_READY) is set.
> 
> Without this change, mx6solox is not capable of doing TFTP transfers.
> 
> Succesfully tested on mx25, mx28, mx51, mx53, mx6q, mx6sl and mx6sx.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - None
> 
>  drivers/net/fec_mxc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 
> 1a5105e..2699f5a 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -726,8 +726,6 @@ static int fec_send(struct eth_device *dev, void 
> *packet, int length) ret = -EINVAL;
> 
>  	invalidate_dcache_range(addr, addr + size);
> -	if (readw(&fec->tbd_base[fec->tbd_index].status) & FEC_TBD_READY)
> -		ret = -EINVAL;

Uh, this means that if the buffer didn't complete for whatever reason, you will happily proceed and claim that this buffer you sent is really sent. You will never figure out that you need to re-send it. Sorry, but such a change cannot be applied, since that just allows errors to creep in. Is there a bug in the MX6SX or something so that it doesn't set this bit ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Marek Vasut]

On Thursday, August 21, 2014 at 06:11:16 AM, Ye Li wrote:
> The TDAR bit is set when the descriptors are all out from TX ring, but the
> descriptor properly is in transmitting not READY.

I don't quite understand this, can you please rephrase ?

> These are two signals,
> and in Ic simulation, we found the TDAR always clear prior than the READY
> bit of last BD. In mx6solox, we use a latest version of FEC IP. It looks
> the intrinsic behave of TDAR bit is changed in this FEC version, not any
> bug in mx6sx.

OK, so this behavior is isolated to MX6SX and newer. Then any adjustment should 
focus solely on MX6SX and newer.

> There are some solutions for this problem. Which would be acceptable?
> 1. Change the TDAR polling to checking the READY bit in BD.

This would return the cache-grinding, so this is not nice.

> 2. Add polling the READY bit of BD after the TDAR polling.

If this would be isolated to MX6SX only, then that is doable.

> 3. Add a delay after the TDAR polling.

This is just work.

> 
> Best regards,
> Ye Li

Thanks for clarifying. Also, can you please stop top-posting when sending to 
mailing lists ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Stefan Roese]

Hi Fabio,

On 20.08.2014 23:24, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.
>
> Adjust it accordingly.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
> by Otavio
>
>   drivers/net/fec_mxc.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 4cefda4..1a5105e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
>    */
>   #define FEC_XFER_TIMEOUT	5000
>
> +#define ARCH_DMA_MINALIGN_MX6SX	64
> +
>   #ifndef CONFIG_MII
>   #error "CONFIG_MII has to be defined!"
>   #endif
> @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
>   	return 0;
>   }
>
> +static int dma_rx_align(void)
> +{
> +#ifdef CONFIG_MX6SX
> +	return	ARCH_DMA_MINALIGN_MX6SX;
> +#else
> +	return ARCH_DMA_MINALIGN;
> +#endif

Why don't you just use the bigger value (64) for all SoC versions? 
Shouldn't hurt, right. And would keep the source clean.

Thanks,
Stefan

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Stefano Babic]

Hi,

On 21/08/2014 07:02, Marek Vasut wrote:
> On Thursday, August 21, 2014 at 06:11:16 AM, Ye Li wrote:
>> The TDAR bit is set when the descriptors are all out from TX ring, but the
>> descriptor properly is in transmitting not READY.
> 
> I don't quite understand this, can you please rephrase ?
> 
>> These are two signals,
>> and in Ic simulation, we found the TDAR always clear prior than the READY
>> bit of last BD. In mx6solox, we use a latest version of FEC IP. It looks
>> the intrinsic behave of TDAR bit is changed in this FEC version, not any
>> bug in mx6sx.
> 
> OK, so this behavior is isolated to MX6SX and newer. Then any adjustment should 
> focus solely on MX6SX and newer.

Not really. It looks like that the implementation is suitable for
current FEC IP, but this does not mean that is correct at all. A
solution working for all FEC IP versions is surely preferable.

> 
>> There are some solutions for this problem. Which would be acceptable?
>> 1. Change the TDAR polling to checking the READY bit in BD.
> 
> This would return the cache-grinding, so this is not nice.
> 

Agree.

>> 2. Add polling the READY bit of BD after the TDAR polling.
> 
> If this would be isolated to MX6SX only, then that is doable.
> 

Why not ? On current FEC IP (not MX6SX), this costs only one read of the
BD register. Only by MX6SX is polled again. This looks to me the best
solution.

>> 3. Add a delay after the TDAR polling.
> 
> This is just work.

Of course, but it is a trick and we have to add some magical number of
uSec, explaining that with "so it works". My preference goes to 2.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Stefano Babic]

Hi Fabio,

On 20/08/2014 23:24, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.
> 
> Adjust it accordingly.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
> by Otavio
> 
>  drivers/net/fec_mxc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 4cefda4..1a5105e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
>   */
>  #define FEC_XFER_TIMEOUT	5000
>  
> +#define ARCH_DMA_MINALIGN_MX6SX	64
> +
>  #ifndef CONFIG_MII
>  #error "CONFIG_MII has to be defined!"
>  #endif
> @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
>  	return 0;
>  }
>  
> +static int dma_rx_align(void)
> +{
> +#ifdef CONFIG_MX6SX
> +	return	ARCH_DMA_MINALIGN_MX6SX;
> +#else
> +	return ARCH_DMA_MINALIGN;
> +#endif
> +}
> +

I go just a bit further according to Otavio's comment. In this way, we
do not remove #ifdef, we have only moved. What about using is_cpu_type()
instead of this ? If you do not want to add the macro for not mx6 socs
(preferable way, I think), the get_cpu_rev() should be available for all
i.MXes.

>  /**
>   * Initialize receive task's buffer descriptors
>   * @param[in] fec all we know about the device yet
> @@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize)
>  	 * Reload the RX descriptors with default values and wipe
>  	 * the RX buffers.
>  	 */
> -	size = roundup(dsize, ARCH_DMA_MINALIGN);
> +	size = roundup(dsize, dma_rx_align());
>  	for (i = 0; i < count; i++) {
>  		data = (uint8_t *)fec->rbd_base[i].data_pointer;
>  		memset(data, 0, dsize);
> @@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec)
>  	/* Allocate RX buffers. */
>  
>  	/* Maximum RX buffer size. */
> -	size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN);
> +	size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align());
>  	for (i = 0; i < FEC_RBD_NUM; i++) {
> -		data = memalign(ARCH_DMA_MINALIGN, size);
> +		data = memalign(dma_rx_align(), size);
>  		if (!data) {
>  			printf("%s: error allocating rxbuf %d\n", __func__, i);
>  			goto err_ring;
> 

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Li Ye-B37916]

On 8/21/2014 3:57 PM, Stefano Babic wrote:
> Hi,
>
> On 21/08/2014 07:02, Marek Vasut wrote:
>> On Thursday, August 21, 2014 at 06:11:16 AM, Ye Li wrote:
>>> The TDAR bit is set when the descriptors are all out from TX ring, but the
>>> descriptor properly is in transmitting not READY.
>> I don't quite understand this, can you please rephrase ?
When transmitting data,  FEC internal DMA reads the TX descriptor and move the data from the buffer pointed by TX descriptor to FEC internal FIFO. All TX descriptors are managed in a ring. 
We found the TDAR is cleared at DMA starting last descriptor of the ring, not at DMA having last descriptor finished. So this bit clears earlier than the READY bit of last descriptor. The delay is the time for the data sending of last descriptor.
>>> These are two signals,
>>> and in Ic simulation, we found the TDAR always clear prior than the READY
>>> bit of last BD. In mx6solox, we use a latest version of FEC IP. It looks
>>> the intrinsic behave of TDAR bit is changed in this FEC version, not any
>>> bug in mx6sx.
>> OK, so this behavior is isolated to MX6SX and newer. Then any adjustment should 
>> focus solely on MX6SX and newer.
> Not really. It looks like that the implementation is suitable for
> current FEC IP, but this does not mean that is correct at all. A
> solution working for all FEC IP versions is surely preferable.
Agree that the solution should cover all FEC IP versions. Thus, to wait all BD sent by FEC, we must check not only the TDAR but also the READY bit in last BD.
>>> There are some solutions for this problem. Which would be acceptable?
>>> 1. Change the TDAR polling to checking the READY bit in BD.
>> This would return the cache-grinding, so this is not nice.
>>
> Agree.
>
>>> 2. Add polling the READY bit of BD after the TDAR polling.
>> If this would be isolated to MX6SX only, then that is doable.
>>
> Why not ? On current FEC IP (not MX6SX), this costs only one read of the
> BD register. Only by MX6SX is polled again. This looks to me the best
> solution.
>
>>> 3. Add a delay after the TDAR polling.
>> This is just work.
> Of course, but it is a trick and we have to add some magical number of
> uSec, explaining that with "so it works". My preference goes to 2.
The delay time is relevant with the data length and transmit frequency,  this is the difficulty.  A long delay decreases the performance.
>
> Best regards,
> Stefano Babic
>
Best regards,
Ye Li

[U-Boot] [PATCH v2 2/2] net: fec_mxc: Do not error out when FEC_TBD_READY

[Marek Vasut]

On Thursday, August 21, 2014 at 09:57:23 AM, Stefano Babic wrote:
> Hi,
> 
> On 21/08/2014 07:02, Marek Vasut wrote:
> > On Thursday, August 21, 2014 at 06:11:16 AM, Ye Li wrote:
> >> The TDAR bit is set when the descriptors are all out from TX ring, but
> >> the descriptor properly is in transmitting not READY.
> > 
> > I don't quite understand this, can you please rephrase ?
> > 
> >> These are two signals,
> >> and in Ic simulation, we found the TDAR always clear prior than the
> >> READY bit of last BD. In mx6solox, we use a latest version of FEC IP.
> >> It looks the intrinsic behave of TDAR bit is changed in this FEC
> >> version, not any bug in mx6sx.
> > 
> > OK, so this behavior is isolated to MX6SX and newer. Then any adjustment
> > should focus solely on MX6SX and newer.
> 
> Not really. It looks like that the implementation is suitable for
> current FEC IP, but this does not mean that is correct at all. A
> solution working for all FEC IP versions is surely preferable.

This implementation is not suitable, since the behavior of the "new" MX6SX IP 
core differs from the old cores. The old cores did set up the descriptor bit 
properly and this check is in place to trigger a resubmission of datagram if the 
TDAR and the DMA descriptor statuses were out of sync.

> >> There are some solutions for this problem. Which would be acceptable?
> >> 1. Change the TDAR polling to checking the READY bit in BD.
> > 
> > This would return the cache-grinding, so this is not nice.
> 
> Agree.
> 
> >> 2. Add polling the READY bit of BD after the TDAR polling.
> > 
> > If this would be isolated to MX6SX only, then that is doable.
> 
> Why not ? On current FEC IP (not MX6SX), this costs only one read of the
> BD register. Only by MX6SX is polled again. This looks to me the best
> solution.

This would be true only if TDAR and BD are in sync, otherwise this would 
introduce change in behavior. But I'm fine with this, it shouldn't be harmful 
either way.

> >> 3. Add a delay after the TDAR polling.
> > 
> > This is just work.
> 
> Of course, but it is a trick and we have to add some magical number of
> uSec, explaining that with "so it works". My preference goes to 2.

Yeah.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Fabio Estevam]

On Thu, Aug 21, 2014 at 5:01 AM, Stefano Babic <sbabic@denx.de> wrote:

> I go just a bit further according to Otavio's comment. In this way, we
> do not remove #ifdef, we have only moved. What about using is_cpu_type()
> instead of this ? If you do not want to add the macro for not mx6 socs
> (preferable way, I think), the get_cpu_rev() should be available for all
> i.MXes.

Yes, the issue with this approach is that is_cpu_type() is currently
only available for mx6.

mx5, mx27, mx25, mx28 also need to be able to 'see' is_cpu_type(), but
this requires some work to do though.

[U-Boot] [PATCH v2 1/2] net: fec_mxc: Adjust RX DMA alignment for mx6solox

[Fabio Estevam]

On Thu, Aug 21, 2014 at 3:03 AM, Stefan Roese <sr@denx.de> wrote:

> Why don't you just use the bigger value (64) for all SoC versions? Shouldn't
> hurt, right. And would keep the source clean.

I think this should work fine. Thanks for the suggestion, Stefan.