"ext Paul Walmsley" <paul@xxxxxxxxx> writes:
> Hello Tero, Jouni,
>
> so is this basically a workaround patch?
Yes in new format. Please do not use word workaround;)
>
> a few comments:
>
> On Wed, 27 Aug 2008, Tero Kristo wrote:
>
>> From: ext Jouni Hogander <jouni.hogander@xxxxxxxxx>
>>
>> In omap3 gpios 2-6 are in per domain. Functional clocks for these
>> should be disabled.
>>
>> GPIO modules in PER domain are not able to act as a wake up source if
>> PER domain is in retention. PER domain sleep transition before MPU is
>> prevented by leaving icks active. PER domain still enters retention
>> together with MPU. When this happens IOPAD wake up mechanism is used
>> for gpios.
>>
>> Signed-off-by: Jouni Hogander <jouni.hogander@xxxxxxxxx>
>> ---
>> arch/arm/mach-omap2/pm.c | 20 ++++++++------
>> arch/arm/mach-omap2/pm.h | 2 +-
>> arch/arm/mach-omap2/pm34xx.c | 56 +++++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 67 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
>> index 4652136..1de5f14 100644
>> --- a/arch/arm/mach-omap2/pm.c
>> +++ b/arch/arm/mach-omap2/pm.c
>> @@ -32,7 +32,7 @@
>> #include "pm.h"
>>
>> unsigned short enable_dyn_sleep;
>> -unsigned short clocks_off_while_idle;
>> +unsigned short gpio_clocks_off_while_idle;
>> atomic_t sleep_block = ATOMIC_INIT(0);
>>
>> static ssize_t idle_show(struct kobject *, struct kobj_attribute *, char *);
>> @@ -42,16 +42,16 @@ static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
>> static struct kobj_attribute sleep_while_idle_attr =
>> __ATTR(sleep_while_idle, 0644, idle_show, idle_store);
>>
>> -static struct kobj_attribute clocks_off_while_idle_attr =
>> - __ATTR(clocks_off_while_idle, 0644, idle_show, idle_store);
>> +static struct kobj_attribute gpio_clocks_off_while_idle_attr =
>> + __ATTR(gpio_clocks_off_while_idle, 0644, idle_show, idle_store);
>>
>> static ssize_t idle_show(struct kobject *kobj, struct kobj_attribute *attr,
>> char *buf)
>> {
>> if (attr == &sleep_while_idle_attr)
>> return sprintf(buf, "%hu\n", enable_dyn_sleep);
>> - else if (attr == &clocks_off_while_idle_attr)
>> - return sprintf(buf, "%hu\n", clocks_off_while_idle);
>> + else if (attr == &gpio_clocks_off_while_idle_attr)
>> + return sprintf(buf, "%hu\n", gpio_clocks_off_while_idle);
>> else
>> return -EINVAL;
>> }
>> @@ -69,8 +69,8 @@ static ssize_t idle_store(struct kobject *kobj, struct kobj_attribute *attr,
>>
>> if (attr == &sleep_while_idle_attr)
>> enable_dyn_sleep = value;
>> - else if (attr == &clocks_off_while_idle_attr)
>> - clocks_off_while_idle = value;
>> + else if (attr == &gpio_clocks_off_while_idle_attr)
>> + gpio_clocks_off_while_idle = value;
>> else
>> return -EINVAL;
>>
>> @@ -106,10 +106,12 @@ static int __init omap_pm_init(void)
>> /* disabled till drivers are fixed */
>> enable_dyn_sleep = 0;
>> error = sysfs_create_file(power_kobj, &sleep_while_idle_attr.attr);
>> - if (error)
>> + if (error) {
>> printk(KERN_ERR "sysfs_create_file failed: %d\n", error);
>> + return error;
>> + }
>> error = sysfs_create_file(power_kobj,
>> - &clocks_off_while_idle_attr.attr);
>> + &gpio_clocks_off_while_idle_attr.attr);
>> if (error)
>> printk(KERN_ERR "sysfs_create_file failed: %d\n", error);
>>
>> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
>> index 68c9278..d446ea4 100644
>> --- a/arch/arm/mach-omap2/pm.h
>> +++ b/arch/arm/mach-omap2/pm.h
>> @@ -17,7 +17,7 @@ extern int omap2_pm_init(void);
>> extern int omap3_pm_init(void);
>>
>> extern unsigned short enable_dyn_sleep;
>> -extern unsigned short clocks_off_while_idle;
>> +extern unsigned short gpio_clocks_off_while_idle;
>> extern atomic_t sleep_block;
>>
>> extern void omap2_block_sleep(void);
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index a16eb33..0baf359 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -53,6 +53,43 @@ static void (*saved_idle)(void);
>>
>> static struct powerdomain *mpu_pwrdm;
>>
>> +/* Dynamic GPIO clock handling for sleep routines.
>> + * omap_sram_idle() will call these functions and they will dynamically
>> + * enable / disable GPIO clocks to allow sleep transitions. */
>
> Please use CodingStyle format for multiple-line comments, e.g.,
Yes, needs to be fixed...
>
> /*
> * Dynamic GPIO clock handling ...
> * etc. etc. etc.
> */
>
> This also applies to several other comments later in the file.
ok
>
>> +#define NUM_OF_PERGPIOS 5
>> +static struct clk *gpio_fcks[NUM_OF_PERGPIOS];
>> +
>> +/* Enable GPIO clocks from sleep routines if allowed */
>> +static void per_gpio_clk_enable(void)
>> +{
>> + int i;
>> +
>> + if (gpio_clocks_off_while_idle == 0)
>> + return;
>> + for (i = 1; i < NUM_OF_PERGPIOS + 1; i++)
>> + clk_enable(gpio_fcks[i-1]);
>
> Why not just:
>
> for (i = 0; i < NUM_OF_PERGPIOS; i++)
> clk_enable(gpio_fcks[i]);
>
> ?
Yes, these all needs to be fixed.
Thank you for you comments.
--
Jouni Högander
--
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
