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

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

[Thierry Reding]

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) {
+		if (!card_detect) {
+			mmc->has_init = 0;
+			printf("MMC: no card present\n");
+			return NO_CARD_ERR;
+		}
+	}
 
 	if (mmc->has_init)
 		return 0;
-- 
1.7.7.3

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

[Thierry Reding]

* Thierry Reding 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) {
> +		if (!card_detect) {
> +			mmc->has_init = 0;
> +			printf("MMC: no card present\n");
> +			return NO_CARD_ERR;
> +		}
> +	}
>  
>  	if (mmc->has_init)
>  		return 0;
> -- 
> 1.7.7.3

I'm adding Andy Fleming to Cc with the official email address as listed on
http://www.denx.de/wiki/U-Boot/Custodians.

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/20111122/fcccb6db/attachment.pgp>

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

[Andy Fleming]

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...
{
    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;
}

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) ?
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.



> + ? ? ? ? ? ? ? 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

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

[Thierry Reding]

* 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>

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

[Andy Fleming]

On Mon, Nov 28, 2011 at 11:47 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
>>
>> 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).


That's a good point. I don't have any issues with this implementation.


>
>> 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.


Well, while this isn't the case in any systems I know of now, it is
quite possible for there to be more than one *type* of SD/MMC
controller on an SoC, and there's always the possibility that an SoC
provides a non-controller-specific card-detect mechanism. The idea is
that, lacking a board-specific card-detect mechanism, the SoC might be
able to direct the query to the right place.

But I'm talking very theoretically, here. I wouldn't object to a
mechanism that was just:

cd = board_mmc_getcd(mmc);

if (cd < 0 && mmc->getcd)
   cd = mmc->getcd(mmc);

If we ever ran into a case where an SoC had better knowledge than the
driver, then it's easy to fix the code.

Andy

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

[Thierry Reding]

* Andy Fleming wrote:
[...]
> Well, while this isn't the case in any systems I know of now, it is
> quite possible for there to be more than one *type* of SD/MMC
> controller on an SoC, and there's always the possibility that an SoC
> provides a non-controller-specific card-detect mechanism. The idea is
> that, lacking a board-specific card-detect mechanism, the SoC might be
> able to direct the query to the right place.
> 
> But I'm talking very theoretically, here. I wouldn't object to a
> mechanism that was just:
> 
> cd = board_mmc_getcd(mmc);
> 
> if (cd < 0 && mmc->getcd)
>    cd = mmc->getcd(mmc);
> 
> If we ever ran into a case where an SoC had better knowledge than the
> driver, then it's easy to fix the code.

That makes sense. The code can always be extended when new hardware requires
it. No need to over-engineer at this point.

Do you want me to prepare a patch or should we rather wait for some more
input from others?

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/20111129/b4ff9217/attachment.pgp>

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

[Andy Fleming]

On Tue, Nov 29, 2011 at 1:02 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> * Andy Fleming wrote:
> [...]
>> Well, while this isn't the case in any systems I know of now, it is
>> quite possible for there to be more than one *type* of SD/MMC
>> controller on an SoC, and there's always the possibility that an SoC
>> provides a non-controller-specific card-detect mechanism. The idea is
>> that, lacking a board-specific card-detect mechanism, the SoC might be
>> able to direct the query to the right place.
>>
>> But I'm talking very theoretically, here. I wouldn't object to a
>> mechanism that was just:
>>
>> cd = board_mmc_getcd(mmc);
>>
>> if (cd < 0 && mmc->getcd)
>> ? ?cd = mmc->getcd(mmc);
>>
>> If we ever ran into a case where an SoC had better knowledge than the
>> driver, then it's easy to fix the code.
>
> That makes sense. The code can always be extended when new hardware requires
> it. No need to over-engineer at this point.
>
> Do you want me to prepare a patch or should we rather wait for some more
> input from others?

I'll happily take a patch. I may wait to apply it, but probably, as
the merge window is *long* closed, so it won't get pulled into
mainline until mid-December (I'll apply it to my -next branch).

Andy

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

[Thierry Reding]

* Andy Fleming wrote:
> On Tue, Nov 29, 2011 at 1:02 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > * Andy Fleming wrote:
> > [...]
> >> Well, while this isn't the case in any systems I know of now, it is
> >> quite possible for there to be more than one *type* of SD/MMC
> >> controller on an SoC, and there's always the possibility that an SoC
> >> provides a non-controller-specific card-detect mechanism. The idea is
> >> that, lacking a board-specific card-detect mechanism, the SoC might be
> >> able to direct the query to the right place.
> >>
> >> But I'm talking very theoretically, here. I wouldn't object to a
> >> mechanism that was just:
> >>
> >> cd = board_mmc_getcd(mmc);
> >>
> >> if (cd < 0 && mmc->getcd)
> >> ? ?cd = mmc->getcd(mmc);
> >>
> >> If we ever ran into a case where an SoC had better knowledge than the
> >> driver, then it's easy to fix the code.
> >
> > That makes sense. The code can always be extended when new hardware requires
> > it. No need to over-engineer at this point.
> >
> > Do you want me to prepare a patch or should we rather wait for some more
> > input from others?
> 
> I'll happily take a patch. I may wait to apply it, but probably, as
> the merge window is *long* closed, so it won't get pulled into
> mainline until mid-December (I'll apply it to my -next branch).

Okay. I may not get to it until later this week anyway.

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/20111129/4e1f0c9c/attachment.pgp>