| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFqz-O9-Douk96LBuFRD_P1146c_Ld14TgiduBgfBqRi_A@mail.gmail.com>
Date: Mon, 26 May 2014 16:21:26 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: Russell King <linux@....linux.org.uk>,
linux-mmc <linux-mmc@...r.kernel.org>,
Chris Ball <chris@...ntf.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-arm-msm@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control
On 23 May 2014 14:52, <srinivas.kandagatla@...aro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.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@...aro.org>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists