[PATCH] [OMAP] GPIO Module disable if all pins inactive

[PATCH] [OMAP] GPIO Module disable if all pins inactive

[charu]

From: Charulatha V <charu@ti.com>

This patch disables a GPIO module when all the pins of GPIO
module are inactive (clock gating forced at module level) and
enables the module when any gpio in the module is requested.

Signed-off-by: Charulatha V <charu@ti.com>
---
 arch/arm/plat-omap/gpio.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index cdc2a58..2304a5d 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -194,6 +194,7 @@ struct gpio_bank {
 	spinlock_t lock;
 	struct gpio_chip chip;
 	struct clk *dbck;
+	u32 gpio_status;
 };
 
 #define METHOD_MPUIO		0
@@ -1080,6 +1081,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;
+	u32 ctrl = 0;
 
 	spin_lock_irqsave(&bank->lock, flags);
 
@@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 		__raw_writel(__raw_readl(reg) | (1 << offset), reg);
 	}
 #endif
+	if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		if (!bank->gpio_status) {
+			ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
+			/* Module is enabled, clocks are not gated */
+			ctrl &= 0xFFFFFFFE;
+			__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
+		}
+		bank->gpio_status |= 1 << offset;
+	}
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1106,6 +1117,7 @@ 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;
+	u32 ctrl = 0;
 
 	spin_lock_irqsave(&bank->lock, flags);
 #ifdef CONFIG_ARCH_OMAP16XX
@@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 		__raw_writel(1 << offset, reg);
 	}
 #endif
+	if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		bank->gpio_status &= ~(1 << offset);
+		if (!bank->gpio_status) {
+			ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
+			/* Module is disabled, clocks are gated */
+			ctrl |= 1;
+			__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
+		}
+	}
 	_reset_gpio(bank, bank->chip.base + offset);
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
@@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
 			gpio_count = 32;
 		}
 #endif
+		bank->gpio_status = 0;
 		/* REVISIT eventually switch from OMAP-specific gpio structs
 		 * over to the generic ones
 		 */
-- 
1.6.0.4

Re: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Nishanth Menon]

Varadarajan, Charu Latha had written, on 10/23/2009 10:55 AM, the following:
> From: Charulatha V <charu@ti.com>
> 
> This patch disables a GPIO module when all the pins of GPIO
> module are inactive (clock gating forced at module level) and
> enables the module when any gpio in the module is requested.
> 
> Signed-off-by: Charulatha V <charu@ti.com>
> ---
>  arch/arm/plat-omap/gpio.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index cdc2a58..2304a5d 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -194,6 +194,7 @@ struct gpio_bank {
>  	spinlock_t lock;
>  	struct gpio_chip chip;
>  	struct clk *dbck;
> +	u32 gpio_status;
please rename this as gpio_usage?

maybe OMAP1 could also benefit out of this..
>  };
>  
>  #define METHOD_MPUIO		0
> @@ -1080,6 +1081,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;
> +	u32 ctrl = 0;
Remove this to the {} no point in wasting stack space when you dont need 
to + you will generate warning for OMAP1 platforms.
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  
> @@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>  		__raw_writel(__raw_readl(reg) | (1 << offset), reg);
>  	}
>  #endif
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
> +		if (!bank->gpio_status) {
> +			ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
> +			/* Module is enabled, clocks are not gated */
> +			ctrl &= 0xFFFFFFFE;
> +			__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
> +		}
> +		bank->gpio_status |= 1 << offset;
> +	}
why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
if (!bank->gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
	u32 ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
	/* Module is enabled, clocks are not gated */
	ctrl &= 0xFFFFFFFE;
	__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
}
bank->gpio_status |= 1 << offset;
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;
> @@ -1106,6 +1117,7 @@ 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;
> +	u32 ctrl = 0;
used just once -> move it to the {} + warning to OMAP1
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  #ifdef CONFIG_ARCH_OMAP16XX
> @@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>  		__raw_writel(1 << offset, reg);
>  	}
>  #endif
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
> +		bank->gpio_status &= ~(1 << offset);
> +		if (!bank->gpio_status) {
> +			ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
> +			/* Module is disabled, clocks are gated */
> +			ctrl |= 1;
> +			__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
> +		}
> +	}
how about this:
	bank->gpio_status &= ~(1 << offset);
if (!bank->gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
	u32 ctrl = __raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
	/* Module is disabled, clocks are gated */
	ctrl |= 1;
	__raw_writel(ctrl, bank->base + OMAP24XX_GPIO_CTRL);
}
>  	_reset_gpio(bank, bank->chip.base + offset);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
> @@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
>  			gpio_count = 32;
>  		}
>  #endif
> +		bank->gpio_status = 0;
>  		/* REVISIT eventually switch from OMAP-specific gpio structs
>  		 * over to the generic ones
>  		 */

Regards,
Nishanth Menon

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Varadarajan, Charu Latha]

>Varadarajan, Charu Latha had written, on 10/23/2009 10:55 AM, the following:
>> From: Charulatha V <charu@ti.com>>
>>
>> This patch disables a GPIO module when all the pins of GPIO
>> module are inactive (clock gating forced at module level) and
>> enables the module when any gpio in the module is requested.
>>
>> Signed-off-by: Charulatha V <charu@ti.com>>
>> ---
>>  arch/arm/plat-omap/gpio.c |   22 ++++++++++++++++++++++
>>  1 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index cdc2a58..2304a5d 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -194,6 +194,7 @@ struct gpio_bank {
>>        spinlock_t lock;
>>        struct gpio_chip chip;
>>        struct clk *dbck;
>> +     u32 gpio_status;
>please rename this as gpio_usage?
okay

>maybe OMAP1 could also benefit out of this..
>>  };
>> 
>>  #define METHOD_MPUIO         0
>> @@ -1080,6 +1081,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;
>> +     u32 ctrl = 0;
>Remove this to the {} no point in wasting stack space when you dont need
>to + you will generate warning for OMAP1 platforms.
>> 
>>        spin_lock_irqsave(&bank->>lock, flags);
>> 
>> @@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>>                __raw_writel(__raw_readl(reg) | (1 << offset), reg);
>>        }
>>  #endif
>> +     if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
>> +             if (!bank->>gpio_status) {
>> +                     ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>> +                     /* Module is enabled, clocks are not gated */
>> +                     ctrl &= 0xFFFFFFFE;
>> +                     __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>> +             }
>> +             bank->>gpio_status |= 1 << offset;
>> +     }
>why do this every time a gpio is enabled? why not do this iff gpio was
>never used before.. how about the following:
The module is enabled only when gpio_status indicates that no GPIO 
in that  module is currently active and the GPIO being requested is the 1st one 
to be active in that module.
Each module would be disabled in free() API when all GPIOs in a particular module 
becomes inactive. The module is re-enabled in request() API when a GPIO is 
requested from the module that was previously disabled.
>if (!bank->>gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
>cpu_is_omap44xx())) {
>        u32 ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>        /* Module is enabled, clocks are not gated */
>        ctrl &= 0xFFFFFFFE;
>        __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>}
>bank->>gpio_status |= 1 << offset;
Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)? 
>>        spin_unlock_irqrestore(&bank->>lock, flags);
>> 
>>        return 0;
>> @@ -1106,6 +1117,7 @@ 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;
>> +     u32 ctrl = 0;
>used just once ->> move it to the {} + warning to OMAP1
>> 
>>        spin_lock_irqsave(&bank->>lock, flags);
>>  #ifdef CONFIG_ARCH_OMAP16XX
>> @@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>>                __raw_writel(1 << offset, reg);
>>        }
>>  #endif
>> +     if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
>> +             bank->>gpio_status &= ~(1 << offset);
>> +             if (!bank->>gpio_status) {
>> +                     ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>> +                     /* Module is disabled, clocks are gated */
>> +                     ctrl |= 1;
>> +                     __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>> +             }
>> +     }
>how about this:
>        bank->>gpio_status &= ~(1 << offset);
>if (!bank->>gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
>cpu_is_omap44xx())) {
>        u32 ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>        /* Module is disabled, clocks are gated */
>        ctrl |= 1;
>        __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>}
Why to touch gpio_status if not used (for other than 24xx/34xx/44xx cases)? 
>>        _reset_gpio(bank, bank->>chip.base + offset);
>>        spin_unlock_irqrestore(&bank->>lock, flags);
>>  }
>> @@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
>>                        gpio_count = 32;
>>                }
>>  #endif
>> +             bank->>gpio_status = 0;
>>                /* REVISIT eventually switch from OMAP-specific gpio structs
>>                 * over to the generic ones
>>                 */

Re: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Nishanth Menon]

Varadarajan, Charu Latha had written, on 10/23/2009 11:05 PM, the following:
>>>  #endif
>>> +     if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
>>> +             if (!bank->>gpio_status) {
>>> +                     ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>>> +                     /* Module is enabled, clocks are not gated */
>>> +                     ctrl &= 0xFFFFFFFE;
>>> +                     __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>>> +             }
>>> +             bank->>gpio_status |= 1 << offset;
>>> +     }
>> why do this every time a gpio is enabled? why not do this iff gpio was
>> never used before.. how about the following:
> The module is enabled only when gpio_status indicates that no GPIO 
> in that  module is currently active and the GPIO being requested is the 1st one 
> to be active in that module.
> Each module would be disabled in free() API when all GPIOs in a particular module 
> becomes inactive. The module is re-enabled in request() API when a GPIO is 
> requested from the module that was previously disabled.
Thanks.

>> if (!bank->>gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
>> cpu_is_omap44xx())) {
>>        u32 ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>>        /* Module is enabled, clocks are not gated */
>>        ctrl &= 0xFFFFFFFE;
>>        __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>> }
>> bank->>gpio_status |= 1 << offset;
> Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)? 
either the gpio_status should be under a #ifdef for 34xx when defining 
or it should be usable by all. what it does now is do both.

my proposal is to allow gpio_status to be usable by ALL OMAPs -> maybe 
OMAP1 also could also use it.. I cannot comment - but it does look to 
have scope of usage beyond omap2/3/4 series?

-- 
Regards,
Nishanth Menon

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Varadarajan, Charu Latha]

>>>>  #endif
>>>> +     if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
>>>> +             if (!bank->>gpio_status) {
>>>> +                     ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>>>> +                     /* Module is enabled, clocks are not gated */
>>>> +                     ctrl &= 0xFFFFFFFE;
>>>> +                     __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>>>> +             }
>>>> +             bank->>gpio_status |= 1 << offset;
>>>> +     }
>>> why do this every time a gpio is enabled? why not do this iff gpio was
>>> never used before.. how about the following:
>> The module is enabled only when gpio_status indicates that no GPIO
>> in that  module is currently active and the GPIO being requested is the 1st one
>> to be active in that module.
>> Each module would be disabled in free() API when all GPIOs in a particular module
>> becomes inactive. The module is re-enabled in request() API when a GPIO is
>> requested from the module that was previously disabled.
>Thanks.
Welcome
>>> if (!bank->>gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
>>> cpu_is_omap44xx())) {
>>>        u32 ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
>>>        /* Module is enabled, clocks are not gated */
>>>        ctrl &= 0xFFFFFFFE;
>>>        __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
>>> }
>>> bank->>gpio_status |= 1 << offset;
>> Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)?
>either the gpio_status should be under a #ifdef for 34xx when defining
>or it should be usable by all. what it does now is do both.
 gpio_status is not used under  #ifdef for 34xx. It is used only with cpu_is_omap
(34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why? 
But I don't see that approach in LO. For eg., usage of dbck_enable_mask is 
used only with cpu_is_omap and is not declared under  #ifdef.

>my proposal is to allow gpio_status to be usable by ALL OMAPs -> maybe
>OMAP1 also could also use it.. I cannot comment - but it does look to
>have scope of usage beyond omap2/3/4 series?
Even though OMAP1 supports the same feature, I am not including it as I
cannot test it and I am not sure about it in OMAP1. For 24xx, 34xx & 44xx, 
the registers used and offsets are all the same. So the same code is reused. 

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Menon, Nishanth]

> From: Varadarajan, Charu Latha
> Sent: Monday, October 26, 2009 4:07 AM
> 
> >>>>  #endif
> >>>> +     if (cpu_is_omap24xx() || cpu_is_omap34xx() ||
> cpu_is_omap44xx()) {
> >>>> +             if (!bank->>gpio_status) {
> >>>> +                     ctrl = __raw_readl(bank->>base +
> OMAP24XX_GPIO_CTRL);
> >>>> +                     /* Module is enabled, clocks are not gated */
> >>>> +                     ctrl &= 0xFFFFFFFE;
> >>>> +                     __raw_writel(ctrl, bank->>base +
> OMAP24XX_GPIO_CTRL);
> >>>> +             }
> >>>> +             bank->>gpio_status |= 1 << offset;
> >>>> +     }
> >>> why do this every time a gpio is enabled? why not do this iff gpio was
> >>> never used before.. how about the following:
> >> The module is enabled only when gpio_status indicates that no GPIO
> >> in that  module is currently active and the GPIO being requested is the
> 1st one
> >> to be active in that module.
> >> Each module would be disabled in free() API when all GPIOs in a
> particular module
> >> becomes inactive. The module is re-enabled in request() API when a GPIO
> is
> >> requested from the module that was previously disabled.
> >Thanks.
> Welcome
> >>> if (!bank->>gpio_status && (cpu_is_omap24xx() || cpu_is_omap34xx() ||
> >>> cpu_is_omap44xx())) {
> >>>        u32 ctrl = __raw_readl(bank->>base + OMAP24XX_GPIO_CTRL);
> >>>        /* Module is enabled, clocks are not gated */
> >>>        ctrl &= 0xFFFFFFFE;
> >>>        __raw_writel(ctrl, bank->>base + OMAP24XX_GPIO_CTRL);
> >>> }
> >>> bank->>gpio_status |= 1 << offset;
> >> Why to touch gpio_status if not used (for other than 34xx/24xx/44xx
> cases)?
> >either the gpio_status should be under a #ifdef for 34xx when defining
> >or it should be usable by all. what it does now is do both.
>  gpio_status is not used under  #ifdef for 34xx. It is used only with
> cpu_is_omap
> (34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why?
> But I don't see that approach in LO. For eg., usage of dbck_enable_mask is
> used only with cpu_is_omap and is not declared under  #ifdef.

Please see [1] saved_datain is an example of why I think gpio.c could go thru a cleanup ;) already in #ifdef in line 1908, in line 1925, we add a new #ifdef under a #ifdef :D.. err...

Ok this piece of code is not perfect.. 

Regards,
Nishanth Menon
[1] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/gpio.c;h=487bea7b5605fe366064d792d0c9cc8aed1eac63;hb=0bbf5337f2f2957775051a3caf60b66d3306c815#l174 

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

[Varadarajan, Charu Latha]

--------sinipped----
>>>>> bank->>gpio_status |= 1 << offset;
>>>> Why to touch gpio_status if not used (for other than 34xx/24xx/44xx
>> cases)?
>>>either the gpio_status should be under a #ifdef for 34xx when defining
>>>or it should be usable by all. what it does now is do both.
>>  gpio_status is not used under  #ifdef for 34xx. It is used only with
>> cpu_is_omap
>> (34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why?
>> But I don't see that approach in LO. For eg., usage of dbck_enable_mask is
>> used only with cpu_is_omap and is not declared under  #ifdef.
>
>Please see [1] saved_datain is an example of why I think gpio.c could go thru a cleanup ;) already in #ifdef in line 1908, in line 1925, we add a new #ifdef under a #ifdef :D.. err...
>
>Ok this piece of code is not perfect..
Looking onto lines 1908 & 1925, I accept that the gpio.c code is not perfect. But they 
are nothing to do with my patch. I guess this is the same at many places in most
of the drivers and someone has to take up the job of cleaning up. 
>Regards,
>Nishanth Menon
>.[1] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/gpio.c;h=487bea7b5605fe366064d792d0c9cc8aed1eac63;hb=0bbf5337f2f2957775051a3caf60b66d3306c815#l174