| Message ID | 1305821134-26147-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 19, 2011 at 06:05:34PM +0200, Linus Walleij wrote: > From: Magnus Templing <magnus.templing@stericsson.com> > > This adds the missing handling of polling timeouts and deletes > our last todo. > > Signed-off-by: Magnus Templing <magnus.templing@stericsson.com> > Reviewed-by: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> > [Fixups from review by Wolfram Sang] > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Applied, thanks. g. > --- > drivers/spi/amba-pl022.c | 23 +++++++++++++++-------- > 1 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c > index 08de58e..18667de 100644 > --- a/drivers/spi/amba-pl022.c > +++ b/drivers/spi/amba-pl022.c > @@ -24,11 +24,6 @@ > * GNU General Public License for more details. > */ > > -/* > - * TODO: > - * - add timeout on polled transfers > - */ > - > #include <linux/init.h> > #include <linux/module.h> > #include <linux/device.h> > @@ -287,6 +282,8 @@ > > #define CLEAR_ALL_INTERRUPTS 0x3 > > +#define SPI_POLLING_TIMEOUT 1000 > + > > /* > * The type of reading going on on this chip > @@ -1378,6 +1375,7 @@ static void do_polling_transfer(struct pl022 *pl022) > struct spi_transfer *transfer = NULL; > struct spi_transfer *previous = NULL; > struct chip_data *chip; > + unsigned long time, timeout; > > chip = pl022->cur_chip; > message = pl022->cur_msg; > @@ -1415,9 +1413,18 @@ static void do_polling_transfer(struct pl022 *pl022) > SSP_CR1(pl022->virtbase)); > > dev_dbg(&pl022->adev->dev, "polling transfer ongoing ...\n"); > - /* FIXME: insert a timeout so we don't hang here indefinitely */ > - while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) > + > + timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); > + while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { > + time = jiffies; > readwriter(pl022); > + if (time_after(time, timeout)) { > + dev_warn(&pl022->adev->dev, > + "%s: timeout!\n", __func__); > + message->state = STATE_ERROR; > + goto out; > + } > + } > > /* Update total byte transferred */ > message->actual_length += pl022->cur_transfer->len; > @@ -1426,7 +1433,7 @@ static void do_polling_transfer(struct pl022 *pl022) > /* Move to next transfer */ > message->state = next_transfer(pl022); > } > - > +out: > /* Handle end of message */ > if (message->state == STATE_DONE) > message->status = 0; > -- > 1.7.3.2 >
Hi Linus, On Thu, May 19, 2011 at 6:05 PM, Linus Walleij <linus.walleij@stericsson.com> wrote: > From: Magnus Templing <magnus.templing@stericsson.com> > > This adds the missing handling of polling timeouts and deletes > our last todo. > > Signed-off-by: Magnus Templing <magnus.templing@stericsson.com> > Reviewed-by: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> > [Fixups from review by Wolfram Sang] > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/spi/amba-pl022.c | 23 +++++++++++++++-------- > 1 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c > index 08de58e..18667de 100644 > --- a/drivers/spi/amba-pl022.c > +++ b/drivers/spi/amba-pl022.c > @@ -24,11 +24,6 @@ > * GNU General Public License for more details. > */ > > -/* > - * TODO: > - * - add timeout on polled transfers > - */ > - > #include <linux/init.h> > #include <linux/module.h> > #include <linux/device.h> > @@ -287,6 +282,8 @@ > > #define CLEAR_ALL_INTERRUPTS 0x3 > > +#define SPI_POLLING_TIMEOUT 1000 > + > > /* > * The type of reading going on on this chip > @@ -1378,6 +1375,7 @@ static void do_polling_transfer(struct pl022 *pl022) > struct spi_transfer *transfer = NULL; > struct spi_transfer *previous = NULL; > struct chip_data *chip; > + unsigned long time, timeout; > > chip = pl022->cur_chip; > message = pl022->cur_msg; > @@ -1415,9 +1413,18 @@ static void do_polling_transfer(struct pl022 *pl022) > SSP_CR1(pl022->virtbase)); > > dev_dbg(&pl022->adev->dev, "polling transfer ongoing ...\n"); > - /* FIXME: insert a timeout so we don't hang here indefinitely */ > - while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) > + > + timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); > + while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { > + time = jiffies; > readwriter(pl022); > + if (time_after(time, timeout)) { > + dev_warn(&pl022->adev->dev, > + "%s: timeout!\n", __func__); > + message->state = STATE_ERROR; > + goto out; > + } > + } just out of curiosity: is it a busy wait? Looks like it is... Thanks, Vitaly
2011/5/19 Vitaly Wool <vitalywool@gmail.com>: >> + >> + timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); >> + while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { >> + time = jiffies; >> readwriter(pl022); >> + if (time_after(time, timeout)) { >> + dev_warn(&pl022->adev->dev, >> + "%s: timeout!\n", __func__); >> + message->state = STATE_ERROR; >> + goto out; >> + } >> + } > > just out of curiosity: is it a busy wait? Looks like it is... Yep that's the polling mode part. IRQ and DMA mode should be the norm I guess. Magnus: do you have a specific use case for this thing? Yours, Linus Walleij
On Thu, May 19, 2011 at 7:36 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> just out of curiosity: is it a busy wait? Looks like it is... > > Yep that's the polling mode part. IRQ and DMA mode should > be the norm I guess. > > Magnus: do you have a specific use case for this thing? Well, adding cpu_relax() somewhere in the loop might be not a bad idea... Thanks, Vitaly
2011/5/19 Vitaly Wool <vitalywool@gmail.com>: > On Thu, May 19, 2011 at 7:36 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> >>> just out of curiosity: is it a busy wait? Looks like it is... >> >> Yep that's the polling mode part. IRQ and DMA mode should >> be the norm I guess. >> >> Magnus: do you have a specific use case for this thing? > > Well, adding cpu_relax() somewhere in the loop might be not a bad idea... OK sent a patch for it, and Grant applied it within something like 30 seconds :-) I think on ARM that just boils down to a barrier() so it basically injects air in the pipeline when busywaiting, I never understood what that is good for, but it's a good marker anyway. Linus Walleij
diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c index 08de58e..18667de 100644 --- a/drivers/spi/amba-pl022.c +++ b/drivers/spi/amba-pl022.c @@ -24,11 +24,6 @@ * GNU General Public License for more details. */ -/* - * TODO: - * - add timeout on polled transfers - */ - #include <linux/init.h> #include <linux/module.h> #include <linux/device.h> @@ -287,6 +282,8 @@ #define CLEAR_ALL_INTERRUPTS 0x3 +#define SPI_POLLING_TIMEOUT 1000 + /* * The type of reading going on on this chip @@ -1378,6 +1375,7 @@ static void do_polling_transfer(struct pl022 *pl022) struct spi_transfer *transfer = NULL; struct spi_transfer *previous = NULL; struct chip_data *chip; + unsigned long time, timeout; chip = pl022->cur_chip; message = pl022->cur_msg; @@ -1415,9 +1413,18 @@ static void do_polling_transfer(struct pl022 *pl022) SSP_CR1(pl022->virtbase)); dev_dbg(&pl022->adev->dev, "polling transfer ongoing ...\n"); - /* FIXME: insert a timeout so we don't hang here indefinitely */ - while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) + + timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); + while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { + time = jiffies; readwriter(pl022); + if (time_after(time, timeout)) { + dev_warn(&pl022->adev->dev, + "%s: timeout!\n", __func__); + message->state = STATE_ERROR; + goto out; + } + } /* Update total byte transferred */ message->actual_length += pl022->cur_transfer->len; @@ -1426,7 +1433,7 @@ static void do_polling_transfer(struct pl022 *pl022) /* Move to next transfer */ message->state = next_transfer(pl022); } - +out: /* Handle end of message */ if (message->state == STATE_DONE) message->status = 0;