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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
