[PATCH 1/4] spi: Add option to wake a device by toggling CS

[PATCH 1/4] spi: Add option to wake a device by toggling CS

[apronin]

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;
+}
+
+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;
 
-- 
2.6.6

[PATCH 1/4] spi: Add option to wake a device by toggling CS

[apronin-F7+t8E8rja9g9hUCZPvPmw]

From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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;
+}
+
+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;
 
-- 
2.6.6

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] spi: Document option to wake a device by toggling CS

[apronin]

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>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 42d5954..1b7ffd4 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -63,6 +63,13 @@ contain the following properties.
                       used for MISO. Defaults to 1 if not present.
 - spi-rx-delay-us  - (optional) Microsecond delay after a read transfer.
 - spi-tx-delay-us  - (optional) Microsecond delay after a write transfer.
+- cs-wake-after-sleep - (optional) Device may go to sleep after a period
+		of SPI inactivity. If this flag is set, toggle CS and
+		wait for it to wake before communicating to it.
+- cs-sleep-delay  - (optional) Delay after which the device may go to
+		sleep if there was no SPI activity (msec).
+- cs-wake-duration - (optional) Time it takes the device to wake up after
+		toggling CS if it went to sleep (msec).
 
 Some SPI controllers and devices support Dual and Quad SPI transfer mode.
 It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
-- 
2.6.6

[PATCH 3/4] spi: Add option to insert delay between transactions

[apronin]

From: Andrey Pronin <apronin@chromium.org>

Some devices may need CS to be deasserted for some time
between transactions. Added a new capability to guarantee
a delay between SPI transactions for the device.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/spi/spi.c       | 3 +++
 include/linux/spi/spi.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index c51c864..639c7bd 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -971,6 +971,9 @@ static int spi_transfer_one_message(struct spi_master *master,
 		spi_reset_cs_wake_timer(msg->spi);
 	}
 
+	if (msg->spi->xfer_delay)
+		mdelay(msg->spi->xfer_delay);
+
 	spi_set_cs(msg->spi, true);
 
 	SPI_STATISTICS_INCREMENT_FIELD(statm, messages);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4b06ba6..4b1aa13 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -137,6 +137,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
  * @cs_wake_timer: Timer measuring the delay before the device goes to
  *	sleep after the last SPI transaction.
  *
+ * @xfer_delay: Delay between SPI transactions (msec).
+ *
  * @statistics: statistics for the spi_device
  *
  * A @spi_device is used to interchange data between an SPI slave
@@ -183,6 +185,8 @@ struct spi_device {
 	bool			cs_wake_needed;
 	struct timer_list	cs_wake_timer;
 
+	u32			xfer_delay;
+
 	/* the statistics */
 	struct spi_statistics	statistics;
 
-- 
2.6.6

[PATCH 4/4] spi: Document option to insert delay between transactions

[apronin]

From: Andrey Pronin <apronin@chromium.org>

Some devices may need CS to be deasserted for some time
between transactions. Added a new capability to guarantee
a delay between SPI transactions for the device.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 1b7ffd4..87c117a 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -70,6 +70,8 @@ contain the following properties.
 		sleep if there was no SPI activity (msec).
 - cs-wake-duration - (optional) Time it takes the device to wake up after
 		toggling CS if it went to sleep (msec).
+- xfer-delay      - (optional) Delay to insert between SPI transactions
+		to guarantee that CS is deasserted at least for some time.
 
 Some SPI controllers and devices support Dual and Quad SPI transfer mode.
 It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
-- 
2.6.6

Re: [PATCH 2/4] spi: Document option to wake a device by toggling CS

[Geert Uytterhoeven]

CC devicetree

On Thu, Jun 30, 2016 at 5:54 AM,  <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>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
> index 42d5954..1b7ffd4 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -63,6 +63,13 @@ contain the following properties.
>                        used for MISO. Defaults to 1 if not present.
>  - spi-rx-delay-us  - (optional) Microsecond delay after a read transfer.
>  - spi-tx-delay-us  - (optional) Microsecond delay after a write transfer.
> +- cs-wake-after-sleep - (optional) Device may go to sleep after a period
> +               of SPI inactivity. If this flag is set, toggle CS and
> +               wait for it to wake before communicating to it.
> +- cs-sleep-delay  - (optional) Delay after which the device may go to
> +               sleep if there was no SPI activity (msec).
> +- cs-wake-duration - (optional) Time it takes the device to wake up after
> +               toggling CS if it went to sleep (msec).
>
>  Some SPI controllers and devices support Dual and Quad SPI transfer mode.
>  It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
> --
> 2.6.6

Re: [PATCH 4/4] spi: Document option to insert delay between transactions

[Geert Uytterhoeven]

CC devicetree

On Thu, Jun 30, 2016 at 5:54 AM,  <apronin@chromium.org> wrote:
> From: Andrey Pronin <apronin@chromium.org>
>
> Some devices may need CS to be deasserted for some time
> between transactions. Added a new capability to guarantee
> a delay between SPI transactions for the device.
>
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
> index 1b7ffd4..87c117a 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -70,6 +70,8 @@ contain the following properties.
>                 sleep if there was no SPI activity (msec).
>  - cs-wake-duration - (optional) Time it takes the device to wake up after
>                 toggling CS if it went to sleep (msec).
> +- xfer-delay      - (optional) Delay to insert between SPI transactions
> +               to guarantee that CS is deasserted at least for some time.
>
>  Some SPI controllers and devices support Dual and Quad SPI transfer mode.
>  It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
> --
> 2.6.6

Re: [PATCH 4/4] spi: Document option to insert delay between transactions

[Geert Uytterhoeven]

CC devicetree

On Thu, Jun 30, 2016 at 5:54 AM,  <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> Some devices may need CS to be deasserted for some time
> between transactions. Added a new capability to guarantee
> a delay between SPI transactions for the device.
>
> Signed-off-by: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
> index 1b7ffd4..87c117a 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -70,6 +70,8 @@ contain the following properties.
>                 sleep if there was no SPI activity (msec).
>  - cs-wake-duration - (optional) Time it takes the device to wake up after
>                 toggling CS if it went to sleep (msec).
> +- xfer-delay      - (optional) Delay to insert between SPI transactions
> +               to guarantee that CS is deasserted at least for some time.
>
>  Some SPI controllers and devices support Dual and Quad SPI transfer mode.
>  It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
> --
> 2.6.6
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Doug Anderson]

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
>

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Doug Anderson]

Andrey,

On Wed, Jun 29, 2016 at 8:54 PM,  <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] spi: Document option to wake a device by toggling CS

[Doug Anderson]

Hi,

On Thu, Jun 30, 2016 at 12:12 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> CC devicetree

And Rob and Mark...

 ./scripts/get_maintainer.pl -f
Documentation/devicetree/bindings/spi/spi-bus.txt

Mark Brown <broonie@kernel.org> (maintainer:SPI SUBSYSTEM)
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)
Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)
linux-spi@vger.kernel.org (open list:SPI SUBSYSTEM)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED
DEVICE TREE BINDINGS)
linux-kernel@vger.kernel.org (open list)


> On Thu, Jun 30, 2016 at 5:54 AM,  <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>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)

Overall note is that devicetree bindings patches should be _before_
the code that uses them, so you should swap patch #1 and patch #2.

>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> index 42d5954..1b7ffd4 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -63,6 +63,13 @@ contain the following properties.
>>                        used for MISO. Defaults to 1 if not present.
>>  - spi-rx-delay-us  - (optional) Microsecond delay after a read transfer.
>>  - spi-tx-delay-us  - (optional) Microsecond delay after a write transfer.
>> +- cs-wake-after-sleep - (optional) Device may go to sleep after a period
>> +               of SPI inactivity. If this flag is set, toggle CS and
>> +               wait for it to wake before communicating to it.

You probably also need a property for how long it needs to be asserted
when you "toggle" it?

>> +- cs-sleep-delay  - (optional) Delay after which the device may go to
>> +               sleep if there was no SPI activity (msec).
>> +- cs-wake-duration - (optional) Time it takes the device to wake up after
>> +               toggling CS if it went to sleep (msec).

I believe that from your code the second two properties are both
required if the first property is set.  That should be in the
bindings.

Re: [PATCH 2/4] spi: Document option to wake a device by toggling CS

[Doug Anderson]

Hi,

On Thu, Jun 30, 2016 at 12:12 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> CC devicetree

And Rob and Mark...

 ./scripts/get_maintainer.pl -f
Documentation/devicetree/bindings/spi/spi-bus.txt

Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> (maintainer:SPI SUBSYSTEM)
Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)
Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:SPI SUBSYSTEM)
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED
DEVICE TREE BINDINGS)
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)


> On Thu, Jun 30, 2016 at 5:54 AM,  <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)

Overall note is that devicetree bindings patches should be _before_
the code that uses them, so you should swap patch #1 and patch #2.

>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> index 42d5954..1b7ffd4 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -63,6 +63,13 @@ contain the following properties.
>>                        used for MISO. Defaults to 1 if not present.
>>  - spi-rx-delay-us  - (optional) Microsecond delay after a read transfer.
>>  - spi-tx-delay-us  - (optional) Microsecond delay after a write transfer.
>> +- cs-wake-after-sleep - (optional) Device may go to sleep after a period
>> +               of SPI inactivity. If this flag is set, toggle CS and
>> +               wait for it to wake before communicating to it.

You probably also need a property for how long it needs to be asserted
when you "toggle" it?

>> +- cs-sleep-delay  - (optional) Delay after which the device may go to
>> +               sleep if there was no SPI activity (msec).
>> +- cs-wake-duration - (optional) Time it takes the device to wake up after
>> +               toggling CS if it went to sleep (msec).

I believe that from your code the second two properties are both
required if the first property is set.  That should be in the
bindings.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] spi: Add option to insert delay between transactions

[Doug Anderson]

Hi,

On Wed, Jun 29, 2016 at 8:54 PM,  <apronin@chromium.org> wrote:
> From: Andrey Pronin <apronin@chromium.org>
>
> Some devices may need CS to be deasserted for some time
> between transactions. Added a new capability to guarantee
> a delay between SPI transactions for the device.
>
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> ---
>  drivers/spi/spi.c       | 3 +++
>  include/linux/spi/spi.h | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index c51c864..639c7bd 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -971,6 +971,9 @@ static int spi_transfer_one_message(struct spi_master *master,
>                 spi_reset_cs_wake_timer(msg->spi);
>         }
>
> +       if (msg->spi->xfer_delay)
> +               mdelay(msg->spi->xfer_delay);
> +

Sounds like you're generalizing "EC_SPI_RECOVERY_TIME_NS" from the
cros EC code.  That code actually keeps track of the end of the last
transfer and only delays as much as needed.

Also note that time is in NS, which seems like a more appropriate time
scale.  Even if the hardware you're talking to needs many milliseconds
to recover, there might be some that need only a few ns.


>         spi_set_cs(msg->spi, true);
>
>         SPI_STATISTICS_INCREMENT_FIELD(statm, messages);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4b06ba6..4b1aa13 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -137,6 +137,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
>   * @cs_wake_timer: Timer measuring the delay before the device goes to
>   *     sleep after the last SPI transaction.
>   *
> + * @xfer_delay: Delay between SPI transactions (msec).
> + *
>   * @statistics: statistics for the spi_device
>   *
>   * A @spi_device is used to interchange data between an SPI slave
> @@ -183,6 +185,8 @@ struct spi_device {
>         bool                    cs_wake_needed;
>         struct timer_list       cs_wake_timer;
>
> +       u32                     xfer_delay;
> +
>         /* the statistics */
>         struct spi_statistics   statistics;

As mentioned in the other patch, some of your code is missing and is
in patch #1.

-Doug

Re: [PATCH 3/4] spi: Add option to insert delay between transactions

[Doug Anderson]

Hi,

On Wed, Jun 29, 2016 at 8:54 PM,  <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> Some devices may need CS to be deasserted for some time
> between transactions. Added a new capability to guarantee
> a delay between SPI transactions for the device.
>
> Signed-off-by: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/spi/spi.c       | 3 +++
>  include/linux/spi/spi.h | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index c51c864..639c7bd 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -971,6 +971,9 @@ static int spi_transfer_one_message(struct spi_master *master,
>                 spi_reset_cs_wake_timer(msg->spi);
>         }
>
> +       if (msg->spi->xfer_delay)
> +               mdelay(msg->spi->xfer_delay);
> +

Sounds like you're generalizing "EC_SPI_RECOVERY_TIME_NS" from the
cros EC code.  That code actually keeps track of the end of the last
transfer and only delays as much as needed.

Also note that time is in NS, which seems like a more appropriate time
scale.  Even if the hardware you're talking to needs many milliseconds
to recover, there might be some that need only a few ns.


>         spi_set_cs(msg->spi, true);
>
>         SPI_STATISTICS_INCREMENT_FIELD(statm, messages);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4b06ba6..4b1aa13 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -137,6 +137,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
>   * @cs_wake_timer: Timer measuring the delay before the device goes to
>   *     sleep after the last SPI transaction.
>   *
> + * @xfer_delay: Delay between SPI transactions (msec).
> + *
>   * @statistics: statistics for the spi_device
>   *
>   * A @spi_device is used to interchange data between an SPI slave
> @@ -183,6 +185,8 @@ struct spi_device {
>         bool                    cs_wake_needed;
>         struct timer_list       cs_wake_timer;
>
> +       u32                     xfer_delay;
> +
>         /* the statistics */
>         struct spi_statistics   statistics;

As mentioned in the other patch, some of your code is missing and is
in patch #1.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: Document option to insert delay between transactions

[Doug Anderson]

Hi,

On Thu, Jun 30, 2016 at 12:12 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> CC devicetree
>
> On Thu, Jun 30, 2016 at 5:54 AM,  <apronin@chromium.org> wrote:
>> From: Andrey Pronin <apronin@chromium.org>
>>
>> Some devices may need CS to be deasserted for some time
>> between transactions. Added a new capability to guarantee
>> a delay between SPI transactions for the device.
>>
>> Signed-off-by: Andrey Pronin <apronin@chromium.org>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> index 1b7ffd4..87c117a 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -70,6 +70,8 @@ contain the following properties.
>>                 sleep if there was no SPI activity (msec).
>>  - cs-wake-duration - (optional) Time it takes the device to wake up after
>>                 toggling CS if it went to sleep (msec).
>> +- xfer-delay      - (optional) Delay to insert between SPI transactions
>> +               to guarantee that CS is deasserted at least for some time.

msec?  usec?  nsec?

Also, as with the other patch, this should be _before_ the code
change, so this should really be patch 3 not patch 4.

Re: [PATCH 4/4] spi: Document option to insert delay between transactions

[Doug Anderson]

Hi,

On Thu, Jun 30, 2016 at 12:12 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> CC devicetree
>
> On Thu, Jun 30, 2016 at 5:54 AM,  <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>
>> Some devices may need CS to be deasserted for some time
>> between transactions. Added a new capability to guarantee
>> a delay between SPI transactions for the device.
>>
>> Signed-off-by: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> index 1b7ffd4..87c117a 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -70,6 +70,8 @@ contain the following properties.
>>                 sleep if there was no SPI activity (msec).
>>  - cs-wake-duration - (optional) Time it takes the device to wake up after
>>                 toggling CS if it went to sleep (msec).
>> +- xfer-delay      - (optional) Delay to insert between SPI transactions
>> +               to guarantee that CS is deasserted at least for some time.

msec?  usec?  nsec?

Also, as with the other patch, this should be _before_ the code
change, so this should really be patch 3 not patch 4.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Wed, Jun 29, 2016 at 08:54:24PM -0700, apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org wrote:
> From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/4] spi: Add option to insert delay between transactions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Wed, Jun 29, 2016 at 08:54:26PM -0700, apronin@chromium.org wrote:

> Some devices may need CS to be deasserted for some time
> between transactions. Added a new capability to guarantee
> a delay between SPI transactions for the device.

This seems like even more of a per device thing - it's a very rare
requirement (I'm guessing for some SPI controller coprocessor) and
there's such a wide range of potential patterns that might be needed
by different devices.  

> +       if (msg->spi->xfer_delay)
> +               mdelay(msg->spi->xfer_delay);
> +
>         spi_set_cs(msg->spi, true);

This isn't a delay between messages, it's a delay before asserting
chip select which will happen every single time we do anything
regardless of if there was any activity immediately before or not.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Doug Anderson]

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

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Doug Anderson]

Hi,

On Fri, Jul 1, 2016 at 1:21 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Andrey Pronin]

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.

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Andrey Pronin]

On Fri, Jul 01, 2016 at 09:02:30AM +0100, Mark Brown wrote:
> On Wed, Jun 29, 2016 at 08:54:24PM -0700, apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org wrote:
> > From: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: Add option to wake a device by toggling CS

[Andrey Pronin]

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.