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

[Tom Rini]

On Mon, Sep 09, 2024 at 10:46:21AM +0200, Rasmus Villemoes wrote:
> Marek Vasut <marek.vasut+renesas at mailbox.org> writes:
> 
> > 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(+)
> >
> 
> [rearranging hunks for easier reading]
> 
> > 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));
> 
> 
> I think that you can simplify this quite a lot by dropping all of the
> the CONFIG_IS_ENABLED stuff. If CYCLIC is not enabled, struct
> cyclic_info is an empty struct, so takes up no space, but it still
> exists in struct mmc, allowing it to be referenced in C code that need
> not be guarded.
> 
> >  };
> >  
> >  #if CONFIG_IS_ENABLED(DM_MMC)
> 
> > 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)
> > +{
> 
> You can drop __maybe_unused.
> 
> > +	struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));
> > +
> 
> No need for the CONFIG_IS_ENABLED(), container_of works just fine when
> the cyclic member exists unconditionally.
> 
> > +	if (!m->has_init)
> > +		return;
> > +
> 
> (I'd assume that in the !CYCLIC case the compiler might warn about this
> unconditional NULL deref; that you avoid with the above.)
> 
> > +	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)));
> > +	}
> > +
> 
> No need for any of the CONFIG_IS_ENABLED nesting. Just do
> cyclic_register() - that's a no-op when !CYCLIC, and the compiler will
> see the reference to mmc_cyclic_cd_poll, so not warn about it being
> unused, yet also see that it is not actually called, so elide it from
> the compiled code.
> 
> >  	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)));
> > +
> 
> Again, just do cyclic_unregister() unconditionally.

The challenge here is that Simon asked for all of this as part of
feedback for v3. What are your thoughts here, Simon?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240909/94f77b8d/attachment.sig>