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

From: William Qiu
Date: Thu Aug 31 2023 - 22:40:56 EST

Next message: Yuan Tan: "[PATCH v2 0/3] Add dependencies of POWER_RESET for MIPS Malta, x86, and PowerMac"
Previous message: JiaJie Ho: "RE: starfive crypto list_add corruption"
In reply to: Emil Renner Berthing: "Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation"
Next in thread: William Qiu: "[PATCH v1 3/3] riscv: dts: starfive: Drop unused properties and limit frquency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2023/8/30 18:28, Emil Renner Berthing wrote:
> On Wed, 30 Aug 2023 at 05:21, William Qiu <william.qiu@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Next message: Yuan Tan: "[PATCH v2 0/3] Add dependencies of POWER_RESET for MIPS Malta, x86, and PowerMac"
Previous message: JiaJie Ho: "RE: starfive crypto list_add corruption"
In reply to: Emil Renner Berthing: "Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation"
Next in thread: William Qiu: "[PATCH v1 3/3] riscv: dts: starfive: Drop unused properties and limit frquency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]