Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1583307441.git.baolin.wang7@gmail.com> <ace53bca354e2846f19684bd33a9c0f3c2ee2c44.1583307441.git.baolin.wang7@gmail.com>
In-Reply-To: <ace53bca354e2846f19684bd33a9c0f3c2ee2c44.1583307441.git.baolin.wang7@gmail.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Mon, 16 Mar 2020 12:47:39 +0100
Message-ID: <CAPDyKFp-HvMtEX4F-aumBy93DMn8aPjx8kBm+rCSL316HFPL0w@mail.gmail.com>
Subject: Re: [RESEND PATCH 1/3] mmc: host: Introduce the request_atomic() for
 the host
To:     Baolin Wang <baolin.wang7@gmail.com>
Cc:     Adrian Hunter <adrian.hunter@intel.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 4 Mar 2020 at 08:42, Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> The SD host controller can process one request in the atomic context if
> the card is nonremovable, which means we can submit next request in the
> irq hard handler when using the MMC software queue to reduce the latency.
> Thus this patch adds a new API request_atomic() for the host controller
> and implement it for the SD host controller.
>
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> ---
>  drivers/mmc/host/sdhci.c | 27 +++++++++++++++++++--------
>  drivers/mmc/host/sdhci.h |  1 +
>  include/linux/mmc/host.h |  3 +++
>  3 files changed, 23 insertions(+), 8 deletions(-)

I think the code split of the changes in the series can be improved a
bit, so I suggest you move the code around in the series to reach
this:

1. A patch that adds the new host ops callback, combined with the
change you have in patch3.
2. The sdhci core specific changes, from $subject patch.
3. The sdhci-sprd changes, as in patch2.

Other than that, I think the code looks good to me, besides a minor
comment on patch2, see separate reply.

Kind regards
Uffe


>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9c37451..4febbcb 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2016,17 +2016,12 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>   *                                                                           *
>  \*****************************************************************************/
>
> -void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +static void sdhci_start_request(struct mmc_host *mmc, struct mmc_request *mrq,
> +                               int present)
>  {
> -       struct sdhci_host *host;
> -       int present;
> +       struct sdhci_host *host = mmc_priv(mmc);
>         unsigned long flags;
>
> -       host = mmc_priv(mmc);
> -
> -       /* Firstly check card presence */
> -       present = mmc->ops->get_cd(mmc);
> -
>         spin_lock_irqsave(&host->lock, flags);
>
>         sdhci_led_activate(host);
> @@ -2043,6 +2038,22 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>         spin_unlock_irqrestore(&host->lock, flags);
>  }
> +
> +void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       sdhci_start_request(mmc, mrq, 1);
> +}
> +EXPORT_SYMBOL_GPL(sdhci_request_atomic);
> +
> +void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       int present;
> +
> +       /* Firstly check card presence */
> +       present = mmc->ops->get_cd(mmc);
> +
> +       sdhci_start_request(mmc, mrq, present);
> +}
>  EXPORT_SYMBOL_GPL(sdhci_request);
>
>  void sdhci_set_bus_width(struct sdhci_host *host, int width)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index cac2d97..5507a73 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -775,6 +775,7 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>  void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
>                            unsigned short vdd);
>  void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
> +void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
>  void sdhci_set_bus_width(struct sdhci_host *host, int width);
>  void sdhci_reset(struct sdhci_host *host, u8 mask);
>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 562ed06..db5e59c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -92,6 +92,9 @@ struct mmc_host_ops {
>                             int err);
>         void    (*pre_req)(struct mmc_host *host, struct mmc_request *req);
>         void    (*request)(struct mmc_host *host, struct mmc_request *req);
> +       /* Submit one request to host in atomic context. */
> +       void    (*request_atomic)(struct mmc_host *host,
> +                                 struct mmc_request *req);
>
>         /*
>          * Avoid calling the next three functions too often or in a "fast
> --
> 1.9.1
>