From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Date: Wed, 20 Jun 2012 06:58:54 +0000 Subject: Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management Message-Id: <20120620065854.GM20030@verge.net.au> List-Id: References: <20120620051857.GF20030@verge.net.au> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guennadi Liakhovetski Cc: linux-mmc@vger.kernel.org, linux-sh@vger.kernel.org, Magnus Damm , Chris Ball , Mark Brown 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 > > > --- > > > 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