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

[Simon Glass]

Hi Marek,

On Fri, 6 Sept 2024 at 11:11, Marek Vasut
<marek.vasut+renesas at 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 at mailbox.org>
> ---
> Cc: Jaehoon Chung <jh80.chung at samsung.com>
> Cc: Peng Fan <peng.fan at nxp.com>
> Cc: Simon Glass <sjg at 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 at 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