quoted

Replace sdhci software reset by scu reset from top.

Signed-off-by: Cool Lee <redacted>

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?

quoted

---
 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);

quoted

+               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.

quoted

+
+               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);

quoted

+               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.

quoted

+}
+
 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