[PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Rajendra Nayak]

Replace the clk_enable()s with a clk_prepare_enable() and
the clk_disables()s with a clk_disable_unprepare()

This never showed issues due to the OMAP platform code (hwmod)
leaving these clocks in clk_prepare()ed state by default.

Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: linux-gpio@vger.kernel.org
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---
 drivers/gpio/gpio-omap.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 19b886c..78bc5a4 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
 static inline void _gpio_dbck_enable(struct gpio_bank *bank)
 {
 	if (bank->dbck_enable_mask && !bank->dbck_enabled) {
-		clk_enable(bank->dbck);
+		clk_prepare_enable(bank->dbck);
 		bank->dbck_enabled = true;
 
 		writel_relaxed(bank->dbck_enable_mask,
@@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
 		 */
 		writel_relaxed(0, bank->base + bank->regs->debounce_en);
 
-		clk_disable(bank->dbck);
+		clk_disable_unprepare(bank->dbck);
 		bank->dbck_enabled = false;
 	}
 }
@@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
 
 	l = GPIO_BIT(bank, gpio);
 
-	clk_enable(bank->dbck);
+	clk_prepare_enable(bank->dbck);
 	reg = bank->base + bank->regs->debounce;
 	writel_relaxed(debounce, reg);
 
@@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
 	bank->dbck_enable_mask = val;
 
 	writel_relaxed(val, reg);
-	clk_disable(bank->dbck);
+	clk_disable_unprepare(bank->dbck);
 	/*
 	 * Enable debounce clock per module.
 	 * This call is mandatory because in omap_gpio_request() when
@@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
 		bank->context.debounce = 0;
 		writel_relaxed(bank->context.debounce, bank->base +
 			     bank->regs->debounce);
-		clk_disable(bank->dbck);
+		clk_disable_unprepare(bank->dbck);
 		bank->dbck_enabled = false;
 	}
 }
-- 
1.7.9.5

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Rajendra Nayak]

On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
> Replace the clk_enable()s with a clk_prepare_enable() and
> the clk_disables()s with a clk_disable_unprepare()
> 
> This never showed issues due to the OMAP platform code (hwmod)
> leaving these clocks in clk_prepare()ed state by default.
> 
> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>

Linus,

Do you mind picking this fix up via the GPIO tree? Alternatively you could
Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
series via the OMAP tree.

Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
gpio would break. More discussions are here [1].
Let us know what you think. Thanks.

regards,
Rajendra

[1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html

> ---
>  drivers/gpio/gpio-omap.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 19b886c..78bc5a4 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>  static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>  {
>  	if (bank->dbck_enable_mask && !bank->dbck_enabled) {
> -		clk_enable(bank->dbck);
> +		clk_prepare_enable(bank->dbck);
>  		bank->dbck_enabled = true;
>  
>  		writel_relaxed(bank->dbck_enable_mask,
> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>  		 */
>  		writel_relaxed(0, bank->base + bank->regs->debounce_en);
>  
> -		clk_disable(bank->dbck);
> +		clk_disable_unprepare(bank->dbck);
>  		bank->dbck_enabled = false;
>  	}
>  }
> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>  
>  	l = GPIO_BIT(bank, gpio);
>  
> -	clk_enable(bank->dbck);
> +	clk_prepare_enable(bank->dbck);
>  	reg = bank->base + bank->regs->debounce;
>  	writel_relaxed(debounce, reg);
>  
> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>  	bank->dbck_enable_mask = val;
>  
>  	writel_relaxed(val, reg);
> -	clk_disable(bank->dbck);
> +	clk_disable_unprepare(bank->dbck);
>  	/*
>  	 * Enable debounce clock per module.
>  	 * This call is mandatory because in omap_gpio_request() when
> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
>  		bank->context.debounce = 0;
>  		writel_relaxed(bank->context.debounce, bank->base +
>  			     bank->regs->debounce);
> -		clk_disable(bank->dbck);
> +		clk_disable_unprepare(bank->dbck);
>  		bank->dbck_enabled = false;
>  	}
>  }
> 

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Javier Martinez Canillas]

Hello Rajendra,

On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
>> Replace the clk_enable()s with a clk_prepare_enable() and
>> the clk_disables()s with a clk_disable_unprepare()
>>
>> This never showed issues due to the OMAP platform code (hwmod)
>> leaving these clocks in clk_prepare()ed state by default.
>>
>> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>> Cc: linux-gpio@vger.kernel.org
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>
> Linus,
>
> Do you mind picking this fix up via the GPIO tree? Alternatively you could
> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
> series via the OMAP tree.
>
> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
> gpio would break. More discussions are here [1].
> Let us know what you think. Thanks.
>

I wonder if that is really the case. Your Patch 2/2 removes the call
to clk_prepare on _init_opt_clks() but it also replaces
clk_{enable,disable} with clk_prepare_enable()/clk_disable_unprepare()
on _enable_optional_clocks() and _disable_optional_clocks()
respectively.

And GPIO banks are reset by hwmod on init which as far as I know
happen very early before the GPIO OMAP driver is even probed so by the
time clk_enable() is called on the GPIO driver the clock will already
be prepared by _enable_optional_clocks(). I tested linux-gpio/devel
branch + only your Patch 2/2 and the GPIOs were working correctly on a
OMAP3 board.

So I think that there isn't a strict dependency between these two
patches or am I missing something?

In fact now that I think about it I wonder what's the functional
change of your Patch 2/2 since hwmod is still calling clk_prepare()
before the driver. If the clocks should actually be controlled by the
drivers like you said then I think that we should remove
_{enable,disable}_optional_clocks() completely and let the drivers do
the clock prepare and enable like is made on your Patch 1/2 for the
GPIO driver.

What do you think about it?

Best regards,
Javier

> regards,
> Rajendra
>
> [1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html
>
>> ---
>>  drivers/gpio/gpio-omap.c |   10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 19b886c..78bc5a4 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>  static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>  {
>>       if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>> -             clk_enable(bank->dbck);
>> +             clk_prepare_enable(bank->dbck);
>>               bank->dbck_enabled = true;
>>
>>               writel_relaxed(bank->dbck_enable_mask,
>> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>                */
>>               writel_relaxed(0, bank->base + bank->regs->debounce_en);
>>
>> -             clk_disable(bank->dbck);
>> +             clk_disable_unprepare(bank->dbck);
>>               bank->dbck_enabled = false;
>>       }
>>  }
>> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>
>>       l = GPIO_BIT(bank, gpio);
>>
>> -     clk_enable(bank->dbck);
>> +     clk_prepare_enable(bank->dbck);
>>       reg = bank->base + bank->regs->debounce;
>>       writel_relaxed(debounce, reg);
>>
>> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>       bank->dbck_enable_mask = val;
>>
>>       writel_relaxed(val, reg);
>> -     clk_disable(bank->dbck);
>> +     clk_disable_unprepare(bank->dbck);
>>       /*
>>        * Enable debounce clock per module.
>>        * This call is mandatory because in omap_gpio_request() when
>> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
>>               bank->context.debounce = 0;
>>               writel_relaxed(bank->context.debounce, bank->base +
>>                            bank->regs->debounce);
>> -             clk_disable(bank->dbck);
>> +             clk_disable_unprepare(bank->dbck);
>>               bank->dbck_enabled = false;
>>       }
>>  }
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Rajendra Nayak]

On Thursday 08 May 2014 02:56 PM, Javier Martinez Canillas wrote:
> Hello Rajendra,
> 
> On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>> On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
>>> Replace the clk_enable()s with a clk_prepare_enable() and
>>> the clk_disables()s with a clk_disable_unprepare()
>>>
>>> This never showed issues due to the OMAP platform code (hwmod)
>>> leaving these clocks in clk_prepare()ed state by default.
>>>
>>> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>>> Cc: linux-gpio@vger.kernel.org
>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>
>> Linus,
>>
>> Do you mind picking this fix up via the GPIO tree? Alternatively you could
>> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
>> series via the OMAP tree.
>>
>> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
>> gpio would break. More discussions are here [1].
>> Let us know what you think. Thanks.
>>
> 
> I wonder if that is really the case. Your Patch 2/2 removes the call
> to clk_prepare on _init_opt_clks() but it also replaces
> clk_{enable,disable} with clk_prepare_enable()/clk_disable_unprepare()
> on _enable_optional_clocks() and _disable_optional_clocks()
> respectively.

Right, the difference being, by the time hwmod is done enabling/disabling
the opt clocks, without patch 2/2, the prepare count is 1, with patch 2/2
prepare count is 0.

> 
> And GPIO banks are reset by hwmod on init which as far as I know
> happen very early before the GPIO OMAP driver is even probed so by the
> time clk_enable() is called on the GPIO driver the clock will already
> be prepared by _enable_optional_clocks(). I tested linux-gpio/devel

and unprepared by _disable_optional_clocks()?

> branch + only your Patch 2/2 and the GPIOs were working correctly on a
> OMAP3 board.

Did gpio_debounce() ever get called for any of the gpios?

> 
> So I think that there isn't a strict dependency between these two
> patches or am I missing something?
> 
> In fact now that I think about it I wonder what's the functional
> change of your Patch 2/2 since hwmod is still calling clk_prepare()
> before the driver. If the clocks should actually be controlled by the

I don't understand why you say 'before the driver'. Hwmod needs to control
optional clocks for some devices in order to do a ocp reset. So it does
touch these optional clocks, but if you look at the code it subsequently
also disables (and unprepares with patch 2/2) these clocks before returning
the control to the driver.

> drivers like you said then I think that we should remove
> _{enable,disable}_optional_clocks() completely and let the drivers do
> the clock prepare and enable like is made on your Patch 1/2 for the
> GPIO driver.
> 
> What do you think about it?
> 
> Best regards,
> Javier
> 
>> regards,
>> Rajendra
>>
>> [1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html
>>
>>> ---
>>>  drivers/gpio/gpio-omap.c |   10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 19b886c..78bc5a4 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>>  static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>>  {
>>>       if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>> -             clk_enable(bank->dbck);
>>> +             clk_prepare_enable(bank->dbck);
>>>               bank->dbck_enabled = true;
>>>
>>>               writel_relaxed(bank->dbck_enable_mask,
>>> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>>                */
>>>               writel_relaxed(0, bank->base + bank->regs->debounce_en);
>>>
>>> -             clk_disable(bank->dbck);
>>> +             clk_disable_unprepare(bank->dbck);
>>>               bank->dbck_enabled = false;
>>>       }
>>>  }
>>> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>
>>>       l = GPIO_BIT(bank, gpio);
>>>
>>> -     clk_enable(bank->dbck);
>>> +     clk_prepare_enable(bank->dbck);
>>>       reg = bank->base + bank->regs->debounce;
>>>       writel_relaxed(debounce, reg);
>>>
>>> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>       bank->dbck_enable_mask = val;
>>>
>>>       writel_relaxed(val, reg);
>>> -     clk_disable(bank->dbck);
>>> +     clk_disable_unprepare(bank->dbck);
>>>       /*
>>>        * Enable debounce clock per module.
>>>        * This call is mandatory because in omap_gpio_request() when
>>> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
>>>               bank->context.debounce = 0;
>>>               writel_relaxed(bank->context.debounce, bank->base +
>>>                            bank->regs->debounce);
>>> -             clk_disable(bank->dbck);
>>> +             clk_disable_unprepare(bank->dbck);
>>>               bank->dbck_enabled = false;
>>>       }
>>>  }
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Javier Martinez Canillas]

Hello Rajendra,

On Thu, May 8, 2014 at 1:10 PM, Rajendra Nayak <rnayak@ti.com> wrote:
> On Thursday 08 May 2014 02:56 PM, Javier Martinez Canillas wrote:
>> Hello Rajendra,
>>
>> On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>>> On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
>>>> Replace the clk_enable()s with a clk_prepare_enable() and
>>>> the clk_disables()s with a clk_disable_unprepare()
>>>>
>>>> This never showed issues due to the OMAP platform code (hwmod)
>>>> leaving these clocks in clk_prepare()ed state by default.
>>>>
>>>> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>>>> Cc: linux-gpio@vger.kernel.org
>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>
>>> Linus,
>>>
>>> Do you mind picking this fix up via the GPIO tree? Alternatively you could
>>> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
>>> series via the OMAP tree.
>>>
>>> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
>>> gpio would break. More discussions are here [1].
>>> Let us know what you think. Thanks.
>>>
>>
>> I wonder if that is really the case. Your Patch 2/2 removes the call
>> to clk_prepare on _init_opt_clks() but it also replaces
>> clk_{enable,disable} with clk_prepare_enable()/clk_disable_unprepare()
>> on _enable_optional_clocks() and _disable_optional_clocks()
>> respectively.
>
> Right, the difference being, by the time hwmod is done enabling/disabling
> the opt clocks, without patch 2/2, the prepare count is 1, with patch 2/2
> prepare count is 0.
>

Ok, got it now.

>>
>> And GPIO banks are reset by hwmod on init which as far as I know
>> happen very early before the GPIO OMAP driver is even probed so by the
>> time clk_enable() is called on the GPIO driver the clock will already
>> be prepared by _enable_optional_clocks(). I tested linux-gpio/devel
>
> and unprepared by _disable_optional_clocks()?
>

I see that _disable_optional_clocks() is called as well so the clock
is left unprepared as you said.

>> branch + only your Patch 2/2 and the GPIOs were working correctly on a
>> OMAP3 board.
>
> Did gpio_debounce() ever get called for any of the gpios?
>

I don't see gpio_debounce() to be called indeed.

omap_gpio_runtime_resume() is executed and calls
_gpio_dbck_enable(bank) but clk_enable(bank->dbck) is not called since
bank->dbck_enable_mask is 0, that was my confusion since I thought
that clk_enable() was called.

Now I understand the dependency between the two patches.

>>
>> So I think that there isn't a strict dependency between these two
>> patches or am I missing something?
>>
>> In fact now that I think about it I wonder what's the functional
>> change of your Patch 2/2 since hwmod is still calling clk_prepare()
>> before the driver. If the clocks should actually be controlled by the
>
> I don't understand why you say 'before the driver'. Hwmod needs to control
> optional clocks for some devices in order to do a ocp reset. So it does
> touch these optional clocks, but if you look at the code it subsequently
> also disables (and unprepares with patch 2/2) these clocks before returning
> the control to the driver.
>

Right, it was just me getting confused by the interaction between
hwmod and the GPIO driver. Thanks a lot for the explanation and sorry
for the noise.

Feel free to add my:

Acked-by: Javier Martinez Canillas <javier@dowhile0.org>

Best regards,
Javier

>> drivers like you said then I think that we should remove
>> _{enable,disable}_optional_clocks() completely and let the drivers do
>> the clock prepare and enable like is made on your Patch 1/2 for the
>> GPIO driver.
>>
>> What do you think about it?
>>
>> Best regards,
>> Javier
>>
>>> regards,
>>> Rajendra
>>>
>>> [1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html
>>>
>>>> ---
>>>>  drivers/gpio/gpio-omap.c |   10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index 19b886c..78bc5a4 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>>>  static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>>>  {
>>>>       if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>>> -             clk_enable(bank->dbck);
>>>> +             clk_prepare_enable(bank->dbck);
>>>>               bank->dbck_enabled = true;
>>>>
>>>>               writel_relaxed(bank->dbck_enable_mask,
>>>> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>>>                */
>>>>               writel_relaxed(0, bank->base + bank->regs->debounce_en);
>>>>
>>>> -             clk_disable(bank->dbck);
>>>> +             clk_disable_unprepare(bank->dbck);
>>>>               bank->dbck_enabled = false;
>>>>       }
>>>>  }
>>>> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>>
>>>>       l = GPIO_BIT(bank, gpio);
>>>>
>>>> -     clk_enable(bank->dbck);
>>>> +     clk_prepare_enable(bank->dbck);
>>>>       reg = bank->base + bank->regs->debounce;
>>>>       writel_relaxed(debounce, reg);
>>>>
>>>> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>>       bank->dbck_enable_mask = val;
>>>>
>>>>       writel_relaxed(val, reg);
>>>> -     clk_disable(bank->dbck);
>>>> +     clk_disable_unprepare(bank->dbck);
>>>>       /*
>>>>        * Enable debounce clock per module.
>>>>        * This call is mandatory because in omap_gpio_request() when
>>>> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
>>>>               bank->context.debounce = 0;
>>>>               writel_relaxed(bank->context.debounce, bank->base +
>>>>                            bank->regs->debounce);
>>>> -             clk_disable(bank->dbck);
>>>> +             clk_disable_unprepare(bank->dbck);
>>>>               bank->dbck_enabled = false;
>>>>       }
>>>>  }
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Rajendra Nayak]

On Thursday 08 May 2014 05:34 PM, Javier Martinez Canillas wrote:
> Hello Rajendra,
> 
> On Thu, May 8, 2014 at 1:10 PM, Rajendra Nayak <rnayak@ti.com> wrote:
>> On Thursday 08 May 2014 02:56 PM, Javier Martinez Canillas wrote:
>>> Hello Rajendra,
>>>
>>> On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>>>> On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
>>>>> Replace the clk_enable()s with a clk_prepare_enable() and
>>>>> the clk_disables()s with a clk_disable_unprepare()
>>>>>
>>>>> This never showed issues due to the OMAP platform code (hwmod)
>>>>> leaving these clocks in clk_prepare()ed state by default.
>>>>>
>>>>> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>>>>> Cc: linux-gpio@vger.kernel.org
>>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>>
>>>> Linus,
>>>>
>>>> Do you mind picking this fix up via the GPIO tree? Alternatively you could
>>>> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
>>>> series via the OMAP tree.
>>>>
>>>> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
>>>> gpio would break. More discussions are here [1].
>>>> Let us know what you think. Thanks.
>>>>
>>>
>>> I wonder if that is really the case. Your Patch 2/2 removes the call
>>> to clk_prepare on _init_opt_clks() but it also replaces
>>> clk_{enable,disable} with clk_prepare_enable()/clk_disable_unprepare()
>>> on _enable_optional_clocks() and _disable_optional_clocks()
>>> respectively.
>>
>> Right, the difference being, by the time hwmod is done enabling/disabling
>> the opt clocks, without patch 2/2, the prepare count is 1, with patch 2/2
>> prepare count is 0.
>>
> 
> Ok, got it now.
> 
>>>
>>> And GPIO banks are reset by hwmod on init which as far as I know
>>> happen very early before the GPIO OMAP driver is even probed so by the
>>> time clk_enable() is called on the GPIO driver the clock will already
>>> be prepared by _enable_optional_clocks(). I tested linux-gpio/devel
>>
>> and unprepared by _disable_optional_clocks()?
>>
> 
> I see that _disable_optional_clocks() is called as well so the clock
> is left unprepared as you said.
> 
>>> branch + only your Patch 2/2 and the GPIOs were working correctly on a
>>> OMAP3 board.
>>
>> Did gpio_debounce() ever get called for any of the gpios?
>>
> 
> I don't see gpio_debounce() to be called indeed.
> 
> omap_gpio_runtime_resume() is executed and calls
> _gpio_dbck_enable(bank) but clk_enable(bank->dbck) is not called since
> bank->dbck_enable_mask is 0, that was my confusion since I thought
> that clk_enable() was called.
> 
> Now I understand the dependency between the two patches.
> 
>>>
>>> So I think that there isn't a strict dependency between these two
>>> patches or am I missing something?
>>>
>>> In fact now that I think about it I wonder what's the functional
>>> change of your Patch 2/2 since hwmod is still calling clk_prepare()
>>> before the driver. If the clocks should actually be controlled by the
>>
>> I don't understand why you say 'before the driver'. Hwmod needs to control
>> optional clocks for some devices in order to do a ocp reset. So it does
>> touch these optional clocks, but if you look at the code it subsequently
>> also disables (and unprepares with patch 2/2) these clocks before returning
>> the control to the driver.
>>
> 
> Right, it was just me getting confused by the interaction between
> hwmod and the GPIO driver. Thanks a lot for the explanation and sorry
> for the noise.

No issues, thanks for the review and ack.

> 
> Feel free to add my:
> 
> Acked-by: Javier Martinez Canillas <javier@dowhile0.org>
> 
> Best regards,
> Javier
> 
>>> drivers like you said then I think that we should remove
>>> _{enable,disable}_optional_clocks() completely and let the drivers do
>>> the clock prepare and enable like is made on your Patch 1/2 for the
>>> GPIO driver.
>>>
>>> What do you think about it?
>>>
>>> Best regards,
>>> Javier
>>>
>>>> regards,
>>>> Rajendra
>>>>
>>>> [1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html
>>>>
>>>>> ---
>>>>>  drivers/gpio/gpio-omap.c |   10 +++++-----
>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>>> index 19b886c..78bc5a4 100644
>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>>>>  static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>>>>  {
>>>>>       if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>>>> -             clk_enable(bank->dbck);
>>>>> +             clk_prepare_enable(bank->dbck);
>>>>>               bank->dbck_enabled = true;
>>>>>
>>>>>               writel_relaxed(bank->dbck_enable_mask,
>>>>> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>>>>                */
>>>>>               writel_relaxed(0, bank->base + bank->regs->debounce_en);
>>>>>
>>>>> -             clk_disable(bank->dbck);
>>>>> +             clk_disable_unprepare(bank->dbck);
>>>>>               bank->dbck_enabled = false;
>>>>>       }
>>>>>  }
>>>>> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>>>
>>>>>       l = GPIO_BIT(bank, gpio);
>>>>>
>>>>> -     clk_enable(bank->dbck);
>>>>> +     clk_prepare_enable(bank->dbck);
>>>>>       reg = bank->base + bank->regs->debounce;
>>>>>       writel_relaxed(debounce, reg);
>>>>>
>>>>> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>>>       bank->dbck_enable_mask = val;
>>>>>
>>>>>       writel_relaxed(val, reg);
>>>>> -     clk_disable(bank->dbck);
>>>>> +     clk_disable_unprepare(bank->dbck);
>>>>>       /*
>>>>>        * Enable debounce clock per module.
>>>>>        * This call is mandatory because in omap_gpio_request() when
>>>>> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
>>>>>               bank->context.debounce = 0;
>>>>>               writel_relaxed(bank->context.debounce, bank->base +
>>>>>                            bank->regs->debounce);
>>>>> -             clk_disable(bank->dbck);
>>>>> +             clk_disable_unprepare(bank->dbck);
>>>>>               bank->dbck_enabled = false;
>>>>>       }
>>>>>  }
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Linus Walleij]

On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rnayak@ti.com> wrote:

> Do you mind picking this fix up via the GPIO tree?

Yes, it's merged to my devel branch now with the ACKs.

> Alternatively you could
> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from this
> series via the OMAP tree.

This probably will not work as I have a set of other changes to this
driver in my tree.

> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that order else
> gpio would break. More discussions are here [1].

Tell me if I should prepare an immutable tag on my branch that you
can pull in. I want an explicit handshake with the platform
maintainer for this kind of stuff.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Santosh Shilimkar]

On Wednesday 23 April 2014 02:11 AM, Rajendra Nayak wrote:
> Replace the clk_enable()s with a clk_prepare_enable() and
> the clk_disables()s with a clk_disable_unprepare()
> 
> This never showed issues due to the OMAP platform code (hwmod)
> leaving these clocks in clk_prepare()ed state by default.
> 
> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  drivers/gpio/gpio-omap.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock

[Santosh Shilimkar]

On Wednesday 23 April 2014 02:11 AM, Rajendra Nayak wrote:
> Replace the clk_enable()s with a clk_prepare_enable() and
> the clk_disables()s with a clk_disable_unprepare()
> 
> This never showed issues due to the OMAP platform code (hwmod)
> leaving these clocks in clk_prepare()ed state by default.
> 
> Reported-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
$subject patch looks fine but I don't see patch 2/2 assuming this
is series of two patches.

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 0/2] OMAP2+: optional clock handling fixes

[Paul Walmsley]

Hi Rajendra,

On Wed, 23 Apr 2014, Rajendra Nayak wrote:

> The patches fix some opt clock handling in gpio and in
> hwmod.
> 
> Rajendra Nayak (2):
>   gpio: omap: prepare and unprepare the debounce clock
>   ARM: OMAP2+: hwmod: Don't leave the optional clocks in
>     clk_prepare()ed state
> 
>  arch/arm/mach-omap2/omap_hwmod.c |   13 ++-----------
>  drivers/gpio/gpio-omap.c         |   10 +++++-----
>  2 files changed, 7 insertions(+), 16 deletions(-)

Can these patches be merged separately?  Looks to me that the two options 
are either to:

A. to merge them together, or 

B. to merge patch 1 first, then patch 2

Or will things break if only patch 1 is merged?


If we merge them together, I'd say the best situation would be to take 
them through the OMAP tree, since the changes are all OMAP-specific.  In 
that case we'll want an ack for the first patch from the GPIO maintainers, 
Linus Walleij <linus.walleij@linaro.org> and Alexandre Courbot 
<gnurou@gmail.com>.

Otherwise the path of least resistance would be (B) - you can get patch 1 
merged via the GPIO tree.  The GPIO maintainers can then provide a 
stable branch for us to base our changes on, or we can wait until the 
change reaches Linus.  Then we can subsequently merge patch 2 via the  
OMAP side.

Thoughts?


- Paul

Re: [PATCH 0/2] OMAP2+: optional clock handling fixes

[Rajendra Nayak]

On Thursday 08 May 2014 05:38 AM, Paul Walmsley wrote:
> Hi Rajendra,
> 
> On Wed, 23 Apr 2014, Rajendra Nayak wrote:
> 
>> The patches fix some opt clock handling in gpio and in
>> hwmod.
>>
>> Rajendra Nayak (2):
>>   gpio: omap: prepare and unprepare the debounce clock
>>   ARM: OMAP2+: hwmod: Don't leave the optional clocks in
>>     clk_prepare()ed state
>>
>>  arch/arm/mach-omap2/omap_hwmod.c |   13 ++-----------
>>  drivers/gpio/gpio-omap.c         |   10 +++++-----
>>  2 files changed, 7 insertions(+), 16 deletions(-)
> 
> Can these patches be merged separately?  Looks to me that the two options 
> are either to:
> 
> A. to merge them together, or 
> 
> B. to merge patch 1 first, then patch 2

Thats right.

> 
> Or will things break if only patch 1 is merged?

Things will break if only patch 2 is merged as gpios clk_enable()
request would fail. Merging only patch 1 has no issues.

> 
> 
> If we merge them together, I'd say the best situation would be to take 
> them through the OMAP tree, since the changes are all OMAP-specific.  In 
> that case we'll want an ack for the first patch from the GPIO maintainers, 
> Linus Walleij <linus.walleij@linaro.org> and Alexandre Courbot 
> <gnurou@gmail.com>.
> 
> Otherwise the path of least resistance would be (B) - you can get patch 1 
> merged via the GPIO tree.  The GPIO maintainers can then provide a 
> stable branch for us to base our changes on, or we can wait until the 
> change reaches Linus.  Then we can subsequently merge patch 2 via the  
> OMAP side.
> 
> Thoughts?

I am fine either way. I will check with Linus W. what he prefers. Thanks.

regards,
Rajendra
> 
> 
> - Paul
>