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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <TYSPR06MB7068FCC2A69BB13A2465DB79957DA@TYSPR06MB7068.apcprd06.prod.outlook.com>
Date: Thu, 19 Jun 2025 06:57:11 +0000
From: Cool Lee <cool_lee@...eedtech.com>
To: Andrew Jeffery <andrew@...econstruct.com.au>, "adrian.hunter@...el.com"
	<adrian.hunter@...el.com>, "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
	"joel@....id.au" <joel@....id.au>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-mmc@...r.kernel.org"
	<linux-mmc@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, BMC-SW <BMC-SW@...eedtech.com>
Subject: RE: [PATCH 2/8] mmc: sdhci-of-aspeed: Add runtime tuning


> > 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.
Ok, this should be dev_dbg() definitely. I will change this.

> 
> > +
> > +               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.
Ok, I will change this.

> 
> Cheers,
> 
> Andrew