Re: [PATCH v4 12/13] mmc: mmci: add explicit clk control

From: Ulf Hansson
Date: Fri May 30 2014 - 06:25:24 EST

Next message: Ulf Hansson: "Re: [PATCH v4 11/13] mmc: mmci: add f_max to variant structure"
Previous message: Sachin Kamat: "[PATCH 1/1] pinctrl: sunxi: Fix potential null pointer dereference"
In reply to: srinivas . kandagatla: "[PATCH v4 12/13] mmc: mmci: add explicit clk control"
Next in thread: Srinivas Kandagatla: "Re: [PATCH v4 12/13] mmc: mmci: add explicit clk control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 28 May 2014 15:47, <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 flag in variant structure giving more flexibility
> to the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> drivers/mmc/host/mmci.c | 96 ++++++++++++++++++++++++++++++++-----------------
> drivers/mmc/host/mmci.h | 2 ++
> 2 files changed, 65 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 202f2d5..6eb0a29 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -72,6 +72,7 @@ static unsigned int fmax = 515633;
> * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
> * @busy_detect: true if busy detection on dat0 is supported
> * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
> + * @explicit_mclk_control: enable explicit mclk control in driver.
> */
> struct variant_data {
> unsigned int clkreg;
> @@ -93,6 +94,7 @@ struct variant_data {
> bool pwrreg_clkgate;
> bool busy_detect;
> bool pwrreg_nopower;
> + bool explicit_mclk_control;
> };
>
> static struct variant_data variant_arm = {
> @@ -199,6 +201,7 @@ static struct variant_data variant_qcom = {
> .datalength_bits = 24,
> .pwrreg_powerup = MCI_PWR_UP,
> .f_max = 208000000,
> + .explicit_mclk_control = true,

This should go in patch3 instead.

> };
>
> static int mmci_card_busy(struct mmc_host *mmc)
> @@ -301,7 +304,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
> host->cclk = 0;
>
> if (desired) {
> - if (desired >= host->mclk) {
> + if (variant->explicit_mclk_control) {
> + host->cclk = host->mclk;
> + } else if (desired >= host->mclk) {
> clk = MCI_CLK_BYPASS;
> if (variant->st_clkdiv)
> clk |= MCI_ST_UX500_NEG_EDGE;
> @@ -1340,6 +1345,18 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (!ios->clock && variant->pwrreg_clkgate)
> pwr &= ~MCI_PWR_ON;
>
> + if ((host->variant->explicit_mclk_control) &&
> + (ios->clock != host->mclk_req)) {
> + 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);
> + host->mclk_req = ios->clock;
> + }
> + }
> +
> spin_lock_irqsave(&host->lock, flags);
>
> mmci_set_clkreg(host, ios->clock);
> @@ -1490,19 +1507,6 @@ static int mmci_probe(struct amba_device *dev,
> host->plat = plat;
> host->variant = variant;
> host->mclk = clk_get_rate(host->clk);
> - /*
> - * According to the spec, mclk is max 100 MHz,
> - * so we try to adjust the clock down to this,
> - * (if possible).
> - */
> - if (host->mclk > host->variant->f_max) {
> - ret = clk_set_rate(host->clk, host->variant->f_max);
> - if (ret < 0)
> - goto clk_disable;
> - host->mclk = clk_get_rate(host->clk);
> - dev_dbg(mmc_dev(mmc), "eventual mclk rate: %u Hz\n",
> - host->mclk);
> - }

The above code is relevant for Qcom as well, I don't think you need
change/move it.

>
> host->phybase = dev->res.start;
> host->base = devm_ioremap_resource(&dev->dev, &dev->res);
> @@ -1511,25 +1515,51 @@ static int mmci_probe(struct amba_device *dev,
> goto clk_disable;
> }
>
> - /*
> - * The ARM and ST versions of the block have slightly different
> - * clock divider equations which means that the minimum divider
> - * differs too.
> - */
> - if (variant->st_clkdiv)
> - mmc->f_min = DIV_ROUND_UP(host->mclk, 257);

I think you could simplify the change of fixing with f_min and f_max.

Start by, just add another statement here "else if
(variant->explicit_mclk_control)" and do the clk_round_rate()

> - else
> - mmc->f_min = DIV_ROUND_UP(host->mclk, 512);
> - /*
> - * If no maximum operating frequency is supplied, fall back to use
> - * the module parameter, which has a (low) default value in case it
> - * 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);

Further replace the above with:
if (mmc->f_max)
mmc->f_max = variant->explicit_mclk_control ? min(mmc->f_max,
variant->f_max) : min(host->mclk, mmc->f_max);
else
mmc->f_max = variant->explicit_mclk_control ? fmax : min(host->mclk, fmax);

That should do it, right?

> + if (variant->explicit_mclk_control) {
> + /* get the nearest minimum clock to 100Khz */
> + mmc->f_min = clk_round_rate(host->clk, 100000);
> +
> + if (mmc->f_max)
> + mmc->f_max = min(host->variant->f_max, mmc->f_max);
> + else
> + mmc->f_max = min(host->variant->f_max, fmax);
> +
> + } else {
> + /*
> + * According to the spec, mclk is max 100 MHz,
> + * so we try to adjust the clock down to this,
> + * (if possible).
> + */
> + if (host->mclk > host->variant->f_max) {
> + ret = clk_set_rate(host->clk, host->variant->f_max);
> + if (ret < 0)
> + goto clk_disable;
> + host->mclk = clk_get_rate(host->clk);
> + dev_dbg(mmc_dev(mmc), "eventual mclk rate: %u Hz\n",
> + host->mclk);
> + }
> + /*
> + * The ARM and ST versions of the block have slightly different
> + * clock divider equations which means that the minimum divider
> + * differs too.
> + */
> + if (variant->st_clkdiv)
> + mmc->f_min = DIV_ROUND_UP(host->mclk, 257);
> + else
> + mmc->f_min = DIV_ROUND_UP(host->mclk, 512);
> + /*
> + * If no maximum operating frequency is supplied, fall back to
> + * use the module parameter, which has a (low) default value in
> + * case it 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);
> +
> + }
> +
> dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>
> /* Get regulators and the supported OCR mask */
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 706eb513..1882e20 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -208,6 +208,8 @@ struct mmci_host {
> spinlock_t lock;
>
> unsigned int mclk;
> + /* cached value of requested clk in set_ios */
> + unsigned int mclk_req;

How about "clock_cache" instead?

> unsigned int cclk;
> u32 pwr_reg;
> u32 pwr_reg_add;
> --
> 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/

Next message: Ulf Hansson: "Re: [PATCH v4 11/13] mmc: mmci: add f_max to variant structure"
Previous message: Sachin Kamat: "[PATCH 1/1] pinctrl: sunxi: Fix potential null pointer dereference"
In reply to: srinivas . kandagatla: "[PATCH v4 12/13] mmc: mmci: add explicit clk control"
Next in thread: Srinivas Kandagatla: "Re: [PATCH v4 12/13] mmc: mmci: add explicit clk control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]