Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation

[Emil Renner Berthing <emil.renner.berthing@canonical.com>]

On Wed, 30 Aug 2023 at 05:21, William Qiu <william.qiu@starfivetech.com> wrote:
>
> Before, we used syscon to achieve tuning, but the actual measurement
> showed little effect, so the tuning implementation was modified here,
> and it was realized by reading and writing the UHS_REG_EXT register.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  drivers/mmc/host/dw_mmc-starfive.c | 131 ++++++++---------------------
>  1 file changed, 37 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
> index fd05a648a8bb..593c995e49f5 100644
> --- a/drivers/mmc/host/dw_mmc-starfive.c
> +++ b/drivers/mmc/host/dw_mmc-starfive.c
> @@ -20,14 +20,6 @@
>  #define ALL_INT_CLR            0x1ffff
>  #define MAX_DELAY_CHAIN                32
>
> -struct starfive_priv {
> -       struct device *dev;
> -       struct regmap *reg_syscon;
> -       u32 syscon_offset;
> -       u32 syscon_shift;
> -       u32 syscon_mask;
> -};
> -
>  static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>  {
>         int ret;
> @@ -44,117 +36,68 @@ static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>         }
>  }
>
> +static void dw_mci_starfive_hs_set_bits(struct dw_mci *host, u32 smpl_phase)

"set bits" is very generic. Maybe dw_mci_starfive_set_sample_phase()
or something more descriptive.

> +{
> +       /* change driver phase and sample phase */
> +       u32 mask = 0x1f;
> +       u32 reg_value;
> +
> +       reg_value = mci_readl(host, UHS_REG_EXT);
> +
> +       /* In UHS_REG_EXT, only 5 bits valid in DRV_PHASE and SMPL_PHASE */
> +       reg_value &= ~(mask << 16);
> +       reg_value |= (smpl_phase << 16);
> +       mci_writel(host, UHS_REG_EXT, reg_value);
> +
> +       /* We should delay 1ms wait for timing setting finished. */
> +       mdelay(1);
> +}

This implementation could use some cleanup. Eg. why do we need the
mask variable?
How about something like this:

#define STARFIVE_SMPL_PHASE     GENMASK(20, 16)

u32 reg_value = mci_read(host, UHS_REG_EXT);
reg_value &= ~STARFIVE_SMPL_PHASE;
reg_value |= FIELD_PREP(STARFIVE_SMPL_PHASE, smpl_phase);
mci_writel(host, UHS_REG_EXT, reg_value);
...

>  static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>                                              u32 opcode)
>  {
>         static const int grade  = MAX_DELAY_CHAIN;
>         struct dw_mci *host = slot->host;
> -       struct starfive_priv *priv = host->priv;
> -       int rise_point = -1, fall_point = -1;
> -       int err, prev_err = 0;
> +       int err = -1;

This variable is always set later so doesn't need initialization and
is better called 'ret' as it's the return value of the function, and
not necessarily an error.

> +       int smpl_phase, smpl_raise = -1, smpl_fall = -1;
>         int i;
> -       bool found = 0;
> -       u32 regval;
> -
> -       /*
> -        * Use grade as the max delay chain, and use the rise_point and
> -        * fall_point to ensure the best sampling point of a data input
> -        * signals.
> -        */
> +
>         for (i = 0; i < grade; i++) {
> -               regval = i << priv->syscon_shift;
> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> -                                               priv->syscon_mask, regval);
> -               if (err)
> -                       return err;
> +               smpl_phase = i;

This can now be written

for (sampl_phase = 0; sampl_phase < grade; sampl_phase++)

> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
>                 mci_writel(host, RINTSTS, ALL_INT_CLR);
>
>                 err = mmc_send_tuning(slot->mmc, opcode, NULL);
> -               if (!err)
> -                       found = 1;
> -
> -               if (i > 0) {
> -                       if (err && !prev_err)
> -                               fall_point = i - 1;
> -                       if (!err && prev_err)
> -                               rise_point = i;
> -               }
>
> -               if (rise_point != -1 && fall_point != -1)
> -                       goto tuning_out;
> -
> -               prev_err = err;
> -               err = 0;
> -       }
> -
> -tuning_out:
> -       if (found) {
> -               if (rise_point == -1)
> -                       rise_point = 0;
> -               if (fall_point == -1)
> -                       fall_point = grade - 1;
> -               if (fall_point < rise_point) {
> -                       if ((rise_point + fall_point) >
> -                           (grade - 1))
> -                               i = fall_point / 2;
> -                       else
> -                               i = (rise_point + grade - 1) / 2;
> -               } else {
> -                       i = (rise_point + fall_point) / 2;
> +               if (!err && smpl_raise < 0) {
> +                       smpl_raise = i;
> +               } else if (err && smpl_raise >= 0) {
> +                       smpl_fall = i - 1;
> +                       break;
>                 }
> +       }
>
> -               regval = i << priv->syscon_shift;
> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> -                                               priv->syscon_mask, regval);
> -               if (err)
> -                       return err;
> -               mci_writel(host, RINTSTS, ALL_INT_CLR);
> +       if (i >= grade && smpl_raise >= 0)
> +               smpl_fall = grade - 1;
>
> -               dev_info(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
> -       } else {
> +       if (smpl_raise < 0) {
>                 dev_err(host->dev, "No valid delay chain! use default\n");
> +               dw_mci_starfive_hs_set_bits(host, 0);
>                 err = -EINVAL;
> +       } else {
> +               smpl_phase = (smpl_raise + smpl_fall) / 2;
> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
> +               dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", smpl_phase);
> +               err = 0;
>         }

Maybe something like:

  if (smpl_raise < 0) {
    smpl_phase = 0;
    dev_err(host->dev, "No valid delay chain, using default\n");
    ret = -EINVAL;
    goto out;
  }

  smpl_phase = (smpl_raise + smpl_fall) / 2;
  dev_dbg(...);
  ret = 0;

out:
  dw_mci_starfive_hs_set_bits(host, smpl_phase);
  mci_writel(host, RINTSTS, ALL_INT_CLR);
  return ret;
>  }
>
> -static int dw_mci_starfive_parse_dt(struct dw_mci *host)
> -{
> -       struct of_phandle_args args;
> -       struct starfive_priv *priv;
> -       int ret;
> -
> -       priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> -       if (!priv)
> -               return -ENOMEM;
> -
> -       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> -                                               "starfive,sysreg", 3, 0, &args);
> -       if (ret) {
> -               dev_err(host->dev, "Failed to parse starfive,sysreg\n");
> -               return -EINVAL;
> -       }
> -
> -       priv->reg_syscon = syscon_node_to_regmap(args.np);
> -       of_node_put(args.np);
> -       if (IS_ERR(priv->reg_syscon))
> -               return PTR_ERR(priv->reg_syscon);
> -
> -       priv->syscon_offset = args.args[0];
> -       priv->syscon_shift  = args.args[1];
> -       priv->syscon_mask   = args.args[2];
> -
> -       host->priv = priv;
> -
> -       return 0;
> -}
> -
>  static const struct dw_mci_drv_data starfive_data = {
>         .common_caps            = MMC_CAP_CMD23,
>         .set_ios                = dw_mci_starfive_set_ios,
> -       .parse_dt               = dw_mci_starfive_parse_dt,
>         .execute_tuning         = dw_mci_starfive_execute_tuning,
>  };
>
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv