| Message ID | 1328203851-20435-1-git-send-email-tarun.kanti@ti.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Thu, Feb 02, 2012 at 11:00:27PM +0530, Tarun Kanti DebBarma wrote: > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 0b05629..6ea7390 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -28,7 +28,10 @@ > #include <asm/gpio.h> > #include <asm/mach/irq.h> > > +static LIST_HEAD(omap_gpio_list); I guess it's now too late because patch is acked and everything, but I think if you make the driver handle one bank alone and just instantiate it multiple times (omap_gpio.0, omap_gpio.1, omap_gpio.3, etc) driver would be faaaaaar simpler.
On Thu, Feb 02, 2012 at 08:41:07PM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Feb 02, 2012 at 11:00:27PM +0530, Tarun Kanti DebBarma wrote: > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 0b05629..6ea7390 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -28,7 +28,10 @@ > > #include <asm/gpio.h> > > #include <asm/mach/irq.h> > > > > +static LIST_HEAD(omap_gpio_list); > > I guess it's now too late because patch is acked and everything, but I > think if you make the driver handle one bank alone and just instantiate > it multiple times (omap_gpio.0, omap_gpio.1, omap_gpio.3, etc) driver > would be faaaaaar simpler. Is there any shared state between the banks? On my very cursory glance it looked like banks still have some interaction between them. If not, then yes I agree that multiple instances would be better. g.
On Thu, Feb 02, 2012 at 11:00:26PM +0530, Tarun Kanti DebBarma wrote: > The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: > Linus Torvalds (1): > Linux 3.3-rc2 > > are available in the git repository at: > http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev > Branch: for_3.4/gpio_cleanup_fixes_v9 Bad git url. I had to replace 'http:' with 'git:'. I've merged this series into my gpio/next branch. It should show up in linux-next in a couple of days. g. > > This series is continuation of cleanup of OMAP GPIO driver and fixes. > The cleanup include getting rid of cpu_is_* checks wherever possible, > use of gpio_bank list instead of static array, use of unique platform > specific value associated data member to OMAP platforms to avoid > cpu_is_* checks. The series also include PM runtime support. > > Power Tests > a) OMAP3430SDP > - Modify board-3430sdp.c file to have multiple GPIO modules active > with debounce timeout enabled. > - Enable CPU-Idle > - Enable UART timeouts > - Enable offmode > - echo mem > /sys/power/state > - Verify retention and offmode count increment > > Used following patches to avoid exception during system suspend:- > [PATCH RFC 1/2] mtd : Prevent the NULL pointer access > [PATCH RFC 2/2] mtd : Make the mtd_suspend return 0 if the suspend is not implemented > > # echo mem > /sys/power/state > [ 47.128021] PM: Syncing filesystems ... done. > [ 47.144104] Freezing user space processes ... (elapsed 0.01 seconds) done. > [ 47.168243] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) don e. > [ 47.205139] Unable to handle kernel NULL pointer dereference at virtual addre ss 000000a0 > [ 47.213317] pgd = deaac000 > [ 47.216033] [000000a0] *pgd=9e932831, *pte=00000000, *ppte=00000000 > [ 47.222381] Internal error: Oops: 17 [#1] SMP > [ 47.226745] Modules linked in: > [ 47.229827] CPU: 0 Not tainted (3.3.0-rc2-00031-g12c5c5c #235) > [ 47.236022] PC is at mtd_cls_suspend+0x8/0x20 > [ 47.240386] LR is at mtd_cls_suspend+0x8/0x20 > [ 47.244750] pc : [<c02e78f8>] lr : [<c02e78f8>] psr: a0000013 > [ 47.244750] sp : dea1fe60 ip : 22222222 fp : c0ee7d40 > [ 47.256256] r10: c0ee7cf0 r9 : 00000000 r8 : c02e78f0 > [ 47.261474] r7 : 00000000 r6 : 00000000 r5 : 00000002 r4 : dea45800 > [ 47.268005] r3 : deb4cac0 r2 : 00000000 r1 : 00000002 r0 : 00000000 > [ 47.274536] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > [ 47.281677] Control: 10c5387d Table: 9eaac019 DAC: 00000015 > [ 47.287445] Process sh (pid: 1177, stack limit = 0xdea1e2f8) > [ 47.293090] Stack: (0xdea1fe60 to 0xdea20000) > [...] > > b) ZOOM3 > - Enable CPU-Idle > - Enable UART timeout > - echo mem > /sys/power/state > - Wakeup system using serial keyboard > - Verify retention count increment > > Functional Tests > - Done on OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430 > > Bootup Test > - Done on OMAP1710 > Used following patch to fix OMAP1 build error:- > [PATCH] i2c: OMAP: Fix OMAP1 build error > > v9: > - Summary of Comments/Issues fixed > * GPIO wakeup does not work > > * Call debounce clock enable/disable functions from PM runtime callbacks. > This will avoid calling the functions from multiple places. > > * Modify description of following patch to match latest changes. > gpio/omap: save and restore debounce registers. > > * Use (bank->regs->set_dataout && bank->regs->clr_dataout) together instead > of using only one of them. > > * Remove cpu_is_omapxxxx() checks from set_gpio_trigger(). > > * _gpio_dbck_enable() in runtime callback triggered from omap_gpio_request > does not enable dbck because dbck_enable_mask is not set at this point. > > * Workaround associated with an errata got missed in v8. This has been > included. > > v8: > - Remove PM_CONFIG macro around following assignment. > pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count; > > - Once pm_runtime is enabled there is no more need for calling the > omap_device_disable_idle_on_suspend(od). > > - With pm_runtime, handling of clocks in Suspend is taken care of by > powerdomain hooks. So remove usage of *_runtime_put/get* from > suspend/resume hooks and Idle path. > > - Add handling of debounce clocks along the Suspend and Idle paths. > > - Remove [PATCH 04/24] gpio/omap: fix pwrdm_post_transition call sequence > from this series. This will be merged as part of power cleanup series. > > - Remove [PATCH v7 20/26] gpio/omap: skip operations in runtime callbacks > The bank->mod_usage check in this patch is not needed any more because > they are now already being taken care in suspend/resume and Idle paths. > > - Remove [PATCH v7 26/26] gpio/omap: add dbclk aliases for all gpio modules > This is already taken care in hwmod. > > - Remove redundant blank line in > [PATCH v7 14/26] gpio/omap: remove unnecessary bit-masking for read access > > - if (cpu_is_omap15xx() && (bank->method == METHOD_MPUIO)) > - isr &= 0x0000ffff; > > if (bank->level_mask) > level_mask = bank->level_mask & enabled; > > v7: > - Use pm_runtime_put() instead of pm_runtime_put_sync_suspend() > - Keep *_runtime_get/put*() outside spinlock > - Remove additional checking of conditions in _restore_context() > From: > if (bank->regs->set_dataout && bank->regs->clear_dataout) > ... > To: > if (bank->regs->set_dataout) > ... > > - Use SET_RUNTIME_PM_OPS and SET_SYSTEM_SLEEP_PM_OPS macros > - In [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle, > protect the bank data elements and register access using spinlock in > runtime_suspend/resume() callbacks. > This is because these callbacks run with interrupts enabled. > - Add dbclk aliases for all GPIO modules. Without this, GPIO modules were not > getting the correct clock handle to enable/disable debounec clock. > - Fix log comments on the following patches: > [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle > [PATCH 20/25] gpio/omap: skip operations in runtime callbacks > [PATCH 24/25] gpio/omap: restore OE only after setting the output level > > v6: > - Save and restore debounce registers for proper driver operation. > - Restore interrupt enable after all configuration to avoid spurious interrupts. > - Restore dataout register before oe register. > - Restore dataout into dataout_set or dataout based upon the OMAP version. > - Change register name from wkup_status to wkup_en. > - Remove wrapper around omap_pm_get_dev_context_loss_count(). Use it directly. > Also, changed the signature of get_context_loss_count in pdata and bank structure > from int to u32. > > - Use 'context' instead of 'ctx' for clarity wherever it is used. > - Merged two patches into one which are related to bank_is_mpuio() modification. > - Use shift operator instead of following: > + .irqctrl = OMAP_MPUIO_GPIO_INT_EDGE / 2, > > - Remove redundant check from the following > + if (bank_is_mpuio(bank)) { > + if (bank->regs->wkup_status) <--- redundant check > + mpuio_init(bank); > > - Change subject of following patch > [PATCH v5 15/22] gpio/omap: use readl in irq_handler for all access > into > [PATCH 14/25] gpio/omap: remove unnecessary bit-masking for read access > > - Fix multi-line comments in > [PATCH v5 20/22] gpio/omap: cleanup prepare_for_idle and resume_after_idle > > v5: > - Reduce runtime callback overhead when *_get/put_sync() called from probe() > and *_gpio_request/free(). > > - Dynamic context save within functions where context is modified instead of > saving all context within a common function. > > - Removed call to mpuio_init() from omap_gpio_mod_init(). Both the functions are > called once during initialization in *_gpio_probe(). > Call to omap_gpio_mod_init() has been removed from omap_gpio_request() on the > first access to gpio bank. One time initialization looks sufficient. > > - In *_gpio_irq_handler() use *_put_sync_suspend() instead of *_put_sync(). > > - Removed hardcoding of OMAP16xx sysconfig register value and instead defined an > associated constant. > > - Removed *_get_sync() call from *_gpio_suspend() and *_put_sync() call from > *_gpio_resume(). They got wrongly slipped into the code. > > - Removed following redundant zero allocated initialization from mach-omap2/gpio.c > + pdata->regs->irqctrl = 0; > + pdata->regs->edgectrl1 = 0; > + pdata->regs->edgectrl2 = 0; > > - Removed following redundant code in gpio-omap.c > -#define bank_is_mpuio(bank) ((bank)->method == METHOD_MPUIO) > > v4: > - since all accesses to registers are 4-byte aligned, removing special > checks and handling of 16 and 32-bit wide bank registers and instead > use 32-bit read/write access consistently. > > - redundant usage of MOD_REG_BIT has been corrected and replaced with > _gpio_rmw(). > > - omap_gpio_mod_init() function has been simplified further using _gpio_rmw(). > > - sysconfig register offset specific to omap16xx has been removed along > with its usage. > > - additional logic to skip from suspend/resume: > > if (!bank->regs->wkup_status || !bank->suspend_wakeup) > return 0; > > if (!bank->regs->wkup_status || !bank->saved_wakeup) > return 0; > > - separated mpuio related changes into a different patch from the patch where > wakeup status register related changes are done. > > - Incorrect replacement of !cpu_class_is_omap2() in gpio_irq_type() > corrected: > + if (!bank->regs->leveldetect0 && > + (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH))) > return -EINVAL; > v3: > - Avoid use of wkup_set and wkup_clear registers. Instead use wkup_status > register for all platforms. This is because on OMAP4 it is recommended > not to use them. > > - Remove duplicate code in omap_gpio_mod_init() for handling the same for > 32-bit and 16-bit GPIO bank widths. This is accomplished by having two > functions to handle each case while assiging a common function pointer > during initialization. > > - Remove OMAP16xx specific one time initialization from omap_gpio_mod_init(). > Move it inside omap16xx_gpio_init(). > > - Avoid usage of USHRT_MAX to indicate undefined values. Use 0 instead. > > - In omap_gpio_suspend()/resume() functions remove code that checks > if the feature is supported. Instead, assign these functions to > struct platform_driver's suspend & resume function pointers for those > OMAP platforms whcih support this feature. > > - Remove 'suspend_support' flag because it is redundant. Instead use > wkup_* registers to decode the same information. > > - Restore context also when we don't know if the context is lost. > > - Make omap_gpio_save_context() and omap_gpio_restore_context() static. > > v2: > - Do special handling of non-wakeup GPIOs only on OMAP2420. Avoid this > handling on OMAP3430. > - Isolate cleanups and fixes into separate set of patches. Keep the cleanup > first followed by the fixes. > - Avoid calling omap_gpio_get_context_loss() directly and instead call it > through function pointer in pdata initialized during init. > - workaround_enabled flag is not longer needed and is removed. > - Call pwrdm_post_transition() before calling omap_gpio_resume_after_idle(). > - In omap2_gpio_resume_after_idle() do context restore before handling > workaround. > - Use PM runtime framework. > - Modify register offset names to : wkup_status, wkup_clear, wkup_set. > Also use 'base + offset' for readibility in all relevant places. > - Remove unwanted messages from commit section like TODO, etc. > > > Charulatha V (8): > gpio/omap: remove dependency on gpio_bank_count > gpio/omap: use flag to identify wakeup domain > gpio/omap: make gpio_context part of gpio_bank structure > gpio/omap: make non-wakeup GPIO part of pdata > gpio/omap: avoid cpu checks during module ena/disable > gpio/omap: use pinctrl offset instead of macro > gpio/omap: remove bank->method & METHOD_* macros > gpio/omap: fix bankwidth for OMAP7xx MPUIO > > Nishanth Menon (4): > gpio/omap: save and restore debounce registers > gpio/omap: enable irq at the end of all configuration in restore > gpio/omap: restore OE only after setting the output level > gpio/omap: handle set_dataout reg capable IP on restore > > Tarun Kanti DebBarma (13): > gpio/omap: handle save/restore context in GPIO driver > gpio/omap: further cleanup using wkup_en register > gpio/omap: use level/edge detect reg offsets > gpio/omap: remove hardcoded offsets in context save/restore > gpio/omap: cleanup set_gpio_triggering function > gpio/omap: cleanup omap_gpio_mod_init function > gpio/omap: remove unnecessary bit-masking for read access > gpio/omap: use pm-runtime framework > gpio/omap: optimize suspend and resume functions > gpio/omap: cleanup prepare_for_idle and resume_after_idle > gpio/omap: fix debounce clock handling > gpio/omap: fix incorrect access of debounce module > gpio/omap: remove omap_gpio_save_context overhead > > arch/arm/mach-omap1/gpio15xx.c | 7 +- > arch/arm/mach-omap1/gpio16xx.c | 47 ++- > arch/arm/mach-omap1/gpio7xx.c | 14 +- > arch/arm/mach-omap2/gpio.c | 36 +- > arch/arm/mach-omap2/pm34xx.c | 14 - > arch/arm/plat-omap/include/plat/gpio.h | 29 +- > drivers/gpio/gpio-omap.c | 1099 +++++++++++++------------------- > 7 files changed, 549 insertions(+), 697 deletions(-) >
On Thu, Feb 02, 2012 at 12:16:30PM -0700, Grant Likely wrote: > On Thu, Feb 02, 2012 at 08:41:07PM +0200, Felipe Balbi wrote: > > Hi, > > > > On Thu, Feb 02, 2012 at 11:00:27PM +0530, Tarun Kanti DebBarma wrote: > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > > index 0b05629..6ea7390 100644 > > > --- a/drivers/gpio/gpio-omap.c > > > +++ b/drivers/gpio/gpio-omap.c > > > @@ -28,7 +28,10 @@ > > > #include <asm/gpio.h> > > > #include <asm/mach/irq.h> > > > > > > +static LIST_HEAD(omap_gpio_list); > > > > I guess it's now too late because patch is acked and everything, but I > > think if you make the driver handle one bank alone and just instantiate > > it multiple times (omap_gpio.0, omap_gpio.1, omap_gpio.3, etc) driver > > would be faaaaaar simpler. > > Is there any shared state between the banks? On my very cursory glance it > looked like banks still have some interaction between them. If not, then > yes I agree that multiple instances would be better. A quick glance at the TRM shows that banks have separate address spaces and IRQ lines. I think it's done this way because we can handoff one (or more) bank to other cores on the SoC, so they need to be pretty independent. I could be missing something though.
On 2/2/2012 8:45 PM, Felipe Balbi wrote: > On Thu, Feb 02, 2012 at 12:16:30PM -0700, Grant Likely wrote: >> On Thu, Feb 02, 2012 at 08:41:07PM +0200, Felipe Balbi wrote: >>> Hi, >>> >>> On Thu, Feb 02, 2012 at 11:00:27PM +0530, Tarun Kanti DebBarma wrote: >>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >>>> index 0b05629..6ea7390 100644 >>>> --- a/drivers/gpio/gpio-omap.c >>>> +++ b/drivers/gpio/gpio-omap.c >>>> @@ -28,7 +28,10 @@ >>>> #include<asm/gpio.h> >>>> #include<asm/mach/irq.h> >>>> >>>> +static LIST_HEAD(omap_gpio_list); >>> >>> I guess it's now too late because patch is acked and everything, but I >>> think if you make the driver handle one bank alone and just instantiate >>> it multiple times (omap_gpio.0, omap_gpio.1, omap_gpio.3, etc) driver >>> would be faaaaaar simpler. >> >> Is there any shared state between the banks? On my very cursory glance it >> looked like banks still have some interaction between them. If not, then >> yes I agree that multiple instances would be better. > > A quick glance at the TRM shows that banks have separate address spaces > and IRQ lines. I think it's done this way because we can handoff one (or > more) bank to other cores on the SoC, so they need to be pretty > independent. > > I could be missing something though. In fact the driver already handled the 6 GPIOS banks as individual devices: [ 0.185638] gpiochip_add: registered GPIOs 0 to 31 on device: gpio [ 0.185882] OMAP GPIO hardware version 0.1 [ 0.186767] gpiochip_add: registered GPIOs 32 to 63 on device: gpio [ 0.187744] gpiochip_add: registered GPIOs 64 to 95 on device: gpio [ 0.188751] gpiochip_add: registered GPIOs 96 to 127 on device: gpio [ 0.189819] gpiochip_add: registered GPIOs 128 to 159 on device: gpio [ 0.190917] gpiochip_add: registered GPIOs 160 to 191 on device: gpio That list is only used to iterate over all the instances during CPU idle: void omap2_gpio_prepare_for_idle(int pwr_mode) { struct gpio_bank *bank; list_for_each_entry(bank, &omap_gpio_list, node) { if (!bank->mod_usage || !bank->loses_context) continue; bank->power_mode = pwr_mode; pm_runtime_put_sync_suspend(bank->dev); } } void omap2_gpio_resume_after_idle(void) { struct gpio_bank *bank; list_for_each_entry(bank, &omap_gpio_list, node) { if (!bank->mod_usage || !bank->loses_context) continue; pm_runtime_get_sync(bank->dev); } } I don't know if there is some reason to not use driver_for_each_device. Kevin, Do we have any constraint inside omap_sram_idle to not use the device iterator? Regards, Benoit
Hi, On Thu, Feb 02, 2012 at 09:48:13PM +0100, Cousson, Benoit wrote: > On 2/2/2012 8:45 PM, Felipe Balbi wrote: > >On Thu, Feb 02, 2012 at 12:16:30PM -0700, Grant Likely wrote: > >>On Thu, Feb 02, 2012 at 08:41:07PM +0200, Felipe Balbi wrote: > >>>Hi, > >>> > >>>On Thu, Feb 02, 2012 at 11:00:27PM +0530, Tarun Kanti DebBarma wrote: > >>>>diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >>>>index 0b05629..6ea7390 100644 > >>>>--- a/drivers/gpio/gpio-omap.c > >>>>+++ b/drivers/gpio/gpio-omap.c > >>>>@@ -28,7 +28,10 @@ > >>>> #include<asm/gpio.h> > >>>> #include<asm/mach/irq.h> > >>>> > >>>>+static LIST_HEAD(omap_gpio_list); > >>> > >>>I guess it's now too late because patch is acked and everything, but I > >>>think if you make the driver handle one bank alone and just instantiate > >>>it multiple times (omap_gpio.0, omap_gpio.1, omap_gpio.3, etc) driver > >>>would be faaaaaar simpler. > >> > >>Is there any shared state between the banks? On my very cursory glance it > >>looked like banks still have some interaction between them. If not, then > >>yes I agree that multiple instances would be better. > > > >A quick glance at the TRM shows that banks have separate address spaces > >and IRQ lines. I think it's done this way because we can handoff one (or > >more) bank to other cores on the SoC, so they need to be pretty > >independent. > > > >I could be missing something though. > > In fact the driver already handled the 6 GPIOS banks as individual devices: > > [ 0.185638] gpiochip_add: registered GPIOs 0 to 31 on device: gpio > [ 0.185882] OMAP GPIO hardware version 0.1 > [ 0.186767] gpiochip_add: registered GPIOs 32 to 63 on device: gpio > [ 0.187744] gpiochip_add: registered GPIOs 64 to 95 on device: gpio > [ 0.188751] gpiochip_add: registered GPIOs 96 to 127 on device: gpio > [ 0.189819] gpiochip_add: registered GPIOs 128 to 159 on device: gpio > [ 0.190917] gpiochip_add: registered GPIOs 160 to 191 on device: gpio yeah, but you can get all of that for free from driver core. Just add one platform_device for each bank and make the omap-gpio.c only understand one bank. No tricks. What I'm trying to say is to remove the Bank array or list_head and make probe() get called 6 times by creating 6 omap_gpio platform_devices. From probe you cann gpiochip_add() once and only once. > That list is only used to iterate over all the instances during CPU idle: > > void omap2_gpio_prepare_for_idle(int pwr_mode) > { > struct gpio_bank *bank; > > list_for_each_entry(bank, &omap_gpio_list, node) { > if (!bank->mod_usage || !bank->loses_context) > continue; > > bank->power_mode = pwr_mode; > > pm_runtime_put_sync_suspend(bank->dev); > } > } > > void omap2_gpio_resume_after_idle(void) > { > struct gpio_bank *bank; > > list_for_each_entry(bank, &omap_gpio_list, node) { > if (!bank->mod_usage || !bank->loses_context) > continue; > > pm_runtime_get_sync(bank->dev); > } > } that's the thing which is unnecessary, actually :-) Why do we even have this omap2_gpio_resume_after_idle() ? Can't the gpio driver handle its own PM or listen to cpuidle notificaitons for that ? I would like to understand why do we need this hack for pm runtime. Can't you just use ->prepare() and ->complete() from dev_pm_ops ? > I don't know if there is some reason to not use driver_for_each_device. driver_for_each_device() is already handled by driver core. So your omap_device_build() would have a loop creating N omap_devices, one for each gpio bank. Each bank would receive one IRQ line and one address base. And they would only understand that. Every instance of the driver handles the GPIOs connected on one bank.
Hi again, On Thu, Feb 02, 2012 at 11:49:08PM +0200, Felipe Balbi wrote: > > In fact the driver already handled the 6 GPIOS banks as individual devices: > > > > [ 0.185638] gpiochip_add: registered GPIOs 0 to 31 on device: gpio > > [ 0.185882] OMAP GPIO hardware version 0.1 > > [ 0.186767] gpiochip_add: registered GPIOs 32 to 63 on device: gpio > > [ 0.187744] gpiochip_add: registered GPIOs 64 to 95 on device: gpio > > [ 0.188751] gpiochip_add: registered GPIOs 96 to 127 on device: gpio > > [ 0.189819] gpiochip_add: registered GPIOs 128 to 159 on device: gpio > > [ 0.190917] gpiochip_add: registered GPIOs 160 to 191 on device: gpio > > yeah, but you can get all of that for free from driver core. Just add > one platform_device for each bank and make the omap-gpio.c only > understand one bank. No tricks. > > What I'm trying to say is to remove the Bank array or list_head and make > probe() get called 6 times by creating 6 omap_gpio platform_devices. > > From probe you cann gpiochip_add() once and only once. ^^^^ call I actually just took the time to go over the driver and that's what it does. So the list_head is only there fo the nasty cpuidle stuff below: > > That list is only used to iterate over all the instances during CPU idle: > > > > void omap2_gpio_prepare_for_idle(int pwr_mode) > > { > > struct gpio_bank *bank; > > > > list_for_each_entry(bank, &omap_gpio_list, node) { > > if (!bank->mod_usage || !bank->loses_context) > > continue; > > > > bank->power_mode = pwr_mode; > > > > pm_runtime_put_sync_suspend(bank->dev); > > } > > } > > > > void omap2_gpio_resume_after_idle(void) > > { > > struct gpio_bank *bank; > > > > list_for_each_entry(bank, &omap_gpio_list, node) { > > if (!bank->mod_usage || !bank->loses_context) > > continue; > > > > pm_runtime_get_sync(bank->dev); > > } > > } > > that's the thing which is unnecessary, actually :-) > > Why do we even have this omap2_gpio_resume_after_idle() ? Can't the gpio > driver handle its own PM or listen to cpuidle notificaitons for that ? > > I would like to understand why do we need this hack for pm runtime. > Can't you just use ->prepare() and ->complete() from dev_pm_ops ? This question remains. Why do we need those funtions ?
On 2/2/2012 10:53 PM, Felipe Balbi wrote: > Hi again, > > On Thu, Feb 02, 2012 at 11:49:08PM +0200, Felipe Balbi wrote: >>> In fact the driver already handled the 6 GPIOS banks as individual devices: >>> >>> [ 0.185638] gpiochip_add: registered GPIOs 0 to 31 on device: gpio >>> [ 0.185882] OMAP GPIO hardware version 0.1 >>> [ 0.186767] gpiochip_add: registered GPIOs 32 to 63 on device: gpio >>> [ 0.187744] gpiochip_add: registered GPIOs 64 to 95 on device: gpio >>> [ 0.188751] gpiochip_add: registered GPIOs 96 to 127 on device: gpio >>> [ 0.189819] gpiochip_add: registered GPIOs 128 to 159 on device: gpio >>> [ 0.190917] gpiochip_add: registered GPIOs 160 to 191 on device: gpio >> >> yeah, but you can get all of that for free from driver core. Just add >> one platform_device for each bank and make the omap-gpio.c only >> understand one bank. No tricks. >> >> What I'm trying to say is to remove the Bank array or list_head and make >> probe() get called 6 times by creating 6 omap_gpio platform_devices. >> >> From probe you cann gpiochip_add() once and only once. > ^^^^ > call > > I actually just took the time to go over the driver and that's what it > does. So the list_head is only there fo the nasty cpuidle stuff below: Yes, that was my point :-) >>> That list is only used to iterate over all the instances during CPU idle: >>> >>> void omap2_gpio_prepare_for_idle(int pwr_mode) >>> { >>> struct gpio_bank *bank; >>> >>> list_for_each_entry(bank,&omap_gpio_list, node) { >>> if (!bank->mod_usage || !bank->loses_context) >>> continue; >>> >>> bank->power_mode = pwr_mode; >>> >>> pm_runtime_put_sync_suspend(bank->dev); >>> } >>> } >>> >>> void omap2_gpio_resume_after_idle(void) >>> { >>> struct gpio_bank *bank; >>> >>> list_for_each_entry(bank,&omap_gpio_list, node) { >>> if (!bank->mod_usage || !bank->loses_context) >>> continue; >>> >>> pm_runtime_get_sync(bank->dev); >>> } >>> } >> >> that's the thing which is unnecessary, actually :-) >> >> Why do we even have this omap2_gpio_resume_after_idle() ? Can't the gpio >> driver handle its own PM or listen to cpuidle notificaitons for that ? >> >> I would like to understand why do we need this hack for pm runtime. >> Can't you just use ->prepare() and ->complete() from dev_pm_ops ? > > This question remains. Why do we need those funtions ? These functions are called from the CPUIdle path so outside the scope of the GPIO driver. These are part of a bunch of nasty PM hacks we are doing in the CPU idle loop. We are in the process of getting rid of most of them, but it looks like some are still needed. Regards, Benoit
Hi, On Thu, Feb 02, 2012 at 11:00:43PM +0100, Cousson, Benoit wrote: > On 2/2/2012 10:53 PM, Felipe Balbi wrote: > >Hi again, > > > >On Thu, Feb 02, 2012 at 11:49:08PM +0200, Felipe Balbi wrote: > >>>In fact the driver already handled the 6 GPIOS banks as individual devices: > >>> > >>>[ 0.185638] gpiochip_add: registered GPIOs 0 to 31 on device: gpio > >>>[ 0.185882] OMAP GPIO hardware version 0.1 > >>>[ 0.186767] gpiochip_add: registered GPIOs 32 to 63 on device: gpio > >>>[ 0.187744] gpiochip_add: registered GPIOs 64 to 95 on device: gpio > >>>[ 0.188751] gpiochip_add: registered GPIOs 96 to 127 on device: gpio > >>>[ 0.189819] gpiochip_add: registered GPIOs 128 to 159 on device: gpio > >>>[ 0.190917] gpiochip_add: registered GPIOs 160 to 191 on device: gpio > >> > >>yeah, but you can get all of that for free from driver core. Just add > >>one platform_device for each bank and make the omap-gpio.c only > >>understand one bank. No tricks. > >> > >>What I'm trying to say is to remove the Bank array or list_head and make > >>probe() get called 6 times by creating 6 omap_gpio platform_devices. > >> > >> From probe you cann gpiochip_add() once and only once. > > ^^^^ > > call > > > >I actually just took the time to go over the driver and that's what it > >does. So the list_head is only there fo the nasty cpuidle stuff below: > > Yes, that was my point :-) :-) my bad > >>>That list is only used to iterate over all the instances during CPU idle: > >>> > >>>void omap2_gpio_prepare_for_idle(int pwr_mode) > >>>{ > >>> struct gpio_bank *bank; > >>> > >>> list_for_each_entry(bank,&omap_gpio_list, node) { > >>> if (!bank->mod_usage || !bank->loses_context) > >>> continue; > >>> > >>> bank->power_mode = pwr_mode; > >>> > >>> pm_runtime_put_sync_suspend(bank->dev); > >>> } > >>>} > >>> > >>>void omap2_gpio_resume_after_idle(void) > >>>{ > >>> struct gpio_bank *bank; > >>> > >>> list_for_each_entry(bank,&omap_gpio_list, node) { > >>> if (!bank->mod_usage || !bank->loses_context) > >>> continue; > >>> > >>> pm_runtime_get_sync(bank->dev); > >>> } > >>>} > >> > >>that's the thing which is unnecessary, actually :-) > >> > >>Why do we even have this omap2_gpio_resume_after_idle() ? Can't the gpio > >>driver handle its own PM or listen to cpuidle notificaitons for that ? > >> > >>I would like to understand why do we need this hack for pm runtime. > >>Can't you just use ->prepare() and ->complete() from dev_pm_ops ? > > > >This question remains. Why do we need those funtions ? > > These functions are called from the CPUIdle path so outside the scope > of the GPIO driver. These are part of a bunch of nasty PM hacks we > are doing in the CPU idle loop. We are in the process of getting rid > of most of them, but it looks like some are still needed. Too bad. I can see that the gpio pm implementation seems a bit "peculiar". I mean, pm does reference counting and yet the driver has checks to prevent multiple gets and puts on a single bank (meaning that pm counter will be either 0 or 1 at any point in time). To me it looks like those functions are there in order to forcefully put PER power domain in OFF because drivers are always holding a reference to their gpios (drivers generally gpio_request() on probe() and gpio_free() on remove()). Looks like the entire pm implementation on OMAP gpio driver has always considered only the fact that gpios can be requested and freed, but never that we want the system to go to OFF even while gpios are requested, because we have I/O PAD wakeups. At some point that has to be sorted out because that HACK is quite ugly :-) I'll see if I find some time to go over the interactions between gpio-omap.c and pm24x.c and pm34xx.c any of these days, but I can't promise anything ;-)
Tarun, I am interested in testing these patches on AM335x. Do you have a tree with these patches applied so that I can pull. On Thu, Feb 02, 2012 at 23:00:26, DebBarma, Tarun Kanti wrote: > The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: > Linus Torvalds (1): > Linux 3.3-rc2 > > are available in the git repository at: > http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev > Branch: for_3.4/gpio_cleanup_fixes_v9 > > This series is continuation of cleanup of OMAP GPIO driver and fixes. > The cleanup include getting rid of cpu_is_* checks wherever possible, > use of gpio_bank list instead of static array, use of unique platform > specific value associated data member to OMAP platforms to avoid > cpu_is_* checks. The series also include PM runtime support. > > Power Tests > a) OMAP3430SDP > - Modify board-3430sdp.c file to have multiple GPIO modules active > with debounce timeout enabled. > - Enable CPU-Idle > - Enable UART timeouts > - Enable offmode > - echo mem > /sys/power/state > - Verify retention and offmode count increment > > Used following patches to avoid exception during system suspend:- > [PATCH RFC 1/2] mtd : Prevent the NULL pointer access > [PATCH RFC 2/2] mtd : Make the mtd_suspend return 0 if the suspend is not implemented > > # echo mem > /sys/power/state > [ 47.128021] PM: Syncing filesystems ... done. > [ 47.144104] Freezing user space processes ... (elapsed 0.01 seconds) done. > [ 47.168243] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) don e. > [ 47.205139] Unable to handle kernel NULL pointer dereference at virtual addre ss 000000a0 > [ 47.213317] pgd = deaac000 > [ 47.216033] [000000a0] *pgd=9e932831, *pte=00000000, *ppte=00000000 > [ 47.222381] Internal error: Oops: 17 [#1] SMP > [ 47.226745] Modules linked in: > [ 47.229827] CPU: 0 Not tainted (3.3.0-rc2-00031-g12c5c5c #235) > [ 47.236022] PC is at mtd_cls_suspend+0x8/0x20 > [ 47.240386] LR is at mtd_cls_suspend+0x8/0x20 > [ 47.244750] pc : [<c02e78f8>] lr : [<c02e78f8>] psr: a0000013 > [ 47.244750] sp : dea1fe60 ip : 22222222 fp : c0ee7d40 > [ 47.256256] r10: c0ee7cf0 r9 : 00000000 r8 : c02e78f0 > [ 47.261474] r7 : 00000000 r6 : 00000000 r5 : 00000002 r4 : dea45800 > [ 47.268005] r3 : deb4cac0 r2 : 00000000 r1 : 00000002 r0 : 00000000 > [ 47.274536] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > [ 47.281677] Control: 10c5387d Table: 9eaac019 DAC: 00000015 > [ 47.287445] Process sh (pid: 1177, stack limit = 0xdea1e2f8) > [ 47.293090] Stack: (0xdea1fe60 to 0xdea20000) > [...] > > b) ZOOM3 > - Enable CPU-Idle > - Enable UART timeout > - echo mem > /sys/power/state > - Wakeup system using serial keyboard > - Verify retention count increment > > Functional Tests > - Done on OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430 > > Bootup Test > - Done on OMAP1710 > Used following patch to fix OMAP1 build error:- > [PATCH] i2c: OMAP: Fix OMAP1 build error > [..] [..] > Charulatha V (8): > gpio/omap: remove dependency on gpio_bank_count > gpio/omap: use flag to identify wakeup domain > gpio/omap: make gpio_context part of gpio_bank structure > gpio/omap: make non-wakeup GPIO part of pdata > gpio/omap: avoid cpu checks during module ena/disable > gpio/omap: use pinctrl offset instead of macro > gpio/omap: remove bank->method & METHOD_* macros > gpio/omap: fix bankwidth for OMAP7xx MPUIO > > Nishanth Menon (4): > gpio/omap: save and restore debounce registers > gpio/omap: enable irq at the end of all configuration in restore > gpio/omap: restore OE only after setting the output level > gpio/omap: handle set_dataout reg capable IP on restore > > Tarun Kanti DebBarma (13): > gpio/omap: handle save/restore context in GPIO driver > gpio/omap: further cleanup using wkup_en register > gpio/omap: use level/edge detect reg offsets > gpio/omap: remove hardcoded offsets in context save/restore > gpio/omap: cleanup set_gpio_triggering function > gpio/omap: cleanup omap_gpio_mod_init function > gpio/omap: remove unnecessary bit-masking for read access > gpio/omap: use pm-runtime framework > gpio/omap: optimize suspend and resume functions > gpio/omap: cleanup prepare_for_idle and resume_after_idle > gpio/omap: fix debounce clock handling > gpio/omap: fix incorrect access of debounce module > gpio/omap: remove omap_gpio_save_context overhead > > arch/arm/mach-omap1/gpio15xx.c | 7 +- > arch/arm/mach-omap1/gpio16xx.c | 47 ++- > arch/arm/mach-omap1/gpio7xx.c | 14 +- > arch/arm/mach-omap2/gpio.c | 36 +- > arch/arm/mach-omap2/pm34xx.c | 14 - > arch/arm/plat-omap/include/plat/gpio.h | 29 +- > drivers/gpio/gpio-omap.c | 1099 +++++++++++++------------------- > 7 files changed, 549 insertions(+), 697 deletions(-) > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Regards, Gururaja
Hi, On Fri, Feb 03, 2012 at 15:29:49, DebBarma, Tarun Kanti wrote: > On Fri, Feb 3, 2012 at 2:51 PM, Hebbar, Gururaja > <gururaja.hebbar@ti.com> wrote: > > > Tarun, > > I am interested in testing these patches on AM335x. Do you have > a tree with these > patches applied so that I can pull. > > > You can find the series here:- > git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-oma > p-dev for_3.4/gpio_cleanup_fixes_v9 I am unable to download from this repo. The Gitorious web page fails to show the tree and gives 500 error downloading using https protocol fails with below error. ghebbar@linux-psp-server:~/projects/linux-gits/gitorious-omap$ git remote add omap-gpio2 https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git ghebbar@linux-psp-server:~/projects/linux-gits/gitorious-omap$ git fetch omap-gpio2 error: Unable to get pack index https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-a461f67f3067aa122c09f38cf55346152c9fafb8.idx error: Unable to get pack index https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-a66a5d34b6388e1b0c6b49fe4a029cbf755b17c7.idx error: Unable to get pack index https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-952401733bd66c72bc947b54ad5a04ca8edfff2a.idx error: Unable to get pack index https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-1f38918dfb5427f9b79b88d7c6cda15e85c2ad92.idx error: Unable to get pack file https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-2e39094e5e34e6ce642fc2513727a2025b23f8c7.pack GnuTLS recv error (-9): A TLS packet with unexpected length was received. error: Unable to find f5af8c1f05d8a1e27ca910285da3fee28eb5acce under https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git Cannot obtain needed object f5af8c1f05d8a1e27ca910285da3fee28eb5acce while processing commit 7e0e6be8759e6423a4f97b4bbc5e9fec0e5cfde3. error: Fetch failed. ghebbar@linux-psp-server:~/projects/linux-gits/gitorious-omap$ git fetch omap-gpio2 error: Unable to get pack index https://git.gitorious.org/omap-sw-develoment/linux-omap-dev.git/objects/pack/pack-a461f67f3067aa122c09f38cf55346152c9fafb8.idx error: wrong index v2 file size in .git/objects/pack/pack-952401733bd66c72bc947b54ad5a04ca8edfff2a.idx error: .git/objects/pack/pack-2e39094e5e34e6ce642fc2513727a2025b23f8c7.pack SHA1 checksum mismatch error: inflate: data stream error (invalid distance too far back) is there any issue with the repo. > -- > Tarun > > > > > On Thu, Feb 02, 2012 at 23:00:26, DebBarma, Tarun Kanti wrote: > > The following changes since commit > 62aa2b537c6f5957afd98e29f96897419ed5ebab: > > Linus Torvalds (1): > > Linux 3.3-rc2 > > > > are available in the git repository at: > > > http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-om > ap-dev > <http://gitorious.org/%7Etarunkanti/omap-sw-develoment/tarunkantis-linux > -omap-dev> > > Branch: for_3.4/gpio_cleanup_fixes_v9 > > > > This series is continuation of cleanup of OMAP GPIO driver and > fixes. > > The cleanup include getting rid of cpu_is_* checks wherever > possible, > > use of gpio_bank list instead of static array, use of unique > platform > > specific value associated data member to OMAP platforms to > avoid > > cpu_is_* checks. The series also include PM runtime support. > > > > Power Tests > > a) OMAP3430SDP > > - Modify board-3430sdp.c file to have multiple GPIO modules > active > > with debounce timeout enabled. > > - Enable CPU-Idle > > - Enable UART timeouts > > - Enable offmode > > - echo mem > /sys/power/state > > - Verify retention and offmode count increment > > > > Used following patches to avoid exception during system > suspend:- > > [PATCH RFC 1/2] mtd : Prevent the NULL pointer access > > [PATCH RFC 2/2] mtd : Make the mtd_suspend return 0 if the > suspend is not implemented > > > > # echo mem > /sys/power/state > > [ 47.128021] PM: Syncing filesystems ... done. > > [ 47.144104] Freezing user space processes ... (elapsed 0.01 > seconds) done. > > [ 47.168243] Freezing remaining freezable tasks ... (elapsed > 0.02 seconds) don > e. > > [ 47.205139] Unable to handle kernel NULL pointer > dereference at virtual addre > ss 000000a0 > > [ 47.213317] pgd = deaac000 > > [ 47.216033] [000000a0] *pgd=9e932831, *pte=00000000, > *ppte=00000000 > > [ 47.222381] Internal error: Oops: 17 [#1] SMP > > [ 47.226745] Modules linked in: > > [ 47.229827] CPU: 0 Not tainted > (3.3.0-rc2-00031-g12c5c5c #235) > > [ 47.236022] PC is at mtd_cls_suspend+0x8/0x20 > > [ 47.240386] LR is at mtd_cls_suspend+0x8/0x20 > > [ 47.244750] pc : [<c02e78f8>] lr : [<c02e78f8>] psr: > a0000013 > > [ 47.244750] sp : dea1fe60 ip : 22222222 fp : c0ee7d40 > > [ 47.256256] r10: c0ee7cf0 r9 : 00000000 r8 : c02e78f0 > > [ 47.261474] r7 : 00000000 r6 : 00000000 r5 : 00000002 r4 > : dea45800 > > [ 47.268005] r3 : deb4cac0 r2 : 00000000 r1 : 00000002 r0 > : 00000000 > > [ 47.274536] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA > ARM Segment user > > [ 47.281677] Control: 10c5387d Table: 9eaac019 DAC: > 00000015 > > [ 47.287445] Process sh (pid: 1177, stack limit = > 0xdea1e2f8) > > [ 47.293090] Stack: (0xdea1fe60 to 0xdea20000) > > [...] > > > > b) ZOOM3 > > - Enable CPU-Idle > > - Enable UART timeout > > - echo mem > /sys/power/state > > - Wakeup system using serial keyboard > > - Verify retention count increment > > > > Functional Tests > > - Done on OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430 > > > > Bootup Test > > - Done on OMAP1710 > > Used following patch to fix OMAP1 build error:- > > [PATCH] i2c: OMAP: Fix OMAP1 build error > > > > > [..] > [..] > > > > Charulatha V (8): > > gpio/omap: remove dependency on gpio_bank_count > > gpio/omap: use flag to identify wakeup domain > > gpio/omap: make gpio_context part of gpio_bank structure > > gpio/omap: make non-wakeup GPIO part of pdata > > gpio/omap: avoid cpu checks during module ena/disable > > gpio/omap: use pinctrl offset instead of macro > > gpio/omap: remove bank->method & METHOD_* macros > > gpio/omap: fix bankwidth for OMAP7xx MPUIO > > > > Nishanth Menon (4): > > gpio/omap: save and restore debounce registers > > gpio/omap: enable irq at the end of all configuration in > restore > > gpio/omap: restore OE only after setting the output level > > gpio/omap: handle set_dataout reg capable IP on restore > > > > Tarun Kanti DebBarma (13): > > gpio/omap: handle save/restore context in GPIO driver > > gpio/omap: further cleanup using wkup_en register > > gpio/omap: use level/edge detect reg offsets > > gpio/omap: remove hardcoded offsets in context save/restore > > gpio/omap: cleanup set_gpio_triggering function > > gpio/omap: cleanup omap_gpio_mod_init function > > gpio/omap: remove unnecessary bit-masking for read access > > gpio/omap: use pm-runtime framework > > gpio/omap: optimize suspend and resume functions > > gpio/omap: cleanup prepare_for_idle and resume_after_idle > > gpio/omap: fix debounce clock handling > > gpio/omap: fix incorrect access of debounce module > > gpio/omap: remove omap_gpio_save_context overhead > > > > arch/arm/mach-omap1/gpio15xx.c | 7 +- > > arch/arm/mach-omap1/gpio16xx.c | 47 ++- > > arch/arm/mach-omap1/gpio7xx.c | 14 +- > > arch/arm/mach-omap2/gpio.c | 36 +- > > arch/arm/mach-omap2/pm34xx.c | 14 - > > arch/arm/plat-omap/include/plat/gpio.h | 29 +- > > drivers/gpio/gpio-omap.c | 1099 > +++++++++++++------------------- > > 7 files changed, 549 insertions(+), 697 deletions(-) > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > Regards, > Gururaja > > > > Regards, Gururaja
Felipe Balbi <balbi@ti.com> writes: [...] >> >This question remains. Why do we need those funtions ? >> >> These functions are called from the CPUIdle path so outside the scope >> of the GPIO driver. These are part of a bunch of nasty PM hacks we >> are doing in the CPU idle loop. We are in the process of getting rid >> of most of them, but it looks like some are still needed. > > Too bad. I can see that the gpio pm implementation seems a bit > "peculiar". I mean, pm does reference counting and yet the driver has > checks to prevent multiple gets and puts on a single bank (meaning that > pm counter will be either 0 or 1 at any point in time). > > To me it looks like those functions are there in order to forcefully put > PER power domain in OFF because drivers are always holding a reference > to their gpios (drivers generally gpio_request() on probe() and > gpio_free() on remove()). > > Looks like the entire pm implementation on OMAP gpio driver has always > considered only the fact that gpios can be requested and freed, but > never that we want the system to go to OFF even while gpios are > requested, because we have I/O PAD wakeups. At some point that has to be > sorted out because that HACK is quite ugly :-) > > I'll see if I find some time to go over the interactions between > gpio-omap.c and pm24x.c and pm34xx.c any of these days, but I can't > promise anything ;-) If you look at the state of these prepare/resume hacks at the end of this series, you'll see that they are significantly cleaner and do nothing but call the runtime PM hooks. We have explored several ways to get rid of them completely in the idle path but have not yet come up with a clean way, but this series gets us a long ways towards that goal. Kevin
Grant Likely <grant.likely@secretlab.ca> writes: > On Thu, Feb 02, 2012 at 11:00:26PM +0530, Tarun Kanti DebBarma wrote: >> The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: >> Linus Torvalds (1): >> Linux 3.3-rc2 >> >> are available in the git repository at: >> http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev >> Branch: for_3.4/gpio_cleanup_fixes_v9 > > Bad git url. I had to replace 'http:' with 'git:'. > > I've merged this series into my gpio/next branch. It should show up in > linux-next in a couple of days. Grant, this series isn't quite ready for merge, and shouldn't be considered a pull request. I still need to give it a once over and some final testing. Kevin
Hi Tarun, Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: > The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: > Linus Torvalds (1): > Linux 3.3-rc2 > > are available in the git repository at: > http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev > Branch: for_3.4/gpio_cleanup_fixes_v9 > > This series is continuation of cleanup of OMAP GPIO driver and fixes. > The cleanup include getting rid of cpu_is_* checks wherever possible, > use of gpio_bank list instead of static array, use of unique platform > specific value associated data member to OMAP platforms to avoid > cpu_is_* checks. The series also include PM runtime support. This version is looking pretty good, and I'm almost ready to queue it up for Grant. However, one more minor nit. Please fix up the compile warnings when built without CONFIG_PM_SLEEP or CONFIG_PM_RUNTIME. The definitons of the dev_pm_ops callbacks need to be conditional when using the SET_*_PM_OPS() macros, otherwise you get these warnings: CC drivers/gpio/gpio-omap.o /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1142:12: warning: 'omap_gpio_suspend' defined but not used [-Wunused-function] /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1167:12: warning: 'omap_gpio_resume' defined but not used [-Wunused-function] /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1191:12: warning: 'omap_gpio_runtime_suspend' defined but not used [-Wunused-function] /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1237:12: warning: 'omap_gpio_runtime_resume' defined but not used [-Wunused-function] Thanks, Kevin
On Fri, Feb 3, 2012 at 10:51 AM, Kevin Hilman <khilman@ti.com> wrote: > Grant Likely <grant.likely@secretlab.ca> writes: > >> On Thu, Feb 02, 2012 at 11:00:26PM +0530, Tarun Kanti DebBarma wrote: >>> The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: >>> Linus Torvalds (1): >>> Linux 3.3-rc2 >>> >>> are available in the git repository at: >>> http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev >>> Branch: for_3.4/gpio_cleanup_fixes_v9 >> >> Bad git url. I had to replace 'http:' with 'git:'. >> >> I've merged this series into my gpio/next branch. It should show up in >> linux-next in a couple of days. > > Grant, this series isn't quite ready for merge, and shouldn't be > considered a pull request. No problem. I'll drop it from my tree. g.
Felipe, On Sat, Feb 4, 2012 at 21:38, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > On Fri, Feb 03, 2012 at 09:50:19AM -0800, Kevin Hilman wrote: > > Felipe Balbi <balbi@ti.com> writes: > > > > [...] > > > > >> >This question remains. Why do we need those funtions ? > > >> > > >> These functions are called from the CPUIdle path so outside the scope > > >> of the GPIO driver. These are part of a bunch of nasty PM hacks we > > >> are doing in the CPU idle loop. We are in the process of getting rid > > >> of most of them, but it looks like some are still needed. > > > > > > Too bad. I can see that the gpio pm implementation seems a bit > > > "peculiar". I mean, pm does reference counting and yet the driver has > > > checks to prevent multiple gets and puts on a single bank (meaning that > > > pm counter will be either 0 or 1 at any point in time). > > > > > > To me it looks like those functions are there in order to forcefully put > > > PER power domain in OFF because drivers are always holding a reference > > > to their gpios (drivers generally gpio_request() on probe() and > > > gpio_free() on remove()). > > > > > > Looks like the entire pm implementation on OMAP gpio driver has always > > > considered only the fact that gpios can be requested and freed, but > > > never that we want the system to go to OFF even while gpios are > > > requested, because we have I/O PAD wakeups. At some point that has to be > > > sorted out because that HACK is quite ugly :-) > > > > > > I'll see if I find some time to go over the interactions between > > > gpio-omap.c and pm24x.c and pm34xx.c any of these days, but I can't > > > promise anything ;-) > > > > If you look at the state of these prepare/resume hacks at the end of > > this series, you'll see that they are significantly cleaner and do > > nothing but call the runtime PM hooks. > > sure, definitely. > > > We have explored several ways to get rid of them completely in the idle > > path but have not yet come up with a clean way, but this series gets us > > a long ways towards that goal. > > have you thought about being a bit more aggressive at when to > runtime_get and runtime_put ? > > I didn't test below (will do probably on monday), but I think this will > help keeping GPIO block always suspended, and only wake it up when truly > needed. That way, you could, at some point, remove that list_head > because by the time you reach CPUIdle path, GPIO module is already > suspended. That's the theory at least, gotta run it first on silicon to > be sure > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 4273401..2dd9ced 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -537,12 +537,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip); > unsigned long flags; > > - /* > - * If this is the first gpio_request for the bank, > - * enable the bank module. > - */ > - if (!bank->mod_usage) > - pm_runtime_get_sync(bank->dev); > + pm_runtime_get_sync(bank->dev); bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only if all the GPIOs in a particular bank are enabled or disabled respectively. With the above change, pm_runtime_put*/get* would be called for every gpio_request() /_free() (that is, for upto 32 pins in OMAP3/4) in a bank irrespective of whether other GPIO pins are enabled or disabled in the same bank. Hence it is required to have a check based on mod_usage. I agree with your approach of aggressively doing pm_runtime* calls and this was discussed internally sometime ago. -V Charulatha > > spin_lock_irqsave(&bank->lock, flags); > /* Set trigger to none. You need to enable the desired trigger with > @@ -572,6 +567,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > > spin_unlock_irqrestore(&bank->lock, flags); > > + pm_runtime_put(bank->dev); > + > return 0; > } > > @@ -581,6 +578,8 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) > void __iomem *base = bank->base; > unsigned long flags; > > + pm_runtime_get_sync(bank->dev); > + > spin_lock_irqsave(&bank->lock, flags); > > if (bank->regs->wkup_en) { > @@ -606,12 +605,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) > _reset_gpio(bank, bank->chip.base + offset); > spin_unlock_irqrestore(&bank->lock, flags); > > - /* > - * If this is the last gpio to be freed in the bank, > - * disable the bank module. > - */ > - if (!bank->mod_usage) > - pm_runtime_put(bank->dev); > + pm_runtime_put(bank->dev); > } > > /* > @@ -707,9 +701,11 @@ static void gpio_irq_shutdown(struct irq_data *d) > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > unsigned long flags; > > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > _reset_gpio(bank, gpio); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > } > > static void gpio_ack_irq(struct irq_data *d) > @@ -717,7 +713,9 @@ static void gpio_ack_irq(struct irq_data *d) > unsigned int gpio = d->irq - IH_GPIO_BASE; > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > > + pm_runtime_get_sync(bank->dev); > _clear_gpio_irqstatus(bank, gpio); > + pm_runtime_put(bank->dev); > } > > static void gpio_mask_irq(struct irq_data *d) > @@ -726,10 +724,12 @@ static void gpio_mask_irq(struct irq_data *d) > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > unsigned long flags; > > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_irqenable(bank, gpio, 0); > _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > } > > static void gpio_unmask_irq(struct irq_data *d) > @@ -740,6 +740,7 @@ static void gpio_unmask_irq(struct irq_data *d) > u32 trigger = irqd_get_trigger_type(d); > unsigned long flags; > > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > if (trigger) > _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > @@ -753,6 +754,7 @@ static void gpio_unmask_irq(struct irq_data *d) > > _set_gpio_irqenable(bank, gpio, 1); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > } > > static struct irq_chip gpio_irq_chip = { > @@ -836,17 +838,26 @@ static int gpio_input(struct gpio_chip *chip, unsigned offset) > unsigned long flags; > > bank = container_of(chip, struct gpio_bank, chip); > + pm_runtime_get_sync(bank->dev); > + > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_direction(bank, offset, 1); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > + > return 0; > } > > 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; > + pm_runtime_get_sync(bank->dev); > + val = __raw_readl(reg) & mask; > + pm_runtime_put(bank->dev); > + > + return val; > } > > static int gpio_get(struct gpio_chip *chip, unsigned offset) > @@ -856,15 +867,20 @@ static int gpio_get(struct gpio_chip *chip, unsigned offset) > int gpio; > u32 mask; > > + int val; > gpio = chip->base + offset; > bank = container_of(chip, struct gpio_bank, chip); > reg = bank->base; > mask = GPIO_BIT(bank, gpio); > > + pm_runtime_get_sync(bank->dev); > if (gpio_is_input(bank, mask)) > - return _get_gpio_datain(bank, gpio); > + val = _get_gpio_datain(bank, gpio); > else > - return _get_gpio_dataout(bank, gpio); > + val = _get_gpio_dataout(bank, gpio); > + pm_runtime_put(bank->dev); > + > + return val; > } > > static int gpio_output(struct gpio_chip *chip, unsigned offset, int value) > @@ -873,10 +889,14 @@ static int gpio_output(struct gpio_chip *chip, unsigned offset, int value) > unsigned long flags; > > bank = container_of(chip, struct gpio_bank, chip); > + > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > bank->set_dataout(bank, offset, value); > _set_gpio_direction(bank, offset, 0); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > + > return 0; > } > > @@ -894,9 +914,11 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset, > dev_err(bank->dev, "Could not get gpio dbck\n"); > } > > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_debounce(bank, offset, debounce); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > > return 0; > } > @@ -907,9 +929,12 @@ static void gpio_set(struct gpio_chip *chip, unsigned offset, int value) > unsigned long flags; > > bank = container_of(chip, struct gpio_bank, chip); > + > + pm_runtime_get_sync(bank->dev); > spin_lock_irqsave(&bank->lock, flags); > bank->set_dataout(bank, offset, value); > spin_unlock_irqrestore(&bank->lock, flags); > + pm_runtime_put(bank->dev); > } > > static int gpio_2irq(struct gpio_chip *chip, unsigned offset) > @@ -1330,7 +1355,8 @@ void omap2_gpio_prepare_for_idle(int pwr_mode) > > bank->power_mode = pwr_mode; > > - pm_runtime_put_sync_suspend(bank->dev); > + if (!pm_runtime_suspended(bank->dev)) > + pm_runtime_suspend(bank->dev); > } > } > > @@ -1342,7 +1368,8 @@ void omap2_gpio_resume_after_idle(void) > if (!bank->mod_usage || !bank->loses_context) > continue; > > - pm_runtime_get_sync(bank->dev); > + if (pm_runtime_suspended(bank->dev)) > + pm_runtime_resume(bank->dev); > } > } > > > -- > balbi
Hi, On Sun, Feb 05, 2012 at 12:37:55PM +0530, Varadarajan, Charulatha wrote: > Felipe, > > On Sat, Feb 4, 2012 at 21:38, Felipe Balbi <balbi@ti.com> wrote: > > > > Hi, > > > > On Fri, Feb 03, 2012 at 09:50:19AM -0800, Kevin Hilman wrote: > > > Felipe Balbi <balbi@ti.com> writes: > > > > > > [...] > > > > > > >> >This question remains. Why do we need those funtions ? > > > >> > > > >> These functions are called from the CPUIdle path so outside the scope > > > >> of the GPIO driver. These are part of a bunch of nasty PM hacks we > > > >> are doing in the CPU idle loop. We are in the process of getting rid > > > >> of most of them, but it looks like some are still needed. > > > > > > > > Too bad. I can see that the gpio pm implementation seems a bit > > > > "peculiar". I mean, pm does reference counting and yet the driver has > > > > checks to prevent multiple gets and puts on a single bank (meaning that > > > > pm counter will be either 0 or 1 at any point in time). > > > > > > > > To me it looks like those functions are there in order to forcefully put > > > > PER power domain in OFF because drivers are always holding a reference > > > > to their gpios (drivers generally gpio_request() on probe() and > > > > gpio_free() on remove()). > > > > > > > > Looks like the entire pm implementation on OMAP gpio driver has always > > > > considered only the fact that gpios can be requested and freed, but > > > > never that we want the system to go to OFF even while gpios are > > > > requested, because we have I/O PAD wakeups. At some point that has to be > > > > sorted out because that HACK is quite ugly :-) > > > > > > > > I'll see if I find some time to go over the interactions between > > > > gpio-omap.c and pm24x.c and pm34xx.c any of these days, but I can't > > > > promise anything ;-) > > > > > > If you look at the state of these prepare/resume hacks at the end of > > > this series, you'll see that they are significantly cleaner and do > > > nothing but call the runtime PM hooks. > > > > sure, definitely. > > > > > We have explored several ways to get rid of them completely in the idle > > > path but have not yet come up with a clean way, but this series gets us > > > a long ways towards that goal. > > > > have you thought about being a bit more aggressive at when to > > runtime_get and runtime_put ? > > > > I didn't test below (will do probably on monday), but I think this will > > help keeping GPIO block always suspended, and only wake it up when truly > > needed. That way, you could, at some point, remove that list_head > > because by the time you reach CPUIdle path, GPIO module is already > > suspended. That's the theory at least, gotta run it first on silicon to > > be sure > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 4273401..2dd9ced 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -537,12 +537,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > > struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip); > > unsigned long flags; > > > > - /* > > - * If this is the first gpio_request for the bank, > > - * enable the bank module. > > - */ > > - if (!bank->mod_usage) > > - pm_runtime_get_sync(bank->dev); > > + pm_runtime_get_sync(bank->dev); > > bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only > if all the GPIOs in a particular bank are enabled or disabled respectively. and why should you care about that ? The first get will enable the resources you need, the second get will just increase a counter and so on. So if you have 32 gets, you will disable the module when you have 32 puts. > With the above change, pm_runtime_put*/get* would be called for every > gpio_request() > /_free() (that is, for upto 32 pins in OMAP3/4) in a bank irrespective > of whether other so ? > GPIO pins are enabled or disabled in the same bank. Hence it is > required to have a > check based on mod_usage. unnecessary.
On Sun, Feb 5, 2012 at 2:38 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Sun, Feb 05, 2012 at 12:37:55PM +0530, Varadarajan, Charulatha wrote: >> Felipe, >> >> On Sat, Feb 4, 2012 at 21:38, Felipe Balbi <balbi@ti.com> wrote: [....] >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only >> if all the GPIOs in a particular bank are enabled or disabled respectively. > > and why should you care about that ? The first get will enable the > resources you need, the second get will just increase a counter and so > on. So if you have 32 gets, you will disable the module when you have 32 > puts. > Am not sure if it is clear to you that the GPIO resources like clock, debounce clk are per bank wise and not per GPIO line. So doing 32 get/put per bank is stupid. runtime pm is for managing pm resources and if the pm resource is per bank, it has to be handled per bank. >> With the above change, pm_runtime_put*/get* would be called for every >> gpio_request() >> /_free() (that is, for upto 32 pins in OMAP3/4) in a bank irrespective >> of whether other > > so ? It's useless. > >> GPIO pins are enabled or disabled in the same bank. Hence it is >> required to have a >> check based on mod_usage. > > unnecessary. > I don't think so. Regards Santosh
Hi, On Sun, Feb 05, 2012 at 02:46:19PM +0530, Shilimkar, Santosh wrote: > >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only > >> if all the GPIOs in a particular bank are enabled or disabled respectively. > > > > and why should you care about that ? The first get will enable the > > resources you need, the second get will just increase a counter and so > > on. So if you have 32 gets, you will disable the module when you have 32 > > puts. > > > Am not sure if it is clear to you that the GPIO resources like clock, > debounce clk are per bank wise and not per GPIO line. So doing 32 this is just one more reason to have usage counters. > get/put per bank is stupid. runtime pm is for managing pm what's stupid is trying to use the pm usage counters as a binary flag, see below. > resources and if the pm resource is per bank, it has to be > handled per bank. hehe, what are you talking about Santosh ? That's the whole point of reference counting. If there are 32 users for 1 resource, you want to make sure that you only free the resources (clocks, or whatever resource you want) after all 32 users are done with it. Trying to use the pm usage counter as a binary flag, that's the stupid part of the OMAP GPIO driver. Instead of adding checks such as: if (module_isnt_used()) enable_clocks(); on get and: if (module_isnt_needed_anymore()) disable_clcoks() on put is the most useless piece of code on that driver. Because such checks are already available on PM core through usage counters. The way that part of the code works is as follow: get() { if (pm_counter_is_zero(dev)) { atomic_inc(); rpm_resume(); } } put() { atomic_dec(); if (pm_counter_is_zero(dev)) { rpm_suspend(); } } Do you not see that you're duplicating functionality by trying to use the pm counter a binary flag ? > >> With the above change, pm_runtime_put*/get* would be called for every > >> gpio_request() > >> /_free() (that is, for upto 32 pins in OMAP3/4) in a bank irrespective > >> of whether other > > > > so ? > It's useless. no, it's not. > >> GPIO pins are enabled or disabled in the same bank. Hence it is > >> required to have a > >> check based on mod_usage. > > > > unnecessary. > > > I don't think so. then show that to me with code. Looking at the usage counters on pm runtime, you're duplicating functionality with it and trying to use it as a binary flag. Preventing multiple gets/puts breaks the whole idea of _having_ usage counters to start with.
On Sun, Feb 5, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Sun, Feb 05, 2012 at 02:46:19PM +0530, Shilimkar, Santosh wrote: >> >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only >> >> if all the GPIOs in a particular bank are enabled or disabled respectively. >> > >> > and why should you care about that ? The first get will enable the >> > resources you need, the second get will just increase a counter and so >> > on. So if you have 32 gets, you will disable the module when you have 32 >> > puts. >> > >> Am not sure if it is clear to you that the GPIO resources like clock, >> debounce clk are per bank wise and not per GPIO line. So doing 32 > > this is just one more reason to have usage counters. > >> get/put per bank is stupid. runtime pm is for managing pm > > what's stupid is trying to use the pm usage counters as a binary flag, > see below. > >> resources and if the pm resource is per bank, it has to be >> handled per bank. > > hehe, what are you talking about Santosh ? That's the whole point of > reference counting. If there are 32 users for 1 resource, you want to > make sure that you only free the resources (clocks, or whatever resource > you want) after all 32 users are done with it. > > Trying to use the pm usage counter as a binary flag, that's the stupid > part of the OMAP GPIO driver. > > Instead of adding checks such as: > > if (module_isnt_used()) > enable_clocks(); > > on get and: > > if (module_isnt_needed_anymore()) > disable_clcoks() > > on put is the most useless piece of code on that driver. Because such > checks are already available on PM core through usage counters. The way > that part of the code works is as follow: > > get() { > if (pm_counter_is_zero(dev)) { > atomic_inc(); > > rpm_resume(); > } > } > > put() { > atomic_dec(); > > if (pm_counter_is_zero(dev)) { > rpm_suspend(); > } > } > > Do you not see that you're duplicating functionality by trying to use > the pm counter a binary flag ? > Ahh.. Now I see your point. I miss-understood the point first time and thought that we have disconnect on the pm resources from number of GPIO perspective. What you are saying is to use pm runtime reference counters rather than creating local ones for GPIO which seems to be right thing to do. Sorry for the noise. The agressive clock cutting was tried initially without much success and may be we can revisit this one more time. As per as this series is concerned, we would like to fix the build error pointed by Kevin and queue it for 3.4. There are few more fixes getting cooked up for GPIO and as part of that series we will have a look at your suggestion. Thanks for review. Regards santosh Regards Santosh
Felipe, On Sun, Feb 5, 2012 at 14:38, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Sun, Feb 05, 2012 at 12:37:55PM +0530, Varadarajan, Charulatha wrote: >> Felipe, >> >> On Sat, Feb 4, 2012 at 21:38, Felipe Balbi <balbi@ti.com> wrote: >> > >> > Hi, >> > >> > On Fri, Feb 03, 2012 at 09:50:19AM -0800, Kevin Hilman wrote: >> > > Felipe Balbi <balbi@ti.com> writes: >> > > >> > > [...] >> > > >> > > >> >This question remains. Why do we need those funtions ? >> > > >> >> > > >> These functions are called from the CPUIdle path so outside the scope >> > > >> of the GPIO driver. These are part of a bunch of nasty PM hacks we >> > > >> are doing in the CPU idle loop. We are in the process of getting rid >> > > >> of most of them, but it looks like some are still needed. >> > > > >> > > > Too bad. I can see that the gpio pm implementation seems a bit >> > > > "peculiar". I mean, pm does reference counting and yet the driver has >> > > > checks to prevent multiple gets and puts on a single bank (meaning that >> > > > pm counter will be either 0 or 1 at any point in time). >> > > > >> > > > To me it looks like those functions are there in order to forcefully put >> > > > PER power domain in OFF because drivers are always holding a reference >> > > > to their gpios (drivers generally gpio_request() on probe() and >> > > > gpio_free() on remove()). >> > > > >> > > > Looks like the entire pm implementation on OMAP gpio driver has always >> > > > considered only the fact that gpios can be requested and freed, but >> > > > never that we want the system to go to OFF even while gpios are >> > > > requested, because we have I/O PAD wakeups. At some point that has to be >> > > > sorted out because that HACK is quite ugly :-) >> > > > >> > > > I'll see if I find some time to go over the interactions between >> > > > gpio-omap.c and pm24x.c and pm34xx.c any of these days, but I can't >> > > > promise anything ;-) >> > > >> > > If you look at the state of these prepare/resume hacks at the end of >> > > this series, you'll see that they are significantly cleaner and do >> > > nothing but call the runtime PM hooks. >> > >> > sure, definitely. >> > >> > > We have explored several ways to get rid of them completely in the idle >> > > path but have not yet come up with a clean way, but this series gets us >> > > a long ways towards that goal. >> > >> > have you thought about being a bit more aggressive at when to >> > runtime_get and runtime_put ? >> > >> > I didn't test below (will do probably on monday), but I think this will >> > help keeping GPIO block always suspended, and only wake it up when truly >> > needed. That way, you could, at some point, remove that list_head >> > because by the time you reach CPUIdle path, GPIO module is already >> > suspended. That's the theory at least, gotta run it first on silicon to >> > be sure >> > >> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> > index 4273401..2dd9ced 100644 >> > --- a/drivers/gpio/gpio-omap.c >> > +++ b/drivers/gpio/gpio-omap.c >> > @@ -537,12 +537,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) >> > struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip); >> > unsigned long flags; >> > >> > - /* >> > - * If this is the first gpio_request for the bank, >> > - * enable the bank module. >> > - */ >> > - if (!bank->mod_usage) >> > - pm_runtime_get_sync(bank->dev); >> > + pm_runtime_get_sync(bank->dev); >> >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only >> if all the GPIOs in a particular bank are enabled or disabled respectively. > > and why should you care about that ? The first get will enable the > resources you need, the second get will just increase a counter and so > on. So if you have 32 gets, you will disable the module when you have 32 > puts. Agreed! -V Charulatha > >> With the above change, pm_runtime_put*/get* would be called for every >> gpio_request() >> /_free() (that is, for upto 32 pins in OMAP3/4) in a bank irrespective >> of whether other > > so ? > >> GPIO pins are enabled or disabled in the same bank. Hence it is >> required to have a >> check based on mod_usage. > > unnecessary. > > -- > balbi
On Sun, Feb 05, 2012 at 06:05:37PM +0530, Shilimkar, Santosh wrote: > On Sun, Feb 5, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Sun, Feb 05, 2012 at 02:46:19PM +0530, Shilimkar, Santosh wrote: > >> >> bank->mod_usage check is used to take care of doing pm_runtime_get*/put* only > >> >> if all the GPIOs in a particular bank are enabled or disabled respectively. > >> > > >> > and why should you care about that ? The first get will enable the > >> > resources you need, the second get will just increase a counter and so > >> > on. So if you have 32 gets, you will disable the module when you have 32 > >> > puts. > >> > > >> Am not sure if it is clear to you that the GPIO resources like clock, > >> debounce clk are per bank wise and not per GPIO line. So doing 32 > > > > this is just one more reason to have usage counters. > > > >> get/put per bank is stupid. runtime pm is for managing pm > > > > what's stupid is trying to use the pm usage counters as a binary flag, > > see below. > > > >> resources and if the pm resource is per bank, it has to be > >> handled per bank. > > > > hehe, what are you talking about Santosh ? That's the whole point of > > reference counting. If there are 32 users for 1 resource, you want to > > make sure that you only free the resources (clocks, or whatever resource > > you want) after all 32 users are done with it. > > > > Trying to use the pm usage counter as a binary flag, that's the stupid > > part of the OMAP GPIO driver. > > > > Instead of adding checks such as: > > > > if (module_isnt_used()) > > enable_clocks(); > > > > on get and: > > > > if (module_isnt_needed_anymore()) > > disable_clcoks() > > > > on put is the most useless piece of code on that driver. Because such > > checks are already available on PM core through usage counters. The way > > that part of the code works is as follow: > > > > get() { > > if (pm_counter_is_zero(dev)) { > > atomic_inc(); > > > > rpm_resume(); > > } > > } > > > > put() { > > atomic_dec(); > > > > if (pm_counter_is_zero(dev)) { > > rpm_suspend(); > > } > > } > > > > Do you not see that you're duplicating functionality by trying to use > > the pm counter a binary flag ? > > > Ahh.. Now I see your point. I miss-understood the point first time > and thought that we have disconnect on the pm resources from > number of GPIO perspective. > > What you are saying is to use pm runtime reference counters rather > than creating local ones for GPIO which seems to be right thing to > do. Sorry for the noise. no problem. > The agressive clock cutting was tried initially without much success > and may be we can revisit this one more time. I think one issue will be wrt to IRQ handling. If you want pm_runtime_* calls to be irq_safe, they will keep the parent always on, so we need to find a way to make that part work properly, I guess. > As per as this series is concerned, we would like to fix > the build error pointed by Kevin and queue it for 3.4. sure, go ahead. No problems with that at all.
Kevin, On Sat, Feb 4, 2012 at 2:31 AM, Kevin Hilman <khilman@ti.com> wrote: > > Hi Tarun, > > Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: > > > The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: > > Linus Torvalds (1): > > Linux 3.3-rc2 > > > > are available in the git repository at: > > http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev > > Branch: for_3.4/gpio_cleanup_fixes_v9 > > > > This series is continuation of cleanup of OMAP GPIO driver and fixes. > > The cleanup include getting rid of cpu_is_* checks wherever possible, > > use of gpio_bank list instead of static array, use of unique platform > > specific value associated data member to OMAP platforms to avoid > > cpu_is_* checks. The series also include PM runtime support. > > This version is looking pretty good, and I'm almost ready to queue it up > for Grant. > > However, one more minor nit. Please fix up the compile warnings when > built without CONFIG_PM_SLEEP or CONFIG_PM_RUNTIME. > > The definitons of the dev_pm_ops callbacks need to be conditional when > using the SET_*_PM_OPS() macros, otherwise you get these warnings: I have updated the branch with the required changes. -- Tarun > > > > CC drivers/gpio/gpio-omap.o > /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1142:12: warning: 'omap_gpio_suspend' defined but not used [-Wunused-function] > /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1167:12: warning: 'omap_gpio_resume' defined but not used [-Wunused-function] > /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1191:12: warning: 'omap_gpio_runtime_suspend' defined but not used [-Wunused-function] > /work/kernel/omap/pm/drivers/gpio/gpio-omap.c:1237:12: warning: 'omap_gpio_runtime_resume' defined but not used [-Wunused-function] > > > Thanks, > > Kevin
Hi Tarun, * Tarun Kanti DebBarma <tarun.kanti@ti.com> [120202 09:00]: > > Used following patches to avoid exception during system suspend:- > [PATCH RFC 1/2] mtd : Prevent the NULL pointer access > [PATCH RFC 2/2] mtd : Make the mtd_suspend return 0 if the suspend is not implemented OK so MTD fixes those are supposedly going in during the -rc cycle.. > # echo mem > /sys/power/state > [ 47.128021] PM: Syncing filesystems ... done. > [ 47.144104] Freezing user space processes ... (elapsed 0.01 seconds) done. > [ 47.168243] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) don e. > [ 47.205139] Unable to handle kernel NULL pointer dereference at virtual addre ss 000000a0 > [ 47.213317] pgd = deaac000 > [ 47.216033] [000000a0] *pgd=9e932831, *pte=00000000, *ppte=00000000 > [ 47.222381] Internal error: Oops: 17 [#1] SMP > [ 47.226745] Modules linked in: > [ 47.229827] CPU: 0 Not tainted (3.3.0-rc2-00031-g12c5c5c #235) > [ 47.236022] PC is at mtd_cls_suspend+0x8/0x20 > [ 47.240386] LR is at mtd_cls_suspend+0x8/0x20 > [ 47.244750] pc : [<c02e78f8>] lr : [<c02e78f8>] psr: a0000013 > [ 47.244750] sp : dea1fe60 ip : 22222222 fp : c0ee7d40 > [ 47.256256] r10: c0ee7cf0 r9 : 00000000 r8 : c02e78f0 > [ 47.261474] r7 : 00000000 r6 : 00000000 r5 : 00000002 r4 : dea45800 > [ 47.268005] r3 : deb4cac0 r2 : 00000000 r1 : 00000002 r0 : 00000000 > [ 47.274536] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > [ 47.281677] Control: 10c5387d Table: 9eaac019 DAC: 00000015 > [ 47.287445] Process sh (pid: 1177, stack limit = 0xdea1e2f8) > [ 47.293090] Stack: (0xdea1fe60 to 0xdea20000) > [...] ..but are there also some patches in your series that should also be applied separately as fixes for the -rc cycle? Regards, Tony
Tony Lindgren <tony@atomide.com> writes: [...] > ..but are there also some patches in your series that should also be > applied separately as fixes for the -rc cycle? IMO, there aren't any things in this series that fall into the regression category. After I get to a final review and test of this (early next week), I'm planning to sent a pull request to Grant. Kevin
On Sat, Feb 11, 2012 at 1:25 AM, Kevin Hilman <khilman@ti.com> wrote: > Tony Lindgren <tony@atomide.com> writes: > > [...] > >> ..but are there also some patches in your series that should also be >> applied separately as fixes for the -rc cycle? > > IMO, there aren't any things in this series that fall into the > regression category. That's right. -- Tarun > > After I get to a final review and test of this (early next week), I'm > planning to sent a pull request to Grant. > > Kevin
On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: > With register offsets now defined for respective OMAP versions we can get rid > of cpu_class_* checks. This function now has common initialization code for > all OMAP versions... > > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Signed-off-by: Charulatha V <charu@ti.com> > Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> Sorry for being so late with my comment for chanes already present in mainline, but this patch breaks GPIO on Amstrad Delta at least, and I have neither enough spare time nor enough experience with non OMAP1 machines to provide a solution myself. > --- > arch/arm/mach-omap1/gpio16xx.c | 35 +++++++++++++++++- > drivers/gpio/gpio-omap.c | 77 ++++++++++++---------------------------- > 2 files changed, 57 insertions(+), 55 deletions(-) ... > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index f39d9e4..a948351 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c ... > @@ -898,62 +896,30 @@ static void __init omap_gpio_show_rev(struct gpio_bank *bank) > */ > static struct lock_class_key gpio_lock_class; > > -/* TODO: Cleanup cpu_is_* checks */ > static void omap_gpio_mod_init(struct gpio_bank *bank) > { > - if (cpu_class_is_omap2()) { > - if (cpu_is_omap44xx()) { > - __raw_writel(0xffffffff, bank->base + > - OMAP4_GPIO_IRQSTATUSCLR0); > - __raw_writel(0x00000000, bank->base + > - OMAP4_GPIO_DEBOUNCENABLE); > - /* Initialize interface clk ungated, module enabled */ > - __raw_writel(0, bank->base + OMAP4_GPIO_CTRL); > - } else if (cpu_is_omap34xx()) { > - __raw_writel(0x00000000, bank->base + > - OMAP24XX_GPIO_IRQENABLE1); > - __raw_writel(0xffffffff, bank->base + > - OMAP24XX_GPIO_IRQSTATUS1); > - __raw_writel(0x00000000, bank->base + > - OMAP24XX_GPIO_DEBOUNCE_EN); > - > - /* Initialize interface clk ungated, module enabled */ > - __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL); > - } > - } else if (cpu_class_is_omap1()) { > - if (bank_is_mpuio(bank)) { > - __raw_writew(0xffff, bank->base + > - OMAP_MPUIO_GPIO_MASKIT / bank->stride); > - mpuio_init(bank); > - } > - if (cpu_is_omap15xx() && bank->method == METHOD_GPIO_1510) { > - __raw_writew(0xffff, bank->base > - + OMAP1510_GPIO_INT_MASK); > - __raw_writew(0x0000, bank->base > - + OMAP1510_GPIO_INT_STATUS); > - } > - if (cpu_is_omap16xx() && bank->method == METHOD_GPIO_1610) { > - __raw_writew(0x0000, bank->base > - + OMAP1610_GPIO_IRQENABLE1); > - __raw_writew(0xffff, bank->base > - + OMAP1610_GPIO_IRQSTATUS1); > - __raw_writew(0x0014, bank->base > - + OMAP1610_GPIO_SYSCONFIG); > + void __iomem *base = bank->base; > + u32 l = 0xffffffff; > > - /* > - * Enable system clock for GPIO module. > - * The CAM_CLK_CTRL *is* really the right place. > - */ > - omap_writel(omap_readl(ULPD_CAM_CLK_CTRL) | 0x04, > - ULPD_CAM_CLK_CTRL); > - } > - if (cpu_is_omap7xx() && bank->method == METHOD_GPIO_7XX) { > - __raw_writel(0xffffffff, bank->base > - + OMAP7XX_GPIO_INT_MASK); > - __raw_writel(0x00000000, bank->base > - + OMAP7XX_GPIO_INT_STATUS); > - } > + if (bank->width == 16) > + l = 0xffff; > + > + if (bank_is_mpuio(bank)) { > + __raw_writel(l, bank->base + bank->regs->irqenable); > + return; > } > + > + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv); > + _gpio_rmw(base, bank->regs->irqstatus, l, > + bank->regs->irqenable_inv == false); > + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0); > + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0); bank->regs->irqenable trgister is manipulated 3 times in a row, each time based on different criteria. This breaks GPIO on Amstrad Delta at least, and generally looks wrong. I was only able to verify that commenting out the above two lines fixes the issue with Amstrad Delta for me. > + if (bank->regs->debounce_en) > + _gpio_rmw(base, bank->regs->debounce_en, 0, 1); This also looks suspicious, I guess the second line should rather be: _gpio_rmw(base, bank->regs->debounce, 0, 1); > + > + /* Initialize interface clk ungated, module enabled */ > + if (bank->regs->ctrl) > + _gpio_rmw(base, bank->regs->ctrl, 0, 1); Is bank->regs->ctrl a flag, or a register offset? If the latter, is that offset guaranteed to never take a valid value of 0? Thanks, Janusz
On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote: > On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: >> With register offsets now defined for respective OMAP versions we can get rid >> of cpu_class_* checks. This function now has common initialization code for >> all OMAP versions... >> >> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> Signed-off-by: Charulatha V <charu@ti.com> >> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Acked-by: Tony Lindgren <tony@atomide.com> > > Sorry for being so late with my comment for chanes already present in > mainline, but this patch breaks GPIO on Amstrad Delta at least, and I > have neither enough spare time nor enough experience with non OMAP1 > machines to provide a solution myself. Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are overwritten. Also looks like there is issue in making distinction between omap15xx and omap16xx. I will post a patch and you can help me testing it in OMAP1 platform. Thanks for pointing this out. -- Tarun > >> --- >> arch/arm/mach-omap1/gpio16xx.c | 35 +++++++++++++++++- >> drivers/gpio/gpio-omap.c | 77 ++++++++++++---------------------------- >> 2 files changed, 57 insertions(+), 55 deletions(-) > ... >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index f39d9e4..a948351 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c > ... >> @@ -898,62 +896,30 @@ static void __init omap_gpio_show_rev(struct gpio_bank *bank) >> */ >> static struct lock_class_key gpio_lock_class; >> >> -/* TODO: Cleanup cpu_is_* checks */ >> static void omap_gpio_mod_init(struct gpio_bank *bank) >> { >> - if (cpu_class_is_omap2()) { >> - if (cpu_is_omap44xx()) { >> - __raw_writel(0xffffffff, bank->base + >> - OMAP4_GPIO_IRQSTATUSCLR0); >> - __raw_writel(0x00000000, bank->base + >> - OMAP4_GPIO_DEBOUNCENABLE); >> - /* Initialize interface clk ungated, module enabled */ >> - __raw_writel(0, bank->base + OMAP4_GPIO_CTRL); >> - } else if (cpu_is_omap34xx()) { >> - __raw_writel(0x00000000, bank->base + >> - OMAP24XX_GPIO_IRQENABLE1); >> - __raw_writel(0xffffffff, bank->base + >> - OMAP24XX_GPIO_IRQSTATUS1); >> - __raw_writel(0x00000000, bank->base + >> - OMAP24XX_GPIO_DEBOUNCE_EN); >> - >> - /* Initialize interface clk ungated, module enabled */ >> - __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL); >> - } >> - } else if (cpu_class_is_omap1()) { >> - if (bank_is_mpuio(bank)) { >> - __raw_writew(0xffff, bank->base + >> - OMAP_MPUIO_GPIO_MASKIT / bank->stride); >> - mpuio_init(bank); >> - } >> - if (cpu_is_omap15xx() && bank->method == METHOD_GPIO_1510) { >> - __raw_writew(0xffff, bank->base >> - + OMAP1510_GPIO_INT_MASK); >> - __raw_writew(0x0000, bank->base >> - + OMAP1510_GPIO_INT_STATUS); >> - } >> - if (cpu_is_omap16xx() && bank->method == METHOD_GPIO_1610) { >> - __raw_writew(0x0000, bank->base >> - + OMAP1610_GPIO_IRQENABLE1); >> - __raw_writew(0xffff, bank->base >> - + OMAP1610_GPIO_IRQSTATUS1); >> - __raw_writew(0x0014, bank->base >> - + OMAP1610_GPIO_SYSCONFIG); >> + void __iomem *base = bank->base; >> + u32 l = 0xffffffff; >> >> - /* >> - * Enable system clock for GPIO module. >> - * The CAM_CLK_CTRL *is* really the right place. >> - */ >> - omap_writel(omap_readl(ULPD_CAM_CLK_CTRL) | 0x04, >> - ULPD_CAM_CLK_CTRL); >> - } >> - if (cpu_is_omap7xx() && bank->method == METHOD_GPIO_7XX) { >> - __raw_writel(0xffffffff, bank->base >> - + OMAP7XX_GPIO_INT_MASK); >> - __raw_writel(0x00000000, bank->base >> - + OMAP7XX_GPIO_INT_STATUS); >> - } >> + if (bank->width == 16) >> + l = 0xffff; >> + >> + if (bank_is_mpuio(bank)) { >> + __raw_writel(l, bank->base + bank->regs->irqenable); >> + return; >> } >> + >> + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv); >> + _gpio_rmw(base, bank->regs->irqstatus, l, >> + bank->regs->irqenable_inv == false); >> + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0); >> + _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0); > > bank->regs->irqenable trgister is manipulated 3 times in a row, each > time based on different criteria. This breaks GPIO on Amstrad Delta at > least, and generally looks wrong. I was only able to verify that > commenting out the above two lines fixes the issue with Amstrad Delta for > me. > >> + if (bank->regs->debounce_en) >> + _gpio_rmw(base, bank->regs->debounce_en, 0, 1); > > This also looks suspicious, I guess the second line should rather be: > > _gpio_rmw(base, bank->regs->debounce, 0, 1); > >> + >> + /* Initialize interface clk ungated, module enabled */ >> + if (bank->regs->ctrl) >> + _gpio_rmw(base, bank->regs->ctrl, 0, 1); > > Is bank->regs->ctrl a flag, or a register offset? If the latter, is that > offset guaranteed to never take a valid value of 0? > > Thanks, > Janusz
* DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: > Hi Janusz, > > On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti > <tarun.kanti@ti.com> wrote: > > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik > > <jkrzyszt@tis.icnet.pl> wrote: > >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: > >>> With register offsets now defined for respective OMAP versions we can get rid > >>> of cpu_class_* checks. This function now has common initialization code for > >>> all OMAP versions... > >>> > >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> > >>> Signed-off-by: Charulatha V <charu@ti.com> > >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>> Acked-by: Tony Lindgren <tony@atomide.com> > >> > >> Sorry for being so late with my comment for chanes already present in > >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I > >> have neither enough spare time nor enough experience with non OMAP1 > >> machines to provide a solution myself. > > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are > > overwritten. > > Also looks like there is issue in making distinction between omap15xx > > and omap16xx. > > I will post a patch and you can help me testing it in OMAP1 platform. > > Thanks for pointing this out. ... > Here is the patch, with attachment as well. I have just tested on > OMAP4 platform. > Testing on other OMAP2+ platforms is pending. In the meantime can you please > validate on OMAP1 platform and confirm? Thanks. > -- > Tarun > > From: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Date: Tue, 24 Apr 2012 20:34:32 +0530 > Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init > > Initialization of irqenable, irqstatus registers is the common > operation done in this function for all OMAP platforms, viz. > OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register > was overwriting the correctly programmed OMAP1 value at the > beginning. As a result, even though it worked on OMAP2+ > platforms it was breaking OMAP1 functionality. Sounds like the original patch was never tested on omap1? > On closer observation it is found that the first _gpio_rmw() > which is supposedly done to take care of OMAP1 platform is > generic enough and takes care of OMAP2+ platform as well. > Therefore remove the latter _gpio_rmw() to irqenable as they > are redundant. > > Also, changing the sequence and logic of initializing the > irqstatus. Please mention also the breaking commit here and get this fix merged as a regression as soon as it's tested for the current -rc series. Regards, Tony > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> > --- > drivers/gpio/gpio-omap.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 1adc2ec..b8f01c1 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -964,11 +964,8 @@ static void omap_gpio_mod_init(struct gpio_bank *bank) > return; > } > > + _gpio_rmw(base, bank->regs->irqstatus, l, > bank->regs->irqenable_inv == 0 ); > _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv); > - _gpio_rmw(base, bank->regs->irqstatus, l, > - bank->regs->irqenable_inv == false); > - _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0); > - _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0); > if (bank->regs->debounce_en) > _gpio_rmw(base, bank->regs->debounce_en, 0, 1); > > -- > 1.7.0.4
Dnia wtorek, 24 kwietnia 2012 21:06:59 DebBarma, Tarun Kanti pisze: > Hi Janusz, > > Here is the patch, with attachment as well. I have just tested on > OMAP4 platform. > Testing on other OMAP2+ platforms is pending. In the meantime can you please > validate on OMAP1 platform and confirm? Thanks. Hi, Please give me a day or two. Thanks, Janusz
On Tue, Apr 24, 2012 at 9:34 PM, Tony Lindgren <tony@atomide.com> wrote: > * DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: >> Hi Janusz, >> >> On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti >> <tarun.kanti@ti.com> wrote: >> > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik >> > <jkrzyszt@tis.icnet.pl> wrote: >> >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: >> >>> With register offsets now defined for respective OMAP versions we can get rid >> >>> of cpu_class_* checks. This function now has common initialization code for >> >>> all OMAP versions... >> >>> >> >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> >>> Signed-off-by: Charulatha V <charu@ti.com> >> >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> >>> Acked-by: Tony Lindgren <tony@atomide.com> >> >> >> >> Sorry for being so late with my comment for chanes already present in >> >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I >> >> have neither enough spare time nor enough experience with non OMAP1 >> >> machines to provide a solution myself. >> > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are >> > overwritten. >> > Also looks like there is issue in making distinction between omap15xx >> > and omap16xx. >> > I will post a patch and you can help me testing it in OMAP1 platform. >> > Thanks for pointing this out. > ... > >> Here is the patch, with attachment as well. I have just tested on >> OMAP4 platform. >> Testing on other OMAP2+ platforms is pending. In the meantime can you please >> validate on OMAP1 platform and confirm? Thanks. >> -- >> Tarun >> >> From: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> Date: Tue, 24 Apr 2012 20:34:32 +0530 >> Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init >> >> Initialization of irqenable, irqstatus registers is the common >> operation done in this function for all OMAP platforms, viz. >> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register >> was overwriting the correctly programmed OMAP1 value at the >> beginning. As a result, even though it worked on OMAP2+ >> platforms it was breaking OMAP1 functionality. > > Sounds like the original patch was never tested on omap1? That's right, only bootup test was done on OMAP1710-SDP. > >> On closer observation it is found that the first _gpio_rmw() >> which is supposedly done to take care of OMAP1 platform is >> generic enough and takes care of OMAP2+ platform as well. >> Therefore remove the latter _gpio_rmw() to irqenable as they >> are redundant. >> >> Also, changing the sequence and logic of initializing the >> irqstatus. > > Please mention also the breaking commit here and get this fix > merged as a regression as soon as it's tested for the current > -rc series. Sure, I will do that! -- Tarun > > Regards, > > Tony > > >> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> --- >> drivers/gpio/gpio-omap.c | 5 +---- >> 1 files changed, 1 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 1adc2ec..b8f01c1 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -964,11 +964,8 @@ static void omap_gpio_mod_init(struct gpio_bank *bank) >> return; >> } >> >> + _gpio_rmw(base, bank->regs->irqstatus, l, >> bank->regs->irqenable_inv == 0 ); >> _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv); >> - _gpio_rmw(base, bank->regs->irqstatus, l, >> - bank->regs->irqenable_inv == false); >> - _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0); >> - _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0); >> if (bank->regs->debounce_en) >> _gpio_rmw(base, bank->regs->debounce_en, 0, 1); >> >> -- >> 1.7.0.4 > >
On Wed, Apr 25, 2012 at 10:04 AM, DebBarma, Tarun Kanti <tarun.kanti@ti.com> wrote: > On Tue, Apr 24, 2012 at 9:34 PM, Tony Lindgren <tony@atomide.com> wrote: >> * DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: >>> Hi Janusz, >>> >>> On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti >>> <tarun.kanti@ti.com> wrote: >>> > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik >>> > <jkrzyszt@tis.icnet.pl> wrote: >>> >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: >>> >>> With register offsets now defined for respective OMAP versions we can get rid >>> >>> of cpu_class_* checks. This function now has common initialization code for >>> >>> all OMAP versions... >>> >>> >>> >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >>> >>> Signed-off-by: Charulatha V <charu@ti.com> >>> >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> >>> Acked-by: Tony Lindgren <tony@atomide.com> >>> >> >>> >> Sorry for being so late with my comment for chanes already present in >>> >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I >>> >> have neither enough spare time nor enough experience with non OMAP1 >>> >> machines to provide a solution myself. >>> > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are >>> > overwritten. >>> > Also looks like there is issue in making distinction between omap15xx >>> > and omap16xx. >>> > I will post a patch and you can help me testing it in OMAP1 platform. >>> > Thanks for pointing this out. >> ... >> >>> Here is the patch, with attachment as well. I have just tested on >>> OMAP4 platform. >>> Testing on other OMAP2+ platforms is pending. In the meantime can you please >>> validate on OMAP1 platform and confirm? Thanks. >>> -- >>> Tarun >>> >>> From: Tarun Kanti DebBarma <tarun.kanti@ti.com> >>> Date: Tue, 24 Apr 2012 20:34:32 +0530 >>> Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init >>> >>> Initialization of irqenable, irqstatus registers is the common >>> operation done in this function for all OMAP platforms, viz. >>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register >>> was overwriting the correctly programmed OMAP1 value at the >>> beginning. As a result, even though it worked on OMAP2+ >>> platforms it was breaking OMAP1 functionality. >> >> Sounds like the original patch was never tested on omap1? > That's right, only bootup test was done on OMAP1710-SDP. > >> >>> On closer observation it is found that the first _gpio_rmw() >>> which is supposedly done to take care of OMAP1 platform is >>> generic enough and takes care of OMAP2+ platform as well. >>> Therefore remove the latter _gpio_rmw() to irqenable as they >>> are redundant. >>> >>> Also, changing the sequence and logic of initializing the >>> irqstatus. >> >> Please mention also the breaking commit here and get this fix >> merged as a regression as soon as it's tested for the current >> -rc series. > Sure, I will do that! Looks like it is regression on 3.4 as well so CC stable when you post the patch. Regards Santosh
On Wed, Apr 25, 2012 at 12:09 PM, Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote: > On Wed, Apr 25, 2012 at 10:04 AM, DebBarma, Tarun Kanti > <tarun.kanti@ti.com> wrote: >> On Tue, Apr 24, 2012 at 9:34 PM, Tony Lindgren <tony@atomide.com> wrote: >>> * DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: >>>> Hi Janusz, >>>> >>>> On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti >>>> <tarun.kanti@ti.com> wrote: >>>> > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik >>>> > <jkrzyszt@tis.icnet.pl> wrote: >>>> >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: >>>> >>> With register offsets now defined for respective OMAP versions we can get rid >>>> >>> of cpu_class_* checks. This function now has common initialization code for >>>> >>> all OMAP versions... >>>> >>> >>>> >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >>>> >>> Signed-off-by: Charulatha V <charu@ti.com> >>>> >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> >>> Acked-by: Tony Lindgren <tony@atomide.com> >>>> >> >>>> >> Sorry for being so late with my comment for chanes already present in >>>> >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I >>>> >> have neither enough spare time nor enough experience with non OMAP1 >>>> >> machines to provide a solution myself. >>>> > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are >>>> > overwritten. >>>> > Also looks like there is issue in making distinction between omap15xx >>>> > and omap16xx. >>>> > I will post a patch and you can help me testing it in OMAP1 platform. >>>> > Thanks for pointing this out. >>> ... >>> >>>> Here is the patch, with attachment as well. I have just tested on >>>> OMAP4 platform. >>>> Testing on other OMAP2+ platforms is pending. In the meantime can you please >>>> validate on OMAP1 platform and confirm? Thanks. >>>> -- >>>> Tarun >>>> >>>> From: Tarun Kanti DebBarma <tarun.kanti@ti.com> >>>> Date: Tue, 24 Apr 2012 20:34:32 +0530 >>>> Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init >>>> >>>> Initialization of irqenable, irqstatus registers is the common >>>> operation done in this function for all OMAP platforms, viz. >>>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register >>>> was overwriting the correctly programmed OMAP1 value at the >>>> beginning. As a result, even though it worked on OMAP2+ >>>> platforms it was breaking OMAP1 functionality. >>> >>> Sounds like the original patch was never tested on omap1? >> That's right, only bootup test was done on OMAP1710-SDP. >> >>> >>>> On closer observation it is found that the first _gpio_rmw() >>>> which is supposedly done to take care of OMAP1 platform is >>>> generic enough and takes care of OMAP2+ platform as well. >>>> Therefore remove the latter _gpio_rmw() to irqenable as they >>>> are redundant. >>>> >>>> Also, changing the sequence and logic of initializing the >>>> irqstatus. >>> >>> Please mention also the breaking commit here and get this fix >>> merged as a regression as soon as it's tested for the current >>> -rc series. >> Sure, I will do that! > > Looks like it is regression on 3.4 as well so CC stable when you > post the patch. Ok, I will do that. -- Tarun > > Regards > Santosh
On Wed, Apr 25, 2012 at 06:24:14PM +0530, DebBarma, Tarun Kanti wrote: > On Wed, Apr 25, 2012 at 12:09 PM, Shilimkar, Santosh > <santosh.shilimkar@ti.com> wrote: > > On Wed, Apr 25, 2012 at 10:04 AM, DebBarma, Tarun Kanti > > <tarun.kanti@ti.com> wrote: > >> On Tue, Apr 24, 2012 at 9:34 PM, Tony Lindgren <tony@atomide.com> wrote: > >>> * DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: > >>>> Hi Janusz, > >>>> > >>>> On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti > >>>> <tarun.kanti@ti.com> wrote: > >>>> > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik > >>>> > <jkrzyszt@tis.icnet.pl> wrote: > >>>> >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: > >>>> >>> With register offsets now defined for respective OMAP versions we can get rid > >>>> >>> of cpu_class_* checks. This function now has common initialization code for > >>>> >>> all OMAP versions... > >>>> >>> > >>>> >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> > >>>> >>> Signed-off-by: Charulatha V <charu@ti.com> > >>>> >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>>> >>> Acked-by: Tony Lindgren <tony@atomide.com> > >>>> >> > >>>> >> Sorry for being so late with my comment for chanes already present in > >>>> >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I > >>>> >> have neither enough spare time nor enough experience with non OMAP1 > >>>> >> machines to provide a solution myself. > >>>> > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are > >>>> > overwritten. > >>>> > Also looks like there is issue in making distinction between omap15xx > >>>> > and omap16xx. > >>>> > I will post a patch and you can help me testing it in OMAP1 platform. > >>>> > Thanks for pointing this out. > >>> ... > >>> > >>>> Here is the patch, with attachment as well. I have just tested on > >>>> OMAP4 platform. > >>>> Testing on other OMAP2+ platforms is pending. In the meantime can you please > >>>> validate on OMAP1 platform and confirm? Thanks. > >>>> -- > >>>> Tarun > >>>> > >>>> From: Tarun Kanti DebBarma <tarun.kanti@ti.com> > >>>> Date: Tue, 24 Apr 2012 20:34:32 +0530 > >>>> Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init > >>>> > >>>> Initialization of irqenable, irqstatus registers is the common > >>>> operation done in this function for all OMAP platforms, viz. > >>>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register > >>>> was overwriting the correctly programmed OMAP1 value at the > >>>> beginning. As a result, even though it worked on OMAP2+ > >>>> platforms it was breaking OMAP1 functionality. > >>> > >>> Sounds like the original patch was never tested on omap1? > >> That's right, only bootup test was done on OMAP1710-SDP. > >> > >>> > >>>> On closer observation it is found that the first _gpio_rmw() > >>>> which is supposedly done to take care of OMAP1 platform is > >>>> generic enough and takes care of OMAP2+ platform as well. > >>>> Therefore remove the latter _gpio_rmw() to irqenable as they > >>>> are redundant. > >>>> > >>>> Also, changing the sequence and logic of initializing the > >>>> irqstatus. > >>> > >>> Please mention also the breaking commit here and get this fix > >>> merged as a regression as soon as it's tested for the current > >>> -rc series. > >> Sure, I will do that! > > > > Looks like it is regression on 3.4 as well so CC stable when you > > post the patch. > Ok, I will do that. Correction. Don't email your patch in any way to the stable folk _before_ it has been taken into Linus' tree. However, you _may_ add in the patch attributations a Cc: <stable@vger.kernel.org> tag if you want the stable folk to automatically pick up your patch when it _does_ end up in Linus' tree. But... make sure that git send-email or whatever doesn't automatically add that to the recipients for the emailed patch. If you send the stable people a patch before its in mainline, you'll get a whinge telling you to read Documentation/stable_kernel_rules.txt
On Wed, Apr 25, 2012 at 7:15 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Apr 25, 2012 at 06:24:14PM +0530, DebBarma, Tarun Kanti wrote: >> On Wed, Apr 25, 2012 at 12:09 PM, Shilimkar, Santosh >> <santosh.shilimkar@ti.com> wrote: >> > On Wed, Apr 25, 2012 at 10:04 AM, DebBarma, Tarun Kanti >> > <tarun.kanti@ti.com> wrote: >> >> On Tue, Apr 24, 2012 at 9:34 PM, Tony Lindgren <tony@atomide.com> wrote: >> >>> * DebBarma, Tarun Kanti <tarun.kanti@ti.com> [120424 08:40]: >> >>>> Hi Janusz, >> >>>> >> >>>> On Tue, Apr 24, 2012 at 12:24 AM, DebBarma, Tarun Kanti >> >>>> <tarun.kanti@ti.com> wrote: >> >>>> > On Sat, Apr 21, 2012 at 7:33 PM, Janusz Krzysztofik >> >>>> > <jkrzyszt@tis.icnet.pl> wrote: >> >>>> >> On Thursday 02 of February 2012 23:00:37 Tarun Kanti DebBarma wrote: >> >>>> >>> With register offsets now defined for respective OMAP versions we can get rid >> >>>> >>> of cpu_class_* checks. This function now has common initialization code for >> >>>> >>> all OMAP versions... >> >>>> >>> >> >>>> >>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> >>>> >>> Signed-off-by: Charulatha V <charu@ti.com> >> >>>> >>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> >>>> >>> Acked-by: Tony Lindgren <tony@atomide.com> >> >>>> >> >> >>>> >> Sorry for being so late with my comment for chanes already present in >> >>>> >> mainline, but this patch breaks GPIO on Amstrad Delta at least, and I >> >>>> >> have neither enough spare time nor enough experience with non OMAP1 >> >>>> >> machines to provide a solution myself. >> >>>> > Yes, I looked at the omap_gpio_mod_init() and OMAP1 configurations are >> >>>> > overwritten. >> >>>> > Also looks like there is issue in making distinction between omap15xx >> >>>> > and omap16xx. >> >>>> > I will post a patch and you can help me testing it in OMAP1 platform. >> >>>> > Thanks for pointing this out. >> >>> ... >> >>> >> >>>> Here is the patch, with attachment as well. I have just tested on >> >>>> OMAP4 platform. >> >>>> Testing on other OMAP2+ platforms is pending. In the meantime can you please >> >>>> validate on OMAP1 platform and confirm? Thanks. >> >>>> -- >> >>>> Tarun >> >>>> >> >>>> From: Tarun Kanti DebBarma <tarun.kanti@ti.com> >> >>>> Date: Tue, 24 Apr 2012 20:34:32 +0530 >> >>>> Subject: [PATCH] gpio/omap: fix omap1 register overwrite in omap_gpio_mod_init >> >>>> >> >>>> Initialization of irqenable, irqstatus registers is the common >> >>>> operation done in this function for all OMAP platforms, viz. >> >>>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register >> >>>> was overwriting the correctly programmed OMAP1 value at the >> >>>> beginning. As a result, even though it worked on OMAP2+ >> >>>> platforms it was breaking OMAP1 functionality. >> >>> >> >>> Sounds like the original patch was never tested on omap1? >> >> That's right, only bootup test was done on OMAP1710-SDP. >> >> >> >>> >> >>>> On closer observation it is found that the first _gpio_rmw() >> >>>> which is supposedly done to take care of OMAP1 platform is >> >>>> generic enough and takes care of OMAP2+ platform as well. >> >>>> Therefore remove the latter _gpio_rmw() to irqenable as they >> >>>> are redundant. >> >>>> >> >>>> Also, changing the sequence and logic of initializing the >> >>>> irqstatus. >> >>> >> >>> Please mention also the breaking commit here and get this fix >> >>> merged as a regression as soon as it's tested for the current >> >>> -rc series. >> >> Sure, I will do that! >> > >> > Looks like it is regression on 3.4 as well so CC stable when you >> > post the patch. >> Ok, I will do that. > > Correction. > > Don't email your patch in any way to the stable folk _before_ it has been > taken into Linus' tree. However, you _may_ add in the patch attributations > a Cc: <stable@vger.kernel.org> tag if you want the stable folk to > automatically pick up your patch when it _does_ end up in Linus' tree. > But... make sure that git send-email or whatever doesn't automatically > add that to the recipients for the emailed patch. > > If you send the stable people a patch before its in mainline, you'll get > a whinge telling you to read Documentation/stable_kernel_rules.txt Alright, I will add Cc: <stable@vger.kernel.org> tag in the patch. Thanks. -- Tarun
"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes: > On Wed, Apr 25, 2012 at 7:15 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: [...] >> Correction. >> >> Don't email your patch in any way to the stable folk _before_ it has been >> taken into Linus' tree. However, you _may_ add in the patch attributations >> a Cc: <stable@vger.kernel.org> tag if you want the stable folk to >> automatically pick up your patch when it _does_ end up in Linus' tree. >> But... make sure that git send-email or whatever doesn't automatically >> add that to the recipients for the emailed patch. >> >> If you send the stable people a patch before its in mainline, you'll get >> a whinge telling you to read Documentation/stable_kernel_rules.txt > > Alright, I will add Cc: <stable@vger.kernel.org> tag in the patch. If you do that, be sure to use --suppress-cc=bodycc when you send it with git-send-email. Kevin
The following changes since commit 62aa2b537c6f5957afd98e29f96897419ed5ebab: Linus Torvalds (1): Linux 3.3-rc2 are available in the git repository at: http://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev Branch: for_3.4/gpio_cleanup_fixes_v9 This series is continuation of cleanup of OMAP GPIO driver and fixes. The cleanup include getting rid of cpu_is_* checks wherever possible, use of gpio_bank list instead of static array, use of unique platform specific value associated data member to OMAP platforms to avoid cpu_is_* checks. The series also include PM runtime support. Power Tests a) OMAP3430SDP - Modify board-3430sdp.c file to have multiple GPIO modules active with debounce timeout enabled. - Enable CPU-Idle - Enable UART timeouts - Enable offmode - echo mem > /sys/power/state - Verify retention and offmode count increment Used following patches to avoid exception during system suspend:- [PATCH RFC 1/2] mtd : Prevent the NULL pointer access [PATCH RFC 2/2] mtd : Make the mtd_suspend return 0 if the suspend is not implemented # echo mem > /sys/power/state [ 47.128021] PM: Syncing filesystems ... done. [ 47.144104] Freezing user space processes ... (elapsed 0.01 seconds) done. [ 47.168243] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) don e. [ 47.205139] Unable to handle kernel NULL pointer dereference at virtual addre ss 000000a0 [ 47.213317] pgd = deaac000 [ 47.216033] [000000a0] *pgd=9e932831, *pte=00000000, *ppte=00000000 [ 47.222381] Internal error: Oops: 17 [#1] SMP [ 47.226745] Modules linked in: [ 47.229827] CPU: 0 Not tainted (3.3.0-rc2-00031-g12c5c5c #235) [ 47.236022] PC is at mtd_cls_suspend+0x8/0x20 [ 47.240386] LR is at mtd_cls_suspend+0x8/0x20 [ 47.244750] pc : [<c02e78f8>] lr : [<c02e78f8>] psr: a0000013 [ 47.244750] sp : dea1fe60 ip : 22222222 fp : c0ee7d40 [ 47.256256] r10: c0ee7cf0 r9 : 00000000 r8 : c02e78f0 [ 47.261474] r7 : 00000000 r6 : 00000000 r5 : 00000002 r4 : dea45800 [ 47.268005] r3 : deb4cac0 r2 : 00000000 r1 : 00000002 r0 : 00000000 [ 47.274536] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 47.281677] Control: 10c5387d Table: 9eaac019 DAC: 00000015 [ 47.287445] Process sh (pid: 1177, stack limit = 0xdea1e2f8) [ 47.293090] Stack: (0xdea1fe60 to 0xdea20000) [...] b) ZOOM3 - Enable CPU-Idle - Enable UART timeout - echo mem > /sys/power/state - Wakeup system using serial keyboard - Verify retention count increment Functional Tests - Done on OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430 Bootup Test - Done on OMAP1710 Used following patch to fix OMAP1 build error:- [PATCH] i2c: OMAP: Fix OMAP1 build error v9: - Summary of Comments/Issues fixed * GPIO wakeup does not work * Call debounce clock enable/disable functions from PM runtime callbacks. This will avoid calling the functions from multiple places. * Modify description of following patch to match latest changes. gpio/omap: save and restore debounce registers. * Use (bank->regs->set_dataout && bank->regs->clr_dataout) together instead of using only one of them. * Remove cpu_is_omapxxxx() checks from set_gpio_trigger(). * _gpio_dbck_enable() in runtime callback triggered from omap_gpio_request does not enable dbck because dbck_enable_mask is not set at this point. * Workaround associated with an errata got missed in v8. This has been included. v8: - Remove PM_CONFIG macro around following assignment. pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count; - Once pm_runtime is enabled there is no more need for calling the omap_device_disable_idle_on_suspend(od). - With pm_runtime, handling of clocks in Suspend is taken care of by powerdomain hooks. So remove usage of *_runtime_put/get* from suspend/resume hooks and Idle path. - Add handling of debounce clocks along the Suspend and Idle paths. - Remove [PATCH 04/24] gpio/omap: fix pwrdm_post_transition call sequence from this series. This will be merged as part of power cleanup series. - Remove [PATCH v7 20/26] gpio/omap: skip operations in runtime callbacks The bank->mod_usage check in this patch is not needed any more because they are now already being taken care in suspend/resume and Idle paths. - Remove [PATCH v7 26/26] gpio/omap: add dbclk aliases for all gpio modules This is already taken care in hwmod. - Remove redundant blank line in [PATCH v7 14/26] gpio/omap: remove unnecessary bit-masking for read access - if (cpu_is_omap15xx() && (bank->method == METHOD_MPUIO)) - isr &= 0x0000ffff; if (bank->level_mask) level_mask = bank->level_mask & enabled; v7: - Use pm_runtime_put() instead of pm_runtime_put_sync_suspend() - Keep *_runtime_get/put*() outside spinlock - Remove additional checking of conditions in _restore_context() From: if (bank->regs->set_dataout && bank->regs->clear_dataout) ... To: if (bank->regs->set_dataout) ... - Use SET_RUNTIME_PM_OPS and SET_SYSTEM_SLEEP_PM_OPS macros - In [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle, protect the bank data elements and register access using spinlock in runtime_suspend/resume() callbacks. This is because these callbacks run with interrupts enabled. - Add dbclk aliases for all GPIO modules. Without this, GPIO modules were not getting the correct clock handle to enable/disable debounec clock. - Fix log comments on the following patches: [PATCH 19/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle [PATCH 20/25] gpio/omap: skip operations in runtime callbacks [PATCH 24/25] gpio/omap: restore OE only after setting the output level v6: - Save and restore debounce registers for proper driver operation. - Restore interrupt enable after all configuration to avoid spurious interrupts. - Restore dataout register before oe register. - Restore dataout into dataout_set or dataout based upon the OMAP version. - Change register name from wkup_status to wkup_en. - Remove wrapper around omap_pm_get_dev_context_loss_count(). Use it directly. Also, changed the signature of get_context_loss_count in pdata and bank structure from int to u32. - Use 'context' instead of 'ctx' for clarity wherever it is used. - Merged two patches into one which are related to bank_is_mpuio() modification. - Use shift operator instead of following: + .irqctrl = OMAP_MPUIO_GPIO_INT_EDGE / 2, - Remove redundant check from the following + if (bank_is_mpuio(bank)) { + if (bank->regs->wkup_status) <--- redundant check + mpuio_init(bank); - Change subject of following patch [PATCH v5 15/22] gpio/omap: use readl in irq_handler for all access into [PATCH 14/25] gpio/omap: remove unnecessary bit-masking for read access - Fix multi-line comments in [PATCH v5 20/22] gpio/omap: cleanup prepare_for_idle and resume_after_idle v5: - Reduce runtime callback overhead when *_get/put_sync() called from probe() and *_gpio_request/free(). - Dynamic context save within functions where context is modified instead of saving all context within a common function. - Removed call to mpuio_init() from omap_gpio_mod_init(). Both the functions are called once during initialization in *_gpio_probe(). Call to omap_gpio_mod_init() has been removed from omap_gpio_request() on the first access to gpio bank. One time initialization looks sufficient. - In *_gpio_irq_handler() use *_put_sync_suspend() instead of *_put_sync(). - Removed hardcoding of OMAP16xx sysconfig register value and instead defined an associated constant. - Removed *_get_sync() call from *_gpio_suspend() and *_put_sync() call from *_gpio_resume(). They got wrongly slipped into the code. - Removed following redundant zero allocated initialization from mach-omap2/gpio.c + pdata->regs->irqctrl = 0; + pdata->regs->edgectrl1 = 0; + pdata->regs->edgectrl2 = 0; - Removed following redundant code in gpio-omap.c -#define bank_is_mpuio(bank) ((bank)->method == METHOD_MPUIO) v4: - since all accesses to registers are 4-byte aligned, removing special checks and handling of 16 and 32-bit wide bank registers and instead use 32-bit read/write access consistently. - redundant usage of MOD_REG_BIT has been corrected and replaced with _gpio_rmw(). - omap_gpio_mod_init() function has been simplified further using _gpio_rmw(). - sysconfig register offset specific to omap16xx has been removed along with its usage. - additional logic to skip from suspend/resume: if (!bank->regs->wkup_status || !bank->suspend_wakeup) return 0; if (!bank->regs->wkup_status || !bank->saved_wakeup) return 0; - separated mpuio related changes into a different patch from the patch where wakeup status register related changes are done. - Incorrect replacement of !cpu_class_is_omap2() in gpio_irq_type() corrected: + if (!bank->regs->leveldetect0 && + (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH))) return -EINVAL; v3: - Avoid use of wkup_set and wkup_clear registers. Instead use wkup_status register for all platforms. This is because on OMAP4 it is recommended not to use them. - Remove duplicate code in omap_gpio_mod_init() for handling the same for 32-bit and 16-bit GPIO bank widths. This is accomplished by having two functions to handle each case while assiging a common function pointer during initialization. - Remove OMAP16xx specific one time initialization from omap_gpio_mod_init(). Move it inside omap16xx_gpio_init(). - Avoid usage of USHRT_MAX to indicate undefined values. Use 0 instead. - In omap_gpio_suspend()/resume() functions remove code that checks if the feature is supported. Instead, assign these functions to struct platform_driver's suspend & resume function pointers for those OMAP platforms whcih support this feature. - Remove 'suspend_support' flag because it is redundant. Instead use wkup_* registers to decode the same information. - Restore context also when we don't know if the context is lost. - Make omap_gpio_save_context() and omap_gpio_restore_context() static. v2: - Do special handling of non-wakeup GPIOs only on OMAP2420. Avoid this handling on OMAP3430. - Isolate cleanups and fixes into separate set of patches. Keep the cleanup first followed by the fixes. - Avoid calling omap_gpio_get_context_loss() directly and instead call it through function pointer in pdata initialized during init. - workaround_enabled flag is not longer needed and is removed. - Call pwrdm_post_transition() before calling omap_gpio_resume_after_idle(). - In omap2_gpio_resume_after_idle() do context restore before handling workaround. - Use PM runtime framework. - Modify register offset names to : wkup_status, wkup_clear, wkup_set. Also use 'base + offset' for readibility in all relevant places. - Remove unwanted messages from commit section like TODO, etc. Charulatha V (8): gpio/omap: remove dependency on gpio_bank_count gpio/omap: use flag to identify wakeup domain gpio/omap: make gpio_context part of gpio_bank structure gpio/omap: make non-wakeup GPIO part of pdata gpio/omap: avoid cpu checks during module ena/disable gpio/omap: use pinctrl offset instead of macro gpio/omap: remove bank->method & METHOD_* macros gpio/omap: fix bankwidth for OMAP7xx MPUIO Nishanth Menon (4): gpio/omap: save and restore debounce registers gpio/omap: enable irq at the end of all configuration in restore gpio/omap: restore OE only after setting the output level gpio/omap: handle set_dataout reg capable IP on restore Tarun Kanti DebBarma (13): gpio/omap: handle save/restore context in GPIO driver gpio/omap: further cleanup using wkup_en register gpio/omap: use level/edge detect reg offsets gpio/omap: remove hardcoded offsets in context save/restore gpio/omap: cleanup set_gpio_triggering function gpio/omap: cleanup omap_gpio_mod_init function gpio/omap: remove unnecessary bit-masking for read access gpio/omap: use pm-runtime framework gpio/omap: optimize suspend and resume functions gpio/omap: cleanup prepare_for_idle and resume_after_idle gpio/omap: fix debounce clock handling gpio/omap: fix incorrect access of debounce module gpio/omap: remove omap_gpio_save_context overhead arch/arm/mach-omap1/gpio15xx.c | 7 +- arch/arm/mach-omap1/gpio16xx.c | 47 ++- arch/arm/mach-omap1/gpio7xx.c | 14 +- arch/arm/mach-omap2/gpio.c | 36 +- arch/arm/mach-omap2/pm34xx.c | 14 - arch/arm/plat-omap/include/plat/gpio.h | 29 +- drivers/gpio/gpio-omap.c | 1099 +++++++++++++------------------- 7 files changed, 549 insertions(+), 697 deletions(-)