Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
On 24 April 2015 at 17:04, D. Dueck wrote:
> As requested:
> Tested-by: David Dueck
>
>
> Am Freitag, 24. April 2015 schrieb Jagan Teki :
>>
>> On 7 April 2015 at 05:55, Tom Rini wrote:
>> > On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
>> >> Hi David,
>> >>
>> >>
>> >>
>> >> > for (i = 0; i < len; i++) {
>> >> > /* wait till TX register is empty (TXS == 1) */
>> >> > + start = get_timer(0);
>> >> > while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
>> >> > OMAP3_MCSPI_CHSTAT_TXS)) {
>> >> > - if (--timeout <= 0) {
>> >> > + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
>> >> > printf("SPI TXS timed out,
>> >> > status=0x%08x\n",
>> >> >readl(&ds->regs->channel[ds-
>> >> > >slave.cs].chstat));
>> >> > return -1;
>> >>
>> >> I have a couple of questions...
>> >>
>> >> Firstly, when in SPL is there access to the get_timer() function?
>> >
>> > We call timer_init() from board_init_r() in SPL, prior to diving down
>> > into loading (or checking for Falcon vs Regular) so this is safe.
>> >
>> >> Secondly, when using Falcon mode to load Linux directly from SPI
>> >> (Falcon
>> >> mode) then we want to maximise the throughput and save every CPU cycle
>> >> we
>> >> possibly can. Adding yet another function call into the for loop and
>> >> hence
>> >> calling it a couple of million times seems, on the face of it, like it
>> >> is
>> >> going to slow things down.
>> >
>> > I'd like to see measurements to prove me wrong but this both seems like
>> > a bad idea (optimizing by being incorrect, this gives us a correct
>> > timeout check like other drivers do) and really unlikely I would think
>> > to be noticable. Since we'll be doing the same code-paths in both
>> > regular and SPL, trying to time things (by loading a big file) would be
>> > easy enough I think. Thanks!
>>
>> Ping
Applied to u-boot-spi/master
thanks!
--
Jagan.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
On 7 April 2015 at 05:55, Tom Rini wrote:
> On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
>> Hi David,
>>
>>
>>
>> > for (i = 0; i < len; i++) {
>> > /* wait till TX register is empty (TXS == 1) */
>> > + start = get_timer(0);
>> > while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
>> > OMAP3_MCSPI_CHSTAT_TXS)) {
>> > - if (--timeout <= 0) {
>> > + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
>> > printf("SPI TXS timed out, status=0x%08x\n",
>> >readl(&ds->regs->channel[ds-
>> > >slave.cs].chstat));
>> > return -1;
>>
>> I have a couple of questions...
>>
>> Firstly, when in SPL is there access to the get_timer() function?
>
> We call timer_init() from board_init_r() in SPL, prior to diving down
> into loading (or checking for Falcon vs Regular) so this is safe.
>
>> Secondly, when using Falcon mode to load Linux directly from SPI (Falcon
>> mode) then we want to maximise the throughput and save every CPU cycle we
>> possibly can. Adding yet another function call into the for loop and hence
>> calling it a couple of million times seems, on the face of it, like it is
>> going to slow things down.
>
> I'd like to see measurements to prove me wrong but this both seems like
> a bad idea (optimizing by being incorrect, this gives us a correct
> timeout check like other drivers do) and really unlikely I would think
> to be noticable. Since we'll be doing the same code-paths in both
> regular and SPL, trying to time things (by loading a big file) would be
> easy enough I think. Thanks!
Ping
thanks!
--
Jagan.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
As requested:
Tested-by: David Dueck
Am Freitag, 24. April 2015 schrieb Jagan Teki :
> On 7 April 2015 at 05:55, Tom Rini >
> wrote:
> > On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
> >> Hi David,
> >>
> >>
> >>
> >> > for (i = 0; i < len; i++) {
> >> > /* wait till TX register is empty (TXS == 1) */
> >> > + start = get_timer(0);
> >> > while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> >> > OMAP3_MCSPI_CHSTAT_TXS)) {
> >> > - if (--timeout <= 0) {
> >> > + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> >> > printf("SPI TXS timed out,
> status=0x%08x\n",
> >> >readl(&ds->regs->channel[ds-
> >> > >slave.cs].chstat));
> >> > return -1;
> >>
> >> I have a couple of questions...
> >>
> >> Firstly, when in SPL is there access to the get_timer() function?
> >
> > We call timer_init() from board_init_r() in SPL, prior to diving down
> > into loading (or checking for Falcon vs Regular) so this is safe.
> >
> >> Secondly, when using Falcon mode to load Linux directly from SPI (Falcon
> >> mode) then we want to maximise the throughput and save every CPU cycle
> we
> >> possibly can. Adding yet another function call into the for loop and
> hence
> >> calling it a couple of million times seems, on the face of it, like it
> is
> >> going to slow things down.
> >
> > I'd like to see measurements to prove me wrong but this both seems like
> > a bad idea (optimizing by being incorrect, this gives us a correct
> > timeout check like other drivers do) and really unlikely I would think
> > to be noticable. Since we'll be doing the same code-paths in both
> > regular and SPL, trying to time things (by loading a big file) would be
> > easy enough I think. Thanks!
>
> Ping
>
> thanks!
> --
> Jagan.
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
> Hi David,
>
>
>
> > for (i = 0; i < len; i++) {
> > /* wait till TX register is empty (TXS == 1) */
> > + start = get_timer(0);
> > while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> > OMAP3_MCSPI_CHSTAT_TXS)) {
> > - if (--timeout <= 0) {
> > + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> > printf("SPI TXS timed out, status=0x%08x\n",
> >readl(&ds->regs->channel[ds-
> > >slave.cs].chstat));
> > return -1;
>
> I have a couple of questions...
>
> Firstly, when in SPL is there access to the get_timer() function?
We call timer_init() from board_init_r() in SPL, prior to diving down
into loading (or checking for Falcon vs Regular) so this is safe.
> Secondly, when using Falcon mode to load Linux directly from SPI (Falcon
> mode) then we want to maximise the throughput and save every CPU cycle we
> possibly can. Adding yet another function call into the for loop and hence
> calling it a couple of million times seems, on the face of it, like it is
> going to slow things down.
I'd like to see measurements to prove me wrong but this both seems like
a bad idea (optimizing by being incorrect, this gives us a correct
timeout check like other drivers do) and really unlikely I would think
to be noticable. Since we'll be doing the same code-paths in both
regular and SPL, trying to time things (by loading a big file) would be
easy enough I think. Thanks!
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
Hi David,
> for (i = 0; i < len; i++) {
> /* wait till TX register is empty (TXS == 1) */
> + start = get_timer(0);
> while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
>OMAP3_MCSPI_CHSTAT_TXS)) {
> - if (--timeout <= 0) {
> + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> printf("SPI TXS timed out, status=0x%08x\n",
> readl(&ds->regs->channel[ds-
> >slave.cs].chstat));
> return -1;
I have a couple of questions...
Firstly, when in SPL is there access to the get_timer() function?
Secondly, when using Falcon mode to load Linux directly from SPI (Falcon
mode) then we want to maximise the throughput and save every CPU cycle we
possibly can. Adding yet another function call into the for loop and hence
calling it a couple of million times seems, on the face of it, like it is
going to slow things down.
Regards,
Andy.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
On 1 April 2015 at 17:50, David Dueck wrote:
> The timeout value is never reset during the transfer. This means that when
> transferring more data we eventually trigger the timeout.
>
> This was reported on the mailing list:
> "Spansion SPI flash read timeout with AM335x"
>
> Signed-off-by: David Dueck
> CC: Tom Rini
> CC: Jagannadh Teki
> CC: Stefan Roese
> CC: Andy Pont
> ---
> Changes since v1:
> - fix style issue
> - fix CC line
>
> drivers/spi/omap3_spi.c | 20
> 1 file changed, 12 insertions(+), 8 deletions(-)
Any Tested-by?
>
> diff --git a/drivers/spi/omap3_spi.c b/drivers/spi/omap3_spi.c
> index 651e46e..85f9e85 100644
> --- a/drivers/spi/omap3_spi.c
> +++ b/drivers/spi/omap3_spi.c
> @@ -20,7 +20,7 @@
> #include
> #include "omap3_spi.h"
>
> -#define SPI_WAIT_TIMEOUT 300
> +#define SPI_WAIT_TIMEOUT 10
>
> static void spi_reset(struct omap3_spi_slave *ds)
> {
> @@ -227,7 +227,7 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int
> len, const void *txp,
> {
> struct omap3_spi_slave *ds = to_omap3_spi(slave);
> int i;
> - int timeout = SPI_WAIT_TIMEOUT;
> + ulong start;
> int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
>
> /* Enable the channel */
> @@ -241,9 +241,10 @@ int omap3_spi_write(struct spi_slave *slave, unsigned
> int len, const void *txp,
>
> for (i = 0; i < len; i++) {
> /* wait till TX register is empty (TXS == 1) */
> + start = get_timer(0);
> while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> OMAP3_MCSPI_CHSTAT_TXS)) {
> - if (--timeout <= 0) {
> + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> printf("SPI TXS timed out, status=0x%08x\n",
>
> readl(&ds->regs->channel[ds->slave.cs].chstat));
> return -1;
> @@ -280,7 +281,7 @@ int omap3_spi_read(struct spi_slave *slave, unsigned int
> len, void *rxp,
> {
> struct omap3_spi_slave *ds = to_omap3_spi(slave);
> int i;
> - int timeout = SPI_WAIT_TIMEOUT;
> + ulong start;
> int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
>
> /* Enable the channel */
> @@ -295,10 +296,11 @@ int omap3_spi_read(struct spi_slave *slave, unsigned
> int len, void *rxp,
> writel(0, &ds->regs->channel[ds->slave.cs].tx);
>
> for (i = 0; i < len; i++) {
> + start = get_timer(0);
> /* Wait till RX register contains data (RXS == 1) */
> while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> OMAP3_MCSPI_CHSTAT_RXS)) {
> - if (--timeout <= 0) {
> + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> printf("SPI RXS timed out, status=0x%08x\n",
>
> readl(&ds->regs->channel[ds->slave.cs].chstat));
> return -1;
> @@ -332,7 +334,7 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
> len,
>const void *txp, void *rxp, unsigned long flags)
> {
> struct omap3_spi_slave *ds = to_omap3_spi(slave);
> - int timeout = SPI_WAIT_TIMEOUT;
> + ulong start;
> int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
> int irqstatus = readl(&ds->regs->irqstatus);
> int i=0;
> @@ -350,9 +352,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
> len,
> for (i=0; i < len; i++){
> /* Write: wait for TX empty (TXS == 1)*/
> irqstatus |= (1<< (4*(ds->slave.bus)));
> + start = get_timer(0);
> while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> OMAP3_MCSPI_CHSTAT_TXS)) {
> - if (--timeout <= 0) {
> + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> printf("SPI TXS timed out, status=0x%08x\n",
>
> readl(&ds->regs->channel[ds->slave.cs].chstat));
> return -1;
> @@ -368,9 +371,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
> len,
> writel(((u8 *)txp)[i], tx);
>
> /*Read: wait for RX containing data (RXS == 1)*/
> + start = get_timer(0);
> while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
> OMAP3_MCSPI_CHSTAT_RXS)) {
> - if (--timeout <= 0) {
> + if (get_timer(start) > SPI_WAIT_TIMEOUT) {
> printf("SPI RXS timed out, status=0x%08x\n",
>
> readl(&ds->regs->channel[ds->slave.cs].chstat));
>
[U-Boot] [PATCH v2] spi: omap3: Fix timeout handling
The timeout value is never reset during the transfer. This means that when
transferring more data we eventually trigger the timeout.
This was reported on the mailing list:
"Spansion SPI flash read timeout with AM335x"
Signed-off-by: David Dueck
CC: Tom Rini
CC: Jagannadh Teki
CC: Stefan Roese
CC: Andy Pont
---
Changes since v1:
- fix style issue
- fix CC line
drivers/spi/omap3_spi.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/omap3_spi.c b/drivers/spi/omap3_spi.c
index 651e46e..85f9e85 100644
--- a/drivers/spi/omap3_spi.c
+++ b/drivers/spi/omap3_spi.c
@@ -20,7 +20,7 @@
#include
#include "omap3_spi.h"
-#define SPI_WAIT_TIMEOUT 300
+#define SPI_WAIT_TIMEOUT 10
static void spi_reset(struct omap3_spi_slave *ds)
{
@@ -227,7 +227,7 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int
len, const void *txp,
{
struct omap3_spi_slave *ds = to_omap3_spi(slave);
int i;
- int timeout = SPI_WAIT_TIMEOUT;
+ ulong start;
int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
/* Enable the channel */
@@ -241,9 +241,10 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int
len, const void *txp,
for (i = 0; i < len; i++) {
/* wait till TX register is empty (TXS == 1) */
+ start = get_timer(0);
while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
OMAP3_MCSPI_CHSTAT_TXS)) {
- if (--timeout <= 0) {
+ if (get_timer(start) > SPI_WAIT_TIMEOUT) {
printf("SPI TXS timed out, status=0x%08x\n",
readl(&ds->regs->channel[ds->slave.cs].chstat));
return -1;
@@ -280,7 +281,7 @@ int omap3_spi_read(struct spi_slave *slave, unsigned int
len, void *rxp,
{
struct omap3_spi_slave *ds = to_omap3_spi(slave);
int i;
- int timeout = SPI_WAIT_TIMEOUT;
+ ulong start;
int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
/* Enable the channel */
@@ -295,10 +296,11 @@ int omap3_spi_read(struct spi_slave *slave, unsigned int
len, void *rxp,
writel(0, &ds->regs->channel[ds->slave.cs].tx);
for (i = 0; i < len; i++) {
+ start = get_timer(0);
/* Wait till RX register contains data (RXS == 1) */
while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
OMAP3_MCSPI_CHSTAT_RXS)) {
- if (--timeout <= 0) {
+ if (get_timer(start) > SPI_WAIT_TIMEOUT) {
printf("SPI RXS timed out, status=0x%08x\n",
readl(&ds->regs->channel[ds->slave.cs].chstat));
return -1;
@@ -332,7 +334,7 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
len,
const void *txp, void *rxp, unsigned long flags)
{
struct omap3_spi_slave *ds = to_omap3_spi(slave);
- int timeout = SPI_WAIT_TIMEOUT;
+ ulong start;
int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
int irqstatus = readl(&ds->regs->irqstatus);
int i=0;
@@ -350,9 +352,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
len,
for (i=0; i < len; i++){
/* Write: wait for TX empty (TXS == 1)*/
irqstatus |= (1<< (4*(ds->slave.bus)));
+ start = get_timer(0);
while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
OMAP3_MCSPI_CHSTAT_TXS)) {
- if (--timeout <= 0) {
+ if (get_timer(start) > SPI_WAIT_TIMEOUT) {
printf("SPI TXS timed out, status=0x%08x\n",
readl(&ds->regs->channel[ds->slave.cs].chstat));
return -1;
@@ -368,9 +371,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int
len,
writel(((u8 *)txp)[i], tx);
/*Read: wait for RX containing data (RXS == 1)*/
+ start = get_timer(0);
while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
OMAP3_MCSPI_CHSTAT_RXS)) {
- if (--timeout <= 0) {
+ if (get_timer(start) > SPI_WAIT_TIMEOUT) {
printf("SPI RXS timed out, status=0x%08x\n",
readl(&ds->regs->channel[ds->slave.cs].chstat));
return -1;
--
2.3.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
