| 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
| ||
|
Message-ID:
<TYSPR06MB7068A5F04CE0A5BDB03073C0957DA@TYSPR06MB7068.apcprd06.prod.outlook.com>
Date: Thu, 19 Jun 2025 06:53:24 +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.
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
Powered by blists - more mailing lists