| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJM55Z9CL8DN+uEhRoR7ZUuwtVudTUzA1+Q4Hn_rukCfT+SXeQ@mail.gmail.com>
Date: Wed, 30 Aug 2023 12:28:08 +0200
From: Emil Renner Berthing <emil.renner.berthing@...onical.com>
To: William Qiu <william.qiu@...rfivetech.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, linux-mmc@...r.kernel.org,
Emil Renner Berthing <kernel@...il.dk>,
Rob Herring <robh+dt@...nel.org>,
Jaehoon Chung <jh80.chung@...sung.com>,
Ulf Hansson <ulf.hansson@...aro.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation
On Wed, 30 Aug 2023 at 05:21, William Qiu <william.qiu@...rfivetech.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@...rfivetech.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Powered by blists - more mailing lists