From: Felipe Balbi <balbi@ti.com>
To: Kevin Hilman <khilman@ti.com>
Cc: balbi@ti.com, "Cousson, Benoit" <b-cousson@ti.com>,
Grant Likely <grant.likely@secretlab.ca>,
Tarun Kanti DebBarma <tarun.kanti@ti.com>,
linux-omap@vger.kernel.org, tony@atomide.com,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Charulatha V <charu@ti.com>
Subject: Re: [PATCH v9 01/25] gpio/omap: remove dependency on gpio_bank_count
Date: Sat, 4 Feb 2012 18:08:04 +0200 [thread overview]
Message-ID: <20120204160802.GA10818@legolas.emea.dhcp.ti.com> (raw)
In-Reply-To: <87liojajs4.fsf@ti.com>
[-- Attachment #1: Type: text/plain, Size: 8820 bytes --]
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);
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2012-02-04 16:08 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-02 17:30 [PATCH v9 00/25] gpio/omap: driver cleanup and fixes Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 01/25] gpio/omap: remove dependency on gpio_bank_count Tarun Kanti DebBarma
2012-02-02 18:41 ` Felipe Balbi
2012-02-02 19:16 ` Grant Likely
2012-02-02 19:45 ` Felipe Balbi
2012-02-02 20:48 ` Cousson, Benoit
2012-02-02 21:49 ` Felipe Balbi
2012-02-02 21:53 ` Felipe Balbi
2012-02-02 22:00 ` Cousson, Benoit
2012-02-02 22:07 ` Felipe Balbi
2012-02-03 17:50 ` Kevin Hilman
2012-02-04 16:08 ` Felipe Balbi [this message]
2012-02-05 7:07 ` Varadarajan, Charulatha
2012-02-05 9:08 ` Felipe Balbi
2012-02-05 9:16 ` Shilimkar, Santosh
2012-02-05 11:35 ` Felipe Balbi
2012-02-05 12:35 ` Shilimkar, Santosh
2012-02-06 6:40 ` Felipe Balbi
2012-02-06 5:18 ` Varadarajan, Charulatha
2012-02-02 17:30 ` [PATCH v9 02/25] gpio/omap: use flag to identify wakeup domain Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 03/25] gpio/omap: make gpio_context part of gpio_bank structure Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 04/25] gpio/omap: handle save/restore context in GPIO driver Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 05/25] gpio/omap: make non-wakeup GPIO part of pdata Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 06/25] gpio/omap: avoid cpu checks during module ena/disable Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 07/25] gpio/omap: further cleanup using wkup_en register Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 08/25] gpio/omap: use level/edge detect reg offsets Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 09/25] gpio/omap: remove hardcoded offsets in context save/restore Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 10/25] gpio/omap: cleanup set_gpio_triggering function Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 11/25] gpio/omap: cleanup omap_gpio_mod_init function Tarun Kanti DebBarma
2012-04-21 14:03 ` Janusz Krzysztofik
2012-04-23 18:54 ` DebBarma, Tarun Kanti
2012-04-24 15:36 ` DebBarma, Tarun Kanti
2012-04-24 16:04 ` Tony Lindgren
2012-04-25 4:34 ` DebBarma, Tarun Kanti
2012-04-25 6:39 ` Shilimkar, Santosh
2012-04-25 12:54 ` DebBarma, Tarun Kanti
2012-04-25 13:45 ` Russell King - ARM Linux
2012-04-26 5:13 ` DebBarma, Tarun Kanti
2012-04-27 21:31 ` Kevin Hilman
2012-04-24 22:37 ` Janusz Krzysztofik
2012-02-02 17:30 ` [PATCH v9 12/25] gpio/omap: use pinctrl offset instead of macro Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 13/25] gpio/omap: remove unnecessary bit-masking for read access Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 14/25] gpio/omap: remove bank->method & METHOD_* macros Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 15/25] gpio/omap: fix bankwidth for OMAP7xx MPUIO Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 16/25] gpio/omap: use pm-runtime framework Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 17/25] gpio/omap: optimize suspend and resume functions Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 18/25] gpio/omap: cleanup prepare_for_idle and resume_after_idle Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 19/25] gpio/omap: fix debounce clock handling Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 20/25] gpio/omap: fix incorrect access of debounce module Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 21/25] gpio/omap: remove omap_gpio_save_context overhead Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 22/25] gpio/omap: save and restore debounce registers Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 23/25] gpio/omap: enable irq at the end of all configuration in restore Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 24/25] gpio/omap: restore OE only after setting the output level Tarun Kanti DebBarma
2012-02-02 17:30 ` [PATCH v9 25/25] gpio/omap: handle set_dataout reg capable IP on restore Tarun Kanti DebBarma
2012-02-02 19:42 ` [PATCH v9 00/25] gpio/omap: driver cleanup and fixes Grant Likely
2012-02-03 17:51 ` Kevin Hilman
2012-02-03 21:16 ` Grant Likely
2012-02-03 9:21 ` Hebbar, Gururaja
[not found] ` <CAC83ZvLoYVofH9oKXw92i-=DbP2i3NfZjLGSJwk1j0JvXcFZVQ@mail.gmail.com>
2012-02-03 12:09 ` Hebbar, Gururaja
2012-02-03 21:01 ` Kevin Hilman
2012-02-06 11:53 ` DebBarma, Tarun Kanti
2012-02-10 19:24 ` Tony Lindgren
2012-02-10 19:55 ` Kevin Hilman
2012-02-13 5:33 ` DebBarma, Tarun Kanti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120204160802.GA10818@legolas.emea.dhcp.ti.com \
--to=balbi@ti.com \
--cc=b-cousson@ti.com \
--cc=charu@ti.com \
--cc=grant.likely@secretlab.ca \
--cc=khilman@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=tarun.kanti@ti.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).