[PATCH 1/2] spi: add an SPI-device flag for systems, preferring power-efficiency

[PATCH 1/2] spi: add an SPI-device flag for systems, preferring power-efficiency

[Guennadi Liakhovetski]

Currently the usual SPI driver behaviour is to keep chip select lines
active between messages, unless explicitly required by client drivers to
release it at the end of an SPI message. This is good for performance,
but not very good for power-efficiency, because it is generally not very
easy to suspend SPI-controllers, while a chip-select is active. If during
such a suspend the controller loses power, its IO lines may change
polarity, which can confuse activated clients. This patch adds a new
.flags field to struct spi_board_info and to struct spi_device and a
flag to let users specify in their SPI board-info, that they prefer to
deactivate this device's chip-select line between messages. Drivers can
be gradually extended to support this feature.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi.c       |    1 +
 include/linux/spi/spi.h |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index bce1a55..f82fa79 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -433,6 +433,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
 	proxy->irq = chip->irq;
+	proxy->flags = chip->flags;
 	strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
 	proxy->dev.platform_data = (void *) chip->platform_data;
 	proxy->controller_data = chip->controller_data;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 176fce9..70cfa7f 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -29,6 +29,12 @@
  */
 extern struct bus_type spi_bus_type;
 
+/*
+ * Release Chip Select after each message to preserve power rather than
+ * optimising for performance by keeping it active.
+ */
+#define SPI_BOARD_FLAG_RELEASE_CS	(1 << 0)
+
 /**
  * struct spi_device - Master side proxy for an SPI slave device
  * @dev: Driver model representation of the device.
@@ -50,6 +56,7 @@ extern struct bus_type spi_bus_type;
  *	The spi_transfer.bits_per_word can override this for each transfer.
  * @irq: Negative, or the number passed to request_irq() to receive
  *	interrupts from this device.
+ * @flags: A mask of SPI_BOARD_FLAG_* flags, copied from spi_board_info
  * @controller_state: Controller's runtime state
  * @controller_data: Board-specific definitions for controller, such as
  *	FIFO initialization parameters; from board_info.controller_data
@@ -86,6 +93,7 @@ struct spi_device {
 #define	SPI_READY	0x80			/* slave pulls low to pause */
 	u8			bits_per_word;
 	int			irq;
+	unsigned long		flags;
 	void			*controller_state;
 	void			*controller_data;
 	char			modalias[SPI_NAME_SIZE];
@@ -703,6 +711,7 @@ static inline ssize_t spi_w8r16(struct spi_device *spi, u8 cmd)
  * @controller_data: Initializes spi_device.controller_data; some
  *	controllers need hints about hardware setup, e.g. for DMA.
  * @irq: Initializes spi_device.irq; depends on how the board is wired.
+ * @flags: A mask of SPI_BOARD_FLAG_* flags
  * @max_speed_hz: Initializes spi_device.max_speed_hz; based on limits
  *	from the chip datasheet and board-specific signal quality issues.
  * @bus_num: Identifies which spi_master parents the spi_device; unused
@@ -732,12 +741,13 @@ struct spi_board_info {
 	 *
 	 * platform_data goes to spi_device.dev.platform_data,
 	 * controller_data goes to spi_device.controller_data,
-	 * irq is copied too
+	 * irq and flags are copied too
 	 */
 	char		modalias[SPI_NAME_SIZE];
 	const void	*platform_data;
 	void		*controller_data;
 	int		irq;
+	unsigned long	flags;
 
 	/* slower signaling on noisy or low voltage boards */
 	u32		max_speed_hz;
-- 
1.7.2.5


------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure

[PATCH 2/2] spi: bitbang: support the new SPI_BOARD_FLAG_RELEASE_CS flag

[Guennadi Liakhovetski]

By supporting the new SPI_BOARD_FLAG_RELEASE_CS flag we let underlying
hardware drivers implement more efficient runtime power-management.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bitbang.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index fe06e1b..c895d85 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -347,17 +347,14 @@ static void bitbang_work(struct work_struct *work)
 			if (t->delay_usecs)
 				udelay(t->delay_usecs);
 
-			if (!cs_change)
-				continue;
-			if (t->transfer_list.next == &m->transfers)
-				break;
-
-			/* sometimes a short mid-message deselect of the chip
-			 * may be needed to terminate a mode or command
-			 */
-			ndelay(nsecs);
-			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
-			ndelay(nsecs);
+			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
+				/* sometimes a short mid-message deselect of the chip
+				 * may be needed to terminate a mode or command
+				 */
+				ndelay(nsecs);
+				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
+				ndelay(nsecs);
+			}
 		}
 
 		m->status = status;
@@ -367,7 +364,8 @@ static void bitbang_work(struct work_struct *work)
 		 * cs_change has hinted that the next message will probably
 		 * be for this chip too.
 		 */
-		if (!(status == 0 && cs_change)) {
+		if (!(status == 0 && cs_change) ||
+		    spi->flags & SPI_BOARD_FLAG_RELEASE_CS) {
 			ndelay(nsecs);
 			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
 			ndelay(nsecs);
-- 
1.7.2.5


------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure

Re: [PATCH 2/2] spi: bitbang: support the new SPI_BOARD_FLAG_RELEASE_CS flag

[Grant Likely]

On Thu, 15 Mar 2012 22:32:14 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> By supporting the new SPI_BOARD_FLAG_RELEASE_CS flag we let underlying
> hardware drivers implement more efficient runtime power-management.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bitbang.c |   22 ++++++++++------------
>  1 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index fe06e1b..c895d85 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -347,17 +347,14 @@ static void bitbang_work(struct work_struct *work)
>  			if (t->delay_usecs)
>  				udelay(t->delay_usecs);
>  
> -			if (!cs_change)
> -				continue;
> -			if (t->transfer_list.next == &m->transfers)
> -				break;
> -
> -			/* sometimes a short mid-message deselect of the chip
> -			 * may be needed to terminate a mode or command
> -			 */
> -			ndelay(nsecs);
> -			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> -			ndelay(nsecs);
> +			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
> +				/* sometimes a short mid-message deselect of the chip
> +				 * may be needed to terminate a mode or command
> +				 */
> +				ndelay(nsecs);
> +				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> +				ndelay(nsecs);
> +			}
>  		}
>  
>  		m->status = status;
> @@ -367,7 +364,8 @@ static void bitbang_work(struct work_struct *work)
>  		 * cs_change has hinted that the next message will probably
>  		 * be for this chip too.
>  		 */
> -		if (!(status == 0 && cs_change)) {
> +		if (!(status == 0 && cs_change) ||
> +		    spi->flags & SPI_BOARD_FLAG_RELEASE_CS) {

That's rather heavy handed, and will probably break drivers that lock the spi
bus to guarantee concurrent messages (like the MMC layer).  I don't think you
can do it this way.

It might be okay to force CS disable if the spi bus hasn't been exclusively
locked.  I know the comment talks about hinting (possibly for performance reasonse),
but I'm more interested in correctness at this point.

g.

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure

Re: [PATCH 2/2] spi: bitbang: support the new SPI_BOARD_FLAG_RELEASE_CS flag

[Guennadi Liakhovetski]

Hi Grant

On Sat, 17 Mar 2012, Grant Likely wrote:

> On Thu, 15 Mar 2012 22:32:14 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> > By supporting the new SPI_BOARD_FLAG_RELEASE_CS flag we let underlying
> > hardware drivers implement more efficient runtime power-management.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> > ---
> >  drivers/spi/spi-bitbang.c |   22 ++++++++++------------
> >  1 files changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> > index fe06e1b..c895d85 100644
> > --- a/drivers/spi/spi-bitbang.c
> > +++ b/drivers/spi/spi-bitbang.c
> > @@ -347,17 +347,14 @@ static void bitbang_work(struct work_struct *work)
> >  			if (t->delay_usecs)
> >  				udelay(t->delay_usecs);
> >  
> > -			if (!cs_change)
> > -				continue;
> > -			if (t->transfer_list.next == &m->transfers)
> > -				break;
> > -
> > -			/* sometimes a short mid-message deselect of the chip
> > -			 * may be needed to terminate a mode or command
> > -			 */
> > -			ndelay(nsecs);
> > -			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> > -			ndelay(nsecs);
> > +			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
> > +				/* sometimes a short mid-message deselect of the chip
> > +				 * may be needed to terminate a mode or command
> > +				 */
> > +				ndelay(nsecs);
> > +				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> > +				ndelay(nsecs);
> > +			}
> >  		}
> >  
> >  		m->status = status;
> > @@ -367,7 +364,8 @@ static void bitbang_work(struct work_struct *work)
> >  		 * cs_change has hinted that the next message will probably
> >  		 * be for this chip too.
> >  		 */
> > -		if (!(status == 0 && cs_change)) {
> > +		if (!(status == 0 && cs_change) ||
> > +		    spi->flags & SPI_BOARD_FLAG_RELEASE_CS) {
> 
> That's rather heavy handed, and will probably break drivers that lock the spi
> bus to guarantee concurrent messages (like the MMC layer).  I don't think you
> can do it this way.
> 
> It might be okay to force CS disable if the spi bus hasn't been exclusively
> locked.  I know the comment talks about hinting (possibly for performance reasonse),
> but I'm more interested in correctness at this point.

I understand your concerns about breaking existing configurations, but let 
us not forget, that this behaviour would only be activated by a new 
platform flag. This means, no existing set up should be affected and any 
new users should know what they are doing...

But let's think about the bus-lock too. A comment in spi.c says:

 * This call should be used by drivers that require exclusive access to the
 * SPI bus.

"Exclusive access" also means, that the chip-select stays active, which 
also means, the bus shall behave sanely, right? In other words, it shall 
not be suspended. It is easy to add a check in the above patch for 
master->bus_lock_flag, but we also probably want to retry releasing the 
CS, when the bus is unlocked. So, we need to inform the bus-driver, that 
the bus has just been released. We could add a callback for that, but 
wouldn't it be better to use runtime PM? Calls to spi_bus_lock() / 
unlock() are balanced, so, we could just add 
pm_runtime_get_sync(&master->dev) and pm_runtime_put(&master->dev) to them 
respectively? Would this be acceptable?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2

Re: [PATCH 2/2] spi: bitbang: support the new SPI_BOARD_FLAG_RELEASE_CS flag

[Grant Likely]

On Fri, 13 Apr 2012 16:43:30 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> Hi Grant
> 
> On Sat, 17 Mar 2012, Grant Likely wrote:
> 
> > On Thu, 15 Mar 2012 22:32:14 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> > > By supporting the new SPI_BOARD_FLAG_RELEASE_CS flag we let underlying
> > > hardware drivers implement more efficient runtime power-management.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> > > ---
> > >  drivers/spi/spi-bitbang.c |   22 ++++++++++------------
> > >  1 files changed, 10 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> > > index fe06e1b..c895d85 100644
> > > --- a/drivers/spi/spi-bitbang.c
> > > +++ b/drivers/spi/spi-bitbang.c
> > > @@ -347,17 +347,14 @@ static void bitbang_work(struct work_struct *work)
> > >  			if (t->delay_usecs)
> > >  				udelay(t->delay_usecs);
> > >  
> > > -			if (!cs_change)
> > > -				continue;
> > > -			if (t->transfer_list.next == &m->transfers)
> > > -				break;
> > > -
> > > -			/* sometimes a short mid-message deselect of the chip
> > > -			 * may be needed to terminate a mode or command
> > > -			 */
> > > -			ndelay(nsecs);
> > > -			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> > > -			ndelay(nsecs);
> > > +			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
> > > +				/* sometimes a short mid-message deselect of the chip
> > > +				 * may be needed to terminate a mode or command
> > > +				 */
> > > +				ndelay(nsecs);
> > > +				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> > > +				ndelay(nsecs);
> > > +			}
> > >  		}
> > >  
> > >  		m->status = status;
> > > @@ -367,7 +364,8 @@ static void bitbang_work(struct work_struct *work)
> > >  		 * cs_change has hinted that the next message will probably
> > >  		 * be for this chip too.
> > >  		 */
> > > -		if (!(status == 0 && cs_change)) {
> > > +		if (!(status == 0 && cs_change) ||
> > > +		    spi->flags & SPI_BOARD_FLAG_RELEASE_CS) {
> > 
> > That's rather heavy handed, and will probably break drivers that lock the spi
> > bus to guarantee concurrent messages (like the MMC layer).  I don't think you
> > can do it this way.
> > 
> > It might be okay to force CS disable if the spi bus hasn't been exclusively
> > locked.  I know the comment talks about hinting (possibly for performance reasonse),
> > but I'm more interested in correctness at this point.
> 
> I understand your concerns about breaking existing configurations, but let 
> us not forget, that this behaviour would only be activated by a new 
> platform flag. This means, no existing set up should be affected and any 
> new users should know what they are doing...
> 
> But let's think about the bus-lock too. A comment in spi.c says:
> 
>  * This call should be used by drivers that require exclusive access to the
>  * SPI bus.
> 
> "Exclusive access" also means, that the chip-select stays active, which 
> also means, the bus shall behave sanely, right? In other words, it shall 
> not be suspended. It is easy to add a check in the above patch for 
> master->bus_lock_flag, but we also probably want to retry releasing the 
> CS, when the bus is unlocked. So, we need to inform the bus-driver, that 
> the bus has just been released. We could add a callback for that, but 
> wouldn't it be better to use runtime PM? Calls to spi_bus_lock() / 
> unlock() are balanced, so, we could just add 
> pm_runtime_get_sync(&master->dev) and pm_runtime_put(&master->dev) to them 
> respectively? Would this be acceptable?

Yes, adding the get/put in the lock/unlock functions does look right,
but I still don't like the logic for clearing the cs flag.  I'd rather
see the cs dropped only in the suspend path since the setting at the
end of a transfer is only a hint if the lock is not held.

g.


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/