diff mbox

[2/3] ARM: OMAP: hwmod: revise deassert sequence

Message ID 1342466485-1050-3-git-send-email-omar.luna@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Omar Ramirez Luna July 16, 2012, 7:21 p.m. UTC
For a reset sequence to complete cleanly, a module needs its
associated clocks to be enabled, otherwise the timeout check
in prcm code can print a false failure (failed to hardreset)
that occurs because the clocks aren't powered ON and the status
bit checked can't transition without them.

Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
---
 arch/arm/mach-omap2/omap_hwmod.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

Comments

Paul Walmsley Aug. 2, 2012, 7:52 a.m. UTC | #1
Hello OMar,

On Mon, 16 Jul 2012, Omar Ramirez Luna wrote:

> For a reset sequence to complete cleanly, a module needs its
> associated clocks to be enabled, otherwise the timeout check
> in prcm code can print a false failure (failed to hardreset)
> that occurs because the clocks aren't powered ON and the status
> bit checked can't transition without them.
> 
> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>

Is enabling the clocks sufficient?  Or do we also need to enable the 
IP block, e.g. by calling

	if (soc_ops.enable_module)
		soc_ops.enable_module(oh);

as we do on OMAP4+ in _enable() ?


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Ramirez Luna Aug. 2, 2012, 10:20 p.m. UTC | #2
Hi.

On 2 August 2012 02:52, Paul Walmsley <paul@pwsan.com> wrote:
> On Mon, 16 Jul 2012, Omar Ramirez Luna wrote:
>
>> For a reset sequence to complete cleanly, a module needs its
>> associated clocks to be enabled, otherwise the timeout check
>> in prcm code can print a false failure (failed to hardreset)
>> that occurs because the clocks aren't powered ON and the status
>> bit checked can't transition without them.
>>
>> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
>
> Is enabling the clocks sufficient?

During my testing it seemed enough, besides it looks clk framework is
doing the same as _omap4_enable_module.

> Or do we also need to enable the
> IP block, e.g. by calling
>
>         if (soc_ops.enable_module)
>                 soc_ops.enable_module(oh);
>
> as we do on OMAP4+ in _enable() ?

Basically this is a call to _omap4_enable_module, and the latter will
"Enable the modulemode inside CLKCTRL".

However, _enable_clocks path which ends calling omap2_dflt_clk_enable
does the same thing with its clk->enable_reg field.

So in _enable:

        _enable_clocks(oh);
        if (soc_ops.enable_module)
                soc_ops.enable_module(oh);

The enable_module part seems redundant to me, since the module should
be already enabled by the first call to _enable_clocks.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath Aug. 3, 2012, 5:24 a.m. UTC | #3
On 8/3/2012 3:50 AM, Omar Ramirez Luna wrote:
> Hi.
> 
> On 2 August 2012 02:52, Paul Walmsley <paul@pwsan.com> wrote:
>> On Mon, 16 Jul 2012, Omar Ramirez Luna wrote:
>>
>>> For a reset sequence to complete cleanly, a module needs its
>>> associated clocks to be enabled, otherwise the timeout check
>>> in prcm code can print a false failure (failed to hardreset)
>>> that occurs because the clocks aren't powered ON and the status
>>> bit checked can't transition without them.
>>>
>>> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
>>
>> Is enabling the clocks sufficient?
> 
> During my testing it seemed enough, besides it looks clk framework is
> doing the same as _omap4_enable_module.
> 
>> Or do we also need to enable the
>> IP block, e.g. by calling
>>
>>         if (soc_ops.enable_module)
>>                 soc_ops.enable_module(oh);
>>
>> as we do on OMAP4+ in _enable() ?
> 
> Basically this is a call to _omap4_enable_module, and the latter will
> "Enable the modulemode inside CLKCTRL".
> 
> However, _enable_clocks path which ends calling omap2_dflt_clk_enable
> does the same thing with its clk->enable_reg field.
> 
> So in _enable:
> 
>         _enable_clocks(oh);
>         if (soc_ops.enable_module)
>                 soc_ops.enable_module(oh);
> 
> The enable_module part seems redundant to me, since the module should
> be already enabled by the first call to _enable_clocks.
> 

Yes they do same thing, I believe the plan is to get rid of all clock
leaf-nodes in the near future, and let hwmod handle module
enable/disable part.

Thanks,
Vaibhav

> Regards,
> 
> Omar
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Ramirez Luna Aug. 3, 2012, 3:52 p.m. UTC | #4
On 3 August 2012 00:24, Vaibhav Hiremath <hvaibhav@ti.com> wrote:
> On 8/3/2012 3:50 AM, Omar Ramirez Luna wrote:
>> So in _enable:
>>
>>         _enable_clocks(oh);
>>         if (soc_ops.enable_module)
>>                 soc_ops.enable_module(oh);
>>
>> The enable_module part seems redundant to me, since the module should
>> be already enabled by the first call to _enable_clocks.
>
> Yes they do same thing, I believe the plan is to get rid of all clock
> leaf-nodes in the near future, and let hwmod handle module
> enable/disable part.

If this is the case then an enable_module call is needed in my patch
for when these changes are made. The original works fine but only
because currently clock framework does what enable_module is doing.

Paul,

Please let me know if you want me to resend with this change.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benoit Cousson Aug. 20, 2012, 10:21 a.m. UTC | #5
Hi Omar,

On 08/03/2012 05:52 PM, Omar Ramirez Luna wrote:
> On 3 August 2012 00:24, Vaibhav Hiremath <hvaibhav@ti.com> wrote:
>> On 8/3/2012 3:50 AM, Omar Ramirez Luna wrote:
>>> So in _enable:
>>>
>>>         _enable_clocks(oh);
>>>         if (soc_ops.enable_module)
>>>                 soc_ops.enable_module(oh);
>>>
>>> The enable_module part seems redundant to me, since the module should
>>> be already enabled by the first call to _enable_clocks.
>>
>> Yes they do same thing, I believe the plan is to get rid of all clock
>> leaf-nodes in the near future, and let hwmod handle module
>> enable/disable part.
> 
> If this is the case then an enable_module call is needed in my patch
> for when these changes are made. The original works fine but only
> because currently clock framework does what enable_module is doing.

Yes, that's the case, but I plan to remove most of the leaf clocks ASAP,
so we cannot rely on that.

> Please let me know if you want me to resend with this change.

Yes, could you please repost with that change?

It will be good as well that you remove the leaf clock and use the
parent clock of current leaf as the main_clock. In that case it will
ensure that this is the hwmod fmwk that does enable the modulemode and
not the clock fmwk.

Regards,
Benoit

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Ramirez Luna Aug. 21, 2012, 1:15 a.m. UTC | #6
Hi Benoit,

On 20 August 2012 05:21, Benoit Cousson <b-cousson@ti.com> wrote:
> Hi Omar,
>
> On 08/03/2012 05:52 PM, Omar Ramirez Luna wrote:
>> On 3 August 2012 00:24, Vaibhav Hiremath <hvaibhav@ti.com> wrote:
>>> On 8/3/2012 3:50 AM, Omar Ramirez Luna wrote:
>>>> So in _enable:
>>>>
>>>>         _enable_clocks(oh);
>>>>         if (soc_ops.enable_module)
>>>>                 soc_ops.enable_module(oh);
>>>>
>>>> The enable_module part seems redundant to me, since the module should
>>>> be already enabled by the first call to _enable_clocks.
>>>
>>> Yes they do same thing, I believe the plan is to get rid of all clock
>>> leaf-nodes in the near future, and let hwmod handle module
>>> enable/disable part.
>>
>> If this is the case then an enable_module call is needed in my patch
>> for when these changes are made. The original works fine but only
>> because currently clock framework does what enable_module is doing.
>
> Yes, that's the case, but I plan to remove most of the leaf clocks ASAP,
> so we cannot rely on that.
>
>> Please let me know if you want me to resend with this change.
>
> Yes, could you please repost with that change?

Not a problem.

> It will be good as well that you remove the leaf clock and use the
> parent clock of current leaf as the main_clock. In that case it will
> ensure that this is the hwmod fmwk that does enable the modulemode and
> not the clock fmwk.

Ok, let me try that.

Thanks for the comments,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 091c199..f6f8d99 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1509,6 +1509,7 @@  static int _deassert_hardreset(struct omap_hwmod *oh, const char *name)
 {
 	struct omap_hwmod_rst_info ohri;
 	int ret = -EINVAL;
+	int hwsup = 0;
 
 	if (!oh)
 		return -EINVAL;
@@ -1520,10 +1521,42 @@  static int _deassert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (IS_ERR_VALUE(ret))
 		return ret;
 
+	if (oh->clkdm) {
+		/*
+		 * A clockdomain must be in SW_SUP otherwise reset
+		 * might not be completed. The clockdomain can be set
+		 * in HW_AUTO only when the module become ready.
+		 */
+		hwsup = clkdm_in_hwsup(oh->clkdm);
+		ret = clkdm_hwmod_enable(oh->clkdm, oh);
+		if (ret) {
+			WARN(1, "omap_hwmod: %s: could not enable clockdomain %s: %d\n",
+			     oh->name, oh->clkdm->name, ret);
+			return ret;
+		}
+	}
+
+	_enable_clocks(oh);
+
 	ret = soc_ops.deassert_hardreset(oh, &ohri);
+
+	_disable_clocks(oh);
+
 	if (ret == -EBUSY)
 		pr_warning("omap_hwmod: %s: failed to hardreset\n", oh->name);
 
+	if (!ret) {
+		/*
+		 * Set the clockdomain to HW_AUTO, assuming that the
+		 * previous state was HW_AUTO.
+		 */
+		if (oh->clkdm && hwsup)
+			clkdm_allow_idle(oh->clkdm);
+	} else {
+		if (oh->clkdm)
+			clkdm_hwmod_disable(oh->clkdm, oh);
+	}
+
 	return ret;
 }