| Message ID | 20230830031846.127957-1-william.qiu@starfivetech.com |
|---|---|
| Headers | show |
| Series | Change tuning implementation | expand |
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
On 2023/8/30 18:28, Emil Renner Berthing wrote: > 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. > Will update. >> +{ >> + /* 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); > ... > I'll try it. >> 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. Will update. > >> + 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++) > Will update. >> + 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; >> } >> I'll try it. Thanks for taking time to review this patch series and giving so much helpful suggestions. Best regards, William >> -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