| Message ID | 1471581384-18961-8-git-send-email-riteshh@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 19/08/16 07:36, Ritesh Harjani wrote: > sdhci-msm controller may have different clk-rates for each > bus speed mode. Thus implement set_clock callback for > sdhci-msm driver. > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> > Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> > --- > drivers/mmc/host/sdhci-msm.c | 103 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 102 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 7c032c3..c0ad9c2 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -89,6 +89,7 @@ struct sdhci_msm_host { > struct mmc_host *mmc; > bool use_14lpp_dll_reset; > struct sdhci_msm_pltfm_data *pdata; > + u32 clk_rate; > }; > > /* Platform specific tuning */ > @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host) > return msm_host->pdata->clk_table[0]; > } > > +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, > + u32 req_clk) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int count = msm_host->pdata->clk_table_sz; > + unsigned int sel_clk = -1; > + int cnt; > + > + if (req_clk < sdhci_msm_get_min_clock(host)) { > + sel_clk = sdhci_msm_get_min_clock(host); > + return sel_clk; > + } > + > + for (cnt = 0; cnt < count; cnt++) { > + if (msm_host->pdata->clk_table[cnt] > req_clk) { > + break; > + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { > + sel_clk = msm_host->pdata->clk_table[cnt]; > + break; > + } else { > + sel_clk = msm_host->pdata->clk_table[cnt]; > + } > + } 'else' is not needed after 'break' but can't this be simpler e.g. static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host *msm_host, u32 req_clk) { int count = msm_host->pdata->clk_table_sz; unsigned int sel_clk = -1; while (count--) { sel_clk = msm_host->pdata->clk_table[count]; if (req_clk >= sel_clk) return sel_clk; } return sel_clk; } > + return sel_clk; > +} Blank line needed > +/** > + * __sdhci_msm_set_clock - sdhci_msm clock control. > + * > + * Description: > + * Implement MSM version of sdhci_set_clock. > + * This is required since MSM controller does not > + * use internal divider and instead directly control > + * the GCC clock as per HW recommendation. > + **/ > +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) > +{ > + u16 clk; > + unsigned long timeout; > + > + host->mmc->actual_clock = 0; > + > + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > + > + if (clock == 0) > + return; Should set host->mmc->actual_clock to the actual rate somewhere. > + > + /* > + * MSM controller do not use clock divider. > + * Thus read SDHCI_CLOCK_CONTROL and only enable > + * clock with no divider value programmed. > + */ > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > + > + clk |= SDHCI_CLOCK_INT_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > + > + /* Wait max 20 ms */ > + timeout = 20; > + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) > + & SDHCI_CLOCK_INT_STABLE)) { > + if (timeout == 0) { > + pr_err("%s: Internal clock never stabilised.\n", > + mmc_hostname(host->mmc)); > + return; > + } > + timeout--; > + mdelay(1); > + } > + > + clk |= SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > +} > + > +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + u32 msm_clock = 0; > + int rc = 0; > + > + if (!clock) > + goto out; > + > + if (clock != msm_host->clk_rate) { > + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); > + rc = clk_set_rate(msm_host->clk, msm_clock); > + if (rc) { > + pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n", > + mmc_hostname(host->mmc), msm_clock, clock); > + goto out; > + } > + msm_host->clk_rate = clock; > + pr_debug("%s: setting clock at rate %lu\n", > + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); > + } > +out: > + __sdhci_msm_set_clock(host, clock); > +} > + > static const struct of_device_id sdhci_msm_dt_match[] = { > { .compatible = "qcom,sdhci-msm-v4" }, > {}, > @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); > static const struct sdhci_ops sdhci_msm_ops = { > .platform_execute_tuning = sdhci_msm_execute_tuning, > .reset = sdhci_reset, > - .set_clock = sdhci_set_clock, > + .set_clock = sdhci_msm_set_clock, > .get_min_clock = sdhci_msm_get_min_clock, > .get_max_clock = sdhci_msm_get_max_clock, > .set_bus_width = sdhci_set_bus_width, > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 8/19/2016 6:34 PM, Adrian Hunter wrote: > On 19/08/16 07:36, Ritesh Harjani wrote: >> sdhci-msm controller may have different clk-rates for each >> bus speed mode. Thus implement set_clock callback for >> sdhci-msm driver. >> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >> --- >> drivers/mmc/host/sdhci-msm.c | 103 ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 102 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> index 7c032c3..c0ad9c2 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >> struct mmc_host *mmc; >> bool use_14lpp_dll_reset; >> struct sdhci_msm_pltfm_data *pdata; >> + u32 clk_rate; >> }; >> >> /* Platform specific tuning */ >> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host) >> return msm_host->pdata->clk_table[0]; >> } >> >> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >> + u32 req_clk) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >> + int count = msm_host->pdata->clk_table_sz; >> + unsigned int sel_clk = -1; >> + int cnt; >> + >> + if (req_clk < sdhci_msm_get_min_clock(host)) { >> + sel_clk = sdhci_msm_get_min_clock(host); >> + return sel_clk; >> + } >> + >> + for (cnt = 0; cnt < count; cnt++) { >> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >> + break; >> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >> + sel_clk = msm_host->pdata->clk_table[cnt]; >> + break; >> + } else { >> + sel_clk = msm_host->pdata->clk_table[cnt]; >> + } >> + } > > 'else' is not needed after 'break' but can't this be simpler e.g. > > static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host *msm_host, u32 req_clk) > { > int count = msm_host->pdata->clk_table_sz; > unsigned int sel_clk = -1; > > while (count--) { > sel_clk = msm_host->pdata->clk_table[count]; > if (req_clk >= sel_clk) > return sel_clk; > } > > return sel_clk; > } Ok, sure I will check and get back on this. > > >> + return sel_clk; >> +} > > Blank line needed Ok done. > >> +/** >> + * __sdhci_msm_set_clock - sdhci_msm clock control. >> + * >> + * Description: >> + * Implement MSM version of sdhci_set_clock. >> + * This is required since MSM controller does not >> + * use internal divider and instead directly control >> + * the GCC clock as per HW recommendation. >> + **/ >> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >> +{ >> + u16 clk; >> + unsigned long timeout; >> + >> + host->mmc->actual_clock = 0; >> + >> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >> + >> + if (clock == 0) >> + return; > > Should set host->mmc->actual_clock to the actual rate somewhere. Since MSM controller does not uses divider then there is no need of having actual clock since it should be same as host->clock. That's why I had kept it 0 intentionally. But I will add a comment here then. > >> + >> + /* >> + * MSM controller do not use clock divider. >> + * Thus read SDHCI_CLOCK_CONTROL and only enable >> + * clock with no divider value programmed. >> + */ >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >> + >> + clk |= SDHCI_CLOCK_INT_EN; >> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >> + >> + /* Wait max 20 ms */ >> + timeout = 20; >> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >> + & SDHCI_CLOCK_INT_STABLE)) { >> + if (timeout == 0) { >> + pr_err("%s: Internal clock never stabilised.\n", >> + mmc_hostname(host->mmc)); >> + return; >> + } >> + timeout--; >> + mdelay(1); >> + } >> + >> + clk |= SDHCI_CLOCK_CARD_EN; >> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >> +} >> + >> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >> + u32 msm_clock = 0; >> + int rc = 0; >> + >> + if (!clock) >> + goto out; >> + >> + if (clock != msm_host->clk_rate) { >> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >> + rc = clk_set_rate(msm_host->clk, msm_clock); >> + if (rc) { >> + pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n", >> + mmc_hostname(host->mmc), msm_clock, clock); >> + goto out; >> + } >> + msm_host->clk_rate = clock; >> + pr_debug("%s: setting clock at rate %lu\n", >> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >> + } >> +out: >> + __sdhci_msm_set_clock(host, clock); >> +} >> + >> static const struct of_device_id sdhci_msm_dt_match[] = { >> { .compatible = "qcom,sdhci-msm-v4" }, >> {}, >> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >> static const struct sdhci_ops sdhci_msm_ops = { >> .platform_execute_tuning = sdhci_msm_execute_tuning, >> .reset = sdhci_reset, >> - .set_clock = sdhci_set_clock, >> + .set_clock = sdhci_msm_set_clock, >> .get_min_clock = sdhci_msm_get_min_clock, >> .get_max_clock = sdhci_msm_get_max_clock, >> .set_bus_width = sdhci_set_bus_width, >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/08/16 16:31, Ritesh Harjani wrote: > Hi, > > > On 8/19/2016 6:34 PM, Adrian Hunter wrote: >> On 19/08/16 07:36, Ritesh Harjani wrote: >>> sdhci-msm controller may have different clk-rates for each >>> bus speed mode. Thus implement set_clock callback for >>> sdhci-msm driver. >>> >>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>> --- >>> drivers/mmc/host/sdhci-msm.c | 103 >>> ++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 102 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>> index 7c032c3..c0ad9c2 100644 >>> --- a/drivers/mmc/host/sdhci-msm.c >>> +++ b/drivers/mmc/host/sdhci-msm.c >>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>> struct mmc_host *mmc; >>> bool use_14lpp_dll_reset; >>> struct sdhci_msm_pltfm_data *pdata; >>> + u32 clk_rate; >>> }; >>> >>> /* Platform specific tuning */ >>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>> sdhci_host *host) >>> return msm_host->pdata->clk_table[0]; >>> } >>> >>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>> + u32 req_clk) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>> + int count = msm_host->pdata->clk_table_sz; >>> + unsigned int sel_clk = -1; >>> + int cnt; >>> + >>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>> + sel_clk = sdhci_msm_get_min_clock(host); >>> + return sel_clk; >>> + } >>> + >>> + for (cnt = 0; cnt < count; cnt++) { >>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>> + break; >>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>> + break; >>> + } else { >>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>> + } >>> + } >> >> 'else' is not needed after 'break' but can't this be simpler e.g. >> >> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >> *msm_host, u32 req_clk) >> { >> int count = msm_host->pdata->clk_table_sz; >> unsigned int sel_clk = -1; >> >> while (count--) { >> sel_clk = msm_host->pdata->clk_table[count]; >> if (req_clk >= sel_clk) >> return sel_clk; >> } >> >> return sel_clk; >> } > > Ok, sure I will check and get back on this. > > >> >> >>> + return sel_clk; >>> +} >> >> Blank line needed > Ok done. > >> >>> +/** >>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>> + * >>> + * Description: >>> + * Implement MSM version of sdhci_set_clock. >>> + * This is required since MSM controller does not >>> + * use internal divider and instead directly control >>> + * the GCC clock as per HW recommendation. >>> + **/ >>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>> +{ >>> + u16 clk; >>> + unsigned long timeout; >>> + >>> + host->mmc->actual_clock = 0; >>> + >>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>> + >>> + if (clock == 0) >>> + return; >> >> Should set host->mmc->actual_clock to the actual rate somewhere. > Since MSM controller does not uses divider then there is no need of having > actual clock since it should be same as host->clock. Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? > > That's why I had kept it 0 intentionally. But I will add a comment > here then. > >> >>> + >>> + /* >>> + * MSM controller do not use clock divider. >>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>> + * clock with no divider value programmed. >>> + */ >>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>> + >>> + clk |= SDHCI_CLOCK_INT_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>> + >>> + /* Wait max 20 ms */ >>> + timeout = 20; >>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>> + & SDHCI_CLOCK_INT_STABLE)) { >>> + if (timeout == 0) { >>> + pr_err("%s: Internal clock never stabilised.\n", >>> + mmc_hostname(host->mmc)); >>> + return; >>> + } >>> + timeout--; >>> + mdelay(1); >>> + } >>> + >>> + clk |= SDHCI_CLOCK_CARD_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>> +} >>> + >>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>> clock) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>> + u32 msm_clock = 0; >>> + int rc = 0; >>> + >>> + if (!clock) >>> + goto out; >>> + >>> + if (clock != msm_host->clk_rate) { >>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>> + if (rc) { >>> + pr_err("%s: failed to set clock at rate %u, requested clock >>> rate %u\n", >>> + mmc_hostname(host->mmc), msm_clock, clock); >>> + goto out; >>> + } >>> + msm_host->clk_rate = clock; >>> + pr_debug("%s: setting clock at rate %lu\n", >>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>> + } >>> +out: >>> + __sdhci_msm_set_clock(host, clock); >>> +} >>> + >>> static const struct of_device_id sdhci_msm_dt_match[] = { >>> { .compatible = "qcom,sdhci-msm-v4" }, >>> {}, >>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>> static const struct sdhci_ops sdhci_msm_ops = { >>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>> .reset = sdhci_reset, >>> - .set_clock = sdhci_set_clock, >>> + .set_clock = sdhci_msm_set_clock, >>> .get_min_clock = sdhci_msm_get_min_clock, >>> .get_max_clock = sdhci_msm_get_max_clock, >>> .set_bus_width = sdhci_set_bus_width, >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On 8/22/2016 11:50 AM, Adrian Hunter wrote: > On 19/08/16 16:31, Ritesh Harjani wrote: >> Hi, >> >> >> On 8/19/2016 6:34 PM, Adrian Hunter wrote: >>> On 19/08/16 07:36, Ritesh Harjani wrote: >>>> sdhci-msm controller may have different clk-rates for each >>>> bus speed mode. Thus implement set_clock callback for >>>> sdhci-msm driver. >>>> >>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>> --- >>>> drivers/mmc/host/sdhci-msm.c | 103 >>>> ++++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 102 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>> index 7c032c3..c0ad9c2 100644 >>>> --- a/drivers/mmc/host/sdhci-msm.c >>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>>> struct mmc_host *mmc; >>>> bool use_14lpp_dll_reset; >>>> struct sdhci_msm_pltfm_data *pdata; >>>> + u32 clk_rate; >>>> }; >>>> >>>> /* Platform specific tuning */ >>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>>> sdhci_host *host) >>>> return msm_host->pdata->clk_table[0]; >>>> } >>>> >>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>>> + u32 req_clk) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>> + int count = msm_host->pdata->clk_table_sz; >>>> + unsigned int sel_clk = -1; >>>> + int cnt; >>>> + >>>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>>> + sel_clk = sdhci_msm_get_min_clock(host); >>>> + return sel_clk; >>>> + } >>>> + >>>> + for (cnt = 0; cnt < count; cnt++) { >>>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>>> + break; >>>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>> + break; >>>> + } else { >>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>> + } >>>> + } >>> >>> 'else' is not needed after 'break' but can't this be simpler e.g. >>> >>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >>> *msm_host, u32 req_clk) >>> { >>> int count = msm_host->pdata->clk_table_sz; >>> unsigned int sel_clk = -1; >>> >>> while (count--) { >>> sel_clk = msm_host->pdata->clk_table[count]; >>> if (req_clk >= sel_clk) >>> return sel_clk; >>> } >>> >>> return sel_clk; >>> } >> >> Ok, sure I will check and get back on this. >> >> >>> >>> >>>> + return sel_clk; >>>> +} >>> >>> Blank line needed >> Ok done. >> >>> >>>> +/** >>>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>>> + * >>>> + * Description: >>>> + * Implement MSM version of sdhci_set_clock. >>>> + * This is required since MSM controller does not >>>> + * use internal divider and instead directly control >>>> + * the GCC clock as per HW recommendation. >>>> + **/ >>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>>> +{ >>>> + u16 clk; >>>> + unsigned long timeout; >>>> + >>>> + host->mmc->actual_clock = 0; >>>> + >>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>>> + >>>> + if (clock == 0) >>>> + return; >>> >>> Should set host->mmc->actual_clock to the actual rate somewhere. >> Since MSM controller does not uses divider then there is no need of having >> actual clock since it should be same as host->clock. > > Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? So I was assuming we need host->mmc->actual_clock rate if host->clock is not the actual rate. Ok, so I will update actual_clock to host->clock itself. pls, let me know if any concerns. > >> >> That's why I had kept it 0 intentionally. But I will add a comment >> here then. >> >>> >>>> + >>>> + /* >>>> + * MSM controller do not use clock divider. >>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>>> + * clock with no divider value programmed. >>>> + */ >>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>> + >>>> + clk |= SDHCI_CLOCK_INT_EN; >>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>> + >>>> + /* Wait max 20 ms */ >>>> + timeout = 20; >>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>>> + & SDHCI_CLOCK_INT_STABLE)) { >>>> + if (timeout == 0) { >>>> + pr_err("%s: Internal clock never stabilised.\n", >>>> + mmc_hostname(host->mmc)); >>>> + return; >>>> + } >>>> + timeout--; >>>> + mdelay(1); >>>> + } >>>> + >>>> + clk |= SDHCI_CLOCK_CARD_EN; >>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>> +} >>>> + >>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>>> clock) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>> + u32 msm_clock = 0; >>>> + int rc = 0; >>>> + >>>> + if (!clock) >>>> + goto out; >>>> + >>>> + if (clock != msm_host->clk_rate) { >>>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>>> + if (rc) { >>>> + pr_err("%s: failed to set clock at rate %u, requested clock >>>> rate %u\n", >>>> + mmc_hostname(host->mmc), msm_clock, clock); >>>> + goto out; >>>> + } >>>> + msm_host->clk_rate = clock; >>>> + pr_debug("%s: setting clock at rate %lu\n", >>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>>> + } >>>> +out: >>>> + __sdhci_msm_set_clock(host, clock); >>>> +} >>>> + >>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>> {}, >>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>>> static const struct sdhci_ops sdhci_msm_ops = { >>>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>>> .reset = sdhci_reset, >>>> - .set_clock = sdhci_set_clock, >>>> + .set_clock = sdhci_msm_set_clock, >>>> .get_min_clock = sdhci_msm_get_min_clock, >>>> .get_max_clock = sdhci_msm_get_max_clock, >>>> .set_bus_width = sdhci_set_bus_width, >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/08/16 12:07, Ritesh Harjani wrote: > Hi Adrian, > > > On 8/22/2016 11:50 AM, Adrian Hunter wrote: >> On 19/08/16 16:31, Ritesh Harjani wrote: >>> Hi, >>> >>> >>> On 8/19/2016 6:34 PM, Adrian Hunter wrote: >>>> On 19/08/16 07:36, Ritesh Harjani wrote: >>>>> sdhci-msm controller may have different clk-rates for each >>>>> bus speed mode. Thus implement set_clock callback for >>>>> sdhci-msm driver. >>>>> >>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>>> --- >>>>> drivers/mmc/host/sdhci-msm.c | 103 >>>>> ++++++++++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 102 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>>> index 7c032c3..c0ad9c2 100644 >>>>> --- a/drivers/mmc/host/sdhci-msm.c >>>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>>>> struct mmc_host *mmc; >>>>> bool use_14lpp_dll_reset; >>>>> struct sdhci_msm_pltfm_data *pdata; >>>>> + u32 clk_rate; >>>>> }; >>>>> >>>>> /* Platform specific tuning */ >>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>>>> sdhci_host *host) >>>>> return msm_host->pdata->clk_table[0]; >>>>> } >>>>> >>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>>>> + u32 req_clk) >>>>> +{ >>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>> + int count = msm_host->pdata->clk_table_sz; >>>>> + unsigned int sel_clk = -1; >>>>> + int cnt; >>>>> + >>>>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>>>> + sel_clk = sdhci_msm_get_min_clock(host); >>>>> + return sel_clk; >>>>> + } >>>>> + >>>>> + for (cnt = 0; cnt < count; cnt++) { >>>>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>>>> + break; >>>>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>> + break; >>>>> + } else { >>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>> + } >>>>> + } >>>> >>>> 'else' is not needed after 'break' but can't this be simpler e.g. >>>> >>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >>>> *msm_host, u32 req_clk) >>>> { >>>> int count = msm_host->pdata->clk_table_sz; >>>> unsigned int sel_clk = -1; >>>> >>>> while (count--) { >>>> sel_clk = msm_host->pdata->clk_table[count]; >>>> if (req_clk >= sel_clk) >>>> return sel_clk; >>>> } >>>> >>>> return sel_clk; >>>> } >>> >>> Ok, sure I will check and get back on this. >>> >>> >>>> >>>> >>>>> + return sel_clk; >>>>> +} >>>> >>>> Blank line needed >>> Ok done. >>> >>>> >>>>> +/** >>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>>>> + * >>>>> + * Description: >>>>> + * Implement MSM version of sdhci_set_clock. >>>>> + * This is required since MSM controller does not >>>>> + * use internal divider and instead directly control >>>>> + * the GCC clock as per HW recommendation. >>>>> + **/ >>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>>>> +{ >>>>> + u16 clk; >>>>> + unsigned long timeout; >>>>> + >>>>> + host->mmc->actual_clock = 0; >>>>> + >>>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>>>> + >>>>> + if (clock == 0) >>>>> + return; >>>> >>>> Should set host->mmc->actual_clock to the actual rate somewhere. >>> Since MSM controller does not uses divider then there is no need of having >>> actual clock since it should be same as host->clock. >> >> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? > So I was assuming we need host->mmc->actual_clock rate if host->clock is not > the actual rate. > Ok, so I will update actual_clock to host->clock itself. > > pls, let me know if any concerns. I am confused. Isn't clk_get_rate(msm_host->clk) the actual clock rate? >> >>> >>> That's why I had kept it 0 intentionally. But I will add a comment >>> here then. >>> >>>> >>>>> + >>>>> + /* >>>>> + * MSM controller do not use clock divider. >>>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>>>> + * clock with no divider value programmed. >>>>> + */ >>>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>>> + >>>>> + clk |= SDHCI_CLOCK_INT_EN; >>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>> + >>>>> + /* Wait max 20 ms */ >>>>> + timeout = 20; >>>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>>>> + & SDHCI_CLOCK_INT_STABLE)) { >>>>> + if (timeout == 0) { >>>>> + pr_err("%s: Internal clock never stabilised.\n", >>>>> + mmc_hostname(host->mmc)); >>>>> + return; >>>>> + } >>>>> + timeout--; >>>>> + mdelay(1); >>>>> + } >>>>> + >>>>> + clk |= SDHCI_CLOCK_CARD_EN; >>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>> +} >>>>> + >>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>>>> clock) >>>>> +{ >>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>> + u32 msm_clock = 0; >>>>> + int rc = 0; >>>>> + >>>>> + if (!clock) >>>>> + goto out; >>>>> + >>>>> + if (clock != msm_host->clk_rate) { >>>>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>>>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>>>> + if (rc) { >>>>> + pr_err("%s: failed to set clock at rate %u, requested clock >>>>> rate %u\n", >>>>> + mmc_hostname(host->mmc), msm_clock, clock); >>>>> + goto out; >>>>> + } >>>>> + msm_host->clk_rate = clock; >>>>> + pr_debug("%s: setting clock at rate %lu\n", >>>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>>>> + } >>>>> +out: >>>>> + __sdhci_msm_set_clock(host, clock); >>>>> +} >>>>> + >>>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>>> {}, >>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>>>> static const struct sdhci_ops sdhci_msm_ops = { >>>>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>>>> .reset = sdhci_reset, >>>>> - .set_clock = sdhci_set_clock, >>>>> + .set_clock = sdhci_msm_set_clock, >>>>> .get_min_clock = sdhci_msm_get_min_clock, >>>>> .get_max_clock = sdhci_msm_get_max_clock, >>>>> .set_bus_width = sdhci_set_bus_width, >>>>> >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On 8/22/2016 2:59 PM, Adrian Hunter wrote: > On 22/08/16 12:07, Ritesh Harjani wrote: >> Hi Adrian, >> >> >> On 8/22/2016 11:50 AM, Adrian Hunter wrote: >>> On 19/08/16 16:31, Ritesh Harjani wrote: >>>> Hi, >>>> >>>> >>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote: >>>>> On 19/08/16 07:36, Ritesh Harjani wrote: >>>>>> sdhci-msm controller may have different clk-rates for each >>>>>> bus speed mode. Thus implement set_clock callback for >>>>>> sdhci-msm driver. >>>>>> >>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>>>> --- >>>>>> drivers/mmc/host/sdhci-msm.c | 103 >>>>>> ++++++++++++++++++++++++++++++++++++++++++- >>>>>> 1 file changed, 102 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>>>> index 7c032c3..c0ad9c2 100644 >>>>>> --- a/drivers/mmc/host/sdhci-msm.c >>>>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>>>>> struct mmc_host *mmc; >>>>>> bool use_14lpp_dll_reset; >>>>>> struct sdhci_msm_pltfm_data *pdata; >>>>>> + u32 clk_rate; >>>>>> }; >>>>>> >>>>>> /* Platform specific tuning */ >>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>>>>> sdhci_host *host) >>>>>> return msm_host->pdata->clk_table[0]; >>>>>> } >>>>>> >>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>>>>> + u32 req_clk) >>>>>> +{ >>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>> + int count = msm_host->pdata->clk_table_sz; >>>>>> + unsigned int sel_clk = -1; >>>>>> + int cnt; >>>>>> + >>>>>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>>>>> + sel_clk = sdhci_msm_get_min_clock(host); >>>>>> + return sel_clk; >>>>>> + } >>>>>> + >>>>>> + for (cnt = 0; cnt < count; cnt++) { >>>>>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>>>>> + break; >>>>>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>> + break; >>>>>> + } else { >>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>> + } >>>>>> + } >>>>> >>>>> 'else' is not needed after 'break' but can't this be simpler e.g. >>>>> >>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >>>>> *msm_host, u32 req_clk) >>>>> { >>>>> int count = msm_host->pdata->clk_table_sz; >>>>> unsigned int sel_clk = -1; >>>>> >>>>> while (count--) { >>>>> sel_clk = msm_host->pdata->clk_table[count]; >>>>> if (req_clk >= sel_clk) >>>>> return sel_clk; >>>>> } >>>>> >>>>> return sel_clk; >>>>> } >>>> >>>> Ok, sure I will check and get back on this. >>>> >>>> >>>>> >>>>> >>>>>> + return sel_clk; >>>>>> +} >>>>> >>>>> Blank line needed >>>> Ok done. >>>> >>>>> >>>>>> +/** >>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>>>>> + * >>>>>> + * Description: >>>>>> + * Implement MSM version of sdhci_set_clock. >>>>>> + * This is required since MSM controller does not >>>>>> + * use internal divider and instead directly control >>>>>> + * the GCC clock as per HW recommendation. >>>>>> + **/ >>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>>>>> +{ >>>>>> + u16 clk; >>>>>> + unsigned long timeout; >>>>>> + >>>>>> + host->mmc->actual_clock = 0; >>>>>> + >>>>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>>>>> + >>>>>> + if (clock == 0) >>>>>> + return; >>>>> >>>>> Should set host->mmc->actual_clock to the actual rate somewhere. >>>> Since MSM controller does not uses divider then there is no need of having >>>> actual clock since it should be same as host->clock. >>> >>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? >> So I was assuming we need host->mmc->actual_clock rate if host->clock is not >> the actual rate. >> Ok, so I will update actual_clock to host->clock itself. >> >> pls, let me know if any concerns. > > I am confused. Isn't clk_get_rate(msm_host->clk) the actual clock rate? Actually, msm controller have few quirks around clocks itself and I was thinking of not using actual_clock to avoid adding any extra quirks for msm. If you see if actual_clock is 0 and if SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK is set, timeout_clk take host->clock for timeout calculation. That's one reason I did not want to update the host->actual_clock. (otherwise timeout calc will go wrong for DDR modes for msm, where clock from GCC is made double of host->clock and internally divided by controller on it's own. In this case actual_clock would be shown as 2x of host->clock). Do you think that keeping actual_clock to 0 should be fine in this case? I have not yet worked over timeout calculation patches yet, since it is of lower priority in my list. After few other areas (mostly HS400), I will take over clock timeout fixes to be up-streamed. > >>> >>>> >>>> That's why I had kept it 0 intentionally. But I will add a comment >>>> here then. >>>> >>>>> >>>>>> + >>>>>> + /* >>>>>> + * MSM controller do not use clock divider. >>>>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>>>>> + * clock with no divider value programmed. >>>>>> + */ >>>>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>>>> + >>>>>> + clk |= SDHCI_CLOCK_INT_EN; >>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>> + >>>>>> + /* Wait max 20 ms */ >>>>>> + timeout = 20; >>>>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>>>>> + & SDHCI_CLOCK_INT_STABLE)) { >>>>>> + if (timeout == 0) { >>>>>> + pr_err("%s: Internal clock never stabilised.\n", >>>>>> + mmc_hostname(host->mmc)); >>>>>> + return; >>>>>> + } >>>>>> + timeout--; >>>>>> + mdelay(1); >>>>>> + } >>>>>> + >>>>>> + clk |= SDHCI_CLOCK_CARD_EN; >>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>> +} >>>>>> + >>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>>>>> clock) >>>>>> +{ >>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>> + u32 msm_clock = 0; >>>>>> + int rc = 0; >>>>>> + >>>>>> + if (!clock) >>>>>> + goto out; >>>>>> + >>>>>> + if (clock != msm_host->clk_rate) { >>>>>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>>>>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>>>>> + if (rc) { >>>>>> + pr_err("%s: failed to set clock at rate %u, requested clock >>>>>> rate %u\n", >>>>>> + mmc_hostname(host->mmc), msm_clock, clock); >>>>>> + goto out; >>>>>> + } >>>>>> + msm_host->clk_rate = clock; >>>>>> + pr_debug("%s: setting clock at rate %lu\n", >>>>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>>>>> + } >>>>>> +out: >>>>>> + __sdhci_msm_set_clock(host, clock); >>>>>> +} >>>>>> + >>>>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>>>> {}, >>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>>>>> static const struct sdhci_ops sdhci_msm_ops = { >>>>>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>>>>> .reset = sdhci_reset, >>>>>> - .set_clock = sdhci_set_clock, >>>>>> + .set_clock = sdhci_msm_set_clock, >>>>>> .get_min_clock = sdhci_msm_get_min_clock, >>>>>> .get_max_clock = sdhci_msm_get_max_clock, >>>>>> .set_bus_width = sdhci_set_bus_width, >>>>>> >>>>> >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/08/16 15:56, Ritesh Harjani wrote: > Hi Adrian, > > > On 8/22/2016 2:59 PM, Adrian Hunter wrote: >> On 22/08/16 12:07, Ritesh Harjani wrote: >>> Hi Adrian, >>> >>> >>> On 8/22/2016 11:50 AM, Adrian Hunter wrote: >>>> On 19/08/16 16:31, Ritesh Harjani wrote: >>>>> Hi, >>>>> >>>>> >>>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote: >>>>>> On 19/08/16 07:36, Ritesh Harjani wrote: >>>>>>> sdhci-msm controller may have different clk-rates for each >>>>>>> bus speed mode. Thus implement set_clock callback for >>>>>>> sdhci-msm driver. >>>>>>> >>>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>>>>> --- >>>>>>> drivers/mmc/host/sdhci-msm.c | 103 >>>>>>> ++++++++++++++++++++++++++++++++++++++++++- >>>>>>> 1 file changed, 102 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>>>>> index 7c032c3..c0ad9c2 100644 >>>>>>> --- a/drivers/mmc/host/sdhci-msm.c >>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>>>>>> struct mmc_host *mmc; >>>>>>> bool use_14lpp_dll_reset; >>>>>>> struct sdhci_msm_pltfm_data *pdata; >>>>>>> + u32 clk_rate; >>>>>>> }; >>>>>>> >>>>>>> /* Platform specific tuning */ >>>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>>>>>> sdhci_host *host) >>>>>>> return msm_host->pdata->clk_table[0]; >>>>>>> } >>>>>>> >>>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>>>>>> + u32 req_clk) >>>>>>> +{ >>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>>> + int count = msm_host->pdata->clk_table_sz; >>>>>>> + unsigned int sel_clk = -1; >>>>>>> + int cnt; >>>>>>> + >>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>>>>>> + sel_clk = sdhci_msm_get_min_clock(host); >>>>>>> + return sel_clk; >>>>>>> + } >>>>>>> + >>>>>>> + for (cnt = 0; cnt < count; cnt++) { >>>>>>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>>>>>> + break; >>>>>>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>>> + break; >>>>>>> + } else { >>>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>>> + } >>>>>>> + } >>>>>> >>>>>> 'else' is not needed after 'break' but can't this be simpler e.g. >>>>>> >>>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >>>>>> *msm_host, u32 req_clk) >>>>>> { >>>>>> int count = msm_host->pdata->clk_table_sz; >>>>>> unsigned int sel_clk = -1; >>>>>> >>>>>> while (count--) { >>>>>> sel_clk = msm_host->pdata->clk_table[count]; >>>>>> if (req_clk >= sel_clk) >>>>>> return sel_clk; >>>>>> } >>>>>> >>>>>> return sel_clk; >>>>>> } >>>>> >>>>> Ok, sure I will check and get back on this. >>>>> >>>>> >>>>>> >>>>>> >>>>>>> + return sel_clk; >>>>>>> +} >>>>>> >>>>>> Blank line needed >>>>> Ok done. >>>>> >>>>>> >>>>>>> +/** >>>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>>>>>> + * >>>>>>> + * Description: >>>>>>> + * Implement MSM version of sdhci_set_clock. >>>>>>> + * This is required since MSM controller does not >>>>>>> + * use internal divider and instead directly control >>>>>>> + * the GCC clock as per HW recommendation. >>>>>>> + **/ >>>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>>>>>> +{ >>>>>>> + u16 clk; >>>>>>> + unsigned long timeout; >>>>>>> + >>>>>>> + host->mmc->actual_clock = 0; >>>>>>> + >>>>>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>>>>>> + >>>>>>> + if (clock == 0) >>>>>>> + return; >>>>>> >>>>>> Should set host->mmc->actual_clock to the actual rate somewhere. >>>>> Since MSM controller does not uses divider then there is no need of having >>>>> actual clock since it should be same as host->clock. >>>> >>>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? >>> So I was assuming we need host->mmc->actual_clock rate if host->clock is not >>> the actual rate. >>> Ok, so I will update actual_clock to host->clock itself. >>> >>> pls, let me know if any concerns. >> >> I am confused. Isn't clk_get_rate(msm_host->clk) the actual clock rate? > > Actually, msm controller have few quirks around clocks itself and > I was thinking of not using actual_clock to avoid adding any extra quirks > for msm. > > If you see if actual_clock is 0 and if SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK > is set, timeout_clk take host->clock for timeout calculation. > > That's one reason I did not want to update the host->actual_clock. > (otherwise timeout calc will go wrong for DDR modes for msm, where clock > from GCC is made double of host->clock and internally divided by controller > on it's own. In this case actual_clock would be shown as 2x of host->clock). > Do you think that keeping actual_clock to 0 should be fine in this case? Yes, but please add a comment in the code. > > > I have not yet worked over timeout calculation patches yet, since it is of > lower priority in my list. After few other areas (mostly HS400), I will take > over clock timeout fixes to be up-streamed. > >> >>>> >>>>> >>>>> That's why I had kept it 0 intentionally. But I will add a comment >>>>> here then. >>>>> >>>>>> >>>>>>> + >>>>>>> + /* >>>>>>> + * MSM controller do not use clock divider. >>>>>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>>>>>> + * clock with no divider value programmed. >>>>>>> + */ >>>>>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>>>>> + >>>>>>> + clk |= SDHCI_CLOCK_INT_EN; >>>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>>> + >>>>>>> + /* Wait max 20 ms */ >>>>>>> + timeout = 20; >>>>>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>>>>>> + & SDHCI_CLOCK_INT_STABLE)) { >>>>>>> + if (timeout == 0) { >>>>>>> + pr_err("%s: Internal clock never stabilised.\n", >>>>>>> + mmc_hostname(host->mmc)); >>>>>>> + return; >>>>>>> + } >>>>>>> + timeout--; >>>>>>> + mdelay(1); >>>>>>> + } >>>>>>> + >>>>>>> + clk |= SDHCI_CLOCK_CARD_EN; >>>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>>> +} >>>>>>> + >>>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>>>>>> clock) >>>>>>> +{ >>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>>> + u32 msm_clock = 0; >>>>>>> + int rc = 0; >>>>>>> + >>>>>>> + if (!clock) >>>>>>> + goto out; >>>>>>> + >>>>>>> + if (clock != msm_host->clk_rate) { >>>>>>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>>>>>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>>>>>> + if (rc) { >>>>>>> + pr_err("%s: failed to set clock at rate %u, requested clock >>>>>>> rate %u\n", >>>>>>> + mmc_hostname(host->mmc), msm_clock, clock); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + msm_host->clk_rate = clock; >>>>>>> + pr_debug("%s: setting clock at rate %lu\n", >>>>>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>>>>>> + } >>>>>>> +out: >>>>>>> + __sdhci_msm_set_clock(host, clock); >>>>>>> +} >>>>>>> + >>>>>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>>>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>>>>> {}, >>>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>>>>>> static const struct sdhci_ops sdhci_msm_ops = { >>>>>>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>>>>>> .reset = sdhci_reset, >>>>>>> - .set_clock = sdhci_set_clock, >>>>>>> + .set_clock = sdhci_msm_set_clock, >>>>>>> .get_min_clock = sdhci_msm_get_min_clock, >>>>>>> .get_max_clock = sdhci_msm_get_max_clock, >>>>>>> .set_bus_width = sdhci_set_bus_width, >>>>>>> >>>>>> >>>>> >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 8/23/2016 6:47 PM, Adrian Hunter wrote: > On 22/08/16 15:56, Ritesh Harjani wrote: >> Hi Adrian, >> >> >> On 8/22/2016 2:59 PM, Adrian Hunter wrote: >>> On 22/08/16 12:07, Ritesh Harjani wrote: >>>> Hi Adrian, >>>> >>>> >>>> On 8/22/2016 11:50 AM, Adrian Hunter wrote: >>>>> On 19/08/16 16:31, Ritesh Harjani wrote: >>>>>> Hi, >>>>>> >>>>>> >>>>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote: >>>>>>> On 19/08/16 07:36, Ritesh Harjani wrote: >>>>>>>> sdhci-msm controller may have different clk-rates for each >>>>>>>> bus speed mode. Thus implement set_clock callback for >>>>>>>> sdhci-msm driver. >>>>>>>> >>>>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>>>>>> --- >>>>>>>> drivers/mmc/host/sdhci-msm.c | 103 >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++- >>>>>>>> 1 file changed, 102 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>>>>>> index 7c032c3..c0ad9c2 100644 >>>>>>>> --- a/drivers/mmc/host/sdhci-msm.c >>>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host { >>>>>>>> struct mmc_host *mmc; >>>>>>>> bool use_14lpp_dll_reset; >>>>>>>> struct sdhci_msm_pltfm_data *pdata; >>>>>>>> + u32 clk_rate; >>>>>>>> }; >>>>>>>> >>>>>>>> /* Platform specific tuning */ >>>>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct >>>>>>>> sdhci_host *host) >>>>>>>> return msm_host->pdata->clk_table[0]; >>>>>>>> } >>>>>>>> >>>>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, >>>>>>>> + u32 req_clk) >>>>>>>> +{ >>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>>>> + int count = msm_host->pdata->clk_table_sz; >>>>>>>> + unsigned int sel_clk = -1; >>>>>>>> + int cnt; >>>>>>>> + >>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host)) { >>>>>>>> + sel_clk = sdhci_msm_get_min_clock(host); >>>>>>>> + return sel_clk; >>>>>>>> + } >>>>>>>> + >>>>>>>> + for (cnt = 0; cnt < count; cnt++) { >>>>>>>> + if (msm_host->pdata->clk_table[cnt] > req_clk) { >>>>>>>> + break; >>>>>>>> + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { >>>>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>>>> + break; >>>>>>>> + } else { >>>>>>>> + sel_clk = msm_host->pdata->clk_table[cnt]; >>>>>>>> + } >>>>>>>> + } >>>>>>> >>>>>>> 'else' is not needed after 'break' but can't this be simpler e.g. >>>>>>> >>>>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host >>>>>>> *msm_host, u32 req_clk) >>>>>>> { >>>>>>> int count = msm_host->pdata->clk_table_sz; >>>>>>> unsigned int sel_clk = -1; >>>>>>> >>>>>>> while (count--) { >>>>>>> sel_clk = msm_host->pdata->clk_table[count]; >>>>>>> if (req_clk >= sel_clk) >>>>>>> return sel_clk; >>>>>>> } >>>>>>> >>>>>>> return sel_clk; >>>>>>> } >>>>>> >>>>>> Ok, sure I will check and get back on this. >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> + return sel_clk; >>>>>>>> +} >>>>>>> >>>>>>> Blank line needed >>>>>> Ok done. >>>>>> >>>>>>> >>>>>>>> +/** >>>>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>>>>>>> + * >>>>>>>> + * Description: >>>>>>>> + * Implement MSM version of sdhci_set_clock. >>>>>>>> + * This is required since MSM controller does not >>>>>>>> + * use internal divider and instead directly control >>>>>>>> + * the GCC clock as per HW recommendation. >>>>>>>> + **/ >>>>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>>>>>>> +{ >>>>>>>> + u16 clk; >>>>>>>> + unsigned long timeout; >>>>>>>> + >>>>>>>> + host->mmc->actual_clock = 0; >>>>>>>> + >>>>>>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>>>>>>> + >>>>>>>> + if (clock == 0) >>>>>>>> + return; >>>>>>> >>>>>>> Should set host->mmc->actual_clock to the actual rate somewhere. >>>>>> Since MSM controller does not uses divider then there is no need of having >>>>>> actual clock since it should be same as host->clock. >>>>> >>>>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate? >>>> So I was assuming we need host->mmc->actual_clock rate if host->clock is not >>>> the actual rate. >>>> Ok, so I will update actual_clock to host->clock itself. >>>> >>>> pls, let me know if any concerns. >>> >>> I am confused. Isn't clk_get_rate(msm_host->clk) the actual clock rate? >> >> Actually, msm controller have few quirks around clocks itself and >> I was thinking of not using actual_clock to avoid adding any extra quirks >> for msm. >> >> If you see if actual_clock is 0 and if SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK >> is set, timeout_clk take host->clock for timeout calculation. >> >> That's one reason I did not want to update the host->actual_clock. >> (otherwise timeout calc will go wrong for DDR modes for msm, where clock >> from GCC is made double of host->clock and internally divided by controller >> on it's own. In this case actual_clock would be shown as 2x of host->clock). >> Do you think that keeping actual_clock to 0 should be fine in this case? > > Yes, but please add a comment in the code. Sure thanks. I will address all comments in v4 spin. > >> >> >> I have not yet worked over timeout calculation patches yet, since it is of >> lower priority in my list. After few other areas (mostly HS400), I will take >> over clock timeout fixes to be up-streamed. >> >>> >>>>> >>>>>> >>>>>> That's why I had kept it 0 intentionally. But I will add a comment >>>>>> here then. >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * MSM controller do not use clock divider. >>>>>>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>>>>>>> + * clock with no divider value programmed. >>>>>>>> + */ >>>>>>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>>>>>> + >>>>>>>> + clk |= SDHCI_CLOCK_INT_EN; >>>>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>>>> + >>>>>>>> + /* Wait max 20 ms */ >>>>>>>> + timeout = 20; >>>>>>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>>>>>>> + & SDHCI_CLOCK_INT_STABLE)) { >>>>>>>> + if (timeout == 0) { >>>>>>>> + pr_err("%s: Internal clock never stabilised.\n", >>>>>>>> + mmc_hostname(host->mmc)); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + timeout--; >>>>>>>> + mdelay(1); >>>>>>>> + } >>>>>>>> + >>>>>>>> + clk |= SDHCI_CLOCK_CARD_EN; >>>>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int >>>>>>>> clock) >>>>>>>> +{ >>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>>>>>>> + u32 msm_clock = 0; >>>>>>>> + int rc = 0; >>>>>>>> + >>>>>>>> + if (!clock) >>>>>>>> + goto out; >>>>>>>> + >>>>>>>> + if (clock != msm_host->clk_rate) { >>>>>>>> + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); >>>>>>>> + rc = clk_set_rate(msm_host->clk, msm_clock); >>>>>>>> + if (rc) { >>>>>>>> + pr_err("%s: failed to set clock at rate %u, requested clock >>>>>>>> rate %u\n", >>>>>>>> + mmc_hostname(host->mmc), msm_clock, clock); >>>>>>>> + goto out; >>>>>>>> + } >>>>>>>> + msm_host->clk_rate = clock; >>>>>>>> + pr_debug("%s: setting clock at rate %lu\n", >>>>>>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>>>>>>> + } >>>>>>>> +out: >>>>>>>> + __sdhci_msm_set_clock(host, clock); >>>>>>>> +} >>>>>>>> + >>>>>>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>>>>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>>>>>> {}, >>>>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); >>>>>>>> static const struct sdhci_ops sdhci_msm_ops = { >>>>>>>> .platform_execute_tuning = sdhci_msm_execute_tuning, >>>>>>>> .reset = sdhci_reset, >>>>>>>> - .set_clock = sdhci_set_clock, >>>>>>>> + .set_clock = sdhci_msm_set_clock, >>>>>>>> .get_min_clock = sdhci_msm_get_min_clock, >>>>>>>> .get_max_clock = sdhci_msm_get_max_clock, >>>>>>>> .set_bus_width = sdhci_set_bus_width, >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 7c032c3..c0ad9c2 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -89,6 +89,7 @@ struct sdhci_msm_host { struct mmc_host *mmc; bool use_14lpp_dll_reset; struct sdhci_msm_pltfm_data *pdata; + u32 clk_rate; }; /* Platform specific tuning */ @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host) return msm_host->pdata->clk_table[0]; } +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host, + u32 req_clk) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + int count = msm_host->pdata->clk_table_sz; + unsigned int sel_clk = -1; + int cnt; + + if (req_clk < sdhci_msm_get_min_clock(host)) { + sel_clk = sdhci_msm_get_min_clock(host); + return sel_clk; + } + + for (cnt = 0; cnt < count; cnt++) { + if (msm_host->pdata->clk_table[cnt] > req_clk) { + break; + } else if (msm_host->pdata->clk_table[cnt] == req_clk) { + sel_clk = msm_host->pdata->clk_table[cnt]; + break; + } else { + sel_clk = msm_host->pdata->clk_table[cnt]; + } + } + return sel_clk; +} +/** + * __sdhci_msm_set_clock - sdhci_msm clock control. + * + * Description: + * Implement MSM version of sdhci_set_clock. + * This is required since MSM controller does not + * use internal divider and instead directly control + * the GCC clock as per HW recommendation. + **/ +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) +{ + u16 clk; + unsigned long timeout; + + host->mmc->actual_clock = 0; + + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); + + if (clock == 0) + return; + + /* + * MSM controller do not use clock divider. + * Thus read SDHCI_CLOCK_CONTROL and only enable + * clock with no divider value programmed. + */ + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + + clk |= SDHCI_CLOCK_INT_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); + + /* Wait max 20 ms */ + timeout = 20; + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) + & SDHCI_CLOCK_INT_STABLE)) { + if (timeout == 0) { + pr_err("%s: Internal clock never stabilised.\n", + mmc_hostname(host->mmc)); + return; + } + timeout--; + mdelay(1); + } + + clk |= SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); +} + +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + u32 msm_clock = 0; + int rc = 0; + + if (!clock) + goto out; + + if (clock != msm_host->clk_rate) { + msm_clock = sdhci_msm_get_msm_clk_rate(host, clock); + rc = clk_set_rate(msm_host->clk, msm_clock); + if (rc) { + pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n", + mmc_hostname(host->mmc), msm_clock, clock); + goto out; + } + msm_host->clk_rate = clock; + pr_debug("%s: setting clock at rate %lu\n", + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); + } +out: + __sdhci_msm_set_clock(host, clock); +} + static const struct of_device_id sdhci_msm_dt_match[] = { { .compatible = "qcom,sdhci-msm-v4" }, {}, @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match); static const struct sdhci_ops sdhci_msm_ops = { .platform_execute_tuning = sdhci_msm_execute_tuning, .reset = sdhci_reset, - .set_clock = sdhci_set_clock, + .set_clock = sdhci_msm_set_clock, .get_min_clock = sdhci_msm_get_min_clock, .get_max_clock = sdhci_msm_get_max_clock, .set_bus_width = sdhci_set_bus_width,