2023-08-30 18:31:40

by William Qiu

[permalink] [raw]
Subject: [PATCH v1 2/3] mmc: starfive: Change tuning implementation

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 <[email protected]>
---
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)
+{
+ /* 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);
+}
+
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;
+ 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;
+ 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;
}

mci_writel(host, RINTSTS, ALL_INT_CLR);
return err;
}

-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



2023-09-01 05:37:57

by William Qiu

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



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