[PATCH v1 0/3] PM / AVS: SmartReflex: driver misc fixes

[PATCH v1 0/3] PM / AVS: SmartReflex: driver misc fixes

[Andrii Tseglytskyi]

The following patch series contain several misc fixes to SmartReflex driver:

1. disable errgen before vpbound disable. Critical fix, needed for
proper work of AVS-VP communicaton protocol.

2. disable runtime PM on driver remove. Trivial - runtime PM cleanup.

3. fix driver name. Trivial - proper DRIVER_NAME was not defined
since SmartReflex driver was introduced with initial commit.

Patches are based on:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
tag: v3.10-rc2

Verified on OMAP4430. Boot - OK. SmartReflex registers debug dump - OK

Available on GitHub:
https://github.com/andriit/linux-omap-k3.8/commits/avs_sr_driver_misc_fixes_v01

Andrii Tseglytskyi (2):
  PM / AVS: SmartReflex: disable runtime PM on driver remove
  PM / AVS: SmartReflex: fix driver name

Nishanth Menon (1):
  PM / AVS: SmartReflex: disable errgen before vpbound disable

 drivers/power/avs/smartreflex.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Andrii Tseglytskyi]

From: Nishanth Menon <nm@ti.com>

vpboundsintr_en is available inside the IP block as an re-sycned
version and one which is not. Due to this, there is an 1 sysclk
cycle window where interruptz could be asserted low for 1 cycle.
IF, intr_en is cleared on the exact same cycle as the irqclr, an
additional pulse is generated which indicates for VP that
an additional adjustment of voltage is required.

This results in VP doing two voltage adjustments for the SRERR
(based on configuration, upto 4 steps), instead of the needed
1 step.
Due to the unexpected pulse from AVS which breaks the AVS-VP
communication protocol, VP also ends up in a stuck condition by
entering a state where VP module remains non-responsive
to any futher AVS adjustment events. This creates the symptom
called "TRANXDONE Timeout" scenario.

By disabling errgen prior to disable of intr_en, this situation
can be avoided.

Signed-off-by: Vincent Bour <v-bour@ti.com>
Signed-off-by: Leonardo Affortunati <l-affortunati@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Andrii.Tseglytskyi <andrii.tseglytskyi@ti.com>
---
 drivers/power/avs/smartreflex.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
index 6b2238b..f34d34d 100644
--- a/drivers/power/avs/smartreflex.c
+++ b/drivers/power/avs/smartreflex.c
@@ -449,12 +449,17 @@ int sr_disable_errgen(struct voltagedomain *voltdm)
 		return -EINVAL;
 	}
 
-	/* Disable the interrupts of ERROR module */
-	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
-
 	/* Disable the Sensor and errorgen */
 	sr_modify_reg(sr, SRCONFIG, SRCONFIG_SENENABLE | SRCONFIG_ERRGEN_EN, 0);
 
+	/*
+	 * Disable the interrupts of ERROR module
+	 * NOTE: modify is a read, modify,write - an implicit OCP barrier
+	 * which is required is present here - sequencing is critical
+	 * at this point (after errgen is disabled, vpboundint disable)
+	 */
+	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Kevin Hilman]

Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:

> From: Nishanth Menon <nm@ti.com>
>
> vpboundsintr_en is available inside the IP block as an re-sycned
> version and one which is not. Due to this, there is an 1 sysclk
> cycle window where interruptz could be asserted low for 1 cycle.
                             ^^^

Is that the way the cool kidz are spelling interrupts these days?  ;)

> IF, intr_en is cleared on the exact same cycle as the irqclr, an
> additional pulse is generated which indicates for VP that
> an additional adjustment of voltage is required.
>
> This results in VP doing two voltage adjustments for the SRERR
> (based on configuration, upto 4 steps), instead of the needed
> 1 step.
> Due to the unexpected pulse from AVS which breaks the AVS-VP
> communication protocol, VP also ends up in a stuck condition by
> entering a state where VP module remains non-responsive
> to any futher AVS adjustment events. This creates the symptom
> called "TRANXDONE Timeout" scenario.
>
> By disabling errgen prior to disable of intr_en, this situation
> can be avoided.
>
> Signed-off-by: Vincent Bour <v-bour@ti.com>
> Signed-off-by: Leonardo Affortunati <l-affortunati@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Andrii.Tseglytskyi <andrii.tseglytskyi@ti.com>

>From Documentation/SubbmittingPatches:

   "If a person was not directly involved in the preparation or handling
   of a patch but wishes to signify and record their approval of it then
   they can arrange to have an Acked-by: line added to the patch's
   changelog."

Are all of the tags above co-authors or on the delivery path?  I suspect
an Acked-by or Reviewed-by is more appropriate here.

Otherwise, patch itself looks fine.

Kevin

> ---
>  drivers/power/avs/smartreflex.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
> index 6b2238b..f34d34d 100644
> --- a/drivers/power/avs/smartreflex.c
> +++ b/drivers/power/avs/smartreflex.c
> @@ -449,12 +449,17 @@ int sr_disable_errgen(struct voltagedomain *voltdm)
>  		return -EINVAL;
>  	}
>  
> -	/* Disable the interrupts of ERROR module */
> -	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
> -
>  	/* Disable the Sensor and errorgen */
>  	sr_modify_reg(sr, SRCONFIG, SRCONFIG_SENENABLE | SRCONFIG_ERRGEN_EN, 0);
>  
> +	/*
> +	 * Disable the interrupts of ERROR module
> +	 * NOTE: modify is a read, modify,write - an implicit OCP barrier
> +	 * which is required is present here - sequencing is critical
> +	 * at this point (after errgen is disabled, vpboundint disable)
> +	 */
> +	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
> +
>  	return 0;
>  }

Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Andrii Tseglytskyi]

Hi Kevin,

Thanks a lot for your comments.

On 05/28/2013 09:45 PM, Kevin Hilman wrote:
> Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:
>
>> From: Nishanth Menon <nm@ti.com>
>>
>> vpboundsintr_en is available inside the IP block as an re-sycned
>> version and one which is not. Due to this, there is an 1 sysclk
>> cycle window where interruptz could be asserted low for 1 cycle.
>                               ^^^
>
> Is that the way the cool kidz are spelling interrupts these days?  ;)

Oh! Shame on me. Thank you for catching this :)

>> IF, intr_en is cleared on the exact same cycle as the irqclr, an
>> additional pulse is generated which indicates for VP that
>> an additional adjustment of voltage is required.
>>
>> This results in VP doing two voltage adjustments for the SRERR
>> (based on configuration, upto 4 steps), instead of the needed
>> 1 step.
>> Due to the unexpected pulse from AVS which breaks the AVS-VP
>> communication protocol, VP also ends up in a stuck condition by
>> entering a state where VP module remains non-responsive
>> to any futher AVS adjustment events. This creates the symptom
>> called "TRANXDONE Timeout" scenario.
>>
>> By disabling errgen prior to disable of intr_en, this situation
>> can be avoided.
>>
>> Signed-off-by: Vincent Bour <v-bour@ti.com>
>> Signed-off-by: Leonardo Affortunati <l-affortunati@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Andrii.Tseglytskyi <andrii.tseglytskyi@ti.com>
>  From Documentation/SubbmittingPatches:
>
>     "If a person was not directly involved in the preparation or handling
>     of a patch but wishes to signify and record their approval of it then
>     they can arrange to have an Acked-by: line added to the patch's
>     changelog."
>
> Are all of the tags above co-authors or on the delivery path?  I suspect
> an Acked-by or Reviewed-by is more appropriate here.
>
> Otherwise, patch itself looks fine.
>
> Kevin

This patch was developed by Nishanth, he is the author of the code.
Patch is the result of 2-week debug session. Vincent, Leonardo and 
Nishanth were involved to that debug session.
My contribution to this patch - is sending it to upstream, no more than 
this.
Could you please suggest - who should remain in Signed-offs ?

>> ---
>>   drivers/power/avs/smartreflex.c |   11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
>> index 6b2238b..f34d34d 100644
>> --- a/drivers/power/avs/smartreflex.c
>> +++ b/drivers/power/avs/smartreflex.c
>> @@ -449,12 +449,17 @@ int sr_disable_errgen(struct voltagedomain *voltdm)
>>   		return -EINVAL;
>>   	}
>>   
>> -	/* Disable the interrupts of ERROR module */
>> -	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
>> -
>>   	/* Disable the Sensor and errorgen */
>>   	sr_modify_reg(sr, SRCONFIG, SRCONFIG_SENENABLE | SRCONFIG_ERRGEN_EN, 0);
>>   
>> +	/*
>> +	 * Disable the interrupts of ERROR module
>> +	 * NOTE: modify is a read, modify,write - an implicit OCP barrier
>> +	 * which is required is present here - sequencing is critical
>> +	 * at this point (after errgen is disabled, vpboundint disable)
>> +	 */
>> +	sr_modify_reg(sr, errconfig_offs, vpboundint_en | vpboundint_st, 0);
>> +
>>   	return 0;
>>   }

Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Kevin Hilman]

Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:

[...]

>>> Signed-off-by: Vincent Bour <v-bour@ti.com>
>>> Signed-off-by: Leonardo Affortunati <l-affortunati@ti.com>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Andrii.Tseglytskyi <andrii.tseglytskyi@ti.com>
>>  From Documentation/SubbmittingPatches:
>>
>>     "If a person was not directly involved in the preparation or handling
>>     of a patch but wishes to signify and record their approval of it then
>>     they can arrange to have an Acked-by: line added to the patch's
>>     changelog."
>>
>> Are all of the tags above co-authors or on the delivery path?  I suspect
>> an Acked-by or Reviewed-by is more appropriate here.

[...]

> This patch was developed by Nishanth, he is the author of the code.
> Patch is the result of 2-week debug session. Vincent, Leonardo and
> Nishanth were involved to that debug session.
> My contribution to this patch - is sending it to upstream, no more
> than this.
> Could you please suggest - who should remain in Signed-offs ?

Nishanth as the author, and you since you're on the delivery path.

If Vincent & Leonardo should be considered as co-authors, then their
signoffs are fine, otherwise they should be reviewed-by or acked-by.

Kevin

Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Andrii Tseglytskyi]

On 05/29/2013 12:58 PM, Andrii Tseglytskyi wrote:
> Hi Kevin,
>
> Thanks a lot for your comments.
>
> On 05/28/2013 09:45 PM, Kevin Hilman wrote:
>> Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:
>>
>>> From: Nishanth Menon <nm@ti.com>
>>>
>>> vpboundsintr_en is available inside the IP block as an re-sycned
>>> version and one which is not. Due to this, there is an 1 sysclk
>>> cycle window where interruptz could be asserted low for 1 cycle.
>>                               ^^^
>>
>> Is that the way the cool kidz are spelling interrupts these days?  ;)
>
> Oh! Shame on me. Thank you for catching this :)

Shame on me again (((. Name "interruptz" is more less OK. This is the 
name of signal between Voltage Processor and SmartReflex.
In documentation it is referenced as "SR_SInterruptz". Anyway commit 
message should be updated to make this more clear.


[snip]
>
>

Regards,
Andrii

Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable

[Kevin Hilman]

Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:

> On 05/29/2013 12:58 PM, Andrii Tseglytskyi wrote:
>> Hi Kevin,
>>
>> Thanks a lot for your comments.
>>
>> On 05/28/2013 09:45 PM, Kevin Hilman wrote:
>>> Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:
>>>
>>>> From: Nishanth Menon <nm@ti.com>
>>>>
>>>> vpboundsintr_en is available inside the IP block as an re-sycned
>>>> version and one which is not. Due to this, there is an 1 sysclk
>>>> cycle window where interruptz could be asserted low for 1 cycle.
>>>                               ^^^
>>>
>>> Is that the way the cool kidz are spelling interrupts these days?  ;)
>>
>> Oh! Shame on me. Thank you for catching this :)
>
> Shame on me again (((. Name "interruptz" is more less OK. This is the
> name of signal between Voltage Processor and SmartReflex.
> In documentation it is referenced as "SR_SInterruptz". Anyway commit
> message should be updated to make this more clear.

Ah, OK.  Makes sense now.  

Yes, the changlog should be more clear that this is referring to a
signal name, e.g. 

       ...there is an 1 sysclk cycle window where the SR_SInterruptz
       signal could be asserted low.

(also note that I think the  "for once cycle" that ends that phrase in
the original changelog is redundant, since it already says a "1 sysclk
cycle window".

Kevin

[PATCH v1 2/3] PM / AVS: SmartReflex: disable runtime PM on driver remove

[Andrii Tseglytskyi]

Runtime PM should be disabled for device on driver remove,
otherwise runtime PM will be not balanced, and this will cause
an error message, on next driver probe.

Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@ti.com>
Acked-by: Nishanth Menon <nm@ti.com>
---
 drivers/power/avs/smartreflex.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
index f34d34d..9b56648 100644
--- a/drivers/power/avs/smartreflex.c
+++ b/drivers/power/avs/smartreflex.c
@@ -1032,6 +1032,7 @@ static int omap_sr_remove(struct platform_device *pdev)
 	if (sr_info->dbg_dir)
 		debugfs_remove_recursive(sr_info->dbg_dir);
 
+	pm_runtime_disable(&pdev->dev);
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
 	kfree(sr_info->name);
-- 
1.7.9.5

[PATCH v1 3/3] PM / AVS: SmartReflex: fix driver name

[Andrii Tseglytskyi]

DRIVER_NAME was undefined for SmartReflex. Now it is
defined with valid value "smartreflex". It is needed
to define proper value for:
MODULE_ALIAS("platform:" DRIVER_NAME);

Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@ti.com>
Acked-by: Nishanth Menon <nm@ti.com>
---
 drivers/power/avs/smartreflex.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
index 9b56648..002005e 100644
--- a/drivers/power/avs/smartreflex.c
+++ b/drivers/power/avs/smartreflex.c
@@ -27,6 +27,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/power/smartreflex.h>
 
+#define DRIVER_NAME	"smartreflex"
 #define SMARTREFLEX_NAME_LEN	16
 #define NVALUE_NAME_LEN		40
 #define SR_DISABLE_TIMEOUT	200
@@ -1070,7 +1071,7 @@ static struct platform_driver smartreflex_driver = {
 	.remove         = omap_sr_remove,
 	.shutdown	= omap_sr_shutdown,
 	.driver		= {
-		.name	= "smartreflex",
+		.name	= DRIVER_NAME,
 	},
 };
 
-- 
1.7.9.5

Re: [PATCH v1 0/3] PM / AVS: SmartReflex: driver misc fixes

[Kevin Hilman]

Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:

> The following patch series contain several misc fixes to SmartReflex driver:
>
> 1. disable errgen before vpbound disable. Critical fix, needed for
> proper work of AVS-VP communicaton protocol.
>
> 2. disable runtime PM on driver remove. Trivial - runtime PM cleanup.
>
> 3. fix driver name. Trivial - proper DRIVER_NAME was not defined
> since SmartReflex driver was introduced with initial commit.
>
> Patches are based on:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> tag: v3.10-rc2
>
> Verified on OMAP4430. Boot - OK. SmartReflex registers debug dump - OK
>
> Available on GitHub:
> https://github.com/andriit/linux-omap-k3.8/commits/avs_sr_driver_misc_fixes_v01
>
> Andrii Tseglytskyi (2):
>   PM / AVS: SmartReflex: disable runtime PM on driver remove
>   PM / AVS: SmartReflex: fix driver name
>
> Nishanth Menon (1):
>   PM / AVS: SmartReflex: disable errgen before vpbound disable
>
>  drivers/power/avs/smartreflex.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

I had a minor comment on PATCH 1/3, otherwise these look good for v3.11
(since they are not regressions, they don't qualify for v3.10.)

Please repost with the changes and be sure to copy linux-pm@vger.kernel.org

Thanks,

Kevin

Re: [PATCH v1 0/3] PM / AVS: SmartReflex: driver misc fixes

[Andrii Tseglytskyi]

On 05/29/2013 12:03 AM, Kevin Hilman wrote:
> Andrii Tseglytskyi <andrii.tseglytskyi@ti.com> writes:
>
>> The following patch series contain several misc fixes to SmartReflex driver:
>>
>> 1. disable errgen before vpbound disable. Critical fix, needed for
>> proper work of AVS-VP communicaton protocol.
>>
>> 2. disable runtime PM on driver remove. Trivial - runtime PM cleanup.
>>
>> 3. fix driver name. Trivial - proper DRIVER_NAME was not defined
>> since SmartReflex driver was introduced with initial commit.
>>
>> Patches are based on:
>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> tag: v3.10-rc2
>>
>> Verified on OMAP4430. Boot - OK. SmartReflex registers debug dump - OK
>>
>> Available on GitHub:
>> https://github.com/andriit/linux-omap-k3.8/commits/avs_sr_driver_misc_fixes_v01
>>
>> Andrii Tseglytskyi (2):
>>    PM / AVS: SmartReflex: disable runtime PM on driver remove
>>    PM / AVS: SmartReflex: fix driver name
>>
>> Nishanth Menon (1):
>>    PM / AVS: SmartReflex: disable errgen before vpbound disable
>>
>>   drivers/power/avs/smartreflex.c |   15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
> I had a minor comment on PATCH 1/3, otherwise these look good for v3.11
> (since they are not regressions, they don't qualify for v3.10.)
>
> Please repost with the changes and be sure to copy linux-pm@vger.kernel.org

OK. Will Cc to linux-pm@vger.kernel.org in next patch version.
Thank you for review.

>
> Thanks,
>
> Kevin
>