On Fri, Oct 14, 2011 at 2:31 AM, Kevin Hilman <khilman@xxxxxx> wrote: > Govindraj <govindraj.ti@xxxxxxxxx> writes: > >> On Thu, Oct 13, 2011 at 5:30 AM, Kevin Hilman <khilman@xxxxxx> wrote: >>> Govindraj <govindraj.ti@xxxxxxxxx> writes: >>> >>>> On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@xxxxxx> wrote: >>>>> "Govindraj.R" <govindraj.raja@xxxxxx> writes: >>>>> >>>>>> For the early console probing we had avoided hwmod reset and idling >>>>>> and uart was idled using hwmod API and enabled back using omap_device API >>>>>> after omap_device registration. >>>>>> >>>>>> Now since we are using runtime API's to enable back uart, move hwmod >>>>>> idling and use runtime API to enable back UART. >>>>>> >>>>>> Signed-off-by: Govindraj.R <govindraj.raja@xxxxxx> >>>>> >>>>> Now that the driver is using runtime PM. Why do we still need >>>>> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET? >>>>> >>>>> The comment in the code says: >>>>> >>>>> /* >>>>> * During UART early init, device need to be probed >>>>> * to determine SoC specific init before omap_device >>>>> * is ready. Therefore, don't allow idle here >>>>> */ >>>>> >>>>> This was true when using the 8250 driver because it was not using >>>>> runtime PM so could not know how to (re)enable the device. >>>>> >>>>> However, since the driver is now runtime PM adapted, any device access >>>>> should be contained within a runtime PM get/put block, so there should >>>>> no longer be a reason not allow the IP blocks to be reset during boot. >>>>> >>>> >>>> Forgot to add, this is still needed for >>>> earlyprintk(CONFIG_EARLY_PRINTK) use case, >>> >>> Ah, right. I forgot about that. Please update the changelog (and >>> comment in the code) to reflect that. >>> >>>> The initial boot prints until a console driver is available is from >>>> "arch/arm/kernel/early_printk.c" which does a tx on uart console >>>> and relies on configuration from bootloader. >>>> >>>> during bootup earlyprink does a tx on uart console and if uart driver >>>> is not available yet >>>> uart reset or idle done by hwmod layer can cause boot failures. >>>> >>>> --> put_char from earlyprintk.c >>>> --> reset/idle from hwmod layer >>>> --> put_char from earlyprintk.c >>>> >>>> >>>> So console_uart reset or clock gating must be done only after uart >>>> driver is available or be prevented using these available hwmod_flags. >>> >>> So why not leave the driver out of it and solve it like the current code >>> does? >>> >>> The current codes use the hwmod flags, then waits until the UART driver >>> is available (after omap_device_build) and uses omap_hwmod_idle() to do >>> an clean idle of the device. >>> >>> Notably this is inside a console_lock/console_unlock block so that >>> prints are buffered. >>> >>> The current code then does an omap_device_enable() to re-enable the >>> device, but you shouldn't need that after the driver is converted to >>> runtime PM. >> >> Yes similar approach here, We are not doing hwmod idle >> until console driver is available, once omap-serial is available >> from probe doing hwmod_idle* and then get_sync. > >> hwmod idle in serial.c will still cause problems if ealryprintk tries >> to print until omap-uart console driver is not available, > > It will try, but note that current code takes the console_lock() during > that time, so those prints will be buffered. > >> as now with rumtime adaptation only driver can enable back clocks. > > I'm not sure why you're calling this "enable back clocks." This patch > is just trying to decide where to idle the hwmod (that is, disable the > clocks.) > Yes correct, its about where exactly to do hwmod_idle for avoiding hmwod_reset/idle from boot up. > Re: only the driver can do it, I can think of at least 2 ways to keep > this out of the driver. > I did some study into the approaches you suggested, Below are my observation, Please correct me if I missed out any thing in my below observation, > 1) Use the a custom activate_func in the omap_device pm_lats struct > to idle the first time. > same activate func gets called for all uarts, how to determine the activate func. was called for first time for the given uart. (use od->pdev->dev.name strcmp with uart name for first name and maintain a state machine ? confused) Then should we have different activate funcs? > 2) Use a bus notifier so the device init can be notified when the > real driver is available. I think you're probably wanting > the BUS_NOTIFY_BIND_DRIVER event, which would happen right > before probe. There's also BUS_NOTIFY_BOUND_DRIVER which > happens right after probe. You might actually want to use > both. e.g. console_lock(); omap_hwmod_idle() in BIND > and console_unlock() in 'BOUND'. > bus_register_notifier is for all drivers within the bus, omap-uart is registered as platform bus and there a lot more devices that register under platform bus, So registering the notifier for platform bus will call notifier for all probes withing platform bus, Is there a way to have notifier per device or given device? Since bus notifier seems to be notify for all devices registered within the given bus. -- Thanks, Govindraj.R >> So have added a function pointer to pdata which calls hwmod_idle >> implemented in serial.c calling omap_hwmod_idle. > > Yes, I saw that in the patch, and that's what I don't like. > > Fixing up after earlyprintk is not the responsiblity of the driver, and > the driver should not know or care whether earlyprintk was or wasn't > used before it was loaded. This needs to be handled in device init code > as it is today. > > 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
