"Cousson, Benoit" <b-cousson@xxxxxx> writes: > Hi Kevin and Rajendra, > > On 11/18/2011 7:44 AM, Rajendra Nayak wrote: >> Hi Kevin, >> >> On Friday 18 November 2011 01:05 AM, Kevin Hilman wrote: >>> Rajendra Nayak<rnayak@xxxxxx> writes: >>> >>>> 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 >>>> 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. >>> >>> Nice. This is indeed much cleaner than what we're doing in the UART >>> code. However... >>> >>>> Signed-off-by: Rajendra Nayak<rnayak@xxxxxx> >>>> --- >>>> 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 >>> >>> (no the ' isn't necessary) >>> >>>> + * 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; >>>> + } >>> >>> ...this subtly changes the behavior, at least compared to how the UART >>> code handles this today. >>> >>> One thing that this doesn't do that the current UART code does is ensure >>> that the IP is actually in a state that can properly idle after this is >>> done. >>> >>> For example, if the bootloader is dumb (most are) and has configured the >>> UARTs in mode that prevents idle (e.g. no-idle mode), then these UARTs >>> will never allow the SoC to hit low power states. > > That will not happen because we already did the complete enable > sequence during setup phase that will set the sysconfig properly. > So the device is properly initialized, we just skipped the idle phase > because we wanted to keep the device operational for the console. Ah, perfect. >>> So, what's really needed is not just a return here, but an _idle() and >>> then continue so we know that the HW is in a state that we know can >>> idle from here on out. > > We are in the enable call, so the driver / omap_device does expect the > IP to be operational at the moment, we cannot put it into idle. > >> I don't know if I completely understand what you are saying. >> The above change is in the _enable() call which would be triggered >> as part of runtime call from the serial driver to enable the module. >> (Since the module is already enabled, you just change state to suggest >> its now enabled by its user, rather than 'left enabled' at init) >> There would be another call from the serial driver to idle it when >> a corresponding hwmod _idle() call would idle the device. >> I don't seem to understand why would a call to _idle() be needed >> in an _enable() call. > > Yep, I agree. I think Kevin's point is due to the previous sequence > used by the uart: > > + omap_hwmod_idle(od->hwmods[0]); > + omap_hwmod_enable(od->hwmods[0]); > > But that sequence is not needed for my point of view. The device is > already properly enabled. I don't think it was even needed in the > previous code. The hwmod was already enabled properly, only the > omap_device was disabled. So the original omap_console_hwmod_enable > should just do nothing. OK, thanks for the clarification. In that case, this patch Acked-by: Kevin Hilman <khilman@xxxxxx> Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html