[PATCH 0/8] Aspeed SDHCI driver workaround and auto tune

[PATCH 0/8] Aspeed SDHCI driver workaround and auto tune

[Cool Lee]

The purpose of this patch series is to workaround that the
Aspeed SDHCI software reset can't be cleared issue, and to add runtime
tuning and sdr50 support. The runtime tuning is to improve the
compatibility of the sdhci driver with different MMC cards.

Cool Lee (8):
  mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.
  mmc: sdhci-of-aspeed: Add runtime tuning
  mmc: sdhci-of-aspeed: Patch HOST_CONTROL2 register missing after top
    reset
  mmc: sdhci-of-aspeed: Get max clockk by using default api
  mmc: sdhci-of-aspeed: Fix null pointer
  mmc: sdhci-of-aspeed: Add output timing phase tuning
  mmc: sdhci-of-aspeed: Remove timing phase
  mmc: sdhci-of-aspeed: Add sdr50 support

 drivers/mmc/host/sdhci-of-aspeed.c | 370 ++++++++++++++---------------
 1 file changed, 183 insertions(+), 187 deletions(-)

--
2.34.1

[PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Cool Lee]

Replace sdhci software reset by scu reset from top.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 55 +++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d6de010551b9..01bc574272eb 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 
 #include "sdhci-pltfm.h"
@@ -39,6 +40,7 @@
 struct aspeed_sdc {
 	struct clk *clk;
 	struct resource *res;
+	struct reset_control *rst;
 
 	spinlock_t lock;
 	void __iomem *regs;
@@ -328,13 +330,58 @@ static u32 aspeed_sdhci_readl(struct sdhci_host *host, int reg)
 	return val;
 }
 
+static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+	struct sdhci_pltfm_host *pltfm_priv;
+	struct aspeed_sdhci *aspeed_sdhci;
+	struct aspeed_sdc *aspeed_sdc;
+	u32 save_array[7];
+	u32 reg_array[] = {SDHCI_DMA_ADDRESS,
+			SDHCI_BLOCK_SIZE,
+			SDHCI_ARGUMENT,
+			SDHCI_HOST_CONTROL,
+			SDHCI_CLOCK_CONTROL,
+			SDHCI_INT_ENABLE,
+			SDHCI_SIGNAL_ENABLE};
+	int i;
+	u16 tran_mode;
+	u32 mmc8_mode;
+
+	pltfm_priv = sdhci_priv(host);
+	aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
+	aspeed_sdc = aspeed_sdhci->parent;
+
+	if (!IS_ERR(aspeed_sdc->rst)) {
+		for (i = 0; i < ARRAY_SIZE(reg_array); i++)
+			save_array[i] = sdhci_readl(host, reg_array[i]);
+
+		tran_mode = sdhci_readw(host, SDHCI_TRANSFER_MODE);
+		mmc8_mode = readl(aspeed_sdc->regs);
+
+		reset_control_assert(aspeed_sdc->rst);
+		mdelay(1);
+		reset_control_deassert(aspeed_sdc->rst);
+		mdelay(1);
+
+		for (i = 0; i < ARRAY_SIZE(reg_array); i++)
+			sdhci_writel(host, save_array[i], reg_array[i]);
+
+		sdhci_writew(host, tran_mode, SDHCI_TRANSFER_MODE);
+		writel(mmc8_mode, aspeed_sdc->regs);
+
+		aspeed_sdhci_set_clock(host, host->clock);
+	}
+
+	sdhci_reset(host, mask);
+}
+
 static const struct sdhci_ops aspeed_sdhci_ops = {
 	.read_l = aspeed_sdhci_readl,
 	.set_clock = aspeed_sdhci_set_clock,
 	.get_max_clock = aspeed_sdhci_get_max_clock,
 	.set_bus_width = aspeed_sdhci_set_bus_width,
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
-	.reset = sdhci_reset,
+	.reset = aspeed_sdhci_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 };
 
@@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&sdc->lock);
 
+	sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (!IS_ERR(sdc->rst)) {
+		reset_control_assert(sdc->rst);
+		reset_control_deassert(sdc->rst);
+	}
+
 	sdc->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(sdc->clk))
 		return PTR_ERR(sdc->clk);
-- 
2.34.1

Re: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Philipp Zabel]

On So, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> Replace sdhci software reset by scu reset from top.
> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 55 +++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index d6de010551b9..01bc574272eb 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  
>  #include "sdhci-pltfm.h"
> @@ -39,6 +40,7 @@
>  struct aspeed_sdc {
>  	struct clk *clk;
>  	struct resource *res;
> +	struct reset_control *rst;
>  
>  	spinlock_t lock;
>  	void __iomem *regs;
> @@ -328,13 +330,58 @@ static u32 aspeed_sdhci_readl(struct sdhci_host *host, int reg)
>  	return val;
>  }
>  
> +static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +	struct sdhci_pltfm_host *pltfm_priv;
> +	struct aspeed_sdhci *aspeed_sdhci;
> +	struct aspeed_sdc *aspeed_sdc;
> +	u32 save_array[7];
> +	u32 reg_array[] = {SDHCI_DMA_ADDRESS,
> +			SDHCI_BLOCK_SIZE,
> +			SDHCI_ARGUMENT,
> +			SDHCI_HOST_CONTROL,
> +			SDHCI_CLOCK_CONTROL,
> +			SDHCI_INT_ENABLE,
> +			SDHCI_SIGNAL_ENABLE};
> +	int i;
> +	u16 tran_mode;
> +	u32 mmc8_mode;
> +
> +	pltfm_priv = sdhci_priv(host);
> +	aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> +	aspeed_sdc = aspeed_sdhci->parent;
> +
> +	if (!IS_ERR(aspeed_sdc->rst)) {
> +		for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> +			save_array[i] = sdhci_readl(host, reg_array[i]);
> +
> +		tran_mode = sdhci_readw(host, SDHCI_TRANSFER_MODE);
> +		mmc8_mode = readl(aspeed_sdc->regs);
> +
> +		reset_control_assert(aspeed_sdc->rst);
> +		mdelay(1);
> +		reset_control_deassert(aspeed_sdc->rst);
> +		mdelay(1);

Why are there delays here ...

[...]
> @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&sdc->lock);
>  
> +	sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> +	if (!IS_ERR(sdc->rst)) {
> +		reset_control_assert(sdc->rst);
> +		reset_control_deassert(sdc->rst);

... but not here?

regards
Philipp

RE: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Cool Lee]

> > Replace sdhci software reset by scu reset from top.
> >
> > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 55
> > +++++++++++++++++++++++++++++-
> >  1 file changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index d6de010551b9..01bc574272eb 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >  #include <linux/spinlock.h>
> >
> >  #include "sdhci-pltfm.h"
> > @@ -39,6 +40,7 @@
> >  struct aspeed_sdc {
> >  	struct clk *clk;
> >  	struct resource *res;
> > +	struct reset_control *rst;
> >
> >  	spinlock_t lock;
> >  	void __iomem *regs;
> > @@ -328,13 +330,58 @@ static u32 aspeed_sdhci_readl(struct sdhci_host
> *host, int reg)
> >  	return val;
> >  }
> >
> > +static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask) {
> > +	struct sdhci_pltfm_host *pltfm_priv;
> > +	struct aspeed_sdhci *aspeed_sdhci;
> > +	struct aspeed_sdc *aspeed_sdc;
> > +	u32 save_array[7];
> > +	u32 reg_array[] = {SDHCI_DMA_ADDRESS,
> > +			SDHCI_BLOCK_SIZE,
> > +			SDHCI_ARGUMENT,
> > +			SDHCI_HOST_CONTROL,
> > +			SDHCI_CLOCK_CONTROL,
> > +			SDHCI_INT_ENABLE,
> > +			SDHCI_SIGNAL_ENABLE};
> > +	int i;
> > +	u16 tran_mode;
> > +	u32 mmc8_mode;
> > +
> > +	pltfm_priv = sdhci_priv(host);
> > +	aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> > +	aspeed_sdc = aspeed_sdhci->parent;
> > +
> > +	if (!IS_ERR(aspeed_sdc->rst)) {
> > +		for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> > +			save_array[i] = sdhci_readl(host, reg_array[i]);
> > +
> > +		tran_mode = sdhci_readw(host, SDHCI_TRANSFER_MODE);
> > +		mmc8_mode = readl(aspeed_sdc->regs);
> > +
> > +		reset_control_assert(aspeed_sdc->rst);
> > +		mdelay(1);
> > +		reset_control_deassert(aspeed_sdc->rst);
> > +		mdelay(1);
> 
> Why are there delays here ...
Ok, I will remove from here. From hardware's perspective, the delay is not needed.

> 
> [...]
> > @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct
> > platform_device *pdev)
> >
> >  	spin_lock_init(&sdc->lock);
> >
> > +	sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +	if (!IS_ERR(sdc->rst)) {
> > +		reset_control_assert(sdc->rst);
> > +		reset_control_deassert(sdc->rst);
> 
> ... but not here?
> 
> regards
> Philipp

Re: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Andrew Jeffery]

Hi,

On Sun, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> Replace sdhci software reset by scu reset from top.
> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>

Can you please add a Fixes: tag?

> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 55 +++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index d6de010551b9..01bc574272eb 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  
>  #include "sdhci-pltfm.h"
> @@ -39,6 +40,7 @@
>  struct aspeed_sdc {
>         struct clk *clk;
>         struct resource *res;
> +       struct reset_control *rst;
>  
>         spinlock_t lock;
>         void __iomem *regs;
> @@ -328,13 +330,58 @@ static u32 aspeed_sdhci_readl(struct sdhci_host *host, int reg)
>         return val;
>  }
>  
> +static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct sdhci_pltfm_host *pltfm_priv;
> +       struct aspeed_sdhci *aspeed_sdhci;
> +       struct aspeed_sdc *aspeed_sdc;
> +       u32 save_array[7];
> +       u32 reg_array[] = {SDHCI_DMA_ADDRESS,
> +                       SDHCI_BLOCK_SIZE,
> +                       SDHCI_ARGUMENT,
> +                       SDHCI_HOST_CONTROL,
> +                       SDHCI_CLOCK_CONTROL,
> +                       SDHCI_INT_ENABLE,
> +                       SDHCI_SIGNAL_ENABLE};
> +       int i;
> +       u16 tran_mode;
> +       u32 mmc8_mode;
> +
> +       pltfm_priv = sdhci_priv(host);
> +       aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> +       aspeed_sdc = aspeed_sdhci->parent;
> +
> +       if (!IS_ERR(aspeed_sdc->rst)) {
> +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> +                       save_array[i] = sdhci_readl(host, reg_array[i]);
> +
> +               tran_mode = sdhci_readw(host, SDHCI_TRANSFER_MODE);
> +               mmc8_mode = readl(aspeed_sdc->regs);
> +
> +               reset_control_assert(aspeed_sdc->rst);
> +               mdelay(1);
> +               reset_control_deassert(aspeed_sdc->rst);
> +               mdelay(1);

See comment below regarding clock/reset behaviour and implementation.

> +
> +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> +                       sdhci_writel(host, save_array[i], reg_array[i]);
> +
> +               sdhci_writew(host, tran_mode, SDHCI_TRANSFER_MODE);
> +               writel(mmc8_mode, aspeed_sdc->regs);
> +
> +               aspeed_sdhci_set_clock(host, host->clock);
> +       }
> +
> +       sdhci_reset(host, mask);

Given that we do this after the SCU reset above, what exactly is the
SCU reset fixing? Can you provide more details?

> +}
> +
>  static const struct sdhci_ops aspeed_sdhci_ops = {
>         .read_l = aspeed_sdhci_readl,
>         .set_clock = aspeed_sdhci_set_clock,
>         .get_max_clock = aspeed_sdhci_get_max_clock,
>         .set_bus_width = aspeed_sdhci_set_bus_width,
>         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> -       .reset = sdhci_reset,
> +       .reset = aspeed_sdhci_reset,
>         .set_uhs_signaling = sdhci_set_uhs_signaling,
>  };
>  
> @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>  
>         spin_lock_init(&sdc->lock);
>  
> +       sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> +       if (!IS_ERR(sdc->rst)) {
> +               reset_control_assert(sdc->rst);
> +               reset_control_deassert(sdc->rst);
> +       }
> +

The clock driver for the AST2400, AST2500 and AST2600 manages the reset
as part of managing the clock[1][2].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-aspeed.c?h=v6.16-rc2#n71
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-aspeed.c?h=v6.16-rc2#n209

What you have here asks for a resets property, but that's not currently
specified in the devicetree binding.

So: is the clock driver not doing the right thing given we enable the
clock directly below this hunk? If not, should we fix that instead?

We can add the resets property to the binding, but I'd also like a
better explanation of the problem.

Andrew

RE: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Cool Lee]

> > Replace sdhci software reset by scu reset from top.
> >
> > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> 
> Can you please add a Fixes: tag?
This patch wasn't used to fix a commit. This is a workaround for a hardware bug.
For this condition, do I need a Fixes?

> 
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 55
> > +++++++++++++++++++++++++++++-
> >  1 file changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index d6de010551b9..01bc574272eb 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >  #include <linux/spinlock.h>
> >
> >  #include "sdhci-pltfm.h"
> > @@ -39,6 +40,7 @@
> >  struct aspeed_sdc {
> >         struct clk *clk;
> >         struct resource *res;
> > +       struct reset_control *rst;
> >
> >         spinlock_t lock;
> >         void __iomem *regs;
> > @@ -328,13 +330,58 @@ static u32 aspeed_sdhci_readl(struct sdhci_host
> > *host, int reg)
> >         return val;
> >  }
> >
> > +static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask) {
> > +       struct sdhci_pltfm_host *pltfm_priv;
> > +       struct aspeed_sdhci *aspeed_sdhci;
> > +       struct aspeed_sdc *aspeed_sdc;
> > +       u32 save_array[7];
> > +       u32 reg_array[] = {SDHCI_DMA_ADDRESS,
> > +                       SDHCI_BLOCK_SIZE,
> > +                       SDHCI_ARGUMENT,
> > +                       SDHCI_HOST_CONTROL,
> > +                       SDHCI_CLOCK_CONTROL,
> > +                       SDHCI_INT_ENABLE,
> > +                       SDHCI_SIGNAL_ENABLE};
> > +       int i;
> > +       u16 tran_mode;
> > +       u32 mmc8_mode;
> > +
> > +       pltfm_priv = sdhci_priv(host);
> > +       aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> > +       aspeed_sdc = aspeed_sdhci->parent;
> > +
> > +       if (!IS_ERR(aspeed_sdc->rst)) {
> > +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> > +                       save_array[i] = sdhci_readl(host,
> > +reg_array[i]);
> > +
> > +               tran_mode = sdhci_readw(host,
> SDHCI_TRANSFER_MODE);
> > +               mmc8_mode = readl(aspeed_sdc->regs);
> > +
> > +               reset_control_assert(aspeed_sdc->rst);
> > +               mdelay(1);
> > +               reset_control_deassert(aspeed_sdc->rst);
> > +               mdelay(1);
> 
> See comment below regarding clock/reset behaviour and implementation.
> 
> > +
> > +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> > +                       sdhci_writel(host, save_array[i],
> > +reg_array[i]);
> > +
> > +               sdhci_writew(host, tran_mode,
> SDHCI_TRANSFER_MODE);
> > +               writel(mmc8_mode, aspeed_sdc->regs);
> > +
> > +               aspeed_sdhci_set_clock(host, host->clock);
> > +       }
> > +
> > +       sdhci_reset(host, mask);
> 
> Given that we do this after the SCU reset above, what exactly is the SCU reset
> fixing? Can you provide more details?
The issue is sdhci Software Reset ALL (0x12C[24]) cannot complete which means it's always being 1 and not back to 0.
The root cause is when sdhci dma operates, it might hold some state signals which is not well cleared by Software Reset. These signals prevent Software Reset to be cleared.
This is a hardware issue so that the workaround is resetting whole SDHCI controller from SCU reset.

> 
> > +}
> > +
> >  static const struct sdhci_ops aspeed_sdhci_ops = {
> >         .read_l = aspeed_sdhci_readl,
> >         .set_clock = aspeed_sdhci_set_clock,
> >         .get_max_clock = aspeed_sdhci_get_max_clock,
> >         .set_bus_width = aspeed_sdhci_set_bus_width,
> >         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> > -       .reset = sdhci_reset,
> > +       .reset = aspeed_sdhci_reset,
> >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> >  };
> >
> > @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct
> > platform_device *pdev)
> >
> >         spin_lock_init(&sdc->lock);
> >
> > +       sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +       if (!IS_ERR(sdc->rst)) {
> > +               reset_control_assert(sdc->rst);
> > +               reset_control_deassert(sdc->rst);
> > +       }
> > +
> 
> The clock driver for the AST2400, AST2500 and AST2600 manages the reset as
> part of managing the clock[1][2].
> 
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers
> /clk/clk-aspeed.c?h=v6.16-rc2#n71
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers
> /clk/clk-aspeed.c?h=v6.16-rc2#n209
> 
> What you have here asks for a resets property, but that's not currently
> specified in the devicetree binding.
> 
> So: is the clock driver not doing the right thing given we enable the clock
> directly below this hunk? If not, should we fix that instead?
> 
> We can add the resets property to the binding, but I'd also like a better
> explanation of the problem.
For legacy projects, the clock property handles reset simultaneously in the clock driver.
For new project AST2700, clock and reset are separated, and we add a reset property to the binding.
Hence, the patch won't affect until the reset property to the binding.
Should I add the reset property in this patch serious?

> 
> Andrew

Re: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Andrew Jeffery]

Hello,

On Thu, 2025-06-19 at 06:53 +0000, Cool Lee wrote:
> 
> > > Replace sdhci software reset by scu reset from top.
> > > 
> > > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> > 
> > Can you please add a Fixes: tag?
> This patch wasn't used to fix a commit. This is a workaround for a hardware bug.

A hardware bug in which SoCs? AST2400-AST2700? Or just the AST2700?

> For this condition, do I need a Fixes?

If the bug exists for all SoCs it's a deficiency in the original driver
and so should have a Fixes: tag.

> > 

> > > +
> > > +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> > > +                       sdhci_writel(host, save_array[i],
> > > +reg_array[i]);
> > > +
> > > +               sdhci_writew(host, tran_mode,
> > SDHCI_TRANSFER_MODE);
> > > +               writel(mmc8_mode, aspeed_sdc->regs);
> > > +
> > > +               aspeed_sdhci_set_clock(host, host->clock);
> > > +       }
> > > +
> > > +       sdhci_reset(host, mask);
> > 
> > Given that we do this after the SCU reset above, what exactly is the SCU reset
> > fixing? Can you provide more details?
> The issue is sdhci Software Reset ALL (0x12C[24]) cannot complete which means it's always being 1 and not back to 0.
> The root cause is when sdhci dma operates, it might hold some state signals which is not well cleared by Software Reset. These signals prevent Software Reset to be cleared.
> This is a hardware issue so that the workaround is resetting whole SDHCI controller from SCU reset.

Can you please put these details in the commit message?

> 
> > 
> > > +}
> > > +
> > >  static const struct sdhci_ops aspeed_sdhci_ops = {
> > >         .read_l = aspeed_sdhci_readl,
> > >         .set_clock = aspeed_sdhci_set_clock,
> > >         .get_max_clock = aspeed_sdhci_get_max_clock,
> > >         .set_bus_width = aspeed_sdhci_set_bus_width,
> > >         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> > > -       .reset = sdhci_reset,
> > > +       .reset = aspeed_sdhci_reset,
> > >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> > >  };
> > > 
> > > @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct
> > > platform_device *pdev)
> > > 
> > >         spin_lock_init(&sdc->lock);
> > > 
> > > +       sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > +       if (!IS_ERR(sdc->rst)) {
> > > +               reset_control_assert(sdc->rst);
> > > +               reset_control_deassert(sdc->rst);
> > > +       }
> > > +
> > 
> > The clock driver for the AST2400, AST2500 and AST2600 manages the reset as
> > part of managing the clock[1][2].
> > 
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers
> > /clk/clk-aspeed.c?h=v6.16-rc2#n71
> > [2]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers
> > /clk/clk-aspeed.c?h=v6.16-rc2#n209
> > 
> > What you have here asks for a resets property, but that's not currently
> > specified in the devicetree binding.
> > 
> > So: is the clock driver not doing the right thing given we enable the clock
> > directly below this hunk? If not, should we fix that instead?
> > 
> > We can add the resets property to the binding, but I'd also like a better
> > explanation of the problem.
> For legacy projects, the clock property handles reset simultaneously in the clock driver.
> For new project AST2700, clock and reset are separated, and we add a reset property to the binding.
> Hence, the patch won't affect until the reset property to the binding.
> Should I add the reset property in this patch serious?

Yes, please.

Andrew

RE: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't be cleared issue.

[Cool Lee]

> >
> > > > Replace sdhci software reset by scu reset from top.
> > > >
> > > > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> > >
> > > Can you please add a Fixes: tag?
> > This patch wasn't used to fix a commit. This is a workaround for a hardware
> bug.
> 
> A hardware bug in which SoCs? AST2400-AST2700? Or just the AST2700?
This is a bug on all platforms, except AST2700.

> 
> > For this condition, do I need a Fixes?
> 
> If the bug exists for all SoCs it's a deficiency in the original driver and so should
> have a Fixes: tag.
Ok, I'll do this.

> 
> > >
> 
> > > > +
> > > > +               for (i = 0; i < ARRAY_SIZE(reg_array); i++)
> > > > +                       sdhci_writel(host, save_array[i],
> > > > +reg_array[i]);
> > > > +
> > > > +               sdhci_writew(host, tran_mode,
> > > SDHCI_TRANSFER_MODE);
> > > > +               writel(mmc8_mode, aspeed_sdc->regs);
> > > > +
> > > > +               aspeed_sdhci_set_clock(host, host->clock);
> > > > +       }
> > > > +
> > > > +       sdhci_reset(host, mask);
> > >
> > > Given that we do this after the SCU reset above, what exactly is the
> > > SCU reset fixing? Can you provide more details?
> > The issue is sdhci Software Reset ALL (0x12C[24]) cannot complete which
> means it's always being 1 and not back to 0.
> > The root cause is when sdhci dma operates, it might hold some state signals
> which is not well cleared by Software Reset. These signals prevent Software
> Reset to be cleared.
> > This is a hardware issue so that the workaround is resetting whole SDHCI
> controller from SCU reset.
> 
> Can you please put these details in the commit message?
Ok, I'll do this.

> 
> >
> > >
> > > > +}
> > > > +
> > > >  static const struct sdhci_ops aspeed_sdhci_ops = {
> > > >         .read_l = aspeed_sdhci_readl,
> > > >         .set_clock = aspeed_sdhci_set_clock,
> > > >         .get_max_clock = aspeed_sdhci_get_max_clock,
> > > >         .set_bus_width = aspeed_sdhci_set_bus_width,
> > > >         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> > > > -       .reset = sdhci_reset,
> > > > +       .reset = aspeed_sdhci_reset,
> > > >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> > > >  };
> > > >
> > > > @@ -535,6 +582,12 @@ static int aspeed_sdc_probe(struct
> > > > platform_device *pdev)
> > > >
> > > >         spin_lock_init(&sdc->lock);
> > > >
> > > > +       sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > +       if (!IS_ERR(sdc->rst)) {
> > > > +               reset_control_assert(sdc->rst);
> > > > +               reset_control_deassert(sdc->rst);
> > > > +       }
> > > > +
> > >
> > > The clock driver for the AST2400, AST2500 and AST2600 manages the
> > > reset as part of managing the clock[1][2].
> > >
> > > [1]:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
> > > ree/drivers
> > > /clk/clk-aspeed.c?h=v6.16-rc2#n71
> > > [2]:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
> > > ree/drivers
> > > /clk/clk-aspeed.c?h=v6.16-rc2#n209
> > >
> > > What you have here asks for a resets property, but that's not
> > > currently specified in the devicetree binding.
> > >
> > > So: is the clock driver not doing the right thing given we enable
> > > the clock directly below this hunk? If not, should we fix that instead?
> > >
> > > We can add the resets property to the binding, but I'd also like a
> > > better explanation of the problem.
> > For legacy projects, the clock property handles reset simultaneously in the
> clock driver.
> > For new project AST2700, clock and reset are separated, and we add a reset
> property to the binding.
> > Hence, the patch won't affect until the reset property to the binding.
> > Should I add the reset property in this patch serious?
> 
> Yes, please.
Ok, I'll do this.

> 
> Andrew

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

[Cool Lee]

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@aspeedtech.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++) {
+			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++) {
+			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);
+
+		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);
+
+	return mmc_send_tuning(host->mmc, opcode, NULL);
+}
+
 static const struct sdhci_ops aspeed_sdhci_ops = {
 	.read_l = aspeed_sdhci_readl,
 	.set_clock = aspeed_sdhci_set_clock,
@@ -383,6 +450,7 @@ static const struct sdhci_ops aspeed_sdhci_ops = {
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.reset = aspeed_sdhci_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.platform_execute_tuning = aspeed_sdhci_execute_tuning,
 };
 
 static const struct sdhci_pltfm_data aspeed_sdhci_pdata = {
-- 
2.34.1

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

[Andrew Jeffery]

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@aspeedtech.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

RE: [PATCH 2/8] mmc: sdhci-of-aspeed: Add runtime tuning

[Cool Lee]

> > 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@aspeedtech.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

[PATCH 3/8] mmc: sdhci-of-aspeed: Patch HOST_CONTROL2 register missing after top reset

[Cool Lee]

HOST_CONTROL2 register will be cleared after top reset,
it needs to be saved/resotred while reset.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 5e5ae1894456..10160a706334 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -336,14 +336,15 @@ static void aspeed_sdhci_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_priv;
 	struct aspeed_sdhci *aspeed_sdhci;
 	struct aspeed_sdc *aspeed_sdc;
-	u32 save_array[7];
+	u32 save_array[8];
 	u32 reg_array[] = {SDHCI_DMA_ADDRESS,
 			SDHCI_BLOCK_SIZE,
 			SDHCI_ARGUMENT,
 			SDHCI_HOST_CONTROL,
 			SDHCI_CLOCK_CONTROL,
 			SDHCI_INT_ENABLE,
-			SDHCI_SIGNAL_ENABLE};
+			SDHCI_SIGNAL_ENABLE,
+			SDHCI_AUTO_CMD_STATUS};
 	int i;
 	u16 tran_mode;
 	u32 mmc8_mode;
-- 
2.34.1

Re: [PATCH 3/8] mmc: sdhci-of-aspeed: Patch HOST_CONTROL2 register missing after top reset

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> HOST_CONTROL2 register will be cleared after top reset,
> it needs to be saved/resotred while reset.
> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>

Please squash this into the first patch.

Andrew

RE: [PATCH 3/8] mmc: sdhci-of-aspeed: Patch HOST_CONTROL2 register missing after top reset

[Cool Lee]

> > HOST_CONTROL2 register will be cleared after top reset, it needs to be
> > saved/resotred while reset.
> >
> > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> 
> Please squash this into the first patch.
Ok, I will do this.

> 
> Andrew

[PATCH 4/8] mmc: sdhci-of-aspeed: Get max clockk by using default api

[Cool Lee]

Don't limit clock frequency by f_max.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 10160a706334..2bdd93a3f91f 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -288,14 +288,6 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_enable_clk(host, clk);
 }
 
-static unsigned int aspeed_sdhci_get_max_clock(struct sdhci_host *host)
-{
-	if (host->mmc->f_max)
-		return host->mmc->f_max;
-
-	return sdhci_pltfm_clk_get_max_clock(host);
-}
-
 static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
 {
 	struct sdhci_pltfm_host *pltfm_priv;
@@ -446,7 +438,7 @@ static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
 static const struct sdhci_ops aspeed_sdhci_ops = {
 	.read_l = aspeed_sdhci_readl,
 	.set_clock = aspeed_sdhci_set_clock,
-	.get_max_clock = aspeed_sdhci_get_max_clock,
+	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width = aspeed_sdhci_set_bus_width,
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.reset = aspeed_sdhci_reset,
-- 
2.34.1

Re: [PATCH 4/8] mmc: sdhci-of-aspeed: Get max clockk by using default api

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> Don't limit clock frequency by f_max.
> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 10160a706334..2bdd93a3f91f 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -288,14 +288,6 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>         sdhci_enable_clk(host, clk);
>  }
>  
> -static unsigned int aspeed_sdhci_get_max_clock(struct sdhci_host *host)
> -{
> -       if (host->mmc->f_max)
> -               return host->mmc->f_max;
> -
> -       return sdhci_pltfm_clk_get_max_clock(host);
> -}
> -
>  static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
>  {
>         struct sdhci_pltfm_host *pltfm_priv;
> @@ -446,7 +438,7 @@ static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
>  static const struct sdhci_ops aspeed_sdhci_ops = {
>         .read_l = aspeed_sdhci_readl,
>         .set_clock = aspeed_sdhci_set_clock,
> -       .get_max_clock = aspeed_sdhci_get_max_clock,
> +       .get_max_clock = sdhci_pltfm_clk_get_max_clock,

This was used to limit the maximum bus speed via the devicetree. See:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.16-rc2&id=0a0e8d7501cda79c9b20f6011814e2ec9b473ade

Why remove it? There's no discussion of the motivation in the commit
message.

Andrew

RE: [PATCH 4/8] mmc: sdhci-of-aspeed: Get max clockk by using default api

[Cool Lee]

> > Don't limit clock frequency by f_max.
> >
> > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 10160a706334..2bdd93a3f91f 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -288,14 +288,6 @@ static void aspeed_sdhci_set_clock(struct
> > sdhci_host *host, unsigned int clock)
> >         sdhci_enable_clk(host, clk);
> >  }
> >
> > -static unsigned int aspeed_sdhci_get_max_clock(struct sdhci_host
> > *host) -{
> > -       if (host->mmc->f_max)
> > -               return host->mmc->f_max;
> > -
> > -       return sdhci_pltfm_clk_get_max_clock(host);
> > -}
> > -
> >  static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int
> > width)
> >  {
> >         struct sdhci_pltfm_host *pltfm_priv; @@ -446,7 +438,7 @@
> > static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32
> > opcode)
> >  static const struct sdhci_ops aspeed_sdhci_ops = {
> >         .read_l = aspeed_sdhci_readl,
> >         .set_clock = aspeed_sdhci_set_clock,
> > -       .get_max_clock = aspeed_sdhci_get_max_clock,
> > +       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> 
> This was used to limit the maximum bus speed via the devicetree. See:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=
> v6.16-rc2&id=0a0e8d7501cda79c9b20f6011814e2ec9b473ade
> 
> Why remove it? There's no discussion of the motivation in the commit
> message.
Yes, you're right. There is no need to change this. I will remove this.
My original thinking is changing to use default sdhci_set_clock and sdhci_get_max_clock that we can simplify the code.
But the aspeed_sdhci_set_clock handles different divider by legacy projects, so I keep that but just missed the get_max_clock.

> 
> Andrew

[PATCH 5/8] mmc: sdhci-of-aspeed: Fix null pointer

[Cool Lee]

Platform data might be null.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 2bdd93a3f91f..22dde915e51b 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -241,7 +241,7 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	struct sdhci_pltfm_host *pltfm_host;
 	unsigned long parent, bus;
 	struct aspeed_sdhci *sdhci;
-	int div;
+	int div = 1;
 	u16 clk;
 
 	pltfm_host = sdhci_priv(host);
@@ -257,6 +257,9 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	if (WARN_ON(clock > host->max_clk))
 		clock = host->max_clk;
 
+	if (sdhci->pdata)
+		div = sdhci->pdata->clk_div_start;
+
 	/*
 	 * Regarding the AST2600:
 	 *
@@ -273,7 +276,7 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	 * supporting the value 0 in (EMMC12C[7:6], EMMC12C[15:8]), and capture
 	 * the 0-value capability in clk_div_start.
 	 */
-	for (div = sdhci->pdata->clk_div_start; div < 256; div *= 2) {
+	for (; div < 256; div *= 2) {
 		bus = parent / div;
 		if (bus <= clock)
 			break;
-- 
2.34.1

Re: [PATCH 5/8] mmc: sdhci-of-aspeed: Fix null pointer

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:58 +0800, Cool Lee wrote:
> Platform data might be null.

Currently it can't be:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c?h=v6.16-rc2#n375

Are there future circumstances where it may be NULL?

I'm all for reducing the reasoning from global to local, but I think
some discussion in the commit message would be good.

> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index 2bdd93a3f91f..22dde915e51b 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -241,7 +241,7 @@ static void aspeed_sdhci_set_clock(struct
> sdhci_host *host, unsigned int clock)
>         struct sdhci_pltfm_host *pltfm_host;
>         unsigned long parent, bus;
>         struct aspeed_sdhci *sdhci;
> -       int div;
> +       int div = 1;
>         u16 clk;
>  
>         pltfm_host = sdhci_priv(host);
> @@ -257,6 +257,9 @@ static void aspeed_sdhci_set_clock(struct
> sdhci_host *host, unsigned int clock)
>         if (WARN_ON(clock > host->max_clk))
>                 clock = host->max_clk;
>  
> +       if (sdhci->pdata)

Given this shouldn't be the case, perhaps precede it with a
WARN_ONCE(!sdhci->pdata)?

Andrew

> +               div = sdhci->pdata->clk_div_start;
> +
>         /*
>          * Regarding the AST2600:
>          *
> @@ -273,7 +276,7 @@ static void aspeed_sdhci_set_clock(struct
> sdhci_host *host, unsigned int clock)
>          * supporting the value 0 in (EMMC12C[7:6], EMMC12C[15:8]),
> and capture
>          * the 0-value capability in clk_div_start.
>          */
> -       for (div = sdhci->pdata->clk_div_start; div < 256; div *= 2)
> {
> +       for (; div < 256; div *= 2) {
>                 bus = parent / div;
>                 if (bus <= clock)
>                         break;

RE: [PATCH 5/8] mmc: sdhci-of-aspeed: Fix null pointer

[Cool Lee]

> > Platform data might be null.
> 
> Currently it can't be:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers
> /mmc/host/sdhci-of-aspeed.c?h=v6.16-rc2#n375
> 
> Are there future circumstances where it may be NULL?
> 
> I'm all for reducing the reasoning from global to local, but I think some
> discussion in the commit message would be good.
Ok, you're right. I'll remove this.
I'm just considering when adding a new platform AST2700 which doesn't have a data, but missed there is a pre-check here.

> 
> >
> > Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 2bdd93a3f91f..22dde915e51b 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -241,7 +241,7 @@ static void aspeed_sdhci_set_clock(struct
> > sdhci_host *host, unsigned int clock)
> >         struct sdhci_pltfm_host *pltfm_host;
> >         unsigned long parent, bus;
> >         struct aspeed_sdhci *sdhci;
> > -       int div;
> > +       int div = 1;
> >         u16 clk;
> >
> >         pltfm_host = sdhci_priv(host); @@ -257,6 +257,9 @@ static
> void
> > aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> >         if (WARN_ON(clock > host->max_clk))
> >                 clock = host->max_clk;
> >
> > +       if (sdhci->pdata)
> 
> Given this shouldn't be the case, perhaps precede it with a
> WARN_ONCE(!sdhci->pdata)?
Agreed.

> 
> Andrew
> 
> > +               div = sdhci->pdata->clk_div_start;
> > +
> >         /*
> >          * Regarding the AST2600:
> >          *
> > @@ -273,7 +276,7 @@ static void aspeed_sdhci_set_clock(struct
> > sdhci_host *host, unsigned int clock)
> >          * supporting the value 0 in (EMMC12C[7:6], EMMC12C[15:8]),
> > and capture
> >          * the 0-value capability in clk_div_start.
> >          */
> > -       for (div = sdhci->pdata->clk_div_start; div < 256; div *= 2) {
> > +       for (; div < 256; div *= 2) {
> >                 bus = parent / div;
> >                 if (bus <= clock)
> >                         break;

[PATCH 6/8] mmc: sdhci-of-aspeed: Add output timing phase tuning

[Cool Lee]

Enhance auto tuning with input and output calibration.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 48 ++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 22dde915e51b..92687fc30d1d 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -25,6 +25,7 @@
 #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_S0_PHASE_OUT_SHIFT 3
 #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)
@@ -422,7 +423,7 @@ static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
 		}
 
 		window = right - left;
-		dev_info(dev, "tuning window = %d\n", window);
+		dev_dbg(dev, "tuning window[%d][%d~%d] = %d\n", edge, left, right, window);
 
 		if (window > oldwindow) {
 			oldwindow = window;
@@ -433,7 +434,50 @@ static int aspeed_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
 	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(dev, "input tuning result=%x\n", val);
+
+	inverted = 0;
+	out_phase = val & ~ASPEED_SDC_S0_PHASE_OUT;
+	in_phase = out_phase;
+	oldwindow = 0;
+
+	for (edge = 0; edge < 2; edge++) {
+		if (edge == 1)
+			inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
+
+		val = (in_phase | enable_mask | (inverted << ASPEED_SDC_S0_PHASE_OUT_SHIFT));
+
+		/* find the left boundary */
+		for (left = 0; left < ASPEED_SDHCI_NR_TAPS + 1; left++) {
+			out_phase = val | (left << ASPEED_SDC_S0_PHASE_OUT_SHIFT);
+			writel(out_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++) {
+			out_phase = val | (right << ASPEED_SDC_S0_PHASE_OUT_SHIFT);
+			writel(out_phase, sdc->regs + ASPEED_SDC_PHASE);
+
+			if (mmc_send_tuning(host->mmc, opcode, NULL))
+				break;
+		}
+
+		window = right - left;
+		dev_info(dev, "tuning window[%d][%d~%d] = %d\n", edge, left, right, window);
+
+		if (window > oldwindow) {
+			oldwindow = window;
+			center = (((right - 1) + left) / 2) | inverted;
+		}
+	}
+
+	val = (in_phase | enable_mask | (center << ASPEED_SDC_S0_PHASE_OUT_SHIFT));
+	writel(val, sdc->regs + ASPEED_SDC_PHASE);
+
+	dev_dbg(dev, "output tuning result=%x\n", val);
 
 	return mmc_send_tuning(host->mmc, opcode, NULL);
 }
-- 
2.34.1

Re: [PATCH 6/8] mmc: sdhci-of-aspeed: Add output timing phase tuning

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:58 +0800, Cool Lee wrote:
> Enhance auto tuning with input and output calibration.
> 

Might be best to squash this into the patch with input phase tuning?
Why split it?

Andrew

RE: [PATCH 6/8] mmc: sdhci-of-aspeed: Add output timing phase tuning

[Cool Lee]

> > Enhance auto tuning with input and output calibration.
> >
> 
> Might be best to squash this into the patch with input phase tuning?
> Why split it?
Ok, I will do that. Just like to make the commit smaller.

> 
> Andrew

[PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Cool Lee]

The timing phase is no more needed since the auto tuning is applied.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 178 +----------------------------
 1 file changed, 3 insertions(+), 175 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 92687fc30d1d..365c02215273 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -34,6 +34,9 @@
 #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
 #define   ASPEED_SDC_PHASE_MAX		31
 
+#define ASPEED_SDHCI_TAP_PARAM_INVERT_CLK	BIT(4)
+#define ASPEED_SDHCI_NR_TAPS		15
+
 /* SDIO{10,20} */
 #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
 /* SDIO{14,24} */
@@ -48,37 +51,14 @@ struct aspeed_sdc {
 	void __iomem *regs;
 };
 
-struct aspeed_sdhci_tap_param {
-	bool valid;
-
-#define ASPEED_SDHCI_TAP_PARAM_INVERT_CLK	BIT(4)
-	u8 in;
-	u8 out;
-};
-
-struct aspeed_sdhci_tap_desc {
-	u32 tap_mask;
-	u32 enable_mask;
-	u8 enable_value;
-};
-
-struct aspeed_sdhci_phase_desc {
-	struct aspeed_sdhci_tap_desc in;
-	struct aspeed_sdhci_tap_desc out;
-};
-
 struct aspeed_sdhci_pdata {
 	unsigned int clk_div_start;
-	const struct aspeed_sdhci_phase_desc *phase_desc;
-	size_t nr_phase_descs;
 };
 
 struct aspeed_sdhci {
 	const struct aspeed_sdhci_pdata *pdata;
 	struct aspeed_sdc *parent;
 	u32 width_mask;
-	struct mmc_clk_phase_map phase_map;
-	const struct aspeed_sdhci_phase_desc *phase_desc;
 };
 
 /*
@@ -129,114 +109,6 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 	spin_unlock(&sdc->lock);
 }
 
-static u32
-aspeed_sdc_set_phase_tap(const struct aspeed_sdhci_tap_desc *desc,
-			 u8 tap, bool enable, u32 reg)
-{
-	reg &= ~(desc->enable_mask | desc->tap_mask);
-	if (enable) {
-		reg |= tap << __ffs(desc->tap_mask);
-		reg |= desc->enable_value << __ffs(desc->enable_mask);
-	}
-
-	return reg;
-}
-
-static void
-aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
-			  const struct aspeed_sdhci_phase_desc *desc,
-			  const struct aspeed_sdhci_tap_param *taps)
-{
-	u32 reg;
-
-	spin_lock(&sdc->lock);
-	reg = readl(sdc->regs + ASPEED_SDC_PHASE);
-
-	reg = aspeed_sdc_set_phase_tap(&desc->in, taps->in, taps->valid, reg);
-	reg = aspeed_sdc_set_phase_tap(&desc->out, taps->out, taps->valid, reg);
-
-	writel(reg, sdc->regs + ASPEED_SDC_PHASE);
-	spin_unlock(&sdc->lock);
-}
-
-#define PICOSECONDS_PER_SECOND		1000000000000ULL
-#define ASPEED_SDHCI_NR_TAPS		15
-/* Measured value with *handwave* environmentals and static loading */
-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS	1253
-static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,
-				     int phase_deg)
-{
-	u64 phase_period_ps;
-	u64 prop_delay_ps;
-	u64 clk_period_ps;
-	unsigned int tap;
-	u8 inverted;
-
-	phase_deg %= 360;
-
-	if (phase_deg >= 180) {
-		inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
-		phase_deg -= 180;
-		dev_dbg(dev,
-			"Inverting clock to reduce phase correction from %d to %d degrees\n",
-			phase_deg + 180, phase_deg);
-	} else {
-		inverted = 0;
-	}
-
-	prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
-	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
-	phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
-
-	tap = div_u64(phase_period_ps, prop_delay_ps);
-	if (tap > ASPEED_SDHCI_NR_TAPS) {
-		dev_dbg(dev,
-			 "Requested out of range phase tap %d for %d degrees of phase compensation at %luHz, clamping to tap %d\n",
-			 tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
-		tap = ASPEED_SDHCI_NR_TAPS;
-	}
-
-	return inverted | tap;
-}
-
-static void
-aspeed_sdhci_phases_to_taps(struct device *dev, unsigned long rate,
-			    const struct mmc_clk_phase *phases,
-			    struct aspeed_sdhci_tap_param *taps)
-{
-	taps->valid = phases->valid;
-
-	if (!phases->valid)
-		return;
-
-	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
-	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
-}
-
-static void
-aspeed_sdhci_configure_phase(struct sdhci_host *host, unsigned long rate)
-{
-	struct aspeed_sdhci_tap_param _taps = {0}, *taps = &_taps;
-	struct mmc_clk_phase *params;
-	struct aspeed_sdhci *sdhci;
-	struct device *dev;
-
-	dev = mmc_dev(host->mmc);
-	sdhci = sdhci_pltfm_priv(sdhci_priv(host));
-
-	if (!sdhci->phase_desc)
-		return;
-
-	params = &sdhci->phase_map.phase[host->timing];
-	aspeed_sdhci_phases_to_taps(dev, rate, params, taps);
-	aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
-	dev_dbg(dev,
-		"Using taps [%d, %d] for [%d, %d] degrees of phase correction at %luHz (%d)\n",
-		taps->in & ASPEED_SDHCI_NR_TAPS,
-		taps->out & ASPEED_SDHCI_NR_TAPS,
-		params->in_deg, params->out_deg, rate, host->timing);
-}
-
 static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host;
@@ -287,8 +159,6 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	clk = div << SDHCI_DIVIDER_SHIFT;
 
-	aspeed_sdhci_configure_phase(host, bus);
-
 	sdhci_enable_clk(host, clk);
 }
 
@@ -550,14 +420,6 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	else if (slot >= 2)
 		return -EINVAL;
 
-	if (slot < dev->pdata->nr_phase_descs) {
-		dev->phase_desc = &dev->pdata->phase_desc[slot];
-	} else {
-		dev_info(&pdev->dev,
-			 "Phase control not supported for slot %d\n", slot);
-		dev->phase_desc = NULL;
-	}
-
 	dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
 
 	dev_info(&pdev->dev, "Configured for slot %d\n", slot);
@@ -589,9 +451,6 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_sdhci_add;
 
-	if (dev->phase_desc)
-		mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map);
-
 	ret = sdhci_add_host(host);
 	if (ret)
 		goto err_sdhci_add;
@@ -624,39 +483,8 @@ static const struct aspeed_sdhci_pdata ast2400_sdhci_pdata = {
 	.clk_div_start = 2,
 };
 
-static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
-	/* SDHCI/Slot 0 */
-	[0] = {
-		.in = {
-			.tap_mask = ASPEED_SDC_S0_PHASE_IN,
-			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
-			.enable_value = 1,
-		},
-		.out = {
-			.tap_mask = ASPEED_SDC_S0_PHASE_OUT,
-			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
-			.enable_value = 3,
-		},
-	},
-	/* SDHCI/Slot 1 */
-	[1] = {
-		.in = {
-			.tap_mask = ASPEED_SDC_S1_PHASE_IN,
-			.enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
-			.enable_value = 1,
-		},
-		.out = {
-			.tap_mask = ASPEED_SDC_S1_PHASE_OUT,
-			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
-			.enable_value = 3,
-		},
-	},
-};
-
 static const struct aspeed_sdhci_pdata ast2600_sdhci_pdata = {
 	.clk_div_start = 1,
-	.phase_desc = ast2600_sdhci_phase,
-	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
 };
 
 static const struct of_device_id aspeed_sdhci_of_match[] = {
-- 
2.34.1

Re: [PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:58 +0800, Cool Lee wrote:
> The timing phase is no more needed since the auto tuning is applied.
> 

I feel this is unwise: we're now ignoring constraints set in the
devicetree. Auto-tuning is fine, but I think that should be a feature
that new platforms can exploit by default. Older platforms that do
specify the phase values via the devicetree can be converted at the
leisure of their maintainers (by removing the phase properties).

Support needs to remain in the driver until there are no (aspeed-based)
devicetrees specifying the phases.

Andrew

RE: [PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Cool Lee]

> > The timing phase is no more needed since the auto tuning is applied.
> >
> 
> I feel this is unwise: we're now ignoring constraints set in the devicetree.
> Auto-tuning is fine, but I think that should be a feature that new platforms can
> exploit by default. Older platforms that do specify the phase values via the
> devicetree can be converted at the leisure of their maintainers (by removing
> the phase properties).
> 
> Support needs to remain in the driver until there are no (aspeed-based)
> devicetrees specifying the phases.
The timing phase only works on AST2600 or newer platform which has added a delay cell in the RTL.
The older platform AST2500, AST2400 doesn't support the timing phase.
It supposed no effect on older platform. 
The old manner that a static timing value customized from devicetree is inconvenient because customer needs to check waveform associated with each delay taps. Once the emmc parts changed, a fixed timing value may not work. That's why auto tune here instead of a static value.

> 
> Andrew

Re: [PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Andrew Jeffery]

On Fri, 2025-06-20 at 10:23 +0000, Cool Lee wrote:
> 
> > > The timing phase is no more needed since the auto tuning is
> > > applied.
> > > 
> > 
> > I feel this is unwise: we're now ignoring constraints set in the
> > devicetree.
> > Auto-tuning is fine, but I think that should be a feature that new
> > platforms can
> > exploit by default. Older platforms that do specify the phase
> > values via the
> > devicetree can be converted at the leisure of their maintainers (by
> > removing
> > the phase properties).
> > 
> > Support needs to remain in the driver until there are no (aspeed-
> > based)
> > devicetrees specifying the phases.
> The timing phase only works on AST2600 or newer platform which has
> added a delay cell in the RTL.
> The older platform AST2500, AST2400 doesn't support the timing phase.
> It supposed no effect on older platform. 
> The old manner that a static timing value customized from devicetree
> is inconvenient because customer needs to check waveform associated
> with each delay taps. Once the emmc parts changed, a fixed timing
> value may not work. That's why auto tune here instead of a static
> value.

Sure, I understand that auto-tuning is more convenient, but in my view,
there's no reason to remove support for static phase values for now. On
the contrary, switching entirely to auto-tuning risks regressions for
existing platforms that do specify static values.

Can you please drop the patch for now? We can revisit removing static
value support in the future.

Andrew

RE: [PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Cool Lee]

> >
> > > > The timing phase is no more needed since the auto tuning is
> > > > applied.
> > > >
> > >
> > > I feel this is unwise: we're now ignoring constraints set in the
> > > devicetree.
> > > Auto-tuning is fine, but I think that should be a feature that new
> > > platforms can exploit by default. Older platforms that do specify
> > > the phase values via the devicetree can be converted at the leisure
> > > of their maintainers (by removing the phase properties).
> > >
> > > Support needs to remain in the driver until there are no (aspeed-
> > > based)
> > > devicetrees specifying the phases.
> > The timing phase only works on AST2600 or newer platform which has
> > added a delay cell in the RTL.
> > The older platform AST2500, AST2400 doesn't support the timing phase.
> > It supposed no effect on older platform.
> > The old manner that a static timing value customized from devicetree
> > is inconvenient because customer needs to check waveform associated
> > with each delay taps. Once the emmc parts changed, a fixed timing
> > value may not work. That's why auto tune here instead of a static
> > value.
> 
> Sure, I understand that auto-tuning is more convenient, but in my view, there's
> no reason to remove support for static phase values for now. On the contrary,
> switching entirely to auto-tuning risks regressions for existing platforms that
> do specify static values.
> 
> Can you please drop the patch for now? We can revisit removing static value
> support in the future.

Ok, I got your point. I can make a new patch to keep static and dynamic both together. If the timing property kept then use it, otherwise try dynamic tuning. Is this OK?

> 
> Andrew

Re: [PATCH 7/8] mmc: sdhci-of-aspeed: Remove timing phase

[Andrew Jeffery]

On Wed, 2025-06-25 at 00:22 +0000, Cool Lee wrote:
> 
> > > 
> > > > > The timing phase is no more needed since the auto tuning is
> > > > > applied.
> > > > > 
> > > > 
> > > > I feel this is unwise: we're now ignoring constraints set in
> > > > the
> > > > devicetree.
> > > > Auto-tuning is fine, but I think that should be a feature that
> > > > new
> > > > platforms can exploit by default. Older platforms that do
> > > > specify
> > > > the phase values via the devicetree can be converted at the
> > > > leisure
> > > > of their maintainers (by removing the phase properties).
> > > > 
> > > > Support needs to remain in the driver until there are no
> > > > (aspeed-
> > > > based)
> > > > devicetrees specifying the phases.
> > > The timing phase only works on AST2600 or newer platform which
> > > has
> > > added a delay cell in the RTL.
> > > The older platform AST2500, AST2400 doesn't support the timing
> > > phase.
> > > It supposed no effect on older platform.
> > > The old manner that a static timing value customized from
> > > devicetree
> > > is inconvenient because customer needs to check waveform
> > > associated
> > > with each delay taps. Once the emmc parts changed, a fixed timing
> > > value may not work. That's why auto tune here instead of a static
> > > value.
> > 
> > Sure, I understand that auto-tuning is more convenient, but in my
> > view, there's
> > no reason to remove support for static phase values for now. On the
> > contrary,
> > switching entirely to auto-tuning risks regressions for existing
> > platforms that
> > do specify static values.
> > 
> > Can you please drop the patch for now? We can revisit removing
> > static value
> > support in the future.
> 
> Ok, I got your point. I can make a new patch to keep static and
> dynamic both together. If the timing property kept then use it,
> otherwise try dynamic tuning. Is this OK?

Yep, that's what I'm after.

Thanks,

Andrew

[PATCH 8/8] mmc: sdhci-of-aspeed: Add sdr50 support

[Cool Lee]

Add support for SDR50 mode in the Aspeed SDHCI driver by setting
capability shadow register.

Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 365c02215273..ec6833295b6a 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -41,6 +41,7 @@
 #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
 /* SDIO{14,24} */
 #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
+#define ASPEED_SDC_CAP2_SDR50		(1 * 32 + 0)
 
 struct aspeed_sdc {
 	struct clk *clk;
@@ -427,11 +428,17 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	sdhci_get_of_property(pdev);
 
 	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
+	    of_property_read_bool(np, "sd-uhs-sdr50") ||
 	    of_property_read_bool(np, "sd-uhs-sdr104")) {
 		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V,
 					       true, slot);
 	}
 
+	if (of_property_read_bool(np, "sd-uhs-sdr50")) {
+		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR50,
+					       true, slot);
+	}
+
 	if (of_property_read_bool(np, "sd-uhs-sdr104")) {
 		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104,
 					       true, slot);
-- 
2.34.1

Re: [PATCH 8/8] mmc: sdhci-of-aspeed: Add sdr50 support

[Andrew Jeffery]

On Sun, 2025-06-15 at 11:58 +0800, Cool Lee wrote:
> Add support for SDR50 mode in the Aspeed SDHCI driver by setting
> capability shadow register.
> 
> Signed-off-by: Cool Lee <cool_lee@aspeedtech.com>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>