Date:	Sat, 4 Feb 2012 18:08:04 +0200
From:	Felipe Balbi <balbi@...com>
To:	Kevin Hilman <khilman@...com>
Cc:	balbi@...com, "Cousson, Benoit" <b-cousson@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Tarun Kanti DebBarma <tarun.kanti@...com>,
	linux-omap@...r.kernel.org, tony@...mide.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Charulatha V <charu@...com>
Subject: Re: [PATCH v9 01/25] gpio/omap: remove dependency on gpio_bank_count

Hi,

On Fri, Feb 03, 2012 at 09:50:19AM -0800, Kevin Hilman wrote:
> Felipe Balbi <balbi@...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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists