* [PATCH 0/2] OMAP2+: optional clock handling fixes
@ 2014-04-23 6:11 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel; +Cc: tony, paul, Rajendra Nayak
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(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2] OMAP2+: optional clock handling fixes
@ 2014-04-23 6:11 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-04-23 6:11 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel
Cc: tony, paul, Rajendra Nayak, linux-gpio, Santosh Shilimkar, Kevin Hilman
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-04-23 6:11 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] ARM: OMAP2+: hwmod: Don't leave the optional clocks in clk_prepare()ed state
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-04-23 6:11 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel; +Cc: tony, paul, Rajendra Nayak, Benoit Cousson
At hwmod init, theres no reason why optional clocks should be left in
clk_prepare()ed state as these are actually directly controlled by the
drivers themselves. Let the drivers prepare/unprepare as well as enable/
disable them.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Benoit Cousson <bcousson@baylibre.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 1f33f5d..9e3afc9 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -857,15 +857,6 @@ static int _init_opt_clks(struct omap_hwmod *oh)
continue;
}
oc->_clk = c;
- /*
- * HACK: This needs a re-visit once clk_prepare() is implemented
- * to do something meaningful. Today its just a no-op.
- * If clk_prepare() is used at some point to do things like
- * voltage scaling etc, then this would have to be moved to
- * some point where subsystems like i2c and pmic become
- * available.
- */
- clk_prepare(oc->_clk);
}
return ret;
@@ -945,7 +936,7 @@ static void _enable_optional_clocks(struct omap_hwmod *oh)
if (oc->_clk) {
pr_debug("omap_hwmod: enable %s:%s\n", oc->role,
__clk_get_name(oc->_clk));
- clk_enable(oc->_clk);
+ clk_prepare_enable(oc->_clk);
}
}
@@ -960,7 +951,7 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
if (oc->_clk) {
pr_debug("omap_hwmod: disable %s:%s\n", oc->role,
__clk_get_name(oc->_clk));
- clk_disable(oc->_clk);
+ clk_disable_unprepare(oc->_clk);
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] ARM: OMAP2+: hwmod: Don't leave the optional clocks in clk_prepare()ed state
@ 2014-04-23 6:11 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-04-23 6:11 UTC (permalink / raw)
To: linux-arm-kernel
At hwmod init, theres no reason why optional clocks should be left in
clk_prepare()ed state as these are actually directly controlled by the
drivers themselves. Let the drivers prepare/unprepare as well as enable/
disable them.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Benoit Cousson <bcousson@baylibre.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 1f33f5d..9e3afc9 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -857,15 +857,6 @@ static int _init_opt_clks(struct omap_hwmod *oh)
continue;
}
oc->_clk = c;
- /*
- * HACK: This needs a re-visit once clk_prepare() is implemented
- * to do something meaningful. Today its just a no-op.
- * If clk_prepare() is used at some point to do things like
- * voltage scaling etc, then this would have to be moved to
- * some point where subsystems like i2c and pmic become
- * available.
- */
- clk_prepare(oc->_clk);
}
return ret;
@@ -945,7 +936,7 @@ static void _enable_optional_clocks(struct omap_hwmod *oh)
if (oc->_clk) {
pr_debug("omap_hwmod: enable %s:%s\n", oc->role,
__clk_get_name(oc->_clk));
- clk_enable(oc->_clk);
+ clk_prepare_enable(oc->_clk);
}
}
@@ -960,7 +951,7 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
if (oc->_clk) {
pr_debug("omap_hwmod: disable %s:%s\n", oc->role,
__clk_get_name(oc->_clk));
- clk_disable(oc->_clk);
+ clk_disable_unprepare(oc->_clk);
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] OMAP2+: optional clock handling fixes
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-05-08 0:08 ` Paul Walmsley
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Walmsley @ 2014-05-08 0:08 UTC (permalink / raw)
To: Rajendra Nayak; +Cc: linux-omap, linux-arm-kernel, tony, linux-gpio
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2] OMAP2+: optional clock handling fixes
@ 2014-05-08 0:08 ` Paul Walmsley
0 siblings, 0 replies; 26+ messages in thread
From: Paul Walmsley @ 2014-05-08 0:08 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] OMAP2+: optional clock handling fixes
2014-05-08 0:08 ` Paul Walmsley
@ 2014-05-08 6:59 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 6:59 UTC (permalink / raw)
To: Paul Walmsley; +Cc: linux-omap, linux-arm-kernel, tony, linux-gpio
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2] OMAP2+: optional clock handling fixes
@ 2014-05-08 6:59 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 6:59 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-05-08 7:06 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 7:06 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linux-omap, linux-arm-kernel, tony, paul, linux-gpio,
Santosh Shilimkar, Kevin Hilman, Linus Walleij, gnurou
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;
> }
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 7:06 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 7:06 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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;
> }
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-05-08 7:06 ` Rajendra Nayak
@ 2014-05-08 9:26 ` Javier Martinez Canillas
-1 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2014-05-08 9:26 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linux-omap, linux-arm-kernel, Tony Lindgren, Paul Walmsley,
Linux GPIO List, Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 9:26 ` Javier Martinez Canillas
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2014-05-08 9:26 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-05-08 9:26 ` Javier Martinez Canillas
@ 2014-05-08 11:10 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 11:10 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-omap, linux-arm-kernel, Tony Lindgren, Paul Walmsley,
Linux GPIO List, Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 11:10 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 11:10 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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 at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-05-08 11:10 ` Rajendra Nayak
@ 2014-05-08 12:04 ` Javier Martinez Canillas
-1 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2014-05-08 12:04 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linux-omap, linux-arm-kernel, Tony Lindgren, Paul Walmsley,
Linux GPIO List, Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 12:04 ` Javier Martinez Canillas
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2014-05-08 12:04 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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 at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-05-08 12:04 ` Javier Martinez Canillas
@ 2014-05-08 12:06 ` Rajendra Nayak
-1 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 12:06 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-omap, linux-arm-kernel, Tony Lindgren, Paul Walmsley,
Linux GPIO List, Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot
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
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 12:06 ` Rajendra Nayak
0 siblings, 0 replies; 26+ messages in thread
From: Rajendra Nayak @ 2014-05-08 12:06 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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 at vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-05-08 14:40 ` Santosh Shilimkar
-1 siblings, 0 replies; 26+ messages in thread
From: Santosh Shilimkar @ 2014-05-08 14:40 UTC (permalink / raw)
To: Rajendra Nayak, linux-omap, linux-arm-kernel
Cc: tony, paul, linux-gpio, Kevin Hilman
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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 14:40 ` Santosh Shilimkar
0 siblings, 0 replies; 26+ messages in thread
From: Santosh Shilimkar @ 2014-05-08 14:40 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-04-23 6:11 ` Rajendra Nayak
@ 2014-05-08 14:45 ` Santosh Shilimkar
-1 siblings, 0 replies; 26+ messages in thread
From: Santosh Shilimkar @ 2014-05-08 14:45 UTC (permalink / raw)
To: Rajendra Nayak, linux-omap, linux-arm-kernel
Cc: tony, paul, linux-gpio, Kevin Hilman
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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-08 14:45 ` Santosh Shilimkar
0 siblings, 0 replies; 26+ messages in thread
From: Santosh Shilimkar @ 2014-05-08 14:45 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
2014-05-08 7:06 ` Rajendra Nayak
@ 2014-05-13 9:24 ` Linus Walleij
-1 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2014-05-13 9:24 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Linux-OMAP, linux-arm-kernel, ext Tony Lindgren, Paul Walmsley,
linux-gpio, Santosh Shilimkar, Kevin Hilman, Alexandre Courbot
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock
@ 2014-05-13 9:24 ` Linus Walleij
0 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2014-05-13 9:24 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2014-05-13 9:24 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-23 6:11 [PATCH 0/2] OMAP2+: optional clock handling fixes Rajendra Nayak
2014-04-23 6:11 ` Rajendra Nayak
2014-04-23 6:11 ` [PATCH 1/2] gpio: omap: prepare and unprepare the debounce clock Rajendra Nayak
2014-04-23 6:11 ` Rajendra Nayak
2014-05-08 7:06 ` Rajendra Nayak
2014-05-08 7:06 ` Rajendra Nayak
2014-05-08 9:26 ` Javier Martinez Canillas
2014-05-08 9:26 ` Javier Martinez Canillas
2014-05-08 11:10 ` Rajendra Nayak
2014-05-08 11:10 ` Rajendra Nayak
2014-05-08 12:04 ` Javier Martinez Canillas
2014-05-08 12:04 ` Javier Martinez Canillas
2014-05-08 12:06 ` Rajendra Nayak
2014-05-08 12:06 ` Rajendra Nayak
2014-05-13 9:24 ` Linus Walleij
2014-05-13 9:24 ` Linus Walleij
2014-05-08 14:40 ` Santosh Shilimkar
2014-05-08 14:40 ` Santosh Shilimkar
2014-05-08 14:45 ` Santosh Shilimkar
2014-05-08 14:45 ` Santosh Shilimkar
2014-04-23 6:11 ` [PATCH 2/2] ARM: OMAP2+: hwmod: Don't leave the optional clocks in clk_prepare()ed state Rajendra Nayak
2014-04-23 6:11 ` Rajendra Nayak
2014-05-08 0:08 ` [PATCH 0/2] OMAP2+: optional clock handling fixes Paul Walmsley
2014-05-08 0:08 ` Paul Walmsley
2014-05-08 6:59 ` Rajendra Nayak
2014-05-08 6:59 ` Rajendra Nayak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.