| Message ID | 1381960030-1640-7-git-send-email-tim.kryger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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 03:13:09PM +0100, Mark Rutland wrote: > On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote: > > + /* 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? Normal kernel incremental development. The notion of breaking DTBs is pretty much incompatible with the long-standing tradition of developing kernel support in an incremental fashion. In this case, the bootloader available on these boards has had clocks and regulators enabled for all the drivers currently upstream. This allowed development of the sdhci driver (and others) independent of having the common clk driver and PMU/regulator drivers ready. That's been especially important given the flux that the common clk framework is in with regards to bindings. Are you suggesting that the required clock needs to be made logically optional in the code to retain backward compatbility? -Matt
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;