Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control
From: Srinivas Kandagatla
Date: Mon May 26 2014 - 18:39:20 EST
Hi Ulf,
Thankyou for the comments.
On 26/05/14 15:21, Ulf Hansson wrote:
On 23 May 2014 14:52, <srinivas.kandagatla@xxxxxxxxxx> wrote:
From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
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@xxxxxxxxxx>
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
---
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/