On Fri, Jun 15, 2018 at 07:45:03AM -0700, Kevin Hilman wrote:
> On Fri, Jun 15, 2018 at 1:56 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >
> > 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".
>
> I added clk_ignore_unused to the kernel command-line, and that didn't
> help, so it's not just an init-time clock that's causing the problem.
>
> > 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().
>
> I tried that and it makes the kernel finish booting, so that smells
> definitely like the MMC is disabling a clock when it goes idle that
> some other device (or CPU) depends on.
I've looked into it, and I can't seem to reproduce it on a 4.18-rc2
kernel with my cubieboard2, booting from NFS and trying to access the
SD card.
Marc is booting from SATA, but apparently your boards Kevin are
booting from initramfs?
Apart from the ftrace option, can you add a call in the probe to
clk_prepare_enable for clk_ahb and clk_mmc clocks, and see if there's
one that fixes the issue? the output and sample clocks are not
gatable, so it shouldn't cause any trouble.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
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
