[PATCH 0/2] OMAP: hwmod: fix hardreset handling

[PATCH 0/2] OMAP: hwmod: fix hardreset handling

[Omar Ramirez Luna]

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

The patch to expose hwmod assert/deassert functions through omap_device
has been accepted and queued for 3.7[1], however these two patches are
needed to make the API functional. Hence a revised version is being sent
according to previous comments:

- ARM: OMAP: hwmod: revise deassert sequence
Now it uses enable|disable_module to handle the configuration of the
modulemode inside CLKCTRL. As part of the cleanup of leaf clocks, the
one associated with ipu_mmu will be removed along with the iommu hwmod
migration[2].

- ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
  enabled
More infomation added in the patch description[3].

[1] http://patchwork.kernel.org/patch/1266731/
[2] http://patchwork.kernel.org/patch/1201791/
[3] http://patchwork.kernel.org/patch/1201801/

Omar Ramirez Luna (2):
  ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
    enabled
  ARM: OMAP: hwmod: revise deassert sequence

 arch/arm/mach-omap2/omap_hwmod.c |   74 ++++++++++++++++++++++++++++++--------
 1 file changed, 59 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] 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.
- As of now cpu1 is not used and hence (with previous check) the
  IP block isn't fully enabled by hwmod code.
- Usually ipu and dsp processors configure their mmu module first
  and then enable the processors, this involves:
    * Deasserting mmu reset line, and enabling the module.
    * Deasserting cpu0 reset line, and enabling the processor.
  The ones portrayed in this example are controlled through
  rproc_fw_boot in drivers/remoteproc/remoteproc_core.c

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

This will allow the driver to:
  a. Deassert the reset line.
  b. Enable the hwmod through runtime PM default callbacks.
  c. Do its usecase.
  d. Disable hwmod through runtime PM.
  e. Assert the reset line.

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

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 6ca8e51..eaedc33 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.9.5

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

[Paul Walmsley]

Hi Omar

On Wed, 22 Aug 2012, 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.
> - As of now cpu1 is not used and hence (with previous check) the
>   IP block isn't fully enabled by hwmod code.
> - Usually ipu and dsp processors configure their mmu module first
>   and then enable the processors, this involves:
>     * Deasserting mmu reset line, and enabling the module.
>     * Deasserting cpu0 reset line, and enabling the processor.
>   The ones portrayed in this example are controlled through
>   rproc_fw_boot in drivers/remoteproc/remoteproc_core.c
> 
> While at it, prevent _omap4_module_disable if all the hardreset
> lines on an IP block are not under reset.
> 
> This will allow the driver to:
>   a. Deassert the reset line.
>   b. Enable the hwmod through runtime PM default callbacks.
>   c. Do its usecase.
>   d. Disable hwmod through runtime PM.
>   e. Assert the reset line.
> 
> Signed-off-by: Omar Ramirez Luna <omar.luna@linaro.org>

Well, I don't think this is the right long-term solution, but I'd like to 
see this moving forward.  So this one has been queued for 3.7.  Hopefully 
we can figure out the right thing once the PRM driver is ready (which will 
handle the low-level hard resets).


- Paul

[PATCH 2/2] 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 |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index eaedc33..b65e021 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,46 @@ 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 (soc_ops.enable_module)
+		soc_ops.enable_module(oh);
+
 	ret = soc_ops.deassert_hardreset(oh, &ohri);
+
+	if (soc_ops.disable_module)
+		soc_ops.disable_module(oh);
+	_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.9.5

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

[Benoit Cousson]

Hi Omar,

On 08/22/2012 07:42 AM, 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>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index eaedc33..b65e021 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,46 @@ 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 (soc_ops.enable_module)
> +		soc_ops.enable_module(oh);
> +
>  	ret = soc_ops.deassert_hardreset(oh, &ohri);
> +
> +	if (soc_ops.disable_module)
> +		soc_ops.disable_module(oh);
> +	_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;
>  }
>  

The sequence is good, I'm just a little bit concern about the
duplication of code compared to _enable sequence.

That being said, this is the consequence of removing the hardreset
sequence outside of the main _enable/_shutdown sequence.

So I'm not sure I have any better way of doing that :-(

Regards,
Benoit

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

[Omar Ramirez Luna]

Hi Benoit,

On 6 September 2012 10:12, Benoit Cousson <b-cousson@ti.com> wrote:
> The sequence is good, I'm just a little bit concern about the
> duplication of code compared to _enable sequence.
>
> That being said, this is the consequence of removing the hardreset
> sequence outside of the main _enable/_shutdown sequence.
>
> So I'm not sure I have any better way of doing that :-(

Indeed, it should be exactly the same as putting back the reset
sequence into _enable/_shutdown, so with these patches I was expecting
we could gather the hard reset users and see if they needed anything
else beyond these functions, if not, perhaps just put back the reset
code into _enable/_shutdown paths.

Thanks,

Omar

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

[Paul Walmsley]

Hi

On Wed, 22 Aug 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>

Queued for 3.7, with the same caveats mentioned regarding patch 1.


- Paul

Re: [PATCH 0/2] OMAP: hwmod: fix hardreset handling

[Omar Ramirez Luna]

On 22 August 2012 00:42, Omar Ramirez Luna <omar.luna@linaro.org> wrote:
> From: Omar Ramirez Luna <omar.ramirez@ti.com>
>
> The patch to expose hwmod assert/deassert functions through omap_device
> has been accepted and queued for 3.7[1], however these two patches are
> needed to make the API functional. Hence a revised version is being sent
> according to previous comments:
>
> - ARM: OMAP: hwmod: revise deassert sequence
> Now it uses enable|disable_module to handle the configuration of the
> modulemode inside CLKCTRL. As part of the cleanup of leaf clocks, the
> one associated with ipu_mmu will be removed along with the iommu hwmod
> migration[2].
>
> - ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
>   enabled
> More infomation added in the patch description[3].
>
> [1] http://patchwork.kernel.org/patch/1266731/
> [2] http://patchwork.kernel.org/patch/1201791/
> [3] http://patchwork.kernel.org/patch/1201801/
>
> Omar Ramirez Luna (2):
>   ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
>     enabled
>   ARM: OMAP: hwmod: revise deassert sequence
>
>  arch/arm/mach-omap2/omap_hwmod.c |   74 ++++++++++++++++++++++++++++++--------
>  1 file changed, 59 insertions(+), 15 deletions(-)

Ping.

Regards,

Omar

Re: [PATCH 0/2] OMAP: hwmod: fix hardreset handling

[Benoit Cousson]

Hi Omar,

On 09/03/2012 04:29 PM, Omar Ramirez Luna wrote:
> On 22 August 2012 00:42, Omar Ramirez Luna <omar.luna@linaro.org> wrote:
>> From: Omar Ramirez Luna <omar.ramirez@ti.com>
>>
>> The patch to expose hwmod assert/deassert functions through omap_device
>> has been accepted and queued for 3.7[1], however these two patches are
>> needed to make the API functional. Hence a revised version is being sent
>> according to previous comments:
>>
>> - ARM: OMAP: hwmod: revise deassert sequence
>> Now it uses enable|disable_module to handle the configuration of the
>> modulemode inside CLKCTRL. As part of the cleanup of leaf clocks, the
>> one associated with ipu_mmu will be removed along with the iommu hwmod
>> migration[2].
>>
>> - ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
>>   enabled
>> More infomation added in the patch description[3].
>>
>> [1] http://patchwork.kernel.org/patch/1266731/
>> [2] http://patchwork.kernel.org/patch/1201791/
>> [3] http://patchwork.kernel.org/patch/1201801/
>>
>> Omar Ramirez Luna (2):
>>   ARM: OMAP: hwmod: partially un-reset hwmods might not be properly
>>     enabled
>>   ARM: OMAP: hwmod: revise deassert sequence
>>
>>  arch/arm/mach-omap2/omap_hwmod.c |   74 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 59 insertions(+), 15 deletions(-)
> 
> Ping.

Oops, sorry, I forgot your series.

Beside the concern about the duplication of code, that looks good to me.

I'll sync with Paul to see who will push that series.

Regards,
Benoit