| Message ID | 1400849578-7585-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > On Controllers like Qcom SD card controller where cclk is mclk and mclk should > be directly controlled by the driver. > > This patch adds support to control mclk directly in the driver, and also > adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving > more flexibility to the driver. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 5cbf644..f6dfd24 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -73,6 +73,8 @@ static unsigned int fmax = 515633; > * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply > * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates > * are not ignored. > + * @explicit_mclk_control: enable explicit mclk control in driver. > + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. > */ > struct variant_data { > unsigned int clkreg; > @@ -94,6 +96,8 @@ struct variant_data { > bool busy_detect; > bool pwrreg_nopower; > bool mclk_delayed_writes; > + bool explicit_mclk_control; > + bool cclk_is_mclk; I can't see why you need to have both these new configurations. Aren't "cclk_is_mclk" just a fact when you use "explicit_mclk_control". I also believe I would prefer something like "qcom_clkdiv" instead. > }; > > static struct variant_data variant_arm = { > @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = { > * for 3 clk cycles. > */ > .mclk_delayed_writes = true, > + .explicit_mclk_control = true, > + .cclk_is_mclk = true, > }; > > static inline u32 mmci_readl(struct mmci_host *host, u32 off) > @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) > host->cclk = 0; > > if (desired) { > - if (desired >= host->mclk) { > + if (variant->cclk_is_mclk) { > + host->cclk = host->mclk; > + } else if (desired >= host->mclk) { > clk = MCI_CLK_BYPASS; > if (variant->st_clkdiv) > clk |= MCI_ST_UX500_NEG_EDGE; > @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > if (!ios->clock && variant->pwrreg_clkgate) > pwr &= ~MCI_PWR_ON; > > + if (ios->clock != host->mclk && host->variant->explicit_mclk_control) { I suggest you should clarify the statement by adding a pair of extra parentheses. Additionally it seems like a good idea to reverse the order of the statements, to clarify this is for qcom clock handling only. More important, what I think you really want to do is to compare "ios->clock" with it's previous value it had when ->set_ios were invoked. Then let a changed value act as the trigger to set a new clk rate. Obvoiusly you need to cache the clock rate in the struct mmci host to handle this. > + int rc = clk_set_rate(host->clk, ios->clock); > + if (rc < 0) { > + dev_err(mmc_dev(host->mmc), > + "Error setting clock rate (%d)\n", rc); > + } else { > + host->mclk = clk_get_rate(host->clk); So here you actually find out the new clk rate, but shouldn't you update "host->mclk" within the spin_lock? Or it might not matter? > + } > + } > + > spin_lock_irqsave(&host->lock, flags); > > mmci_set_clkreg(host, ios->clock); > @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, > * is not specified. Either value must not exceed the clock rate into > * the block, of course. > */ > - if (mmc->f_max) > - mmc->f_max = min(host->mclk, mmc->f_max); > - else > - mmc->f_max = min(host->mclk, fmax); > + if (!host->variant->explicit_mclk_control) { > + if (mmc->f_max) > + mmc->f_max = min(host->mclk, mmc->f_max); > + else > + mmc->f_max = min(host->mclk, fmax); > + } This means your mmc->f_max value will either be zero or the one you provided through DT. And since zero won't work, that means you _require_ to get the value from DT. According to the documentation of this DT binding, f_max is optional. So unless you fine another way of dynamically at runtime figure out the value of f_max (using the clk API), you need to update the DT documentation for mmci. Additionally, this makes me wonder about f_min. I haven't seen anywhere in this patch were that value is being set to proper value, right? > dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); > > /* Get regulators and the supported OCR mask */ > -- > 1.9.1 > Kind regards Ulf Hansson -- 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 26 May 2014 16:21, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> On Controllers like Qcom SD card controller where cclk is mclk and mclk should >> be directly controlled by the driver. >> >> This patch adds support to control mclk directly in the driver, and also >> adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving >> more flexibility to the driver. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++----- >> 1 file changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 5cbf644..f6dfd24 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633; >> * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply >> * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates >> * are not ignored. >> + * @explicit_mclk_control: enable explicit mclk control in driver. >> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. >> */ >> struct variant_data { >> unsigned int clkreg; >> @@ -94,6 +96,8 @@ struct variant_data { >> bool busy_detect; >> bool pwrreg_nopower; >> bool mclk_delayed_writes; >> + bool explicit_mclk_control; >> + bool cclk_is_mclk; > > I can't see why you need to have both these new configurations. Aren't > "cclk_is_mclk" just a fact when you use "explicit_mclk_control". > > I also believe I would prefer something like "qcom_clkdiv" instead. > >> }; >> >> static struct variant_data variant_arm = { >> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = { >> * for 3 clk cycles. >> */ >> .mclk_delayed_writes = true, >> + .explicit_mclk_control = true, >> + .cclk_is_mclk = true, >> }; >> >> static inline u32 mmci_readl(struct mmci_host *host, u32 off) >> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) >> host->cclk = 0; >> >> if (desired) { >> - if (desired >= host->mclk) { >> + if (variant->cclk_is_mclk) { >> + host->cclk = host->mclk; >> + } else if (desired >= host->mclk) { >> clk = MCI_CLK_BYPASS; >> if (variant->st_clkdiv) >> clk |= MCI_ST_UX500_NEG_EDGE; >> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> if (!ios->clock && variant->pwrreg_clkgate) >> pwr &= ~MCI_PWR_ON; >> >> + if (ios->clock != host->mclk && host->variant->explicit_mclk_control) { > > I suggest you should clarify the statement by adding a pair of extra > parentheses. Additionally it seems like a good idea to reverse the > order of the statements, to clarify this is for qcom clock handling > only. > > More important, what I think you really want to do is to compare > "ios->clock" with it's previous value it had when ->set_ios were > invoked. Then let a changed value act as the trigger to set a new clk > rate. Obvoiusly you need to cache the clock rate in the struct mmci > host to handle this. > >> + int rc = clk_set_rate(host->clk, ios->clock); >> + if (rc < 0) { >> + dev_err(mmc_dev(host->mmc), >> + "Error setting clock rate (%d)\n", rc); >> + } else { >> + host->mclk = clk_get_rate(host->clk); > > So here you actually find out the new clk rate, but shouldn't you > update "host->mclk" within the spin_lock? Or it might not matter? > >> + } >> + } >> + >> spin_lock_irqsave(&host->lock, flags); >> >> mmci_set_clkreg(host, ios->clock); >> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, >> * is not specified. Either value must not exceed the clock rate into >> * the block, of course. >> */ >> - if (mmc->f_max) >> - mmc->f_max = min(host->mclk, mmc->f_max); >> - else >> - mmc->f_max = min(host->mclk, fmax); >> + if (!host->variant->explicit_mclk_control) { >> + if (mmc->f_max) >> + mmc->f_max = min(host->mclk, mmc->f_max); >> + else >> + mmc->f_max = min(host->mclk, fmax); >> + } > > This means your mmc->f_max value will either be zero or the one you > provided through DT. And since zero won't work, that means you > _require_ to get the value from DT. According to the documentation of > this DT binding, f_max is optional. > > So unless you fine another way of dynamically at runtime figure out > the value of f_max (using the clk API), you need to update the DT > documentation for mmci. > > Additionally, this makes me wonder about f_min. I haven't seen > anywhere in this patch were that value is being set to proper value, > right? > >> dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); >> >> /* Get regulators and the supported OCR mask */ >> -- >> 1.9.1 >> > > Kind regards > Ulf Hansson And one more thing, just for your information. If you intend to support UHS cards, your need to be able to gate the clock though ->set_ios function, once the clock is 0. Let's handle that in a separate patch - if needed though. Kind regards Ulf Hansson -- 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 Ulf, Thankyou for the comments. On 26/05/14 15:21, Ulf Hansson wrote: > On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> On Controllers like Qcom SD card controller where cclk is mclk and mclk should >> be directly controlled by the driver. >> >> This patch adds support to control mclk directly in the driver, and also >> adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving >> more flexibility to the driver. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++----- >> 1 file changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 5cbf644..f6dfd24 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633; >> * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply >> * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates >> * are not ignored. >> + * @explicit_mclk_control: enable explicit mclk control in driver. >> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. >> */ >> struct variant_data { >> unsigned int clkreg; >> @@ -94,6 +96,8 @@ struct variant_data { >> bool busy_detect; >> bool pwrreg_nopower; >> bool mclk_delayed_writes; >> + bool explicit_mclk_control; >> + bool cclk_is_mclk; > > I can't see why you need to have both these new configurations. Aren't > "cclk_is_mclk" just a fact when you use "explicit_mclk_control". > > I also believe I would prefer something like "qcom_clkdiv" instead. There is a subtle difference between both the flags. Am happy to change it to qcom_clkdiv. > >> }; >> >> static struct variant_data variant_arm = { >> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = { >> * for 3 clk cycles. >> */ >> .mclk_delayed_writes = true, >> + .explicit_mclk_control = true, >> + .cclk_is_mclk = true, >> }; >> >> static inline u32 mmci_readl(struct mmci_host *host, u32 off) >> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) >> host->cclk = 0; >> >> if (desired) { >> - if (desired >= host->mclk) { >> + if (variant->cclk_is_mclk) { >> + host->cclk = host->mclk; >> + } else if (desired >= host->mclk) { >> clk = MCI_CLK_BYPASS; >> if (variant->st_clkdiv) >> clk |= MCI_ST_UX500_NEG_EDGE; >> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> if (!ios->clock && variant->pwrreg_clkgate) >> pwr &= ~MCI_PWR_ON; >> >> + if (ios->clock != host->mclk && host->variant->explicit_mclk_control) { > > I suggest you should clarify the statement by adding a pair of extra > parentheses. Additionally it seems like a good idea to reverse the > order of the statements, to clarify this is for qcom clock handling > only. Yes, sure Will fix this in next version. > > More important, what I think you really want to do is to compare > "ios->clock" with it's previous value it had when ->set_ios were > invoked. Then let a changed value act as the trigger to set a new clk > rate. Obvoiusly you need to cache the clock rate in the struct mmci > host to handle this. host->mclk already has this cached value. > >> + int rc = clk_set_rate(host->clk, ios->clock); >> + if (rc < 0) { >> + dev_err(mmc_dev(host->mmc), >> + "Error setting clock rate (%d)\n", rc); >> + } else { >> + host->mclk = clk_get_rate(host->clk); > > So here you actually find out the new clk rate, but shouldn't you > update "host->mclk" within the spin_lock? Or it might not matter? > I think it does not matter in this case, as this is the only place mclk gets modified. >> + } >> + } >> + >> spin_lock_irqsave(&host->lock, flags); >> >> mmci_set_clkreg(host, ios->clock); >> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, >> * is not specified. Either value must not exceed the clock rate into >> * the block, of course. >> */ >> - if (mmc->f_max) >> - mmc->f_max = min(host->mclk, mmc->f_max); >> - else >> - mmc->f_max = min(host->mclk, fmax); >> + if (!host->variant->explicit_mclk_control) { >> + if (mmc->f_max) >> + mmc->f_max = min(host->mclk, mmc->f_max); >> + else >> + mmc->f_max = min(host->mclk, fmax); >> + } > > This means your mmc->f_max value will either be zero or the one you > provided through DT. And since zero won't work, that means you > _require_ to get the value from DT. According to the documentation of > this DT binding, f_max is optional. > > So unless you fine another way of dynamically at runtime figure out > the value of f_max (using the clk API), you need to update the DT > documentation for mmci. > You are right there is a possibility of f_max to be zero. This logic could fix it. if (host->variant->explicit_mclk_control) { if (mmc->f_max) mmc->f_max = max(host->mclk, mmc->f_max); else mmc->f_max = max(host->mclk, fmax); } else { if (mmc->f_max) mmc->f_max = min(host->mclk, mmc->f_max); else mmc->f_max = min(host->mclk, fmax); } > Additionally, this makes me wonder about f_min. I haven't seen > anywhere in this patch were that value is being set to proper value, > right? > f_min should be 400000 for qcom, I think with the default mclk frequency and a divider of 512 used for calculating the f_min is bringing down the f_min to lessthan 400Kz. Which is why its working fine. I think the possibility of mclk default frequency being greater than 208Mhz is very less. so I could either leave it as it is Or force this to 400000 all the time for qcom chips. Am ok either way.. Thanks, srini >> dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); >> >> /* Get regulators and the supported OCR mask */ >> -- >> 1.9.1 >> > > Kind regards > Ulf Hansson > -- 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
On 27 May 2014 00:39, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > Hi Ulf, > Thankyou for the comments. > > > On 26/05/14 15:21, Ulf Hansson wrote: >> >> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >>> >>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>> >>> On Controllers like Qcom SD card controller where cclk is mclk and mclk >>> should >>> be directly controlled by the driver. >>> >>> This patch adds support to control mclk directly in the driver, and also >>> adds explicit_mclk_control and cclk_is_mclk flags in variant structure >>> giving >>> more flexibility to the driver. >>> >>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>> --- >>> drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++----- >>> 1 file changed, 25 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >>> index 5cbf644..f6dfd24 100644 >>> --- a/drivers/mmc/host/mmci.c >>> +++ b/drivers/mmc/host/mmci.c >>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633; >>> * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply >>> * @mclk_delayed_writes: enable delayed writes to ensure, subsequent >>> updates >>> * are not ignored. >>> + * @explicit_mclk_control: enable explicit mclk control in driver. >>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter >>> clock. >>> */ >>> struct variant_data { >>> unsigned int clkreg; >>> @@ -94,6 +96,8 @@ struct variant_data { >>> bool busy_detect; >>> bool pwrreg_nopower; >>> bool mclk_delayed_writes; >>> + bool explicit_mclk_control; >>> + bool cclk_is_mclk; >> >> >> I can't see why you need to have both these new configurations. Aren't >> "cclk_is_mclk" just a fact when you use "explicit_mclk_control". >> > > >> I also believe I would prefer something like "qcom_clkdiv" instead. > > > There is a subtle difference between both the flags. Am happy to change it > to qcom_clkdiv. > > >> >>> }; >>> >>> static struct variant_data variant_arm = { >>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = { >>> * for 3 clk cycles. >>> */ >>> .mclk_delayed_writes = true, >>> + .explicit_mclk_control = true, >>> + .cclk_is_mclk = true, >>> }; >>> >>> static inline u32 mmci_readl(struct mmci_host *host, u32 off) >>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, >>> unsigned int desired) >>> host->cclk = 0; >>> >>> if (desired) { >>> - if (desired >= host->mclk) { >>> + if (variant->cclk_is_mclk) { >>> + host->cclk = host->mclk; >>> + } else if (desired >= host->mclk) { >>> clk = MCI_CLK_BYPASS; >>> if (variant->st_clkdiv) >>> clk |= MCI_ST_UX500_NEG_EDGE; >>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, >>> struct mmc_ios *ios) >>> if (!ios->clock && variant->pwrreg_clkgate) >>> pwr &= ~MCI_PWR_ON; >>> >>> + if (ios->clock != host->mclk && >>> host->variant->explicit_mclk_control) { >> >> >> I suggest you should clarify the statement by adding a pair of extra >> parentheses. Additionally it seems like a good idea to reverse the >> order of the statements, to clarify this is for qcom clock handling >> only. > > Yes, sure Will fix this in next version. > > >> >> More important, what I think you really want to do is to compare >> "ios->clock" with it's previous value it had when ->set_ios were >> invoked. Then let a changed value act as the trigger to set a new clk >> rate. Obvoiusly you need to cache the clock rate in the struct mmci >> host to handle this. > > > host->mclk already has this cached value. There are no guarantees clk_set_rate() will manage to set the exact requested rate as ios->clock. At least if you follow the clk API documentation. > > >> >>> + int rc = clk_set_rate(host->clk, ios->clock); >>> + if (rc < 0) { >>> + dev_err(mmc_dev(host->mmc), >>> + "Error setting clock rate (%d)\n", rc); >>> + } else { >>> + host->mclk = clk_get_rate(host->clk); >> >> >> So here you actually find out the new clk rate, but shouldn't you >> update "host->mclk" within the spin_lock? Or it might not matter? >> > I think it does not matter in this case, as this is the only place mclk gets > modified. OK. > >>> + } >>> + } >>> + >>> spin_lock_irqsave(&host->lock, flags); >>> >>> mmci_set_clkreg(host, ios->clock); >>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, >>> * is not specified. Either value must not exceed the clock rate >>> into >>> * the block, of course. >>> */ >>> - if (mmc->f_max) >>> - mmc->f_max = min(host->mclk, mmc->f_max); >>> - else >>> - mmc->f_max = min(host->mclk, fmax); >>> + if (!host->variant->explicit_mclk_control) { >>> + if (mmc->f_max) >>> + mmc->f_max = min(host->mclk, mmc->f_max); >>> + else >>> + mmc->f_max = min(host->mclk, fmax); >>> + } >> >> >> This means your mmc->f_max value will either be zero or the one you >> provided through DT. And since zero won't work, that means you >> _require_ to get the value from DT. According to the documentation of >> this DT binding, f_max is optional. >> >> So unless you fine another way of dynamically at runtime figure out >> the value of f_max (using the clk API), you need to update the DT >> documentation for mmci. >> > > You are right there is a possibility of f_max to be zero. > > This logic could fix it. > > if (host->variant->explicit_mclk_control) { > if (mmc->f_max) > mmc->f_max = max(host->mclk, mmc->f_max); > else > mmc->f_max = max(host->mclk, fmax); > } else { > if (mmc->f_max) > > mmc->f_max = min(host->mclk, mmc->f_max); > else > > mmc->f_max = min(host->mclk, fmax); > } > Hmm. Looking a bit deeper into this, we have some additional related code to fixup. :-) In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's due to the current variants don't support higher frequency than this. It seems like the Qcom variant may support up to 208MHz? Now, if that's the case, we need to add "f_max" to the struct variant_data to store this information, so we can respect different values while doing clk_set_rate() at ->probe(). While updating mmc->f_max for host->variant->explicit_mclk_control, we shouldn't care about using the host->mclk as a limiter, instead, use min(mmc->f_max, host->variant->f_max) and fallback to fmax. Not sure how that will affect the logic. :-) > > >> Additionally, this makes me wonder about f_min. I haven't seen >> anywhere in this patch were that value is being set to proper value, >> right? >> > > f_min should be 400000 for qcom, I think with the default mclk frequency and > a divider of 512 used for calculating the f_min is bringing down the f_min > to lessthan 400Kz. Which is why its working fine. > I think the possibility of mclk default frequency being greater than 208Mhz > is very less. so I could either leave it as it is Or force this to 400000 > all the time for qcom chips. No, this seems like a wrong approach. I think you would like to do use the clk_round_rate() find out the lowest possible rate. Or just use a fixed fallback value somehow. Kind regards Ulf Hansson -- 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
On 27/05/14 10:32, Ulf Hansson wrote: > On 27 May 2014 00:39, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> Hi Ulf, >> Thankyou for the comments. >> >> >> On 26/05/14 15:21, Ulf Hansson wrote: >>> >>> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >>>> >>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>>> >>>> On Controllers like Qcom SD card controller where cclk is mclk and mclk >>>> should >>>> be directly controlled by the driver. >>>> ... >>>> * for 3 clk cycles. >>>> */ >>>> .mclk_delayed_writes = true, >>>> + .explicit_mclk_control = true, >>>> + .cclk_is_mclk = true, >>>> }; >>>> >>>> static inline u32 mmci_readl(struct mmci_host *host, u32 off) >>>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, >>>> unsigned int desired) >>>> host->cclk = 0; >>>> >>>> if (desired) { >>>> - if (desired >= host->mclk) { >>>> + if (variant->cclk_is_mclk) { >>>> + host->cclk = host->mclk; >>>> + } else if (desired >= host->mclk) { >>>> clk = MCI_CLK_BYPASS; >>>> if (variant->st_clkdiv) >>>> clk |= MCI_ST_UX500_NEG_EDGE; >>>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, >>>> struct mmc_ios *ios) >>>> if (!ios->clock && variant->pwrreg_clkgate) >>>> pwr &= ~MCI_PWR_ON; >>>> >>>> + if (ios->clock != host->mclk && >>>> host->variant->explicit_mclk_control) { >>> >>> >>> I suggest you should clarify the statement by adding a pair of extra >>> parentheses. Additionally it seems like a good idea to reverse the >>> order of the statements, to clarify this is for qcom clock handling >>> only. >> >> Yes, sure Will fix this in next version. >> >> >>> >>> More important, what I think you really want to do is to compare >>> "ios->clock" with it's previous value it had when ->set_ios were >>> invoked. Then let a changed value act as the trigger to set a new clk >>> rate. Obvoiusly you need to cache the clock rate in the struct mmci >>> host to handle this. >> >> >> host->mclk already has this cached value. > > There are no guarantees clk_set_rate() will manage to set the exact > requested rate as ios->clock. At least if you follow the clk API > documentation. > Yes, I agree. caching ios->clock looks like the best option. >> >> >>> ... >> >>>> + } >>>> + } >>>> + >>>> spin_lock_irqsave(&host->lock, flags); >>>> >>>> mmci_set_clkreg(host, ios->clock); >>>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, >>>> * is not specified. Either value must not exceed the clock rate >>>> into >>>> * the block, of course. >>>> */ >>>> - if (mmc->f_max) >>>> - mmc->f_max = min(host->mclk, mmc->f_max); >>>> - else >>>> - mmc->f_max = min(host->mclk, fmax); >>>> + if (!host->variant->explicit_mclk_control) { >>>> + if (mmc->f_max) >>>> + mmc->f_max = min(host->mclk, mmc->f_max); >>>> + else >>>> + mmc->f_max = min(host->mclk, fmax); >>>> + } >>> >>> >>> This means your mmc->f_max value will either be zero or the one you >>> provided through DT. And since zero won't work, that means you >>> _require_ to get the value from DT. According to the documentation of >>> this DT binding, f_max is optional. >>> >>> So unless you fine another way of dynamically at runtime figure out >>> the value of f_max (using the clk API), you need to update the DT >>> documentation for mmci. >>> >> >> You are right there is a possibility of f_max to be zero. >> >> This logic could fix it. >> >> if (host->variant->explicit_mclk_control) { >> if (mmc->f_max) >> mmc->f_max = max(host->mclk, mmc->f_max); >> else >> mmc->f_max = max(host->mclk, fmax); >> } else { >> if (mmc->f_max) >> >> mmc->f_max = min(host->mclk, mmc->f_max); >> else >> >> mmc->f_max = min(host->mclk, fmax); >> } >> > > Hmm. Looking a bit deeper into this, we have some additional related > code to fixup. :-) > > In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's > due to the current variants don't support higher frequency than this. > It seems like the Qcom variant may support up to 208MHz? Now, if > that's the case, we need to add "f_max" to the struct variant_data to > store this information, so we can respect different values while doing > clk_set_rate() at ->probe(). > Yes, qcom SOCs support more than 100Hhz clock. Probe and clk_set_rate/set_ios should respect this. On the other thought, Should probe actually care about clocks which are explicitly controlled? It should not even attempt to set any frequency to start with. mmc-core would set the right frequency depending on the mmc state-machine respecting f_min and f_max. I think for qcom, probe should just check the if f_max and f_min are valid and set them to defaults if any in the same lines as existing code. > While updating mmc->f_max for host->variant->explicit_mclk_control, we > shouldn't care about using the host->mclk as a limiter, instead, use > min(mmc->f_max, host->variant->f_max) and fallback to fmax. > Yes, that's correct, mclk should not be used as limiter. Adding f_max to the variant looks useful. > Not sure how that will affect the logic. :-) > >> >> >>> Additionally, this makes me wonder about f_min. I haven't seen >>> anywhere in this patch were that value is being set to proper value, >>> right? >>> >> >> f_min should be 400000 for qcom, I think with the default mclk frequency and >> a divider of 512 used for calculating the f_min is bringing down the f_min >> to lessthan 400Kz. Which is why its working fine. >> I think the possibility of mclk default frequency being greater than 208Mhz >> is very less. so I could either leave it as it is Or force this to 400000 >> all the time for qcom chips. > > No, this seems like a wrong approach. > > I think you would like to do use the clk_round_rate() find out the > lowest possible rate. Or just use a fixed fallback value somehow. clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback. Let the mmc-core figure out what frequency would be best from its table starting from f_min. thanks, srini > > Kind regards > Ulf Hansson > -- 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
>> Hmm. Looking a bit deeper into this, we have some additional related >> code to fixup. :-) >> >> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's >> due to the current variants don't support higher frequency than this. >> It seems like the Qcom variant may support up to 208MHz? Now, if >> that's the case, we need to add "f_max" to the struct variant_data to >> store this information, so we can respect different values while doing >> clk_set_rate() at ->probe(). >> > Yes, qcom SOCs support more than 100Hhz clock. > > Probe and clk_set_rate/set_ios should respect this. > > On the other thought, Should probe actually care about clocks which are > explicitly controlled? It should not even attempt to set any frequency to > start with. The 100 MHz is related to constraints set by the specification of the IP block, not the MMC/SD/SDIO spec. Thus at ->probe() we must perform the clk_set_rate(). > mmc-core would set the right frequency depending on the mmc > state-machine respecting f_min and f_max. > I think for qcom, probe should just check the if f_max and f_min are valid > and set them to defaults if any in the same lines as existing code. > > >> While updating mmc->f_max for host->variant->explicit_mclk_control, we >> shouldn't care about using the host->mclk as a limiter, instead, use >> min(mmc->f_max, host->variant->f_max) and fallback to fmax. >> > Yes, that's correct, mclk should not be used as limiter. Adding f_max to the > variant looks useful. > > >> Not sure how that will affect the logic. :-) >> >>> >>> >>>> Additionally, this makes me wonder about f_min. I haven't seen >>>> anywhere in this patch were that value is being set to proper value, >>>> right? >>>> >>> >>> f_min should be 400000 for qcom, I think with the default mclk frequency >>> and >>> a divider of 512 used for calculating the f_min is bringing down the >>> f_min >>> to lessthan 400Kz. Which is why its working fine. >>> I think the possibility of mclk default frequency being greater than >>> 208Mhz >>> is very less. so I could either leave it as it is Or force this to 400000 >>> all the time for qcom chips. >> >> >> No, this seems like a wrong approach. >> >> I think you would like to do use the clk_round_rate() find out the >> lowest possible rate. Or just use a fixed fallback value somehow. > > > clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback. > Let the mmc-core figure out what frequency would be best from its table > starting from f_min. Agree! clk_round_rate(mclk, 100KHz), might be better though - since that is actually the lowest request frequency whatsoever. Kind regards Ulf Hansson -- 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 27/05/14 15:07, Ulf Hansson wrote: >>> Hmm. Looking a bit deeper into this, we have some additional related >>> code to fixup. :-) >>> >>> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's >>> due to the current variants don't support higher frequency than this. >>> It seems like the Qcom variant may support up to 208MHz? Now, if >>> that's the case, we need to add "f_max" to the struct variant_data to >>> store this information, so we can respect different values while doing >>> clk_set_rate() at ->probe(). >>> >> Yes, qcom SOCs support more than 100Hhz clock. >> >> Probe and clk_set_rate/set_ios should respect this. >> >> On the other thought, Should probe actually care about clocks which are >> explicitly controlled? It should not even attempt to set any frequency to >> start with. > > The 100 MHz is related to constraints set by the specification of the > IP block, not the MMC/SD/SDIO spec. > > Thus at ->probe() we must perform the clk_set_rate(). I agree its valid for controllers which have this constraints. > >> mmc-core would set the right frequency depending on the mmc >> state-machine respecting f_min and f_max. >> I think for qcom, probe should just check the if f_max and f_min are valid >> and set them to defaults if any in the same lines as existing code. >> >> >>> While updating mmc->f_max for host->variant->explicit_mclk_control, we >>> shouldn't care about using the host->mclk as a limiter, instead, use >>> min(mmc->f_max, host->variant->f_max) and fallback to fmax. >>> >> Yes, that's correct, mclk should not be used as limiter. Adding f_max to the >> variant looks useful. >> >> >>> Not sure how that will affect the logic. :-) >>> >>>> >>>> >>>>> Additionally, this makes me wonder about f_min. I haven't seen >>>>> anywhere in this patch were that value is being set to proper value, >>>>> right? >>>>> >>>> >>>> f_min should be 400000 for qcom, I think with the default mclk frequency >>>> and >>>> a divider of 512 used for calculating the f_min is bringing down the >>>> f_min >>>> to lessthan 400Kz. Which is why its working fine. >>>> I think the possibility of mclk default frequency being greater than >>>> 208Mhz >>>> is very less. so I could either leave it as it is Or force this to 400000 >>>> all the time for qcom chips. >>> >>> >>> No, this seems like a wrong approach. >>> >>> I think you would like to do use the clk_round_rate() find out the >>> lowest possible rate. Or just use a fixed fallback value somehow. >> >> >> clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback. >> Let the mmc-core figure out what frequency would be best from its table >> starting from f_min. > > Agree! > > clk_round_rate(mclk, 100KHz), might be better though - since that is > actually the lowest request frequency whatsoever. > Perfect. --srini > Kind regards > Ulf Hansson > -- 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 Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 26/05/14 15:21, Ulf Hansson wrote: >> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >>> >>> + bool explicit_mclk_control; >>> + bool cclk_is_mclk; >> >> I can't see why you need to have both these new configurations. Aren't >> "cclk_is_mclk" just a fact when you use "explicit_mclk_control". > >> I also believe I would prefer something like "qcom_clkdiv" instead. > > There is a subtle difference between both the flags. Am happy to change it > to qcom_clkdiv. I think this was due to me wanting the variant variables to be more about the actual technical difference they indicate rather than pointing to a certain vendor or variant where that difference occurs. It's a very minor thing though, if you prefer it this way, go for it. Yours, Linus Walleij -- 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
On 28/05/14 09:02, Linus Walleij wrote: > On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> On 26/05/14 15:21, Ulf Hansson wrote: >>> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: > >>>> >>>> + bool explicit_mclk_control; >>>> + bool cclk_is_mclk; >>> >>> I can't see why you need to have both these new configurations. Aren't >>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control". >> >>> I also believe I would prefer something like "qcom_clkdiv" instead. >> >> There is a subtle difference between both the flags. Am happy to change it >> to qcom_clkdiv. > > I think this was due to me wanting the variant variables to be more about > the actual technical difference they indicate rather than pointing to > a certain vendor or variant where that difference occurs. > Yes, that's correct, I think having these two variables seems to be more generic than qcom_clkdiv. I will keep it as it is and fix other comments from Ulf in next version. > It's a very minor thing though, if you prefer it this way, go for it. > Thanks, sirni > Yours, > Linus Walleij > -- 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 28 May 2014 10:28, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > On 28/05/14 09:02, Linus Walleij wrote: >> >> On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla >> <srinivas.kandagatla@linaro.org> wrote: >>> >>> On 26/05/14 15:21, Ulf Hansson wrote: >>>> >>>> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote: >> >> >>>>> >>>>> + bool explicit_mclk_control; >>>>> + bool cclk_is_mclk; >>>> >>>> >>>> I can't see why you need to have both these new configurations. Aren't >>>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control". >>> >>> >>>> I also believe I would prefer something like "qcom_clkdiv" instead. >>> >>> >>> There is a subtle difference between both the flags. Am happy to change >>> it >>> to qcom_clkdiv. >> >> >> I think this was due to me wanting the variant variables to be more about >> the actual technical difference they indicate rather than pointing to >> a certain vendor or variant where that difference occurs. >> > Yes, that's correct, I think having these two variables seems to be more > generic than qcom_clkdiv. > > I will keep it as it is and fix other comments from Ulf in next version. > I think this relates to the discussion we had around fetching the f_min and f_max in ->probe(). It, just seems silly to have to check for an extra flag there as well, because that is in principle what this would mean, right? So, please adjust to my proposal, I strongly think this should be only one flag. You might want a more generic name of the flag in favour of qcom_clkdiv, feel free to change to whatever you think make sense. Kind regards Ulf Hansson > >> It's a very minor thing though, if you prefer it this way, go for it. >> > > Thanks, > sirni >> >> Yours, >> Linus Walleij >> > -- 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
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 5cbf644..f6dfd24 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -73,6 +73,8 @@ static unsigned int fmax = 515633; * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates * are not ignored. + * @explicit_mclk_control: enable explicit mclk control in driver. + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. */ struct variant_data { unsigned int clkreg; @@ -94,6 +96,8 @@ struct variant_data { bool busy_detect; bool pwrreg_nopower; bool mclk_delayed_writes; + bool explicit_mclk_control; + bool cclk_is_mclk; }; static struct variant_data variant_arm = { @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = { * for 3 clk cycles. */ .mclk_delayed_writes = true, + .explicit_mclk_control = true, + .cclk_is_mclk = true, }; static inline u32 mmci_readl(struct mmci_host *host, u32 off) @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) host->cclk = 0; if (desired) { - if (desired >= host->mclk) { + if (variant->cclk_is_mclk) { + host->cclk = host->mclk; + } else if (desired >= host->mclk) { clk = MCI_CLK_BYPASS; if (variant->st_clkdiv) clk |= MCI_ST_UX500_NEG_EDGE; @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) if (!ios->clock && variant->pwrreg_clkgate) pwr &= ~MCI_PWR_ON; + if (ios->clock != host->mclk && host->variant->explicit_mclk_control) { + int rc = clk_set_rate(host->clk, ios->clock); + if (rc < 0) { + dev_err(mmc_dev(host->mmc), + "Error setting clock rate (%d)\n", rc); + } else { + host->mclk = clk_get_rate(host->clk); + } + } + spin_lock_irqsave(&host->lock, flags); mmci_set_clkreg(host, ios->clock); @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev, * is not specified. Either value must not exceed the clock rate into * the block, of course. */ - if (mmc->f_max) - mmc->f_max = min(host->mclk, mmc->f_max); - else - mmc->f_max = min(host->mclk, fmax); + if (!host->variant->explicit_mclk_control) { + if (mmc->f_max) + mmc->f_max = min(host->mclk, mmc->f_max); + else + mmc->f_max = min(host->mclk, fmax); + } dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); /* Get regulators and the supported OCR mask */