linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Ludovic Barre <ludovic.Barre@st.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Gerald Baeza <gerald.baeza@st.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>
Subject: Re: [PATCH 05/19] mmc: mmci: allow to overwrite clock/power procedure to specific variant
Date: Thu, 5 Jul 2018 15:48:02 +0200	[thread overview]
Message-ID: <CAPDyKFpwVMX9LoC_abHm09UNhLse8MJBUpCCHkR8NnK7fOU5Yg@mail.gmail.com> (raw)
In-Reply-To: <1528809280-31116-6-git-send-email-ludovic.Barre@st.com>

On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
>
> A specific variant could have different power or clock procedures.
> This patch allows to overwrite the default mmci_set_clkreg and
> mmci_set_pwrreg for a specific variant.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
>  drivers/mmc/host/mmci.c | 96 +++++++++++++++++++++++++++++--------------------
>  drivers/mmc/host/mmci.h |  7 ++++
>  2 files changed, 64 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index ede95b7..801c86b 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -374,6 +374,52 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>         mmci_write_clkreg(host, clk);
>  }
>
> +static void mmci_set_pwrreg(struct mmci_host *host, unsigned char power_mode,
> +                           unsigned int pwr)
> +{
> +       struct variant_data *variant = host->variant;
> +       struct mmc_host *mmc = host->mmc;
> +
> +       switch (power_mode) {
> +       case MMC_POWER_OFF:
> +               if (!IS_ERR(mmc->supply.vmmc))
> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> +
> +               if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) {
> +                       regulator_disable(mmc->supply.vqmmc);
> +                       host->vqmmc_enabled = false;
> +               }
> +
> +               break;
> +       case MMC_POWER_UP:
> +               if (!IS_ERR(mmc->supply.vmmc))
> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +                                             mmc->ios.vdd);
> +
> +               /*
> +                * The ST Micro variant doesn't have the PL180s MCI_PWR_UP
> +                * and instead uses MCI_PWR_ON so apply whatever value is
> +                * configured in the variant data.
> +                */
> +               pwr |= variant->pwrreg_powerup;
> +
> +               break;
> +       case MMC_POWER_ON:
> +               if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) {
> +                       if (regulator_enable(mmc->supply.vqmmc) < 0)
> +                               dev_err(mmc_dev(mmc),
> +                                       "failed to enable vqmmc regulator\n");
> +                       else
> +                               host->vqmmc_enabled = true;
> +               }
> +
> +               pwr |= MCI_PWR_ON;
> +               break;
> +       }
> +
> +       mmci_write_pwrreg(host, pwr);
> +}
> +
>  static void
>  mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
>  {
> @@ -1031,7 +1077,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct mmci_host *host = mmc_priv(mmc);
>         struct variant_data *variant = host->variant;
> -       u32 pwr = 0;
> +       unsigned int pwr = 0;

?

>         unsigned long flags;
>         int ret;
>
> @@ -1039,42 +1085,6 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                 host->plat->ios_handler(mmc_dev(mmc), ios))
>                         dev_err(mmc_dev(mmc), "platform ios_handler failed\n");
>
> -       switch (ios->power_mode) {
> -       case MMC_POWER_OFF:
> -               if (!IS_ERR(mmc->supply.vmmc))
> -                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> -
> -               if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) {
> -                       regulator_disable(mmc->supply.vqmmc);
> -                       host->vqmmc_enabled = false;
> -               }
> -
> -               break;
> -       case MMC_POWER_UP:
> -               if (!IS_ERR(mmc->supply.vmmc))
> -                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
> -
> -               /*
> -                * The ST Micro variant doesn't have the PL180s MCI_PWR_UP
> -                * and instead uses MCI_PWR_ON so apply whatever value is
> -                * configured in the variant data.
> -                */
> -               pwr |= variant->pwrreg_powerup;
> -
> -               break;
> -       case MMC_POWER_ON:
> -               if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) {
> -                       ret = regulator_enable(mmc->supply.vqmmc);
> -                       if (ret < 0)
> -                               dev_err(mmc_dev(mmc),
> -                                       "failed to enable vqmmc regulator\n");
> -                       else
> -                               host->vqmmc_enabled = true;
> -               }
> -
> -               pwr |= MCI_PWR_ON;
> -               break;
> -       }

This above looks like pure re-factoring. Please make the above change
a separate patch.

>
>         if (variant->signal_direction && ios->power_mode != MMC_POWER_OFF) {
>                 /*
> @@ -1126,8 +1136,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
>         spin_lock_irqsave(&host->lock, flags);
>
> -       mmci_set_clkreg(host, ios->clock);
> -       mmci_write_pwrreg(host, pwr);
> +       if (variant->set_clkreg)
> +               variant->set_clkreg(host, ios->clock);
> +       else
> +               mmci_set_clkreg(host, ios->clock);

This means a change in behavior, which I don't think is what you want.

1) The spin lock will be held while doing the regulator operations.
2) The clock register will be written to before the regulator
operations have been done. Not sure if that works fine!?

An overall comment, I think we should create a mmci_host_ops structure
and put the needed callbacks in there (kept to a minimum of course),
rather than putting them as a part of the variant struct. More
importantly, also the legacy mmci variants should get a mmci_host_ops
structure assigned during probe.

The point is, I think it makes the code above (and future wise) more
flexible. It should also allow us to better share functions between
the variants. In principle, I expect that we end up with a bunch of
"library" mmci functions that can be invoked from variant's
mmci_host_ops (if not assigned directly).

> +
> +       if (variant->set_pwrreg)
> +               variant->set_pwrreg(host, ios->power_mode, pwr);
> +       else
> +               mmci_set_pwrreg(host, ios->power_mode, pwr);
> +
>         mmci_reg_delay(host);
>
>         spin_unlock_irqrestore(&host->lock, flags);
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 2ba9640..7265ca6 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -231,6 +231,7 @@
>
>  struct clk;
>  struct dma_chan;
> +struct mmci_host;
>
>  /**
>   * struct variant_data - MMCI variant-specific quirks
> @@ -273,6 +274,8 @@ struct dma_chan;
>   *            register.
>   * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register
>   * @mmci_dma: Pointer to platform-specific DMA callbacks.
> + * @set_clk_ios: if clock procedure of variant is specific
> + * @set_pwr_ios: if power procedure of variant is specific
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -307,6 +310,9 @@ struct variant_data {
>         u32                     start_err;
>         u32                     opendrain;
>         struct mmci_dma_ops     *mmci_dma;
> +       void (*set_clkreg)(struct mmci_host *host, unsigned int desired);
> +       void (*set_pwrreg)(struct mmci_host *host, unsigned char power_mode,
> +                          unsigned int pwr);
>  };
>
>  struct mmci_host {
> @@ -328,6 +334,7 @@ struct mmci_host {
>         u32                     pwr_reg;
>         u32                     pwr_reg_add;
>         u32                     clk_reg;
> +       u32                     clk_reg_add;

What's this? Some leftover I guess?

>         u32                     datactrl_reg;
>         u32                     busy_status;
>         u32                     mask1_reg;
> --
> 2.7.4
>

Kind regards
Uffe

  reply	other threads:[~2018-07-05 13:48 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-12 13:14 [PATCH 00/19] mmc: mmci: add stm32 sdmmc variant Ludovic Barre
2018-06-12 13:14 ` [PATCH 01/19] mmc: mmci: regroup and define dma operations Ludovic Barre
2018-07-05 15:17   ` Ulf Hansson
2018-07-11  9:41     ` Ludovic BARRE
2018-07-11 12:16       ` Ulf Hansson
2018-07-12  9:09         ` Ludovic BARRE
2018-06-12 13:14 ` [PATCH 02/19] mmc: mmci: merge qcom dml feature into mmci dma Ludovic Barre
2018-07-05 15:26   ` Ulf Hansson
2018-07-11 15:19     ` Ludovic BARRE
2018-07-13 11:17       ` Ulf Hansson
2018-07-13 13:08         ` Ludovic BARRE
2018-07-30 15:15           ` Ulf Hansson
2018-06-12 13:14 ` [PATCH 03/19] mmc: mmci: add datactrl block size variant property Ludovic Barre
2018-06-12 13:14 ` [PATCH 04/19] mmc: mmci: expand startbiterr to irqmask and error check Ludovic Barre
2018-06-12 13:14 ` [PATCH 05/19] mmc: mmci: allow to overwrite clock/power procedure to specific variant Ludovic Barre
2018-07-05 13:48   ` Ulf Hansson [this message]
2018-07-11 12:19     ` Ludovic BARRE
2018-07-11 12:38       ` Ulf Hansson
2018-06-12 13:14 ` [PATCH 06/19] mmc: mmci: add variant properties to define cpsm & cmdresp bits Ludovic Barre
2018-07-05 14:20   ` Ulf Hansson
2018-06-12 13:14 ` [PATCH 07/19] mmc: mmci: add variant property to define dpsm bit Ludovic Barre
2018-06-12 13:14 ` [PATCH 08/19] mmc: mmci: add variant property to define irq pio mask Ludovic Barre
2018-06-12 13:14 ` [PATCH 09/19] mmc: mmci: add variant property to write datactrl before command Ludovic Barre
2018-06-12 13:14 ` [PATCH 10/19] mmc: mmci: add variant property to allow remain data Ludovic Barre
2018-07-05 13:55   ` Ulf Hansson
2018-06-12 13:14 ` [PATCH 11/19] mmc: mmci: add variant property to check specific data constraint Ludovic Barre
2018-06-12 13:14 ` [PATCH 12/19] mmc: mmci: add variant property to request a reset Ludovic Barre
2018-06-25 21:23   ` Rob Herring
2018-06-12 13:14 ` [PATCH 13/19] mmc: mmci: send stop cmd if a data command fail Ludovic Barre
2018-07-04 13:37   ` Ulf Hansson
2018-07-11  8:57     ` Ludovic BARRE
2018-06-12 13:14 ` [PATCH 14/19] mmc: mmci: add clock divider for stm32 sdmmc Ludovic Barre
2018-06-12 13:14 ` [PATCH 15/19] mmc: mmci: add stm32 sdmmc registers Ludovic Barre
2018-06-12 13:14 ` [PATCH 16/19] mmc: mmci: add DT bindings for STM32 sdmmc Ludovic Barre
2018-06-25 21:47   ` Rob Herring
2018-06-12 13:14 ` [PATCH 17/19] mmc: mmci: add stm32 sdmmc idma support Ludovic Barre
2018-06-12 13:14 ` [PATCH 18/19] mmc: mmci: add specific clk/pwr procedure for stm32 sdmmc Ludovic Barre
2018-07-05 14:49   ` Ulf Hansson
2018-06-12 13:14 ` [PATCH 19/19] mmc: mmci: add stm32 sdmmc variant Ludovic Barre
2018-06-29 13:51 ` [PATCH 00/19] " Ludovic BARRE
2018-06-29 15:18   ` Ulf Hansson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPDyKFpwVMX9LoC_abHm09UNhLse8MJBUpCCHkR8NnK7fOU5Yg@mail.gmail.com \
    --to=ulf.hansson@linaro.org \
    --cc=alexandre.torgue@st.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gerald.baeza@st.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=ludovic.Barre@st.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).