From: William Qiu <william.qiu@starfivetech.com>
To: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Cc: <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-riscv@lists.infradead.org>, <linux-mmc@vger.kernel.org>,
"Emil Renner Berthing" <kernel@esmil.dk>,
Rob Herring <robh+dt@kernel.org>,
Jaehoon Chung <jh80.chung@samsung.com>,
Ulf Hansson <ulf.hansson@linaro.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>
Subject: Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation
Date: Fri, 1 Sep 2023 10:40:41 +0800 [thread overview]
Message-ID: <f130f1c5-d59b-6d5c-e2e8-5ce3e6948543@starfivetech.com> (raw)
In-Reply-To: <CAJM55Z9CL8DN+uEhRoR7ZUuwtVudTUzA1+Q4Hn_rukCfT+SXeQ@mail.gmail.com>
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-09-01 2:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-30 3:18 [PATCH v1 0/3] Change tuning implementation William Qiu
2023-08-30 3:18 ` [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties William Qiu
2023-08-30 6:50 ` Conor Dooley
2023-08-30 7:29 ` Krzysztof Kozlowski
2023-08-30 8:34 ` Conor Dooley
2023-09-01 2:33 ` William Qiu
2023-09-01 15:42 ` Conor Dooley
2023-09-01 17:20 ` Jessica Clarke
2023-09-01 17:43 ` Conor Dooley
2023-09-01 18:39 ` Jessica Clarke
2023-09-08 10:01 ` William Qiu
2023-09-08 13:32 ` Emil Renner Berthing
2023-09-11 16:14 ` Conor Dooley
2023-09-12 2:00 ` William Qiu
2023-08-30 3:18 ` [PATCH v1 2/3] mmc: starfive: Change tuning implementation William Qiu
2023-08-30 10:28 ` Emil Renner Berthing
2023-09-01 2:40 ` William Qiu [this message]
2023-08-30 3:18 ` [PATCH v1 3/3] riscv: dts: starfive: Drop unused properties and limit frquency William Qiu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f130f1c5-d59b-6d5c-e2e8-5ce3e6948543@starfivetech.com \
--to=william.qiu@starfivetech.com \
--cc=aou@eecs.berkeley.edu \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=emil.renner.berthing@canonical.com \
--cc=jh80.chung@samsung.com \
--cc=kernel@esmil.dk \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh+dt@kernel.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).