linux-kernel - Re: [PATCH 2/8] mmc: sdhci-of-aspeed: Add runtime tuning

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <750e6e51e0943e50162851ca8df2befcea3e9bbe.camel@codeconstruct.com.au>
Date: Wed, 18 Jun 2025 12:01:57 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Cool Lee <cool_lee@...eedtech.com>, adrian.hunter@...el.com, 
	ulf.hansson@...aro.org, joel@....id.au, p.zabel@...gutronix.de, 
	linux-aspeed@...ts.ozlabs.org, openbmc@...ts.ozlabs.org, 
	linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8] mmc: sdhci-of-aspeed: Add runtime tuning

On Sun, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> Add support for runtime tuning in the Aspeed SDHCI driver.
> Using the timing phase register to adjust the clock phase with mmc
> tuning command to find the left and right boundary.
> 
> Signed-off-by: Cool Lee <cool_lee@...eedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 68 ++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 01bc574272eb..5e5ae1894456 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -24,6 +24,7 @@
>  #define ASPEED_SDC_PHASE               0xf4
>  #define   ASPEED_SDC_S1_PHASE_IN       GENMASK(25, 21)
>  #define   ASPEED_SDC_S0_PHASE_IN       GENMASK(20, 16)
> +#define   ASPEED_SDC_S0_PHASE_IN_SHIFT  16
>  #define   ASPEED_SDC_S1_PHASE_OUT      GENMASK(15, 11)
>  #define   ASPEED_SDC_S1_PHASE_IN_EN    BIT(10)
>  #define   ASPEED_SDC_S1_PHASE_OUT_EN   GENMASK(9, 8)
> @@ -375,6 +376,72 @@ static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask)
>         sdhci_reset(host, mask);
>  }
>  
> +static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +       struct sdhci_pltfm_host *pltfm_priv;
> +       struct aspeed_sdhci *sdhci;
> +       struct aspeed_sdc *sdc;
> +       struct device *dev;
> +
> +       u32 val, left, right, edge;
> +       u32 window, oldwindow = 0, center;
> +       u32 in_phase, out_phase, enable_mask, inverted = 0;
> +
> +       dev = mmc_dev(host->mmc);
> +       pltfm_priv = sdhci_priv(host);
> +       sdhci = sdhci_pltfm_priv(pltfm_priv);
> +       sdc = sdhci->parent;
> +
> +       out_phase = readl(sdc->regs + ASPEED_SDC_PHASE) & ASPEED_SDC_S0_PHASE_OUT;
> +
> +       enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN | ASPEED_SDC_S0_PHASE_IN_EN;
> +
> +       /*
> +        * There are two window upon clock rising and falling edge.
> +        * Iterate each tap delay to find the valid window and choose the
> +        * bigger one, set the tap delay at the middle of window.
> +        */
> +       for (edge = 0; edge < 2; edge++) {
> +               if (edge == 1)
> +                       inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> +
> +               val = (out_phase | enable_mask | (inverted << ASPEED_SDC_S0_PHASE_IN_SHIFT));
> +
> +               /* find the left boundary */
> +               for (left = 0; left < ASPEED_SDHCI_NR_TAPS + 1; left++) {

Bit of a nit, but maybe `left <= ASPEED_SDHCI_NR_TAPS` rather than + 1?

> +                       in_phase = val | (left << ASPEED_SDC_S0_PHASE_IN_SHIFT);
> +                       writel(in_phase, sdc->regs + ASPEED_SDC_PHASE);
> +
> +                       if (!mmc_send_tuning(host->mmc, opcode, NULL))
> +                               break;
> +               }
> +
> +               /* find the right boundary */
> +               for (right = left + 1; right < ASPEED_SDHCI_NR_TAPS + 1; right++) {

<= again here if you agree.

> +                       in_phase = val | (right << ASPEED_SDC_S0_PHASE_IN_SHIFT);
> +                       writel(in_phase, sdc->regs + ASPEED_SDC_PHASE);
> +
> +                       if (mmc_send_tuning(host->mmc, opcode, NULL))
> +                               break;
> +               }
> +
> +               window = right - left;
> +               dev_info(dev, "tuning window = %d\n", window);

I think this should be dev_dbg() rather than dev_info(). Tuning data is
not something that should normally be printed. I'd also print the
values of left and right, for reference.

> +
> +               if (window > oldwindow) {
> +                       oldwindow = window;
> +                       center = (((right - 1) + left) / 2) | inverted;
> +               }
> +       }
> +
> +       val = (out_phase | enable_mask | (center << ASPEED_SDC_S0_PHASE_IN_SHIFT));
> +       writel(val, sdc->regs + ASPEED_SDC_PHASE);
> +
> +       dev_info(dev, "tuning result=%x\n", val);

dev_dbg() again.

Cheers,

Andrew