[RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Michał Mirosław]

This is needed to avoid code duplication in SD-combo support.
---
 drivers/mmc/core/sd.c |  256 ++++++++++++++++++++++++++++--------------------
 drivers/mmc/core/sd.h |   17 ++++
 2 files changed, 166 insertions(+), 107 deletions(-)
 create mode 100644 drivers/mmc/core/sd.h

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5eac21d..75fad9a 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -59,7 +59,7 @@ static const unsigned int tacc_mant[] = {
 /*
  * Given the decoded CSD structure, decode the raw CID to our CID structure.
  */
-static void mmc_decode_cid(struct mmc_card *card)
+void mmc_decode_cid(struct mmc_card *card)
 {
 	u32 *resp = card->raw_cid;
 
@@ -238,22 +238,22 @@ out:
 /*
  * Test if the card supports high-speed mode and, if so, switch to it.
  */
-static int mmc_switch_hs(struct mmc_card *card)
+int mmc_sd_switch_hs(struct mmc_card *card)
 {
 	int err;
 	u8 *status;
 
 	if (card->scr.sda_vsn < SCR_SPEC_VER_1)
-		return 0;
+		return -EOPNOTSUPP;
 
 	if (!(card->csd.cmdclass & CCC_SWITCH))
-		return 0;
+		return -EOPNOTSUPP;
 
 	if (!(card->host->caps & MMC_CAP_SD_HIGHSPEED))
-		return 0;
+		return -EOPNOTSUPP;
 
 	if (card->sw_caps.hs_max_dtr == 0)
-		return 0;
+		return -EOPNOTSUPP;
 
 	err = -EIO;
 
@@ -272,9 +272,7 @@ static int mmc_switch_hs(struct mmc_card *card)
 		printk(KERN_WARNING "%s: Problem switching card "
 			"into high-speed mode!\n",
 			mmc_hostname(card->host));
-	} else {
-		mmc_card_set_highspeed(card);
-		mmc_set_timing(card->host, MMC_TIMING_SD_HS);
+		err = -EOPNOTSUPP;
 	}
 
 out:
@@ -320,26 +318,16 @@ static const struct attribute_group *sd_attr_groups[] = {
 	NULL,
 };
 
-static struct device_type sd_type = {
+struct device_type sd_type = {
 	.groups = sd_attr_groups,
 };
 
 /*
- * Handle the detection and initialisation of a card.
- *
- * In the case of a resume, "oldcard" will contain the card
- * we're trying to reinitialise.
+ * Fetch CID from card.
  */
-static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
-	struct mmc_card *oldcard)
+int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
 {
-	struct mmc_card *card;
 	int err;
-	u32 cid[4];
-	unsigned int max_dtr;
-
-	BUG_ON(!host);
-	WARN_ON(!host->claimed);
 
 	/*
 	 * Since we're changing the OCR value, we seem to
@@ -361,23 +349,137 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 
 	err = mmc_send_app_op_cond(host, ocr, NULL);
 	if (err)
-		goto err;
+		return err;
 
-	/*
-	 * Fetch CID from card.
-	 */
 	if (mmc_host_is_spi(host))
 		err = mmc_send_cid(host, cid);
 	else
 		err = mmc_all_send_cid(host, cid);
+	
+	return err;
+}
+
+int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card)
+{
+	int err;
+
+	/*
+	 * Fetch CSD from card.
+	 */
+	err = mmc_send_csd(card, card->raw_csd);
 	if (err)
-		goto err;
+		return err;
 
-	if (oldcard) {
-		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0) {
-			err = -ENOENT;
-			goto err;
+	err = mmc_decode_csd(card);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
+	bool reinit)
+{
+	int err;
+
+	if (!reinit) {
+		/*
+		 * Fetch SCR from card.
+		 */
+		err = mmc_app_send_scr(card, card->raw_scr);
+		if (err)
+			return err;
+
+		err = mmc_decode_scr(card);
+		if (err)
+			return err;
+
+		/*
+		 * Fetch switch information from card.
+		 */
+		err = mmc_read_switch(card);
+		if (err)
+			return err;
+	}
+
+	/*
+	 * For SPI, enable CRC as appropriate.
+	 * This CRC enable is located AFTER the reading of the
+	 * card registers because some SDHC cards are not able
+	 * to provide valid CRCs for non-512-byte blocks.
+	 */
+	if (mmc_host_is_spi(host)) {
+		err = mmc_spi_set_crc(host, use_spi_crc);
+		if (err)
+			return err;
+	}
+
+	/*
+	 * Check if read-only switch is active.
+	 */
+	if (!reinit) {
+		int ro = -1;
+
+		if (host->ops->get_ro)
+			ro = host->ops->get_ro(host);
+
+		if (ro < 0) {
+			printk(KERN_WARNING "%s: host does not "
+				"support reading read-only "
+				"switch. assuming write-enable.\n",
+				mmc_hostname(host));
+		} else if (ro > 0) {
+			mmc_card_set_readonly(card);
 		}
+	}
+
+	return 0;
+}
+
+unsigned mmc_sd_get_max_clock(struct mmc_card *card)
+{
+	unsigned max_dtr = (unsigned int)-1;
+
+	if (mmc_card_highspeed(card)) {
+		if (max_dtr > card->sw_caps.hs_max_dtr)
+			max_dtr = card->sw_caps.hs_max_dtr;
+	} else if (max_dtr > card->csd.max_dtr) {
+		max_dtr = card->csd.max_dtr;
+	}
+
+	return max_dtr;
+}
+
+void mmc_sd_go_highspeed(struct mmc_card *card)
+{
+	mmc_card_set_highspeed(card);
+	mmc_set_timing(card->host, MMC_TIMING_SD_HS);
+}
+
+/*
+ * Handle the detection and initialisation of a card.
+ *
+ * In the case of a resume, "oldcard" will contain the card
+ * we're trying to reinitialise.
+ */
+static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
+	struct mmc_card *oldcard)
+{
+	struct mmc_card *card;
+	int err;
+	u32 cid[4];
+	unsigned int max_dtr;
+
+	BUG_ON(!host);
+	WARN_ON(!host->claimed);
+
+	err = mmc_sd_get_cid(host, ocr, cid);
+	if (err)
+		return err;
+
+	if (oldcard) {
+		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
+			return -ENOENT;
 
 		card = oldcard;
 	} else {
@@ -385,10 +487,8 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		 * Allocate card structure.
 		 */
 		card = mmc_alloc_card(host, &sd_type);
-		if (IS_ERR(card)) {
-			err = PTR_ERR(card);
-			goto err;
-		}
+		if (IS_ERR(card))
+			return PTR_ERR(card);
 
 		card->type = MMC_TYPE_SD;
 		memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
@@ -400,22 +500,15 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	if (!mmc_host_is_spi(host)) {
 		err = mmc_send_relative_addr(host, &card->rca);
 		if (err)
-			goto free_card;
+			return err;
 
 		mmc_set_bus_mode(host, MMC_BUSMODE_PUSHPULL);
 	}
 
 	if (!oldcard) {
-		/*
-		 * Fetch CSD from card.
-		 */
-		err = mmc_send_csd(card, card->raw_csd);
+		err = mmc_sd_get_csd(host, card);
 		if (err)
-			goto free_card;
-
-		err = mmc_decode_csd(card);
-		if (err)
-			goto free_card;
+			return err;
 
 		mmc_decode_cid(card);
 	}
@@ -426,60 +519,27 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	if (!mmc_host_is_spi(host)) {
 		err = mmc_select_card(card);
 		if (err)
-			goto free_card;
+			return err;
 	}
 
-	if (!oldcard) {
-		/*
-		 * Fetch SCR from card.
-		 */
-		err = mmc_app_send_scr(card, card->raw_scr);
-		if (err)
-			goto free_card;
-
-		err = mmc_decode_scr(card);
-		if (err < 0)
-			goto free_card;
-
-		/*
-		 * Fetch switch information from card.
-		 */
-		err = mmc_read_switch(card);
-		if (err)
-			goto free_card;
-	}
+	err = mmc_sd_setup_card(host, card, oldcard != NULL);
+	if (err)
+		goto free_card;
 
 	/*
-	 * For SPI, enable CRC as appropriate.
-	 * This CRC enable is located AFTER the reading of the
-	 * card registers because some SDHC cards are not able
-	 * to provide valid CRCs for non-512-byte blocks.
+	 * Attempt to change to high-speed (if supported)
 	 */
-	if (mmc_host_is_spi(host)) {
-		err = mmc_spi_set_crc(host, use_spi_crc);
-		if (err)
+	err = mmc_sd_switch_hs(card);
+	if (err) {
+		if (err != -EOPNOTSUPP)
 			goto free_card;
+		mmc_sd_go_highspeed(card);
 	}
 
 	/*
-	 * Attempt to change to high-speed (if supported)
-	 */
-	err = mmc_switch_hs(card);
-	if (err)
-		goto free_card;
-
-	/*
 	 * Compute bus speed.
 	 */
-	max_dtr = (unsigned int)-1;
-
-	if (mmc_card_highspeed(card)) {
-		if (max_dtr > card->sw_caps.hs_max_dtr)
-			max_dtr = card->sw_caps.hs_max_dtr;
-	} else if (max_dtr > card->csd.max_dtr) {
-		max_dtr = card->csd.max_dtr;
-	}
-
+	max_dtr = mmc_sd_get_max_clock(card);
 	mmc_set_clock(host, max_dtr);
 
 	/*
@@ -494,30 +554,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
 	}
 
-	/*
-	 * Check if read-only switch is active.
-	 */
-	if (!oldcard) {
-		if (!host->ops->get_ro || host->ops->get_ro(host) < 0) {
-			printk(KERN_WARNING "%s: host does not "
-				"support reading read-only "
-				"switch. assuming write-enable.\n",
-				mmc_hostname(host));
-		} else {
-			if (host->ops->get_ro(host) > 0)
-				mmc_card_set_readonly(card);
-		}
-	}
-
-	if (!oldcard)
-		host->card = card;
-
+	host->card = card;
 	return 0;
 
 free_card:
 	if (!oldcard)
 		mmc_remove_card(card);
-err:
 
 	return err;
 }
diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
new file mode 100644
index 0000000..3d8800f
--- /dev/null
+++ b/drivers/mmc/core/sd.h
@@ -0,0 +1,17 @@
+#ifndef _MMC_CORE_SD_H
+#define _MMC_CORE_SD_H
+
+#include <linux/mmc/card.h>
+
+extern struct device_type sd_type;
+
+int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid);
+int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
+void mmc_decode_cid(struct mmc_card *card);
+int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
+	bool reinit);
+unsigned mmc_sd_get_max_clock(struct mmc_card *card);
+int mmc_sd_switch_hs(struct mmc_card *card);
+void mmc_sd_go_highspeed(struct mmc_card *card);
+
+#endif
-- 
1.6.4.4

[RFC] [PATCH 2/2] mmc: implement SD-combo (IO+mem) support

[Michał Mirosław]

---
 drivers/mmc/core/bus.c   |    9 +++
 drivers/mmc/core/core.c  |   12 +++-
 drivers/mmc/core/sdio.c  |  178 ++++++++++++++++++++++++++++++++++++---------
 include/linux/mmc/card.h |    1 +
 4 files changed, 162 insertions(+), 38 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 49d9dca..7cd9749 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -37,6 +37,8 @@ static ssize_t mmc_type_show(struct device *dev,
 		return sprintf(buf, "SD\n");
 	case MMC_TYPE_SDIO:
 		return sprintf(buf, "SDIO\n");
+	case MMC_TYPE_SD_COMBO:
+		return sprintf(buf, "SDcombo\n");
 	default:
 		return -EFAULT;
 	}
@@ -74,6 +76,9 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 	case MMC_TYPE_SDIO:
 		type = "SDIO";
 		break;
+	case MMC_TYPE_SD_COMBO:
+		type = "SDcombo";
+		break;
 	default:
 		type = NULL;
 	}
@@ -239,6 +244,10 @@ int mmc_add_card(struct mmc_card *card)
 	case MMC_TYPE_SDIO:
 		type = "SDIO";
 		break;
+	case MMC_TYPE_SD_COMBO:
+		type = "SD-combo";
+		if (mmc_card_blockaddr(card))
+			type = "SDHC-combo";
 	default:
 		type = "?";
 		break;
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3168ebd..87cf0de 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1099,8 +1099,15 @@ void mmc_rescan(struct work_struct *work)
 	 */
 	err = mmc_send_io_op_cond(host, 0, &ocr);
 	if (!err) {
-		if (mmc_attach_sdio(host, ocr))
-			mmc_power_off(host);
+		if (mmc_attach_sdio(host, ocr)) {
+			mmc_claim_host(host);
+			/* try SDMEM (but not MMC) even if SDIO is broken */
+			if (mmc_send_app_op_cond(host, 0, &ocr))
+				goto out_fail;
+
+			if (mmc_attach_sd(host, ocr))
+				mmc_power_off(host);
+		}
 		goto out;
 	}
 
@@ -1124,6 +1131,7 @@ void mmc_rescan(struct work_struct *work)
 		goto out;
 	}
 
+out_fail:
 	mmc_release_host(host);
 	mmc_power_off(host);
 
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2dd4cfe..aebb0b0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -18,6 +18,7 @@
 
 #include "core.h"
 #include "bus.h"
+#include "sd.h"
 #include "sdio_bus.h"
 #include "mmc_ops.h"
 #include "sd_ops.h"
@@ -144,10 +145,10 @@ static int sdio_enable_wide(struct mmc_card *card)
 	u8 ctrl;
 
 	if (!(card->host->caps & MMC_CAP_4_BIT_DATA))
-		return 0;
+		return -EOPNOTSUPP;
 
 	if (card->cccr.low_speed && !card->cccr.wide_bus)
-		return 0;
+		return -EOPNOTSUPP;
 
 	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_IF, 0, &ctrl);
 	if (ret)
@@ -159,8 +160,6 @@ static int sdio_enable_wide(struct mmc_card *card)
 	if (ret)
 		return ret;
 
-	mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
-
 	return 0;
 }
 
@@ -221,36 +220,98 @@ static int sdio_disable_wide(struct mmc_card *card)
 	return 0;
 }
 
+
+static int sdio_enable_4bit_bus(struct mmc_card *card)
+{
+	int err;
+
+	if (card->type == MMC_TYPE_SD_COMBO) {
+		if ((card->host->caps & MMC_CAP_4_BIT_DATA) &&
+			(card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) {
+			err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_4);
+			if (err)
+				return err;
+		} else
+			return -EOPNOTSUPP;
+	}
+
+	err = sdio_enable_wide(card);
+	if (err == -EOPNOTSUPP && card->type == MMC_TYPE_SD_COMBO)
+		mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1);
+
+	return err;
+}
+
+
 /*
  * Test if the card supports high-speed mode and, if so, switch to it.
  */
-static int sdio_enable_hs(struct mmc_card *card)
+static int mmc_sdio_switch_hs(struct mmc_card *card, int enable)
 {
 	int ret;
 	u8 speed;
 
 	if (!(card->host->caps & MMC_CAP_SD_HIGHSPEED))
-		return 0;
+		return -EOPNOTSUPP;
 
 	if (!card->cccr.high_speed)
-		return 0;
+		return -EOPNOTSUPP;
 
 	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_SPEED, 0, &speed);
 	if (ret)
 		return ret;
 
-	speed |= SDIO_SPEED_EHS;
+	if (enable)
+		speed |= SDIO_SPEED_EHS;
+	else
+		speed &= ~SDIO_SPEED_EHS;
 
 	ret = mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_SPEED, speed, NULL);
 	if (ret)
 		return ret;
 
-	mmc_card_set_highspeed(card);
-	mmc_set_timing(card->host, MMC_TIMING_SD_HS);
-
 	return 0;
 }
 
+static int sdio_enable_hs(struct mmc_card *card)
+{
+	int err;
+
+	err = mmc_sdio_switch_hs(card, true);
+	if (err)
+		return err;
+
+	if (card->type == MMC_TYPE_SD_COMBO) {
+		err = mmc_sd_switch_hs(card);
+		if (err == -EOPNOTSUPP)
+			mmc_sdio_switch_hs(card, false);
+	}
+
+	return err;
+}
+
+static unsigned mmc_sdio_get_max_clock(struct mmc_card *card)
+{
+	unsigned max_dtr;
+
+	if (mmc_card_highspeed(card)) {
+		/*
+		 * The SDIO specification doesn't mention how
+		 * the CIS transfer speed register relates to
+		 * high-speed, but it seems that 50 MHz is
+		 * mandatory.
+		 */
+		max_dtr = 50000000;
+	} else {
+		max_dtr = card->cis.max_dtr;
+	}
+
+	if (card->type == MMC_TYPE_SD_COMBO)
+		max_dtr = min(max_dtr, mmc_sd_get_max_clock(card));
+
+	return max_dtr;
+}
+
 /*
  * Handle the detection and initialisation of a card.
  *
@@ -295,6 +356,34 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 
 	card->type = MMC_TYPE_SDIO;
 
+	if (!oldcard || oldcard->type == MMC_TYPE_SDIO) {
+		u32 cid[4];
+
+		err = mmc_sd_get_cid(host, host->ocr & ocr, cid);
+		if (!err) {
+			/* this is SD-combo card */
+			if (oldcard && oldcard->type == MMC_TYPE_SDIO) {
+				err = -ENOENT;
+				goto remove;
+			}
+			card->type = MMC_TYPE_SD_COMBO;
+			memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
+		}
+	} else if (oldcard->type == MMC_TYPE_SD_COMBO) {
+		u32 cid[4];
+
+		err = mmc_sd_get_cid(host, host->ocr & ocr, cid);
+		if (err) {
+			/* this is SDIO-only card */
+			goto remove;
+		}
+
+		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0) {
+			err = -ENOENT;
+			goto remove;
+		}
+	}
+
 	/*
 	 * For native busses:  set card RCA and quit open drain mode.
 	 */
@@ -307,6 +396,17 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	}
 
 	/*
+	 * Read CSD, before selecting the card
+	 */
+	if (!oldcard && card->type == MMC_TYPE_SD_COMBO) {
+		err = mmc_sd_get_csd(host, card);
+		if (err)
+			return err;
+
+		mmc_decode_cid(card);
+	}
+
+	/*
 	 * Select card, as all following commands rely on that.
 	 */
 	if (!powered_resume && !mmc_host_is_spi(host)) {
@@ -333,41 +433,54 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		int same = (card->cis.vendor == oldcard->cis.vendor &&
 			    card->cis.device == oldcard->cis.device);
 		mmc_remove_card(card);
-		if (!same) {
-			err = -ENOENT;
-			goto err;
-		}
+		if (!same)
+			return -ENOENT;
+
 		card = oldcard;
 		return 0;
 	}
 
+	if (card->type == MMC_TYPE_SD_COMBO) {
+		err = mmc_sd_setup_card(host, card, oldcard != NULL);
+		/* handle as SDIO-only card if memory init failed */
+		if (err) {
+			mmc_go_idle(host);
+			if (mmc_host_is_spi(host))
+				/* should not fail, as it worked previously */
+				mmc_spi_set_crc(host, use_spi_crc);
+			card->type = MMC_TYPE_SDIO;
+		} else
+			card->dev.type = &sd_type;
+	}
+
+	/*
+	 * If needed, disconnect card detection pull-up resistor.
+	 */
+	err = sdio_disable_cd(card);
+	if (err)
+		goto remove;
+
 	/*
 	 * Switch to high-speed (if supported).
 	 */
 	err = sdio_enable_hs(card);
-	if (err)
+	if (!err)
+		mmc_sd_go_highspeed(card);
+	else if (err != -EOPNOTSUPP)
 		goto remove;
 
 	/*
 	 * Change to the card's maximum speed.
 	 */
-	if (mmc_card_highspeed(card)) {
-		/*
-		 * The SDIO specification doesn't mention how
-		 * the CIS transfer speed register relates to
-		 * high-speed, but it seems that 50 MHz is
-		 * mandatory.
-		 */
-		mmc_set_clock(host, 50000000);
-	} else {
-		mmc_set_clock(host, card->cis.max_dtr);
-	}
+	mmc_set_clock(host, mmc_sdio_get_max_clock(card));
 
 	/*
 	 * Switch to wider bus (if supported).
 	 */
-	err = sdio_enable_wide(card);
-	if (err)
+	err = sdio_enable_4bit_bus(card);
+	if (!err)
+		mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
+	else if (err != -EOPNOTSUPP)
 		goto remove;
 
 	if (!oldcard)
@@ -568,13 +681,6 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	card->sdio_funcs = 0;
 
 	/*
-	 * If needed, disconnect card detection pull-up resistor.
-	 */
-	err = sdio_disable_cd(card);
-	if (err)
-		goto remove;
-
-	/*
 	 * Initialize (but don't add) all present functions.
 	 */
 	for (i = 0; i < funcs; i++, card->sdio_funcs++) {
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index d02d2c6..dc570f5 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -92,6 +92,7 @@ struct mmc_card {
 #define MMC_TYPE_MMC		0		/* MMC card */
 #define MMC_TYPE_SD		1		/* SD card */
 #define MMC_TYPE_SDIO		2		/* SDIO card */
+#define MMC_TYPE_SD_COMBO	3		/* SD combo (IO+mem) card */
 	unsigned int		state;		/* (our) card state */
 #define MMC_STATE_PRESENT	(1<<0)		/* present in sysfs */
 #define MMC_STATE_READONLY	(1<<1)		/* card is read-only */
-- 
1.6.4.4

Re: [RFC] [PATCH 2/2] mmc: implement SD-combo (IO+mem) support

[Andrew Morton]

On Tue, 25 May 2010 18:10:28 +0200 (CEST)
Micha__ Miros__aw <mirq-linux@rere.qmqm.pl> wrote:

> ---

There's really nothing at all to be said?  I don't even know what
"SD-combo" _is_.  Help us out here!

Why do we need this patch?  What happens if we don't have it?  Where's
the value to our users?  etcetera.


>
> ...
>
> @@ -144,10 +145,10 @@ static int sdio_enable_wide(struct mmc_card *card)
>  	u8 ctrl;
>  
>  	if (!(card->host->caps & MMC_CAP_4_BIT_DATA))
> -		return 0;
> +		return -EOPNOTSUPP;

grr.

"The requested operation is not supported on this type of object. 
 This usually occurs if you are trying to accept(2) a non SOCK_STREAM
 socket."

So if this EOPNOTSUPP gets propagated back to userspace, our poor user
will be looking at his networking code wondering what he did wrong!

Please, just because an error code sounds appropriate, that doesn't
mean that it was the correct thing to use.  ENXIO might be appropriate,
but when in doubt, plain old EIO is good.

> +static int sdio_enable_hs(struct mmc_card *card)
> +{
> +	int err;
> +
> +	err = mmc_sdio_switch_hs(card, true);
> +	if (err)
> +		return err;
> +
> +	if (card->type == MMC_TYPE_SD_COMBO) {
> +		err = mmc_sd_switch_hs(card);
> +		if (err == -EOPNOTSUPP)
> +			mmc_sdio_switch_hs(card, false);
> +	}

Now, if the EOPNOTSUPP is purely for internal use and never gets
propagated back to userspace then I _guess_ that's acceptable.  It's a
kind of internal overloading of an inappropriate identifier for MMC's
own usage.

But it would be clearer and cleaner to not borrow the Unix errnos in
this fashion.  Better to define a set of MMC-specific error codes and
put them in an MMC-specific header file.

> @@ -295,6 +356,34 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>  
>  	card->type = MMC_TYPE_SDIO;
>  
> +	if (!oldcard || oldcard->type == MMC_TYPE_SDIO) {
> +		u32 cid[4];
> +
> +		err = mmc_sd_get_cid(host, host->ocr & ocr, cid);
> +		if (!err) {
> +			/* this is SD-combo card */
> +			if (oldcard && oldcard->type == MMC_TYPE_SDIO) {
> +				err = -ENOENT;
> +				goto remove;
> +			}
> +			card->type = MMC_TYPE_SD_COMBO;
> +			memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> +		}
> +	} else if (oldcard->type == MMC_TYPE_SD_COMBO) {
> +		u32 cid[4];
> +
> +		err = mmc_sd_get_cid(host, host->ocr & ocr, cid);
> +		if (err) {
> +			/* this is SDIO-only card */
> +			goto remove;
> +		}
> +
> +		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0) {
> +			err = -ENOENT;
> +			goto remove;
> +		}
> +	}

Be aware that gcc has a habit of being inefficient with local
variables.  It will probably allocate 32 bytes of stack for the above
two cid[] arrays.  But it could have allocated just 16 bytes.

It's not a big problem in this particular case, but it should be
remembered.  Unfortunately any C-level workaround for this involves
making the source code nastier :(

Re: [RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Andrew Morton]

On Tue, 25 May 2010 18:10:28 +0200 (CEST)
Micha__ Miros__aw <mirq-linux@rere.qmqm.pl> wrote:

> This is needed to avoid code duplication in SD-combo support.

hm.  Perhaps you could have told us a bit more about it than this.

> +int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
> +int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card)
> +int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
> +unsigned mmc_sd_get_max_clock(struct mmc_card *card)
> +void mmc_sd_go_highspeed(struct mmc_card *card)

These are global symbols, but they are not exported to (other) modules.
That's odd.  I'd have expected to see them either `static' or
EXPORT_MODULE()d.

I'll take the lack of a signed-off-by: and the "RFC" as signal to not
apply these patches.

Re: [RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Michał Mirosław]

Thanks for your review!

On Fri, May 28, 2010 at 02:40:27PM -0700, Andrew Morton wrote:
> On Tue, 25 May 2010 18:10:28 +0200 (CEST)
> Micha__ Miros__aw <mirq-linux@rere.qmqm.pl> wrote:
> > This is needed to avoid code duplication in SD-combo support.
> hm.  Perhaps you could have told us a bit more about it than this.
> > +int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
> > +int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card)
> > +int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
> > +unsigned mmc_sd_get_max_clock(struct mmc_card *card)
> > +void mmc_sd_go_highspeed(struct mmc_card *card)
> These are global symbols, but they are not exported to (other) modules.
> That's odd.  I'd have expected to see them either `static' or
> EXPORT_MODULE()d.

This is all contained in mmc_core.ko and needs not be exported to other
modules. A SD-combo card is a SD (memory) and SDIO in one package - the
only changes needed are in initialization sequence, after which both
parts are mostly independent.

> I'll take the lack of a signed-off-by: and the "RFC" as signal to not
> apply these patches.

I would like for this to get tested by other people first, but it looks
like not many kernel developers use SD combo cards.

I'll resend the patches after applying your suggestions from the other
mail.

Best Regards,
Michał Mirosław

Re: [RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Adrian Hunter]

Michał Mirosław wrote:
> Thanks for your review!
> 
> On Fri, May 28, 2010 at 02:40:27PM -0700, Andrew Morton wrote:
>> On Tue, 25 May 2010 18:10:28 +0200 (CEST)
>> Micha__ Miros__aw <mirq-linux@rere.qmqm.pl> wrote:
>>> This is needed to avoid code duplication in SD-combo support.
>> hm.  Perhaps you could have told us a bit more about it than this.
>>> +int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
>>> +int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card)
>>> +int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>> +unsigned mmc_sd_get_max_clock(struct mmc_card *card)
>>> +void mmc_sd_go_highspeed(struct mmc_card *card)
>> These are global symbols, but they are not exported to (other) modules.
>> That's odd.  I'd have expected to see them either `static' or
>> EXPORT_MODULE()d.
> 
> This is all contained in mmc_core.ko and needs not be exported to other
> modules. A SD-combo card is a SD (memory) and SDIO in one package - the
> only changes needed are in initialization sequence, after which both
> parts are mostly independent.

That is a bit vague.  An SD card is a memory.  An SDIO "card" is an
I/O connection made using the same physical interface as an SD card
and a very similar protocal to SD cards (at least with respect to
reading and writing).  I do not understand how a card can be SD and
SDIO.  Are you sure you are not talking about a controller that supports
SD and SDIO?  A lot of controllers do that.

> 
>> I'll take the lack of a signed-off-by: and the "RFC" as signal to not
>> apply these patches.
> 
> I would like for this to get tested by other people first, but it looks
> like not many kernel developers use SD combo cards.
> 
> I'll resend the patches after applying your suggestions from the other
> mail.
> 
> Best Regards,
> Michał Mirosław
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Chris Ball]

Hi,

   > That is a bit vague.  An SD card is a memory.  An SDIO "card" is
   > an I/O connection made using the same physical interface as an SD
   > card and a very similar protocal to SD cards (at least with
   > respect to reading and writing).  I do not understand how a card
   > can be SD and SDIO.  Are you sure you are not talking about a
   > controller that supports SD and SDIO?  A lot of controllers do
   > that.

Adrian, I presume we're talking about something like:

http://www.arasan.com/products/sd/SD-Combo-Flyer.pdf

Michał, could you let us know which SD-combo device(s) in particular
you've been working with?

- Chris.
-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: [RFC] [PATCH 1/2] mmc: split mmc_sd_init_card()

[Michał Mirosław]

On Thu, Jun 03, 2010 at 01:21:23PM -0400, Chris Ball wrote:
>    > That is a bit vague.  An SD card is a memory.  An SDIO "card" is
>    > an I/O connection made using the same physical interface as an SD
>    > card and a very similar protocal to SD cards (at least with
>    > respect to reading and writing).  I do not understand how a card
>    > can be SD and SDIO.  Are you sure you are not talking about a
>    > controller that supports SD and SDIO?  A lot of controllers do
>    > that.
> Adrian, I presume we're talking about something like:
> http://www.arasan.com/products/sd/SD-Combo-Flyer.pdf
> Michał, could you let us know which SD-combo device(s) in particular
> you've been working with?

The card I'm using is one I'm working on (SDIO-like card interoperating
with optional SD memory card) and I can't disclose what it really is nor
when it will be available.

Spectec SDG-810 (GPS with micro-SD slot) looks like a similar SD-combo card.

Answering Adrian's question: SDIO uses separate command set (CMD5 for
init, CMD52 and CMD53 for IO transactions) than SD memory and so the
parts can coexist in one card (provided some additional constraints are
met). The SDIO specification covers the details.

Best Regards,
Michał Mirosław