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
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 <[email protected]>
Acked-by: Nishanth Menon <[email protected]>
---
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
From: Nishanth Menon <[email protected]>
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 <[email protected]>
Signed-off-by: Leonardo Affortunati <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
Signed-off-by: Andrii.Tseglytskyi <[email protected]>
---
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
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 <[email protected]>
Acked-by: Nishanth Menon <[email protected]>
---
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
Andrii Tseglytskyi <[email protected]> writes:
> From: Nishanth Menon <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Leonardo Affortunati <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> Signed-off-by: Andrii.Tseglytskyi <[email protected]>
>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;
> }
Andrii Tseglytskyi <[email protected]> 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 [email protected]
Thanks,
Kevin
Hi Kevin,
Thanks a lot for your comments.
On 05/28/2013 09:45 PM, Kevin Hilman wrote:
> Andrii Tseglytskyi <[email protected]> writes:
>
>> From: Nishanth Menon <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Leonardo Affortunati <[email protected]>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> Signed-off-by: Andrii.Tseglytskyi <[email protected]>
> 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;
>> }
On 05/29/2013 12:03 AM, Kevin Hilman wrote:
> Andrii Tseglytskyi <[email protected]> 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 [email protected]
OK. Will Cc to [email protected] in next patch version.
Thank you for review.
>
> Thanks,
>
> Kevin
>
Andrii Tseglytskyi <[email protected]> writes:
[...]
>>> Signed-off-by: Vincent Bour <[email protected]>
>>> Signed-off-by: Leonardo Affortunati <[email protected]>
>>> Signed-off-by: Nishanth Menon <[email protected]>
>>> Signed-off-by: Andrii.Tseglytskyi <[email protected]>
>> 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
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 <[email protected]> writes:
>>
>>> From: Nishanth Menon <[email protected]>
>>>
>>> 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
Andrii Tseglytskyi <[email protected]> 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 <[email protected]> writes:
>>>
>>>> From: Nishanth Menon <[email protected]>
>>>>
>>>> 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