Re: [PATCH] ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

[Rajendra Nayak <rnayak@ti.com>]

On Thursday 17 November 2011 04:09 PM, Cousson, Benoit wrote:
> + Paul
>
> On 11/17/2011 11:11 AM, Rajendra Nayak wrote:
>> A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in
>> enabled state by the hwmod framework post the initial setup.
>> Once a real user of the device (a driver) tries to enable it
>> at a later point, the hmwod framework throws a WARN() about
>
> typo: -------------------^
>
>> the device being already in enabled state.
>>
>> Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT'
>> to identify such devices/hwmods, so nothing but just a state
>> change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod
>> is requested to be enabled (the first time) by its driver/user.
>>
>> A good example of a such a device is an UART used as debug console.
>> The UART module needs to be kept enabled through the boot, until the
>> UART driver takes control of it, for debug prints to appear on
>> the console.
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>> ---
>> arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++++--
>> arch/arm/plat-omap/include/plat/omap_hwmod.h | 1 +
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
>> b/arch/arm/mach-omap2/omap_hwmod.c
>> index d7f4623..7d94cc3 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
>>
>> pr_debug("omap_hwmod: %s: enabling\n", oh->name);
>>
>> + /*
>> + * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left
>
> Is the ' needed here? But I'm not an expert in English grammar...

Me neither, lets ask Paul :)

>
>> + * in enabled state at init.
>> + * Now that someone is really trying to enable them,
>> + * just update the state.
>> + */
>> + if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
>> + oh->_state = _HWMOD_STATE_ENABLED;
>> + return 0;
>> + }
>> +
>> if (oh->_state != _HWMOD_STATE_INITIALIZED&&
>> oh->_state != _HWMOD_STATE_IDLE&&
>> oh->_state != _HWMOD_STATE_DISABLED) {
>> @@ -1449,7 +1460,6 @@ static int _enable(struct omap_hwmod *oh)
>> return -EINVAL;
>> }
>>
>> -
>
> Not related to the patch changelog but that make senses to do it, so
> just update the changelog.

No, its just a stay change. I'll get rid of it.

>
>> /*
>> * If an IP contains only one HW reset line, then de-assert it in order
>> * to allow the module state transition. Otherwise the PRCM will return
>> @@ -1744,8 +1754,10 @@ static int _setup(struct omap_hwmod *oh, void
>> *data)
>> * it should be set by the core code as a runtime flag during startup
>> */
>> if ((oh->flags& HWMOD_INIT_NO_IDLE)&&
>> - (postsetup_state == _HWMOD_STATE_IDLE))
>> + (postsetup_state == _HWMOD_STATE_IDLE)) {
>> + oh->_state = _HWMOD_STATE_ENABLED_AT_INIT;
>> postsetup_state = _HWMOD_STATE_ENABLED;
>> + }
>>
>> if (postsetup_state == _HWMOD_STATE_IDLE)
>> _idle(oh);
>> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> index 1b81dfb..cd49d60 100644
>> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> @@ -436,6 +436,7 @@ struct omap_hwmod_omap4_prcm {
>> #define _HWMOD_STATE_ENABLED 4
>> #define _HWMOD_STATE_IDLE 5
>> #define _HWMOD_STATE_DISABLED 6
>> +#define _HWMOD_STATE_ENABLED_AT_INIT 7
>
> You should update the comment before... yeah I know, the other states
> are not documented, but they are much more understandable than this new
> one.

Yeah, makes sense. Will add comments.

>
>> /**
>> * struct omap_hwmod_class - the type of an IP block
>
> +1 Looks good to me but someone else must approve...
> Oops, sorry its not Gerrit :-)
>
> Acked-by: Benoit Cousson <b-cousson@ti.com>
>
> Thanks,
> Benoit