Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
Hi Kevin, Thanks a lot for your comments. On 05/28/2013 09:45 PM, Kevin Hilman wrote: Andrii Tseglytskyi andrii.tseglyts...@ti.com writes: From: Nishanth Menon n...@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-b...@ti.com Signed-off-by: Leonardo Affortunati l-affortun...@ti.com Signed-off-by: Nishanth Menon n...@ti.com Signed-off-by: Andrii.Tseglytskyi andrii.tseglyts...@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; } -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
Andrii Tseglytskyi andrii.tseglyts...@ti.com writes: [...] Signed-off-by: Vincent Bour v-b...@ti.com Signed-off-by: Leonardo Affortunati l-affortun...@ti.com Signed-off-by: Nishanth Menon n...@ti.com Signed-off-by: Andrii.Tseglytskyi andrii.tseglyts...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
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.tseglyts...@ti.com writes: From: Nishanth Menon n...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
Andrii Tseglytskyi andrii.tseglyts...@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.tseglyts...@ti.com writes: From: Nishanth Menon n...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
Andrii Tseglytskyi andrii.tseglyts...@ti.com writes: From: Nishanth Menon n...@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-b...@ti.com Signed-off-by: Leonardo Affortunati l-affortun...@ti.com Signed-off-by: Nishanth Menon n...@ti.com Signed-off-by: Andrii.Tseglytskyi andrii.tseglyts...@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; } -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1 1/3] PM / AVS: SmartReflex: disable errgen before vpbound disable
From: Nishanth Menon n...@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-b...@ti.com Signed-off-by: Leonardo Affortunati l-affortun...@ti.com Signed-off-by: Nishanth Menon n...@ti.com Signed-off-by: Andrii.Tseglytskyi andrii.tseglyts...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
