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: <CAKekbetcu9QHi2j-EYG2JCqDU=M8_5wMohbmTvkDU2nJjx6ySQ@mail.gmail.com>
Date: Mon, 6 Oct 2025 14:25:05 -0400
From: Kamal Dasu <kamal.dasu@...adcom.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: andersson@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, florian.fainelli@...adcom.com, ulf.hansson@...aro.org, 
	bcm-kernel-feedback-list@...adcom.com, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-mmc@...r.kernel.org
Subject: Re: [PATCH 3/3] mmc: brcmstb: save and restore registers during PM

On Mon, Oct 6, 2025 at 6:22 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 06/10/2025 13:08, Adrian Hunter wrote:
> > On 03/10/2025 00:04, Kamal Dasu wrote:
> >> Added support to save and restore registers that are critical
> >> during PM.
> >>
> >> Signed-off-by: Kamal Dasu <kamal.dasu@...adcom.com>
> >> ---
> >>  drivers/mmc/host/sdhci-brcmstb.c | 124 +++++++++++++++++++++++++++++--
> >>  1 file changed, 119 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> >> index 0905b316a24b..ffa602a99ab7 100644
> >> --- a/drivers/mmc/host/sdhci-brcmstb.c
> >> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> >> @@ -24,7 +24,9 @@
> >>  #define BRCMSTB_MATCH_FLAGS_NO_64BIT                BIT(0)
> >>  #define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT  BIT(1)
> >>  #define BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE  BIT(2)
> >> -#define BRCMSTB_MATCH_FLAGS_USE_CARD_BUSY   BIT(4)
> >> +#define BRCMSTB_MATCH_FLAGS_HAS_CFG_V1              BIT(3)
> >> +#define BRCMSTB_MATCH_FLAGS_HAS_CFG_V2              BIT(4)
> >> +#define BRCMSTB_MATCH_FLAGS_USE_CARD_BUSY   BIT(5)
> >>
> >>  #define BRCMSTB_PRIV_FLAGS_HAS_CQE          BIT(0)
> >>  #define BRCMSTB_PRIV_FLAGS_GATE_CLOCK               BIT(1)
> >> @@ -38,19 +40,39 @@
> >>  #define SDIO_CFG_OP_DLY_DEFAULT                     0x80000003
> >>  #define SDIO_CFG_CQ_CAPABILITY                      0x4c
> >>  #define SDIO_CFG_CQ_CAPABILITY_FMUL         GENMASK(13, 12)
> >> +#define SDIO_CFG_SD_PIN_SEL                 0x44
> >> +#define SDIO_CFG_V1_SD_PIN_SEL                      0x54
> >> +#define SDIO_CFG_PHY_SW_MODE_0_RX_CTRL              0x7C
> >>  #define SDIO_CFG_MAX_50MHZ_MODE                     0x1ac
> >>  #define SDIO_CFG_MAX_50MHZ_MODE_STRAP_OVERRIDE      BIT(31)
> >>  #define SDIO_CFG_MAX_50MHZ_MODE_ENABLE              BIT(0)
> >>
> >> +#define SDIO_BOOT_MAIN_CTL                  0x0
> >> +
> >>  #define MMC_CAP_HSE_MASK    (MMC_CAP2_HSX00_1_2V | MMC_CAP2_HSX00_1_8V)
> >>  /* Select all SD UHS type I SDR speed above 50MB/s */
> >>  #define MMC_CAP_UHS_I_SDR_MASK      (MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104)
> >>
> >> +enum cfg_core_ver {
> >> +    SDIO_CFG_CORE_V1 = 1,
> >> +    SDIO_CFG_CORE_V2,
> >> +};
> >> +
> >> +struct sdhci_brcmstb_saved_regs {
> >> +    u32 sd_pin_sel;
> >> +    u32 phy_sw_mode0_rxctrl;
> >> +    u32 max_50mhz_mode;
> >> +    u32 boot_main_ctl;
> >> +};
> >> +
> >>  struct sdhci_brcmstb_priv {
> >>      void __iomem *cfg_regs;
> >> +    void __iomem *boot_regs;
> >> +    struct sdhci_brcmstb_saved_regs saved_regs;
> >>      unsigned int flags;
> >>      struct clk *base_clk;
> >>      u32 base_freq_hz;
> >> +    void (*save_restore_regs)(struct mmc_host *mmc, int save);
> >>  };
> >>
> >>  struct brcmstb_match_priv {
> >> @@ -60,6 +82,69 @@ struct brcmstb_match_priv {
> >>      const unsigned int flags;
> >>  };
> >>
> >> +static void sdhci_brcmstb_save_regs(struct mmc_host *mmc, enum cfg_core_ver ver)
> >> +{
> >> +    struct sdhci_host *host = mmc_priv(mmc);
> >> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +    struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> +    struct sdhci_brcmstb_saved_regs *sr = &priv->saved_regs;
> >> +    void __iomem *cr = priv->cfg_regs;
> >> +    bool is_emmc = mmc->caps & MMC_CAP_NONREMOVABLE;
> >> +
> >> +    /* save  */
>
> Comment is not really needed.
>
> >> +    if (is_emmc && priv->boot_regs)
> >> +            sr->boot_main_ctl = readl(priv->boot_regs + SDIO_BOOT_MAIN_CTL);
> >> +
> >> +    if (ver == SDIO_CFG_CORE_V1) {
> >> +            sr->sd_pin_sel = readl(cr + SDIO_CFG_V1_SD_PIN_SEL);
> >> +            return;
> >> +    }
> >> +
> >> +    sr->sd_pin_sel = readl(cr + SDIO_CFG_SD_PIN_SEL);
> >> +    sr->phy_sw_mode0_rxctrl = readl(cr + SDIO_CFG_PHY_SW_MODE_0_RX_CTRL);
> >> +    sr->max_50mhz_mode = readl(cr + SDIO_CFG_MAX_50MHZ_MODE);
> >> +}
> >> +
> >> +static void sdhci_brcmstb_restore_regs(struct mmc_host *mmc,
> >> +                                            enum cfg_core_ver ver)
>
> Prefer to line up function parameters.  Using up to 100 columns is also ok.
>
> >> +{
> >> +    struct sdhci_host *host = mmc_priv(mmc);
> >> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +    struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> +    struct sdhci_brcmstb_saved_regs *sr = &priv->saved_regs;
> >> +    void __iomem *cr = priv->cfg_regs;
> >> +    bool is_emmc = mmc->caps & MMC_CAP_NONREMOVABLE;
> >> +
> >> +    /* restore */
>
> Comment is not really needed.
>
> >> +    if (is_emmc && priv->boot_regs)
> >> +            writel(sr->boot_main_ctl, priv->boot_regs + SDIO_BOOT_MAIN_CTL);
> >> +
> >> +    if (ver == SDIO_CFG_CORE_V1) {
> >> +            writel(sr->sd_pin_sel, cr + SDIO_CFG_SD_PIN_SEL);
> >> +            return;
> >> +    }
> >> +
> >> +    writel(sr->sd_pin_sel, cr + SDIO_CFG_SD_PIN_SEL);
> >> +    writel(sr->phy_sw_mode0_rxctrl, cr + SDIO_CFG_PHY_SW_MODE_0_RX_CTRL);
> >> +    writel(sr->max_50mhz_mode, cr + SDIO_CFG_MAX_50MHZ_MODE);
> >> +}
> >> +
> >> +static void sdhci_brcmstb_save_restore_regs_v1(struct mmc_host *mmc, int save)
> >> +{
> >> +    if (save)
> >> +            sdhci_brcmstb_save_regs(mmc, SDIO_CFG_CORE_V1);
> >> +    else
> >> +            sdhci_brcmstb_restore_regs(mmc, SDIO_CFG_CORE_V1);
> >> +}
> >> +
> >> +static void sdhci_brcmstb_save_restore_regs_v2(struct mmc_host *mmc, int save)
> >> +{
> >> +    if (save)
> >> +            sdhci_brcmstb_save_regs(mmc, SDIO_CFG_CORE_V2);
> >> +    else
> >> +            sdhci_brcmstb_restore_regs(mmc, SDIO_CFG_CORE_V2);
> >> +}
> >> +
> >>  static inline void enable_clock_gating(struct sdhci_host *host)
> >>  {
> >>      struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> @@ -300,24 +385,33 @@ static struct brcmstb_match_priv match_priv_7425 = {
> >>      .ops = &sdhci_brcmstb_ops,
> >>  };
> >>
> >> -static struct brcmstb_match_priv match_priv_7445 = {
> >> +static struct brcmstb_match_priv match_priv_74371 = {
> >>      .flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
> >>      .ops = &sdhci_brcmstb_ops,
> >>  };
> >>
> >> +static struct brcmstb_match_priv match_priv_7445 = {
> >> +    .flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT |
> >> +                    BRCMSTB_MATCH_FLAGS_HAS_CFG_V1,
> >> +    .ops = &sdhci_brcmstb_ops,
> >> +};
> >> +
> >>  static struct brcmstb_match_priv match_priv_72116 = {
> >> -    .flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
> >> +    .flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT |
> >> +                    BRCMSTB_MATCH_FLAGS_HAS_CFG_V1,
> >>      .ops = &sdhci_brcmstb_ops_72116,
> >>  };
> >>
> >>  static const struct brcmstb_match_priv match_priv_7216 = {
> >> -    .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
> >> +    .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE |
> >> +                    BRCMSTB_MATCH_FLAGS_HAS_CFG_V2,
> >>      .hs400es = sdhci_brcmstb_hs400es,
> >>      .ops = &sdhci_brcmstb_ops_7216,
> >>  };
> >>
> >>  static struct brcmstb_match_priv match_priv_74165b0 = {
> >> -    .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
> >> +    .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE |
> >> +                    BRCMSTB_MATCH_FLAGS_HAS_CFG_V2,
> >>      .hs400es = sdhci_brcmstb_hs400es,
> >>      .ops = &sdhci_brcmstb_ops_74165b0,
> >>  };
> >> @@ -325,6 +419,7 @@ static struct brcmstb_match_priv match_priv_74165b0 = {
> >>  static const struct of_device_id __maybe_unused sdhci_brcm_of_match[] = {
> >>      { .compatible = "brcm,bcm2712-sdhci", .data = &match_priv_2712 },
> >>      { .compatible = "brcm,bcm7425-sdhci", .data = &match_priv_7425 },
> >> +    { .compatible = "brcm,bcm74371-sdhci", .data = &match_priv_74371 },
> >
> > Shouldn't adding brcm,bcm74371-sdhci be a separate patch?
> >
> >>      { .compatible = "brcm,bcm7445-sdhci", .data = &match_priv_7445 },
> >>      { .compatible = "brcm,bcm72116-sdhci", .data = &match_priv_72116 },
> >>      { .compatible = "brcm,bcm7216-sdhci", .data = &match_priv_7216 },
> >> @@ -441,6 +536,19 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
> >>      if (res)
> >>              goto err;
> >>
> >> +    /* map non-standard BOOT registers if present */
> >> +    if (host->mmc->caps & MMC_CAP_NONREMOVABLE) {
> >> +            priv->boot_regs = devm_platform_get_and_ioremap_resource(pdev, 2, NULL);
> >> +            if (IS_ERR(priv->boot_regs))
> >> +                    priv->boot_regs = NULL;
> >> +    }
> >> +
> >> +    if (match_priv->flags & BRCMSTB_MATCH_FLAGS_HAS_CFG_V1)
> >> +            priv->save_restore_regs = sdhci_brcmstb_save_restore_regs_v1;
> >> +
> >> +    if (match_priv->flags & BRCMSTB_MATCH_FLAGS_HAS_CFG_V2)
> >> +            priv->save_restore_regs = sdhci_brcmstb_save_restore_regs_v2;
> >
> > It is not ideal to use a flag to set a callback.  Perhaps sdhci_brcmstb_priv
> > should have a pointer to brcmstb_match_priv, then ->save_restore_regs() could
> > just be put there, and BRCMSTB_MATCH_FLAGS_HAS_CFG_V* are not needed.
> >
> >> +
> >>      /*
> >>       * Automatic clock gating does not work for SD cards that may
> >>       * voltage switch so only enable it for non-removable devices.
> >> @@ -533,6 +641,9 @@ static int sdhci_brcmstb_suspend(struct device *dev)
> >>      struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >>      int ret;
> >>
> >> +    if (priv->save_restore_regs)
> >> +            priv->save_restore_regs(host->mmc, 1);
> >> +
> >>      clk_disable_unprepare(priv->base_clk);
> >>      if (host->mmc->caps2 & MMC_CAP2_CQE) {
> >>              ret = cqhci_suspend(host->mmc);
> >> @@ -564,6 +675,9 @@ static int sdhci_brcmstb_resume(struct device *dev)
> >>                      ret = clk_set_rate(priv->base_clk, priv->base_freq_hz);
> >>      }
> >>
> >> +    if (priv->save_restore_regs)
> >> +            priv->save_restore_regs(host->mmc, 0);
> >> +
> >>      if (host->mmc->caps2 & MMC_CAP2_CQE)
> >>              ret = cqhci_resume(host->mmc);
> >>
> >
>
Thanks for the review. Ok let me fix all the review comments and send a v2.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5461 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ