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@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.
>
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
>
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
