Re: [PATCH V2] gpio: omap: refresh patch "be more aggressive withpm_runtime" against v3.12-rc5

[Chao Xu]

Sorry for the late reply.

On Fri, Nov 29, 2013 at 11:04 AM, Santosh Shilimkar
<santosh.shilimkar@xxxxxx> wrote:
> Adding Kevin's Linaro id, + Nishant,
>
> On Tuesday 26 November 2013 05:46 PM, Chao Xu wrote:
>> From: Felipe Balbi <balbi@xxxxxx>
>>
>> try to keep gpio block suspended as much as possible.
>>
>> Tested with pandaboard and a sysfs exported gpio.
>>
>> Signed-off-by: Felipe Balbi <balbi at ti.com>
>>
>> [caesarxuchao@xxxxxxxxx : Refreshed against v3.12-rc5, and added revision check to enable aggressive pm_runtime on OMAP4-only. Because am33xx_gpio_sysc.idlemodes seems to be wrongly marked as SIDLE_SMART_WKUP, which might cause missed interrupts with this patch. Tested on Pandaboard rev A2.]
>>
> Please format the text and limit it to 80 char rule.
>
Sorry. It's my first time submitting a patch. I will fix it and resubmit.
>> Signed-off-by: Chao Xu <caesarxuchao@xxxxxxxxx>
>> ---
>> According to Mr. Felipe Balbi, the original patch was not accepted because interrupts would be missed when GPIO was used as IRQ source. But in my tests with pandaboard, interrupts were not missed. This is because _idle_sysc() sets the idlemode of gpio module to smart-idle-wakeup, and according to OMAP4430 TRM, under this idlemode, the gpio can generate an asynchronous wakeup request to the PRCM. And after GPIO is awake, the wake-up request is reflected into the interrupt status registers.
>>
>> A change made on the original patch is only applying the aggressive runtime pm scheme on OMAP4, because I don’t have HW to test OMAP3 or am33xx devices. According to the respective TRMs, their GPIO modules also can generate wake-up request if the idlemode is set to smart-idle or smart-idle-wakeup. So the patch should work for them, too. But I suspect a potential SW bug may cause missed interrupts: the am33xx_gpio_sysc.idlemodes is marked as capable of SIDLE_SMART_WKUP in omap_hwmod_33xx.c. But according to AM335x TRM, _only_ gpio0 is capable of this mode. This may make GPIO stuck in force-idle mode and unable to generate wakeup request. And thus interrupt will be missed. Again, I don’t have the HW to verify whether this is a bug or not :(
>>
> In general I am not against this idea but patch has many assumptions which
> are not entirely correct.
>
> - SMART_WAKEUP will take care of waking IP etc not always true to power
> domain getting into idle.
>
I agree that if the power domain goes to idle, SMART_WAKEUP
won't be effective. But, correct me if i'm wrong, even if we don't call
runtime_put(), the power domain can still go to idle. Because the
modulemode of GPIO is set to HW_AUTO in this case, which won't
prevent the power domain goes to retention. So this patch doesn't
make the situation worse.
> - If we want to go on this path then I would like to see we address
> omap2_gpio_[prepare/resumne]_for_idle()
>
I did a quick google search but didn't find the problem with the two
functions. Could you give me a pointer here?
> - GPIO though sound simple is one of the most notorious IP block
> on PM front so this needs lot of testing. This patch not seems be
> tested at all so I would have much liked it to be in RFC/RFT
> state.
I tested using a gpio pin as interrupt source after applying the patch.
What are the other tests that I should do? Is there a formal way of
reporting test results here?
>
>>  drivers/gpio/gpio-omap.c                |  103 ++++++++++++++++++++++++++-----
>>  include/linux/platform_data/gpio-omap.h |    1 +
>>  2 files changed, 87 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 89675f8..90661f2 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -76,6 +76,7 @@ struct gpio_bank {
>>       int context_loss_count;
>>       int power_mode;
>>       bool workaround_enabled;
>> +     bool is_aggressive_pm;
>>
>>       void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
>>       int (*get_context_loss_count)(struct device *dev);
>> @@ -473,8 +474,15 @@ static void _disable_gpio_module(struct gpio_bank *bank, unsigned offset)
>>  static int gpio_is_input(struct gpio_bank *bank, int mask)
>>  {
>>       void __iomem *reg = bank->base + bank->regs->direction;
>> +     u32 val;
>>
>> -     return __raw_readl(reg) & mask;
>> +     if (bank->is_aggressive_pm)
>> +             pm_runtime_get_sync(bank->dev);
>> +     val = __raw_readl(reg) & mask;
>> +     if (bank->is_aggressive_pm)
>> +             pm_runtime_put(bank->dev);
>> +
> Move above in some wrapper instead of sprinkling the
> 'is_aggressive_pm' check everywhere.
>
Will do.
>> @@ -1585,18 +1651,21 @@ static const struct omap_gpio_platform_data omap2_pdata = {
>>       .regs = &omap2_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = false,
>> +     .is_aggressive_pm = false,
>>  };
>>
>>  static const struct omap_gpio_platform_data omap3_pdata = {
>>       .regs = &omap2_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = true,
>> +     .is_aggressive_pm = false,
>>  };
>>
>>  static const struct omap_gpio_platform_data omap4_pdata = {
>>       .regs = &omap4_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = true,
>> +     .is_aggressive_pm = true,
>>  };
>>
> Well OMAP IP shouldn't have different behavior on OMAP3 and
> OMAP4 at least so something you can enable for OMAP4, you
> should be able to do it for OMAP3 as well.
>
the am33xx_gpio_sysc.idlemodes is marked as capable of
SMART_WKUP in omap_hwmod_33xx.c. But according to TRM,
only gpio0 is capable of this. So i suspect the patch won't work
for omap3. I don't have the hardware to verify this. Could someone
verify it? Thank you.
> Kevin might have some more concerns to add.
>
> Regards,
> Santosh
>



-- 
Regards,
Chao Xu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/