Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230809052952.323-1-wenchao.chen@unisoc.com>
In-Reply-To: <20230809052952.323-1-wenchao.chen@unisoc.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 9 Aug 2023 12:08:41 +0200
Message-ID: <CAPDyKFoykxdP70t2pjeiX0pkKuUCZ2AeFM4yT4-wfVijxB7OHw@mail.gmail.com>
Subject: Re: [PATCH] mmc: core: Add host specific tuning support for SD HS mode
To:     Wenchao Chen <wenchao.chen@unisoc.com>
Cc:     adrian.hunter@intel.com, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, wenchao.chen666@gmail.com,
        zhenxiong.lai@unisoc.com, chunyan.zhang@unisoc.com,
        yuelin.tang@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 9 Aug 2023 at 07:30, Wenchao Chen <wenchao.chen@unisoc.com> wrote:
>
> Added .prepare_hs_tuning and .execute_hs_tuning host
> callbacks to support host-specific tuning for SD high
> speed mode.

This certainly needs to be clarified more. Especially why it's needed
for the sdhci-sprd variant.

>
> Signed-off-by: Wenchao Chen <wenchao.chen@unisoc.com>
> ---
>  drivers/mmc/core/sd.c         |  12 ++++
>  drivers/mmc/host/sdhci-sprd.c | 124 ++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h      |   6 ++
>  3 files changed, 142 insertions(+)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 246ce027ae0a..ac2da8f2fbce 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1518,6 +1518,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>                  */
>                 mmc_set_clock(host, mmc_sd_get_max_clock(card));
>
> +               if (host->ops->prepare_hs_tuning) {
> +                       err = host->ops->prepare_hs_tuning(host, card);
> +                       if (err)
> +                               goto free_card;
> +               }

Adding a new callback for this is a bit questionable, I think.

In the ->set_ios() callback, we could instead check MMC_TIMING_SD_HS
and when the clock is updated, then also run a tuning sequence, right?

> +
>                 /*
>                  * Switch to wider bus (if supported).
>                  */
> @@ -1529,6 +1535,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>
>                         mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
>                 }
> +
> +               if (host->ops->execute_hs_tuning) {
> +                       err = host->ops->execute_hs_tuning(host, card);
> +                       if (err)
> +                               goto free_card;
> +               }

Similar to the above comment, in the ->set_ios() callback we could
instead check MMC_TIMING_SD_HS when moving to MMC_BUS_WIDTH_4, then
run a tuning sequence, right?

>         }
>  cont:
>         if (!oldcard) {
> diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> index 7f4ee2e12735..5f365dcbb9c7 100644
> --- a/drivers/mmc/host/sdhci-sprd.c
> +++ b/drivers/mmc/host/sdhci-sprd.c
> @@ -9,6 +9,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/highmem.h>
>  #include <linux/iopoll.h>
> +#include <linux/mmc/mmc.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -22,6 +23,9 @@
>  #include "sdhci-pltfm.h"
>  #include "mmc_hsq.h"
>
> +#include "../core/mmc_ops.h"
> +#include "../core/sd_ops.h"

No, this isn't how we include header files. Instead move the functions
that you may need to include/linux/mmc/host.h.

Also, please split up core changes from host driver changes.

> +
>  /* SDHCI_ARGUMENT2 register high 16bit */
>  #define SDHCI_SPRD_ARG2_STUFF          GENMASK(31, 16)
>
> @@ -73,6 +77,11 @@
>  #define SDHCI_SPRD_CLK_DEF_RATE                26000000
>  #define SDHCI_SPRD_PHY_DLL_CLK         52000000
>
> +#define SDHCI_SPRD_MAX_PHASE           0xff
> +#define SDHCI_SPRD_CMD_DLY_MASK                GENMASK(15, 8)
> +#define SDHCI_SPRD_POSRD_DLY_MASK      GENMASK(23, 16)
> +#define SDHCI_SPRD_CPST_EN             GENMASK(27, 24)
> +
>  struct sdhci_sprd_host {
>         u32 version;
>         struct clk *clk_sdio;
> @@ -86,6 +95,11 @@ struct sdhci_sprd_host {
>         u32 phy_delay[MMC_TIMING_MMC_HS400 + 2];
>  };
>
> +enum sdhci_sprd_tuning_type {
> +       SDHCI_SPRD_TUNING_SD_HS_CMD,
> +       SDHCI_SPRD_TUNING_SD_HS_DATA,
> +};
> +
>  struct sdhci_sprd_phy_cfg {
>         const char *property;
>         u8 timing;
> @@ -533,6 +547,111 @@ static void sdhci_sprd_hs400_enhanced_strobe(struct mmc_host *mmc,
>                      SDHCI_SPRD_REG_32_DLL_DLY);
>  }
>
> +static int mmc_send_tuning_cmd(struct mmc_card *card)
> +{
> +       return mmc_send_status(card, NULL);
> +}
> +
> +static int mmc_send_tuning_data(struct mmc_card *card)
> +{
> +       u8 status[64];

We use kmalloc-ed data for data transfers.

> +
> +       return mmc_sd_switch(card, 0, 0, 0, status);
> +}
> +
> +static int sdhci_sprd_tuning(struct mmc_host *mmc, struct mmc_card *card,
> +                       enum sdhci_sprd_tuning_type type)
> +{
> +       struct sdhci_host *host = mmc_priv(mmc);
> +       struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
> +       u32 *p = sprd_host->phy_delay;
> +       int err = 0;
> +       int i;
> +       bool value;
> +       int final_phase;
> +       u32 dll_cfg, dll_dly;
> +       int range_end = SDHCI_SPRD_MAX_PHASE;
> +       int len = 0;
> +       int count = 0;
> +
> +       sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> +
> +       dll_cfg = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_CFG);
> +       dll_cfg &= ~SDHCI_SPRD_CPST_EN;
> +       sdhci_writel(host, dll_cfg, SDHCI_SPRD_REG_32_DLL_CFG);
> +
> +       dll_dly = p[mmc->ios.timing];
> +
> +       for (i = 0; i <= SDHCI_SPRD_MAX_PHASE; i++) {
> +               if (type == SDHCI_SPRD_TUNING_SD_HS_CMD) {
> +                       dll_dly &= ~SDHCI_SPRD_CMD_DLY_MASK;
> +                       dll_dly |= ((i << 8) & SDHCI_SPRD_CMD_DLY_MASK);
> +               } else {
> +                       dll_dly &= ~SDHCI_SPRD_POSRD_DLY_MASK;
> +                       dll_dly |= ((i << 16) & SDHCI_SPRD_POSRD_DLY_MASK);
> +               }
> +
> +               sdhci_writel(host, dll_dly, SDHCI_SPRD_REG_32_DLL_DLY);
> +
> +               if (type == SDHCI_SPRD_TUNING_SD_HS_CMD)
> +                       value = !mmc_send_tuning_cmd(card);
> +               else
> +                       value = !mmc_send_tuning_data(card);
> +
> +               if (value) {
> +                       dev_dbg(mmc_dev(host->mmc), "tuning ok: %d\n", i);
> +                       count++;
> +               } else {
> +                       dev_dbg(mmc_dev(host->mmc), "tuning fail: %d\n", i);
> +                       if (len < count) {
> +                               len = count;
> +                               range_end = i - 1;
> +                               count = 0;
> +                       }
> +               }
> +       }
> +
> +       if (!count) {
> +               final_phase = 0;
> +               dev_err(mmc_dev(host->mmc), "all tuning phase fail!\n");
> +               err = -EIO;
> +               goto out;
> +       }
> +
> +       if (count > len) {
> +               len = count;
> +               range_end = i - 1;
> +       }
> +
> +       final_phase = range_end - (len - 1) / 2;

The whole len, count, range_end, final_phase things look rather messy.
Please have a look and try to clean up that part a bit, I am sure it
can be done, somehow.

> +
> +       if (type == SDHCI_SPRD_TUNING_SD_HS_CMD) {
> +               p[mmc->ios.timing] &= ~SDHCI_SPRD_CMD_DLY_MASK;
> +               p[mmc->ios.timing] |= ((final_phase << 8) & SDHCI_SPRD_CMD_DLY_MASK);
> +       } else {
> +               p[mmc->ios.timing] &= ~(SDHCI_SPRD_POSRD_DLY_MASK);
> +               p[mmc->ios.timing] |= ((final_phase << 16) & SDHCI_SPRD_POSRD_DLY_MASK);
> +       }
> +
> +       dev_info(mmc_dev(host->mmc), "the best step %d, phase 0x%02x, delay value 0x%08x\n",
> +                       final_phase, final_phase, p[mmc->ios.timing]);

Does this really deserve to be a dev_info? Looks like a dev_dbg to me, no?

> +
> +out:
> +       sdhci_writel(host, p[mmc->ios.timing], SDHCI_SPRD_REG_32_DLL_DLY);
> +
> +       return err;
> +}
> +
> +static int sdhci_sprd_prepare_hs_cmd_tuning(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +       return sdhci_sprd_tuning(mmc, card, SDHCI_SPRD_TUNING_SD_HS_CMD);
> +}
> +
> +static int sdhci_sprd_execute_hs_data_tuning(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +       return sdhci_sprd_tuning(mmc, card, SDHCI_SPRD_TUNING_SD_HS_DATA);
> +}
> +
>  static void sdhci_sprd_phy_param_parse(struct sdhci_sprd_host *sprd_host,
>                                        struct device_node *np)
>  {
> @@ -577,6 +696,11 @@ static int sdhci_sprd_probe(struct platform_device *pdev)
>         host->mmc_host_ops.request = sdhci_sprd_request;
>         host->mmc_host_ops.hs400_enhanced_strobe =
>                 sdhci_sprd_hs400_enhanced_strobe;
> +       host->mmc_host_ops.prepare_hs_tuning =
> +               sdhci_sprd_prepare_hs_cmd_tuning;
> +       host->mmc_host_ops.execute_hs_tuning =
> +               sdhci_sprd_execute_hs_data_tuning;
> +
>         /*
>          * We can not use the standard ops to change and detect the voltage
>          * signal for Spreadtrum SD host controller, since our voltage regulator
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 461d1543893b..13cf894b9e3c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -184,6 +184,12 @@ struct mmc_host_ops {
>         /* Execute HS400 tuning depending host driver */
>         int     (*execute_hs400_tuning)(struct mmc_host *host, struct mmc_card *card);
>
> +       /* Prepare HS tuning depending host driver */
> +       int     (*prepare_hs_tuning)(struct mmc_host *host, struct mmc_card *card);
> +
> +       /* Execute HS tuning depending host driver */
> +       int     (*execute_hs_tuning)(struct mmc_host *host, struct mmc_card *card);
> +
>         /* Prepare switch to DDR during the HS400 init sequence */
>         int     (*hs400_prepare_ddr)(struct mmc_host *host);
>

Kind regards
Uffe