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

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <TYSPR06MB706845CACFD7095F53BBCDCD957FA@TYSPR06MB7068.apcprd06.prod.outlook.com>
Date: Sat, 21 Jun 2025 08:29:31 +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 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset can't
 be cleared issue.


> >
> > > > Replace sdhci software reset by scu reset from top.
> > > >
> > > > Signed-off-by: Cool Lee <cool_lee@...eedtech.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