[RESEND,v2,6/6] mmc: sdhci-bcm-kona: Add basic use of clocks

Message ID 1381960030-1640-7-git-send-email-tim.kryger@linaro.org
State New
Headers show

Commit Message

Tim Kryger Oct. 16, 2013, 9:47 p.m.
Enable the external clock needed by the host controller during the
probe and disable it during the remove.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

Comments

Mark Rutland Oct. 17, 2013, 2:13 p.m. | #1
On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote:
> Enable the external clock needed by the host controller during the
> probe and disable it during the remove.

This requires a biding document update.

I note that the binding is already incomplete (it does not describe the
interrupts or reg).

> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
> index 85472d3..91db099 100644
> --- a/drivers/mmc/host/sdhci-bcm-kona.c
> +++ b/drivers/mmc/host/sdhci-bcm-kona.c
> @@ -54,6 +54,7 @@
>  
>  struct sdhci_bcm_kona_dev {
>  	struct mutex	write_lock; /* protect back to back writes */
> +	struct clk	*external_clk;

Is this the only clock input the unit has?

Are there any other external resources the device needs (e.g. gpios,
regulators)?

>  };
>  
>  
> @@ -252,11 +253,29 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
>  	mmc_of_parse(host->mmc);
>  
>  	if (!host->mmc->f_max) {
> -		dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
> +		dev_err(dev, "Missing max-freq for SDHCI cfg\n");

This seems like an unrelated change.

>  		ret = -ENXIO;
>  		goto err_pltfm_free;
>  	}
>  
> +	/* Get and enable the external clock */
> +	kona_dev->external_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(kona_dev->external_clk)) {
> +		dev_err(dev, "Failed to get external clock\n");
> +		ret = PTR_ERR(kona_dev->external_clk);
> +		goto err_pltfm_free;

This seems like a pretty clear breakage of existing DTBs.

Why?

Thanks,
Mark.
Tim Kryger Oct. 21, 2013, 11:13 p.m. | #2
On Thu, Oct 17, 2013 at 7:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote:
>> Enable the external clock needed by the host controller during the
>> probe and disable it during the remove.
>
> This requires a biding document update.

The current best practice for declaring the association of a clock
with a consumer device is to embed the common clock bindings 'clocks'
property inside its node but this is not the only way it may be done.

Given that clk_get() still works for clocks found using string
matching, is it appropriate to mandate that all drivers that uses the
clk api mention the common clock bindings 'clocks' property in the
documentation for their device's binding?

> I note that the binding is already incomplete (it does not describe the
> interrupts or reg).

The current document reflects the standard for
Documentation/devicetree/bindings/mmc where normal properties are
described in mmc.txt and only the deviations described in vendor
specific files.

>>
>> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
>> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
>> Reviewed-by: Matt Porter <matt.porter@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
>> index 85472d3..91db099 100644
>> --- a/drivers/mmc/host/sdhci-bcm-kona.c
>> +++ b/drivers/mmc/host/sdhci-bcm-kona.c
>> @@ -54,6 +54,7 @@
>>
>>  struct sdhci_bcm_kona_dev {
>>       struct mutex    write_lock; /* protect back to back writes */
>> +     struct clk      *external_clk;
>
> Is this the only clock input the unit has?

The SDHCI block only receives one clock.

> Are there any other external resources the device needs (e.g. gpios,
> regulators)?

Yes but the cd-gpio and vmmc/vqmmc regulators are handled by the
common SDHCI driver.

>
>>  };
>>
>>
>> @@ -252,11 +253,29 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
>>       mmc_of_parse(host->mmc);
>>
>>       if (!host->mmc->f_max) {
>> -             dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
>> +             dev_err(dev, "Missing max-freq for SDHCI cfg\n");
>
> This seems like an unrelated change.

Agreed.  I will remove this change.

>>               ret = -ENXIO;
>>               goto err_pltfm_free;
>>       }
>>
>> +     /* Get and enable the external clock */
>> +     kona_dev->external_clk = devm_clk_get(dev, NULL);
>> +     if (IS_ERR(kona_dev->external_clk)) {
>> +             dev_err(dev, "Failed to get external clock\n");
>> +             ret = PTR_ERR(kona_dev->external_clk);
>> +             goto err_pltfm_free;
>
> This seems like a pretty clear breakage of existing DTBs.
>
> Why?

The defconfig that builds this driver and DTBs currently sets
CONFIG_ARM_APPENDED_DTB=y so there isn't any actual breakage risk.

Patch

diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
index 85472d3..91db099 100644
--- a/drivers/mmc/host/sdhci-bcm-kona.c
+++ b/drivers/mmc/host/sdhci-bcm-kona.c
@@ -54,6 +54,7 @@ 
 
 struct sdhci_bcm_kona_dev {
 	struct mutex	write_lock; /* protect back to back writes */
+	struct clk	*external_clk;
 };
 
 
@@ -252,11 +253,29 @@  static int sdhci_bcm_kona_probe(struct platform_device *pdev)
 	mmc_of_parse(host->mmc);
 
 	if (!host->mmc->f_max) {
-		dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
+		dev_err(dev, "Missing max-freq for SDHCI cfg\n");
 		ret = -ENXIO;
 		goto err_pltfm_free;
 	}
 
+	/* Get and enable the external clock */
+	kona_dev->external_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(kona_dev->external_clk)) {
+		dev_err(dev, "Failed to get external clock\n");
+		ret = PTR_ERR(kona_dev->external_clk);
+		goto err_pltfm_free;
+	}
+
+	if (clk_set_rate(kona_dev->external_clk, host->mmc->f_max) != 0) {
+		dev_err(dev, "Failed to set rate external clock\n");
+		goto err_pltfm_free;
+	}
+
+	if (clk_prepare_enable(kona_dev->external_clk) != 0) {
+		dev_err(dev, "Failed to enable external clock\n");
+		goto err_pltfm_free;
+	}
+
 	dev_dbg(dev, "non-removable=%c\n",
 		(host->mmc->caps & MMC_CAP_NONREMOVABLE) ? 'Y' : 'N');
 	dev_dbg(dev, "cd_gpio %c, wp_gpio %c\n",
@@ -271,7 +290,7 @@  static int sdhci_bcm_kona_probe(struct platform_device *pdev)
 
 	ret = sdhci_bcm_kona_sd_reset(host);
 	if (ret)
-		goto err_pltfm_free;
+		goto err_clk_disable;
 
 	sdhci_bcm_kona_sd_init(host);
 
@@ -307,6 +326,9 @@  err_remove_host:
 err_reset:
 	sdhci_bcm_kona_sd_reset(host);
 
+err_clk_disable:
+	clk_disable_unprepare(kona_dev->external_clk);
+
 err_pltfm_free:
 	sdhci_pltfm_free(pdev);
 
@@ -317,6 +339,8 @@  err_pltfm_free:
 static int __exit sdhci_bcm_kona_remove(struct platform_device *pdev)
 {
 	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_priv = sdhci_priv(host);
+	struct sdhci_bcm_kona_dev *kona_dev = sdhci_pltfm_priv(pltfm_priv);
 	int dead;
 	u32 scratch;
 
@@ -326,6 +350,8 @@  static int __exit sdhci_bcm_kona_remove(struct platform_device *pdev)
 		dead = 1;
 	sdhci_remove_host(host, dead);
 
+	clk_disable_unprepare(kona_dev->external_clk);
+
 	sdhci_free_host(host);
 
 	return 0;