Skip to contentSkip to conversation

sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

PATCH In review Jaehoon Chung 7 participants 22 messages Aug 27, 2010 – Sep 27, 2010

Subsystems (2)
MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver
lore
Share & tools
lore

Jaehoon Chung

author
#0 lore raw

PATCH 1/1 sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -2
index 401527d..4bc5d3c 100644
@@ -1826,8 +1826,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!(host->mmc->caps & MMC_CAP_NONREMOVABLE))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)
Signature · 1 line
1.6.0.4
#1 lore raw
Quoted 33 lines · Jaehoon Chung

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -2
index 401527d..4bc5d3c 100644
@@ -1826,8 +1826,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!(host->mmc->caps & MMC_CAP_NONREMOVABLE))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)
Signature · 2 lines
1.6.0.4
--

I don't have any non-removable hardware so I can't test this patch but I think it makes sense. The indentation looks a bit weird, but Andrew may fix that up if he thinks it's a problem.

I'm assuming that you're testing this change with the sdhci-s3c.c driver? Is it a good idea that the driver is using SDHCI_QUIRK_BROKEN_CARD_DETECTION when it has a nonremovable card? Currently we seem to be using SDHCI_QUIRK_BROKEN_CARD_DETECTION and MMC_CAP_NONREMOVABLE to mean that same thing in some places which is really confusing.

Like in drivers/mmc/host/sdhci.c we can enable card detection irqs for cards that are nonremovable which is a bit pointless (though obviously not harmful).

But your patch seems fine unless anyone else has an issue with it, I'm just trying to see if we could clean this up ontop of your patch.

Adding Andrew Morton to the CC list.

#2 lore raw
Quoted 39 lines · Matt Fleming
Quoted 31 lines · Jaehoon Chung

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>

---
 drivers/mmc/host/sdhci.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -3
index 401527d..4bc5d3c 100644
@@ -1826,8 +1826,9 @@ int sdhci_add_host(struct sdhci_host *host)
       if (caps & SDHCI_CAN_DO_HISPD)
               mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
-     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-             mmc->caps |= MMC_CAP_NEEDS_POLL;
+     if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+             !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
+                     mmc->caps |= MMC_CAP_NEEDS_POLL;
 
       mmc->ocr_avail = 0;
       if (caps & SDHCI_CAN_VDD_330)
--
Quoted 2 lines

1.6.0.4
--

I don't have any non-removable hardware so I can't test this patch but I think it makes sense. The indentation looks a bit weird, but Andrew may fix that up if he thinks it's a problem.

Quoted 6 lines

I'm assuming that you're testing this change with the sdhci-s3c.c driver? Is it a good idea that the driver is using SDHCI_QUIRK_BROKEN_CARD_DETECTION when it has a nonremovable card? Currently we seem to be using SDHCI_QUIRK_BROKEN_CARD_DETECTION and MMC_CAP_NONREMOVABLE to mean that same thing in some places which is really confusing.

There are some reason to use both.
s5pc110/c210 has 3 or 4 mmc devices and basically our board don't connect the card detection pin to sdhci.
That's reason to define BROKEN_CARD_DETECTION. but some devices such as eMMC and WiFi has connected permanently.
it's reason to define CAP_NONREMOVABLE for these devices.

The main reason of this patch is that don't poll anymore when it's non-removable.

Thank you,
Kyungmin Park

Quoted 14 lines

Like in drivers/mmc/host/sdhci.c we can enable card detection irqs for cards that are nonremovable which is a bit pointless (though obviously not harmful).

But your patch seems fine unless anyone else has an issue with it, I'm just trying to see if we could clean this up ontop of your patch.

Adding Andrew Morton to the CC list.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 34 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kyungmin Park
Sent: Friday, August 27, 2010 3:15 PM
To: Matt Fleming
Cc: Jaehoon Chung; [email protected]; Marek Szyprowski; Andrew Morton; Ben Dooks
Subject: Re: [PATCH] sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

Quoted 5 lines · Matt Fleming
Quoted 4 lines · Jaehoon Chung

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card

status.

Quoted 18 lines
Quoted 18 lines

I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>

---
 drivers/mmc/host/sdhci.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index 401527d..4bc5d3c 100644
@@ -1826,8 +1826,9 @@ int sdhci_add_host(struct sdhci_host *host)
       if (caps & SDHCI_CAN_DO_HISPD)
               mmc->caps |= MMC_CAP_SD_HIGHSPEED;
Quoted 5 lines
Quoted 5 lines

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1831

Quoted 5 lines
drivers/mmc/host/sdhci.c +3 -2
-     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-             mmc->caps |= MMC_CAP_NEEDS_POLL;
+     if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+             !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
+                     mmc->caps |= MMC_CAP_NEEDS_POLL;

Since module parameter 'mmc_assume_removable' (defined in core.c) also controls the nonremovable attribute of the mmc/eMMC device, will it be better to also check value of 'mmc_assume_removable' here? Thanks.

Regards,
Yunpeng Gao

#4 lore raw
Quoted 34 lines · Yunpeng
Quoted 34 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kyungmin Park
Sent: Friday, August 27, 2010 3:15 PM
To: Matt Fleming
Cc: Jaehoon Chung; [email protected]; Marek Szyprowski; Andrew Morton; Ben Dooks
Subject: Re: [PATCH] sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

Quoted 5 lines · Matt Fleming
Quoted 4 lines · Jaehoon Chung

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card

status.

Quoted 18 lines
Quoted 18 lines

I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>

---
 drivers/mmc/host/sdhci.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index 401527d..4bc5d3c 100644
@@ -1826,8 +1826,9 @@ int sdhci_add_host(struct sdhci_host *host)
       if (caps & SDHCI_CAN_DO_HISPD)
               mmc->caps |= MMC_CAP_SD_HIGHSPEED;
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1831

Quoted 5 lines
drivers/mmc/host/sdhci.c +3 -2
-     if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-             mmc->caps |= MMC_CAP_NEEDS_POLL;
+     if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+             !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
+                     mmc->caps |= MMC_CAP_NEEDS_POLL;

Since module parameter 'mmc_assume_removable' (defined in core.c) also controls the nonremovable attribute of the mmc/eMMC device, will it be better to also check value of 'mmc_assume_removable' here? Thanks.

Yeah, I think that check is also needed. I've got a couple of patches that I'll reply to this thread with. There should really be a helper function for determining when a card is either physially non-removable (eMMC) or logically non-removable (mmc_assume_removable == 0).

#5 lore raw

PATCH 1/2 mmc: Add helper function to check if a card is removable

There are two checks that need to be made when determining whether a card is removable. A host controller may set MMC_CAP_NONREMOVABLE if the controller does not support removing cards (e.g. eMMC), in which case the card is physically non-removable. Also the 'mmc_assume_removable' module parameter can be configured at module load time, in which case the card may be logically non-removable.

A helper function keeps the logic in one place so that code always checks both conditions.

Signed-off-by Matt Fleming <[email protected]>
---
 drivers/mmc/core/mmc.c   |    2 +-
 drivers/mmc/core/sd.c    |    2 +-
 include/linux/mmc/host.h |    7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)
drivers/mmc/core/mmc.c +1 -1
index dcfc921..66c4a59 100644
@@ -718,7 +718,7 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_ops_unsafe;
 	else
 		bus_ops = &mmc_ops;
drivers/mmc/core/sd.c +1 -1
index 0f52410..bc745e1 100644
@@ -750,7 +750,7 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_sd_ops_unsafe;
 	else
 		bus_ops = &mmc_sd_ops;
include/linux/mmc/host.h +7 -0
index 09dbb90..cf4b71f 100644
@@ -272,5 +272,12 @@ static inline void mmc_set_disable_delay(struct mmc_host *host,
 	host->disable_delay = disable_delay;
 }
 
+extern int mmc_assume_removable;
+
+static inline int mmc_card_is_removable(struct mmc_host *host)
+{
+	return (!(host->caps & MMC_CAP_NONREMOVABLE) || mmc_assume_removable);
+}
+
 #endif
Signature · 1 line
1.7.1
#6 lore raw

PATCH 2/2 sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -2
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)
Signature · 1 line
1.7.1
Acked-by on v1 2/2
#7 lore raw
Acked-by Kyungmin Park <[email protected]>
Quoted 40 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>

---
 drivers/mmc/host/sdhci.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -3
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
        if (caps & SDHCI_CAN_DO_HISPD)
                mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-       if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-               mmc->caps |= MMC_CAP_NEEDS_POLL;
+       if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+               !mmc_card_is_removable(mmc))
+                       mmc->caps |= MMC_CAP_NEEDS_POLL;
 
        mmc->ocr_avail = 0;
        if (caps & SDHCI_CAN_VDD_330)
--

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

author
#8 lore raw
In reply to Matt Fleming
Quoted 23 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1835

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -2
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)

!mmc_card_is_removable(mmc) is right?
i think that is mmc_card_is_removable..
because when card is removable, host controller needs polling.

+ if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+ *mmc_card_is_removable(mmc))*
+ mmc->caps |= MMC_CAP_NEEDS_POLL;

#9 lore raw
Quoted 39 lines · Jaehoon Chung
Quoted 23 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1835

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -2
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)

!mmc_card_is_removable(mmc) is right?
i think that is mmc_card_is_removable..
because when card is removable, host controller needs polling.

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1832

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		*mmc_card_is_removable(mmc))*
+			mmc->caps |= MMC_CAP_NEEDS_POLL;

Sorry, I messed up the logic here. You're completely correct.

With this change and patch 2/2 does your controller function correctly?

author
#10 lore raw
Quoted 46 lines · Matt Fleming
Quoted 39 lines · Jaehoon Chung
Quoted 23 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1835

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -2
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)

!mmc_card_is_removable(mmc) is right?
i think that is mmc_card_is_removable..
because when card is removable, host controller needs polling.

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1832

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		*mmc_card_is_removable(mmc))*
+			mmc->caps |= MMC_CAP_NEEDS_POLL;

Sorry, I messed up the logic here. You're completely correct.

Quoted 3 lines

With this change and patch 2/2 does your controller function correctly?

yes. i changed and applied this patch, then i tested in my controller. it's working well.

Thanks
Jaehoon Chung

#11 lore raw
Quoted 47 lines · Jaehoon Chung
Quoted 46 lines · Matt Fleming
Quoted 39 lines · Jaehoon Chung
Quoted 23 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1835

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -2
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)

!mmc_card_is_removable(mmc) is right?
i think that is mmc_card_is_removable..
because when card is removable, host controller needs polling.

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1832

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		*mmc_card_is_removable(mmc))*
+			mmc->caps |= MMC_CAP_NEEDS_POLL;

Sorry, I messed up the logic here. You're completely correct.

Quoted 5 lines
Quoted 3 lines

With this change and patch 2/2 does your controller function correctly?

yes. i changed and applied this patch, then i tested in my controller. it's working well.

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

#12 lore raw
Quoted 48 lines · Matt Fleming
Quoted 47 lines · Jaehoon Chung
Quoted 46 lines · Matt Fleming
Quoted 39 lines · Jaehoon Chung
Quoted 23 lines · Matt Fleming

From: Jaehoon Chung <[email protected]>

If controller use SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk, controller need always polling detect

In this case, always generated interrupt.Because controller checked card status. I think that is not efficiently.

But if card is nonremovable, we need not always polling. So i added the check-point which is nonremovable or not

Signed-off-by Jaehoon Chung <[email protected]>
---
 drivers/mmc/host/sdhci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index f6be963..3111859 100644
@@ -1827,8 +1827,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1835

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -2
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		!mmc_card_is_removable(mmc))
+			mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)

!mmc_card_is_removable(mmc) is right?
i think that is mmc_card_is_removable..
because when card is removable, host controller needs polling.

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 1832

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+		*mmc_card_is_removable(mmc))*
+			mmc->caps |= MMC_CAP_NEEDS_POLL;

Sorry, I messed up the logic here. You're completely correct.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

With this change and patch 2/2 does your controller function correctly?

yes. i changed and applied this patch, then i tested in my controller. it's working well.

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Argh, sorry! I hit send before changing the "To:" field.

Acked-by Kyungmin Park
#13 lore raw
In reply to Matt Fleming

Hi Matt,

Quoted 2 lines · Matt Fleming

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Thanks, that's fine, I've applied both patches to mmc-next:

http://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commit;h=c1e1b0d22967e9ddd02c3099e894c888798c56ea
http://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commit;h=5c0e8a196827961f7256879e5e45d34ad5d61430

I modified the changelog and indentation for this one, resulting in the patch below; please check it over and let me know if anything's wrong:

From: Jaehoon Chung <[email protected]>
Date: Wed, 15 Sep 2010 15:30:20 -0400
Subject: [PATCH 2/2] mmc: sdhci: disable MMC_CAP_NEEDS_POLL in nonremovable case

When a controller requires SDHCI_QUIRK_BROKEN_CARD_DETECTION, we poll for card insertion/removal, and that creates interrupts. There's no need to be doing this if we have a non-removable card.

This patch requires cards to be removable before we're willing to set MMC_CAP_NEEDS_POLL.

Signed-off-by Jaehoon Chung <[email protected]>
Acked-by Kyungmin Park <[email protected]>
Cc Matt Fleming <[email protected]>
[cjb: modified changelog and code indentation]
Signed-off-by Chris Ball <[email protected]>
---
 drivers/mmc/host/sdhci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/mmc/host/sdhci.c +2 -1
index ac8b12b..fb6b170 100644
@@ -1846,7 +1846,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+	    mmc_card_is_removable(mmc))
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	mmc->ocr_avail = 0;
Signature · 5 lines
1.7.2.2

-- 
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#14 lore raw

Hi Matt,

Quoted 5 lines · Chris Ball
Quoted 2 lines · Matt Fleming

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Thanks, that's fine, I've applied both patches to mmc-next:

This (patch 2/2) doesn't resolve symbols properly, so I've dropped it:

Building modules, stage 2.
MODPOST 448 modules
ERROR: "mmc_assume_removable" [drivers/mmc/host/sdhci.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

There's no EXPORT_SYMBOL(mmc_assume_removable) in core/core.c, yet it's being referenced in host/sdhci.c.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#15 lore raw
Quoted 11 lines · Chris Ball

Hi Matt,

Quoted 5 lines · Chris Ball
Quoted 2 lines · Matt Fleming

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Thanks, that's fine, I've applied both patches to mmc-next:

This (patch 2/2) doesn't resolve symbols properly, so I've dropped it:

Quoted 8 lines

Building modules, stage 2.
MODPOST 448 modules
ERROR: "mmc_assume_removable" [drivers/mmc/host/sdhci.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

There's no EXPORT_SYMBOL(mmc_assume_removable) in core/core.c, yet it's being referenced in host/sdhci.c.

Bah! That'll teach me for not compiling an allmodconfig kernel. I'll fix this up and resubmit.

#16 lore raw
Quoted 21 lines · Matt Fleming
Quoted 11 lines · Chris Ball

Hi Matt,

Quoted 5 lines · Chris Ball
Quoted 2 lines · Matt Fleming

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Thanks, that's fine, I've applied both patches to mmc-next:

This (patch 2/2) doesn't resolve symbols properly, so I've dropped it:

Quoted 8 lines

  Building modules, stage 2.
  MODPOST 448 modules
ERROR: "mmc_assume_removable" [drivers/mmc/host/sdhci.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

There's no EXPORT_SYMBOL(mmc_assume_removable) in core/core.c, yet it's being referenced in host/sdhci.c.

Quoted 7 lines

Bah! That'll teach me for not compiling an allmodconfig kernel. I'll fix this up and resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Any new patch of this, then we could re-use SDHCI_QUIRK_BROKEN_CARD_DETECTION :)

#17 lore raw
Quoted 30 lines · zhangfei gao
Quoted 21 lines · Matt Fleming
Quoted 11 lines · Chris Ball

Hi Matt,

Quoted 5 lines · Chris Ball
Quoted 2 lines · Matt Fleming

Chris, are you OK to pick this up (including Jaehoon's change)? Or would you prefer me to resubmit?

Thanks, that's fine, I've applied both patches to mmc-next:

This (patch 2/2) doesn't resolve symbols properly, so I've dropped it:

Quoted 8 lines

  Building modules, stage 2.
  MODPOST 448 modules
ERROR: "mmc_assume_removable" [drivers/mmc/host/sdhci.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

There's no EXPORT_SYMBOL(mmc_assume_removable) in core/core.c, yet it's being referenced in host/sdhci.c.

Quoted 7 lines

Bah! That'll teach me for not compiling an allmodconfig kernel. I'll fix this up and resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line

Any new patch of this, then we could re-use SDHCI_QUIRK_BROKEN_CARD_DETECTION :)

Thanks a lot for the reminder! I've just re-sent these two patches.

#18 lore raw

Re: [PATCH 1/2] mmc: Add helper function to check if a card is removable

In reply to Matt Fleming

On Sat, 2010-08-28 at 14:53 +0100, Matt Fleming wrote:
[...]

include/linux/mmc/host.h static inline void mmc_set_disable_delay(struct mmc_host *host, · line 275

Quoted 7 lines
include/linux/mmc/host.h +1 -0
@@ -272,5 +272,12 @@ static inline void mmc_set_disable_delay(struct mmc_host *host,
 	host->disable_delay = disable_delay;
 }
 
+extern int mmc_assume_removable;

This variable is already declared in drivers/mmc/core/core.h. Either remove the declaration from there or define the following function there instead of here.

include/linux/mmc/host.h static inline void mmc_set_disable_delay(struct mmc_host *host, · line 281

Quoted 5 lines
include/linux/mmc/host.h +5 -0
+static inline int mmc_card_is_removable(struct mmc_host *host)
+{
+	return (!(host->caps & MMC_CAP_NONREMOVABLE) || mmc_assume_removable);
+}
+

That '||' should be an '&&'.

Ben.

Signature · 2 lines
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
#19 lore raw

Re: [PATCH 1/2] mmc: Add helper function to check if a card is removable

Quoted 8 lines · Ben Hutchings

On Sat, 2010-08-28 at 14:53 +0100, Matt Fleming wrote:
[...]

Quoted 6 lines
include/linux/mmc/host.h +0 -0
@@ -272,5 +272,12 @@ static inline void mmc_set_disable_delay(struct mmc_host *host,
 	host->disable_delay = disable_delay;
 }
Quoted 5 lines

include/linux/mmc/host.h static inline void mmc_set_disable_delay(struct mmc_host *host, · line 275

Quoted 1 line
include/linux/mmc/host.h +1 -0
+extern int mmc_assume_removable;

This variable is already declared in drivers/mmc/core/core.h. Either remove the declaration from there or define the following function there instead of here.

Sure, I'll remove the declaration from drivers/mmc/core/core.h.

Quoted 7 lines

include/linux/mmc/host.h static inline void mmc_set_disable_delay(struct mmc_host *host, · line 281

Quoted 5 lines
include/linux/mmc/host.h +5 -0
+static inline int mmc_card_is_removable(struct mmc_host *host)
+{
+	return (!(host->caps & MMC_CAP_NONREMOVABLE) || mmc_assume_removable);
+}
+

That '||' should be an '&&'.

Oops, yes, it should be.

Thanks for the review.

#20 lore raw

PATCH v2 1/1 mmc: Add helper function to check if a card is removable

In reply to Matt Fleming

There are two checks that need to be made when determining whether a card is removable. A host controller may set MMC_CAP_NONREMOVABLE if the controller does not support removing cards (e.g. eMMC), in which case the card is physically non-removable. Also the 'mmc_assume_removable' module parameter can be configured at module load time, in which case the card may be logically non-removable.

A helper function keeps the logic in one place so that code always checks both conditions.

Signed-off-by Matt Fleming <[email protected]>

---

Guys, it'd be good if someone could give me a Tested-by: for this patch just to make sure that it works as intended. Jaehoon, Kyungmin, does this patch work for you?

 drivers/mmc/core/core.h  |    1 -
 drivers/mmc/core/mmc.c   |    2 +-
 drivers/mmc/core/sd.c    |    2 +-
 include/linux/mmc/host.h |    8 ++++++++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/mmc/core/core.h +0 -1
index 9d9eef5..a2ca770 100644
@@ -58,7 +58,6 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
 
 /* Module parameters */
 extern int use_spi_crc;
-extern int mmc_assume_removable;
 
 /* Debugfs information for hosts and cards */
 void mmc_add_host_debugfs(struct mmc_host *host);
drivers/mmc/core/mmc.c +1 -1
index dcfc921..66c4a59 100644
@@ -718,7 +718,7 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_ops_unsafe;
 	else
 		bus_ops = &mmc_ops;
drivers/mmc/core/sd.c +1 -1
index 0f52410..bc745e1 100644
@@ -750,7 +750,7 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_sd_ops_unsafe;
 	else
 		bus_ops = &mmc_sd_ops;
include/linux/mmc/host.h +8 -0
index 09dbb90..c920cfc 100644
@@ -272,5 +272,13 @@ static inline void mmc_set_disable_delay(struct mmc_host *host,
 	host->disable_delay = disable_delay;
 }
 
+/* Module parameter */
+extern int mmc_assume_removable;
+
+static inline int mmc_card_is_removable(struct mmc_host *host)
+{
+	return (!(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable);
+}
+
 #endif
Signature · 1 line
1.7.1
author
#21 lore raw

Re: [PATCH v2] mmc: Add helper function to check if a card is removable

Quoted 70 lines · Matt Fleming

There are two checks that need to be made when determining whether a card is removable. A host controller may set MMC_CAP_NONREMOVABLE if the controller does not support removing cards (e.g. eMMC), in which case the card is physically non-removable. Also the 'mmc_assume_removable' module parameter can be configured at module load time, in which case the card may be logically non-removable.

A helper function keeps the logic in one place so that code always checks both conditions.

Signed-off-by Matt Fleming <[email protected]>

---

Guys, it'd be good if someone could give me a Tested-by: for this patch just to make sure that it works as intended. Jaehoon, Kyungmin, does this patch work for you?

 drivers/mmc/core/core.h  |    1 -
 drivers/mmc/core/mmc.c   |    2 +-
 drivers/mmc/core/sd.c    |    2 +-
 include/linux/mmc/host.h |    8 ++++++++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/mmc/core/core.h +0 -1
index 9d9eef5..a2ca770 100644
@@ -58,7 +58,6 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
 
 /* Module parameters */
 extern int use_spi_crc;
-extern int mmc_assume_removable;
 
 /* Debugfs information for hosts and cards */
 void mmc_add_host_debugfs(struct mmc_host *host);
drivers/mmc/core/mmc.c +1 -1
index dcfc921..66c4a59 100644
@@ -718,7 +718,7 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_ops_unsafe;
 	else
 		bus_ops = &mmc_ops;
drivers/mmc/core/sd.c +1 -1
index 0f52410..bc745e1 100644
@@ -750,7 +750,7 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 {
 	const struct mmc_bus_ops *bus_ops;
 
-	if (host->caps & MMC_CAP_NONREMOVABLE || !mmc_assume_removable)
+	if (!mmc_card_is_removable(host))
 		bus_ops = &mmc_sd_ops_unsafe;
 	else
 		bus_ops = &mmc_sd_ops;
include/linux/mmc/host.h +0 -0
index 09dbb90..c920cfc 100644
@@ -272,5 +272,13 @@ static inline void mmc_set_disable_delay(struct mmc_host *host,
 	host->disable_delay = disable_delay;
 }

include/linux/mmc/host.h static inline void mmc_set_disable_delay(struct mmc_host *host, · line 284

Quoted 10 lines
include/linux/mmc/host.h +8 -0
+/* Module parameter */
+extern int mmc_assume_removable;
+
+static inline int mmc_card_is_removable(struct mmc_host *host)
+{
+	return (!(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable);
+}
+
 #endif

It's working well.

Tested-by Jaehoon Chung <[email protected]>