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,
<snipped for brevity>
for (i = 0; i < len; i++) { /* wait till TX register is empty (TXS == 1) */
start = get_timer(0);
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!
On 7 April 2015 at 05:55, Tom Rini trini@konsulko.com wrote:
On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
Hi David,
<snipped for brevity>
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!
As requested: Tested-by: David Dueck davidcdueck@googlemail.com
Am Freitag, 24. April 2015 schrieb Jagan Teki :
On 7 April 2015 at 05:55, Tom Rini <trini@konsulko.com javascript:;> wrote:
On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
Hi David,
<snipped for brevity>
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.
On 24 April 2015 at 17:04, D. Dueck davidcdueck@googlemail.com wrote:
As requested: Tested-by: David Dueck davidcdueck@googlemail.com
Am Freitag, 24. April 2015 schrieb Jagan Teki :
On 7 April 2015 at 05:55, Tom Rini trini@konsulko.com wrote:
On Wed, Apr 01, 2015 at 04:21:50PM +0100, Andy Pont wrote:
Hi David,
<snipped for brevity>
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!
participants (3)
-
D. Dueck -
Jagan Teki -
Tom Rini