| Message ID | 1402343774-1072-1-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Robert Jarzmik <robert.jarzmik@free.fr> writes: > Add the clock prepare and unprepare call to the driver initialization > phase. This will remove a warning once the PXA architecture is migrated > to the clock infrastructure. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> Ping ? -- Robert > --- > drivers/mmc/host/pxamci.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c > index 32fe113..f0f2074 100644 > --- a/drivers/mmc/host/pxamci.c > +++ b/drivers/mmc/host/pxamci.c > @@ -681,6 +681,9 @@ static int pxamci_probe(struct platform_device *pdev) > host->clk = NULL; > goto out; > } > + ret = clk_prepare(host->clk); > + if (ret) > + goto out; > > host->clkrate = clk_get_rate(host->clk); > > @@ -820,8 +823,10 @@ err_gpio_ro: > iounmap(host->base); > if (host->sg_cpu) > dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); > - if (host->clk) > + if (host->clk) { > + clk_unprepare(host->clk); > clk_put(host->clk); > + } > } > if (mmc) > mmc_free_host(mmc); > @@ -871,6 +876,7 @@ static int pxamci_remove(struct platform_device *pdev) > iounmap(host->base); > dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); > > + clk_unprepare(host->clk); > clk_put(host->clk); > > release_resource(host->res); -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 June 2014 21:56, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Add the clock prepare and unprepare call to the driver initialization > phase. This will remove a warning once the PXA architecture is migrated > to the clock infrastructure. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/mmc/host/pxamci.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c > index 32fe113..f0f2074 100644 > --- a/drivers/mmc/host/pxamci.c > +++ b/drivers/mmc/host/pxamci.c > @@ -681,6 +681,9 @@ static int pxamci_probe(struct platform_device *pdev) > host->clk = NULL; > goto out; > } > + ret = clk_prepare(host->clk); > + if (ret) > + goto out; > > host->clkrate = clk_get_rate(host->clk); > > @@ -820,8 +823,10 @@ err_gpio_ro: > iounmap(host->base); > if (host->sg_cpu) > dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); > - if (host->clk) > + if (host->clk) { > + clk_unprepare(host->clk); > clk_put(host->clk); > + } > } > if (mmc) > mmc_free_host(mmc); > @@ -871,6 +876,7 @@ static int pxamci_remove(struct platform_device *pdev) > iounmap(host->base); > dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); > > + clk_unprepare(host->clk); > clk_put(host->clk); > > release_resource(host->res); > -- > 2.0.0.rc2 > I would suggest you to re-place the existing clk_enable() with clk_prepare_enable() and clk_disable with clk_disable_unprepare() instead of the approach taken in this patch. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson <ulf.hansson@linaro.org> writes: > > I would suggest you to re-place the existing clk_enable() with > clk_prepare_enable() and clk_disable with clk_disable_unprepare() > instead of the approach taken in this patch. I don't think it works in this driver. Look at pxamci_set_ios(), and all the handling of host->clkrt (CLKRT_OFF, ...). This driver enables the clock only when transfers are required, not all the time. Therefore I'd rather let it be that way. Cheers.
On 1 September 2014 14:20, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> >> I would suggest you to re-place the existing clk_enable() with >> clk_prepare_enable() and clk_disable with clk_disable_unprepare() >> instead of the approach taken in this patch. > > I don't think it works in this driver. > > Look at pxamci_set_ios(), and all the handling of host->clkrt (CLKRT_OFF, > ...). This driver enables the clock only when transfers are required, not all > the time. Therefore I'd rather let it be that way. I understand your concern, but I don't see why there should be any major difference in clock management code (clk tree wise), due to this patch. It worked before, so likely it will work now!? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson <ulf.hansson@linaro.org> writes: > I understand your concern, but I don't see why there should be any > major difference in clock management code (clk tree wise), due to this > patch. It worked before, so likely it will work now!? It will ony work *differently*, it will change the clock management. It won't break, but again it's *not* the purpose of the patch. The patch is aimed at removing a warning. As for the clock management, it will the change the behaviour : Let's see the current clock management : - pxamci_probe() => clock is disabled mmc_add_host() mmc_start_host() mmc_power_up() (as pxamci is unaware of caps2) Here the comment of the function is (drivers/mmc/core/core.c:1534): "First, we enable power to the card without the clock running" => this won't be true if the clock is enabled in pxamci_probe() mmc_set_ios(host, host->ios.clock=host->f_init) pxamci_set_ios() clk_enable() => here the clock is enabled, enable_count=1 mmc_host_clk_release() pxamci_set_ios() => here the clock is disabled, enable_count=0 Let's see the your proposal clock management : - pxamci_probe() => clock is enabled mmc_add_host() mmc_start_host() mmc_power_up() (as pxamci is unaware of caps2) mmc_set_ios(host, host->ios.clock=host->f_init) pxamci_set_ios() clk_enable() => here the clock is enabled twice, enable_count=2 mmc_host_clk_release() pxamci_set_ios() => here the clock *remains enabled*, enable_count=1 - time passes with clock enabled So I think there is a difference, unless my understand of the MMC core stack is wrong. Cheers.
On 1 September 2014 15:28, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> I understand your concern, but I don't see why there should be any >> major difference in clock management code (clk tree wise), due to this >> patch. It worked before, so likely it will work now!? > > It will ony work *differently*, it will change the clock management. It won't > break, but again it's *not* the purpose of the patch. The patch is aimed at > removing a warning. Sorry if I was to vague, you must have misinterpreted my proposal. Let me try clarify how I think a patch should look like to solve the warning. 1) In pxamci_set_ios() replace " clk_enable()" with clk_prepare_enable(). 2) In pxamci_set_ios() replace " clk_disable()" with clk_disable_unprepare(). That should do the trick! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson <ulf.hansson@linaro.org> writes: > On 1 September 2014 15:28, Robert Jarzmik <robert.jarzmik@free.fr> wrote: >> Ulf Hansson <ulf.hansson@linaro.org> writes: >> >>> I understand your concern, but I don't see why there should be any >>> major difference in clock management code (clk tree wise), due to this >>> patch. It worked before, so likely it will work now!? >> >> It will ony work *differently*, it will change the clock management. It won't >> break, but again it's *not* the purpose of the patch. The patch is aimed at >> removing a warning. > > Sorry if I was to vague, you must have misinterpreted my proposal. Let > me try clarify how I think a patch should look like to solve the > warning. > > 1) In pxamci_set_ios() replace " clk_enable()" with clk_prepare_enable(). > 2) In pxamci_set_ios() replace " clk_disable()" with clk_disable_unprepare(). > > That should do the trick! OK, I got it now, and yes, the clock flow will be the same. I'm on my way to send v2 patch. Cheers.
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c index 32fe113..f0f2074 100644 --- a/drivers/mmc/host/pxamci.c +++ b/drivers/mmc/host/pxamci.c @@ -681,6 +681,9 @@ static int pxamci_probe(struct platform_device *pdev) host->clk = NULL; goto out; } + ret = clk_prepare(host->clk); + if (ret) + goto out; host->clkrate = clk_get_rate(host->clk); @@ -820,8 +823,10 @@ err_gpio_ro: iounmap(host->base); if (host->sg_cpu) dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); - if (host->clk) + if (host->clk) { + clk_unprepare(host->clk); clk_put(host->clk); + } } if (mmc) mmc_free_host(mmc); @@ -871,6 +876,7 @@ static int pxamci_remove(struct platform_device *pdev) iounmap(host->base); dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); + clk_unprepare(host->clk); clk_put(host->clk); release_resource(host->res);
Add the clock prepare and unprepare call to the driver initialization phase. This will remove a warning once the PXA architecture is migrated to the clock infrastructure. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/mmc/host/pxamci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)