[PATCH] mmc: pxamci: prepare and unprepare the clocks

[PATCH] mmc: pxamci: prepare and unprepare the clocks

[Robert Jarzmik]

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

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Robert Jarzmik]

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);

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Ulf Hansson]

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

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Robert Jarzmik]

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.

-- 
Robert

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Ulf Hansson]

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

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Robert Jarzmik]

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.

-- 
Robert

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Ulf Hansson]

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

Re: [PATCH] mmc: pxamci: prepare and unprepare the clocks

[Robert Jarzmik]

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.

-- 
Robert