OMAP2: Spi: Force CS to be in inactive state after off-mode transition

PATCH Superseded Gregory CLEMENT 2 participants 4 messages Dec 24, 2010 – Dec 29, 2010 SPI

lore

Share & tools

lore

Gregory CLEMENT

author

2010-12-24 12:21 UTC #0 lore raw

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

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

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

* Change from v5 to v6:
Rebase on linus/master (after 2.6.37-rc7)
Use CONFIG_SUSPEND instead of CONFIG_PM
Didn't use legacy PM methods anymore. Instead, add a struct dev_pm_ops and add the resume method there. Fix multi-line comment style

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

drivers/spi/omap2_mcspi.c +39 -0

index 2a651e6..a17ea97 100644
@@ -1305,10 +1305,50 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:omap2_mcspi");
 +#ifdef	CONFIG_SUSPEND
+/*
+ * 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 device *dev)
+{
+	struct spi_master	*master = dev_get_drvdata(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 const struct dev_pm_ops omap2_mcspi_pm_ops = {
+	.resume = omap2_mcspi_resume,
+};
+
 static struct platform_driver omap2_mcspi_driver = {
 	.driver = {
 		.name =		"omap2_mcspi",
 		.owner =	THIS_MODULE,
+		.pm =		&omap2_mcspi_pm_ops
+
 	},
 	.remove =	__exit_p(omap2_mcspi_remove),
 };

Signature · 1 line

1.7.0.4

Grant Likely

2010-12-29 07:28 UTC #1 lore raw

Quoted 28 lines · Gregory CLEMENT

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

Quoted 7 lines

* Change from v5 to v6:
Rebase on linus/master (after 2.6.37-rc7)
Use CONFIG_SUSPEND instead of CONFIG_PM
Didn't use legacy PM methods anymore. Instead, add a struct dev_pm_ops and add the resume method there. Fix multi-line comment style
Signed-off-by Gregory CLEMENT <[email protected]>

Hi Gregory,

Patch does not apply, but I cannot immediately isolate the damage. Please see if you can figure out the issue on your end. Is it your mailer? I could fudge it, but in the long run it is better if this issue can get sorted out now.

Also, a few other minor nitpicks (I won't reject a patch over these, but it helps to have them right):

Please prefix spi patch names with "spi[/driver]:". It is the format for all spi patches I take through my tree, and if it already follows that convention, then I don't need to edit it manually.

Second, please keep the line length in the patch description short; in general I find that a textwidth limit of 70 characters is about right for the purpose of reading/replying in email. A lot of maintainers use text email clients like mutt in 80 column terminal windows. Keeping a reasonable right hand margin in the text body is simply friendly.

Finally, in changelogs it is easier to read if there is some kind of itemization ticks. For example:

* Change from v5 to v6:
- Rebase on linus/master (after 2.6.37-rc7)
- Use CONFIG_SUSPEND instead of CONFIG_PM
- Didn't use legacy PM methods anymore. Instead, add a struct dev_pm_ops and add the resume method there.
- Fix multi-line comment style

Quoted 45 lines

---
 drivers/spi/omap2_mcspi.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

drivers/spi/omap2_mcspi.c +33 -0

index 2a651e6..a17ea97 100644
@@ -1305,10 +1305,50 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:omap2_mcspi");
 +#ifdef	CONFIG_SUSPEND
+/*
+ * 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 device *dev)
+{
+	struct spi_master	*master = dev_get_drvdata(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
+

drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1350

Quoted 10 lines

drivers/spi/omap2_mcspi.c +6 -0

+static const struct dev_pm_ops omap2_mcspi_pm_ops = {
+	.resume = omap2_mcspi_resume,
+};
+
 static struct platform_driver omap2_mcspi_driver = {
 	.driver = {
 		.name =		"omap2_mcspi",
 		.owner =	THIS_MODULE,
+		.pm =		&omap2_mcspi_pm_ops
+

Drop the extra line.

Quoted 6 lines

drivers/spi/omap2_mcspi.c +0 -0
 	},
 	.remove =	__exit_p(omap2_mcspi_remove),
 };
Signature · 1 line
1.7.0.4

Gregory CLEMENT

author

2010-12-29 09:01 UTC #2 lore raw

Quoted 2 lines

Hi Gregory,

Hi Grant,

Quoted 5 lines

Patch does not apply, but I cannot immediately isolate the damage. Please see if you can figure out the issue on your end. Is it your mailer? I could fudge it, but in the long run it is better if this issue can get sorted out now.

I found the problem: see below in the patch itself.

Quoted 17 lines

Also, a few other minor nitpicks (I won't reject a patch over these, but it helps to have them right):
Please prefix spi patch names with "spi[/driver]:". It is the format for all spi patches I take through my tree, and if it already follows that convention, then I don't need to edit it manually.
Second, please keep the line length in the patch description short; in general I find that a textwidth limit of 70 characters is about right for the purpose of reading/replying in email. A lot of maintainers use text email clients like mutt in 80 column terminal windows. Keeping a reasonable right hand margin in the text body is simply friendly.
Finally, in changelogs it is easier to read if there is some kind of itemization ticks. For example:

Quoted 7 lines

* Change from v5 to v6:
- Rebase on linus/master (after 2.6.37-rc7)
- Use CONFIG_SUSPEND instead of CONFIG_PM
- Didn't use legacy PM methods anymore. Instead, add a struct dev_pm_ops and add the resume method there.
- Fix multi-line comment style

I will fix these (patch prefix, wrapping and itemization ticks) and send the patch again today.

Quoted 6 lines

Quoted 4 lines

---
 drivers/spi/omap2_mcspi.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

Quoted 8 lines

drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1307
Quoted 8 lines
drivers/spi/omap2_mcspi.c +0 -0
index 2a651e6..a17ea97 100644
@@ -1305,10 +1305,50 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:omap2_mcspi");
 +#ifdef	CONFIG_SUSPEND

^
|
Problem was here, I deleted a carrier in error :(

Quoted 52 lines

Quoted 33 lines

drivers/spi/omap2_mcspi.c +33 -0

+/*
+ * 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 device *dev)
+{
+	struct spi_master	*master = dev_get_drvdata(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
+

drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1350

Quoted 10 lines

drivers/spi/omap2_mcspi.c +6 -0

+static const struct dev_pm_ops omap2_mcspi_pm_ops = {
+	.resume = omap2_mcspi_resume,
+};
+
 static struct platform_driver omap2_mcspi_driver = {
 	.driver = {
 		.name =		"omap2_mcspi",
 		.owner =	THIS_MODULE,
+		.pm =		&omap2_mcspi_pm_ops
+

Drop the extra line.

Quoted 6 lines

drivers/spi/omap2_mcspi.c +0 -0
 	},
 	.remove =	__exit_p(omap2_mcspi_remove),
 };
Signature · 1 line
1.7.0.4

Signature · 4 lines

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

Gregory CLEMENT

author

2010-12-29 11:07 UTC #3 lore raw

Quoted 4 lines · Gregory CLEMENT

Quoted 2 lines
Hi Gregory,
Hi Grant,

Quoted 8 lines

Quoted 5 lines
Patch does not apply, but I cannot immediately isolate the damage. Please see if you can figure out the issue on your end. Is it your mailer? I could fudge it, but in the long run it is better if this issue can get sorted out now.
I found the problem: see below in the patch itself.

Indeed it was may mailer which had broken the patch. I use git-format-patch and git imap send to create the mail. But when I open it from the draft folder to add the email address, it decides by itself to remove an end of line!
It is not the first time that I have some problems with Thunderbird. I consider changing my mailer to a more "hacker friendly" one.

Signature · 4 lines

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