[PATCH V2 1/2] mmc: sdhci: Add support for setting parent clock

[PATCH V2 1/2] mmc: sdhci: Add support for setting parent clock

[Jon Hunter]

It is common for SD/MMC host controllers to set the parent clock that
drives the SD/MMC interface in order to support various operating
speeds. Typically, this is performed by calling common clock framework
APIs such as clk_set_rate(). The problem is that these APIs may sleep
and must not be called from within atomic sections and therefore, these
functions cannot be called within the existing 'set_clock' SDHCI
operator because they are called from within the context of a spinlock.
Add a new 'set_parent_clock' operator for the SDHCI driver that is
called early during the SDHCI 'set_ios' before the spinlock is aquired
to give the platform driver the opportunity to set the parent clock
rate.

Please note that the Tegra and MSM SDHCI drivers currently appear to
mis-use the 'set_clock' operator by calling clk_set_rate(). In the case
of Tegra, occasionally but not always, 'scheduling while atomic' errors
are reported (so most of the time we are getting lucky). In the of the
MSM SDHCI driver, it is releasing and re-acquiring the spinlock which is
bad.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

Changes since V1:
- Fixed idiotic copy-paste error and testing thoroughly!

 drivers/mmc/host/sdhci.c | 3 +++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6fdd7a70f229..8effc28ece15 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1579,6 +1579,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->power_mode == MMC_POWER_UNDEFINED)
 		return;
 
+	if (host->ops->set_parent_clock)
+		host->ops->set_parent_clock(host, ios->clock);
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	if (host->flags & SDHCI_DEVICE_DEAD) {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index edf3adfbc213..585fbcdab70c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -541,6 +541,8 @@ struct sdhci_ops {
 #endif
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
+	void	(*set_parent_clock)(struct sdhci_host *host,
+				    unsigned int clock);
 	void	(*set_power)(struct sdhci_host *host, unsigned char mode,
 			     unsigned short vdd);
 
-- 
2.7.4

[PATCH V2 1/2] mmc: sdhci: Add support for setting parent clock

[Jon Hunter]

It is common for SD/MMC host controllers to set the parent clock that
drives the SD/MMC interface in order to support various operating
speeds. Typically, this is performed by calling common clock framework
APIs such as clk_set_rate(). The problem is that these APIs may sleep
and must not be called from within atomic sections and therefore, these
functions cannot be called within the existing 'set_clock' SDHCI
operator because they are called from within the context of a spinlock.
Add a new 'set_parent_clock' operator for the SDHCI driver that is
called early during the SDHCI 'set_ios' before the spinlock is aquired
to give the platform driver the opportunity to set the parent clock
rate.

Please note that the Tegra and MSM SDHCI drivers currently appear to
mis-use the 'set_clock' operator by calling clk_set_rate(). In the case
of Tegra, occasionally but not always, 'scheduling while atomic' errors
are reported (so most of the time we are getting lucky). In the of the
MSM SDHCI driver, it is releasing and re-acquiring the spinlock which is
bad.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---

Changes since V1:
- Fixed idiotic copy-paste error and testing thoroughly!

 drivers/mmc/host/sdhci.c | 3 +++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6fdd7a70f229..8effc28ece15 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1579,6 +1579,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->power_mode == MMC_POWER_UNDEFINED)
 		return;
 
+	if (host->ops->set_parent_clock)
+		host->ops->set_parent_clock(host, ios->clock);
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	if (host->flags & SDHCI_DEVICE_DEAD) {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index edf3adfbc213..585fbcdab70c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -541,6 +541,8 @@ struct sdhci_ops {
 #endif
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
+	void	(*set_parent_clock)(struct sdhci_host *host,
+				    unsigned int clock);
 	void	(*set_power)(struct sdhci_host *host, unsigned char mode,
 			     unsigned short vdd);
 
-- 
2.7.4

[PATCH V2 2/2] mmc: tegra: Fix setting of Tegra SDHCI module clock

[Jon Hunter]

Commit a8e326a911d3 ("mmc: tegra: implement module external clock change")
implemented the SDHCI 'set_clock' handler for Tegra in order to change
the module clock for the Tegra SDHCI controller by using the common
clock framework API clk_set_rate(). The problem is the clk_set_rate()
may sleep and the 'set_clock' handler is always called from within the
context of a spinlock. Hence, occasionally, 'scheduling while atomic'
errors are seen. Fix this by moving the setting of the module clock to
the new 'set_parent_clock' handler which is not called from within the
context of a spinlock.

Fixes: a8e326a911d3 ("mmc: tegra: implement module external clock change")

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

Changes since V1:
- None

 drivers/mmc/host/sdhci-tegra.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 20b6ff5b4af1..048f84e615d3 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -217,18 +217,25 @@ static void tegra_sdhci_pad_autocalib(struct sdhci_host *host)
 	sdhci_writel(host,val, SDHCI_TEGRA_AUTO_CAL_CONFIG);
 }
 
-static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+static void tegra_sdhci_set_parent_clock(struct sdhci_host *host,
+					 unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	unsigned long host_clk;
 
 	if (!clock)
-		return sdhci_set_clock(host, clock);
+		return;
 
 	host_clk = tegra_host->ddr_signaling ? clock * 2 : clock;
-	clk_set_rate(pltfm_host->clk, host_clk);
+	WARN_ON(clk_set_rate(pltfm_host->clk, host_clk));
 	host->max_clk = clk_get_rate(pltfm_host->clk);
+}
+
+static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 
 	sdhci_set_clock(host, clock);
 
@@ -320,6 +327,7 @@ static const struct sdhci_ops tegra_sdhci_ops = {
 	.read_w     = tegra_sdhci_readw,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
+	.set_parent_clock = tegra_sdhci_set_parent_clock,
 	.set_bus_width = tegra_sdhci_set_bus_width,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
@@ -368,6 +376,7 @@ static const struct sdhci_ops tegra114_sdhci_ops = {
 	.write_w    = tegra_sdhci_writew,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
+	.set_parent_clock = tegra_sdhci_set_parent_clock,
 	.set_bus_width = tegra_sdhci_set_bus_width,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
-- 
2.7.4

[PATCH V2 2/2] mmc: tegra: Fix setting of Tegra SDHCI module clock

[Jon Hunter]

Commit a8e326a911d3 ("mmc: tegra: implement module external clock change")
implemented the SDHCI 'set_clock' handler for Tegra in order to change
the module clock for the Tegra SDHCI controller by using the common
clock framework API clk_set_rate(). The problem is the clk_set_rate()
may sleep and the 'set_clock' handler is always called from within the
context of a spinlock. Hence, occasionally, 'scheduling while atomic'
errors are seen. Fix this by moving the setting of the module clock to
the new 'set_parent_clock' handler which is not called from within the
context of a spinlock.

Fixes: a8e326a911d3 ("mmc: tegra: implement module external clock change")

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---

Changes since V1:
- None

 drivers/mmc/host/sdhci-tegra.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 20b6ff5b4af1..048f84e615d3 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -217,18 +217,25 @@ static void tegra_sdhci_pad_autocalib(struct sdhci_host *host)
 	sdhci_writel(host,val, SDHCI_TEGRA_AUTO_CAL_CONFIG);
 }
 
-static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+static void tegra_sdhci_set_parent_clock(struct sdhci_host *host,
+					 unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	unsigned long host_clk;
 
 	if (!clock)
-		return sdhci_set_clock(host, clock);
+		return;
 
 	host_clk = tegra_host->ddr_signaling ? clock * 2 : clock;
-	clk_set_rate(pltfm_host->clk, host_clk);
+	WARN_ON(clk_set_rate(pltfm_host->clk, host_clk));
 	host->max_clk = clk_get_rate(pltfm_host->clk);
+}
+
+static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 
 	sdhci_set_clock(host, clock);
 
@@ -320,6 +327,7 @@ static const struct sdhci_ops tegra_sdhci_ops = {
 	.read_w     = tegra_sdhci_readw,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
+	.set_parent_clock = tegra_sdhci_set_parent_clock,
 	.set_bus_width = tegra_sdhci_set_bus_width,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
@@ -368,6 +376,7 @@ static const struct sdhci_ops tegra114_sdhci_ops = {
 	.write_w    = tegra_sdhci_writew,
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
+	.set_parent_clock = tegra_sdhci_set_parent_clock,
 	.set_bus_width = tegra_sdhci_set_bus_width,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
-- 
2.7.4

Re: [PATCH V2 1/2] mmc: sdhci: Add support for setting parent clock

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Fri, Mar 17, 2017 at 09:25:31AM +0000, Jon Hunter wrote:
> It is common for SD/MMC host controllers to set the parent clock that
> drives the SD/MMC interface in order to support various operating
> speeds. Typically, this is performed by calling common clock framework
> APIs such as clk_set_rate(). The problem is that these APIs may sleep
> and must not be called from within atomic sections and therefore, these
> functions cannot be called within the existing 'set_clock' SDHCI
> operator because they are called from within the context of a spinlock.
> Add a new 'set_parent_clock' operator for the SDHCI driver that is
> called early during the SDHCI 'set_ios' before the spinlock is aquired
> to give the platform driver the opportunity to set the parent clock
> rate.
> 
> Please note that the Tegra and MSM SDHCI drivers currently appear to
> mis-use the 'set_clock' operator by calling clk_set_rate(). In the case
> of Tegra, occasionally but not always, 'scheduling while atomic' errors
> are reported (so most of the time we are getting lucky). In the of the
> MSM SDHCI driver, it is releasing and re-acquiring the spinlock which is
> bad.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 
> Changes since V1:
> - Fixed idiotic copy-paste error and testing thoroughly!
> 
>  drivers/mmc/host/sdhci.c | 3 +++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 5 insertions(+)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 2/2] mmc: tegra: Fix setting of Tegra SDHCI module clock

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On Fri, Mar 17, 2017 at 09:25:32AM +0000, Jon Hunter wrote:
> Commit a8e326a911d3 ("mmc: tegra: implement module external clock change")
> implemented the SDHCI 'set_clock' handler for Tegra in order to change
> the module clock for the Tegra SDHCI controller by using the common
> clock framework API clk_set_rate(). The problem is the clk_set_rate()
> may sleep and the 'set_clock' handler is always called from within the
> context of a spinlock. Hence, occasionally, 'scheduling while atomic'
> errors are seen. Fix this by moving the setting of the module clock to
> the new 'set_parent_clock' handler which is not called from within the
> context of a spinlock.
> 
> Fixes: a8e326a911d3 ("mmc: tegra: implement module external clock change")
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 
> Changes since V1:
> - None
> 
>  drivers/mmc/host/sdhci-tegra.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]