[1/1] gpio: omap: Fix PM runtime issue and remove most BANK_USED macros

Commit Message

[e-mail] April 30, 2015, 7:10 p.m. UTC

On 04/29/2015 05:26 PM, Tony Lindgren wrote:
> * [e-mail] <[e-mail]> [150428 14:58]:
>> Hi Tony,
>>
>> Sorry for delayed reply.
>>
>> On 04/23/2015 05:39 PM, Tony Lindgren wrote:
>>> * [e-mail] <[e-mail]> [150423 04:13]:
>>>> On 04/21/2015 07:08 PM, Tony Lindgren wrote:
>>>>> @@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
>>>>>     		writel_relaxed(ctrl, reg);
>>>>>     		bank->context.ctrl = ctrl;
>>>>>     	}
>>>>> +
>>>>> +	if (is_irq) {
>>>>> +		omap_set_gpio_direction(bank, offset, 1);
>>>>> +		bank->irq_usage |= BIT(offset);
>>>>> +	} else {
>>>>> +		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
>>>>> +		bank->mod_usage |= BIT(offset);
>>>>> +	}
>>>>
>>>> The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general,
>>>> more or less independent.
>>>>
>>>> So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code.
>>>> And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might
>>>> introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may
>>>> affect on on GPIO-chip and vise versa :(
>>>
>>> Hmm I'm thinking omap_enable/disable_gpio_module() eventually becomes
>>> our runtime_resume/suspend(). Currently the enabling and disabling is
>>> buggy for GPIO for some corner cases.. AFAIK the only difference between
>>
>> It might be very helpful if you'd able to share additional info about
>> any "corner cases" you know.
> 
> Well not much to share there. I started trying to figure out why your
> earlier patch causes PM regressions trying reduce the GPIO_BANK usage,
> but had not luck. Based on looking at the code, I then noticed that some
> resources are not freed for omap_gpio_irq_type() with the early returns.
> And then I noticed we have piles of duplicate code all over the place
> pretty much trying to do the same thing.
> 
>>> enabling GPIO vs GPIO-IRQ is the calling of omap_set_gpio_direction
>>> vs omap_set_gpio_triggering. Or at least that's the way it should be
>>> unless I'm missing something?
>>
>> I think yes. what i'd like to say, first of all, is that it might be not good idea to mix
>> too much functionality in  omap_enable/disable_gpio_module() - especially GPIO-IRQ vs
>> GPIO-chip ( Very easy to get lost ;)
> 
> In general removing 30 lines of duplicate code to fix freeing
> of resources makes sense :) I'd rather do that than add more similar
> code to fix it for sure. That will make further changes easier too.

Unfortunately I'm not sure ( at least not for this driver.
Future fixes most probably will eat all these removed lines and
will just add anther "if else" but in different places (see description or corner case below).

In my opinion, It might be better to review each interface function first as for GPIO chip
as for GPIOIRQ chip and ensure that they are doing exactly what need to be done
(as part this job - get rid of over-optimized functions like omap_reset_gpio or
omap_gpio_init_irq). And after that do further optimization. 

For example:
irqc->irq_set_type = omap_gpio_irq_type

should do:
 - configure request GPIO IRQ triggering type in GPIO bank registers
 - prerequisite: GPIO bank should be powered (ON)

can be called:
 - from OF core as part of IRQ mapping process
 - from __setup_irq
 - any time by user as before as after request_irq()/free_irq()

Redundant functionality:
- we don't need to enable GPIO bank
- we don't need to configure GPIO as input
  because the same will be always done from omap_gpio_irq_startup

Missed functionality:
 - if GPIO bank is unused at the moment when omap_gpio_irq_type()
   is called we should kept it disabled, because setting IRQ
   type doesn't mean that IRQ is going to be really used.

As result below diff (still under testing):


>   
>> For example (1) - your change of omap_gpio_request() looks like invalid:
>>
>>    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_USED(bank))
>> -		pm_runtime_get_sync(bank->dev);
>> -
>> -	spin_lock_irqsave(&bank->lock, flags);
>> -	/* Set trigger to none. You need to enable the desired trigger with
>> -	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
>> -	 * not already been requested.
>> -	 */
>> -	if (!LINE_USED(bank->irq_usage, offset)) {
>> -		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
>> -		omap_enable_gpio_module(bank, offset);
>>
>> 	^^ above two line should be executed only if GPIO line was not requested as IRQ before
>>
>> -	}
>>
>>
>> -	bank->mod_usage |= BIT(offset);
>> -	spin_unlock_irqrestore(&bank->lock, flags);
>> +	omap_enable_gpio_module(bank, offset, false);
>>
>> ^^ after your change, it looks like omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE)
>>     will be called always and GPIO triggering configuration might be lost
> 
> OK yeah that's a valid concern thanks. Let's keep that part to be sure
> we're not introducing regressions.
>     
>>    	return 0;
>>    }
>>
>> Example (2)
>>   I've found commit 55b6019ae294 "OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask"
>>   which does the following
>>    gpio_mask_irq()
>>     |-  _set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
>>    
>>    gpio_unmask_irq()
>>     |- u32 trigger = irqd_get_trigger_type(d);
>>        if (trigger)
>>               omap_set_gpio_triggering(bank, offset, trigger);
>>
>>    As result, it seems unreasonable to physically configure IRQ triggering type in GPIO bank registers
>>    inside omap_gpio_irq_type(). Of course, such assumption should be double checked taking into account that
>>    __irq_set_trigger() can be called any time even before request_irq().
>>    Also, seems the same could be applied to omap_enable_gpio_module and omap_set_gpio_direction and they
>>    could be removed from omap_gpio_irq_type().
> 
> In omap_gpio_irq_type() we call omap_set_gpio_triggering() after we call
> omap_enable_gpio_module(), so I don't think we need to change anything
> there. Or am I missing something here?
>    
>>>> Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and
>>>> enabling/disabling?
>>>
>>> If you're thinking about just thinking about having separate wrappers around
>>> it like this::
>>>
>>> static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
>>> 					bool is_irq)
>>> {
>>> 	...
>>> }
>>>
>>> static void omap_enable_gpio((struct gpio_bank *bank, unsigned offset)
>>> {
>>> 	omap_enable_gpio_module(bpio_bank, offset, 0);
>>> }
>>>
>>> static void omap_enable_gpio_irq((struct gpio_bank *bank, unsigned offset)
>>> {
>>> 	omap_enable_gpio_module(bpio_bank, offset, 1);
>>> }
>>>
>>> Then yes makes sense to me. Or do you have something else in mind?
>>
>> Yep. Commented above.
> 
> Sounds like adding the wrappers is not needed as we're planning to replace
> these with line specific pm runtime calls anyways.
>   
>> Also, it probably could work if we will set GPIO_CTRL.DISABLEMODULE=1
>> in omap_gpio_runtime_resume and GPIO_CTRL.DISABLEMODULE=0 in _runtime_suspend,
>> but it may require from us to split CPUIdle and suspend code path (
> 
> What Felipe suggested a while back to me was we implement runtime_idle()
> that checks for GPIO lines used and then we can still suspend and resume
> for idle.
> 
> Anyways, updated patch below with the line check added. Do you see
> other issues?
> 
> Regards,
> 
> Tony
> 
> 8< -------------------------
> From: Tony Lindgren <[e-mail]>
> Date: Mon, 27 Apr 2015 10:18:17 -0700
> Subject: [PATCH] gpio: omap: Fix PM runtime issue and remove most BANK_USED macros
> 
> Looks like omap_gpio_irq_type can return early at several places
> leaving a GPIO bank enabled without doing pm_runtime_put if wrong
> GPIO arguments are passed.
> 
> Instead of adding more complicated BANK_USED macros, let's fix the
> issue properly. We can pass is_irq flag to omap_enable_gpio_module
> and omap_disble_gpio_module. And with that we can remove all the
> similar code elsewhere to get rid of most BANK_USED macros.
> 
> Note that the reason for the BANK_USED macro is that we need to manage
> PM runtime on per GPIO bank basis. In the long run we want to move to
> using PM runtime counts for each GPIO line to determine if a GPIO
> bank is used. Once we have a solution for omap_enable_gpio_module
> and omap_disable_gpio_module, we can remove the remaining BANK_USED
> macros.
> 
> Cc: Felipe Balbi <[e-mail]>
> Cc: Javier Martinez Canillas <[e-mail]>
> Cc: Grygorii Strashko <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Cc: Santosh Shilimkar <[e-mail]>
> Signed-off-by: Tony Lindgren <[e-mail]>
> 
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -86,6 +86,7 @@ struct gpio_bank {
>   #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
>   #define LINE_USED(line, offset) (line & (BIT(offset)))
>   
> +static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset);
>   static void omap_gpio_unmask_irq(struct irq_data *d);
>   
>   static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)
> @@ -419,8 +420,16 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
>   	return 0;
>   }
>   
> -static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
> +static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
> +				    bool is_irq)
>   {
> +	unsigned long flags;
> +
> +	/* PM runtime is per bank, not per GPIO line */
> +	if (!BANK_USED(bank))
> +		pm_runtime_get_sync(bank->dev);
> +
> +	spin_lock_irqsave(&bank->lock, flags);
>   	if (bank->regs->pinctrl) {
>   		void __iomem *reg = bank->base + bank->regs->pinctrl;
>   
> @@ -438,11 +447,36 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
>   		writel_relaxed(ctrl, reg);
>   		bank->context.ctrl = ctrl;
>   	}
> +
> +	if (is_irq) {
> +		omap_set_gpio_direction(bank, offset, 1);
> +		bank->irq_usage |= BIT(offset);
> +	} else {
> +		/*
> +		 * Set trigger to none. You need to enable the desired trigger
> +		 * with request_irq() or set_irq_type(). Only do this if the
> +		 * IRQ line has not already been requested.
> +		 */
> +		if (!LINE_USED(bank->irq_usage, offset))
> +			omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> +		bank->mod_usage |= BIT(offset);
> +	}
> +	spin_unlock_irqrestore(&bank->lock, flags);
>   }
>   
> -static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
> +static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset,
> +				     bool is_irq)
>   {
>   	void __iomem *base = bank->base;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	if (is_irq)
> +		bank->irq_usage &= ~(BIT(offset));
> +	else
> +		bank->mod_usage &= ~(BIT(offset));
> +
> +	omap_reset_gpio(bank, offset);

There is another corner case :) - easy to reproduce
and it's present in old code.

GPIOn is used as IRQ by some dev:
- in my case PCF8575.INT ->  gpio6.11
- PCFx driver knows nothing about type of IRQ line (GPIO or not)
  so it doesn't request gpio and just do request_irq()

If I'll export gpio6.11 through sysfs and then unxeport it
then IRQs from PCFx will not be received any more.

Seems omap_reset_gpio has to be split for GPIO and GPIO-IRQ cases.


>   
>   	if (bank->regs->wkup_en &&
>   	    !LINE_USED(bank->mod_usage, offset) &&
> @@ -463,6 +497,11 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
>   		writel_relaxed(ctrl, reg);
>   		bank->context.ctrl = ctrl;
>   	}
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +
> +	/* PM runtime is per bank, not per GPIO line */
> +	if (!BANK_USED(bank))
> +		pm_runtime_put(bank->dev);
>   }
>   
>   static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
> @@ -472,15 +511,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
>   	return readl_relaxed(reg) & BIT(offset);
>   }
>   
> -static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
> -{
> -	if (!LINE_USED(bank->mod_usage, offset)) {
> -		omap_enable_gpio_module(bank, offset);
> -		omap_set_gpio_direction(bank, offset, 1);
> -	}
> -	bank->irq_usage |= BIT(offset);
> -}
> -
>   static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> @@ -488,9 +518,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>   	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
>   	if (type & ~IRQ_TYPE_SENSE_MASK)
>   		return -EINVAL;
>   
> @@ -498,13 +525,9 @@ static int oomap_gpio_irq_startupmap_gpio_irq_type(struct irq_data *d, unsigned type)
>   		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
>   		return -EINVAL;
>   
> +	omap_enable_gpio_module(bank, offset, true);
>   	spin_lock_irqsave(&bank->lock, flags);
>   	retval = omap_set_gpio_triggering(bank, offset, type);
> -	omap_gpio_init_irq(bank, offset);
> -	if (!omap_gpio_is_input(bank, offset)) {
> -		spin_unlock_irqrestore(&bank->lock, flags);
> -		return -EINVAL;
> -	}
>   	spin_unlock_irqrestore(&bank->lock, flags);
>   
>   	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> @@ -659,26 +682,8 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
>   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_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	/* Set trigger to none. You need to enable the desired trigger with
> -	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
> -	 * not already been requested.
> -	 */
> -	if (!LINE_USED(bank->irq_usage, offset)) {
> -		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> -		omap_enable_gpio_module(bank, offset);
> -	}
> -	bank->mod_usage |= BIT(offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> +	omap_enable_gpio_module(bank, offset, false);
>   
>   	return 0;
>   }
> @@ -686,20 +691,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>   static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	bank->mod_usage &= ~(BIT(offset));
> -	omap_disable_gpio_module(bank, offset);
> -	omap_reset_gpio(bank, 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_USED(bank))
> -		pm_runtime_put(bank->dev);
> +	omap_disable_gpio_module(bank, offset, false);
>   }
>   
>   /*
> @@ -788,15 +781,9 @@ exit:
>   static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> -	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	omap_gpio_init_irq(bank, offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> +	omap_enable_gpio_module(bank, offset, true);
>   	omap_gpio_unmask_irq(d);
>   
>   	return 0;
> @@ -805,21 +792,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>   static void omap_gpio_irq_shutdown(struct irq_data *d)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> -	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	spin_lock_irqsave(&bank->lock, flags);
> -	bank->irq_usage &= ~(BIT(offset));
> -	omap_disable_gpio_module(bank, offset);
> -	omap_reset_gpio(bank, offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> -
> -	/*
> -	 * If this is the last IRQ to be freed in the bank,
> -	 * disable the bank module.
> -	 */
> -	if (!BANK_USED(bank))
> -		pm_runtime_put(bank->dev);
> +	omap_disable_gpio_module(bank, offset, true);
>   }
>

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 6efee35..be71354 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -488,9 +488,6 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
        unsigned long flags;
        unsigned offset = d->hwirq;
 
-       if (!BANK_USED(bank))
-               pm_runtime_get_sync(bank->dev);
-
        if (type & ~IRQ_TYPE_SENSE_MASK)
                return -EINVAL;
 
@@ -498,13 +495,11 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
                (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
                return -EINVAL;
 
+       if (!BANK_USED(bank))
+               pm_runtime_get_sync(bank->dev);
+
        spin_lock_irqsave(&bank->lock, flags);
        retval = omap_set_gpio_triggering(bank, offset, type);
-       omap_gpio_init_irq(bank, offset);
-       if (!omap_gpio_is_input(bank, offset)) {
-               spin_unlock_irqrestore(&bank->lock, flags);
-               return -EINVAL;
-       }
        spin_unlock_irqrestore(&bank->lock, flags);
 
        if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
@@ -512,6 +507,9 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
        else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
                __irq_set_handler_locked(d->irq, handle_edge_irq);
 
+       if (!BANK_USED(bank))
+               pm_runtime_put(bank->dev);
+
        return retval;
 }