Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602165 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] mmc: sdhci: Add support for setting parent clock Jon Hunter <jonathanh@nvidia.com> - 2017-03-16 11:40 +0100
    [PATCH 2/2] mmc: tegra: Fix setting of Tegra SDHCI module clock Jon Hunter <jonathanh@nvidia.com> - 2017-03-16 11:40 +0100
    Re: [PATCH 1/2] mmc: sdhci: Add support for setting parent clock Jon Hunter <jonathanh@nvidia.com> - 2017-03-16 19:10 +0100
    Re: [PATCH 1/2] mmc: sdhci: Add support for setting parent clock Adrian Hunter <adrian.hunter@intel.com> - 2017-03-20 20:40 +0100
      Re: [PATCH 1/2] mmc: sdhci: Add support for setting parent clock Jon Hunter <jonathanh@nvidia.com> - 2017-03-20 21:50 +0100

#1602165 — [PATCH 1/2] mmc: sdhci: Add support for setting parent clock

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 aquire to
give the platform driver the opportunity to set the parent clock rate.

Please note that, unfortunately, 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>
---

I have not attempted to fix the MSM driver in this seris, but I am
copying hopefully, the right people to fix it.

 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..b7f1521edbec 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->power_mode);
+
 	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

[toc] | [next] | [standalone]

#1602168 — [PATCH 2/2] mmc: tegra: Fix setting of Tegra SDHCI module clock

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 CCF 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' 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>
---
 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

[toc] | [prev] | [next] | [standalone]

#1602724

On 16/03/17 10:32, 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 aquire to
> give the platform driver the opportunity to set the parent clock rate.
> 
> Please note that, unfortunately, 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>
> ---
> 
> I have not attempted to fix the MSM driver in this seris, but I am
> copying hopefully, the right people to fix it.
> 
>  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..b7f1521edbec 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->power_mode);

Ugh ... not sure what happened here but this should be 'ios->clock'! And
I did test this!

Sorry will resend.

Jon

-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1605089

On 03/16/2017 12:32 PM, 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 aquire to
> give the platform driver the opportunity to set the parent clock rate.

I just posted a patch to remove the spin lock from set_ios().  Does that help?

[toc] | [prev] | [next] | [standalone]

#1605128

On 20/03/17 19:22, Adrian Hunter wrote:
> On 03/16/2017 12:32 PM, 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 aquire to
>> give the platform driver the opportunity to set the parent clock rate.
> 
> I just posted a patch to remove the spin lock from set_ios().  Does that help?

Yes it does, thanks!

Technically, the only other place this could occur is in
sdhci_request_done() because this also calls ->set_clock() with the
spinlock held. However, this is only called if
SDHCI_QUIRK_CLOCK_BEFORE_RESET is set, which is only currently true for
a device in sdhci-pci-core.c and this one just uses the normal
sdhci_set_clock() routine. 	

Cheers
Jon

-- 
nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web