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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |