| Message ID | 20160819140003.22608-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.08.2016 17:00, 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 1e93dc4e303e..4c29a64721aa 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) > @@ -464,6 +467,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); Why just not to do a proper reset here? You won't need a custom .remove then. > + usleep_range(2000, 4000); > + Is this sleep after deassertion really needed? > rc = sdhci_add_host(host); > if (rc) > goto err_add_host; > @@ -479,6 +492,29 @@ err_parse_dt: > 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", > @@ -486,7 +522,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); >
On Fri, Aug 19, 2016 at 05:44:32PM +0300, Dmitry Osipenko wrote: > On 19.08.2016 17:00, 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 1e93dc4e303e..4c29a64721aa 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) > > @@ -464,6 +467,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); > > Why just not to do a proper reset here? You won't need a custom .remove then. We could do a proper reset here, although the Tegra CAR driver currently doesn't implement it. I'm also not sure whether we can implement it safely because not all resets use the same sequence. That said, we could do a manual assert/sleep/deassert cycle here, just to be on the safe side. Also, the custom ->remove() implementation is still necessary to make sure that the device is held in reset after the driver's unloaded. It isn't strictly necessary to do that because disabling the clock might be enough to stop the hardware block from consuming power in most cases, but I think it's good practice to do so anyway, if for nothing else than force the next driver to deassert the reset and hence start from pristine hardware state. > > + usleep_range(2000, 4000); > > + > > Is this sleep after deassertion really needed? Yeah, I think it is. There are various bits of documentation that suggest that a delay of at least a few microseconds is needed. 2 to 4 milliseconds is probably a little excessive, but this isn't in a time critical path anyway, and the large sleep avoids any potential subtle timing issue. It's also in line with what other drivers do. Thierry
On 20.08.2016 13:29, Thierry Reding wrote: > On Fri, Aug 19, 2016 at 05:44:32PM +0300, Dmitry Osipenko wrote: >> On 19.08.2016 17:00, 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 1e93dc4e303e..4c29a64721aa 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) >>> @@ -464,6 +467,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); >> >> Why just not to do a proper reset here? You won't need a custom .remove then. > > We could do a proper reset here, although the Tegra CAR driver currently > doesn't implement it. I'm also not sure whether we can implement it > safely because not all resets use the same sequence. > > That said, we could do a manual assert/sleep/deassert cycle here, just > to be on the safe side. > > Also, the custom ->remove() implementation is still necessary to make > sure that the device is held in reset after the driver's unloaded. It > isn't strictly necessary to do that because disabling the clock might > be enough to stop the hardware block from consuming power in most > cases, but I think it's good practice to do so anyway, if for nothing > else than force the next driver to deassert the reset and hence start > from pristine hardware state. > Couldn't that make more bad than good, given that it's not a proper reset sequence? >>> + usleep_range(2000, 4000); >>> + >> >> Is this sleep after deassertion really needed? > > Yeah, I think it is. There are various bits of documentation that > suggest that a delay of at least a few microseconds is needed. 2 to 4 > milliseconds is probably a little excessive, but this isn't in a time > critical path anyway, and the large sleep avoids any potential subtle > timing issue. It's also in line with what other drivers do. > Okay > Thierry >
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 1e93dc4e303e..4c29a64721aa 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) @@ -464,6 +467,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; @@ -479,6 +492,29 @@ err_parse_dt: 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", @@ -486,7 +522,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);