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

[Andrew Jeffery <andrew@codeconstruct.com.au>]

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