[U-Boot] [PATCH] mmc: Implement card detection.

[Thierry Reding <thierry.reding@avionic-design.de>]

* Andy Fleming wrote:
> On Thu, Nov 17, 2011 at 5:51 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > Check for board-specific card detect each time an MMC/SD device is
> > initialized. If card detection is not implemented, this code behaves as
> > before and continues assuming a card is present. If no card is detected,
> > has_init is reset for the MMC/SD device (to force initialization next
> > time) and an error is returned.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> > ?drivers/mmc/mmc.c | ? ?9 +++++++++
> > ?1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index 37ce6e8..d18c095 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -1191,6 +1191,15 @@ block_dev_desc_t *mmc_get_dev(int dev)
> > ?int mmc_init(struct mmc *mmc)
> > ?{
> > ? ? ? ?int err, retry = 3;
> > + ? ? ? u8 card_detect = 0;
> > +
> > + ? ? ? if (board_mmc_getcd(&card_detect, mmc) == 0) {
> 
> 
> Well, now the time has come to think about how we want this to work.
> As some others noted, the FSL code has been treating the cd argument
> to board_mmc_getcd() as a bit that is asserted low. My inclination is
> to change all of them to work as you have proposed (cd == 1 ==> card
> is detected), but I'd like input from the community. Card detection
> isn't so speedy that the extra inversion required is much of a factor.
> Are there any good arguments for keeping the meaning as ~CD?
> 
> Also, to handle code like is in fsl_esdhc.c for fallback in case no
> board-specific code is written, I'm thinking we should use a mechanism
> similar to the ethernet drivers:
> 
>         if (board_eth_init != __def_eth_init) {
>                 if (board_eth_init(bis) < 0)
>                         printf("Board Net Initialization Failed\n");
>         } else if (cpu_eth_init != __def_eth_init) {
>                 if (cpu_eth_init(bis) < 0)
>                         printf("CPU Net Initialization Failed\n");
> 
> Basically, if the board_eth_init is set to something, call it.
> Otherwise, call the cpu_eth_init, if it exists.
> 
> So we could have:
> 
> int mmc_getcd(mmc, &cd) // mmc should always have been the first argument...

Yes, the cd parameter really should be second, or, as you propose later, not
be a parameter at all.

> {
>     if (board_mmc_getcd != __def_mmc_getcd)
>         return board_mmc_getcd(mmc, cd);
>     else if (cpu_mmc_getcd != __def_mmc_getcd)
>         return cpu_mmc_getcd(mmc, cd);
> 
>     return -1;
> }

I don't see how that buys us much. What's wrong with the following? That's
much more straightforward in my opinion.

	int mmc_getcd(struct mmc *mmc)
	{
		int cd;

		cd = board_mmc_getcd(mmc);
		if (cd < 0)
			cd = cpu_mmc_getcd(mmc);

		return cd;
	}

I don't have any real objections, though. It's probably more of a matter of
taste than technical merit. The only advantage that the second version has is
that it allows the cpu_mmc_getcd() to serve as fallback if board_mmc_getcd()
is implemented but still returns -1 (not implemented, for whatever reason).

> Open questions:
> 1) If we use this sort of mechanism, we don't really need the "-1
> means it's not implemented" error. Perhaps we could change it to be:
>     cd = mmc_getcd(mmc) ?

That can even work if we keep -1 to mean "not implemented".

> 2) Should we add the ability for *drivers* to specify a card detect
> mechanism, and if so, where should it fit in the priority order? Most
> systems seem to provide GPIOs for the card detection, and even the
> controllers which support card detection from the device are often not
> always hooked up in such a way to actually *use* that support.

I'm not sure I understand what cpu_mmc_getcd() is supposed to mean. Shouldn't
that really be the driver default implementation instead? Typically the CPU
implementation will be the SoC implementation, right? In that case it would
really be the driver card-detect mechanism, not that of the CPU.

> > + ? ? ? ? ? ? ? if (!card_detect) {
> > + ? ? ? ? ? ? ? ? ? ? ? mmc->has_init = 0;
> > + ? ? ? ? ? ? ? ? ? ? ? printf("MMC: no card present\n");
> > + ? ? ? ? ? ? ? ? ? ? ? return NO_CARD_ERR;
> > + ? ? ? ? ? ? ? }
> > + ? ? ? }
> 
> Anyway, I liked this patch, and want to apply it, but there are some
> issues we need to resolve before we can apply it.
> 
> Andy

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111128/001809eb/attachment.pgp>