diff mbox

[1/2] mmc: tegra: Support module reset

Message ID 20161117171855.20843-1-thierry.reding@gmail.com
State New, archived
Headers show

Commit Message

Thierry Reding Nov. 17, 2016, 5:18 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

The device tree binding for the SDHCI controller found on Tegra SoCs
specifies that a reset control can be provided by the device tree. No
code was ever added to support the module reset, which can cause the
driver to try and access registers from a module that's in reset. On
most Tegra SoC generations doing so would cause a hang.

Note that it's unlikely to see this happen because on most platforms
these resets will have been deasserted by the bootloader. However the
portability can be improved by making sure the driver deasserts the
reset before accessing any registers.

Since resets are synchronous on Tegra SoCs, the platform driver needs
to implement a custom ->remove() callback now to make sure the clock
is disabled after the reset is asserted.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

Adrian Hunter Nov. 22, 2016, 11:30 a.m. UTC | #1
On 17/11/16 19:18, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The device tree binding for the SDHCI controller found on Tegra SoCs
> specifies that a reset control can be provided by the device tree. No
> code was ever added to support the module reset, which can cause the
> driver to try and access registers from a module that's in reset. On
> most Tegra SoC generations doing so would cause a hang.
> 
> Note that it's unlikely to see this happen because on most platforms
> these resets will have been deasserted by the bootloader. However the
> portability can be improved by making sure the driver deasserts the
> reset before accessing any registers.
> 
> Since resets are synchronous on Tegra SoCs, the platform driver needs
> to implement a custom ->remove() callback now to make sure the clock
> is disabled after the reset is asserted.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 20b6ff5b4af1..eac57bb161d3 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/reset.h>
>  #include <linux/mmc/card.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/mmc.h>
> @@ -65,6 +66,8 @@ struct sdhci_tegra {
>  	struct gpio_desc *power_gpio;
>  	bool ddr_signaling;
>  	bool pad_calib_required;
> +
> +	struct reset_control *rst;
>  };
>  
>  static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> @@ -489,6 +492,16 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>  	clk_prepare_enable(clk);
>  	pltfm_host->clk = clk;
>  
> +	tegra_host->rst = devm_reset_control_get(&pdev->dev, "sdhci");
> +	if (IS_ERR(tegra_host->rst)) {
> +		rc = PTR_ERR(tegra_host->rst);
> +		dev_err(&pdev->dev, "failed to get reset control: %d\n", rc);
> +		goto err_clk_get;
> +	}
> +
> +	reset_control_deassert(tegra_host->rst);
> +	usleep_range(2000, 4000);
> +
>  	rc = sdhci_add_host(host);
>  	if (rc)
>  		goto err_add_host;

Should you reset_control_assert() in the err_add_host path?

> @@ -504,6 +517,29 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>  	return rc;
>  }
>  
> +static int sdhci_tegra_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pltfm_host *platform = sdhci_priv(host);
> +	struct sdhci_tegra *tegra = sdhci_pltfm_priv(platform);
> +	int dead = 0;
> +	u32 value;
> +
> +	value = readl(host->ioaddr + SDHCI_INT_STATUS);
> +	if (value == 0xffffffff)

I think this check of 0xffffffff is a PCI thing.  Unless you know it is
meaningful for your platforms, you could leave it out.

> +		dead = 1;
> +
> +	sdhci_remove_host(host, dead);
> +
> +	reset_control_assert(tegra->rst);
> +	usleep_range(2000, 4000);
> +	clk_disable_unprepare(platform->clk);
> +
> +	sdhci_pltfm_free(pdev);
> +
> +	return 0;
> +}
> +
>  static struct platform_driver sdhci_tegra_driver = {
>  	.driver		= {
>  		.name	= "sdhci-tegra",
> @@ -511,7 +547,7 @@ static struct platform_driver sdhci_tegra_driver = {
>  		.pm	= &sdhci_pltfm_pmops,
>  	},
>  	.probe		= sdhci_tegra_probe,
> -	.remove		= sdhci_pltfm_unregister,
> +	.remove		= sdhci_tegra_remove,
>  };
>  
>  module_platform_driver(sdhci_tegra_driver);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 20b6ff5b4af1..eac57bb161d3 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -21,6 +21,7 @@ 
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/reset.h>
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
@@ -65,6 +66,8 @@  struct sdhci_tegra {
 	struct gpio_desc *power_gpio;
 	bool ddr_signaling;
 	bool pad_calib_required;
+
+	struct reset_control *rst;
 };
 
 static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
@@ -489,6 +492,16 @@  static int sdhci_tegra_probe(struct platform_device *pdev)
 	clk_prepare_enable(clk);
 	pltfm_host->clk = clk;
 
+	tegra_host->rst = devm_reset_control_get(&pdev->dev, "sdhci");
+	if (IS_ERR(tegra_host->rst)) {
+		rc = PTR_ERR(tegra_host->rst);
+		dev_err(&pdev->dev, "failed to get reset control: %d\n", rc);
+		goto err_clk_get;
+	}
+
+	reset_control_deassert(tegra_host->rst);
+	usleep_range(2000, 4000);
+
 	rc = sdhci_add_host(host);
 	if (rc)
 		goto err_add_host;
@@ -504,6 +517,29 @@  static int sdhci_tegra_probe(struct platform_device *pdev)
 	return rc;
 }
 
+static int sdhci_tegra_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *platform = sdhci_priv(host);
+	struct sdhci_tegra *tegra = sdhci_pltfm_priv(platform);
+	int dead = 0;
+	u32 value;
+
+	value = readl(host->ioaddr + SDHCI_INT_STATUS);
+	if (value == 0xffffffff)
+		dead = 1;
+
+	sdhci_remove_host(host, dead);
+
+	reset_control_assert(tegra->rst);
+	usleep_range(2000, 4000);
+	clk_disable_unprepare(platform->clk);
+
+	sdhci_pltfm_free(pdev);
+
+	return 0;
+}
+
 static struct platform_driver sdhci_tegra_driver = {
 	.driver		= {
 		.name	= "sdhci-tegra",
@@ -511,7 +547,7 @@  static struct platform_driver sdhci_tegra_driver = {
 		.pm	= &sdhci_pltfm_pmops,
 	},
 	.probe		= sdhci_tegra_probe,
-	.remove		= sdhci_pltfm_unregister,
+	.remove		= sdhci_tegra_remove,
 };
 
 module_platform_driver(sdhci_tegra_driver);