On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Hi Maxime,
>
> On 16/04/18 15:23, Maxime Ripard wrote:
>> So far, even if our card was not in use, we didn't shut down our MMC
>> controller, which meant that it was still active and clocking the bus.
>>
>> While this obviously means that we could save some power there, it also
>> creates issues when it comes to EMC control since we'll have a perfect peak
>> at the card clock rate.
>>
>> Let's implement runtime_pm with autosuspend so that we will shut down the
>> controller when it's not been in use for quite some time.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
>> ---
>> drivers/mmc/host/sunxi-mmc.c | 48 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 0165da0d022a..0253deb153a4 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -35,6 +35,7 @@
>> #include <linux/of_gpio.h>
>> #include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> #include <linux/scatterlist.h>
>> @@ -969,6 +970,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> unsigned long flags;
>> u32 imask;
>>
>> + if (enable)
>> + pm_runtime_get_noresume(host->dev);
>> +
>> spin_lock_irqsave(&host->lock, flags);
>>
>> imask = mmc_readl(host, REG_IMASK);
>> @@ -981,6 +985,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> }
>> mmc_writel(host, REG_IMASK, imask);
>> spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + if (!enable)
>> + pm_runtime_put_noidle(host->mmc->parent);
>> }
>>
>> static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
>> @@ -1394,6 +1401,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>> if (ret)
>> goto error_free_dma;
>>
>> + pm_runtime_set_active(&pdev->dev);
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> +
>> ret = mmc_add_host(mmc);
>> if (ret)
>> goto error_free_dma;
>> @@ -1414,6 +1426,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> struct sunxi_mmc_host *host = mmc_priv(mmc);
>>
>> mmc_remove_host(mmc);
>> + pm_runtime_force_suspend(&pdev->dev);
>> disable_irq(host->irq);
>> sunxi_mmc_disable(host);
>> dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
>> @@ -1422,10 +1435,45 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int sunxi_mmc_runtime_resume(struct device *dev)
>> +{
>> + struct mmc_host *mmc = dev_get_drvdata(dev);
>> + struct sunxi_mmc_host *host = mmc_priv(mmc);
>> + int ret;
>> +
>> + ret = sunxi_mmc_enable(host);
>> + if (ret)
>> + return ret;
>> +
>> + sunxi_mmc_init_host(host);
>> + sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
>> + sunxi_mmc_set_clk(host, &mmc->ios);
>> +
>> + return 0;
>> +}
>> +
>> +static int sunxi_mmc_runtime_suspend(struct device *dev)
>> +{
>> + struct mmc_host *mmc = dev_get_drvdata(dev);
>> + struct sunxi_mmc_host *host = mmc_priv(mmc);
>> +
>> + sunxi_mmc_reset_host(host);
>> + sunxi_mmc_disable(host);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops sunxi_mmc_pm_ops = {
>> + SET_RUNTIME_PM_OPS(sunxi_mmc_runtime_suspend,
>> + sunxi_mmc_runtime_resume,
>> + NULL)
>> +};
>> +
>> static struct platform_driver sunxi_mmc_driver = {
>> .driver = {
>> .name = "sunxi-mmc",
>> .of_match_table = of_match_ptr(sunxi_mmc_of_match),
>> + .pm = &sunxi_mmc_pm_ops,
>> },
>> .probe = sunxi_mmc_probe,
>> .remove = sunxi_mmc_remove,
>>
>
> This patch has the unfortunate impact of killing my A20 system
> (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
>
> [...]
> [ 3.286649] NET: Registered protocol family 10
> [ 3.291898] mmcblk0: p1
> [ 3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> [ 3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [ 3.305787] Segment Routing with IPv6
> [ 3.312225] mip6: Mobile IPv6
> [ 3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [ 3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [ 3.323721] ip6_gre: GRE over IPv6 tunneling driver
> [ 3.333954] NET: Registered protocol family 17
> [ 3.338837] 9pnet: Installing 9P2000 support
> [ 3.343379] NET: Registered protocol family 37
> [ 3.347885] Key type dns_resolver registered
> [ 3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> [ 3.352217] openvswitch: Open vSwitch switching datapath
> [ 3.352620] mpls_gso: MPLS GSO support
> [ 3.367001] ThumbEE CPU extension supported.
>
> and that's where it stops. No message, just a hard lockup (I suppose
> that both the CPUs are trying to access some device that is now not
> clocked).
Seems like a reasonable analyze.
>
> With a working kernel, I see SATA and the wifi SDIO being probed.
>
> Happy to help testing stuff if you have any idea.
In principle I would start with avoiding having the sunxi-mmc driver
to probe. Or bail out early in probe, whichever is the easiest for
you.
The point is, if the sunxi-mmc driver doesn't even enable its clock,
it would be interesting to see if there are other that depends on it.
One could also play with clk_disable_unused(), the
late_initcall_sync(), which can be turned off with the module
parameter "clk_ignore_unused".
Anyway, to hide/fix the problem for now, we could add a call to
pm_runtime_get_noresume() before the sunxi-driver calls
pm_runtime_enable().
Kind regards
Uffe
--
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
