| Message ID | 1467258867-117727-1-git-send-email-apronin@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Andrey, On Wed, Jun 29, 2016 at 8:54 PM, <apronin@chromium.org> wrote: > From: Andrey Pronin <apronin@chromium.org> > > Some SPI devices may go to sleep after a period of inactivity > on SPI. For such devices, if enough time has passed since the > last SPI transaction, toggle CS and wait for the device to > start before communicating with it. > > Signed-off-by: Andrey Pronin <apronin@chromium.org> > --- > drivers/spi/spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 17 +++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 77e6e45..c51c864 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -700,6 +700,20 @@ static void spi_set_cs(struct spi_device *spi, bool enable) > spi->master->set_cs(spi, !enable); > } > > +static void spi_cs_wake_timer_func(unsigned long arg) > +{ > + struct spi_device *spi = (struct spi_device *)arg; > + > + spi->cs_wake_needed = true; Don't you need some type of lock? If nothing else you might run into weakly ordered memory issues and so the write you doing here might not take effect right away. Seems like you need _something_ other than just a simple assignment here, but maybe I'm mixed up. Also, something doesn't seem terribly robust about this, buy maybe I'm being paranoid. If something happens where the timer hasn't fired quickly enough then you might not know that you need to assert the wakeup, right? I don't think there's a 100% guarantee that a timer will fire and finish running within a certain period of time, is there? > +} > + > +static void spi_reset_cs_wake_timer(struct spi_device *spi) > +{ > + spi->cs_wake_needed = false; > + mod_timer(&spi->cs_wake_timer, > + jiffies + spi->cs_sleep_jiffies); Seems like a race here, right? If you set: spi->cs_wake_needed = false; ...and then the timer fires, it will be set to "true" and then you'll call your mod_timer(). That seems bad, right? Also: is it valid for spi->cs_sleep_jiffies to be 0 (if the device might go to sleep in < 1 jiffy of activity?). Does that work OK? > +} > + > #ifdef CONFIG_HAS_DMA > static int spi_map_buf(struct spi_master *master, struct device *dev, > struct sg_table *sgt, void *buf, size_t len, > @@ -948,6 +962,15 @@ static int spi_transfer_one_message(struct spi_master *master, > struct spi_statistics *statm = &master->statistics; > struct spi_statistics *stats = &msg->spi->statistics; > > + if (msg->spi->cs_wake_after_sleep && msg->spi->cs_wake_needed) { Do you need to think about keep_cs here? Said another way: what happens if you haven't communicated for a while but the last communication left the cs asserted? Seems like in that case you wouldn't need to wake the device up, right? > + dev_info(&msg->spi->dev, "waking after possible sleep\n"); dev_info seems awfully spammy. deb_dbg()? > + spi_set_cs(msg->spi, true); > + mdelay(1); Why 1 millisecond? > + spi_set_cs(msg->spi, false); > + msleep(msg->spi->cs_wake_duration); > + spi_reset_cs_wake_timer(msg->spi); > + } > + > spi_set_cs(msg->spi, true); > > SPI_STATISTICS_INCREMENT_FIELD(statm, messages); > @@ -1024,6 +1047,9 @@ out: > if (ret != 0 || !keep_cs) > spi_set_cs(msg->spi, false); > > + if (msg->spi->cs_wake_after_sleep && !ret) > + spi_reset_cs_wake_timer(msg->spi); > + > if (msg->status == -EINPROGRESS) > msg->status = ret; > > @@ -1551,6 +1577,45 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc) > } > spi->max_speed_hz = value; > > + /* Do we need to assert CS to wake the device after sleep */ > + spi->cs_wake_after_sleep = > + of_property_read_bool(nc, "cs-wake-after-sleep"); > + if (spi->cs_wake_after_sleep) { > + dev_info(&master->dev, "cs-wake-after-sleep enabled\n"); Again, probably too spammy. dev_dbg() > + > + /* After what delay device goes to sleep */ > + rc = of_property_read_u32(nc, "cs-sleep-delay", &value); > + if (rc) { > + dev_err(&master->dev, > + "%s has no valid 'cs-sleep-delay' property (%d)\n", > + nc->full_name, rc); > + goto err_out; > + } > + spi->cs_sleep_jiffies = value * HZ / 1000; /* jiffies */ Explain why you're not using msecs_to_jiffies() here? Presumably because you want to round down? BTW: why do you need the "jiffies" comment? > + > + /* How long to wait after waking */ > + rc = of_property_read_u32(nc, "cs-wake-duration", &value); > + if (rc) { > + dev_err(&master->dev, > + "%s has no valid 'cs-wake-duration' property (%d)\n", > + nc->full_name, rc); > + goto err_out; > + } > + spi->cs_wake_duration = value; /* msec */ Why not name it "cs_wake_ms" and get rid of the "msec" comment? > + /* Wake before accessing for the 1st time */ > + spi->cs_wake_needed = true; > + init_timer(&spi->cs_wake_timer); > + spi->cs_wake_timer.data = (unsigned long)spi; > + spi->cs_wake_timer.function = spi_cs_wake_timer_func; > + } > + > + /* Should there be a delay before each transfer */ > + spi->xfer_delay = 0; > + of_property_read_u32(nc, "xfer-delay", &spi->xfer_delay); > + if (spi->xfer_delay) > + dev_info(&master->dev, "xfer-delay = %u\n", spi->xfer_delay); > + Isn't xfer_delay part of another patch in this series? > /* Store a pointer to the node in the device structure */ > of_node_get(nc); > spi->dev.of_node = nc; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 1f03483..4b06ba6 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -126,6 +126,17 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats, > * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when > * when not using a GPIO line) > * > + * @cs_wake_after_sleep: Briefly toggle CS before talking to a device > + * if it could go to sleep. > + * @cs_sleep_jiffies: Delay after which a device may go to sleep if there > + * was no SPI activity for it (jiffies). > + * @cs_wake_duration: Delay after waking the device by toggling CS before > + * it is ready (msec). > + * @cs_wake_needed: Is the wake needed (cs_sleep_jiffies passed since > + * the last SPI transaction). > + * @cs_wake_timer: Timer measuring the delay before the device goes to > + * sleep after the last SPI transaction. > + * > * @statistics: statistics for the spi_device > * > * A @spi_device is used to interchange data between an SPI slave > @@ -166,6 +177,12 @@ struct spi_device { > char modalias[SPI_NAME_SIZE]; > int cs_gpio; /* chip select gpio */ > > + bool cs_wake_after_sleep; > + unsigned long cs_sleep_jiffies; > + unsigned long cs_wake_duration; > + bool cs_wake_needed; > + struct timer_list cs_wake_timer; Tiny nit: group bools together and get a slightly more optimal structure packing? > + > /* the statistics */ > struct spi_statistics statistics; > > -- > 2.6.6 > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 29, 2016 at 08:54:24PM -0700, apronin@chromium.org wrote: > From: Andrey Pronin <apronin@chromium.org> > > Some SPI devices may go to sleep after a period of inactivity > on SPI. For such devices, if enough time has passed since the > last SPI transaction, toggle CS and wait for the device to > start before communicating with it. This seems incredibly specialist, I can imagine someone might implement something like this but it really feels like something I'd expect the driver for the device to be doing rather than the core. Is it really a bounce that's needed and not something like putting a small delay after asserting chip select?
On Thu, Jun 30, 2016 at 09:23:26PM -0700, Doug Anderson wrote: > Also, something doesn't seem terribly robust about this, buy maybe I'm > being paranoid. If something happens where the timer hasn't fired > quickly enough then you might not know that you need to assert the > wakeup, right? I don't think there's a 100% guarantee that a timer > will fire and finish running within a certain period of time, is > there? No, or at least a minimum bound on accuracy - you'd need to set the timer for something lower than the actual limit.
Hi, On Fri, Jul 1, 2016 at 1:21 AM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 30, 2016 at 09:23:26PM -0700, Doug Anderson wrote: > >> Also, something doesn't seem terribly robust about this, buy maybe I'm >> being paranoid. If something happens where the timer hasn't fired >> quickly enough then you might not know that you need to assert the >> wakeup, right? I don't think there's a 100% guarantee that a timer >> will fire and finish running within a certain period of time, is >> there? > > No, or at least a minimum bound on accuracy - you'd need to set the > timer for something lower than the actual limit. I'm curious why you you need a timer at all. Can't you just keep track of the jiffies that you last sent and do subtraction? ...or you could get even more accurate and use a ktime_t. That avoids a whole lot of synchronization / locking issues too... Also: presumably you'll need to make sure that there's some margin in this whole thing. I'd imagine that if the timeout is 10000 nanoseconds and you do the calculation and you last sent 9999 nanoseconds ago then you might decide that the other side isn't asleep yet. ...but by the time the transfer starts it might be asleep... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 01, 2016 at 10:05:50AM -0700, Doug Anderson wrote: > I'm curious why you you need a timer at all. Can't you just keep > track of the jiffies that you last sent and do subtraction? ...or you > could get even more accurate and use a ktime_t. That avoids a whole > lot of synchronization / locking issues too... Yeah, that'd be a lot better. > Also: presumably you'll need to make sure that there's some margin in > this whole thing. I'd imagine that if the timeout is 10000 > nanoseconds and you do the calculation and you last sent 9999 > nanoseconds ago then you might decide that the other side isn't asleep > yet. ...but by the time the transfer starts it might be asleep... Indeed.
On Fri, Jul 01, 2016 at 09:02:30AM +0100, Mark Brown wrote: > On Wed, Jun 29, 2016 at 08:54:24PM -0700, apronin@chromium.org wrote: > > From: Andrey Pronin <apronin@chromium.org> > > > > Some SPI devices may go to sleep after a period of inactivity > > on SPI. For such devices, if enough time has passed since the > > last SPI transaction, toggle CS and wait for the device to > > start before communicating with it. > > This seems incredibly specialist, I can imagine someone might implement > something like this but it really feels like something I'd expect the > driver for the device to be doing rather than the core. Is it really a > bounce that's needed and not something like putting a small delay after > asserting chip select? Yes, I agree it's quite device-specific, though, I guess, more than a single device can benefit from that. I'm playing with it now to see if I can indeed move it to a more device-specific driver from generic spi. It is indeed a bounce and a rather long delay to let the device actually wake from a long sleep, go through internal startup, test, etc. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 01, 2016 at 07:17:08PM +0200, Mark Brown wrote: > On Fri, Jul 01, 2016 at 10:05:50AM -0700, Doug Anderson wrote: > > > I'm curious why you you need a timer at all. Can't you just keep > > track of the jiffies that you last sent and do subtraction? ...or you > > could get even more accurate and use a ktime_t. That avoids a whole > > lot of synchronization / locking issues too... > > Yeah, that'd be a lot better. > > > Also: presumably you'll need to make sure that there's some margin in > > this whole thing. I'd imagine that if the timeout is 10000 > > nanoseconds and you do the calculation and you last sent 9999 > > nanoseconds ago then you might decide that the other side isn't asleep > > yet. ...but by the time the transfer starts it might be asleep... > > Indeed. Lots of godd points in the feedback. Let me re-visit the whole idea. And may be I'll move it to a more device-specific driver. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 77e6e45..c51c864 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -700,6 +700,20 @@ static void spi_set_cs(struct spi_device *spi, bool enable) spi->master->set_cs(spi, !enable); } +static void spi_cs_wake_timer_func(unsigned long arg) +{ + struct spi_device *spi = (struct spi_device *)arg; + + spi->cs_wake_needed = true; +} + +static void spi_reset_cs_wake_timer(struct spi_device *spi) +{ + spi->cs_wake_needed = false; + mod_timer(&spi->cs_wake_timer, + jiffies + spi->cs_sleep_jiffies); +} + #ifdef CONFIG_HAS_DMA static int spi_map_buf(struct spi_master *master, struct device *dev, struct sg_table *sgt, void *buf, size_t len, @@ -948,6 +962,15 @@ static int spi_transfer_one_message(struct spi_master *master, struct spi_statistics *statm = &master->statistics; struct spi_statistics *stats = &msg->spi->statistics; + if (msg->spi->cs_wake_after_sleep && msg->spi->cs_wake_needed) { + dev_info(&msg->spi->dev, "waking after possible sleep\n"); + spi_set_cs(msg->spi, true); + mdelay(1); + spi_set_cs(msg->spi, false); + msleep(msg->spi->cs_wake_duration); + spi_reset_cs_wake_timer(msg->spi); + } + spi_set_cs(msg->spi, true); SPI_STATISTICS_INCREMENT_FIELD(statm, messages); @@ -1024,6 +1047,9 @@ out: if (ret != 0 || !keep_cs) spi_set_cs(msg->spi, false); + if (msg->spi->cs_wake_after_sleep && !ret) + spi_reset_cs_wake_timer(msg->spi); + if (msg->status == -EINPROGRESS) msg->status = ret; @@ -1551,6 +1577,45 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc) } spi->max_speed_hz = value; + /* Do we need to assert CS to wake the device after sleep */ + spi->cs_wake_after_sleep = + of_property_read_bool(nc, "cs-wake-after-sleep"); + if (spi->cs_wake_after_sleep) { + dev_info(&master->dev, "cs-wake-after-sleep enabled\n"); + + /* After what delay device goes to sleep */ + rc = of_property_read_u32(nc, "cs-sleep-delay", &value); + if (rc) { + dev_err(&master->dev, + "%s has no valid 'cs-sleep-delay' property (%d)\n", + nc->full_name, rc); + goto err_out; + } + spi->cs_sleep_jiffies = value * HZ / 1000; /* jiffies */ + + /* How long to wait after waking */ + rc = of_property_read_u32(nc, "cs-wake-duration", &value); + if (rc) { + dev_err(&master->dev, + "%s has no valid 'cs-wake-duration' property (%d)\n", + nc->full_name, rc); + goto err_out; + } + spi->cs_wake_duration = value; /* msec */ + + /* Wake before accessing for the 1st time */ + spi->cs_wake_needed = true; + init_timer(&spi->cs_wake_timer); + spi->cs_wake_timer.data = (unsigned long)spi; + spi->cs_wake_timer.function = spi_cs_wake_timer_func; + } + + /* Should there be a delay before each transfer */ + spi->xfer_delay = 0; + of_property_read_u32(nc, "xfer-delay", &spi->xfer_delay); + if (spi->xfer_delay) + dev_info(&master->dev, "xfer-delay = %u\n", spi->xfer_delay); + /* Store a pointer to the node in the device structure */ of_node_get(nc); spi->dev.of_node = nc; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 1f03483..4b06ba6 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -126,6 +126,17 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats, * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when * when not using a GPIO line) * + * @cs_wake_after_sleep: Briefly toggle CS before talking to a device + * if it could go to sleep. + * @cs_sleep_jiffies: Delay after which a device may go to sleep if there + * was no SPI activity for it (jiffies). + * @cs_wake_duration: Delay after waking the device by toggling CS before + * it is ready (msec). + * @cs_wake_needed: Is the wake needed (cs_sleep_jiffies passed since + * the last SPI transaction). + * @cs_wake_timer: Timer measuring the delay before the device goes to + * sleep after the last SPI transaction. + * * @statistics: statistics for the spi_device * * A @spi_device is used to interchange data between an SPI slave @@ -166,6 +177,12 @@ struct spi_device { char modalias[SPI_NAME_SIZE]; int cs_gpio; /* chip select gpio */ + bool cs_wake_after_sleep; + unsigned long cs_sleep_jiffies; + unsigned long cs_wake_duration; + bool cs_wake_needed; + struct timer_list cs_wake_timer; + /* the statistics */ struct spi_statistics statistics;