On Wed, Jun 20, 2012 at 08:41:42AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 20 Jun 2012, Simon Horman wrote:
>
> > Hi Guennadi,
> >
> > On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> > > Regardless, whether the MMC bus clock is the same, as the PM clock on this
> > > specific interface, it has to be managed separately. Its proper management
> > > should also include enabling and disabling of the clock, whenever the
> > > interface is becoming active or going idle respectively.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > > ---
> > > drivers/mmc/host/sh_mmcif.c | 46 +++++++++++++++++++++---------------------
> > > 1 files changed, 23 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > > index d6ffb05..6a93b04 100644
> > > --- a/drivers/mmc/host/sh_mmcif.c
> > > +++ b/drivers/mmc/host/sh_mmcif.c
> > > @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > > }
> > > if (host->power) {
> > > pm_runtime_put(&host->pd->dev);
> > > + clk_disable(host->hclk);
> > > host->power = false;
> > > if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
> > > p->down_pwr(host->pd);
> > > @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > > if (!host->power) {
> > > if (p->set_pwr)
> > > p->set_pwr(host->pd, ios->power_mode);
> > > + clk_enable(host->hclk);
> > > pm_runtime_get_sync(&host->pd->dev);
> > > host->power = true;
> > > sh_mmcif_sync_reset(host);
> > > @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > > host->addr = reg;
> > > host->timeout = 1000;
> > >
> > > - snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > > - host->hclk = clk_get(&pdev->dev, clk_name);
> > > - if (IS_ERR(host->hclk)) {
> > > - dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> > > - ret = PTR_ERR(host->hclk);
> > > - goto eclkget;
> > > - }
> > > - clk_enable(host->hclk);
> > > - host->clk = clk_get_rate(host->hclk);
> > > host->pd = pdev;
> > >
> > > spin_lock_init(&host->lock);
> > >
> > > mmc->ops = &sh_mmcif_ops;
> > > - mmc->f_max = host->clk / 2;
> > > - mmc->f_min = host->clk / 512;
> > > if (pd->ocr)
> > > mmc->ocr_avail = pd->ocr;
> > > mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> > > @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > > mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
> > > mmc->max_seg_size = mmc->max_req_size;
> > >
> > > - sh_mmcif_sync_reset(host);
> > > platform_set_drvdata(pdev, host);
> > >
> > > pm_runtime_enable(&pdev->dev);
> > > host->power = false;
> > >
> > > + snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > > + host->hclk = clk_get(&pdev->dev, clk_name);
> > > + if (IS_ERR(host->hclk)) {
> > > + ret = PTR_ERR(host->hclk);
> > > + dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> > > + goto eclkget;
> > > + }
> > > + clk_enable(host->hclk);
> > > + host->clk = clk_get_rate(host->hclk);
> > > + mmc->f_max = host->clk / 2;
> > > + mmc->f_min = host->clk / 512;
> > > +
> > > ret = pm_runtime_resume(&pdev->dev);
> > > if (ret < 0)
> > > goto eresume;
> > >
> > > INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
> > >
> > > + sh_mmcif_sync_reset(host);
> > > sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
> > >
> > > ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> > > @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > > goto ereqirq1;
> > > }
> > >
> > > + clk_disable(host->hclk);
> > > ret = mmc_add_host(mmc);
> > > if (ret < 0)
> > > goto emmcaddh;
> >
> > Could you clarify the purpose of two hunks above.
> > They seem to move code down in sh_mmcif_probe().
> > Is this related to the call to pm_runtime_enable() ?
>
> The sh_mmcif_sync_reset() call accesses the hardware so it only makes
> sense after the hardware has been resumed. Moving the clk_enable() call
> down improves consistency by grouping clock and runtime PM code together
> during probe() similar to other paths.
Thanks.
Reviewed-by: Simon Horman <horms@xxxxxxxxxxxx>
--
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
