diff mbox

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

Message ID 1408569876-28539-2-git-send-email-festevam@gmail.com
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Fabio Estevam Aug. 20, 2014, 9:24 p.m. UTC 
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(-)

Comments

Otavio Salvador Aug. 20, 2014, 9:34 p.m. UTC | #1 
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?
Marek Vasut Aug. 21, 2014, 3:47 a.m. UTC | #2 
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
Marek Vasut Aug. 21, 2014, 3:53 a.m. UTC | #3 
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
Ye Li Aug. 21, 2014, 4:11 a.m. UTC | #4 
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@denx.de] 
Sent: Thursday, August 21, 2014 11:53 AM
To: Fabio Estevam
Cc: joe.hershberger@gmail.com; sbabic@denx.de; u-boot@lists.denx.de; Li Ye-B37916; otavio@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
Marek Vasut Aug. 21, 2014, 5:02 a.m. UTC | #5 
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
Stefano Babic Aug. 21, 2014, 7:57 a.m. UTC | #6 
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
Ye.Li Aug. 21, 2014, 8:35 a.m. UTC | #7 
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
Marek Vasut Aug. 21, 2014, 11:39 a.m. UTC | #8 
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
diff mbox

Patch

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