[PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
chipselect toggling. However, this is largely inefficient, since SPI
client drivers are allowed to keep the chipselect active for long periods
of time. This patch allows the driver to runtime-suspend the controller
between SPI transfers. While moving runtime-PM hooks this patch also 
removes manual PM clock manipulation.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Marked as RFC, because I'm actually not sure, whether suspending after 
each transfer is actually allowed. This seems to bring problems with 
mmc-spi at least. Some commands return errors, but a retry recovers. More 
investigation is required. Any hints concerning why this is not perfectly 
functioning would be appreciated!

 drivers/spi/spi-sh-msiof.c |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 1f466bc..ece38c2 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -397,7 +397,6 @@ static int sh_msiof_spi_setup_transfer(struct spi_device *spi,
 
 static void sh_msiof_spi_chipselect(struct spi_device *spi, int is_on)
 {
-	struct sh_msiof_spi_priv *p = spi_master_get_devdata(spi->master);
 	int value;
 
 	/* chip select is active low unless SPI_CS_HIGH is set */
@@ -406,28 +405,8 @@ static void sh_msiof_spi_chipselect(struct spi_device *spi, int is_on)
 	else
 		value = (is_on = BITBANG_CS_ACTIVE) ? 0 : 1;
 
-	if (is_on = BITBANG_CS_ACTIVE) {
-		if (!test_and_set_bit(0, &p->flags)) {
-			pm_runtime_get_sync(&p->pdev->dev);
-			clk_enable(p->clk);
-		}
-
-		/* Configure pins before asserting CS */
-		sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
-					  !!(spi->mode & SPI_CPHA),
-					  !!(spi->mode & SPI_3WIRE),
-					  !!(spi->mode & SPI_LSB_FIRST));
-	}
-
 	/* use spi->controller data for CS (same strategy as spi_gpio) */
 	gpio_set_value((unsigned)spi->controller_data, value);
-
-	if (is_on = BITBANG_CS_INACTIVE) {
-		if (test_and_clear_bit(0, &p->flags)) {
-			clk_disable(p->clk);
-			pm_runtime_put(&p->pdev->dev);
-		}
-	}
 }
 
 static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
@@ -560,6 +539,14 @@ static int sh_msiof_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
 			rx_fifo = sh_msiof_spi_read_fifo_32;
 	}
 
+	pm_runtime_get_sync(&p->pdev->dev);
+
+	/* Configure pins before asserting CS */
+	sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
+				  !!(spi->mode & SPI_CPHA),
+				  !!(spi->mode & SPI_3WIRE),
+				  !!(spi->mode & SPI_LSB_FIRST));
+
 	/* setup clocks (clock already enabled in chipselect()) */
 	sh_msiof_spi_set_clk_regs(p, clk_get_rate(p->clk),
 				  sh_msiof_spi_hz(spi, t));
@@ -582,6 +569,7 @@ static int sh_msiof_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
 		words -= n;
 	}
 
+	pm_runtime_put(&p->pdev->dev);
 	return bytes_done;
 }
 
-- 
1.7.2.5

[PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
chipselect toggling. However, this is largely inefficient, since SPI
client drivers are allowed to keep the chipselect active for long periods
of time. This patch allows the driver to runtime-suspend the controller
between SPI transfers. While moving runtime-PM hooks this patch also 
removes manual PM clock manipulation.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Marked as RFC, because I'm actually not sure, whether suspending after 
each transfer is actually allowed. This seems to bring problems with 
mmc-spi at least. Some commands return errors, but a retry recovers. More 
investigation is required. Any hints concerning why this is not perfectly 
functioning would be appreciated!

 drivers/spi/spi-sh-msiof.c |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 1f466bc..ece38c2 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -397,7 +397,6 @@ static int sh_msiof_spi_setup_transfer(struct spi_device *spi,
 
 static void sh_msiof_spi_chipselect(struct spi_device *spi, int is_on)
 {
-	struct sh_msiof_spi_priv *p = spi_master_get_devdata(spi->master);
 	int value;
 
 	/* chip select is active low unless SPI_CS_HIGH is set */
@@ -406,28 +405,8 @@ static void sh_msiof_spi_chipselect(struct spi_device *spi, int is_on)
 	else
 		value = (is_on == BITBANG_CS_ACTIVE) ? 0 : 1;
 
-	if (is_on == BITBANG_CS_ACTIVE) {
-		if (!test_and_set_bit(0, &p->flags)) {
-			pm_runtime_get_sync(&p->pdev->dev);
-			clk_enable(p->clk);
-		}
-
-		/* Configure pins before asserting CS */
-		sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
-					  !!(spi->mode & SPI_CPHA),
-					  !!(spi->mode & SPI_3WIRE),
-					  !!(spi->mode & SPI_LSB_FIRST));
-	}
-
 	/* use spi->controller data for CS (same strategy as spi_gpio) */
 	gpio_set_value((unsigned)spi->controller_data, value);
-
-	if (is_on == BITBANG_CS_INACTIVE) {
-		if (test_and_clear_bit(0, &p->flags)) {
-			clk_disable(p->clk);
-			pm_runtime_put(&p->pdev->dev);
-		}
-	}
 }
 
 static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
@@ -560,6 +539,14 @@ static int sh_msiof_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
 			rx_fifo = sh_msiof_spi_read_fifo_32;
 	}
 
+	pm_runtime_get_sync(&p->pdev->dev);
+
+	/* Configure pins before asserting CS */
+	sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
+				  !!(spi->mode & SPI_CPHA),
+				  !!(spi->mode & SPI_3WIRE),
+				  !!(spi->mode & SPI_LSB_FIRST));
+
 	/* setup clocks (clock already enabled in chipselect()) */
 	sh_msiof_spi_set_clk_regs(p, clk_get_rate(p->clk),
 				  sh_msiof_spi_hz(spi, t));
@@ -582,6 +569,7 @@ static int sh_msiof_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
 		words -= n;
 	}
 
+	pm_runtime_put(&p->pdev->dev);
 	return bytes_done;
 }
 
-- 
1.7.2.5

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Magnus Damm]

On Thu, Mar 15, 2012 at 2:35 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> chipselect toggling. However, this is largely inefficient, since SPI
> client drivers are allowed to keep the chipselect active for long periods
> of time. This patch allows the driver to runtime-suspend the controller
> between SPI transfers. While moving runtime-PM hooks this patch also
> removes manual PM clock manipulation.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Marked as RFC, because I'm actually not sure, whether suspending after
> each transfer is actually allowed. This seems to bring problems with
> mmc-spi at least. Some commands return errors, but a retry recovers. More
> investigation is required. Any hints concerning why this is not perfectly
> functioning would be appreciated!

Hi Guennadi,

Thanks for your efforts. I believe that the error you are seeing with
this patch applied is because the code has been updated to change the
polarity of some shared SPI bus pins while CS is being asserted. This
will result in potential spikes on the shared SPI bus pins which is
far from perfect. =)

Cheers,

/ magnus

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Magnus Damm]

On Thu, Mar 15, 2012 at 2:35 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> chipselect toggling. However, this is largely inefficient, since SPI
> client drivers are allowed to keep the chipselect active for long periods
> of time. This patch allows the driver to runtime-suspend the controller
> between SPI transfers. While moving runtime-PM hooks this patch also
> removes manual PM clock manipulation.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Marked as RFC, because I'm actually not sure, whether suspending after
> each transfer is actually allowed. This seems to bring problems with
> mmc-spi at least. Some commands return errors, but a retry recovers. More
> investigation is required. Any hints concerning why this is not perfectly
> functioning would be appreciated!

Hi Guennadi,

Thanks for your efforts. I believe that the error you are seeing with
this patch applied is because the code has been updated to change the
polarity of some shared SPI bus pins while CS is being asserted. This
will result in potential spikes on the shared SPI bus pins which is
far from perfect. =)

Cheers,

/ magnus

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Hi Magnus

On Thu, 15 Mar 2012, Magnus Damm wrote:

> On Thu, Mar 15, 2012 at 2:35 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> > chipselect toggling. However, this is largely inefficient, since SPI
> > client drivers are allowed to keep the chipselect active for long periods
> > of time. This patch allows the driver to runtime-suspend the controller
> > between SPI transfers. While moving runtime-PM hooks this patch also
> > removes manual PM clock manipulation.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > Marked as RFC, because I'm actually not sure, whether suspending after
> > each transfer is actually allowed. This seems to bring problems with
> > mmc-spi at least. Some commands return errors, but a retry recovers. More
> > investigation is required. Any hints concerning why this is not perfectly
> > functioning would be appreciated!
> 
> Hi Guennadi,
> 
> Thanks for your efforts. I believe that the error you are seeing with
> this patch applied is because the code has been updated to change the
> polarity of some shared SPI bus pins while CS is being asserted. This
> will result in potential spikes on the shared SPI bus pins which is
> far from perfect. =)

You're probably right:-) I forgot to mention, that the problem goes away 
if PM QoS is used, but we certainly cannot rely on it. As we further 
discussed in a chat, the problem can be relatively easily solved by 
postponing the CS activation until in the first transfer, but this only 
helps as long as the runtime PM doesn't power the domain down. Whereas, as 
discussed in my reply to Grant in this thread, if this happens, a 
different problem occurs, which anyway makes this approach much more 
problematic, that originally thought.

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

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Hi Magnus

On Thu, 15 Mar 2012, Magnus Damm wrote:

> On Thu, Mar 15, 2012 at 2:35 AM, Guennadi Liakhovetski
> <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> > Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> > chipselect toggling. However, this is largely inefficient, since SPI
> > client drivers are allowed to keep the chipselect active for long periods
> > of time. This patch allows the driver to runtime-suspend the controller
> > between SPI transfers. While moving runtime-PM hooks this patch also
> > removes manual PM clock manipulation.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> > ---
> >
> > Marked as RFC, because I'm actually not sure, whether suspending after
> > each transfer is actually allowed. This seems to bring problems with
> > mmc-spi at least. Some commands return errors, but a retry recovers. More
> > investigation is required. Any hints concerning why this is not perfectly
> > functioning would be appreciated!
> 
> Hi Guennadi,
> 
> Thanks for your efforts. I believe that the error you are seeing with
> this patch applied is because the code has been updated to change the
> polarity of some shared SPI bus pins while CS is being asserted. This
> will result in potential spikes on the shared SPI bus pins which is
> far from perfect. =)

You're probably right:-) I forgot to mention, that the problem goes away 
if PM QoS is used, but we certainly cannot rely on it. As we further 
discussed in a chat, the problem can be relatively easily solved by 
postponing the CS activation until in the first transfer, but this only 
helps as long as the runtime PM doesn't power the domain down. Whereas, as 
discussed in my reply to Grant in this thread, if this happens, a 
different problem occurs, which anyway makes this approach much more 
problematic, that originally thought.

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

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

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Grant Likely]

On Wed, 14 Mar 2012 18:35:58 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> chipselect toggling. However, this is largely inefficient, since SPI
> client drivers are allowed to keep the chipselect active for long periods
> of time. This patch allows the driver to runtime-suspend the controller
> between SPI transfers. While moving runtime-PM hooks this patch also 
> removes manual PM clock manipulation.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Marked as RFC, because I'm actually not sure, whether suspending after 
> each transfer is actually allowed. This seems to bring problems with 
> mmc-spi at least. Some commands return errors, but a retry recovers. More 
> investigation is required. Any hints concerning why this is not perfectly 
> functioning would be appreciated!

Suspending in the txrx function is fine since each bitbang instance gets its own
workqueue.  It's only the transfer() hook (which bitbang supplies for you) that
cannot sleep.

g.

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Grant Likely]

On Wed, 14 Mar 2012 18:35:58 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> chipselect toggling. However, this is largely inefficient, since SPI
> client drivers are allowed to keep the chipselect active for long periods
> of time. This patch allows the driver to runtime-suspend the controller
> between SPI transfers. While moving runtime-PM hooks this patch also 
> removes manual PM clock manipulation.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Marked as RFC, because I'm actually not sure, whether suspending after 
> each transfer is actually allowed. This seems to bring problems with 
> mmc-spi at least. Some commands return errors, but a retry recovers. More 
> investigation is required. Any hints concerning why this is not perfectly 
> functioning would be appreciated!

Suspending in the txrx function is fine since each bitbang instance gets its own
workqueue.  It's only the transfer() hook (which bitbang supplies for you) that
cannot sleep.

g.

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Hi Grant

On Thu, 15 Mar 2012, Grant Likely wrote:

> On Wed, 14 Mar 2012 18:35:58 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> > chipselect toggling. However, this is largely inefficient, since SPI
> > client drivers are allowed to keep the chipselect active for long periods
> > of time. This patch allows the driver to runtime-suspend the controller
> > between SPI transfers. While moving runtime-PM hooks this patch also 
> > removes manual PM clock manipulation.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Marked as RFC, because I'm actually not sure, whether suspending after 
> > each transfer is actually allowed. This seems to bring problems with 
> > mmc-spi at least. Some commands return errors, but a retry recovers. More 
> > investigation is required. Any hints concerning why this is not perfectly 
> > functioning would be appreciated!
> 
> Suspending in the txrx function is fine since each bitbang instance gets its own
> workqueue.  It's only the transfer() hook (which bitbang supplies for you) that
> cannot sleep.

Thanks for the reply, but I think, the problem is different. The problem 
is, that the hardware can be suspended or even powered down during that 
suspend. On some systems, where these controllers are used, there are at 
least 2 possibilities to save power: (1) turn off the clock, (2) power 
down the controller completely as a part of a power domain. The former is 
ok - the pins retain their static configuration (hope, Magnus or Rafael 
will correct me if I confuse something), and the bus clock shall be turned 
off anyway. But if the controller is really powered down, its pins will 
lose their configuration, which might produce spurious edges on the SPI 
bus with an active chip-select. This is certainly not allowed. So, we 
would either have to only allow the "light" form of power-saving, or to 
try to reconfigure pins as GPIOs for the suspend time to preserve their 
levels, but this can be difficult... So, looks like we'll have to leave 
this idea for now.

What would help is a finer-grained chip-select control. The above shows, 
that suspending the hardware during an active CS is difficult, so, it 
would be good to, maybe, have an option to really minimise CS-active 
intervals. But that goes down to individual SPI client drivers...
 
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC] spi: sh-msiof: per-transaction runtime PM

[Guennadi Liakhovetski]

Hi Grant

On Thu, 15 Mar 2012, Grant Likely wrote:

> On Wed, 14 Mar 2012 18:35:58 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Currently the spi-sh-msiof driver implements a runtime PM scheme, based on
> > chipselect toggling. However, this is largely inefficient, since SPI
> > client drivers are allowed to keep the chipselect active for long periods
> > of time. This patch allows the driver to runtime-suspend the controller
> > between SPI transfers. While moving runtime-PM hooks this patch also 
> > removes manual PM clock manipulation.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Marked as RFC, because I'm actually not sure, whether suspending after 
> > each transfer is actually allowed. This seems to bring problems with 
> > mmc-spi at least. Some commands return errors, but a retry recovers. More 
> > investigation is required. Any hints concerning why this is not perfectly 
> > functioning would be appreciated!
> 
> Suspending in the txrx function is fine since each bitbang instance gets its own
> workqueue.  It's only the transfer() hook (which bitbang supplies for you) that
> cannot sleep.

Thanks for the reply, but I think, the problem is different. The problem 
is, that the hardware can be suspended or even powered down during that 
suspend. On some systems, where these controllers are used, there are at 
least 2 possibilities to save power: (1) turn off the clock, (2) power 
down the controller completely as a part of a power domain. The former is 
ok - the pins retain their static configuration (hope, Magnus or Rafael 
will correct me if I confuse something), and the bus clock shall be turned 
off anyway. But if the controller is really powered down, its pins will 
lose their configuration, which might produce spurious edges on the SPI 
bus with an active chip-select. This is certainly not allowed. So, we 
would either have to only allow the "light" form of power-saving, or to 
try to reconfigure pins as GPIOs for the suspend time to preserve their 
levels, but this can be difficult... So, looks like we'll have to leave 
this idea for now.

What would help is a finer-grained chip-select control. The above shows, 
that suspending the hardware during an active CS is difficult, so, it 
would be good to, maybe, have an option to really minimise CS-active 
intervals. But that goes down to individual SPI client drivers...
 
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/