linux 4.2-rc1 broken Nokia N900
diff mbox

Message ID 20150722020306.GA8379@earth
State New
Headers show

Commit Message

Sebastian Reichel July 22, 2015, 2:03 a.m. UTC
Hi,

On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> 
> This code has my head spinning.
> 
> I found that the errors do not occur when the driver is built into the kernel.
> 
> I also found that with the patch below the errors go away.
> 
> Not sure if it is acceptible but see if it fixes things on your side.
> 
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index cf8b91b..3164d13 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
>  	if (!spi->max_speed_hz)
>  		spi->max_speed_hz = spi->master->max_speed_hz;
>  
> -	spi_set_cs(spi, false);
> -
>  	if (spi->master->setup)
>  		status = spi->master->setup(spi);
>  
> +	spi_set_cs(spi, false);
> +
>  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",

mh. maybe a runtime PM issue?

 * external abort on non-linefetch: address cannot be accessed,
   since the module's clocks are disabled
 * built-in works, module not: built-in is probably a little bit
   faster (module must not be loaded from filesystem), so that
   the device has not yet been suspended
 * Before 4.2, omap2_mcspi_set_cs() was called in the setup
   routine, which acquired runtime PM
 * In 4.2, omap2_mcspi_set_cs() seems to be called without a
   prior pm_runtime_get_sync()
 * With your workaround, the device has not yet returned to
   suspend after the runtime PM acquisition in setup()

So I suggest trying the following (compile tested only) patch:

-- Sebastian

Comments

Michael Welling July 22, 2015, 5:13 a.m. UTC | #1
On Wed, Jul 22, 2015 at 04:03:07AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> > 
> > This code has my head spinning.
> > 
> > I found that the errors do not occur when the driver is built into the kernel.
> > 
> > I also found that with the patch below the errors go away.
> > 
> > Not sure if it is acceptible but see if it fixes things on your side.
> > 
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index cf8b91b..3164d13 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
> >  	if (!spi->max_speed_hz)
> >  		spi->max_speed_hz = spi->master->max_speed_hz;
> >  
> > -	spi_set_cs(spi, false);
> > -
> >  	if (spi->master->setup)
> >  		status = spi->master->setup(spi);
> >  
> > +	spi_set_cs(spi, false);
> > +
> >  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> >  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> >  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> 
> mh. maybe a runtime PM issue?

mh?

> 
>  * external abort on non-linefetch: address cannot be accessed,
>    since the module's clocks are disabled
>  * built-in works, module not: built-in is probably a little bit
>    faster (module must not be loaded from filesystem), so that
>    the device has not yet been suspended
>  * Before 4.2, omap2_mcspi_set_cs() was called in the setup
>    routine, which acquired runtime PM
>  * In 4.2, omap2_mcspi_set_cs() seems to be called without a
>    prior pm_runtime_get_sync()
>  * With your workaround, the device has not yet returned to
>    suspend after the runtime PM acquisition in setup()
> 
> So I suggest trying the following (compile tested only) patch:
>

It seems you are right.

With this patch the SPI drivers no longer cause data aborts.

I will wait for feedback from Pavel and Pali but it looks like
we have a winner.

> -- Sebastian
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 5867384..f7d9ffd 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
>  
>  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>  {
> +       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
>         u32 l;
>  
>         /* The controller handles the inverted chip selects
> @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>                 enable = !enable;
>  
>         if (spi->controller_state) {
> +               pm_runtime_get_sync(mcspi->dev);
> +

Should the return code should be checked here as with the other
instances of pm_runtime_get_sync?

>                 l = mcspi_cached_chconf0(spi);
>  
>                 if (enable)
> @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>                         l |= OMAP2_MCSPI_CHCONF_FORCE;
>  
>                 mcspi_write_chconf0(spi, l);
> +
> +               pm_runtime_mark_last_busy(mcspi->dev);
> +               pm_runtime_put_autosuspend(mcspi->dev);
>         }
>  }
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár July 22, 2015, 7:27 a.m. UTC | #2
On Wednesday 22 July 2015 00:13:34 Michael Welling wrote:
> On Wed, Jul 22, 2015 at 04:03:07AM +0200, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> > > 
> > > This code has my head spinning.
> > > 
> > > I found that the errors do not occur when the driver is built into the kernel.
> > > 
> > > I also found that with the patch below the errors go away.
> > > 
> > > Not sure if it is acceptible but see if it fixes things on your side.
> > > 
> > > 
> > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > > index cf8b91b..3164d13 100644
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
> > >  	if (!spi->max_speed_hz)
> > >  		spi->max_speed_hz = spi->master->max_speed_hz;
> > >  
> > > -	spi_set_cs(spi, false);
> > > -
> > >  	if (spi->master->setup)
> > >  		status = spi->master->setup(spi);
> > >  
> > > +	spi_set_cs(spi, false);
> > > +
> > >  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> > >  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> > >  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> > 
> > mh. maybe a runtime PM issue?
> 
> mh?
> 
> > 
> >  * external abort on non-linefetch: address cannot be accessed,
> >    since the module's clocks are disabled
> >  * built-in works, module not: built-in is probably a little bit
> >    faster (module must not be loaded from filesystem), so that
> >    the device has not yet been suspended
> >  * Before 4.2, omap2_mcspi_set_cs() was called in the setup
> >    routine, which acquired runtime PM
> >  * In 4.2, omap2_mcspi_set_cs() seems to be called without a
> >    prior pm_runtime_get_sync()
> >  * With your workaround, the device has not yet returned to
> >    suspend after the runtime PM acquisition in setup()
> > 
> > So I suggest trying the following (compile tested only) patch:
> >
> 
> It seems you are right.
> 
> With this patch the SPI drivers no longer cause data aborts.
> 
> I will wait for feedback from Pavel and Pali but it looks like
> we have a winner.
> 

I'm busy right now for testing something on my N900...

> > -- Sebastian
> > 
> > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> > index 5867384..f7d9ffd 100644
> > --- a/drivers/spi/spi-omap2-mcspi.c
> > +++ b/drivers/spi/spi-omap2-mcspi.c
> > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
> >  
> >  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> >  {
> > +       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
> >         u32 l;
> >  
> >         /* The controller handles the inverted chip selects
> > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> >                 enable = !enable;
> >  
> >         if (spi->controller_state) {
> > +               pm_runtime_get_sync(mcspi->dev);
> > +
> 
> Should the return code should be checked here as with the other
> instances of pm_runtime_get_sync?
> 

Yes, you *must* check return value of pm_runtime_get_sync! Otherwise we
can get another oops/kernel panic. See similar commits:

f7b2b5dd6a62a47e871627c71ed01aa3482154d9
604c31039dae4653f33003d08c91ef58b70b5e63

Anyway, what do you think about adding gcc macro for pm_runtime_get_sync
function which report compiler warning when return value is not checked?

> >                 l = mcspi_cached_chconf0(spi);
> >  
> >                 if (enable)
> > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> >                         l |= OMAP2_MCSPI_CHCONF_FORCE;
> >  
> >                 mcspi_write_chconf0(spi, l);
> > +
> > +               pm_runtime_mark_last_busy(mcspi->dev);
> > +               pm_runtime_put_autosuspend(mcspi->dev);
> >         }
> >  }
> >  
> 
>
Pavel Machek July 22, 2015, 1:30 p.m. UTC | #3
On Wed 2015-07-22 04:03:07, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> > 
> > This code has my head spinning.
> > 
> > I found that the errors do not occur when the driver is built into the kernel.
> > 
> > I also found that with the patch below the errors go away.
> > 
> > Not sure if it is acceptible but see if it fixes things on your side.
> > 
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index cf8b91b..3164d13 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
> >  	if (!spi->max_speed_hz)
> >  		spi->max_speed_hz = spi->master->max_speed_hz;
> >  
> > -	spi_set_cs(spi, false);
> > -
> >  	if (spi->master->setup)
> >  		status = spi->master->setup(spi);
> >  
> > +	spi_set_cs(spi, false);
> > +
> >  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> >  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> >  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> 
> mh. maybe a runtime PM issue?
> 
>  * external abort on non-linefetch: address cannot be accessed,
>    since the module's clocks are disabled
>  * built-in works, module not: built-in is probably a little bit
>    faster (module must not be loaded from filesystem), so that
>    the device has not yet been suspended
>  * Before 4.2, omap2_mcspi_set_cs() was called in the setup
>    routine, which acquired runtime PM
>  * In 4.2, omap2_mcspi_set_cs() seems to be called without a
>    prior pm_runtime_get_sync()
>  * With your workaround, the device has not yet returned to
>    suspend after the runtime PM acquisition in setup()
> 
> So I suggest trying the following (compile tested only) patch:

Solves problem for me. I had to apply it by patch -l.

Tested-by: Pavel Machek <pavel@ucw.cz>
									Pavel
									

> -- Sebastian
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 5867384..f7d9ffd 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
>  
>  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>  {
> +       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
>         u32 l;
>  
>         /* The controller handles the inverted chip selects
> @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>                 enable = !enable;
>  
>         if (spi->controller_state) {
> +               pm_runtime_get_sync(mcspi->dev);
> +
>                 l = mcspi_cached_chconf0(spi);
>  
>                 if (enable)
> @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>                         l |= OMAP2_MCSPI_CHCONF_FORCE;
>  
>                 mcspi_write_chconf0(spi, l);
> +
> +               pm_runtime_mark_last_busy(mcspi->dev);
> +               pm_runtime_put_autosuspend(mcspi->dev);
>         }
>  }
>
Sebastian Reichel July 22, 2015, 1:33 p.m. UTC | #4
Hi,

On Wed, Jul 22, 2015 at 09:27:32AM +0200, Pali Rohár wrote:
> > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> > > index 5867384..f7d9ffd 100644
> > > --- a/drivers/spi/spi-omap2-mcspi.c
> > > +++ b/drivers/spi/spi-omap2-mcspi.c
> > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
> > >  
> > >  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> > >  {
> > > +       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
> > >         u32 l;
> > >  
> > >         /* The controller handles the inverted chip selects
> > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> > >                 enable = !enable;
> > >  
> > >         if (spi->controller_state) {
> > > +               pm_runtime_get_sync(mcspi->dev);
> > > +
> > 
> > Should the return code should be checked here as with the other
> > instances of pm_runtime_get_sync?

Yes. If it fails, the line fetch error would be triggered again.

Probably the set_cs callback should also be changed, so that it can
return an error code. This is not something for 4.2-rc, though.

> Anyway, what do you think about adding gcc macro for pm_runtime_get_sync
> function which report compiler warning when return value is not checked?

Adding __must_check to pm_runtime_get_sync() should be discussed
separately with the relevant people in Cc. A quick grep in drivers/
reveals a huge amount of code not handling pm_runtime_get_sync's return
code.

-- Sebastian
Pali Rohár July 22, 2015, 2:27 p.m. UTC | #5
On Wednesday 22 July 2015 15:33:18 Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jul 22, 2015 at 09:27:32AM +0200, Pali Rohár wrote:
> > > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> > > > index 5867384..f7d9ffd 100644
> > > > --- a/drivers/spi/spi-omap2-mcspi.c
> > > > +++ b/drivers/spi/spi-omap2-mcspi.c
> > > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
> > > >  
> > > >  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> > > >  {
> > > > +       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
> > > >         u32 l;
> > > >  
> > > >         /* The controller handles the inverted chip selects
> > > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> > > >                 enable = !enable;
> > > >  
> > > >         if (spi->controller_state) {
> > > > +               pm_runtime_get_sync(mcspi->dev);
> > > > +
> > > 
> > > Should the return code should be checked here as with the other
> > > instances of pm_runtime_get_sync?
> 
> Yes. If it fails, the line fetch error would be triggered again.
> 
> Probably the set_cs callback should also be changed, so that it can
> return an error code. This is not something for 4.2-rc, though.
> 
> > Anyway, what do you think about adding gcc macro for pm_runtime_get_sync
> > function which report compiler warning when return value is not checked?
> 
> Adding __must_check to pm_runtime_get_sync() should be discussed
> separately with the relevant people in Cc. A quick grep in drivers/
> reveals a huge amount of code not handling pm_runtime_get_sync's return
> code.
> 
> -- Sebastian

Two times, missing check for pm_runtime_get_sync() cased kernel crash on
Nokia N900. See that crypto commits. I think all drivers which do not
check should be fixed. Otherwise we can see another mysterious problems.

Sebastian, can you CC relevant people? Or maybe start new thread?
Pali Rohár July 25, 2015, 8:55 a.m. UTC | #6
On Wednesday 22 July 2015 04:03:07 Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> > 
> > This code has my head spinning.
> > 
> > I found that the errors do not occur when the driver is built into
> > the kernel.
> > 
> > I also found that with the patch below the errors go away.
> > 
> > Not sure if it is acceptible but see if it fixes things on your
> > side.
> > 
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index cf8b91b..3164d13 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
> > 
> >  	if (!spi->max_speed_hz)
> >  	
> >  		spi->max_speed_hz = spi->master->max_speed_hz;
> > 
> > -	spi_set_cs(spi, false);
> > -
> > 
> >  	if (spi->master->setup)
> >  	
> >  		status = spi->master->setup(spi);
> > 
> > +	spi_set_cs(spi, false);
> > +
> > 
> >  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max
> >  	--> %d\n",
> >  	
> >  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> >  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> 
> mh. maybe a runtime PM issue?
> 
>  * external abort on non-linefetch: address cannot be accessed,
>    since the module's clocks are disabled
>  * built-in works, module not: built-in is probably a little bit
>    faster (module must not be loaded from filesystem), so that
>    the device has not yet been suspended
>  * Before 4.2, omap2_mcspi_set_cs() was called in the setup
>    routine, which acquired runtime PM
>  * In 4.2, omap2_mcspi_set_cs() seems to be called without a
>    prior pm_runtime_get_sync()
>  * With your workaround, the device has not yet returned to
>    suspend after the runtime PM acquisition in setup()
> 
> So I suggest trying the following (compile tested only) patch:
> 
> -- Sebastian
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c
> b/drivers/spi/spi-omap2-mcspi.c index 5867384..f7d9ffd 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct
> spi_device *spi, int enable)
> 
>  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>  {
> +       struct omap2_mcspi *mcspi =
> spi_master_get_devdata(spi->master); u32 l;
> 
>         /* The controller handles the inverted chip selects
> @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device
> *spi, bool enable) enable = !enable;
> 
>         if (spi->controller_state) {
> +               pm_runtime_get_sync(mcspi->dev);
> +
>                 l = mcspi_cached_chconf0(spi);
> 
>                 if (enable)
> @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device
> *spi, bool enable) l |= OMAP2_MCSPI_CHCONF_FORCE;
> 
>                 mcspi_write_chconf0(spi, l);
> +
> +               pm_runtime_mark_last_busy(mcspi->dev);
> +               pm_runtime_put_autosuspend(mcspi->dev);
>         }
>  }

This patch fix spi bus for me.

Tested-by: Pali Rohár <pali.rohar@gmail.com>
Pali Rohár July 28, 2015, 8:39 a.m. UTC | #7
On Saturday 25 July 2015 10:55:50 Pali Rohár wrote:
> On Wednesday 22 July 2015 04:03:07 Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote:
> > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote:
> > > 
> > > This code has my head spinning.
> > > 
> > > I found that the errors do not occur when the driver is built into
> > > the kernel.
> > > 
> > > I also found that with the patch below the errors go away.
> > > 
> > > Not sure if it is acceptible but see if it fixes things on your
> > > side.
> > > 
> > > 
> > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > > index cf8b91b..3164d13 100644
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi)
> > > 
> > >  	if (!spi->max_speed_hz)
> > >  	
> > >  		spi->max_speed_hz = spi->master->max_speed_hz;
> > > 
> > > -	spi_set_cs(spi, false);
> > > -
> > > 
> > >  	if (spi->master->setup)
> > >  	
> > >  		status = spi->master->setup(spi);
> > > 
> > > +	spi_set_cs(spi, false);
> > > +
> > > 
> > >  	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max
> > >  	--> %d\n",
> > >  	
> > >  			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> > >  			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> > 
> > mh. maybe a runtime PM issue?
> > 
> >  * external abort on non-linefetch: address cannot be accessed,
> >    since the module's clocks are disabled
> >  * built-in works, module not: built-in is probably a little bit
> >    faster (module must not be loaded from filesystem), so that
> >    the device has not yet been suspended
> >  * Before 4.2, omap2_mcspi_set_cs() was called in the setup
> >    routine, which acquired runtime PM
> >  * In 4.2, omap2_mcspi_set_cs() seems to be called without a
> >    prior pm_runtime_get_sync()
> >  * With your workaround, the device has not yet returned to
> >    suspend after the runtime PM acquisition in setup()
> > 
> > So I suggest trying the following (compile tested only) patch:
> > 
> > -- Sebastian
> > 
> > diff --git a/drivers/spi/spi-omap2-mcspi.c
> > b/drivers/spi/spi-omap2-mcspi.c index 5867384..f7d9ffd 100644
> > --- a/drivers/spi/spi-omap2-mcspi.c
> > +++ b/drivers/spi/spi-omap2-mcspi.c
> > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct
> > spi_device *spi, int enable)
> > 
> >  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
> >  {
> > +       struct omap2_mcspi *mcspi =
> > spi_master_get_devdata(spi->master); u32 l;
> > 
> >         /* The controller handles the inverted chip selects
> > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device
> > *spi, bool enable) enable = !enable;
> > 
> >         if (spi->controller_state) {
> > +               pm_runtime_get_sync(mcspi->dev);
> > +
> >                 l = mcspi_cached_chconf0(spi);
> > 
> >                 if (enable)
> > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device
> > *spi, bool enable) l |= OMAP2_MCSPI_CHCONF_FORCE;
> > 
> >                 mcspi_write_chconf0(spi, l);
> > +
> > +               pm_runtime_mark_last_busy(mcspi->dev);
> > +               pm_runtime_put_autosuspend(mcspi->dev);
> >         }
> >  }
> 
> This patch fix spi bus for me.
> 
> Tested-by: Pali Rohár <pali.rohar@gmail.com>
> 

Sebastian or Michael: Can you add check for pm_runtime_get_sync()
function and send patch for including in mainline kernel?
Sebastian Reichel July 28, 2015, 12:26 p.m. UTC | #8
Hi Pali,

On Tue, Jul 28, 2015 at 10:39:32AM +0200, Pali Rohár wrote:
> Sebastian or Michael: Can you add check for pm_runtime_get_sync()
> function and send patch for including in mainline kernel?

This actually already happened before your Tested-By.
I accidently did not CC you, sorry for that:

https://lkml.org/lkml/2015/7/22/581
https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?h=for-linus&id=5f74db105b1c0980c9863e7a7d1bc0525e0316e8

-- Sebastian
Pali Rohár July 28, 2015, 12:32 p.m. UTC | #9
On Tuesday 28 July 2015 14:26:13 Sebastian Reichel wrote:
> Hi Pali,
> 
> On Tue, Jul 28, 2015 at 10:39:32AM +0200, Pali Rohár wrote:
> > Sebastian or Michael: Can you add check for pm_runtime_get_sync()
> > function and send patch for including in mainline kernel?
> 
> This actually already happened before your Tested-By.
> I accidently did not CC you, sorry for that:
> 
> https://lkml.org/lkml/2015/7/22/581
> https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?h=for-linus&id=5f74db105b1c0980c9863e7a7d1bc0525e0316e8
> 
> -- Sebastian

Ok, so it will be fixed in 4.2. Thanks!

Patch
diff mbox

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 5867384..f7d9ffd 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -245,6 +245,7 @@  static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
 
 static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
 {
+       struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
        u32 l;
 
        /* The controller handles the inverted chip selects
@@ -255,6 +256,8 @@  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
                enable = !enable;
 
        if (spi->controller_state) {
+               pm_runtime_get_sync(mcspi->dev);
+
                l = mcspi_cached_chconf0(spi);
 
                if (enable)
@@ -263,6 +266,9 @@  static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
                        l |= OMAP2_MCSPI_CHCONF_FORCE;
 
                mcspi_write_chconf0(spi, l);
+
+               pm_runtime_mark_last_busy(mcspi->dev);
+               pm_runtime_put_autosuspend(mcspi->dev);
        }
 }