| Message ID | 551A61FB.1040401@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Javier Martinez Canillas (2015-03-31 01:59:39) > +Tomeu who I forgot to add to the cc list. > > Hello Mike, > > Thanks a lot for your feedback. > > On 03/31/2015 03:40 AM, Michael Turquette wrote: > >> > >> I don't performance is a big issue here. I just thought that since the > >> lookup table is already filled by the driver and the lookup function > >> is one line, we could use that instead to get the performance benefit. > >> > >> But I don't mind to drop this patch and use the generic lookup function > >> from the CCF API if that is preferred. > > > > Hello, > > > > I am not a fan of __clk_lookup and I don't like to see it used more and > > more outside of drivers/clk/clk.c. You mentioned that performance wasn't > > really the problem here. The real method for a driver to get a clock is > > with clk_get(). Any reason to not use that? > > > > I can certainly use clk_get() but I thought that the clk consumer API was > not supposed to be used from within clock drivers. That's why I mentioned > __clk_lookup() as a possibility since that is part of the provider API. I would like to remove __clk_lookup some day, so the fewer users now the better. Additionally, now that we have unique struct clk pointers from clk_get it makes a lot of sense for clk_register to stop returning pointers to a struct clk. Hopefully we can get around to that soon. These two points above are enough reason for the clock provider to use clk_get. > > Below is a RFC patch that uses clk_get() [0]. That needs another patch > which was part of a previous RFC and adds an alias for the mdma0 clock: > > https://lkml.org/lkml/2015/3/27/769 > > If you think that is the correct approach then I can post it as a patch. > > It would be great if you can also provide some feedback about the other > patch in the first RFC that instead of enabling and disabling the mdma0 > clock in driver, does it in the exynos5420 platform PM callbacks: > > https://lkml.org/lkml/2015/3/27/770 This seems like a big hack to me. > > I was asked to do it in the exynos5420 clk driver instead but maybe you > have a different opinion on that. > > Best regards, > Javier > > [0]: > From c118df83da8cac65cc218ae9443622592222f5d0 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Date: Mon, 30 Mar 2015 17:11:40 +0200 > Subject: [RFC] clk: exynos5420: Make sure MDMA0 clock is enabled during > suspend > > Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power > Management support v12") added pm support for the pl330 dma driver but > it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated > during suspend and this clock needs to remain enabled in order to make > the system resume from a system suspend state. > > To make sure that the clock is enabled during suspend, enable it prior > to entering a suspend state and disable it once the system has resumed. I'd like to understand the issue a bit further. Isn't the correct solution that the clk's prepare/unprepare and enable/disable callbacks should support the CG_STATUS requirement? Regards, Mike > > Thanks to Abhilash Kesavan for figuring out that this was the issue. > > Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12") > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/clk/samsung/clk-exynos5420.c | 44 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c > index 8b49e8b3b548..02029cf9fcb8 100644 > --- a/drivers/clk/samsung/clk-exynos5420.c > +++ b/drivers/clk/samsung/clk-exynos5420.c > @@ -155,6 +155,7 @@ static enum exynos5x_soc exynos5x_soc; > #ifdef CONFIG_PM_SLEEP > static struct samsung_clk_reg_dump *exynos5x_save; > static struct samsung_clk_reg_dump *exynos5800_save; > +static struct clk **exynos5x_clks; > > /* > * list of controller registers to be saved and restored during a > @@ -275,8 +276,17 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = { > { .offset = GATE_IP_PERIC, .value = 0xffffffff, }, > }; > > +/* > + * list of clocks that have to be kept enabled during suspend/resume cycle. > + */ > +static const char *exynos5x_clk_pm[] __initdata = { > + "mdma0", > +}; > + > static int exynos5420_clk_suspend(void) > { > + int i; > + > samsung_clk_save(reg_base, exynos5x_save, > ARRAY_SIZE(exynos5x_clk_regs)); > > @@ -287,11 +297,19 @@ static int exynos5420_clk_suspend(void) > samsung_clk_restore(reg_base, exynos5420_set_clksrc, > ARRAY_SIZE(exynos5420_set_clksrc)); > > + for (i = 0; i < ARRAY_SIZE(exynos5x_clk_pm); i++) > + clk_prepare_enable(exynos5x_clks[i]); > + > return 0; > } > > static void exynos5420_clk_resume(void) > { > + int i; > + > + for (i = 0; i < ARRAY_SIZE(exynos5x_clk_pm); i++) > + clk_disable_unprepare(exynos5x_clks[i]); > + > samsung_clk_restore(reg_base, exynos5x_save, > ARRAY_SIZE(exynos5x_clk_regs)); > > @@ -307,6 +325,9 @@ static struct syscore_ops exynos5420_clk_syscore_ops = { > > static void exynos5420_clk_sleep_init(void) > { > + int i; > + int clk_len = ARRAY_SIZE(exynos5x_clk_pm); > + > exynos5x_save = samsung_clk_alloc_reg_dump(exynos5x_clk_regs, > ARRAY_SIZE(exynos5x_clk_regs)); > if (!exynos5x_save) { > @@ -323,8 +344,31 @@ static void exynos5420_clk_sleep_init(void) > goto err_soc; > } > > + exynos5x_clks = kzalloc(sizeof(struct clk *) * clk_len, GFP_KERNEL); > + if (!exynos5x_clks) > + goto err_clks; > + > + for (i = 0; i < clk_len; i++) { > + exynos5x_clks[i] = clk_get(NULL, exynos5x_clk_pm[i]); > + if (IS_ERR(exynos5x_clks[i])) { > + pr_warn("Failed to get %s clk (%ld)\n", > + exynos5x_clk_pm[i], PTR_ERR(exynos5x_clks[i])); > + > + while (i--) > + clk_put(exynos5x_clks[i]); > + > + goto err_clkget; > + } > + } > + > register_syscore_ops(&exynos5420_clk_syscore_ops); > return; > +err_clkget: > + kfree(exynos5x_clks); > +err_clks: > + kfree(exynos5800_save); > + pr_warn("%s: failed to allocate suspend clocks, no sleep support!\n", > + __func__); > err_soc: > kfree(exynos5x_save); > pr_warn("%s: failed to allocate sleep save data, no sleep support!\n", > -- > 2.1.4
Hello Mike, On 04/01/2015 03:29 AM, Michael Turquette wrote: > Quoting Javier Martinez Canillas (2015-03-31 01:59:39) >> +Tomeu who I forgot to add to the cc list. >> >> Hello Mike, >> >> Thanks a lot for your feedback. >> >> On 03/31/2015 03:40 AM, Michael Turquette wrote: >> >> >> >> I don't performance is a big issue here. I just thought that since the >> >> lookup table is already filled by the driver and the lookup function >> >> is one line, we could use that instead to get the performance benefit. >> >> >> >> But I don't mind to drop this patch and use the generic lookup function >> >> from the CCF API if that is preferred. >> > >> > Hello, >> > >> > I am not a fan of __clk_lookup and I don't like to see it used more and >> > more outside of drivers/clk/clk.c. You mentioned that performance wasn't >> > really the problem here. The real method for a driver to get a clock is >> > with clk_get(). Any reason to not use that? >> > >> >> I can certainly use clk_get() but I thought that the clk consumer API was >> not supposed to be used from within clock drivers. That's why I mentioned >> __clk_lookup() as a possibility since that is part of the provider API. > > I would like to remove __clk_lookup some day, so the fewer users now the > better. Additionally, now that we have unique struct clk pointers from > clk_get it makes a lot of sense for clk_register to stop returning > pointers to a struct clk. Hopefully we can get around to that soon. > > These two points above are enough reason for the clock provider to use > clk_get. > Got it. >> >> Below is a RFC patch that uses clk_get() [0]. That needs another patch >> which was part of a previous RFC and adds an alias for the mdma0 clock: >> >> https://lkml.org/lkml/2015/3/27/769 >> >> If you think that is the correct approach then I can post it as a patch. >> >> It would be great if you can also provide some feedback about the other >> patch in the first RFC that instead of enabling and disabling the mdma0 >> clock in driver, does it in the exynos5420 platform PM callbacks: >> >> https://lkml.org/lkml/2015/3/27/770 > > This seems like a big hack to me. > Ok, good to know. I just did because initially I thought that the clock consumer API was not allowed to be used from within clock drivers. >> >> I was asked to do it in the exynos5420 clk driver instead but maybe you >> have a different opinion on that. >> >> Best regards, >> Javier >> >> [0]: >> From c118df83da8cac65cc218ae9443622592222f5d0 Mon Sep 17 00:00:00 2001 >> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Date: Mon, 30 Mar 2015 17:11:40 +0200 >> Subject: [RFC] clk: exynos5420: Make sure MDMA0 clock is enabled during >> suspend >> >> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power >> Management support v12") added pm support for the pl330 dma driver but >> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated >> during suspend and this clock needs to remain enabled in order to make >> the system resume from a system suspend state. >> >> To make sure that the clock is enabled during suspend, enable it prior >> to entering a suspend state and disable it once the system has resumed. > > I'd like to understand the issue a bit further. Isn't the correct > solution that the clk's prepare/unprepare and enable/disable callbacks > should support the CG_STATUS requirement? > Agreed, I'll take a look to that. > Regards, > Mike > Best regards, Javier
diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c index 8b49e8b3b548..02029cf9fcb8 100644 --- a/drivers/clk/samsung/clk-exynos5420.c +++ b/drivers/clk/samsung/clk-exynos5420.c @@ -155,6 +155,7 @@ static enum exynos5x_soc exynos5x_soc; #ifdef CONFIG_PM_SLEEP static struct samsung_clk_reg_dump *exynos5x_save; static struct samsung_clk_reg_dump *exynos5800_save; +static struct clk **exynos5x_clks; /* * list of controller registers to be saved and restored during a @@ -275,8 +276,17 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = { { .offset = GATE_IP_PERIC, .value = 0xffffffff, }, }; +/* + * list of clocks that have to be kept enabled during suspend/resume cycle. + */ +static const char *exynos5x_clk_pm[] __initdata = { + "mdma0", +}; + static int exynos5420_clk_suspend(void) { + int i; + samsung_clk_save(reg_base, exynos5x_save, ARRAY_SIZE(exynos5x_clk_regs)); @@ -287,11 +297,19 @@ static int exynos5420_clk_suspend(void) samsung_clk_restore(reg_base, exynos5420_set_clksrc, ARRAY_SIZE(exynos5420_set_clksrc)); + for (i = 0; i < ARRAY_SIZE(exynos5x_clk_pm); i++) + clk_prepare_enable(exynos5x_clks[i]); + return 0; } static void exynos5420_clk_resume(void) { + int i; + + for (i = 0; i < ARRAY_SIZE(exynos5x_clk_pm); i++) + clk_disable_unprepare(exynos5x_clks[i]); + samsung_clk_restore(reg_base, exynos5x_save, ARRAY_SIZE(exynos5x_clk_regs)); @@ -307,6 +325,9 @@ static struct syscore_ops exynos5420_clk_syscore_ops = { static void exynos5420_clk_sleep_init(void) { + int i; + int clk_len = ARRAY_SIZE(exynos5x_clk_pm); + exynos5x_save = samsung_clk_alloc_reg_dump(exynos5x_clk_regs, ARRAY_SIZE(exynos5x_clk_regs)); if (!exynos5x_save) { @@ -323,8 +344,31 @@ static void exynos5420_clk_sleep_init(void) goto err_soc; } + exynos5x_clks = kzalloc(sizeof(struct clk *) * clk_len, GFP_KERNEL); + if (!exynos5x_clks) + goto err_clks; + + for (i = 0; i < clk_len; i++) { + exynos5x_clks[i] = clk_get(NULL, exynos5x_clk_pm[i]); + if (IS_ERR(exynos5x_clks[i])) { + pr_warn("Failed to get %s clk (%ld)\n", + exynos5x_clk_pm[i], PTR_ERR(exynos5x_clks[i])); + + while (i--) + clk_put(exynos5x_clks[i]); + + goto err_clkget; + } + } + register_syscore_ops(&exynos5420_clk_syscore_ops); return; +err_clkget: + kfree(exynos5x_clks); +err_clks: + kfree(exynos5800_save); + pr_warn("%s: failed to allocate suspend clocks, no sleep support!\n", + __func__); err_soc: kfree(exynos5x_save); pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",