[PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

As request by Grant Likely, there is no more cover letter. Full changelog is following.
I am still reluctant to add this changelog in the patch description, as it adds no value to
the patch itself: when it was needed I try to updat comments or patch description.
I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".


Changelog:
* Change from v1 to v2:
Rebase on linus/master (after 2.6.37-rc1)
Do some clean-up and fix indentation on both patches
Add more explanations for patch 2

* Change from v2 to v3:
Use directly resume function of spi_master instead of using function
from spi_device as Grant Likely pointed it out.
Force this transition explicitly for each CS used by a device.

* Change from v3 to v4:
Patch clean-up according to Kevin Hilman and checkpatch.
Now force CS to be in inactive state only if it was inactive when it was
suspended.

* Change from v4 to v5:
Rebase on linus/master (after 2.6.37-rc3)
Collapse some lines as pointed by Grant Likely
Fix a spelling


== CUT HERE ==
When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.

During the system life, I monitored the CS behavior using a oscilloscope.
I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
Each time the CS was in the correct state.
It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index 2a651e6..dcc024a 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:omap2_mcspi");

+#ifdef	CONFIG_PM
+/* When SPI wake up from off-mode, CS is in activate state. If it was in
+ * unactive state when driver was suspend, then force it to unactive state at
+ * wake up.
+ */
+static int omap2_mcspi_resume(struct platform_device *pdev)
+{
+	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
+	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
+	struct omap2_mcspi_cs *cs;
+
+	omap2_mcspi_enable_clocks(mcspi);
+	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
+			    node) {
+		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
+
+			/* We need to toggle CS state for OMAP take this
+			 * change in account.
+			 */
+			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
+			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
+			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
+			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
+		}
+	}
+	omap2_mcspi_disable_clocks(mcspi);
+	return 0;
+}
+#else
+#define	omap2_mcspi_resume	NULL
+#endif
+
 static struct platform_driver omap2_mcspi_driver = {
 	.driver = {
 		.name =		"omap2_mcspi",
 		.owner =	THIS_MODULE,
 	},
+	.resume =	omap2_mcspi_resume,
 	.remove =	__exit_p(omap2_mcspi_remove),
 };
 -- 1.7.0.4

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Kevin Hilman]

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> As request by Grant Likely, there is no more cover letter. Full changelog is following.
> I am still reluctant to add this changelog in the patch description, as it adds no value to
> the patch itself: when it was needed I try to updat comments or patch description.

You're right, the changelog should not be in the patch description.
This bit of meta-description and changelog should go after the '---'
just after your signoff.  That way, git tools can still auto-apply the
email, and git ignores stuff after the '---' so it doesn't end up in the
git history.

> I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
> Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".

I haven't actually tested it, only reviewed it, so you can add a
Reviewed by for me, but I'm not SPI-aware enough to ack this patch or
test it thoroughly.  Also,  I have some more comments below...

Also, in the last patch I suggested you do more of a save/restore of
this value instead of a restore to a hard-coded value.  IOW, save the
value in the suspend method, restore it in resume. I thought you had
agreed to that.  I'm not an SPI expert, so not sure if it makes sense,
but it seems more robust that way and more future proof.

>
> Changelog:
> * Change from v1 to v2:
> Rebase on linus/master (after 2.6.37-rc1)
> Do some clean-up and fix indentation on both patches
> Add more explanations for patch 2
>
> * Change from v2 to v3:
> Use directly resume function of spi_master instead of using function
> from spi_device as Grant Likely pointed it out.
> Force this transition explicitly for each CS used by a device.
>
> * Change from v3 to v4:
> Patch clean-up according to Kevin Hilman and checkpatch.
> Now force CS to be in inactive state only if it was inactive when it was
> suspended.
>
> * Change from v4 to v5:
> Rebase on linus/master (after 2.6.37-rc3)
> Collapse some lines as pointed by Grant Likely
> Fix a spelling
>
>
> == CUT HERE ==
> When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
>
> During the system life, I monitored the CS behavior using a oscilloscope.
> I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
> Each time the CS was in the correct state.
> It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---

This is where any changelog and extra info for reviewers should go.

>  drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index 2a651e6..dcc024a 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:omap2_mcspi");
>
> +#ifdef	CONFIG_PM
> +/* When SPI wake up from off-mode, CS is in activate state. If it was in
> + * unactive state when driver was suspend, then force it to unactive state at
> + * wake up.
> + */

please fix multi-line comment style.  Search for 'multi-line' in
CodingStyle.

> +static int omap2_mcspi_resume(struct platform_device *pdev)
> +{
> +	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
> +	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
> +	struct omap2_mcspi_cs *cs;
> +
> +	omap2_mcspi_enable_clocks(mcspi);
> +	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
> +			    node) {
> +		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
> +
> +			/* We need to toggle CS state for OMAP take this
> +			 * change in account.
> +			 */

multi-line coding style

> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> +		}
> +	}
> +	omap2_mcspi_disable_clocks(mcspi);
> +	return 0;
> +}
> +#else
> +#define	omap2_mcspi_resume	NULL
> +#endif
> +
>  static struct platform_driver omap2_mcspi_driver = {
>  	.driver = {
>  		.name =		"omap2_mcspi",
>  		.owner =	THIS_MODULE,
>  	},
> +	.resume =	omap2_mcspi_resume,
>  	.remove =	__exit_p(omap2_mcspi_remove),
>  };
>  -- 1.7.0.4

Kevin

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[David Brownell]

--- On Wed, 11/24/10, Kevin Hilman <khilman@deeprootsystems.com> wrote:

>  I'm not SPI-aware enough to ack
> this patch or
> test it thoroughly.

Heh, my excuse is usually "not enough time"
or sometimes "no test setup" ... ;)

In this case I can at least ack the fix in
principle.  CS active means an active trransfer,
which must not happen during OFF  or other
suspend states.


Acked-by: David Brownell <dbrownell@users.sourceforge.net>



> Also, in the last patch I suggested you do more of a
> save/restore of
> this value instead of a restore to a hard-coded
> value.  IOW, save the
> value in the suspend method, restore it in resume.


More correct to restart
message queue processing
after resume, if it's non-empty, and to have
cleanly stopped it (between messages) before
entering suspend states like OFF.


--
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

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

On 11/25/2010 01:49 AM, Kevin Hilman wrote:
> This bit of meta-description and changelog should go after the '---'
> just after your signoff.  That way, git tools can still auto-apply the
> email, and git ignores stuff after the '---' so it doesn't end up in the
> git history.

Thanks for the advice

>> I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
>> Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".
> 
> I haven't actually tested it, only reviewed it, so you can add a
> Reviewed by for me, but I'm not SPI-aware enough to ack this patch or
> test it thoroughly.  Also,  I have some more comments below...

Thanks I will add it in my next version.

> Also, in the last patch I suggested you do more of a save/restore of
> this value instead of a restore to a hard-coded value.  IOW, save the
> value in the suspend method, restore it in resume. I thought you had
> agreed to that.  I'm not an SPI expert, so not sure if it makes sense,
> but it seems more robust that way and more future proof.

Well I took in account your suggestion and I didn't restore anymore an
hardcore value. The state of the CS is already store in cs->chconf0, so
now I use it. The tricky part is omap2_mcspi_enable_clocks restore the
register when clock was disable, but for OMAP2_MCSPI_CHCONF_FORCE bit, we
had to toggle it. When resumed from OFF mode the OMAP2_MCSPI_CHCONF0
register have default value, which means that OMAP2_MCSPI_CHCONF_FORCE
 is 0. So if we want that OMAP2_MCSPI_CHCONF_FORCE=0 is taken in account
we have to first write a 1 and then a 0. But if we want that
OMAP2_MCSPI_CHCONF_FORCE=1, then we have nothing more to do, because in
omap2_mcspi_enable_clocks this bit was change from 0 (its default value)
to 1. That's why I only do a change when OMAP2_MCSPI_CHCONF_FORCE have to
be changed to 0.
I suppose this means that I had to add more comment around my code about
it.

>> When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
>>
>> During the system life, I monitored the CS behavior using a oscilloscope.
>> I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
>> Each time the CS was in the correct state.
>> It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
> 
> This is where any changelog and extra info for reviewers should go.
> 
>>  drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
>>  1 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index 2a651e6..dcc024a 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
>>  /* work with hotplug and coldplug */
>>  MODULE_ALIAS("platform:omap2_mcspi");
>>
>> +#ifdef	CONFIG_PM
>> +/* When SPI wake up from off-mode, CS is in activate state. If it was in
>> + * unactive state when driver was suspend, then force it to unactive state at
>> + * wake up.
>> + */
> 
> please fix multi-line comment style.  Search for 'multi-line' in
> CodingStyle.

OK, I though CodingStyle was about burning GNU coding standards ;)
More seriously I keep the same style that other multi-line comment
in this file. But I have no problem to change it.

> 
>> +static int omap2_mcspi_resume(struct platform_device *pdev)
>> +{
>> +	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
>> +	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
>> +	struct omap2_mcspi_cs *cs;
>> +
>> +	omap2_mcspi_enable_clocks(mcspi);
>> +	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
>> +			    node) {
>> +		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
>> +
>> +			/* We need to toggle CS state for OMAP take this
>> +			 * change in account.
>> +			 */
> 
> multi-line coding style
> 
>> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
>> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
>> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
>> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
>> +		}
>> +	}
>> +	omap2_mcspi_disable_clocks(mcspi);
>> +	return 0;
>> +}
>> +#else
>> +#define	omap2_mcspi_resume	NULL
>> +#endif
>> +
>>  static struct platform_driver omap2_mcspi_driver = {
>>  	.driver = {
>>  		.name =		"omap2_mcspi",
>>  		.owner =	THIS_MODULE,
>>  	},
>> +	.resume =	omap2_mcspi_resume,
>>  	.remove =	__exit_p(omap2_mcspi_remove),
>>  };
>>  -- 1.7.0.4
> 
> Kevin


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Grant Likely]

On Wed, Nov 24, 2010 at 04:49:47PM -0800, Kevin Hilman wrote:
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
> 
> > As request by Grant Likely, there is no more cover letter. Full changelog is following.
> > I am still reluctant to add this changelog in the patch description, as it adds no value to
> > the patch itself: when it was needed I try to updat comments or patch description.
> 
> You're right, the changelog should not be in the patch description.
> This bit of meta-description and changelog should go after the '---'
> just after your signoff.  That way, git tools can still auto-apply the
> email, and git ignores stuff after the '---' so it doesn't end up in the
> git history.

Actually, I used to have the same opinion, but dwmw2 clued me in that
it really is valuable and proper to have the revision history and
changelog in the patch description.  When looking back at what
actually got committed into Linus' tree, the changelog gives hints as
to which exact version of the patch got committed (for instance, if a
v6 got merged, but a v7 was also posted that didn't get merged.)

BTW, this thread has some discussion about this patch not actually
working correctly.  What is the state?  Should this version get
merged, or do I need to wait for a v6?

g.

> 
> > I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
> > Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".
> 
> I haven't actually tested it, only reviewed it, so you can add a
> Reviewed by for me, but I'm not SPI-aware enough to ack this patch or
> test it thoroughly.  Also,  I have some more comments below...
> 
> Also, in the last patch I suggested you do more of a save/restore of
> this value instead of a restore to a hard-coded value.  IOW, save the
> value in the suspend method, restore it in resume. I thought you had
> agreed to that.  I'm not an SPI expert, so not sure if it makes sense,
> but it seems more robust that way and more future proof.
> 
> >
> > Changelog:
> > * Change from v1 to v2:
> > Rebase on linus/master (after 2.6.37-rc1)
> > Do some clean-up and fix indentation on both patches
> > Add more explanations for patch 2
> >
> > * Change from v2 to v3:
> > Use directly resume function of spi_master instead of using function
> > from spi_device as Grant Likely pointed it out.
> > Force this transition explicitly for each CS used by a device.
> >
> > * Change from v3 to v4:
> > Patch clean-up according to Kevin Hilman and checkpatch.
> > Now force CS to be in inactive state only if it was inactive when it was
> > suspended.
> >
> > * Change from v4 to v5:
> > Rebase on linus/master (after 2.6.37-rc3)
> > Collapse some lines as pointed by Grant Likely
> > Fix a spelling
> >
> >
> > == CUT HERE ==
> > When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
> >
> > During the system life, I monitored the CS behavior using a oscilloscope.
> > I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
> > Each time the CS was in the correct state.
> > It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
> >
> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> 
> This is where any changelog and extra info for reviewers should go.
> 
> >  drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
> >  1 files changed, 33 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> > index 2a651e6..dcc024a 100644
> > --- a/drivers/spi/omap2_mcspi.c
> > +++ b/drivers/spi/omap2_mcspi.c
> > @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
> >  /* work with hotplug and coldplug */
> >  MODULE_ALIAS("platform:omap2_mcspi");
> >
> > +#ifdef	CONFIG_PM
> > +/* When SPI wake up from off-mode, CS is in activate state. If it was in
> > + * unactive state when driver was suspend, then force it to unactive state at
> > + * wake up.
> > + */
> 
> please fix multi-line comment style.  Search for 'multi-line' in
> CodingStyle.
> 
> > +static int omap2_mcspi_resume(struct platform_device *pdev)
> > +{
> > +	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
> > +	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
> > +	struct omap2_mcspi_cs *cs;
> > +
> > +	omap2_mcspi_enable_clocks(mcspi);
> > +	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
> > +			    node) {
> > +		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
> > +
> > +			/* We need to toggle CS state for OMAP take this
> > +			 * change in account.
> > +			 */
> 
> multi-line coding style
> 
> > +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
> > +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> > +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
> > +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> > +		}
> > +	}
> > +	omap2_mcspi_disable_clocks(mcspi);
> > +	return 0;
> > +}
> > +#else
> > +#define	omap2_mcspi_resume	NULL
> > +#endif
> > +
> >  static struct platform_driver omap2_mcspi_driver = {
> >  	.driver = {
> >  		.name =		"omap2_mcspi",
> >  		.owner =	THIS_MODULE,
> >  	},
> > +	.resume =	omap2_mcspi_resume,
> >  	.remove =	__exit_p(omap2_mcspi_remove),
> >  };
> >  -- 1.7.0.4
> 
> Kevin

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

On 12/24/2010 12:08 AM, Grant Likely wrote:
> On Wed, Nov 24, 2010 at 04:49:47PM -0800, Kevin Hilman wrote:
>> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
>>
>>> As request by Grant Likely, there is no more cover letter. Full changelog is following.
>>> I am still reluctant to add this changelog in the patch description, as it adds no value to
>>> the patch itself: when it was needed I try to updat comments or patch description.
>>
>> You're right, the changelog should not be in the patch description.
>> This bit of meta-description and changelog should go after the '---'
>> just after your signoff.  That way, git tools can still auto-apply the
>> email, and git ignores stuff after the '---' so it doesn't end up in the
>> git history.
> 
> Actually, I used to have the same opinion, but dwmw2 clued me in that
> it really is valuable and proper to have the revision history and
> changelog in the patch description.  When looking back at what
> actually got committed into Linus' tree, the changelog gives hints as
> to which exact version of the patch got committed (for instance, if a
> v6 got merged, but a v7 was also posted that didn't get merged.)
> 
OK if it is the rule for spi subsystem, I will conform to it, and I will
add my changelog in the patch description.

> BTW, this thread has some discussion about this patch not actually
> working correctly.  What is the state?  Should this version get
> merged, or do I need to wait for a v6?
Well, I tried to answer to each points to Kevin and to David Brownell. I
was waiting for some feedback. I didn't realize it was already near a
month ago, so it should be OK. Then I will insert the last comments of
Kevin and release the v6.

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Hemanth V]

----- Original Message ----- 
From: "Gregory CLEMENT" <gregory.clement@free-electrons.com>
To: "linux-omap" <linux-omap@vger.kernel.org>; 
<spi-devel-general@lists.sourceforge.net>
Cc: "David Brownell" <dbrownell@users.sourceforge.net>; "Grant Likely" 
<grant.likely@secretlab.ca>; "Kevin Hilman" <khilman@deeprootsystems.com>
Sent: Thursday, November 25, 2010 3:49 AM
Subject: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after 
off-mode transition


> As request by Grant Likely, there is no more cover letter. Full changelog 
> is following.
> I am still reluctant to add this changelog in the patch description, as it 
> adds no value to
> the patch itself: when it was needed I try to updat comments or patch 
> description.
> I understand that Grant Likely would need an ack from other user as this 
> patch fix a corner case.
> Kevin Hilman made a few comments on this patch so he could add his "Ack 
> by" or at least his "Review by".
>

I was trying to run some tests with this patch. I find that the resume 
function registered by this patch doesnot seem to
get called during system wide suspend/resume, since spi_resume only calls 
the resume routine registered by spi client driver.
Is there something I am missing.

Thanks
Hemanth

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

On 11/25/2010 09:58 AM, Hemanth V wrote:
> ----- Original Message ----- 
> From: "Gregory CLEMENT" <gregory.clement@free-electrons.com>
> To: "linux-omap" <linux-omap@vger.kernel.org>; 
> <spi-devel-general@lists.sourceforge.net>
> Cc: "David Brownell" <dbrownell@users.sourceforge.net>; "Grant Likely" 
> <grant.likely@secretlab.ca>; "Kevin Hilman" <khilman@deeprootsystems.com>
> Sent: Thursday, November 25, 2010 3:49 AM
> Subject: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after 
> off-mode transition
> 
> 
>> As request by Grant Likely, there is no more cover letter. Full changelog 
>> is following.
>> I am still reluctant to add this changelog in the patch description, as it 
>> adds no value to
>> the patch itself: when it was needed I try to updat comments or patch 
>> description.
>> I understand that Grant Likely would need an ack from other user as this 
>> patch fix a corner case.
>> Kevin Hilman made a few comments on this patch so he could add his "Ack 
>> by" or at least his "Review by".
>>
> 
> I was trying to run some tests with this patch. I find that the resume 
> function registered by this patch doesnot seem to
> get called during system wide suspend/resume, since spi_resume only calls 
> the resume routine registered by spi client driver.
> Is there something I am missing.

In fact the resume function for this driver won't be called by spi bus but by
platform bus. Indeed this function is registered in a platform_driver structure.

> 
> Thanks
> Hemanth
> 
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Kevin Hilman]

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

> As request by Grant Likely, there is no more cover letter. Full changelog is following.
> I am still reluctant to add this changelog in the patch description, as it adds no value to
> the patch itself: when it was needed I try to updat comments or patch description.
> I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
> Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".

A couple more comments...

>
> Changelog:
> * Change from v1 to v2:
> Rebase on linus/master (after 2.6.37-rc1)
> Do some clean-up and fix indentation on both patches
> Add more explanations for patch 2
>
> * Change from v2 to v3:
> Use directly resume function of spi_master instead of using function
> from spi_device as Grant Likely pointed it out.
> Force this transition explicitly for each CS used by a device.
>
> * Change from v3 to v4:
> Patch clean-up according to Kevin Hilman and checkpatch.
> Now force CS to be in inactive state only if it was inactive when it was
> suspended.
>
> * Change from v4 to v5:
> Rebase on linus/master (after 2.6.37-rc3)
> Collapse some lines as pointed by Grant Likely
> Fix a spelling
>
>
> == CUT HERE ==
> When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
>
> During the system life, I monitored the CS behavior using a oscilloscope.
> I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
> Each time the CS was in the correct state.
> It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index 2a651e6..dcc024a 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:omap2_mcspi");
>
> +#ifdef	CONFIG_PM

You should use CONFIG_SUSPEND here

> +/* When SPI wake up from off-mode, CS is in activate state. If it was in
> + * unactive state when driver was suspend, then force it to unactive state at
> + * wake up.
> + */
> +static int omap2_mcspi_resume(struct platform_device *pdev)
> +{
> +	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
> +	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
> +	struct omap2_mcspi_cs *cs;
> +
> +	omap2_mcspi_enable_clocks(mcspi);
> +	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
> +			    node) {
> +		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
> +
> +			/* We need to toggle CS state for OMAP take this
> +			 * change in account.
> +			 */
> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> +		}
> +	}
> +	omap2_mcspi_disable_clocks(mcspi);
> +	return 0;
> +}
> +#else
> +#define	omap2_mcspi_resume	NULL
> +#endif
> +
>  static struct platform_driver omap2_mcspi_driver = {
>  	.driver = {
>  		.name =		"omap2_mcspi",
>  		.owner =	THIS_MODULE,
>  	},
> +	.resume =	omap2_mcspi_resume,

This is adding legacy PM methods.  Instead, you should add a struct
dev_pm_ops and add the resume method there.

Kevin

>  	.remove =	__exit_p(omap2_mcspi_remove),
>  };
>  -- 1.7.0.4

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

On 11/29/2010 03:22 PM, Kevin Hilman wrote:
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
> 
>> As request by Grant Likely, there is no more cover letter. Full changelog is following.
>> I am still reluctant to add this changelog in the patch description, as it adds no value to
>> the patch itself: when it was needed I try to updat comments or patch description.
>> I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
>> Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".
> 
> A couple more comments...
> 
>>
>> Changelog:
>> * Change from v1 to v2:
>> Rebase on linus/master (after 2.6.37-rc1)
>> Do some clean-up and fix indentation on both patches
>> Add more explanations for patch 2
>>
>> * Change from v2 to v3:
>> Use directly resume function of spi_master instead of using function
>> from spi_device as Grant Likely pointed it out.
>> Force this transition explicitly for each CS used by a device.
>>
>> * Change from v3 to v4:
>> Patch clean-up according to Kevin Hilman and checkpatch.
>> Now force CS to be in inactive state only if it was inactive when it was
>> suspended.
>>
>> * Change from v4 to v5:
>> Rebase on linus/master (after 2.6.37-rc3)
>> Collapse some lines as pointed by Grant Likely
>> Fix a spelling
>>
>>
>> == CUT HERE ==
>> When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
>>
>> During the system life, I monitored the CS behavior using a oscilloscope.
>> I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
>> Each time the CS was in the correct state.
>> It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/spi/omap2_mcspi.c |   33 +++++++++++++++++++++++++++++++++
>>  1 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index 2a651e6..dcc024a 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
>>  /* work with hotplug and coldplug */
>>  MODULE_ALIAS("platform:omap2_mcspi");
>>
>> +#ifdef	CONFIG_PM
> 
> You should use CONFIG_SUSPEND here

OK I will do this.

> 
>> +/* When SPI wake up from off-mode, CS is in activate state. If it was in
>> + * unactive state when driver was suspend, then force it to unactive state at
>> + * wake up.
>> + */
>> +static int omap2_mcspi_resume(struct platform_device *pdev)
>> +{
>> +	struct spi_master	*master = dev_get_drvdata(&pdev->dev);
>> +	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
>> +	struct omap2_mcspi_cs *cs;
>> +
>> +	omap2_mcspi_enable_clocks(mcspi);
>> +	list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
>> +			    node) {
>> +		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
>> +
>> +			/* We need to toggle CS state for OMAP take this
>> +			 * change in account.
>> +			 */
>> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
>> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
>> +			MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
>> +			__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
>> +		}
>> +	}
>> +	omap2_mcspi_disable_clocks(mcspi);
>> +	return 0;
>> +}
>> +#else
>> +#define	omap2_mcspi_resume	NULL
>> +#endif
>> +
>>  static struct platform_driver omap2_mcspi_driver = {
>>  	.driver = {
>>  		.name =		"omap2_mcspi",
>>  		.owner =	THIS_MODULE,
>>  	},
>> +	.resume =	omap2_mcspi_resume,
> 
> This is adding legacy PM methods.  Instead, you should add a struct
> dev_pm_ops and add the resume method there.

OK, I shouldn't copy and paste an old driver!
Thanks for your advices.

> 
> Kevin
> 
>>  	.remove =	__exit_p(omap2_mcspi_remove),
>>  };
>>  -- 1.7.0.4


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[David Brownell]

> > Now force CS to be in inactive state only if it was
> inactive when it was suspended.

Note that it's a bug if CS is active in
any suspend state (including OFF).  That
strongly suggests $SUBJECT is an incomplete
workaround for that other bug...

> >
> > When SPI wake up from OFF mode, CS is in the wrong
> state: force it to the inactive state.

Best report the bug that the suspend/OFF state
was mis-handled...  That is, it didn't
correctly ENTER that OFF mode...

In fact ... I'd like to see that fixed more
than the $SUBJECT issue, so the root cause
gets resolved...

CS should only be active while a SPI message
is being processed, and such processing must
have completed before suspend/OFF/... starts

Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Gregory CLEMENT]

On 11/30/2010 04:08 AM, David Brownell wrote:
>>> Now force CS to be in inactive state only if it was
>> inactive when it was suspended.
> 
> Note that it's a bug if CS is active in
> any suspend state (including OFF).  That
> strongly suggests $SUBJECT is an incomplete
> workaround for that other bug...
> 

It is not the case, at least it is not what we
observed. In suspend state, CS is inactive.

>>>
>>> When SPI wake up from OFF mode, CS is in the wrong
>> state: force it to the inactive state.
> 
> Best report the bug that the suspend/OFF state
> was mis-handled...  That is, it didn't
> correctly ENTER that OFF mode...
> 
> In fact ... I'd like to see that fixed more
> than the $SUBJECT issue, so the root cause
> gets resolved...

As far as I know, it enter correctly that OFF mode.
It is only at wake up, that CS become active while
there is no SPI message being processed. It is the
point of my patch.

My first patch version forced unconditionally CS to
inactive state at wake up. It was correct from the
point of view of the SPI but not for suspend/resume.
Resume should only restore the state of the driver
just before suspend. If resume force the inactive
state it could mask a bug in the suspend path.
But as I wrote before, as far as I know there is no
bug when driver enter suspend mode, at this moment
CS is inactive.

> 
> CS should only be active while a SPI message
> is being processed, and such processing must
> have completed before suspend/OFF/... starts
> 
I agree and this driver seems to respect this point.

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com