[PATCH 0/3] OMAP: hwmod: reset API proposal

[PATCH 0/3] OMAP: hwmod: reset API proposal

[Omar Ramirez Luna]

From: Omar Ramirez Luna <omar.ramirez@copitl.com>

***
RESEND: http://bit.ly/PZDbcc
Needed for ipu, dsp and their mmu reset lines.
***

Recent changes in omap_hwmod framework have reworked the behaviour
towards hardreset handling, commit 747834a (ARM: OMAP2+: hwmod:
revise hardreset behavior) recommends for drivers to implement
their own reset sequences until code out-of-tree hits mainline
and then their needs and code can be reviewed.

Since it is not clear when this will occur for all drivers and
hwmod code was not deleted (presumably because at some point it
will handle the resets once again), this series exports functions
to handle hardreset lines in an attempt to reduce code duplication
for those who have a common reset sequence.

These APIs are intended to be used by iommu for now, but were
tested with IPU and remoteproc on Pandaboard.

Omar Ramirez Luna (3):
  ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
    enabled
  ARM: OMAP: hwmod: revise deassert sequence
  ARM: OMAP: omap_device: expose hwmod assert/deassert to omap devices

 arch/arm/mach-omap2/omap_hwmod.c              |   70 +++++++++++++++++++-----
 arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
 arch/arm/plat-omap/omap_device.c              |   45 ++++++++++++++++
 3 files changed, 104 insertions(+), 15 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] ARM: OMAP: hwmod: partially un-reset hwmods might not be properly enabled

[Omar Ramirez Luna]

Some IP blocks might not be using/controlling more than one
reset line, this check loosens the restriction to fully use
hwmod framework for those drivers.

E.g.: ipu has reset lines: mmu_cache, cpu0 and cpu1.
- cpu1 might not be used and hence (with previous check)
  won't be fully enabled by hwmod code.

While at it, prevent _omap4_module_disable if all the hardreset
lines on an IP block are not under reset.

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

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 3215dad..091c199 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1558,25 +1558,28 @@ static int _read_hardreset(struct omap_hwmod *oh, const char *name)
 }
 
 /**
- * _are_any_hardreset_lines_asserted - return true if part of @oh is hard-reset
+ * _are_all_hardreset_lines_asserted - return true if the @oh is hard-reset
  * @oh: struct omap_hwmod *
  *
- * If any hardreset line associated with @oh is asserted, then return true.
- * Otherwise, if @oh has no hardreset lines associated with it, or if
- * no hardreset lines associated with @oh are asserted, then return false.
+ * If all hardreset lines associated with @oh are asserted, then return true.
+ * Otherwise, if part of @oh is out hardreset or if no hardreset lines
+ * associated with @oh are asserted, then return false.
  * This function is used to avoid executing some parts of the IP block
- * enable/disable sequence if a hardreset line is set.
+ * enable/disable sequence if its hardreset line is set.
  */
-static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
+static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
 {
-	int i;
+	int i, rst_cnt = 0;
 
 	if (oh->rst_lines_cnt == 0)
 		return false;
 
 	for (i = 0; i < oh->rst_lines_cnt; i++)
 		if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
-			return true;
+			rst_cnt++;
+
+	if (oh->rst_lines_cnt == rst_cnt)
+		return true;
 
 	return false;
 }
@@ -1595,6 +1598,13 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
 	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
 		return -EINVAL;
 
+	/*
+	 * Since integration code might still be doing something, only
+	 * disable if all lines are under hardreset.
+	 */
+	if (!_are_all_hardreset_lines_asserted(oh))
+		return 0;
+
 	pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
 
 	omap4_cminst_module_disable(oh->clkdm->prcm_partition,
@@ -1602,9 +1612,6 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
 				    oh->clkdm->clkdm_offs,
 				    oh->prcm.omap4.clkctrl_offs);
 
-	if (_are_any_hardreset_lines_asserted(oh))
-		return 0;
-
 	v = _omap4_wait_target_disable(oh);
 	if (v)
 		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
@@ -1830,7 +1837,7 @@ static int _enable(struct omap_hwmod *oh)
 	}
 
 	/*
-	 * If an IP block contains HW reset lines and any of them are
+	 * If an IP block contains HW reset lines and all of them are
 	 * asserted, we let integration code associated with that
 	 * block handle the enable.  We've received very little
 	 * information on what those driver authors need, and until
@@ -1838,7 +1845,7 @@ static int _enable(struct omap_hwmod *oh)
 	 * posted to the public lists, this is probably the best we
 	 * can do.
 	 */
-	if (_are_any_hardreset_lines_asserted(oh))
+	if (_are_all_hardreset_lines_asserted(oh))
 		return 0;
 
 	/* Mux pins for device runtime if populated */
@@ -1918,7 +1925,7 @@ static int _idle(struct omap_hwmod *oh)
 		return -EINVAL;
 	}
 
-	if (_are_any_hardreset_lines_asserted(oh))
+	if (_are_all_hardreset_lines_asserted(oh))
 		return 0;
 
 	if (oh->class->sysc)
@@ -2006,7 +2013,7 @@ static int _shutdown(struct omap_hwmod *oh)
 		return -EINVAL;
 	}
 
-	if (_are_any_hardreset_lines_asserted(oh))
+	if (_are_all_hardreset_lines_asserted(oh))
 		return 0;
 
 	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
-- 
1.7.4.1

[PATCH 1/3] ARM: OMAP: hwmod: partially un-reset hwmods might not be properly enabled

[Benoit Cousson]

Hi Omar,

On 07/16/2012 09:21 PM, Omar Ramirez Luna wrote:
> Some IP blocks might not be using/controlling more than one
> reset line, this check loosens the restriction to fully use
> hwmod framework for those drivers.
> 
> E.g.: ipu has reset lines: mmu_cache, cpu0 and cpu1.
> - cpu1 might not be used and hence (with previous check)
>   won't be fully enabled by hwmod code.

You mean that you might have some case where you need to enable the
mmu_cache and cpu0 and thus deassert only the mmu/cpu0 while keeping the
cpu1 under reset?

So the any_hardreset is indeed not appropriate in that case.

In fact, since the hardreset cannot be handled at all by the hwmod fmwk,
I'm even wondering if we should take care of checking the state at all.
But as Paul stated, if was done due to the lack of understanding about
the diver usage, so maybe things will become clearer once we will have
that code available.

So I guess that checking for all the lines for the hardreset state is
anyway already a first step toward a good understanding of the reset
need for the remote processors.

The patch looks fine to me considering that we do not have a lot of
information about the usage :-(

Maybe you could add more information in the changelog to explain the way
you are going to use the reset API.

Regards,
Benoit


> While at it, prevent _omap4_module_disable if all the hardreset
> lines on an IP block are not under reset.


> 
> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   37 ++++++++++++++++++++++---------------
>  1 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 3215dad..091c199 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1558,25 +1558,28 @@ static int _read_hardreset(struct omap_hwmod *oh, const char *name)
>  }
>  
>  /**
> - * _are_any_hardreset_lines_asserted - return true if part of @oh is hard-reset
> + * _are_all_hardreset_lines_asserted - return true if the @oh is hard-reset
>   * @oh: struct omap_hwmod *
>   *
> - * If any hardreset line associated with @oh is asserted, then return true.
> - * Otherwise, if @oh has no hardreset lines associated with it, or if
> - * no hardreset lines associated with @oh are asserted, then return false.
> + * If all hardreset lines associated with @oh are asserted, then return true.
> + * Otherwise, if part of @oh is out hardreset or if no hardreset lines
> + * associated with @oh are asserted, then return false.
>   * This function is used to avoid executing some parts of the IP block
> - * enable/disable sequence if a hardreset line is set.
> + * enable/disable sequence if its hardreset line is set.
>   */
> -static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
> +static bool _are_all_hardreset_lines_asserted(struct omap_hwmod *oh)
>  {
> -	int i;
> +	int i, rst_cnt = 0;
>  
>  	if (oh->rst_lines_cnt == 0)
>  		return false;
>  
>  	for (i = 0; i < oh->rst_lines_cnt; i++)
>  		if (_read_hardreset(oh, oh->rst_lines[i].name) > 0)
> -			return true;
> +			rst_cnt++;
> +
> +	if (oh->rst_lines_cnt == rst_cnt)
> +		return true;
>  
>  	return false;
>  }
> @@ -1595,6 +1598,13 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
>  		return -EINVAL;
>  
> +	/*
> +	 * Since integration code might still be doing something, only
> +	 * disable if all lines are under hardreset.
> +	 */
> +	if (!_are_all_hardreset_lines_asserted(oh))
> +		return 0;
> +
>  	pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
>  
>  	omap4_cminst_module_disable(oh->clkdm->prcm_partition,
> @@ -1602,9 +1612,6 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>  				    oh->clkdm->clkdm_offs,
>  				    oh->prcm.omap4.clkctrl_offs);
>  
> -	if (_are_any_hardreset_lines_asserted(oh))
> -		return 0;
> -
>  	v = _omap4_wait_target_disable(oh);
>  	if (v)
>  		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> @@ -1830,7 +1837,7 @@ static int _enable(struct omap_hwmod *oh)
>  	}
>  
>  	/*
> -	 * If an IP block contains HW reset lines and any of them are
> +	 * If an IP block contains HW reset lines and all of them are
>  	 * asserted, we let integration code associated with that
>  	 * block handle the enable.  We've received very little
>  	 * information on what those driver authors need, and until
> @@ -1838,7 +1845,7 @@ static int _enable(struct omap_hwmod *oh)
>  	 * posted to the public lists, this is probably the best we
>  	 * can do.
>  	 */
> -	if (_are_any_hardreset_lines_asserted(oh))
> +	if (_are_all_hardreset_lines_asserted(oh))
>  		return 0;
>  
>  	/* Mux pins for device runtime if populated */
> @@ -1918,7 +1925,7 @@ static int _idle(struct omap_hwmod *oh)
>  		return -EINVAL;
>  	}
>  
> -	if (_are_any_hardreset_lines_asserted(oh))
> +	if (_are_all_hardreset_lines_asserted(oh))
>  		return 0;
>  
>  	if (oh->class->sysc)
> @@ -2006,7 +2013,7 @@ static int _shutdown(struct omap_hwmod *oh)
>  		return -EINVAL;
>  	}
>  
> -	if (_are_any_hardreset_lines_asserted(oh))
> +	if (_are_all_hardreset_lines_asserted(oh))
>  		return 0;
>  
>  	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
> 

[PATCH 1/3] ARM: OMAP: hwmod: partially un-reset hwmods might not be properly enabled

[Omar Ramirez Luna]

Hi Benoit,

On 20 August 2012 09:49, Benoit Cousson <b-cousson@ti.com> wrote:
> On 07/16/2012 09:21 PM, Omar Ramirez Luna wrote:
>> Some IP blocks might not be using/controlling more than one
>> reset line, this check loosens the restriction to fully use
>> hwmod framework for those drivers.
>>
>> E.g.: ipu has reset lines: mmu_cache, cpu0 and cpu1.
>> - cpu1 might not be used and hence (with previous check)
>>   won't be fully enabled by hwmod code.
>
> You mean that you might have some case where you need to enable the
> mmu_cache and cpu0 and thus deassert only the mmu/cpu0 while keeping the
> cpu1 under reset?
>
> So the any_hardreset is indeed not appropriate in that case.
>
> In fact, since the hardreset cannot be handled at all by the hwmod fmwk,
> I'm even wondering if we should take care of checking the state at all.
> But as Paul stated, if was done due to the lack of understanding about
> the diver usage, so maybe things will become clearer once we will have
> that code available.
>
> So I guess that checking for all the lines for the hardreset state is
> anyway already a first step toward a good understanding of the reset
> need for the remote processors.
>
> The patch looks fine to me considering that we do not have a lot of
> information about the usage :-(
>
> Maybe you could add more information in the changelog to explain the way
> you are going to use the reset API.

I'll add an updated description with more information.

Thanks for the comments,

Omar

[PATCH 1/3] ARM: OMAP: hwmod: partially un-reset hwmods might not be properly enabled

[Omar Ramirez Luna]

On 20 August 2012 09:49, Benoit Cousson <b-cousson@ti.com> wrote:
> On 07/16/2012 09:21 PM, Omar Ramirez Luna wrote:
>> Some IP blocks might not be using/controlling more than one
>> reset line, this check loosens the restriction to fully use
>> hwmod framework for those drivers.
>>
>> E.g.: ipu has reset lines: mmu_cache, cpu0 and cpu1.
>> - cpu1 might not be used and hence (with previous check)
>>   won't be fully enabled by hwmod code.
>
> You mean that you might have some case where you need to enable the
> mmu_cache and cpu0 and thus deassert only the mmu/cpu0 while keeping the
> cpu1 under reset?

Looks like I didn't reply to this question.

Yes, initially cpu1 might not be used and kept under reset. Or even
the mmu can be taken out of reset and configured, and cpu0 after it,
creating a small window where one is taken out and the other is under
reset.

> So the any_hardreset is indeed not appropriate in that case.
>
> In fact, since the hardreset cannot be handled at all by the hwmod fmwk,
> I'm even wondering if we should take care of checking the state at all.
> But as Paul stated, if was done due to the lack of understanding about
> the diver usage, so maybe things will become clearer once we will have
> that code available.

I still think it can, in fact all the code I'm using comes from the
hwmod fmwk. _deassert_reset is almost the same as _enable, I left it
this way to avoid reintroducing the issues that caused reset code to
be stripped out from _enable path.

Regards,

Omar

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

[Omar Ramirez Luna]

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(-)

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;
 }
 
-- 
1.7.4.1

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

[Paul Walmsley]

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

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

[Omar Ramirez Luna]

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

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

[Vaibhav Hiremath]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Omar Ramirez Luna]

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

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

[Benoit Cousson]

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

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

[Omar Ramirez Luna]

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

[PATCH 3/3] ARM: OMAP: omap_device: expose hwmod assert/deassert to omap devices

[Omar Ramirez Luna]

This APIs are meant to be an interface to hwmod assert/deassert
function, omap devices can call them through their platform data
to control their reset lines, they are expected to know the name
of the reset line they are trying to control.

Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
 arch/arm/plat-omap/omap_device.c              |   45 +++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 4327b2c..27bcc24 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -118,6 +118,10 @@ int omap_device_get_context_loss_count(struct platform_device *pdev);
 
 /* Other */
 
+int omap_device_assert_hardreset(struct platform_device *pdev,
+				 const char *name);
+int omap_device_deassert_hardreset(struct platform_device *pdev,
+				 const char *name);
 int omap_device_idle_hwmods(struct omap_device *od);
 int omap_device_enable_hwmods(struct omap_device *od);
 
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index c490240..8883074 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -925,6 +925,51 @@ int omap_device_shutdown(struct platform_device *pdev)
 }
 
 /**
+ * omap_device_assert_hardreset - set a device's reset line
+ * @pdev: struct platform_device * to reset
+ * @name: const char * with the name of the reset line
+ *
+ * According to @name, set the reset line of the hwmods associated
+ * with this @pdev deivce.
+ */
+int omap_device_assert_hardreset(struct platform_device *pdev, const char *name)
+{
+	int i, ret = 0;
+	struct omap_device *od = to_omap_device(pdev);
+
+	for (i = 0; i < od->hwmods_cnt; i++) {
+		ret = omap_hwmod_assert_hardreset(od->hwmods[i], name);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * omap_device_deassert_hardreset - lift a device's reset line
+ * @pdev: struct platform_device * to reset
+ * @name: const char * with the name of the reset line
+ *
+ * According to @name, lift the reset line of the hwmods associated
+ * with this @pdev deivce.
+ */
+int omap_device_deassert_hardreset(struct platform_device *pdev,
+				const char *name)
+{
+	int i, ret = 0;
+	struct omap_device *od = to_omap_device(pdev);
+
+	for (i = 0; i < od->hwmods_cnt; i++) {
+		ret = omap_hwmod_deassert_hardreset(od->hwmods[i], name);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+/**
  * omap_device_align_pm_lat - activate/deactivate device to match wakeup lat lim
  * @od: struct omap_device *
  *
-- 
1.7.4.1

[PATCH 3/3] ARM: OMAP: omap_device: expose hwmod assert/deassert to omap devices

[Paul Walmsley]

Hi Omar

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

> This APIs are meant to be an interface to hwmod assert/deassert
> function, omap devices can call them through their platform data
> to control their reset lines, they are expected to know the name
> of the reset line they are trying to control.
> 
> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>

This one has been queued for 3.7 with a few changes. Some more detail was 
added to the function documentationrovement.  Also the multiple 
assignments were removed per Documentation/CodingStyle chapter 1:

"Don't put multiple assignments on a single line either."

Please let me know if you have any comments.


- Paul

From: Omar Ramirez Luna <omar.luna@linaro.org>
Date: Mon, 16 Jul 2012 14:21:25 -0500
Subject: [PATCH] ARM: OMAP2+: omap_device: expose hwmod assert/deassert to
 omap devices

This API is meant to be an interface to hwmod assert/deassert
function, omap devices can call them through their platform data to
control their reset lines, they are expected to know the name of the
reset line they are trying to control.

Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
[paul at pwsan.com: tweaked some documentation; fixed CodingStyle issue]
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
 arch/arm/plat-omap/omap_device.c              |   55 +++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 4327b2c..27bcc24 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -118,6 +118,10 @@ int omap_device_get_context_loss_count(struct platform_device *pdev);
 
 /* Other */
 
+int omap_device_assert_hardreset(struct platform_device *pdev,
+				 const char *name);
+int omap_device_deassert_hardreset(struct platform_device *pdev,
+				 const char *name);
 int omap_device_idle_hwmods(struct omap_device *od);
 int omap_device_enable_hwmods(struct omap_device *od);
 
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index c490240..3b02312 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -925,6 +925,61 @@ int omap_device_shutdown(struct platform_device *pdev)
 }
 
 /**
+ * omap_device_assert_hardreset - set a device's hardreset line
+ * @pdev: struct platform_device * to reset
+ * @name: const char * name of the reset line
+ *
+ * Set the hardreset line identified by @name on the IP blocks
+ * associated with the hwmods backing the platform_device @pdev.  All
+ * of the hwmods associated with @pdev must have the same hardreset
+ * line linked to them for this to work.  Passes along the return value
+ * of omap_hwmod_assert_hardreset() in the event of any failure, or
+ * returns 0 upon success.
+ */
+int omap_device_assert_hardreset(struct platform_device *pdev, const char *name)
+{
+	struct omap_device *od = to_omap_device(pdev);
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < od->hwmods_cnt; i++) {
+		ret = omap_hwmod_assert_hardreset(od->hwmods[i], name);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+/**
+ * omap_device_deassert_hardreset - release a device's hardreset line
+ * @pdev: struct platform_device * to reset
+ * @name: const char * name of the reset line
+ *
+ * Release the hardreset line identified by @name on the IP blocks
+ * associated with the hwmods backing the platform_device @pdev.  All
+ * of the hwmods associated with @pdev must have the same hardreset
+ * line linked to them for this to work.  Passes along the return
+ * value of omap_hwmod_deassert_hardreset() in the event of any
+ * failure, or returns 0 upon success.
+ */
+int omap_device_deassert_hardreset(struct platform_device *pdev,
+				   const char *name)
+{
+	struct omap_device *od = to_omap_device(pdev);
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < od->hwmods_cnt; i++) {
+		ret = omap_hwmod_deassert_hardreset(od->hwmods[i], name);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+/**
  * omap_device_align_pm_lat - activate/deactivate device to match wakeup lat lim
  * @od: struct omap_device *
  *
-- 
1.7.10.4

[PATCH 3/3] ARM: OMAP: omap_device: expose hwmod assert/deassert to omap devices

[Omar Ramirez Luna]

On 2 August 2012 02:43, Paul Walmsley <paul@pwsan.com> wrote:
>> This APIs are meant to be an interface to hwmod assert/deassert
>> function, omap devices can call them through their platform data
>> to control their reset lines, they are expected to know the name
>> of the reset line they are trying to control.
>>
>> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>
>
> This one has been queued for 3.7 with a few changes. Some more detail was
> added to the function documentationrovement.  Also the multiple
> assignments were removed per Documentation/CodingStyle chapter 1:
>
> "Don't put multiple assignments on a single line either."
>
> Please let me know if you have any comments.

Agree.

Thanks,

Omar

[PATCH 0/3] OMAP: hwmod: reset API proposal

[Omar Ramirez Luna]

Recent changes in omap_hwmod framework have reworked the behaviour
towards hardreset handling, commit 747834a (ARM: OMAP2+: hwmod:
revise hardreset behavior) recommends for drivers to implement
their own reset sequences until code out-of-tree hits mainline
and then their needs and code can be reviewed.

Since it is not clear when this will occur for all drivers and
hwmod code was not deleted (presumably because at some point it
will handle the resets once again), this series exports functions
to handle hardreset lines in an attempt to reduce code duplication
for those who have a common reset sequence.

These APIs are intended to be used by iommu for now, but were
tested with IPU and remoteproc on Pandaboard.
 
Omar Ramirez Luna (3):
  ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
    enabled
  ARM: OMAP: hwmod: revise deassert sequence
  ARM: OMAP: omap_device: expose hwmod assert/deassert to omap devices

 arch/arm/mach-omap2/omap_hwmod.c              |   70 +++++++++++++++++++------
 arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
 arch/arm/plat-omap/omap_device.c              |   45 ++++++++++++++++
 3 files changed, 103 insertions(+), 16 deletions(-)

-- 
1.7.4.1

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

[Omar Ramirez Luna]

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, 32 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 5f0811a..8a7e5bc 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1472,7 +1472,7 @@ static int _assert_hardreset(struct omap_hwmod *oh, const char *name)
 static int _deassert_hardreset(struct omap_hwmod *oh, const char *name)
 {
 	struct omap_hwmod_rst_info ohri;
-	int ret;
+	int ret, hwsup = 0;
 
 	if (!oh)
 		return -EINVAL;
@@ -1481,6 +1481,23 @@ 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);
+
 	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
 		ret = omap2_prm_deassert_hardreset(oh->prcm.omap2.module_offs,
 						   ohri.rst_shift,
@@ -1497,9 +1514,23 @@ static int _deassert_hardreset(struct omap_hwmod *oh, const char *name)
 		return -EINVAL;
 	}
 
+	_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;
 }
 
-- 
1.7.4.1