* Issue with _are_all_hardreset_lines_asserted()
@ 2012-10-04 13:15 Archit Taneja
2012-10-09 5:38 ` Paul Walmsley
0 siblings, 1 reply; 6+ messages in thread
From: Archit Taneja @ 2012-10-04 13:15 UTC (permalink / raw)
To: Omar Ramirez Luna, paul@pwsan.com
Cc: linux-omap@vger.kernel.org, Rajendra Nayak
Hi,
I was trying out the linux-next kernel, and I noticed that DSS
MODULEMODE bits are never cleared.
In _omap4_disable_module(), there is a check:
...
if (!_are_all_hardreset_lines_asserted(oh))
return 0;
/* MODULEMODE bits cleared here */
...
...
...
The function _are_all_hardreset_lines_asserted() returns false if
'oh->rst_lines_cnt == 0', so we bail out from _omap4_disable_module()
before clearing the MODULEMODE bits.
Is this correct behavior? This would prevent all hwmods who have
rst_lines_cnt as 0 to not get their MODULEMODE bits cleared.
Thanks,
Archit
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Issue with _are_all_hardreset_lines_asserted()
2012-10-04 13:15 Issue with _are_all_hardreset_lines_asserted() Archit Taneja
@ 2012-10-09 5:38 ` Paul Walmsley
2012-10-09 6:43 ` Archit Taneja
2012-10-10 2:21 ` Omar Ramirez Luna
0 siblings, 2 replies; 6+ messages in thread
From: Paul Walmsley @ 2012-10-09 5:38 UTC (permalink / raw)
To: Archit Taneja
Cc: Omar Ramirez Luna, hvaibhav, linux-omap@vger.kernel.org,
Rajendra Nayak
cc Vaibhav due to the AM33xx change
Hi
On Thu, 4 Oct 2012, Archit Taneja wrote:
> I was trying out the linux-next kernel, and I noticed that DSS MODULEMODE bits
> are never cleared.
>
> In _omap4_disable_module(), there is a check:
>
> ...
>
> if (!_are_all_hardreset_lines_asserted(oh))
> return 0;
>
> /* MODULEMODE bits cleared here */
> ...
> ...
> ...
>
> The function _are_all_hardreset_lines_asserted() returns false if
> 'oh->rst_lines_cnt == 0', so we bail out from _omap4_disable_module() before
> clearing the MODULEMODE bits.
>
> Is this correct behavior? This would prevent all hwmods who have rst_lines_cnt
> as 0 to not get their MODULEMODE bits cleared.
You're right, that looks bogus. What do you think about the following
patch?
- Paul
From: Paul Walmsley <paul@pwsan.com>
Date: Mon, 8 Oct 2012 23:08:15 -0600
Subject: [PATCH] ARM: OMAP4/AM335x: hwmod: fix disable_module regression in
hardreset handling
Commit eb05f691290e99ee0bd1672317d6add789523c1e ("ARM: OMAP: hwmod:
partially un-reset hwmods might not be properly enabled") added code
to skip the IP block disable sequence if any hardreset lines were left
unasserted. But this did not handle the case when no hardreset lines
were associated with a module, which is the general case. In that
situation, the IP block would be skipped, which is likely to cause PM
regressions.
So, modify _omap4_disable_module() and _am33xx_disable_module() to
only bail out early if there are any hardreset lines asserted. And
move the AM33xx test above the actual module disable code to ensure
that the behavior is consistent.
Reported-by: Archit Taneja <a0393947@ti.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 299ca28..b969ab1 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1698,6 +1698,29 @@ static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
}
/**
+ * _are_any_hardreset_lines_asserted - return true if any part of @oh is
+ * hard-reset
+ * @oh: struct omap_hwmod *
+ *
+ * If any hardreset lines associated with @oh are asserted, then
+ * return true. Otherwise, if no hardreset lines associated with @oh
+ * are asserted, or if @oh has no hardreset lines, then return false.
+ * This function is used to avoid executing some parts of the IP block
+ * enable/disable sequence if any hardreset line is set.
+ */
+static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
+{
+ int rst_cnt = 0;
+ int i;
+
+ for (i = 0; i < oh->rst_lines_cnt && rst_cnt == 0; i++)
+ if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
+ rst_cnt++;
+
+ return (rst_cnt) ? true : false;
+}
+
+/**
* _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
* @oh: struct omap_hwmod *
*
@@ -1715,7 +1738,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
* Since integration code might still be doing something, only
* disable if all lines are under hardreset.
*/
- if (!_are_all_hardreset_lines_asserted(oh))
+ if (_are_any_hardreset_lines_asserted(oh))
return 0;
pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
@@ -1749,12 +1772,12 @@ static int _am33xx_disable_module(struct omap_hwmod *oh)
pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
+ if (_are_any_hardreset_lines_asserted(oh))
+ return 0;
+
am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
oh->prcm.omap4.clkctrl_offs);
- if (_are_all_hardreset_lines_asserted(oh))
- return 0;
-
v = _am33xx_wait_target_disable(oh);
if (v)
pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Issue with _are_all_hardreset_lines_asserted()
2012-10-09 5:38 ` Paul Walmsley
@ 2012-10-09 6:43 ` Archit Taneja
2012-10-09 6:59 ` Paul Walmsley
2012-10-10 2:21 ` Omar Ramirez Luna
1 sibling, 1 reply; 6+ messages in thread
From: Archit Taneja @ 2012-10-09 6:43 UTC (permalink / raw)
To: Paul Walmsley
Cc: Omar Ramirez Luna, hvaibhav, linux-omap@vger.kernel.org,
Rajendra Nayak
Hi,
On Tuesday 09 October 2012 11:08 AM, Paul Walmsley wrote:
> cc Vaibhav due to the AM33xx change
>
> Hi
>
> On Thu, 4 Oct 2012, Archit Taneja wrote:
>
>> I was trying out the linux-next kernel, and I noticed that DSS MODULEMODE bits
>> are never cleared.
>>
>> In _omap4_disable_module(), there is a check:
>>
>> ...
>>
>> if (!_are_all_hardreset_lines_asserted(oh))
>> return 0;
>>
>> /* MODULEMODE bits cleared here */
>> ...
>> ...
>> ...
>>
>> The function _are_all_hardreset_lines_asserted() returns false if
>> 'oh->rst_lines_cnt == 0', so we bail out from _omap4_disable_module() before
>> clearing the MODULEMODE bits.
>>
>> Is this correct behavior? This would prevent all hwmods who have rst_lines_cnt
>> as 0 to not get their MODULEMODE bits cleared.
>
> You're right, that looks bogus. What do you think about the following
> patch?
>
The patch looks fine to me. I tried it out for DSS(with an additional
patch to not represent DSS modulemode bits as a slave clock), and
modulemode is getting disabled correctly now.
Thanks,
Archit
>
> - Paul
>
> From: Paul Walmsley <paul@pwsan.com>
> Date: Mon, 8 Oct 2012 23:08:15 -0600
> Subject: [PATCH] ARM: OMAP4/AM335x: hwmod: fix disable_module regression in
> hardreset handling
>
> Commit eb05f691290e99ee0bd1672317d6add789523c1e ("ARM: OMAP: hwmod:
> partially un-reset hwmods might not be properly enabled") added code
> to skip the IP block disable sequence if any hardreset lines were left
> unasserted. But this did not handle the case when no hardreset lines
> were associated with a module, which is the general case. In that
> situation, the IP block would be skipped, which is likely to cause PM
> regressions.
>
> So, modify _omap4_disable_module() and _am33xx_disable_module() to
> only bail out early if there are any hardreset lines asserted. And
> move the AM33xx test above the actual module disable code to ensure
> that the behavior is consistent.
>
> Reported-by: Archit Taneja <a0393947@ti.com>
> Cc: Omar Ramirez Luna <omar.luna@linaro.org>
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
> arch/arm/mach-omap2/omap_hwmod.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 299ca28..b969ab1 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1698,6 +1698,29 @@ static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
> }
>
> /**
> + * _are_any_hardreset_lines_asserted - return true if any part of @oh is
> + * hard-reset
> + * @oh: struct omap_hwmod *
> + *
> + * If any hardreset lines associated with @oh are asserted, then
> + * return true. Otherwise, if no hardreset lines associated with @oh
> + * are asserted, or if @oh has no hardreset lines, then return false.
> + * This function is used to avoid executing some parts of the IP block
> + * enable/disable sequence if any hardreset line is set.
> + */
> +static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
> +{
> + int rst_cnt = 0;
> + int i;
> +
> + for (i = 0; i < oh->rst_lines_cnt && rst_cnt == 0; i++)
> + if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
> + rst_cnt++;
> +
> + return (rst_cnt) ? true : false;
> +}
> +
> +/**
> * _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
> * @oh: struct omap_hwmod *
> *
> @@ -1715,7 +1738,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
> * Since integration code might still be doing something, only
> * disable if all lines are under hardreset.
> */
> - if (!_are_all_hardreset_lines_asserted(oh))
> + if (_are_any_hardreset_lines_asserted(oh))
> return 0;
>
> pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
> @@ -1749,12 +1772,12 @@ static int _am33xx_disable_module(struct omap_hwmod *oh)
>
> pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
>
> + if (_are_any_hardreset_lines_asserted(oh))
> + return 0;
> +
> am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
> oh->prcm.omap4.clkctrl_offs);
>
> - if (_are_all_hardreset_lines_asserted(oh))
> - return 0;
> -
> v = _am33xx_wait_target_disable(oh);
> if (v)
> pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Issue with _are_all_hardreset_lines_asserted()
2012-10-09 6:43 ` Archit Taneja
@ 2012-10-09 6:59 ` Paul Walmsley
2012-10-09 8:38 ` Hiremath, Vaibhav
0 siblings, 1 reply; 6+ messages in thread
From: Paul Walmsley @ 2012-10-09 6:59 UTC (permalink / raw)
To: Archit Taneja
Cc: Omar Ramirez Luna, hvaibhav, linux-omap@vger.kernel.org,
Rajendra Nayak
On Tue, 9 Oct 2012, Archit Taneja wrote:
> The patch looks fine to me. I tried it out for DSS(with an additional patch to
> not represent DSS modulemode bits as a slave clock), and modulemode is getting
> disabled correctly now.
Thanks, I added your Tested-by: and also updated the patch description
which was a little unclear.
- Paul
From: Paul Walmsley <paul@pwsan.com>
Date: Mon, 8 Oct 2012 23:08:15 -0600
Subject: [PATCH] ARM: OMAP4/AM335x: hwmod: fix disable_module regression in
hardreset handling
Commit eb05f691290e99ee0bd1672317d6add789523c1e ("ARM: OMAP: hwmod:
partially un-reset hwmods might not be properly enabled") added code
to skip the IP block disable sequence if all of the block's hardreset
lines weren't asserted. But this did not handle the case when no
hardreset lines were associated with a module, which is the general
case. In that situation, the IP block disable would be skipped. This
is likely to cause PM regressions.
So, modify _omap4_disable_module() and _am33xx_disable_module() to
only bail out early if there are any hardreset lines asserted. And
move the AM33xx test above the actual module disable code to ensure
that the behavior is consistent.
Reported-by: Archit Taneja <a0393947@ti.com>
Tested-by: Archit Taneja <a0393947@ti.com> # DSS
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 299ca28..b969ab1 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1698,6 +1698,29 @@ static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
}
/**
+ * _are_any_hardreset_lines_asserted - return true if any part of @oh is
+ * hard-reset
+ * @oh: struct omap_hwmod *
+ *
+ * If any hardreset lines associated with @oh are asserted, then
+ * return true. Otherwise, if no hardreset lines associated with @oh
+ * are asserted, or if @oh has no hardreset lines, then return false.
+ * This function is used to avoid executing some parts of the IP block
+ * enable/disable sequence if any hardreset line is set.
+ */
+static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
+{
+ int rst_cnt = 0;
+ int i;
+
+ for (i = 0; i < oh->rst_lines_cnt && rst_cnt == 0; i++)
+ if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
+ rst_cnt++;
+
+ return (rst_cnt) ? true : false;
+}
+
+/**
* _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
* @oh: struct omap_hwmod *
*
@@ -1715,7 +1738,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
* Since integration code might still be doing something, only
* disable if all lines are under hardreset.
*/
- if (!_are_all_hardreset_lines_asserted(oh))
+ if (_are_any_hardreset_lines_asserted(oh))
return 0;
pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
@@ -1749,12 +1772,12 @@ static int _am33xx_disable_module(struct omap_hwmod *oh)
pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
+ if (_are_any_hardreset_lines_asserted(oh))
+ return 0;
+
am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
oh->prcm.omap4.clkctrl_offs);
- if (_are_all_hardreset_lines_asserted(oh))
- return 0;
-
v = _am33xx_wait_target_disable(oh);
if (v)
pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: Issue with _are_all_hardreset_lines_asserted()
2012-10-09 6:59 ` Paul Walmsley
@ 2012-10-09 8:38 ` Hiremath, Vaibhav
0 siblings, 0 replies; 6+ messages in thread
From: Hiremath, Vaibhav @ 2012-10-09 8:38 UTC (permalink / raw)
To: Paul Walmsley, Taneja, Archit
Cc: Omar Ramirez Luna, linux-omap@vger.kernel.org, Nayak, Rajendra
On Tue, Oct 09, 2012 at 12:29:53, Paul Walmsley wrote:
> On Tue, 9 Oct 2012, Archit Taneja wrote:
>
> > The patch looks fine to me. I tried it out for DSS(with an additional patch to
> > not represent DSS modulemode bits as a slave clock), and modulemode is getting
> > disabled correctly now.
>
> Thanks, I added your Tested-by: and also updated the patch description
> which was a little unclear.
>
>
> - Paul
>
> From: Paul Walmsley <paul@pwsan.com>
> Date: Mon, 8 Oct 2012 23:08:15 -0600
> Subject: [PATCH] ARM: OMAP4/AM335x: hwmod: fix disable_module regression in
> hardreset handling
>
> Commit eb05f691290e99ee0bd1672317d6add789523c1e ("ARM: OMAP: hwmod:
> partially un-reset hwmods might not be properly enabled") added code
> to skip the IP block disable sequence if all of the block's hardreset
> lines weren't asserted. But this did not handle the case when no
> hardreset lines were associated with a module, which is the general
> case. In that situation, the IP block disable would be skipped. This
> is likely to cause PM regressions.
>
> So, modify _omap4_disable_module() and _am33xx_disable_module() to
> only bail out early if there are any hardreset lines asserted. And
> move the AM33xx test above the actual module disable code to ensure
> that the behavior is consistent.
>
Paul,
I just tested it on Bone + one gpmc fix (will submit shortly) and it boots
up fine for me. I have also tested for module disable, and it works.
Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>
Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
Log for reference -
[ 0.749504] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 84, v - 2
[ 0.749630] omap_hwmod: timer3: _am33xx_disable_module
[ 0.749652] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 84, v - 30000
[ 0.749819] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 88, v - 30002
[ 0.749841] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 88, v - 2
[ 0.749904] omap_hwmod: timer4: _am33xx_disable_module
[ 0.749923] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 88, v - 30000
[ 0.750131] _clkctrl_idlest:109 inst - 0, clkctrl_offs - ec, v - 10002
[ 0.750152] _clkctrl_idlest:109 inst - 0, clkctrl_offs - ec, v - 2
[ 0.750218] omap_hwmod: timer5: _am33xx_disable_module
[ 0.750236] _clkctrl_idlest:109 inst - 0, clkctrl_offs - ec, v - 10000
[ 0.750253] _clkctrl_idlest:109 inst - 0, clkctrl_offs - ec, v - 30000
[ 0.750404] _clkctrl_idlest:109 inst - 0, clkctrl_offs - f0, v - 30002
[ 0.750423] _clkctrl_idlest:109 inst - 0, clkctrl_offs - f0, v - 2
[ 0.750485] omap_hwmod: timer6: _am33xx_disable_module
[ 0.750504] _clkctrl_idlest:109 inst - 0, clkctrl_offs - f0, v - 10000
[ 0.750520] _clkctrl_idlest:109 inst - 0, clkctrl_offs - f0, v - 30000
[ 0.750666] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 7c, v - 30002
[ 0.750685] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 7c, v - 2
[ 0.750747] omap_hwmod: timer7: _am33xx_disable_module
[ 0.750765] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 7c, v - 10000
[ 0.750782] _clkctrl_idlest:109 inst - 0, clkctrl_offs - 7c, v - 30000
Thanks,
Vaibhav
> Reported-by: Archit Taneja <a0393947@ti.com>
> Tested-by: Archit Taneja <a0393947@ti.com> # DSS
> Cc: Omar Ramirez Luna <omar.luna@linaro.org>
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
> arch/arm/mach-omap2/omap_hwmod.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 299ca28..b969ab1 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1698,6 +1698,29 @@ static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
> }
>
> /**
> + * _are_any_hardreset_lines_asserted - return true if any part of @oh is
> + * hard-reset
> + * @oh: struct omap_hwmod *
> + *
> + * If any hardreset lines associated with @oh are asserted, then
> + * return true. Otherwise, if no hardreset lines associated with @oh
> + * are asserted, or if @oh has no hardreset lines, then return false.
> + * This function is used to avoid executing some parts of the IP block
> + * enable/disable sequence if any hardreset line is set.
> + */
> +static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
> +{
> + int rst_cnt = 0;
> + int i;
> +
> + for (i = 0; i < oh->rst_lines_cnt && rst_cnt == 0; i++)
> + if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
> + rst_cnt++;
> +
> + return (rst_cnt) ? true : false;
> +}
> +
> +/**
> * _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
> * @oh: struct omap_hwmod *
> *
> @@ -1715,7 +1738,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
> * Since integration code might still be doing something, only
> * disable if all lines are under hardreset.
> */
> - if (!_are_all_hardreset_lines_asserted(oh))
> + if (_are_any_hardreset_lines_asserted(oh))
> return 0;
>
> pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
> @@ -1749,12 +1772,12 @@ static int _am33xx_disable_module(struct omap_hwmod *oh)
>
> pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
>
> + if (_are_any_hardreset_lines_asserted(oh))
> + return 0;
> +
> am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
> oh->prcm.omap4.clkctrl_offs);
>
> - if (_are_all_hardreset_lines_asserted(oh))
> - return 0;
> -
> v = _am33xx_wait_target_disable(oh);
> if (v)
> pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> --
> 1.7.10.4
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Issue with _are_all_hardreset_lines_asserted()
2012-10-09 5:38 ` Paul Walmsley
2012-10-09 6:43 ` Archit Taneja
@ 2012-10-10 2:21 ` Omar Ramirez Luna
1 sibling, 0 replies; 6+ messages in thread
From: Omar Ramirez Luna @ 2012-10-10 2:21 UTC (permalink / raw)
To: Paul Walmsley
Cc: Archit Taneja, hvaibhav, linux-omap@vger.kernel.org,
Rajendra Nayak
Hi,
On 9 October 2012 00:38, Paul Walmsley <paul@pwsan.com> wrote:
> cc Vaibhav due to the AM33xx change
>
> Hi
>
> On Thu, 4 Oct 2012, Archit Taneja wrote:
>
>> I was trying out the linux-next kernel, and I noticed that DSS MODULEMODE bits
>> are never cleared.
>>
>> In _omap4_disable_module(), there is a check:
>>
>> ...
>>
>> if (!_are_all_hardreset_lines_asserted(oh))
>> return 0;
>>
>> /* MODULEMODE bits cleared here */
>> ...
>> ...
>> ...
>>
>> The function _are_all_hardreset_lines_asserted() returns false if
>> 'oh->rst_lines_cnt == 0', so we bail out from _omap4_disable_module() before
>> clearing the MODULEMODE bits.
>>
>> Is this correct behavior? This would prevent all hwmods who have rst_lines_cnt
>> as 0 to not get their MODULEMODE bits cleared.
>
> You're right, that looks bogus. What do you think about the following
> patch?
I was a little busy creating the patch but seems like Paul beat me to
it, yes it was a wrong case on disable_module.
> From: Paul Walmsley <paul@pwsan.com>
> Date: Mon, 8 Oct 2012 23:08:15 -0600
> Subject: [PATCH] ARM: OMAP4/AM335x: hwmod: fix disable_module regression in
> hardreset handling
>
> Commit eb05f691290e99ee0bd1672317d6add789523c1e ("ARM: OMAP: hwmod:
> partially un-reset hwmods might not be properly enabled") added code
> to skip the IP block disable sequence if any hardreset lines were left
> unasserted. But this did not handle the case when no hardreset lines
> were associated with a module, which is the general case. In that
> situation, the IP block would be skipped, which is likely to cause PM
> regressions.
>
> So, modify _omap4_disable_module() and _am33xx_disable_module() to
> only bail out early if there are any hardreset lines asserted. And
> move the AM33xx test above the actual module disable code to ensure
> that the behavior is consistent.
I think that on _omap4_disable_module() we want to check if the module
is deasserted rather than if it is asserted.
E.g.: If you have 2 hwmods for the same module (ipu with reset "cpu0"
and mmu_ipu with reset "mmu") controlled by different drivers,
depending on which one is idled first, the other one will cause L3
aborts given that the module is already disabled. So, if ipu is idled
and disabled first, then all tear down operations on iommu will cause
L3 aborts.
I created the following:
From: Omar Ramirez Luna <omar.luna@linaro.org>
Subject: [PATCH] ARM: OMAP: hwmod: allow hwmods without reset lines to be
disabled
_are_all_hardreset_lines_asserted treats hwmods without reset lines
as always deasserted, this is correct for: _enable, _idle and
_shutdown paths, however for _omap4_disable_module it might not
result in the correct behavior, given that:
- hwmod without reset line can be safely disabled.
- hwmod with 1 or more reset lines, must be completely asserted
before disabled or be left to integration code to handle it.
Create a function to check for any hwmod to be partially out of
hardreset and bail of _omap4_disable_module if this is the case.
Hwmods without reset lines can skip these check and continue.
Reported-by: Archit Taneja <a0393947@ti.com>
Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
---
arch/arm/mach-omap2/omap_hwmod.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 299ca28..f9d9bfb 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1698,6 +1698,29 @@ static bool
_are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
}
/**
+ * _are_any_hardreset_lines_deasserted - true if part of @oh isn't hard-reset
+ * @oh: struct omap_hwmod *
+ *
+ * If any hardreset line associated with @oh is deasserted, then return true.
+ * Otherwise, if no hardreset lines associated with @oh are deasserted,
+ * then return false. This function is used to avoid SOC's disable_module to
+ * be called while the @oh is still deasserted.
+ *
+ * Check for oh->rst_lines_cnt is left to the caller, since a return value
+ * could mean that a hwmod with no reset lines is deasserted.
+ */
+static bool _are_any_hardreset_lines_deasserted(struct omap_hwmod *oh)
+{
+ int i;
+
+ for (i = 0; i < oh->rst_lines_cnt; i++)
+ if (_read_hardreset(oh, oh->rst_lines[i].name) == 0)
+ return true;
+
+ return false;
+}
+
+/**
* _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
* @oh: struct omap_hwmod *
*
@@ -1713,9 +1736,12 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
/*
* Since integration code might still be doing something, only
- * disable if all lines are under hardreset.
+ * disable if all lines are under hardreset. Explicitly check for
+ * reset lines before the call, otherwise the abscence of a reset
+ * line is assumed as a deasserted hwmod and won't execute the
+ * disable sequence.
*/
- if (!_are_all_hardreset_lines_asserted(oh))
+ if (oh->rst_lines_cnt && _are_any_hardreset_lines_deasserted(oh))
return 0;
pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-10 2:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-04 13:15 Issue with _are_all_hardreset_lines_asserted() Archit Taneja
2012-10-09 5:38 ` Paul Walmsley
2012-10-09 6:43 ` Archit Taneja
2012-10-09 6:59 ` Paul Walmsley
2012-10-09 8:38 ` Hiremath, Vaibhav
2012-10-10 2:21 ` Omar Ramirez Luna
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox