| Message ID | 1347613731-29428-2-git-send-email-chander.kashyap@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
Hi Chander, Could you add the error control for clk_enable()? On 09/14/2012 06:08 PM, Chander Kashyap wrote: > In case of multiple bus clock sources, all the clock sources were > getting enabled. As only one clock source is needed at the time hence > enable only the required bus clock. > > This patch does as follows: > 1. In sdhci_s3c_probe enable only required bus clock source. > > 2. Handle the disabling of old bus clock and enables the > best clock selected in sdhci_s3c_set_clock(). > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > drivers/mmc/host/sdhci-s3c.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c > index 00969ba..0cbb4c2 100644 > --- a/drivers/mmc/host/sdhci-s3c.c > +++ b/drivers/mmc/host/sdhci-s3c.c > @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock) > best_src, clock, best); > > /* select the new clock source */ > - > if (ourhost->cur_clk != best_src) { > struct clk *clk = ourhost->clk_bus[best_src]; > > + clk_enable(clk); > + clk_disable(ourhost->clk_bus[ourhost->cur_clk]); > + Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()? > /* turn clock off to card before changing clock source */ > writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL); > > @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > */ > sc->cur_clk = ptr; > > - clk_enable(clk); > - > dev_info(dev, "clock source %d: %s (%ld Hz)\n", > ptr, name, clk_get_rate(clk)); > } > @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > goto err_no_busclks; > } > > + clk_enable(sc->clk_bus[sc->cur_clk]); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > host->ioaddr = devm_request_and_ioremap(&pdev->dev, res); > if (!host->ioaddr) { > @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > return 0; > > err_req_regs: > + clk_disable(sc->clk_bus[sc->cur_clk]); > for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) { > if (sc->clk_bus[ptr]) { > - clk_disable(sc->clk_bus[ptr]); > clk_put(sc->clk_bus[ptr]); > } > } > @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > + clk_disable(sc->clk_bus[sc->cur_clk]); > for (ptr = 0; ptr < 3; ptr++) { > if (sc->clk_bus[ptr]) { > - clk_disable(sc->clk_bus[ptr]); > clk_put(sc->clk_bus[ptr]); > } > } >
looks good Reviewed by: Girish KS <girish.shivananjappa@linaro.org> On 14 September 2012 14:38, Chander Kashyap <chander.kashyap@linaro.org> wrote: > In case of multiple bus clock sources, all the clock sources were > getting enabled. As only one clock source is needed at the time hence > enable only the required bus clock. > > This patch does as follows: > 1. In sdhci_s3c_probe enable only required bus clock source. > > 2. Handle the disabling of old bus clock and enables the > best clock selected in sdhci_s3c_set_clock(). > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > drivers/mmc/host/sdhci-s3c.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c > index 00969ba..0cbb4c2 100644 > --- a/drivers/mmc/host/sdhci-s3c.c > +++ b/drivers/mmc/host/sdhci-s3c.c > @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock) > best_src, clock, best); > > /* select the new clock source */ > - > if (ourhost->cur_clk != best_src) { > struct clk *clk = ourhost->clk_bus[best_src]; > > + clk_enable(clk); > + clk_disable(ourhost->clk_bus[ourhost->cur_clk]); > + > /* turn clock off to card before changing clock source */ > writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL); > > @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > */ > sc->cur_clk = ptr; > > - clk_enable(clk); > - > dev_info(dev, "clock source %d: %s (%ld Hz)\n", > ptr, name, clk_get_rate(clk)); > } > @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > goto err_no_busclks; > } > > + clk_enable(sc->clk_bus[sc->cur_clk]); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > host->ioaddr = devm_request_and_ioremap(&pdev->dev, res); > if (!host->ioaddr) { > @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > return 0; > > err_req_regs: > + clk_disable(sc->clk_bus[sc->cur_clk]); > for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) { > if (sc->clk_bus[ptr]) { > - clk_disable(sc->clk_bus[ptr]); > clk_put(sc->clk_bus[ptr]); > } > } > @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > + clk_disable(sc->clk_bus[sc->cur_clk]); > for (ptr = 0; ptr < 3; ptr++) { > if (sc->clk_bus[ptr]) { > - clk_disable(sc->clk_bus[ptr]); > clk_put(sc->clk_bus[ptr]); > } > } > -- > 1.7.9.5 >
Hi Jaehoon Chung, On 14 September 2012 15:25, Jaehoon Chung <jh80.chung@samsung.com> wrote: > Hi Chander, > > Could you add the error control for clk_enable()? It is maintained as original and clk is already checked for error using IS_ERR. > > On 09/14/2012 06:08 PM, Chander Kashyap wrote: >> In case of multiple bus clock sources, all the clock sources were >> getting enabled. As only one clock source is needed at the time hence >> enable only the required bus clock. >> >> This patch does as follows: >> 1. In sdhci_s3c_probe enable only required bus clock source. >> >> 2. Handle the disabling of old bus clock and enables the >> best clock selected in sdhci_s3c_set_clock(). >> >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> >> --- >> drivers/mmc/host/sdhci-s3c.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c >> index 00969ba..0cbb4c2 100644 >> --- a/drivers/mmc/host/sdhci-s3c.c >> +++ b/drivers/mmc/host/sdhci-s3c.c >> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock) >> best_src, clock, best); >> >> /* select the new clock source */ >> - >> if (ourhost->cur_clk != best_src) { >> struct clk *clk = ourhost->clk_bus[best_src]; >> >> + clk_enable(clk); >> + clk_disable(ourhost->clk_bus[ourhost->cur_clk]); >> + > Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()? Yes considered. > >> /* turn clock off to card before changing clock source */ >> writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL); >> >> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> */ >> sc->cur_clk = ptr; >> >> - clk_enable(clk); >> - >> dev_info(dev, "clock source %d: %s (%ld Hz)\n", >> ptr, name, clk_get_rate(clk)); >> } >> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> goto err_no_busclks; >> } >> >> + clk_enable(sc->clk_bus[sc->cur_clk]); >> + >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> host->ioaddr = devm_request_and_ioremap(&pdev->dev, res); >> if (!host->ioaddr) { >> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> return 0; >> >> err_req_regs: >> + clk_disable(sc->clk_bus[sc->cur_clk]); >> for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) { >> if (sc->clk_bus[ptr]) { >> - clk_disable(sc->clk_bus[ptr]); >> clk_put(sc->clk_bus[ptr]); >> } >> } >> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev) >> >> pm_runtime_disable(&pdev->dev); >> >> + clk_disable(sc->clk_bus[sc->cur_clk]); >> for (ptr = 0; ptr < 3; ptr++) { >> if (sc->clk_bus[ptr]) { >> - clk_disable(sc->clk_bus[ptr]); >> clk_put(sc->clk_bus[ptr]); >> } >> } >> >
Acked-by: Jaehoon Chung <jh80.chung@samsung.com> On 09/14/2012 07:46 PM, Chander Kashyap wrote: > Hi Jaehoon Chung, > > On 14 September 2012 15:25, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> Hi Chander, >> >> Could you add the error control for clk_enable()? > It is maintained as original and clk is already checked for error using IS_ERR. >> >> On 09/14/2012 06:08 PM, Chander Kashyap wrote: >>> In case of multiple bus clock sources, all the clock sources were >>> getting enabled. As only one clock source is needed at the time hence >>> enable only the required bus clock. >>> >>> This patch does as follows: >>> 1. In sdhci_s3c_probe enable only required bus clock source. >>> >>> 2. Handle the disabling of old bus clock and enables the >>> best clock selected in sdhci_s3c_set_clock(). >>> >>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> >>> --- >>> drivers/mmc/host/sdhci-s3c.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c >>> index 00969ba..0cbb4c2 100644 >>> --- a/drivers/mmc/host/sdhci-s3c.c >>> +++ b/drivers/mmc/host/sdhci-s3c.c >>> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock) >>> best_src, clock, best); >>> >>> /* select the new clock source */ >>> - >>> if (ourhost->cur_clk != best_src) { >>> struct clk *clk = ourhost->clk_bus[best_src]; >>> >>> + clk_enable(clk); >>> + clk_disable(ourhost->clk_bus[ourhost->cur_clk]); >>> + >> Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()? > Yes considered. >> >>> /* turn clock off to card before changing clock source */ >>> writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL); >>> >>> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >>> */ >>> sc->cur_clk = ptr; >>> >>> - clk_enable(clk); >>> - >>> dev_info(dev, "clock source %d: %s (%ld Hz)\n", >>> ptr, name, clk_get_rate(clk)); >>> } >>> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >>> goto err_no_busclks; >>> } >>> >>> + clk_enable(sc->clk_bus[sc->cur_clk]); >>> + >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> host->ioaddr = devm_request_and_ioremap(&pdev->dev, res); >>> if (!host->ioaddr) { >>> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >>> return 0; >>> >>> err_req_regs: >>> + clk_disable(sc->clk_bus[sc->cur_clk]); >>> for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) { >>> if (sc->clk_bus[ptr]) { >>> - clk_disable(sc->clk_bus[ptr]); >>> clk_put(sc->clk_bus[ptr]); >>> } >>> } >>> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev) >>> >>> pm_runtime_disable(&pdev->dev); >>> >>> + clk_disable(sc->clk_bus[sc->cur_clk]); >>> for (ptr = 0; ptr < 3; ptr++) { >>> if (sc->clk_bus[ptr]) { >>> - clk_disable(sc->clk_bus[ptr]); >>> clk_put(sc->clk_bus[ptr]); >>> } >>> } >>> >> > > >
Hi, On Fri, Sep 14 2012, Chander Kashyap wrote: > In case of multiple bus clock sources, all the clock sources were > getting enabled. As only one clock source is needed at the time hence > enable only the required bus clock. > > This patch does as follows: > 1. In sdhci_s3c_probe enable only required bus clock source. > > 2. Handle the disabling of old bus clock and enables the > best clock selected in sdhci_s3c_set_clock(). > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> Thanks, pushed to mmc-next for 3.7. - Chris.
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c index 00969ba..0cbb4c2 100644 --- a/drivers/mmc/host/sdhci-s3c.c +++ b/drivers/mmc/host/sdhci-s3c.c @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock) best_src, clock, best); /* select the new clock source */ - if (ourhost->cur_clk != best_src) { struct clk *clk = ourhost->clk_bus[best_src]; + clk_enable(clk); + clk_disable(ourhost->clk_bus[ourhost->cur_clk]); + /* turn clock off to card before changing clock source */ writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL); @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) */ sc->cur_clk = ptr; - clk_enable(clk); - dev_info(dev, "clock source %d: %s (%ld Hz)\n", ptr, name, clk_get_rate(clk)); } @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) goto err_no_busclks; } + clk_enable(sc->clk_bus[sc->cur_clk]); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); host->ioaddr = devm_request_and_ioremap(&pdev->dev, res); if (!host->ioaddr) { @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) return 0; err_req_regs: + clk_disable(sc->clk_bus[sc->cur_clk]); for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) { if (sc->clk_bus[ptr]) { - clk_disable(sc->clk_bus[ptr]); clk_put(sc->clk_bus[ptr]); } } @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); + clk_disable(sc->clk_bus[sc->cur_clk]); for (ptr = 0; ptr < 3; ptr++) { if (sc->clk_bus[ptr]) { - clk_disable(sc->clk_bus[ptr]); clk_put(sc->clk_bus[ptr]); } }
In case of multiple bus clock sources, all the clock sources were getting enabled. As only one clock source is needed at the time hence enable only the required bus clock. This patch does as follows: 1. In sdhci_s3c_probe enable only required bus clock source. 2. Handle the disabling of old bus clock and enables the best clock selected in sdhci_s3c_set_clock(). Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> --- drivers/mmc/host/sdhci-s3c.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)