net: fec_mxc: Adjust RX DMA alignment for mx6solox

From: Fabio Estevam <[email protected]>

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 <[email protected]>

Quoted 10 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

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 <[email protected]>

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?

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
Changes since v1:
- None

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

drivers/net/fec_mxc.c +0 -0

index 1a5105e..2699f5a 100644

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
Changes since v1:
- None

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

drivers/net/fec_mxc.c index renamed from drivers/net/fec_mxc.c +0 -0

1a5105e..2699f5a 100644

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

Quoted 2 lines · 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.

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

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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

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

Quoted 1 line

3. Add a delay after the TDAR polling.

This is just work.

Hi,

Quoted 4 lines · Marek Vasut

Quoted 2 lines · 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.

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 2 lines

Quoted 1 line

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

Hi,

Quoted 12 lines · Marek Vasut

Quoted 2 lines · 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.

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

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line

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.

From: Fabio Estevam <[email protected]>

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

Adjust it accordingly.

Signed-off-by Fabio Estevam <[email protected]>

---
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(-)

drivers/net/fec_mxc.c +14 -3

index 4cefda4..1a5105e 100644
@@ -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;

From: Fabio Estevam <[email protected]>

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

Adjust it accordingly.

Signed-off-by Fabio Estevam <[email protected]>

---
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(-)

drivers/net/fec_mxc.c +2 -0

index 4cefda4..1a5105e 100644
@@ -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;

From: Fabio Estevam <[email protected]>

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

Adjust it accordingly.

Signed-off-by Fabio Estevam <[email protected]>

---
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(-)

drivers/net/fec_mxc.c +2 -0

index 4cefda4..1a5105e 100644
@@ -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

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

From: Fabio Estevam <[email protected]>

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

Adjust it accordingly.

Signed-off-by Fabio Estevam <[email protected]>

---
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(-)

drivers/net/fec_mxc.c +2 -0

index 4cefda4..1a5105e 100644
@@ -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;
 }

drivers/net/fec_mxc.c +1 -1

 /**
  * 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. */

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.