Govindraj <govindraj.ti@xxxxxxxxx> writes:
> On Fri, Sep 9, 2011 at 11:44 PM, Kevin Hilman <khilman@xxxxxx> wrote:
>> Govindraj <govindraj.ti@xxxxxxxxx> writes:
>>
>>> On Fri, Sep 9, 2011 at 5:54 AM, Kevin Hilman <khilman@xxxxxx> wrote:
>>>> "Govindraj.R" <govindraj.raja@xxxxxx> writes:
>>>>
>>>>> Adapts omap-serial driver to use pm_runtime API's.
>>>>>
>>>>> 1.) Moving context_restore func to driver from serial.c
>>>>> 2.) Use runtime irq safe to use get_sync from irq context.
>>>>> 3.) autosuspend for port specific activities and put for reg access.
>>>>
>>>> Re: autosuspend, it looks like the driver now has 2 mechanisms for
>>>> tracking activity. The runtime PM 'mark last busy' and the existing
>>>> 'up->port_activity = jiffies' stuff. Do you think those could be
>>>> unified?
>>>>
>>>
>>> Is there a way where we can retrieve the last busy jiffie value
>>> using runtime API? I don't find that available.
>>>
>>
>> Not currently. The question is more along the lines of: what is the
>> port_activity jiffies used for, and can it be replaced by using
>> _mark_last_busy().
>>
>> If it cannot, that's fine, but it should be described.
>
> Okay.
>
>>
>>>> At first glance, it looks like most places with a _mark_last_busy() also
>>>> have a up->port_activity update. I'm not familiar enough with the
>>>> driver to make the call, but they look awfully similar.
>>>>
>>>
>>> Ok, I will check whether I can get rid if it.
>>>
>>>>> 4.) for earlyprintk usage the debug macro will write to console uart
>>>>> so keep uart clocks active from boot, idle using hwmod API's re-enable back
>>>>> using runtime API's.
>>>>
>>>> /me doesn't understand
>>>
>>> I have added this reason in early mail reply to 04/11 patch review
>>> [needed for earlyprintk option from low level debug ]
>>
>> OK, but AFAIK, DEBUG_LL + earlyprintk do not use this driver. Instead
>> they use the debug macros, so any hacks to deal with that do not belong
>> in the driver.
>>
>> If there are hacks required, they should be in a separate patch, with a
>> separate descriptive changelog.
>>
>
> Yes these are required, because early printk relies on clocks from bootloader
> and if clocks are gated even before console_driver is available it can
> lead to boot failures.
My point is that hacks/workaround for DEBUG_LL + earlyprintk don't
belong in this driver, since this driver is not involved.
> I can keep that part as a separate patch.
Yes please.
>>>>
>>>>> 5.) Moving context restore to runtime suspend and using reg values from port
>>>>> structure to restore the uart port context based on context_loss_count.
>>>>> Maintain internal state machine for avoiding repeated enable/disable of
>>>>> uart port wakeup mechanism.
>>>>> 6.) Add API to enable wakeup and check wakeup status.
>>>>>
>>>>> Acked-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Govindraj.R <govindraj.raja@xxxxxx>
>>>>> ---
>>>>> arch/arm/mach-omap2/serial.c | 49 ++++++
>>>>> arch/arm/plat-omap/include/plat/omap-serial.h | 10 ++
>>>>> drivers/tty/serial/omap-serial.c | 211 ++++++++++++++++++++++---
>>>>> 3 files changed, 250 insertions(+), 20 deletions(-)
>>>>>
>>>
>>> [..]
>>>
>>>>> +
>>>>> +static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
>>>>> +{
>>>>> + struct omap_device *od;
>>>>> + struct omap_uart_port_info *up = pdev->dev.platform_data;
>>>>> +
>>>>> + /* Set or clear wake-enable bit */
>>>>> + if (up->wk_en && up->wk_mask) {
>>>>> + u32 v = __raw_readl(up->wk_en);
>>>>> + if (enable)
>>>>> + v |= up->wk_mask;
>>>>> + else
>>>>> + v &= ~up->wk_mask;
>>>>> + __raw_writel(v, up->wk_en);
>>>>> + }
>>>>> +
>>>>> + od = to_omap_device(pdev);
>>>>> + if (enable)
>>>>> + omap_hwmod_enable_wakeup(od->hwmods[0]);
>>>>> + else
>>>>> + omap_hwmod_disable_wakeup(od->hwmods[0]);
>>>>> +}
>>>>> +
>>>>
>>>> Here again, this is difficult to review because you removed the code in
>>>> [4/11] and add it back here, but with rather different functionality
>>>> with no description as to why.
>>>>
>>>
>>> will move this change as part of 4/11 itself.
>>>
>>
>> The point was not just that the move was confusing, but that you changed
>> the functionality along with the move without any description.
>>
>
> If are referring to uart rx pad wakeup its still retained but done
> using mux framework.
OK, let me try this a slightly different way. Here's what you did:
- remove some code in patch 4
- add it back in patch 7, but in different form (with no description of diffs)
- the missing parts of original code are not documented
- moved code has some new (but undocumented) features (module-level wakeup)
- turns out the missing parts (IO pad wakeups) are done using a
different framework in yet another patch (which would be fine, if it
the reviewers were given a hint.)
I hope this is enough to demonstrate that trying to decipher what what
you meant to do, what you did, and what you didn't do is extremely
time consuming for a reviewer.
Well-constructed patches, broken up into *logical* chunks with
descriptive changelogs are invaluable to an efficient review process.
>>>> The previous version enabled wakeups at the PRCM and at the IO ring.
>>>> This version enables wakeups at the PRCM as well but instead of enabling
>>>> them at the IO ring, enables them at the module.
>>>>
>>>
>>> Since we are gating using prepare idle call I think
>>> we can use this enable wake-up call as part of prepare idle call itself,
>>> as done earlier.
>>
>> Not sure what that has to with my comment.
>>
>> In moving this code, you removed the enable/disable of IO ring wakeups.
>> It probably continues to work because they're enabled by the bootloader,
>> but that is not correct and the driver should be managing the IO ring
>> wakeups as before.
>>
>
> Prior to this we used io-pad offsets address and modified to enable wakeup
> mechanism, But since we received some comments from Tony to use mux framework
> for any mux changes, I removed using mux address and use mux framework from
> hwmod, I have not removed rx-pad wakeup still enable io-pad wakeup.
> otherwise wakeup from off-mode will fail on 3430SDP.
>
> omap_uart_wakeup_enable
> --> omap_hwmod_enable_wakeup
> --> omap_hwmod_enable_ioring_wakeup [PATCH v4 01/11]
> OMAP2+: hwmod: Add API to enable IO ring wakeup.]
>
> boot-loaders doesn't enable io pad wakeup for uart.
>
>
>> [...]
>>
>>>>> }
>>>>>
>>>>> static int __init
>>>>> @@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
>>>>> .cons = OMAP_CONSOLE,
>>>>> };
>>>>>
>>>>> -static int
>>>>> -serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
>>>>> +static int serial_omap_suspend(struct device *dev)
>>>>> {
>>>>> - struct uart_omap_port *up = platform_get_drvdata(pdev);
>>>>> + struct uart_omap_port *up = dev_get_drvdata(dev);
>>>>>
>>>>> - if (up)
>>>>> + if (up) {
>>>>> uart_suspend_port(&serial_omap_reg, &up->port);
>>>>> + if (up->port.line == up->port.cons->index &&
>>>>> + !is_console_locked())
>>>>> + up->console_locked = console_trylock();
>>>>> + }
>>>>> +
>>>>
>>>> Motiviation for console locking in this version of the series is not
>>>> clear, and not described in the changelog.
>>>>
>>>> It's also present here in static suspend/resume but not in runtime
>>>> suspend/resume, which also is suspicious.
>>>>
>>>
>>> Yes will add to change log,
>>>
>>> We needed this for no console suspend use case
>>> no console lock is taken in no_console_suspend is specified,
>>>
>>> Since our pwr_dmn hooks disable clocks left enabled during suspend,
>>> so any prints after pwr_dmn hooks can cause problems since clocks
>>> are already cut from pwr_dmn hooks.
>>>
>>> So buffer prints while entering suspend by taking console lock
>>> if not taken already and print back after uart state machine restores
>>> console uart.
>>
>> Any special consideration for features like no_console_suspend should be
>> done in a separate patch with a separate changelog.
>>
>
> will add as separate patch.
yes please.
>> However, as I've stated before during previous reviews, if someone has
>> put no_console_suspend on the command line, that means they've
>> specifically stated they *want* to see console writes during suspend.
>> That means, we should not cut clocks at all.
>>
>> Of course, that means the system will not hit the low power state becase
>> the UART clocks are not gated, but that is what the user requested.
>
> Yes correct, But pwr domain callback will now cut all enabled clocks
> now, so only available method was to use console_lock and print later.
>
> pwr_domain callback done in omap-device.c
> will force to low powers state by gating all clocks that where left
> enabled by driver.
These are the kinds of things that belong in a descriptive changelog.
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
