[v2,4/8] ARM: OMAP2+: hwmod: revise hardreset behavior

Change the way that hardreset lines are handled by the hwmod code.
Hardreset lines are generally associated with initiator IP blocks.
Prior to this change, the hwmod code expected to control hardreset
lines itself, asserting them on shutdown and deasserting them upon
enable.  But driver authors inside TI have commented to us that their
drivers require direct control over these lines.  Unfortunately, these
drivers haven't been posted publicly yet, so it's hard to determine
exactly what is needed, a priori.  This change attempts to set forth
some reasonable semantics that should be an improvement over the
current code.

The semantics implemented by this patch are as follows:

- If the hwmod is not marked with HWMOD_INIT_NO_RESET, then assert all
  associated hardreset lines during IP block setup.  This is intended
  to place the IP blocks into a known state that will not interfere
  with other devices during kernel boot.

- IP blocks with hardreset lines will not be automatically enabled or
  idled during setup.  Instead, they will be left in the INITIALIZED
  state.

- When the hwmod code is asked to enable the IP block -- presumably by
  the driver -- all clocks will be enabled, but the hardreset lines
  will remain untouched.  For this to not return timeout warnings if
  the hardreset lines are asserted, we bypass the PRCM transition
  test.

- When the hwmod code is asked to idle the IP block -- presumably by
  the driver -- all clocks will be disabled, but the hardreset lines
  will remain untouched.  For this to not return timeout warnings if
  the hardreset lines are asserted, we bypass the PRCM transition
  test.

Custom reset functions for IP blocks with hardreset lines still should
be supported and are strongly endorsed.  It is intended that every
subsystem with hardreset lines should have a custom reset function
that can place their subsystem into quiescent idle with the hardreset
lines deasserted.

This reverts most of commit 5365efbe29250a227502256cc912351fe2157b42
("OMAP: hwmod: Add hardreset management support").  Later code
reorganizations caused the sequencing of the code from this patch to
be changed, anyway.

It is expected that drivers may require additional functions to be
exposed from the hwmod code, such as a function to allow drivers to
wait for IDLEST bits to change after hardreset lines are toggled.
Driver authors are expected to propose these if needed.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Benoît Cousson <b-cousson@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   51 ++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 27 deletions(-)

+ Omar and Ohad,

Salut Paul,

On 4/19/2012 8:53 AM, Paul Walmsley wrote:
> Hi Benoît,
>
> On Mon, 27 Feb 2012, Paul Walmsley wrote:
>
>> Change the way that hardreset lines are handled by the hwmod code.
>> Hardreset lines are generally associated with initiator IP blocks.
>> Prior to this change, the hwmod code expected to control hardreset
>> lines itself, asserting them on shutdown and deasserting them upon
>> enable.  But driver authors inside TI have commented to us that their
>> drivers require direct control over these lines.  Unfortunately, these
>> drivers haven't been posted publicly yet, so it's hard to determine
>> exactly what is needed, a priori.  This change attempts to set forth
>> some reasonable semantics that should be an improvement over the
>> current code.
>
> I took another look at this patch, and upon further thought, there are
> some aspects of this design that really don't make sense.

That's perfect timing, because I was struggling with some hardreset 
issue on OMAP5 yesterday, so I thought as well about that patch.

This is just some more thoughts on that topic more than a pure comment 
on this patch :-)

What I realized is that the main issue was due to the fake hwmods to 
control processor reset lines. So these one has to be removed, it was 
clearly a mistake.

The other issue was due to the wrong association between the reset lines 
and the hwmod. In fact I was trying to create ipu & dsp hwmods, whereas 
these subsystem do not have any direct connection with the main 
interconnect but only through their MMU.
This is exactly what Omar raised when he tried to introduce the MMU hwmods.
And in fact the new name used in OMAP5 are highlight that a little bit 
netter than on OMAP4"

     'ipu':
         'rst_cpu0'
         'rst_cpu1'
         'rst_ipu_mmu_cache'

     'dsp':
         'rst_dsp'
         'rst_dsp_mmu_cache'

     'iva':
         'rst_seq1'
         'rst_seq2'
         'rst_logic'

The concern of the people doing SW in these embedded processors was 
mainly because we were releasing the reset of processor without loading 
any SW first and thus the processor was executing some random instructions.

So if we consider a better hwmods definition, we can potentially fix the 
MMU case:

     'mmu_ipu':
         'rst_ipu_mmu_cache'

     'mmu_dsp':
         'rst_dsp_mmu_cache'

     'iva_config':
         'rst_logic'


But then we do have the processor resets that have to be exposed somewhere.

     'ipu':
         'rst_cpu0'
         'rst_cpu1'

     'dsp':
         'rst_dsp'

     'iva':
         'rst_seq1'
         'rst_seq2'

None of these one should be controlled automatically.

>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>> index 5b368ee..db4ad41 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -805,6 +805,9 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>>   				    oh->clkdm->clkdm_offs,
>>   				    oh->prcm.omap4.clkctrl_offs);
>>
>> +	if (oh->rst_lines_cnt>= 0)
>> +		return 0;
>
> This change prevents any IP block from waiting for the target to disable
> -- which is not what we want.  The naïve fix would be to only skip the
> disable-wait if oh->rst_lines_cnt is greater than zero.  But if there are
> no hardreset lines asserted, then we should probably wait for the disable
> in that case.
>
>>   	v = _omap4_wait_target_disable(oh);
>>   	if (v)
>>   		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
>
> [...]
>
>> @@ -1575,7 +1568,19 @@ static int _enable(struct omap_hwmod *oh)
>>   	_enable_clocks(oh);
>>   	_enable_module(oh);
>>
>> -	r = _wait_target_ready(oh);
>> +	/*
>> +	 * If an IP contains HW reset lines, we leave them
>> +	 * asserted.  But this will block the module's idle state
>> +	 * transition - the PRCM will return Intransition status.  So
>> +	 * we need to avoid the target ready-wait in this case.  XXX
>> +	 * We also need to give the drivers a way to wait for the
>> +	 * target to become ready once they decide to deassert some
>> +	 * hardreset lines.  XXX Is this strategy going to break PM
>> +	 * because the clockdomain may not be able to enter idle while
>> +	 * the module's idle status is in-transition?  We may just need
>> +	 * custom reset blocks for all IPs with hardreset lines.
>> +	 */
>> +	r = (oh->rst_lines_cnt == 0) ? _wait_target_ready(oh) : 1;
>
> And this part is at odds with the patch description.  If there are
> hardreset lines associated with an IP block, then this code will cause the
> following code to unconditionally disable the module clocks.  Considering
> that this is the _enable() function, this seems counterproductive.
>
> I looked into changing this code to align with the original semantics we
> discussed.  It seems quite challenging.  With the current codebase, we'd
> have to bail out in the middle of the enable sequence.  Then we'd lose the
> clockdomain state (the 'hwsup' variable).
>
> So I've updated the patch to essentially bail out early from all hwmod
> enable, idle, and shutdown code, if any hardreset lines associated with
> the IP block are asserted.  It will then be the driver integration code's
> responsibility for enabling the IP block when the hardreset lines are
> asserted.  When the hardreset lines are deasserted, the usual hwmod code
> will be executed -- I'd assume this would be the case during normal
> operation of the device.  I think this is probably the best we can do
> until we actually hear back from the people responsible for drivers for
> these special IP blocks.
>
> A revised patch is below.  Care to take a look and see if it makes sense
> to you?
>
>
> regards,
>
> - Paul
>
>
> From: Paul Walmsley<paul@pwsan.com>
> Date: Wed, 18 Apr 2012 19:10:04 -0600
> Subject: [PATCH] ARM: OMAP2+: hwmod: revise hardreset behavior
>
> Change the way that hardreset lines are handled by the hwmod code.
> Hardreset lines are generally associated with initiator IP blocks.
> Prior to this change, the hwmod code expected to control hardreset
> lines itself, asserting them on shutdown and deasserting them upon
> enable.  But driver authors inside TI have commented to us that their
> drivers require direct control over these lines.  Unfortunately, these
> drivers haven't been posted publicly yet, so it's hard to determine
> exactly what is needed, a priori.  This change attempts to set forth
> some reasonable semantics that should be an improvement over the
> current code.
>
> The semantics implemented by this patch are as follows:
>
> - If the hwmod is not marked with HWMOD_INIT_NO_RESET, then assert all
>    associated hardreset lines during IP block setup.  This is intended
>    to place the IP blocks into a known state that will not interfere
>    with other devices during kernel boot.
>
> - IP blocks with hardreset lines will not be automatically enabled or
>    idled during setup.  Instead, they will be left in the INITIALIZED
>    state.
>
> - When the hwmod code is asked to enable, idle, or shutdown an IP
>    block with asserted hardreset lines, the hwmod code will do nothing.
>    The driver integration code must do the remaining work needed to
>    control these IP blocks.  Once this driver integration code is posted
>    to the lists, hopefully we can consolidate it and move it inside the
>    hwmod code.
>
> Custom reset functions for IP blocks with hardreset lines still should
> be supported and are strongly endorsed.  It is intended that every
> subsystem with hardreset lines should have a custom reset function
> that can place their subsystem into quiescent idle with the hardreset
> lines deasserted.
>
> This reverts most of commit 5365efbe29250a227502256cc912351fe2157b42
> ("OMAP: hwmod: Add hardreset management support").  Later code
> reorganizations caused the sequencing of the code from this patch to
> be changed, anyway.
>
> Signed-off-by: Paul Walmsley<paul@pwsan.com>
> Cc: Benoît Cousson<b-cousson@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |  140 ++++++++++++++++++++++----------------
>   1 file changed, 83 insertions(+), 57 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e6aa14f..a5c3a9a 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -781,39 +781,6 @@ static int _omap4_wait_target_disable(struct omap_hwmod *oh)
>   }
>
>   /**
> - * _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
> - * @oh: struct omap_hwmod *
> - *
> - * Disable the PRCM module mode related to the hwmod @oh.
> - * Return EINVAL if the modulemode is not supported and 0 in case of success.
> - */
> -static int _omap4_disable_module(struct omap_hwmod *oh)
> -{
> -	int v;
> -
> -	/* The module mode does not exist prior OMAP4 */
> -	if (!cpu_is_omap44xx())
> -		return -EINVAL;
> -
> -	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
> -		return -EINVAL;
> -
> -	pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
> -
> -	omap4_cminst_module_disable(oh->clkdm->prcm_partition,
> -				    oh->clkdm->cm_inst,
> -				    oh->clkdm->clkdm_offs,
> -				    oh->prcm.omap4.clkctrl_offs);
> -
> -	v = _omap4_wait_target_disable(oh);
> -	if (v)
> -		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> -			oh->name);
> -
> -	return 0;
> -}
> -
> -/**
>    * _count_mpu_irqs - count the number of MPU IRQ lines associated with @oh
>    * @oh: struct omap_hwmod *oh
>    *
> @@ -1378,6 +1345,66 @@ 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
> + * @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.
> + * This function is used to avoid executing some parts of the IP block
> + * enable/disable sequence if a hardreset line is set.
> + */
> +static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
> +{
> +	int i;
> +
> +	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;
> +
> +	return false;
> +}
> +
> +/**
> + * _omap4_disable_module - enable CLKCTRL modulemode on OMAP4
> + * @oh: struct omap_hwmod *
> + *
> + * Disable the PRCM module mode related to the hwmod @oh.
> + * Return EINVAL if the modulemode is not supported and 0 in case of success.
> + */
> +static int _omap4_disable_module(struct omap_hwmod *oh)
> +{
> +	int v;
> +
> +	/* The module mode does not exist prior OMAP4 */
> +	if (!cpu_is_omap44xx())
> +		return -EINVAL;
> +
> +	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
> +		return -EINVAL;
> +
> +	pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
> +
> +	omap4_cminst_module_disable(oh->clkdm->prcm_partition,
> +				    oh->clkdm->cm_inst,
> +				    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",
> +			oh->name);
> +
> +	return 0;
> +}
> +
> +/**
>    * _ocp_softreset - reset an omap_hwmod via the OCP_SYSCONFIG bit
>    * @oh: struct omap_hwmod *
>    *
> @@ -1519,7 +1546,7 @@ static int _reset(struct omap_hwmod *oh)
>    */
>   static int _enable(struct omap_hwmod *oh)
>   {
> -	int r, i;
> +	int r;
>   	int hwsup = 0;
>
>   	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
> @@ -1551,14 +1578,16 @@ static int _enable(struct omap_hwmod *oh)
>   	}
>
>   	/*
> -	 * If an IP contains HW reset lines, then de-assert them in order
> -	 * to allow the module state transition. Otherwise the PRCM will return
> -	 * Intransition status, and the init will failed.
> +	 * If an IP block contains HW reset lines and any 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
> +	 * detailed information is provided and the driver code is
> +	 * posted to the public lists, this is probably the best we
> +	 * can do.

Maybe we should keep that with some mechanism to prevent it for some IP 
like processors .

I guess that with that patch, we cannot enable anymore IPU/DSP and IVA 
at boot time.

Otherwise, it looks fine to me.

Thanks,
Benoit

>   	 */
> -	if (oh->_state == _HWMOD_STATE_INITIALIZED ||
> -	    oh->_state == _HWMOD_STATE_DISABLED)
> -		for (i = 0; i<  oh->rst_lines_cnt; i++)
> -			_deassert_hardreset(oh, oh->rst_lines[i].name);
> +	if (_are_any_hardreset_lines_asserted(oh))
> +		return 0;
>
>   	/* Mux pins for device runtime if populated */
>   	if (oh->mux&&  (!oh->mux->enabled ||
> @@ -1633,6 +1662,9 @@ static int _idle(struct omap_hwmod *oh)
>   		return -EINVAL;
>   	}
>
> +	if (_are_any_hardreset_lines_asserted(oh))
> +		return 0;
> +
>   	if (oh->class->sysc)
>   		_idle_sysc(oh);
>   	_del_initiator_dep(oh, mpu_oh);
> @@ -1715,6 +1747,9 @@ static int _shutdown(struct omap_hwmod *oh)
>   		return -EINVAL;
>   	}
>
> +	if (_are_any_hardreset_lines_asserted(oh))
> +		return 0;
> +
>   	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
>
>   	if (oh->class->pre_shutdown) {
> @@ -1824,27 +1859,18 @@ static int __init _setup_reset(struct omap_hwmod *oh)
>   	if (oh->_state != _HWMOD_STATE_INITIALIZED)
>   		return -EINVAL;
>
> -	/*
> -	 * In the case of hwmod with hardreset that should not be
> -	 * de-assert at boot time, we have to keep the module
> -	 * initialized, because we cannot enable it properly with the
> -	 * reset asserted. Exit without warning because that behavior
> -	 * is expected.
> -	 */
> -	if ((oh->flags&  HWMOD_INIT_NO_RESET)&&  oh->rst_lines_cnt>  0)
> -		return 0;
> -
> -	r = _enable(oh);
> -	if (r) {
> -		pr_warning("omap_hwmod: %s: cannot be enabled (%d)\n",
> -			   oh->name, oh->_state);
> -		return 0;
> +	if (oh->rst_lines_cnt == 0) {
> +		r = _enable(oh);
> +		if (r) {
> +			pr_warning("omap_hwmod: %s: cannot be enabled for reset (%d)\n",
> +				   oh->name, oh->_state);
> +			return -EINVAL;
> +		}
>   	}
>
>   	if (!(oh->flags&  HWMOD_INIT_NO_RESET))
>   		r = _reset(oh);
>
> -
>   	return r;
>   }
>   /**

Hi Benoît,

On Thu, 19 Apr 2012, Cousson, Benoit wrote:

> The concern of the people doing SW in these embedded processors was mainly
> because we were releasing the reset of processor without loading any SW first
> and thus the processor was executing some random instructions.
> 
> So if we consider a better hwmods definition, we can potentially fix the MMU
> case:
> 
>     'mmu_ipu':
>         'rst_ipu_mmu_cache'
> 
>     'mmu_dsp':
>         'rst_dsp_mmu_cache'
> 
>     'iva_config':
>         'rst_logic'

Wouldn't these still be "pseudo-hwmods?"  Or do each of these actually 
have address space, interconnect ports, etc.?

Another approach that might not require defining pseudo-hwmods is to 
define a per-hard-reset-line flag that could be used to alter the way the 
hwmod code handles the hardreset line.

> But then we do have the processor resets that have to be exposed somewhere.
> 
>     'ipu':
>         'rst_cpu0'
>         'rst_cpu1'
> 
>     'dsp':
>         'rst_dsp'
> 
>     'iva':
>         'rst_seq1'
>         'rst_seq2'
> 
> None of these one should be controlled automatically.

Don't we still want to put these IP blocks into reset until a driver for 
these IP blocks is loaded?

> >   	/*
> > -	 * If an IP contains HW reset lines, then de-assert them in order
> > -	 * to allow the module state transition. Otherwise the PRCM will
> > return
> > -	 * Intransition status, and the init will failed.
> > +	 * If an IP block contains HW reset lines and any 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
> > +	 * detailed information is provided and the driver code is
> > +	 * posted to the public lists, this is probably the best we
> > +	 * can do.
> 
> Maybe we should keep that with some mechanism to prevent it for some IP like
> processors .
> 
> I guess that with that patch, we cannot enable anymore IPU/DSP and IVA at boot
> time.

I am probably misunderstanding your comment, but I don't think there's any 
way at the moment to generically enable those IP blocks without driver 
integration code assistance.

If we leave the hardreset lines asserted in _enable(), then the PRCM 
status for those modules will be stuck in In-transition state, according 
to the previous comments in the code.  I think this will block PM idle 
transitions.  Also we have no way to restore the clockdomain idle settings 
during the second part of the hwmod enable sequence, I think, since it 
will need to be executed by driver integration code :-(

And I don't think we can deassert the hardreset lines in _enable() right 
now, for the reason that you mentioned in your message: unless the driver 
integration code implements a custom reset function, we don't have any way 
to load code into the IP block before deasserting the hardreset.

So at this point I'd propose that we encourage these driver authors to 
implement custom reset functions for their IP blocks that load firmware if 
needed, and put them into idle, similar to what omap3_iva_idle() does in 
mach-omap2/pm34xx.c.  If the custom reset function takes the IP block out 
of hardreset, then the subsequent hwmod enable/idle/shutdown calls should 
work, after this patch.

> Otherwise, it looks fine to me.

Thanks for the review.


- Paul

Hi Paul,

On 4/19/2012 7:17 PM, Paul Walmsley wrote:
> Hi Benoît,
>
> On Thu, 19 Apr 2012, Cousson, Benoit wrote:
>
>> The concern of the people doing SW in these embedded processors was mainly
>> because we were releasing the reset of processor without loading any SW first
>> and thus the processor was executing some random instructions.
>>
>> So if we consider a better hwmods definition, we can potentially fix the MMU
>> case:
>>
>>      'mmu_ipu':
>>          'rst_ipu_mmu_cache'
>>
>>      'mmu_dsp':
>>          'rst_dsp_mmu_cache'
>>
>>      'iva_config':
>>          'rst_logic'
>
> Wouldn't these still be "pseudo-hwmods?"  Or do each of these actually
> have address space, interconnect ports, etc.?

Yes, these ones are the only real IPs for MPU stand point, with 
interconnect port and configuration registers.

> Another approach that might not require defining pseudo-hwmods is to
> define a per-hard-reset-line flag that could be used to alter the way the
> hwmod code handles the hardreset line.

Yes, I think this is what Rajendra proposed long time ago...

>> But then we do have the processor resets that have to be exposed somewhere.
>>
>>      'ipu':
>>          'rst_cpu0'
>>          'rst_cpu1'
>>
>>      'dsp':
>>          'rst_dsp'
>>
>>      'iva':
>>          'rst_seq1'
>>          'rst_seq2'
>>
>> None of these one should be controlled automatically.
>
> Don't we still want to put these IP blocks into reset until a driver for
> these IP blocks is loaded?

Yes, indeed, my point is where are we going to declare these reset lines 
if we do not have any fake hwmods to store them.

>>>    	/*
>>> -	 * If an IP contains HW reset lines, then de-assert them in order
>>> -	 * to allow the module state transition. Otherwise the PRCM will
>>> return
>>> -	 * Intransition status, and the init will failed.
>>> +	 * If an IP block contains HW reset lines and any 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
>>> +	 * detailed information is provided and the driver code is
>>> +	 * posted to the public lists, this is probably the best we
>>> +	 * can do.
>>
>> Maybe we should keep that with some mechanism to prevent it for some IP like
>> processors .
>>
>> I guess that with that patch, we cannot enable anymore IPU/DSP and IVA at boot
>> time.
>
> I am probably misunderstanding your comment, but I don't think there's any
> way at the moment to generically enable those IP blocks without driver
> integration code assistance.

Well, for the MMU we can, these are regular IPs (almost). The real 
issues was for the processors.

> If we leave the hardreset lines asserted in _enable(), then the PRCM
> status for those modules will be stuck in In-transition state, according
> to the previous comments in the code.  I think this will block PM idle
> transitions.  Also we have no way to restore the clockdomain idle settings
> during the second part of the hwmod enable sequence, I think, since it
> will need to be executed by driver integration code :-(
>
> And I don't think we can deassert the hardreset lines in _enable() right
> now, for the reason that you mentioned in your message: unless the driver
> integration code implements a custom reset function, we don't have any way
> to load code into the IP block before deasserting the hardreset.

Fully agree, what I was trying to highlight is that we do have two types 
of hardreset here: the processors ones and the MMU / Logic ones.
Only the processors do require some special management because a 
firmware has to be loaded first.

> So at this point I'd propose that we encourage these driver authors to
> implement custom reset functions for their IP blocks that load firmware if
> needed, and put them into idle, similar to what omap3_iva_idle() does in
> mach-omap2/pm34xx.c.  If the custom reset function takes the IP block out
> of hardreset, then the subsequent hwmod enable/idle/shutdown calls should
> work, after this patch.

First they will have to release their driver :-)

Regards,
Benoit

Hi Benoît,

On Thu, 19 Apr 2012, Cousson, Benoit wrote:

> On 4/19/2012 7:17 PM, Paul Walmsley wrote:
> > On Thu, 19 Apr 2012, Cousson, Benoit wrote:
> > 
> > > The concern of the people doing SW in these embedded processors was mainly
> > > because we were releasing the reset of processor without loading any SW
> > > first
> > > and thus the processor was executing some random instructions.
> > > 
> > > So if we consider a better hwmods definition, we can potentially fix the
> > > MMU
> > > case:
> > > 
> > >      'mmu_ipu':
> > >          'rst_ipu_mmu_cache'
> > > 
> > >      'mmu_dsp':
> > >          'rst_dsp_mmu_cache'
> > > 
> > >      'iva_config':
> > >          'rst_logic'
> > 
> > Wouldn't these still be "pseudo-hwmods?"  Or do each of these actually
> > have address space, interconnect ports, etc.?
> 
> Yes, these ones are the only real IPs for MPU stand point, with interconnect
> port and configuration registers.

These are the MMUs documented in Chapter 20 of the OMAP4430 TRM 
vX, right?  

I don't really understand the interaction of the hardreset lines with 
these IPs.  Chapter 20 doesn't seem to mention the PRCM-controllable 
hardreset lines at all.  It only mentions the OCP_SYSCONFIG registers 
associated with them.

Meanwhile Table 3-375 "RM_DSP_RSTCTRL" mentions a DSP MMU, cache, and 
slave interface reset line.  Is that referring to the same MMU that is 
mentioned in Chapter 20?  The end of Section 20.2 "MMU Integration" 
mentions two different DSP MMUs, a "shared cache MMU" and an "L2 MMU" - 
maybe the hardreset line only pertains to the first MMU?

> > > But then we do have the processor resets that have to be exposed
> > > somewhere.
> > > 
> > >      'ipu':
> > >          'rst_cpu0'
> > >          'rst_cpu1'
> > > 
> > >      'dsp':
> > >          'rst_dsp'
> > > 
> > >      'iva':
> > >          'rst_seq1'
> > >          'rst_seq2'
> > > 
> > > None of these one should be controlled automatically.
> > 
> > Don't we still want to put these IP blocks into reset until a driver for
> > these IP blocks is loaded?
> 
> Yes, indeed, my point is where are we going to declare these reset lines if we
> do not have any fake hwmods to store them.

Wouldn't we associate them with the processor hwmods?  e.g., 
omap44xx_iva_hwmod, omap44xx_dsp_hwmod ?

> Well, for the MMU we can, these are regular IPs (almost). The real issues was
> for the processors.

Omar, do you know  how these hardreset lines interact with the MMUs 
described in the TRM Chapter 20?

> First they will have to release their driver :-)

Hehe, indeed.


- Paul

Hi Paul,

On Thu, Apr 19, 2012 at 2:46 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi Benoît,
>
> On Thu, 19 Apr 2012, Cousson, Benoit wrote:
>
>> On 4/19/2012 7:17 PM, Paul Walmsley wrote:
>> > On Thu, 19 Apr 2012, Cousson, Benoit wrote:
>> >
>> > > The concern of the people doing SW in these embedded processors was mainly
>> > > because we were releasing the reset of processor without loading any SW
>> > > first
>> > > and thus the processor was executing some random instructions.
>> > >
>> > > So if we consider a better hwmods definition, we can potentially fix the
>> > > MMU
>> > > case:
>> > >
>> > >      'mmu_ipu':
>> > >          'rst_ipu_mmu_cache'
>> > >
>> > >      'mmu_dsp':
>> > >          'rst_dsp_mmu_cache'
>> > >
>> > >      'iva_config':
>> > >          'rst_logic'
>> >
>> > Wouldn't these still be "pseudo-hwmods?"  Or do each of these actually
>> > have address space, interconnect ports, etc.?
>>
>> Yes, these ones are the only real IPs for MPU stand point, with interconnect
>> port and configuration registers.
>
> These are the MMUs documented in Chapter 20 of the OMAP4430 TRM
> vX, right?
>
> I don't really understand the interaction of the hardreset lines with
> these IPs.  Chapter 20 doesn't seem to mention the PRCM-controllable
> hardreset lines at all.  It only mentions the OCP_SYSCONFIG registers
> associated with them.
>
> Meanwhile Table 3-375 "RM_DSP_RSTCTRL" mentions a DSP MMU, cache, and
> slave interface reset line.  Is that referring to the same MMU that is
> mentioned in Chapter 20?  The end of Section 20.2 "MMU Integration"
> mentions two different DSP MMUs, a "shared cache MMU" and an "L2 MMU" -
> maybe the hardreset line only pertains to the first MMU?

I don't thinks so, because trying to read the L2 MMU registers with
DSP RST2 asserted causes an L3 error.

DSP's "shared cache MMU" refers to section 5.3.2.5 Attribute MMU which
is inside the dsp megamodule, OTOH "L2 MMU" refers to section 5.3.4
MMU, the latter is the one being referenced in section 20.

>> > > But then we do have the processor resets that have to be exposed
>> > > somewhere.
>> > >
>> > >      'ipu':
>> > >          'rst_cpu0'
>> > >          'rst_cpu1'
>> > >
>> > >      'dsp':
>> > >          'rst_dsp'
>> > >
>> > >      'iva':
>> > >          'rst_seq1'
>> > >          'rst_seq2'
>> > >
>> > > None of these one should be controlled automatically.
>> >
>> > Don't we still want to put these IP blocks into reset until a driver for
>> > these IP blocks is loaded?
>>
>> Yes, indeed, my point is where are we going to declare these reset lines if we
>> do not have any fake hwmods to store them.
>
> Wouldn't we associate them with the processor hwmods?  e.g.,
> omap44xx_iva_hwmod, omap44xx_dsp_hwmod ?
>
>> Well, for the MMU we can, these are regular IPs (almost). The real issues was
>> for the processors.
>
> Omar, do you know  how these hardreset lines interact with the MMUs
> described in the TRM Chapter 20?

For M3, RM_MPU_M3_RSTCTRL[2].RST3 releases both the cache and mmu
interfaces, this one is needed if you want to program the mmu by
touching any of the MMU registers, typical initialization is to
deassert this line and then enable the M3 clock (since it is shared
with its MMU) which should allow the reset to complete *only* after
the clock has been enabled.

For DSP, RM_DSP_RSTCTRL[1].RST2 it also releases dsp, cache and mmu
(however the dsp doesn't boot until RST1 is deasserted), it is the
same rationale as above; you deassert RST2, enable dsp clock (so reset
can complete) and then program mmu.

Touching any mmu registers without deasserting its reset line (even if
the associated clock is ON) generates a L3 error.

This can be found in 3.5.6 Reset sequences for DSP and M3.

Regards,

Omar