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] [thread-next>] [day] [month] [year] [list]
Message-ID: <80f56269175d8658ba1ab4a1fe9a43d18294ca60.camel@codeconstruct.com.au>
Date: Wed, 18 Jun 2025 11:44:00 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Cool Lee <cool_lee@...eedtech.com>, adrian.hunter@...el.com, 
	ulf.hansson@...aro.org, joel@....id.au, p.zabel@...gutronix.de, 
	linux-aspeed@...ts.ozlabs.org, openbmc@...ts.ozlabs.org, 
	linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/8] mmc: sdhci-of-aspeed: Fix sdhci software reset
 can't be cleared issue.

Hi,

On Sun, 2025-06-15 at 11:57 +0800, Cool Lee wrote:
> 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?

> ---
>  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?

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

Andrew