Hello,

On Thu, 2025-06-19 at 06:53 +0000, Cool Lee wrote:

quoted

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.

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.

quoted

quoted

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.

Can you please put these details in the commit message?

quoted

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?

Yes, please.

Andrew