| Message ID | 1381960030-1640-7-git-send-email-tim.kryger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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.
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;