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
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).
--- drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)
drivers/spi/omap2_mcspi.c +33 -0
@@ -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
Quoted 3 lines · 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.
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.
Quoted 2 lines
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.
Quoted 26 lines
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 ofspi_masterinstead of using function fromspi_deviceas 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.
Quoted 7 lines
During the system life, I monitored the CS behavior using a oscilloscope.
I also activated debug inomap2_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 <[email protected]>---
This is where any changelog and extra info for reviewers should go.
Quoted 8 lines
drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)drivers/spi/omap2_mcspi.c +0
-0
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1312
Quoted 8 lines
drivers/spi/omap2_mcspi.c +5
-0@@ -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.
Quoted 4 lines
drivers/spi/omap2_mcspi.c +4
-0+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);
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1326
Quoted 10 lines
drivers/spi/omap2_mcspi.c +10
-0+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
Quoted 12 lines
drivers/spi/omap2_mcspi.c +12
-0+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 1348
Quoted 10 lines
drivers/spi/omap2_mcspi.c +2
-0+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
David Brownell
Acked-byQuoted 3 lines · Kevin Hilman
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.
Quoted 5 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
----- Original Message ----- From: "Gregory CLEMENT" <[email protected]>
To: "linux-omap" <[email protected]>; <[email protected]>
Cc: "David Brownell" <[email protected]>; "Grant Likely" <[email protected]>; "Kevin Hilman" <[email protected]>
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
Quoted 6 lines
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
Quoted 5 lines · 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".
A couple more comments...
Quoted 41 lines
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 ofspi_masterinstead of using function fromspi_deviceas 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 inomap2_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 <[email protected]>--- drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)drivers/spi/omap2_mcspi.c +0
-0
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1308
Quoted 4 lines
drivers/spi/omap2_mcspi.c +1
-0@@ -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
Quoted 27 lines
drivers/spi/omap2_mcspi.c +27
-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 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 ; +}
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1345
Quoted 10 lines
drivers/spi/omap2_mcspi.c +5
-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
Quoted 3 lines
drivers/spi/omap2_mcspi.c +0
-0.remove = __exit_p(omap2_mcspi_remove), }; -- 1.7.0.4
Quoted 4 lines · Kevin Hilman
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
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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.
Quoted 23 lines
Quoted 9 lines
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 inomap2_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 <[email protected]>---
This is where any changelog and extra info for reviewers should go.
Quoted 11 lines
drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)drivers/spi/omap2_mcspi.c +0
-0@@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)/* work with hotplug and coldplug */ MODULE_ALIAS ("platform:omap2_mcspi" );
Quoted 8 lines
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1312
Quoted 5 lines
drivers/spi/omap2_mcspi.c +5
-0+#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.
Quoted 42 lines
Quoted 5 lines
drivers/spi/omap2_mcspi.c +4
-0+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);drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1326
Quoted 10 lines
drivers/spi/omap2_mcspi.c +10
-0+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
Quoted 12 lines
drivers/spi/omap2_mcspi.c +12
-0+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 1348
Quoted 10 lines
drivers/spi/omap2_mcspi.c +2
-0+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.4Kevin
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 54 lines · Kevin Hilman
Quoted 5 lines · 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".A couple more comments...
Quoted 44 lines
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 ofspi_masterinstead of using function fromspi_deviceas 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 inomap2_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 <[email protected]>--- drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)drivers/spi/omap2_mcspi.c +0
-0@@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)/* work with hotplug and coldplug */ MODULE_ALIAS ("platform:omap2_mcspi" );
Quoted 3 lines
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1308
Quoted 1 line
drivers/spi/omap2_mcspi.c +1
-0+#ifdef CONFIG_PM You should use
CONFIG_SUSPENDhere
OK I will do this.
Quoted 33 lines
Quoted 33 lines
drivers/spi/omap2_mcspi.c +31
-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 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 = {
Quoted 8 lines
drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1345
Quoted 5 lines
drivers/spi/omap2_mcspi.c +1
-0.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_opsand add the resume method there.
OK, I shouldn't copy and paste an old driver! Thanks for your advices.
Quoted 6 lines
Kevin
Quoted 3 lines
drivers/spi/omap2_mcspi.c +0
-0.remove = __exit_p(omap2_mcspi_remove), }; -- 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
Quoted 23 lines · Hemanth V
----- Original Message -----
From: "Gregory CLEMENT" <[email protected]>
To: "linux-omap" <[email protected]>; <[email protected]>
Cc: "David Brownell" <[email protected]>; "Grant Likely" <[email protected]>; "Kevin Hilman" <[email protected]>
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 transitionQuoted 11 lines
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".
Quoted 5 lines
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, sincespi_resumeonly 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.
Quoted 5 lines
Thanks
Hemanth
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 2 lines
Quoted 1 line
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...
Quoted 3 lines
Quoted 2 lines
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
Quoted 8 lines · David Brownell
Quoted 2 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 3 lines
Quoted 2 lines
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.
Quoted 5 lines
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.
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 11 lines · Kevin Hilman
Quoted 3 lines · 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.
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.
Quoted 111 lines
Quoted 3 lines
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.
Quoted 26 lines
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 ofspi_masterinstead of using function fromspi_deviceas 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.Quoted 7 lines
During the system life, I monitored the CS behavior using a oscilloscope.
I also activated debug inomap2_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 <[email protected]>---
This is where any changelog and extra info for reviewers should go.
Quoted 8 lines
drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)drivers/spi/omap2_mcspi.c +0
-0drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1312
Quoted 8 lines
drivers/spi/omap2_mcspi.c +5
-0@@ -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.
Quoted 4 lines
drivers/spi/omap2_mcspi.c +4
-0+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);drivers/spi/omap2_mcspi.c static int __exit omap2_mcspi_remove(struct platform_device *pdev) · line 1326
Quoted 10 lines
drivers/spi/omap2_mcspi.c +10
-0+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
Quoted 12 lines
drivers/spi/omap2_mcspi.c +12
-0+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 1348
Quoted 10 lines
drivers/spi/omap2_mcspi.c +2
-0+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.4Kevin
Quoted 13 lines · Grant Likely
Quoted 11 lines · Kevin Hilman
Quoted 3 lines · 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.
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.
Quoted 7 lines
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.
Quoted 3 lines
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.
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com