Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

Hi Marek,

On Fri, 6 Sept 2024 at 11:11, Marek Vasut marek.vasut+renesas@mailbox.org wrote:

In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.

Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org

Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org

---

V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next

---

drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++ include/mmc.h | 3 +++ 2 files changed, 28 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

Some things below to do now or later.

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0b881f11b4a..c787ff6bc49 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc) return err; }

+static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c)

Add a comment here to explain what is going on - e.g. stuff from your comment message.

+{

•   struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));
  

•   if (!m->has_init)
  

•           return;
  

•   if (mmc_getcd(m))
  

•           return;
  

•   mmc_deinit(m);
  

•   m->has_init = 0;
  

+}

int mmc_init(struct mmc *mmc) { int err = 0; @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc) if (err) pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));

•   if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {
  

•           /* Register cyclic function for card detect polling */
  

•           CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic,
  

•                                                      mmc_cyclic_cd_poll,
  

•                                                      100 * 1000,
  

•                                                      mmc->cfg->name)));
  

•   }
  

This seems a bit confusing. How about:

if (CONFIG_IS_ENABLED(CYCLIC) && !cyclic_mmc->cyclic.func)

then in cyclic.h something like

static inline bool cyclic_valid(struct cyclic *cyc) { #if CONFIG_IS_ENABLED(CYCLIC) return cyc->func; #else return false; #endif }

We really should not have clients looking at the 'func' to decide if something is active.

You can also turn cyclic_register into a macro (empty if cyclic not enabled) to avoid the problem of ->cyclic not existing.

    return err;

}

@@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc) { u32 caps_filtered;

•   if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))
  

•           CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic)));
  

Same comment here.

•   if (!CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) &&
        !CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) &&
        !CONFIG_IS_ENABLED(MMC_HS400_SUPPORT))
  

diff --git a/include/mmc.h b/include/mmc.h index f508cd15700..0044ff8bef7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -14,6 +14,7 @@ #include <linux/sizes.h> #include <linux/compiler.h> #include <linux/dma-direction.h> +#include <cyclic.h> #include <part.h>

struct bd_info; @@ -757,6 +758,8 @@ struct mmc { bool hs400_tuning:1;

    enum bus_mode user_speed_mode; /* input speed mode from user */

•   CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic));
  

};

#if CONFIG_IS_ENABLED(DM_MMC)

2.45.2

Regards, Simon