* [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
* 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-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
* 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
* [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
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.