On Sat, Mar 5, 2011 at 5:24 AM, Kevin Hilman <khilman@xxxxxx> wrote:
> Hi Govindraj,
>
> "Govindraj.R" <govindraj.raja@xxxxxx> writes:
>
>> For device pads which have OMAP_DEVICE_PAD_WAKEUP set (which means they
>> are wakeup capable) enable the IO-daisy wakeup capability.
>> During re-muxing avoid direct write with val as this can disturb if any
>> mux done at bootloader level so read the pad then write back.
>>
>> Also add an API to fetch the wake-up status bit is set for given omap-hwmod
>> device using available mux info which is added during omap_hwmod_mux_init.
>> Wakeup status bit is needed for uart_resume_idle call from sram_idle path
>> based on wake-up event has occurred for given uart we can enable clocks back.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@xxxxxx>
>> Signed-off-by: Govindraj.R <govindraj.raja@xxxxxx>
>> ---
>> arch/arm/mach-omap2/mux.c | 28 ++++++++++++++++++++++++++
>> arch/arm/mach-omap2/mux.h | 13 ++++++++++++
>> arch/arm/mach-omap2/omap_hwmod.c | 13 ++++++++++++
>> arch/arm/plat-omap/include/plat/omap_hwmod.h | 1 +
>> 4 files changed, 55 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/mux.c b/arch/arm/mach-omap2/mux.c
>> index 98148b6..50edcea 100644
>> --- a/arch/arm/mach-omap2/mux.c
>> +++ b/arch/arm/mach-omap2/mux.c
>> @@ -317,6 +317,29 @@ err1:
>> return NULL;
>> }
>>
>> +/* Gets the wakeup status of given pad from omap-hwmod */
>> +int omap_hwmod_mux_wakeup(struct omap_hwmod_mux_info *hmux)
>
> should return a bool
>
> Also, documentation should be clear that it returns true if *any* pad
> associated with this device has a wakeup event.
>
> Also a name reflecting the get/check/status nature of this function
> would help readability, maybe:
>
> bool omap_hwmod_mux_wakeup_event(struct omap_hwmod_mux_info *hmux)
Yes sure will modify.
>
>> +{
>> + int i;
>> + unsigned int val;
>> + u8 ret = 0;
>> +
>> + for (i = 0; i < hmux->nr_pads; i++) {
>> + struct omap_device_pad *pad = &hmux->pads[i];
>> +
>> + if (pad->flags & OMAP_DEVICE_PAD_WAKEUP) {
>> + val = omap_mux_read(pad->partition,
>> + pad->mux->reg_offset);
>> + if (val & OMAP_WAKEUP_EVENT) {
>> + ret = 1;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> /* Assumes the calling function takes care of locking */
>> void omap_hwmod_mux(struct omap_hwmod_mux_info *hmux, u8 state)
>> {
>> @@ -342,6 +365,9 @@ void omap_hwmod_mux(struct omap_hwmod_mux_info *hmux, u8 state)
>> break;
>> flags &= ~OMAP_DEVICE_PAD_ENABLED;
>> val = pad->idle;
>> + if (flags & OMAP_DEVICE_PAD_WAKEUP)
>> + val |= OMAP_WAKEUP_EN;
>> +
>
> Is this needed on every idle transition?
>
> You're currently setting it on every idle transition, but never clearing
> it. If it is to be a one-time thing, then move it to the early init of
> the mux.
I will be removing this as said in earlier mail.
>
>> pr_debug("%s: Idling %s %x\n", __func__,
>> pad->name, val);
>> break;
>> @@ -358,6 +384,8 @@ void omap_hwmod_mux(struct omap_hwmod_mux_info *hmux, u8 state)
>> };
>>
>> if (val >= 0) {
>> + val |= omap_mux_read(pad->partition,
>> + pad->mux->reg_offset);
>
> I don't think this is doing what you expect, and will lead to very
> difficult to find bugs. Since your OR'ing in bits from the current
> value, you will never be able to clear any bits in this register.
>
> As just a dumb example, consider the enable case where
> pad->enable=MUX_MODE0 (0x0). If the bootloader has initialized this pad
> to MUX_MODE7 (0x7), this code will never set it to MUX_MODE0, since this
> read and OR of the existing value will always set it back to MUX_MODE7.
>
> At a minimum, this change should be a separate patch with a more
> detailed description.
>
Also this is change will be removed and default serial_pad
reg will have proper values for .enable and .idle fields using
which it will muxed accordingly
[snip of default rx serial reg given in previous mail.]
>
>> omap_mux_write(pad->partition, val,
>> pad->mux->reg_offset);
>> pad->flags = flags;
>> diff --git a/arch/arm/mach-omap2/mux.h b/arch/arm/mach-omap2/mux.h
>> index a4ab17a..84a8fc2 100644
>> --- a/arch/arm/mach-omap2/mux.h
>> +++ b/arch/arm/mach-omap2/mux.h
>> @@ -220,8 +220,21 @@ omap_hwmod_mux_init(struct omap_device_pad *bpads, int nr_pads);
>> */
>> void omap_hwmod_mux(struct omap_hwmod_mux_info *hmux, u8 state);
>>
>> +
>> +/**
>> + * omap_hwmod_mux_wakeup - omap hwmod check given pad had wakeup event
>> + * @hmux: Pads for a hwmod
>> + *
>> + * Called only from omap_hwmod.c, do not use.
>> + */
>> +int omap_hwmod_mux_wakeup(struct omap_hwmod_mux_info *hmux);
>> #else
>>
>> +static inline int omap_hwmod_mux_wakeup(struct omap_hwmod_mux_info *hmux)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int omap_mux_init_gpio(int gpio, int val)
>> {
>> return 0;
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>> index 9e89a58..893dae1 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -2239,3 +2239,16 @@ u32 omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
>>
>> return ret;
>> }
>> +
>> +/**
>> + * omap_hwmod_pad_wakeup_status - get pad wakeup status if mux is available.
>> + * @oh: struct omap_hwmod *
>> + *
>> + * Returns the wake_up status bit of available pad mux pin.
>> + */
>> +int omap_hmwod_pad_wakeup_status(struct omap_hwmod *oh)
>
> Again, naming here could improve readability. Maybe:
>
> bool omap_hmwod_get_io_wakeup_status(struct omap_hwmod *oh)
>
> From the name and the bool return, it should be clear that this is just
> checking for the wakeup event. I also think it should be clear that
> this is checking for IO-ring wakeups, not module wakeups.
>
> Also, in PATCH 5/7, you're calling this hwmod API directly from a
> driver. Drivers should not know anything about hwmods. Any
> OMAP-specific hooks must be called through pdata function pointers.
> Also, they should go through omap_device, which would then call
> omap_hwmod.
>
Agree omap-device level api to pad wakeup check is better.
> Thinking more about this (and how you use it in PATCH 5/7), what is
> needed is an omap_device level API to check if a wakeup occured for that
> device. That function would check for either module-level wakeup or IO
> pad wakeup. The driver should not need to care about how the wakeup
> occurred.
>
Module level wakeup doesn't seem to work and only
way it seem to wakeup is through using resume_idle
from sram_idle.
One simple way to reproduce the problem with
existing code base also is adding this patch which
will enable module level wakeup always.
https://patchwork.kernel.org/patch/501211/
and with below change.
govindraj@Linux-BSP-Server:~/clones/linux-omap-2.6$ gd
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 47eef48..6343773 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -380,6 +380,7 @@ static void omap_uart_allow_sleep(struct
omap_uart_state *uart)
omap_uart_smart_idle_enable(uart, 1);
uart->can_sleep = 1;
del_timer(&uart->timer);
+ omap_uart_disable_clocks(uart);
}
We can see module level wakeup doesn't wakeup the uart.
once we set sleep_timeout for uart and timer starts.
>> +{
>> + if (oh->mux)
>> + return omap_hwmod_mux_wakeup(oh->mux);
>> + return -EINVAL;
>> +}
>> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> index fedd829..4100be0 100644
>> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> @@ -588,6 +588,7 @@ int omap_hwmod_for_each_by_class(const char *classname,
>> int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
>> u32 omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
>>
>> +int omap_hmwod_pad_wakeup_status(struct omap_hwmod *oh);
>> /*
>> * Chip variant-specific hwmod init routines - XXX should be converted
>> * to use initcalls once the initial boot ordering is straightened out
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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
