[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

The card-present polling within sdhci based driver is very expensive
in terms of the impact to system performance.  We observe a few
system performance issues from Freescale and Linaro on mx5 platforms,
which have been proved card polling related.

The patch set extends the current sdhci-esdhc-imx card_detect and
write_protect support to cover mx5 platforms, and solves above
performance issues.

Shawn Guo (4):
      mmc: sdhci: fix interrupt storm from card detection
      mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
      mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
      mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

 arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c |    3 +-
 arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |    3 +-
 arch/arm/mach-imx/mach-mx25_3ds.c              |    2 +
 arch/arm/mach-imx/mach-mx35_3ds.c              |    7 +-
 arch/arm/mach-imx/mach-pcm043.c                |    2 +
 arch/arm/mach-imx/mach-vpr200.c                |    7 +-
 arch/arm/mach-mx5/board-mx51_3ds.c             |    7 +-
 arch/arm/mach-mx5/board-mx51_babbage.c         |   27 +++++-
 arch/arm/mach-mx5/board-mx51_efikamx.c         |    7 +-
 arch/arm/mach-mx5/board-mx51_efikasb.c         |    7 +-
 arch/arm/mach-mx5/board-mx53_evk.c             |   14 +++-
 arch/arm/mach-mx5/board-mx53_loco.c            |   14 +++-
 arch/arm/mach-mx5/board-mx53_smd.c             |   21 ++++-
 arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c   |   14 +++-
 arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c   |    7 +-
 arch/arm/mach-mx5/mx51_efika.c                 |    7 +-
 arch/arm/plat-mxc/include/mach/esdhc.h         |   25 ++++-
 drivers/mmc/host/sdhci-esdhc-imx.c             |  117 +++++++++++++----------
 drivers/mmc/host/sdhci.c                       |   13 ++-
 19 files changed, 225 insertions(+), 79 deletions(-)

[PATCH 1/4] mmc: sdhci: fix interrupt storm from card detection

[Shawn Guo]

The issue was initially found by Eric Benard as below.

http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031

Not sure about other SDHCI based controller, but on Freescale eSDHC,
the SDHCI_INT_CARD_INSERT bits will be immediately set again when it
gets cleared, if a card is inserted. The driver need to mask the irq
to prevent interrupt storm which will freeze the system.  And the
SDHCI_INT_CARD_REMOVE gets the same situation.

The patch fixes the problem based on the initial idea from
Eric Benard.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Eric Benard <eric@eukrea.com>
---
 drivers/mmc/host/sdhci.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 91d9892..71a2505 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2154,13 +2154,20 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		mmc_hostname(host->mmc), intmask);
 
 	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
+		u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
+			      SDHCI_CARD_PRESENT;
+
+		sdhci_mask_irqs(host, present ? SDHCI_INT_CARD_INSERT :
+						SDHCI_INT_CARD_REMOVE);
+		sdhci_unmask_irqs(host, present ? SDHCI_INT_CARD_REMOVE :
+						  SDHCI_INT_CARD_INSERT);
+
 		sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
-			SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
+			     SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
+		intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 		tasklet_schedule(&host->card_tasklet);
 	}
 
-	intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
-
 	if (intmask & SDHCI_INT_CMD_MASK) {
 		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK,
 			SDHCI_INT_STATUS);
-- 
1.7.4.1

[PATCH 2/4] mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared

[Shawn Guo]

The function esdhc_readl_le intends to clear bit SDHCI_CARD_PRESENT,
when the card detect gpio tells there is no card.  But it does not
clear the bit actually.  The patch gives a fix on that.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 248b8e5..94097c0 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -84,7 +84,7 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 		if (boarddata && gpio_is_valid(boarddata->cd_gpio)
 				&& gpio_get_value(boarddata->cd_gpio))
 			/* no card, if a valid gpio says so... */
-			val &= SDHCI_CARD_PRESENT;
+			val &= ~SDHCI_CARD_PRESENT;
 		else
 			/* ... in all other cases assume card is present */
 			val |= SDHCI_CARD_PRESENT;
-- 
1.7.4.1

[PATCH 3/4] mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP

[Shawn Guo]

The use of flag ESDHC_FLAG_GPIO_FOR_CD_WP is all CD related.  It does
not necessarily need to bother WP in the flag name.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 94097c0..79b7a9a 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -29,7 +29,7 @@
 #define SDHCI_VENDOR_SPEC		0xC0
 #define  SDHCI_VENDOR_SPEC_SDIO_QUIRK	0x00000002
 
-#define ESDHC_FLAG_GPIO_FOR_CD_WP	(1 << 0)
+#define ESDHC_FLAG_GPIO_FOR_CD		(1 << 0)
 /*
  * The CMDTYPE of the CMD register (offset 0xE) should be set to
  * "11" when the STOP CMD12 is issued on imx53 to abort one
@@ -77,7 +77,7 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 	u32 val = readl(host->ioaddr + reg);
 
 	if (unlikely((reg == SDHCI_PRESENT_STATE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD_WP))) {
+			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD))) {
 		struct esdhc_platform_data *boarddata =
 				host->mmc->parent->platform_data;
 
@@ -99,7 +99,7 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 
 	if (unlikely((reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD_WP)))
+			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD)))
 		/*
 		 * these interrupts won't work with a custom card_detect gpio
 		 * (only applied to mx25/35)
@@ -308,7 +308,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 			goto no_card_detect_irq;
 		}
 
-		imx_data->flags |= ESDHC_FLAG_GPIO_FOR_CD_WP;
+		imx_data->flags |= ESDHC_FLAG_GPIO_FOR_CD;
 		/* Now we have a working card_detect again */
 		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
 	}
-- 
1.7.4.1

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Shawn Guo]

The patch extends card_detect and write_protect support to get mx5
family and more scenarios supported.  The changes include:

 * Turn platform_data from optional to mandatory
 * Add cd_types and wp_types into platform_data to cover more use
   cases
 * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
 * Adjust machine codes to adopt the platform_data changes

With this patch, card_detect and write_protect gets supported on
mx51_babbage, and other mx5 machines can easily get there with the
least board level configuration added.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c |    3 +-
 arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |    3 +-
 arch/arm/mach-imx/mach-mx25_3ds.c              |    2 +
 arch/arm/mach-imx/mach-mx35_3ds.c              |    7 ++-
 arch/arm/mach-imx/mach-pcm043.c                |    2 +
 arch/arm/mach-imx/mach-vpr200.c                |    7 ++-
 arch/arm/mach-mx5/board-mx51_3ds.c             |    7 ++-
 arch/arm/mach-mx5/board-mx51_babbage.c         |   27 +++++-
 arch/arm/mach-mx5/board-mx51_efikamx.c         |    7 ++-
 arch/arm/mach-mx5/board-mx51_efikasb.c         |    7 ++-
 arch/arm/mach-mx5/board-mx53_evk.c             |   14 +++-
 arch/arm/mach-mx5/board-mx53_loco.c            |   14 +++-
 arch/arm/mach-mx5/board-mx53_smd.c             |   21 ++++-
 arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c   |   14 +++-
 arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c   |    7 ++-
 arch/arm/mach-mx5/mx51_efika.c                 |    7 ++-
 arch/arm/plat-mxc/include/mach/esdhc.h         |   25 ++++-
 drivers/mmc/host/sdhci-esdhc-imx.c             |  115 +++++++++++++----------
 18 files changed, 214 insertions(+), 75 deletions(-)

diff --git a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
index 01ebcb3..66e8726 100644
--- a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
+++ b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
@@ -225,7 +225,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
 
 static struct esdhc_platform_data sd1_pdata = {
 	.cd_gpio = GPIO_SD1CD,
-	.wp_gpio = -EINVAL,
+	.cd_type = ESDHC_CD_GPIO,
+	.wp_type = ESDHC_WP_NONE,
 };
 
 /*
diff --git a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
index 558eb52..0f0af02 100644
--- a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
+++ b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
@@ -236,7 +236,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
 
 static struct esdhc_platform_data sd1_pdata = {
 	.cd_gpio = GPIO_SD1CD,
-	.wp_gpio = -EINVAL,
+	.cd_type = ESDHC_CD_GPIO,
+	.wp_type = ESDHC_WP_NONE,
 };
 
 /*
diff --git a/arch/arm/mach-imx/mach-mx25_3ds.c b/arch/arm/mach-imx/mach-mx25_3ds.c
index 01534bb..7f66a91 100644
--- a/arch/arm/mach-imx/mach-mx25_3ds.c
+++ b/arch/arm/mach-imx/mach-mx25_3ds.c
@@ -215,6 +215,8 @@ static const struct imxi2c_platform_data mx25_3ds_i2c0_data __initconst = {
 static const struct esdhc_platform_data mx25pdk_esdhc_pdata __initconst = {
 	.wp_gpio = SD1_GPIO_WP,
 	.cd_gpio = SD1_GPIO_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
 };
 
 static void __init mx25pdk_init(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c b/arch/arm/mach-imx/mach-mx35_3ds.c
index b3b9bd8..9146a6c 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -174,6 +174,11 @@ static const struct imxi2c_platform_data mx35_3ds_i2c0_data __initconst = {
 	.bitrate = 100000,
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 /*
  * Board specific initialization.
  */
@@ -198,7 +203,7 @@ static void __init mx35_3ds_init(void)
 		imx35_add_fsl_usb2_udc(&usb_otg_pdata);
 
 	imx35_add_mxc_nand(&mx35pdk_nand_board_info);
-	imx35_add_sdhci_esdhc_imx(0, NULL);
+	imx35_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
 
 	if (mxc_expio_init(MX35_CS5_BASE_ADDR, EXPIO_PARENT_INT))
 		pr_warn("Init of the debugboard failed, all "
diff --git a/arch/arm/mach-imx/mach-pcm043.c b/arch/arm/mach-imx/mach-pcm043.c
index 163cc31..660ec3e 100644
--- a/arch/arm/mach-imx/mach-pcm043.c
+++ b/arch/arm/mach-imx/mach-pcm043.c
@@ -349,6 +349,8 @@ __setup("otg_mode=", pcm043_otg_mode);
 static struct esdhc_platform_data sd1_pdata = {
 	.wp_gpio = SD1_GPIO_WP,
 	.cd_gpio = SD1_GPIO_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
 };
 
 /*
diff --git a/arch/arm/mach-imx/mach-vpr200.c b/arch/arm/mach-imx/mach-vpr200.c
index 7d8e012..fab8f8c 100644
--- a/arch/arm/mach-imx/mach-vpr200.c
+++ b/arch/arm/mach-imx/mach-vpr200.c
@@ -262,6 +262,11 @@ static struct platform_device *devices[] __initdata = {
 	&vpr200_flash,
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 /*
  * Board specific initialization.
  */
@@ -297,7 +302,7 @@ static void __init vpr200_board_init(void)
 	imx35_add_mxc_ehci_hs(&usb_host_pdata);
 
 	imx35_add_mxc_nand(&vpr200_nand_board_info);
-	imx35_add_sdhci_esdhc_imx(0, NULL);
+	imx35_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
 
 	i2c_register_board_info(0, vpr200_i2c_devices,
 			ARRAY_SIZE(vpr200_i2c_devices));
diff --git a/arch/arm/mach-mx5/board-mx51_3ds.c b/arch/arm/mach-mx5/board-mx51_3ds.c
index 07a3815..2b37100b 100644
--- a/arch/arm/mach-mx5/board-mx51_3ds.c
+++ b/arch/arm/mach-mx5/board-mx51_3ds.c
@@ -131,6 +131,11 @@ static struct spi_board_info mx51_3ds_spi_nor_device[] = {
 	 .platform_data = NULL,},
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 /*
  * Board specific initialization.
  */
@@ -153,7 +158,7 @@ static void __init mx51_3ds_init(void)
 		printk(KERN_WARNING "Init of the debugboard failed, all "
 				    "devices on the board are unusable.\n");
 
-	imx51_add_sdhci_esdhc_imx(0, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
 	imx51_add_imx_keypad(&mx51_3ds_map_data);
 	imx51_add_imx2_wdt(0, NULL);
 }
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
index e54e4bf..4db6cf9 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -34,6 +34,8 @@
 #include "devices.h"
 #include "cpu_op-mx51.h"
 
+#define BABBAGE_ESDHC2_WP	IMX_GPIO_NR(1, 5)
+#define BABBAGE_ESDHC2_CD	IMX_GPIO_NR(1, 6)
 #define BABBAGE_USB_HUB_RESET	IMX_GPIO_NR(1, 7)
 #define BABBAGE_USBH1_STP	IMX_GPIO_NR(1, 27)
 #define BABBAGE_USB_PHY_RESET	IMX_GPIO_NR(2, 5)
@@ -142,6 +144,10 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
 	MX51_PAD_SD1_DATA1__SD1_DATA1,
 	MX51_PAD_SD1_DATA2__SD1_DATA2,
 	MX51_PAD_SD1_DATA3__SD1_DATA3,
+	/* CD signal */
+	MX51_PAD_GPIO1_0__SD1_CD,
+	/* WP signal */
+	MX51_PAD_GPIO1_1__SD1_WP,
 
 	/* SD 2 */
 	MX51_PAD_SD2_CMD__SD2_CMD,
@@ -150,6 +156,11 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
 	MX51_PAD_SD2_DATA1__SD2_DATA1,
 	MX51_PAD_SD2_DATA2__SD2_DATA2,
 	MX51_PAD_SD2_DATA3__SD2_DATA3,
+	/* WP gpio */
+	MX51_PAD_GPIO1_5__GPIO1_5,
+	/* CD gpio */
+	MX51_PAD_GPIO1_6__GPIO1_6,
+
 
 	/* eCSPI1 */
 	MX51_PAD_CSPI1_MISO__ECSPI1_MISO,
@@ -331,6 +342,18 @@ static const struct spi_imx_master mx51_babbage_spi_pdata __initconst = {
 	.num_chipselect = ARRAY_SIZE(mx51_babbage_spi_cs),
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_SIGNAL,
+	.cd_type = ESDHC_CD_SIGNAL,
+};
+
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_gpio = BABBAGE_ESDHC2_WP,
+	.cd_gpio = BABBAGE_ESDHC2_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
+};
+
 /*
  * Board specific initialization.
  */
@@ -376,8 +399,8 @@ static void __init mx51_babbage_init(void)
 	mxc_iomux_v3_setup_pad(usbh1stp);
 	babbage_usbhub_reset();
 
-	imx51_add_sdhci_esdhc_imx(0, NULL);
-	imx51_add_sdhci_esdhc_imx(1, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 
 	spi_register_board_info(mx51_babbage_spi_board_info,
 		ARRAY_SIZE(mx51_babbage_spi_board_info));
diff --git a/arch/arm/mach-mx5/board-mx51_efikamx.c b/arch/arm/mach-mx5/board-mx51_efikamx.c
index f70700d..5ee76cd 100644
--- a/arch/arm/mach-mx5/board-mx51_efikamx.c
+++ b/arch/arm/mach-mx5/board-mx51_efikamx.c
@@ -179,6 +179,11 @@ static const struct gpio_keys_platform_data mx51_efikamx_powerkey_data __initcon
 	.nbuttons = ARRAY_SIZE(mx51_efikamx_powerkey),
 };
 
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 void mx51_efikamx_reset(void)
 {
 	if (system_rev == 0x11)
@@ -239,7 +244,7 @@ static void __init mx51_efikamx_init(void)
 
 	/* on < 1.2 boards both SD controllers are used */
 	if (system_rev < 0x12) {
-		imx51_add_sdhci_esdhc_imx(1, NULL);
+		imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 		mx51_efikamx_leds[2].default_trigger = "mmc1";
 	}
 
diff --git a/arch/arm/mach-mx5/board-mx51_efikasb.c b/arch/arm/mach-mx5/board-mx51_efikasb.c
index 2e4d9d3..b0779de 100644
--- a/arch/arm/mach-mx5/board-mx51_efikasb.c
+++ b/arch/arm/mach-mx5/board-mx51_efikasb.c
@@ -181,6 +181,11 @@ static const struct gpio_keys_platform_data mx51_efikasb_keys_data __initconst =
 	.nbuttons = ARRAY_SIZE(mx51_efikasb_keys),
 };
 
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 static struct regulator *pwgt1, *pwgt2;
 
 static void mx51_efikasb_power_off(void)
@@ -249,7 +254,7 @@ static void __init efikasb_board_init(void)
 
 	mx51_efikasb_board_id();
 	mx51_efikasb_usb();
-	imx51_add_sdhci_esdhc_imx(1, NULL);
+	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 
 	gpio_led_register_device(-1, &mx51_efikasb_leds_data);
 	imx_add_gpio_keys(&mx51_efikasb_keys_data);
diff --git a/arch/arm/mach-mx5/board-mx53_evk.c b/arch/arm/mach-mx5/board-mx53_evk.c
index 1b417b0..1274de2 100644
--- a/arch/arm/mach-mx5/board-mx53_evk.c
+++ b/arch/arm/mach-mx5/board-mx53_evk.c
@@ -131,6 +131,16 @@ static const struct spi_imx_master mx53_evk_spi_data __initconst = {
 	.num_chipselect = ARRAY_SIZE(mx53_evk_spi_cs),
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 static void __init mx53_evk_board_init(void)
 {
 	imx53_soc_init();
@@ -144,8 +154,8 @@ static void __init mx53_evk_board_init(void)
 	imx53_add_imx_i2c(0, &mx53_evk_i2c_data);
 	imx53_add_imx_i2c(1, &mx53_evk_i2c_data);
 
-	imx53_add_sdhci_esdhc_imx(0, NULL);
-	imx53_add_sdhci_esdhc_imx(1, NULL);
+	imx53_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx53_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 
 	spi_register_board_info(mx53_evk_spi_board_info,
 		ARRAY_SIZE(mx53_evk_spi_board_info));
diff --git a/arch/arm/mach-mx5/board-mx53_loco.c b/arch/arm/mach-mx5/board-mx53_loco.c
index 57576f7..80572cd 100644
--- a/arch/arm/mach-mx5/board-mx53_loco.c
+++ b/arch/arm/mach-mx5/board-mx53_loco.c
@@ -239,6 +239,16 @@ static const struct gpio_led_platform_data mx53loco_leds_data __initconst = {
 	.num_leds	= ARRAY_SIZE(mx53loco_leds),
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
+static const struct esdhc_platform_data esdhc3_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 static void __init mx53_loco_board_init(void)
 {
 	imx53_soc_init();
@@ -251,8 +261,8 @@ static void __init mx53_loco_board_init(void)
 	imx53_add_imx2_wdt(0, NULL);
 	imx53_add_imx_i2c(0, &mx53_loco_i2c_data);
 	imx53_add_imx_i2c(1, &mx53_loco_i2c_data);
-	imx53_add_sdhci_esdhc_imx(0, NULL);
-	imx53_add_sdhci_esdhc_imx(2, NULL);
+	imx53_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx53_add_sdhci_esdhc_imx(2, &esdhc3_pdata);
 	imx_add_gpio_keys(&loco_button_data);
 	gpio_led_register_device(-1, &mx53loco_leds_data);
 }
diff --git a/arch/arm/mach-mx5/board-mx53_smd.c b/arch/arm/mach-mx5/board-mx53_smd.c
index bc02894..c910535 100644
--- a/arch/arm/mach-mx5/board-mx53_smd.c
+++ b/arch/arm/mach-mx5/board-mx53_smd.c
@@ -111,6 +111,21 @@ static const struct imxi2c_platform_data mx53_smd_i2c_data __initconst = {
 	.bitrate = 100000,
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
+static const struct esdhc_platform_data esdhc3_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 static void __init mx53_smd_board_init(void)
 {
 	imx53_soc_init();
@@ -122,9 +137,9 @@ static void __init mx53_smd_board_init(void)
 	imx53_add_fec(&mx53_smd_fec_data);
 	imx53_add_imx2_wdt(0, NULL);
 	imx53_add_imx_i2c(0, &mx53_smd_i2c_data);
-	imx53_add_sdhci_esdhc_imx(0, NULL);
-	imx53_add_sdhci_esdhc_imx(1, NULL);
-	imx53_add_sdhci_esdhc_imx(2, NULL);
+	imx53_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx53_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
+	imx53_add_sdhci_esdhc_imx(2, &esdhc3_pdata);
 }
 
 static void __init mx53_smd_timer_init(void)
diff --git a/arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c b/arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c
index bbf4564..3bed60c 100644
--- a/arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c
+++ b/arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c
@@ -167,6 +167,16 @@ static struct i2c_board_info mbimx51_i2c_devices[] = {
 	},
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 /*
  * baseboard initialization.
  */
@@ -202,6 +212,6 @@ void __init eukrea_mbimx51_baseboard_init(void)
 	i2c_register_board_info(1, mbimx51_i2c_devices,
 				ARRAY_SIZE(mbimx51_i2c_devices));
 
-	imx51_add_sdhci_esdhc_imx(0, NULL);
-	imx51_add_sdhci_esdhc_imx(1, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 }
diff --git a/arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c b/arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c
index 2619239..dd414ac 100644
--- a/arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c
+++ b/arch/arm/mach-mx5/eukrea_mbimxsd-baseboard.c
@@ -115,6 +115,11 @@ static struct i2c_board_info eukrea_mbimxsd_i2c_devices[] = {
 	},
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 /*
  * system init for baseboard usage. Will be called by cpuimx51sd init.
  *
@@ -130,7 +135,7 @@ void __init eukrea_mbimxsd51_baseboard_init(void)
 	imx51_add_imx_uart(1, NULL);
 	imx51_add_imx_uart(2, &uart_pdata);
 
-	imx51_add_sdhci_esdhc_imx(0, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
 
 	gpio_request(GPIO_LED1, "LED1");
 	gpio_direction_output(GPIO_LED1, 1);
diff --git a/arch/arm/mach-mx5/mx51_efika.c b/arch/arm/mach-mx5/mx51_efika.c
index 56739c2..a7b51b4 100644
--- a/arch/arm/mach-mx5/mx51_efika.c
+++ b/arch/arm/mach-mx5/mx51_efika.c
@@ -603,13 +603,18 @@ static const struct spi_imx_master mx51_efika_spi_pdata __initconst = {
 	.num_chipselect = ARRAY_SIZE(mx51_efika_spi_cs),
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 void __init efika_board_common_init(void)
 {
 	mxc_iomux_v3_setup_multiple_pads(mx51efika_pads,
 					ARRAY_SIZE(mx51efika_pads));
 	imx51_add_imx_uart(0, &uart_pdata);
 	mx51_efika_usb();
-	imx51_add_sdhci_esdhc_imx(0, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
 
 	/* FIXME: comes from original code. check this. */
 	if (mx51_revision() < IMX_CHIP_REVISION_2_0)
diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
index 86003f4..fced348 100644
--- a/arch/arm/plat-mxc/include/mach/esdhc.h
+++ b/arch/arm/plat-mxc/include/mach/esdhc.h
@@ -10,17 +10,32 @@
 #ifndef __ASM_ARCH_IMX_ESDHC_H
 #define __ASM_ARCH_IMX_ESDHC_H
 
+enum wp_types {
+	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
+	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */
+	ESDHC_WP_GPIO,		/* external gpio pin for WP */
+};
+
+enum cd_types {
+	ESDHC_CD_NONE,		/* no CD, neither signal nor gpio */
+	ESDHC_CD_SIGNAL,	/* mmc internal CD signal */
+	ESDHC_CD_GPIO,		/* external gpio pin for CD */
+	ESDHC_CD_PERMANENT,	/* no CD, card permanently wired to host */
+};
+
 /**
- * struct esdhc_platform_data - optional platform data for esdhc on i.MX
- *
- * strongly recommended for i.MX25/35, not needed for other variants
+ * struct esdhc_platform_data - platform data for esdhc on i.MX
  *
- * @wp_gpio:	gpio for write_protect (-EINVAL if unused)
- * @cd_gpio:	gpio for card_detect interrupt (-EINVAL if unused)
+ * @wp_gpio:	gpio for write_protect
+ * @cd_gpio:	gpio for card_detect interrupt
+ * @wp_type:	type of write_protect method (see wp_types enum above)
+ * @cd_type:	type of card_detect method (see cd_types enum above)
  */
 
 struct esdhc_platform_data {
 	unsigned int wp_gpio;
 	unsigned int cd_gpio;
+	enum wp_types wp_type;
+	enum cd_types cd_type;
 };
 #endif /* __ASM_ARCH_IMX_ESDHC_H */
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 79b7a9a..9cba6eb 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -29,7 +29,6 @@
 #define SDHCI_VENDOR_SPEC		0xC0
 #define  SDHCI_VENDOR_SPEC_SDIO_QUIRK	0x00000002
 
-#define ESDHC_FLAG_GPIO_FOR_CD		(1 << 0)
 /*
  * The CMDTYPE of the CMD register (offset 0xE) should be set to
  * "11" when the STOP CMD12 is issued on imx53 to abort one
@@ -70,19 +69,15 @@ static inline void esdhc_clrset_le(struct sdhci_host *host, u32 mask, u32 val, i
 
 static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 {
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
 
-	/* fake CARD_PRESENT flag on mx25/35 */
+	/* fake CARD_PRESENT flag */
 	u32 val = readl(host->ioaddr + reg);
 
 	if (unlikely((reg == SDHCI_PRESENT_STATE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD))) {
-		struct esdhc_platform_data *boarddata =
-				host->mmc->parent->platform_data;
-
-		if (boarddata && gpio_is_valid(boarddata->cd_gpio)
-				&& gpio_get_value(boarddata->cd_gpio))
+			&& gpio_is_valid(boarddata->cd_gpio))) {
+		if (gpio_get_value(boarddata->cd_gpio))
 			/* no card, if a valid gpio says so... */
 			val &= ~SDHCI_CARD_PRESENT;
 		else
@@ -97,12 +92,13 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
 
 	if (unlikely((reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD)))
+			&& (boarddata->cd_type == ESDHC_CD_GPIO)))
 		/*
 		 * these interrupts won't work with a custom card_detect gpio
-		 * (only applied to mx25/35)
 		 */
 		val &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
 
@@ -201,6 +197,18 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host)
 	return clk_get_rate(pltfm_host->clk) / 256 / 16;
 }
 
+static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
+{
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
+
+	if (gpio_is_valid(boarddata->wp_gpio))
+		return gpio_get_value(boarddata->wp_gpio);
+	else
+		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
+				SDHCI_WRITE_PROTECT);
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -212,6 +220,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_max_blk_size = esdhc_pltfm_get_max_blk_size,
 	.get_max_blk_count = esdhc_pltfm_get_max_blk_count,
+	.get_ro = esdhc_pltfm_get_ro,
 };
 
 static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
@@ -221,17 +230,6 @@ static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
 	.ops = &sdhci_esdhc_ops,
 };
 
-static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
-{
-	struct esdhc_platform_data *boarddata =
-			host->mmc->parent->platform_data;
-
-	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
-		return gpio_get_value(boarddata->wp_gpio);
-	else
-		return -ENOSYS;
-}
-
 static irqreturn_t cd_irq(int irq, void *data)
 {
 	struct sdhci_host *sdhost = (struct sdhci_host *)data;
@@ -272,23 +270,33 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (!cpu_is_mx25())
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
-	if (cpu_is_mx25() || cpu_is_mx35()) {
-		/* write_protect can't be routed to controller, use gpio */
-		sdhci_esdhc_ops.get_ro = esdhc_pltfm_get_ro;
-	}
-
 	if (!(cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51()))
 		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
 
 	boarddata = host->mmc->parent->platform_data;
-	if (boarddata) {
+	if (!boarddata) {
+		dev_err(mmc_dev(host->mmc), "no board data!\n");
+		err = -EINVAL;
+		goto no_board_data;
+	}
+
+	/* write_protect */
+	if (boarddata->wp_type == ESDHC_WP_GPIO) {
 		err = gpio_request_one(boarddata->wp_gpio, GPIOF_IN, "ESDHC_WP");
 		if (err) {
 			dev_warn(mmc_dev(host->mmc),
-				"no write-protect pin available!\n");
-			boarddata->wp_gpio = err;
+				 "no write-protect pin available!\n");
+			boarddata->wp_gpio = -EINVAL;
 		}
+	} else
+		boarddata->wp_gpio = -EINVAL;
 
+	/* card_detect */
+	if (boarddata->cd_type != ESDHC_CD_GPIO)
+		boarddata->cd_gpio = -EINVAL;
+
+	switch (boarddata->cd_type) {
+	case ESDHC_CD_GPIO:
 		err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
 		if (err) {
 			dev_warn(mmc_dev(host->mmc),
@@ -296,10 +304,6 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 			goto no_card_detect_pin;
 		}
 
-		/* i.MX5x has issues to be researched */
-		if (!cpu_is_mx25() && !cpu_is_mx35())
-			goto not_supported;
-
 		err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
 				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 				 mmc_hostname(host->mmc), host);
@@ -307,10 +311,19 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 			dev_warn(mmc_dev(host->mmc), "request irq error\n");
 			goto no_card_detect_irq;
 		}
+		/* fall through */
 
-		imx_data->flags |= ESDHC_FLAG_GPIO_FOR_CD;
-		/* Now we have a working card_detect again */
+	case ESDHC_CD_SIGNAL:
+		/* we have a working card_detect back */
 		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+		break;
+
+	case ESDHC_CD_PERMANENT:
+		host->mmc->caps = MMC_CAP_NONREMOVABLE;
+		break;
+
+	case ESDHC_CD_NONE:
+		break;
 	}
 
 	err = sdhci_add_host(host);
@@ -319,16 +332,20 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 
 	return 0;
 
- no_card_detect_irq:
-	gpio_free(boarddata->cd_gpio);
- no_card_detect_pin:
-	boarddata->cd_gpio = err;
- not_supported:
-	kfree(imx_data);
- err_add_host:
+err_add_host:
+	if (gpio_is_valid(boarddata->cd_gpio))
+		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
+no_card_detect_irq:
+	if (gpio_is_valid(boarddata->cd_gpio))
+		gpio_free(boarddata->cd_gpio);
+	if (gpio_is_valid(boarddata->wp_gpio))
+		gpio_free(boarddata->wp_gpio);
+no_card_detect_pin:
+no_board_data:
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
- err_clk_get:
+err_clk_get:
+	kfree(imx_data);
 	sdhci_pltfm_free(pdev);
 	return err;
 }
@@ -343,14 +360,12 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 
-	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
+	if (gpio_is_valid(boarddata->wp_gpio))
 		gpio_free(boarddata->wp_gpio);
 
-	if (boarddata && gpio_is_valid(boarddata->cd_gpio)) {
+	if (gpio_is_valid(boarddata->cd_gpio)) {
+		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
 		gpio_free(boarddata->cd_gpio);
-
-		if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION))
-			free_irq(gpio_to_irq(boarddata->cd_gpio), host);
 	}
 
 	clk_disable(pltfm_host->clk);
-- 
1.7.4.1

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@linaro.org> writes:

Hi,

> The patch extends card_detect and write_protect support to get mx5
> family and more scenarios supported.  The changes include:
>
>  * Turn platform_data from optional to mandatory
>  * Add cd_types and wp_types into platform_data to cover more use
>    cases
>  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
>  * Adjust machine codes to adopt the platform_data changes

Before I go and test theses patches, I'd like to get some
clarification. From what I see, you've modified all over the place the
code to provide a platform_data, setting wp/cd type to type "NONE", as
if it was the default you choose. Why this default and not considerer
the "SIGNAL" type being the default ? Is this choice the safest one when
one doesn't know what type to choose or can it have some bad side
effects ?
Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
provide the default platform_data by themselves that if the 2nd argument
was NULL instead of modifying all theses machines files ?


Last comment: How did you choose the platform_data values ? I mean, for
that the cases I'm mainly take care of (efika mx and sb platforms), you
choose NONE type, while the code has :

        MX51_PAD_GPIO1_0__SD1_CD,
        MX51_PAD_GPIO1_1__SD1_WP,
        MX51_PAD_GPIO1_7__SD2_WP,
        MX51_PAD_GPIO1_8__SD2_CD,

which means that it would rather be the SIGNAL type if I got it
right. Does this mean that you've set the type to NONE for all platforms
you didn't know what the answer was ? (I guess/hope that theses 2
questions will be answered by your answers to my previous questions tbh).

Thanks,
Arnaud

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Shawn Guo]

On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@linaro.org> writes:
> 
> Hi,
> 
> > The patch extends card_detect and write_protect support to get mx5
> > family and more scenarios supported.  The changes include:
> >
> >  * Turn platform_data from optional to mandatory
> >  * Add cd_types and wp_types into platform_data to cover more use
> >    cases
> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
> >  * Adjust machine codes to adopt the platform_data changes
> 
> Before I go and test theses patches, I'd like to get some
> clarification. From what I see, you've modified all over the place the
> code to provide a platform_data, setting wp/cd type to type "NONE", as
> if it was the default you choose. Why this default and not considerer
> the "SIGNAL" type being the default ? Is this choice the safest one when
> one doesn't know what type to choose or can it have some bad side
> effects ?

The mx51_babbage is the only board support I'm concerned about in
this patch.  For other boards, I chose to translate the NULL pdata
into "NONE" for both wp/cd types as the safest one, because I do not
have (or care to check) the board schematics telling how wp/cd are
routed on those boards.  The patch ensures there is no regression
for those boards, and let people who have schematics to set up wp/cd
types later.

> Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
> provide the default platform_data by themselves that if the 2nd argument
> was NULL instead of modifying all theses machines files ?
> 
As I said above, the wp/cd "NONE" types translated from NULL pdata
will be set up properly later by people who have schematics.

> 
> Last comment: How did you choose the platform_data values ? I mean, for
> that the cases I'm mainly take care of (efika mx and sb platforms), you
> choose NONE type, while the code has :
> 
>         MX51_PAD_GPIO1_0__SD1_CD,
>         MX51_PAD_GPIO1_1__SD1_WP,
>         MX51_PAD_GPIO1_7__SD2_WP,
>         MX51_PAD_GPIO1_8__SD2_CD,
> 
> which means that it would rather be the SIGNAL type if I got it
> right. Does this mean that you've set the type to NONE for all platforms
> you didn't know what the answer was ? (I guess/hope that theses 2
> questions will be answered by your answers to my previous questions tbh).
> 
I hope you have got the answers to these 2 questions from above
answer.  Otherwise, please let me know.

-- 
Regards,
Shawn

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@freescale.com> writes:

> On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
>> Shawn Guo <shawn.guo@linaro.org> writes:
>> 
>> Hi,
>> 
>> > The patch extends card_detect and write_protect support to get mx5
>> > family and more scenarios supported.  The changes include:
>> >
>> >  * Turn platform_data from optional to mandatory
>> >  * Add cd_types and wp_types into platform_data to cover more use
>> >    cases
>> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
>> >  * Adjust machine codes to adopt the platform_data changes
>> 
>> Before I go and test theses patches, I'd like to get some
>> clarification. From what I see, you've modified all over the place the
>> code to provide a platform_data, setting wp/cd type to type "NONE", as
>> if it was the default you choose. Why this default and not considerer
>> the "SIGNAL" type being the default ? Is this choice the safest one when
>> one doesn't know what type to choose or can it have some bad side
>> effects ?
>
> The mx51_babbage is the only board support I'm concerned about in
> this patch.  For other boards, I chose to translate the NULL pdata
> into "NONE" for both wp/cd types as the safest one, because I do not
> have (or care to check) the board schematics telling how wp/cd are
> routed on those boards.  The patch ensures there is no regression
> for those boards, and let people who have schematics to set up wp/cd
> types later.

ok. Thanks for making things clear. I see some changes for
loco/imx53qsb. Do you need testers for it too or you've tested it ?

>
>> Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
>> provide the default platform_data by themselves that if the 2nd argument
>> was NULL instead of modifying all theses machines files ?
>> 
> As I said above, the wp/cd "NONE" types translated from NULL pdata
> will be set up properly later by people who have schematics.

You're not answering my question about moving the NULL-> "NONE" type
from *all* modified machine file into the imx*add_sdhci_esdhc_imx(). If
it's the default, why all machines file have to be modified to set it ?
Moreover, *nothing* AFAICS is preventing to call theses functions will
NULL. What will happen ? An oops ? To me, a default is the value set
when nothing is set, and clearly modifying all functions call site due
to having to provide the default seems imho wrong.

Thanks,
Arnaud

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Shawn Guo]

On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@freescale.com> writes:
> 
> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
> >> Shawn Guo <shawn.guo@linaro.org> writes:
> >> 
> >> Hi,
> >> 
> >> > The patch extends card_detect and write_protect support to get mx5
> >> > family and more scenarios supported.  The changes include:
> >> >
> >> >  * Turn platform_data from optional to mandatory
> >> >  * Add cd_types and wp_types into platform_data to cover more use
> >> >    cases
> >> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
> >> >  * Adjust machine codes to adopt the platform_data changes
> >> 
> >> Before I go and test theses patches, I'd like to get some
> >> clarification. From what I see, you've modified all over the place the
> >> code to provide a platform_data, setting wp/cd type to type "NONE", as
> >> if it was the default you choose. Why this default and not considerer
> >> the "SIGNAL" type being the default ? Is this choice the safest one when
> >> one doesn't know what type to choose or can it have some bad side
> >> effects ?
> >
> > The mx51_babbage is the only board support I'm concerned about in
> > this patch.  For other boards, I chose to translate the NULL pdata
> > into "NONE" for both wp/cd types as the safest one, because I do not
> > have (or care to check) the board schematics telling how wp/cd are
> > routed on those boards.  The patch ensures there is no regression
> > for those boards, and let people who have schematics to set up wp/cd
> > types later.
> 
> ok. Thanks for making things clear. I see some changes for
> loco/imx53qsb. Do you need testers for it too or you've tested it ?
> 
I do not see changes for loco except NULL pdata -> "NONE" types.  But
testing are always welcomed and appreciated.

> >
> >> Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
> >> provide the default platform_data by themselves that if the 2nd argument
> >> was NULL instead of modifying all theses machines files ?
> >> 
> > As I said above, the wp/cd "NONE" types translated from NULL pdata
> > will be set up properly later by people who have schematics.
> 
> You're not answering my question about moving the NULL-> "NONE" type
> from *all* modified machine file into the imx*add_sdhci_esdhc_imx(). If
> it's the default, why all machines file have to be modified to set it ?
> Moreover, *nothing* AFAICS is preventing to call theses functions will
> NULL. What will happen ? An oops ? To me, a default is the value set
> when nothing is set, and clearly modifying all functions call site due
> to having to provide the default seems imho wrong.
> 
Ah, good point.  Please review changes below.  If it looks good to
you, I will incorporate it in the next version of the patch.

diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
index 6b2940b..a880f73 100644
--- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
+++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
@@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
 };
 #endif /* ifdef CONFIG_SOC_IMX53 */

+static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
+       .wp_type = ESDHC_WP_NONE,
+       .cd_type = ESDHC_CD_NONE,
+};
+
 struct platform_device *__init imx_add_sdhci_esdhc_imx(
                const struct imx_sdhci_esdhc_imx_data *data,
                const struct esdhc_platform_data *pdata)
@@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
                },
        };

+       /*
+        * If machine does not provide a pdata, use the default one
+        * which means none WP/CD support
+        */
+       if (!pdata)
+               pdata = &default_esdhc_pdata;
+
        return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
                        ARRAY_SIZE(res), pdata, sizeof(*pdata));
 }

-- 
Regards,
Shawn

[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@freescale.com> writes:

> On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
>> Shawn Guo <shawn.guo@freescale.com> writes:
>> 
>> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
>> >> Shawn Guo <shawn.guo@linaro.org> writes:
>> >> 
>> >> Hi,
>> >> 
>> >> > The patch extends card_detect and write_protect support to get mx5
>> >> > family and more scenarios supported.  The changes include:
>> >> >
>> >> >  * Turn platform_data from optional to mandatory
>> >> >  * Add cd_types and wp_types into platform_data to cover more use
>> >> >    cases
>> >> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
>> >> >  * Adjust machine codes to adopt the platform_data changes
>> >> 
>> >> Before I go and test theses patches, I'd like to get some
>> >> clarification. From what I see, you've modified all over the place the
>> >> code to provide a platform_data, setting wp/cd type to type "NONE", as
>> >> if it was the default you choose. Why this default and not considerer
>> >> the "SIGNAL" type being the default ? Is this choice the safest one when
>> >> one doesn't know what type to choose or can it have some bad side
>> >> effects ?
>> >
>> > The mx51_babbage is the only board support I'm concerned about in
>> > this patch.  For other boards, I chose to translate the NULL pdata
>> > into "NONE" for both wp/cd types as the safest one, because I do not
>> > have (or care to check) the board schematics telling how wp/cd are
>> > routed on those boards.  The patch ensures there is no regression
>> > for those boards, and let people who have schematics to set up wp/cd
>> > types later.
>> 
>> ok. Thanks for making things clear. I see some changes for
>> loco/imx53qsb. Do you need testers for it too or you've tested it ?
>> 
> I do not see changes for loco except NULL pdata -> "NONE" types.  But
> testing are always welcomed and appreciated.

oops, sorry. I mixed with the other patches sent today
(http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052705.html)

>
>> >
>> >> Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
>> >> provide the default platform_data by themselves that if the 2nd argument
>> >> was NULL instead of modifying all theses machines files ?
>> >> 
>> > As I said above, the wp/cd "NONE" types translated from NULL pdata
>> > will be set up properly later by people who have schematics.
>> 
>> You're not answering my question about moving the NULL-> "NONE" type
>> from *all* modified machine file into the imx*add_sdhci_esdhc_imx(). If
>> it's the default, why all machines file have to be modified to set it ?
>> Moreover, *nothing* AFAICS is preventing to call theses functions will
>> NULL. What will happen ? An oops ? To me, a default is the value set
>> when nothing is set, and clearly modifying all functions call site due
>> to having to provide the default seems imho wrong.
>> 
> Ah, good point.  Please review changes below.  If it looks good to
> you, I will incorporate it in the next version of the patch.
>
> diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> index 6b2940b..a880f73 100644
> --- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> +++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> @@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
>  };
>  #endif /* ifdef CONFIG_SOC_IMX53 */
>
> +static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
> +       .wp_type = ESDHC_WP_NONE,
> +       .cd_type = ESDHC_CD_NONE,
> +};
> +
>  struct platform_device *__init imx_add_sdhci_esdhc_imx(
>                 const struct imx_sdhci_esdhc_imx_data *data,
>                 const struct esdhc_platform_data *pdata)
> @@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
>                 },
>         };
>
> +       /*
> +        * If machine does not provide a pdata, use the default one
> +        * which means none WP/CD support
> +        */
> +       if (!pdata)
> +               pdata = &default_esdhc_pdata;
> +
>         return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
>                         ARRAY_SIZE(res), pdata, sizeof(*pdata));
>  }

Great. I've still not tested it but it's exactly what I was thinking.

Thanks,
Arnaud

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Shawn Guo]

The patch extends card_detect and write_protect support to get mx5
family and more scenarios supported.  The changes include:

 * Turn platform_data from optional to mandatory
 * Add cd_types and wp_types into platform_data to cover more use
   cases
 * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
 * Adjust some machine codes to adopt the platform_data changes

With this patch, card_detect and write_protect gets supported on
mx51_babbage, and other mx5 machines can easily get there with the
least board level configuration added.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v1:
 * Took the suggestion from Arnaud Patard to add default pdata
   in imx_add_sdhci_esdhc_imx(), to avoid touching every single
   board file for the platform_data changes

 arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c     |    3 +-
 arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c     |    3 +-
 arch/arm/mach-imx/mach-mx25_3ds.c                  |    2 +
 arch/arm/mach-imx/mach-pcm043.c                    |    2 +
 arch/arm/mach-mx5/board-mx51_babbage.c             |   27 +++++-
 .../plat-mxc/devices/platform-sdhci-esdhc-imx.c    |   12 ++
 arch/arm/plat-mxc/include/mach/esdhc.h             |   25 ++++-
 drivers/mmc/host/sdhci-esdhc-imx.c                 |  115 +++++++++++---------
 8 files changed, 130 insertions(+), 59 deletions(-)

diff --git a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
index 01ebcb3..66e8726 100644
--- a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
+++ b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
@@ -225,7 +225,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
 
 static struct esdhc_platform_data sd1_pdata = {
 	.cd_gpio = GPIO_SD1CD,
-	.wp_gpio = -EINVAL,
+	.cd_type = ESDHC_CD_GPIO,
+	.wp_type = ESDHC_WP_NONE,
 };
 
 /*
diff --git a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
index 558eb52..0f0af02 100644
--- a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
+++ b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
@@ -236,7 +236,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
 
 static struct esdhc_platform_data sd1_pdata = {
 	.cd_gpio = GPIO_SD1CD,
-	.wp_gpio = -EINVAL,
+	.cd_type = ESDHC_CD_GPIO,
+	.wp_type = ESDHC_WP_NONE,
 };
 
 /*
diff --git a/arch/arm/mach-imx/mach-mx25_3ds.c b/arch/arm/mach-imx/mach-mx25_3ds.c
index 01534bb..7f66a91 100644
--- a/arch/arm/mach-imx/mach-mx25_3ds.c
+++ b/arch/arm/mach-imx/mach-mx25_3ds.c
@@ -215,6 +215,8 @@ static const struct imxi2c_platform_data mx25_3ds_i2c0_data __initconst = {
 static const struct esdhc_platform_data mx25pdk_esdhc_pdata __initconst = {
 	.wp_gpio = SD1_GPIO_WP,
 	.cd_gpio = SD1_GPIO_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
 };
 
 static void __init mx25pdk_init(void)
diff --git a/arch/arm/mach-imx/mach-pcm043.c b/arch/arm/mach-imx/mach-pcm043.c
index 163cc31..660ec3e 100644
--- a/arch/arm/mach-imx/mach-pcm043.c
+++ b/arch/arm/mach-imx/mach-pcm043.c
@@ -349,6 +349,8 @@ __setup("otg_mode=", pcm043_otg_mode);
 static struct esdhc_platform_data sd1_pdata = {
 	.wp_gpio = SD1_GPIO_WP,
 	.cd_gpio = SD1_GPIO_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
 };
 
 /*
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
index e54e4bf..4db6cf9 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -34,6 +34,8 @@
 #include "devices.h"
 #include "cpu_op-mx51.h"
 
+#define BABBAGE_ESDHC2_WP	IMX_GPIO_NR(1, 5)
+#define BABBAGE_ESDHC2_CD	IMX_GPIO_NR(1, 6)
 #define BABBAGE_USB_HUB_RESET	IMX_GPIO_NR(1, 7)
 #define BABBAGE_USBH1_STP	IMX_GPIO_NR(1, 27)
 #define BABBAGE_USB_PHY_RESET	IMX_GPIO_NR(2, 5)
@@ -142,6 +144,10 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
 	MX51_PAD_SD1_DATA1__SD1_DATA1,
 	MX51_PAD_SD1_DATA2__SD1_DATA2,
 	MX51_PAD_SD1_DATA3__SD1_DATA3,
+	/* CD signal */
+	MX51_PAD_GPIO1_0__SD1_CD,
+	/* WP signal */
+	MX51_PAD_GPIO1_1__SD1_WP,
 
 	/* SD 2 */
 	MX51_PAD_SD2_CMD__SD2_CMD,
@@ -150,6 +156,11 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
 	MX51_PAD_SD2_DATA1__SD2_DATA1,
 	MX51_PAD_SD2_DATA2__SD2_DATA2,
 	MX51_PAD_SD2_DATA3__SD2_DATA3,
+	/* WP gpio */
+	MX51_PAD_GPIO1_5__GPIO1_5,
+	/* CD gpio */
+	MX51_PAD_GPIO1_6__GPIO1_6,
+
 
 	/* eCSPI1 */
 	MX51_PAD_CSPI1_MISO__ECSPI1_MISO,
@@ -331,6 +342,18 @@ static const struct spi_imx_master mx51_babbage_spi_pdata __initconst = {
 	.num_chipselect = ARRAY_SIZE(mx51_babbage_spi_cs),
 };
 
+static const struct esdhc_platform_data esdhc1_pdata __initconst = {
+	.wp_type = ESDHC_WP_SIGNAL,
+	.cd_type = ESDHC_CD_SIGNAL,
+};
+
+static const struct esdhc_platform_data esdhc2_pdata __initconst = {
+	.wp_gpio = BABBAGE_ESDHC2_WP,
+	.cd_gpio = BABBAGE_ESDHC2_CD,
+	.wp_type = ESDHC_WP_GPIO,
+	.cd_type = ESDHC_CD_GPIO,
+};
+
 /*
  * Board specific initialization.
  */
@@ -376,8 +399,8 @@ static void __init mx51_babbage_init(void)
 	mxc_iomux_v3_setup_pad(usbh1stp);
 	babbage_usbhub_reset();
 
-	imx51_add_sdhci_esdhc_imx(0, NULL);
-	imx51_add_sdhci_esdhc_imx(1, NULL);
+	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
+	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
 
 	spi_register_board_info(mx51_babbage_spi_board_info,
 		ARRAY_SIZE(mx51_babbage_spi_board_info));
diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
index 6b2940b..a880f73 100644
--- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
+++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
@@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
 };
 #endif /* ifdef CONFIG_SOC_IMX53 */
 
+static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
+	.wp_type = ESDHC_WP_NONE,
+	.cd_type = ESDHC_CD_NONE,
+};
+
 struct platform_device *__init imx_add_sdhci_esdhc_imx(
 		const struct imx_sdhci_esdhc_imx_data *data,
 		const struct esdhc_platform_data *pdata)
@@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
 		},
 	};
 
+	/*
+	 * If machine does not provide a pdata, use the default one
+	 * which means none WP/CD support
+	 */
+	if (!pdata)
+		pdata = &default_esdhc_pdata;
+
 	return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
 			ARRAY_SIZE(res), pdata, sizeof(*pdata));
 }
diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
index 86003f4..fced348 100644
--- a/arch/arm/plat-mxc/include/mach/esdhc.h
+++ b/arch/arm/plat-mxc/include/mach/esdhc.h
@@ -10,17 +10,32 @@
 #ifndef __ASM_ARCH_IMX_ESDHC_H
 #define __ASM_ARCH_IMX_ESDHC_H
 
+enum wp_types {
+	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
+	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */
+	ESDHC_WP_GPIO,		/* external gpio pin for WP */
+};
+
+enum cd_types {
+	ESDHC_CD_NONE,		/* no CD, neither signal nor gpio */
+	ESDHC_CD_SIGNAL,	/* mmc internal CD signal */
+	ESDHC_CD_GPIO,		/* external gpio pin for CD */
+	ESDHC_CD_PERMANENT,	/* no CD, card permanently wired to host */
+};
+
 /**
- * struct esdhc_platform_data - optional platform data for esdhc on i.MX
- *
- * strongly recommended for i.MX25/35, not needed for other variants
+ * struct esdhc_platform_data - platform data for esdhc on i.MX
  *
- * @wp_gpio:	gpio for write_protect (-EINVAL if unused)
- * @cd_gpio:	gpio for card_detect interrupt (-EINVAL if unused)
+ * @wp_gpio:	gpio for write_protect
+ * @cd_gpio:	gpio for card_detect interrupt
+ * @wp_type:	type of write_protect method (see wp_types enum above)
+ * @cd_type:	type of card_detect method (see cd_types enum above)
  */
 
 struct esdhc_platform_data {
 	unsigned int wp_gpio;
 	unsigned int cd_gpio;
+	enum wp_types wp_type;
+	enum cd_types cd_type;
 };
 #endif /* __ASM_ARCH_IMX_ESDHC_H */
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 79b7a9a..9cba6eb 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -29,7 +29,6 @@
 #define SDHCI_VENDOR_SPEC		0xC0
 #define  SDHCI_VENDOR_SPEC_SDIO_QUIRK	0x00000002
 
-#define ESDHC_FLAG_GPIO_FOR_CD		(1 << 0)
 /*
  * The CMDTYPE of the CMD register (offset 0xE) should be set to
  * "11" when the STOP CMD12 is issued on imx53 to abort one
@@ -70,19 +69,15 @@ static inline void esdhc_clrset_le(struct sdhci_host *host, u32 mask, u32 val, i
 
 static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 {
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
 
-	/* fake CARD_PRESENT flag on mx25/35 */
+	/* fake CARD_PRESENT flag */
 	u32 val = readl(host->ioaddr + reg);
 
 	if (unlikely((reg == SDHCI_PRESENT_STATE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD))) {
-		struct esdhc_platform_data *boarddata =
-				host->mmc->parent->platform_data;
-
-		if (boarddata && gpio_is_valid(boarddata->cd_gpio)
-				&& gpio_get_value(boarddata->cd_gpio))
+			&& gpio_is_valid(boarddata->cd_gpio))) {
+		if (gpio_get_value(boarddata->cd_gpio))
 			/* no card, if a valid gpio says so... */
 			val &= ~SDHCI_CARD_PRESENT;
 		else
@@ -97,12 +92,13 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
 
 	if (unlikely((reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)
-			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD)))
+			&& (boarddata->cd_type == ESDHC_CD_GPIO)))
 		/*
 		 * these interrupts won't work with a custom card_detect gpio
-		 * (only applied to mx25/35)
 		 */
 		val &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
 
@@ -201,6 +197,18 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host)
 	return clk_get_rate(pltfm_host->clk) / 256 / 16;
 }
 
+static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
+{
+	struct esdhc_platform_data *boarddata =
+			host->mmc->parent->platform_data;
+
+	if (gpio_is_valid(boarddata->wp_gpio))
+		return gpio_get_value(boarddata->wp_gpio);
+	else
+		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
+				SDHCI_WRITE_PROTECT);
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -212,6 +220,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_max_blk_size = esdhc_pltfm_get_max_blk_size,
 	.get_max_blk_count = esdhc_pltfm_get_max_blk_count,
+	.get_ro = esdhc_pltfm_get_ro,
 };
 
 static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
@@ -221,17 +230,6 @@ static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
 	.ops = &sdhci_esdhc_ops,
 };
 
-static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
-{
-	struct esdhc_platform_data *boarddata =
-			host->mmc->parent->platform_data;
-
-	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
-		return gpio_get_value(boarddata->wp_gpio);
-	else
-		return -ENOSYS;
-}
-
 static irqreturn_t cd_irq(int irq, void *data)
 {
 	struct sdhci_host *sdhost = (struct sdhci_host *)data;
@@ -272,23 +270,33 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (!cpu_is_mx25())
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
-	if (cpu_is_mx25() || cpu_is_mx35()) {
-		/* write_protect can't be routed to controller, use gpio */
-		sdhci_esdhc_ops.get_ro = esdhc_pltfm_get_ro;
-	}
-
 	if (!(cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51()))
 		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
 
 	boarddata = host->mmc->parent->platform_data;
-	if (boarddata) {
+	if (!boarddata) {
+		dev_err(mmc_dev(host->mmc), "no board data!\n");
+		err = -EINVAL;
+		goto no_board_data;
+	}
+
+	/* write_protect */
+	if (boarddata->wp_type == ESDHC_WP_GPIO) {
 		err = gpio_request_one(boarddata->wp_gpio, GPIOF_IN, "ESDHC_WP");
 		if (err) {
 			dev_warn(mmc_dev(host->mmc),
-				"no write-protect pin available!\n");
-			boarddata->wp_gpio = err;
+				 "no write-protect pin available!\n");
+			boarddata->wp_gpio = -EINVAL;
 		}
+	} else
+		boarddata->wp_gpio = -EINVAL;
 
+	/* card_detect */
+	if (boarddata->cd_type != ESDHC_CD_GPIO)
+		boarddata->cd_gpio = -EINVAL;
+
+	switch (boarddata->cd_type) {
+	case ESDHC_CD_GPIO:
 		err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
 		if (err) {
 			dev_warn(mmc_dev(host->mmc),
@@ -296,10 +304,6 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 			goto no_card_detect_pin;
 		}
 
-		/* i.MX5x has issues to be researched */
-		if (!cpu_is_mx25() && !cpu_is_mx35())
-			goto not_supported;
-
 		err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
 				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 				 mmc_hostname(host->mmc), host);
@@ -307,10 +311,19 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 			dev_warn(mmc_dev(host->mmc), "request irq error\n");
 			goto no_card_detect_irq;
 		}
+		/* fall through */
 
-		imx_data->flags |= ESDHC_FLAG_GPIO_FOR_CD;
-		/* Now we have a working card_detect again */
+	case ESDHC_CD_SIGNAL:
+		/* we have a working card_detect back */
 		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+		break;
+
+	case ESDHC_CD_PERMANENT:
+		host->mmc->caps = MMC_CAP_NONREMOVABLE;
+		break;
+
+	case ESDHC_CD_NONE:
+		break;
 	}
 
 	err = sdhci_add_host(host);
@@ -319,16 +332,20 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 
 	return 0;
 
- no_card_detect_irq:
-	gpio_free(boarddata->cd_gpio);
- no_card_detect_pin:
-	boarddata->cd_gpio = err;
- not_supported:
-	kfree(imx_data);
- err_add_host:
+err_add_host:
+	if (gpio_is_valid(boarddata->cd_gpio))
+		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
+no_card_detect_irq:
+	if (gpio_is_valid(boarddata->cd_gpio))
+		gpio_free(boarddata->cd_gpio);
+	if (gpio_is_valid(boarddata->wp_gpio))
+		gpio_free(boarddata->wp_gpio);
+no_card_detect_pin:
+no_board_data:
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
- err_clk_get:
+err_clk_get:
+	kfree(imx_data);
 	sdhci_pltfm_free(pdev);
 	return err;
 }
@@ -343,14 +360,12 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 
-	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
+	if (gpio_is_valid(boarddata->wp_gpio))
 		gpio_free(boarddata->wp_gpio);
 
-	if (boarddata && gpio_is_valid(boarddata->cd_gpio)) {
+	if (gpio_is_valid(boarddata->cd_gpio)) {
+		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
 		gpio_free(boarddata->cd_gpio);
-
-		if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION))
-			free_irq(gpio_to_irq(boarddata->cd_gpio), host);
 	}
 
 	clk_disable(pltfm_host->clk);
-- 
1.7.4.1

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
> The card-present polling within sdhci based driver is very expensive
> in terms of the impact to system performance.  We observe a few
> system performance issues from Freescale and Linaro on mx5 platforms,
> which have been proved card polling related.
> 
> The patch set extends the current sdhci-esdhc-imx card_detect and
> write_protect support to cover mx5 platforms, and solves above
> performance issues.
> 
> Shawn Guo (4):
>       mmc: sdhci: fix interrupt storm from card detection
>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
> 
Hi Arnaud,

Any chance to play with it yet?

Hi Wolfram,

Any comment?

-- 
Regards,
Shawn

[PATCH 2/4] mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared

[Wolfram Sang]

On Fri, Jun 10, 2011 at 06:42:50PM +0800, Shawn Guo wrote:
> The function esdhc_readl_le intends to clear bit SDHCI_CARD_PRESENT,
> when the card detect gpio tells there is no card.  But it does not
> clear the bit actually.  The patch gives a fix on that.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

Should go to stable, too.

Thanks for catching it!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/65ad02ca/attachment.sig>

[PATCH 1/4] mmc: sdhci: fix interrupt storm from card detection

[Wolfram Sang]

On Fri, Jun 10, 2011 at 06:42:49PM +0800, Shawn Guo wrote:
> The issue was initially found by Eric Benard as below.
> 
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031
> 
> Not sure about other SDHCI based controller, but on Freescale eSDHC,
> the SDHCI_INT_CARD_INSERT bits will be immediately set again when it
> gets cleared, if a card is inserted. The driver need to mask the irq
> to prevent interrupt storm which will freeze the system.  And the
> SDHCI_INT_CARD_REMOVE gets the same situation.
> 
> The patch fixes the problem based on the initial idea from
> Eric Benard.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Eric Benard <eric@eukrea.com>

Hmm, that should get enough testing on non-imx (and even non-ARM)
devices. And a comment describing the situation.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/43d260b1/attachment.sig>

[PATCH 3/4] mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP

[Wolfram Sang]

On Fri, Jun 10, 2011 at 06:42:51PM +0800, Shawn Guo wrote:
> The use of flag ESDHC_FLAG_GPIO_FOR_CD_WP is all CD related.  It does
> not necessarily need to bother WP in the flag name.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

I'd just squash it into the next one?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/b61e9c87/attachment.sig>

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Wolfram Sang]

On Tue, Jun 14, 2011 at 02:47:39PM +0800, Shawn Guo wrote:
> The patch extends card_detect and write_protect support to get mx5
> family and more scenarios supported.  The changes include:
> 
>  * Turn platform_data from optional to mandatory
>  * Add cd_types and wp_types into platform_data to cover more use
>    cases
>  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
>  * Adjust some machine codes to adopt the platform_data changes
> 
> With this patch, card_detect and write_protect gets supported on
> mx51_babbage, and other mx5 machines can easily get there with the
> least board level configuration added.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

No time for a full review, but it looks a lot like going into the right
direction. Thanks.

A few comments:

> ---
> Changes since v1:
>  * Took the suggestion from Arnaud Patard to add default pdata
>    in imx_add_sdhci_esdhc_imx(), to avoid touching every single
>    board file for the platform_data changes
> 
>  arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c     |    3 +-
>  arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c     |    3 +-
>  arch/arm/mach-imx/mach-mx25_3ds.c                  |    2 +
>  arch/arm/mach-imx/mach-pcm043.c                    |    2 +
>  arch/arm/mach-mx5/board-mx51_babbage.c             |   27 +++++-
>  .../plat-mxc/devices/platform-sdhci-esdhc-imx.c    |   12 ++
>  arch/arm/plat-mxc/include/mach/esdhc.h             |   25 ++++-
>  drivers/mmc/host/sdhci-esdhc-imx.c                 |  115 +++++++++++---------
>  8 files changed, 130 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> index 01ebcb3..66e8726 100644
> --- a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> +++ b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> @@ -225,7 +225,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
>  
>  static struct esdhc_platform_data sd1_pdata = {
>  	.cd_gpio = GPIO_SD1CD,
> -	.wp_gpio = -EINVAL,
> +	.cd_type = ESDHC_CD_GPIO,
> +	.wp_type = ESDHC_WP_NONE,
>  };
>  
>  /*
> diff --git a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> index 558eb52..0f0af02 100644
> --- a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> +++ b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> @@ -236,7 +236,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
>  
>  static struct esdhc_platform_data sd1_pdata = {
>  	.cd_gpio = GPIO_SD1CD,
> -	.wp_gpio = -EINVAL,
> +	.cd_type = ESDHC_CD_GPIO,
> +	.wp_type = ESDHC_WP_NONE,
>  };
>  
>  /*
> diff --git a/arch/arm/mach-imx/mach-mx25_3ds.c b/arch/arm/mach-imx/mach-mx25_3ds.c
> index 01534bb..7f66a91 100644
> --- a/arch/arm/mach-imx/mach-mx25_3ds.c
> +++ b/arch/arm/mach-imx/mach-mx25_3ds.c
> @@ -215,6 +215,8 @@ static const struct imxi2c_platform_data mx25_3ds_i2c0_data __initconst = {
>  static const struct esdhc_platform_data mx25pdk_esdhc_pdata __initconst = {
>  	.wp_gpio = SD1_GPIO_WP,
>  	.cd_gpio = SD1_GPIO_CD,
> +	.wp_type = ESDHC_WP_GPIO,
> +	.cd_type = ESDHC_CD_GPIO,
>  };
>  
>  static void __init mx25pdk_init(void)
> diff --git a/arch/arm/mach-imx/mach-pcm043.c b/arch/arm/mach-imx/mach-pcm043.c
> index 163cc31..660ec3e 100644
> --- a/arch/arm/mach-imx/mach-pcm043.c
> +++ b/arch/arm/mach-imx/mach-pcm043.c
> @@ -349,6 +349,8 @@ __setup("otg_mode=", pcm043_otg_mode);
>  static struct esdhc_platform_data sd1_pdata = {
>  	.wp_gpio = SD1_GPIO_WP,
>  	.cd_gpio = SD1_GPIO_CD,
> +	.wp_type = ESDHC_WP_GPIO,
> +	.cd_type = ESDHC_CD_GPIO,
>  };
>  
>  /*
> diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
> index e54e4bf..4db6cf9 100644
> --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> @@ -34,6 +34,8 @@
>  #include "devices.h"
>  #include "cpu_op-mx51.h"
>  
> +#define BABBAGE_ESDHC2_WP	IMX_GPIO_NR(1, 5)
> +#define BABBAGE_ESDHC2_CD	IMX_GPIO_NR(1, 6)
>  #define BABBAGE_USB_HUB_RESET	IMX_GPIO_NR(1, 7)
>  #define BABBAGE_USBH1_STP	IMX_GPIO_NR(1, 27)
>  #define BABBAGE_USB_PHY_RESET	IMX_GPIO_NR(2, 5)
> @@ -142,6 +144,10 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
>  	MX51_PAD_SD1_DATA1__SD1_DATA1,
>  	MX51_PAD_SD1_DATA2__SD1_DATA2,
>  	MX51_PAD_SD1_DATA3__SD1_DATA3,
> +	/* CD signal */
> +	MX51_PAD_GPIO1_0__SD1_CD,
> +	/* WP signal */
> +	MX51_PAD_GPIO1_1__SD1_WP,
>  
>  	/* SD 2 */
>  	MX51_PAD_SD2_CMD__SD2_CMD,
> @@ -150,6 +156,11 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
>  	MX51_PAD_SD2_DATA1__SD2_DATA1,
>  	MX51_PAD_SD2_DATA2__SD2_DATA2,
>  	MX51_PAD_SD2_DATA3__SD2_DATA3,
> +	/* WP gpio */
> +	MX51_PAD_GPIO1_5__GPIO1_5,
> +	/* CD gpio */
> +	MX51_PAD_GPIO1_6__GPIO1_6,
> +
>  
>  	/* eCSPI1 */
>  	MX51_PAD_CSPI1_MISO__ECSPI1_MISO,
> @@ -331,6 +342,18 @@ static const struct spi_imx_master mx51_babbage_spi_pdata __initconst = {
>  	.num_chipselect = ARRAY_SIZE(mx51_babbage_spi_cs),
>  };
>  
> +static const struct esdhc_platform_data esdhc1_pdata __initconst = {
> +	.wp_type = ESDHC_WP_SIGNAL,
> +	.cd_type = ESDHC_CD_SIGNAL,
> +};
> +
> +static const struct esdhc_platform_data esdhc2_pdata __initconst = {
> +	.wp_gpio = BABBAGE_ESDHC2_WP,
> +	.cd_gpio = BABBAGE_ESDHC2_CD,
> +	.wp_type = ESDHC_WP_GPIO,
> +	.cd_type = ESDHC_CD_GPIO,
> +};
> +
>  /*
>   * Board specific initialization.
>   */
> @@ -376,8 +399,8 @@ static void __init mx51_babbage_init(void)
>  	mxc_iomux_v3_setup_pad(usbh1stp);
>  	babbage_usbhub_reset();
>  
> -	imx51_add_sdhci_esdhc_imx(0, NULL);
> -	imx51_add_sdhci_esdhc_imx(1, NULL);
> +	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
> +	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
>  
>  	spi_register_board_info(mx51_babbage_spi_board_info,
>  		ARRAY_SIZE(mx51_babbage_spi_board_info));
> diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> index 6b2940b..a880f73 100644
> --- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> +++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> @@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
>  };
>  #endif /* ifdef CONFIG_SOC_IMX53 */
>  
> +static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
> +	.wp_type = ESDHC_WP_NONE,
> +	.cd_type = ESDHC_CD_NONE,
> +};
> +
>  struct platform_device *__init imx_add_sdhci_esdhc_imx(
>  		const struct imx_sdhci_esdhc_imx_data *data,
>  		const struct esdhc_platform_data *pdata)
> @@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
>  		},
>  	};
>  
> +	/*
> +	 * If machine does not provide a pdata, use the default one

s/a pdata/pdata/ (not 100% sure, though)

> +	 * which means none WP/CD support

s/none/no/

> +	 */
> +	if (!pdata)
> +		pdata = &default_esdhc_pdata;
> +
>  	return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
>  			ARRAY_SIZE(res), pdata, sizeof(*pdata));
>  }
> diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
> index 86003f4..fced348 100644
> --- a/arch/arm/plat-mxc/include/mach/esdhc.h
> +++ b/arch/arm/plat-mxc/include/mach/esdhc.h
> @@ -10,17 +10,32 @@
>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>  #define __ASM_ARCH_IMX_ESDHC_H
>  
> +enum wp_types {
> +	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
> +	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */

I think SIGNAL is not descriptive enough. Maybe

ESDHC_WP_INTERNAL		/* WP routed directly to mmc controller */

? It should be mentioned on which SoCs this is not available?

> +	ESDHC_WP_GPIO,		/* external gpio pin for WP */
> +};
> +
> +enum cd_types {
> +	ESDHC_CD_NONE,		/* no CD, neither signal nor gpio */
> +	ESDHC_CD_SIGNAL,	/* mmc internal CD signal */

ditto

> +	ESDHC_CD_GPIO,		/* external gpio pin for CD */
> +	ESDHC_CD_PERMANENT,	/* no CD, card permanently wired to host */
> +};
> +
>  /**
> - * struct esdhc_platform_data - optional platform data for esdhc on i.MX
> - *
> - * strongly recommended for i.MX25/35, not needed for other variants
> + * struct esdhc_platform_data - platform data for esdhc on i.MX
>   *
> - * @wp_gpio:	gpio for write_protect (-EINVAL if unused)
> - * @cd_gpio:	gpio for card_detect interrupt (-EINVAL if unused)
> + * @wp_gpio:	gpio for write_protect
> + * @cd_gpio:	gpio for card_detect interrupt
> + * @wp_type:	type of write_protect method (see wp_types enum above)
> + * @cd_type:	type of card_detect method (see cd_types enum above)
>   */
>  
>  struct esdhc_platform_data {
>  	unsigned int wp_gpio;
>  	unsigned int cd_gpio;
> +	enum wp_types wp_type;
> +	enum cd_types cd_type;
>  };
>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 79b7a9a..9cba6eb 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -29,7 +29,6 @@
>  #define SDHCI_VENDOR_SPEC		0xC0
>  #define  SDHCI_VENDOR_SPEC_SDIO_QUIRK	0x00000002
>  
> -#define ESDHC_FLAG_GPIO_FOR_CD		(1 << 0)
>  /*
>   * The CMDTYPE of the CMD register (offset 0xE) should be set to
>   * "11" when the STOP CMD12 is issued on imx53 to abort one
> @@ -70,19 +69,15 @@ static inline void esdhc_clrset_le(struct sdhci_host *host, u32 mask, u32 val, i
>  
>  static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>  {
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +	struct esdhc_platform_data *boarddata =
> +			host->mmc->parent->platform_data;
>  
> -	/* fake CARD_PRESENT flag on mx25/35 */
> +	/* fake CARD_PRESENT flag */
>  	u32 val = readl(host->ioaddr + reg);
>  
>  	if (unlikely((reg == SDHCI_PRESENT_STATE)
> -			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD))) {
> -		struct esdhc_platform_data *boarddata =
> -				host->mmc->parent->platform_data;
> -
> -		if (boarddata && gpio_is_valid(boarddata->cd_gpio)
> -				&& gpio_get_value(boarddata->cd_gpio))
> +			&& gpio_is_valid(boarddata->cd_gpio))) {
> +		if (gpio_get_value(boarddata->cd_gpio))
>  			/* no card, if a valid gpio says so... */
>  			val &= ~SDHCI_CARD_PRESENT;
>  		else
> @@ -97,12 +92,13 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +	struct esdhc_platform_data *boarddata =
> +			host->mmc->parent->platform_data;
>  
>  	if (unlikely((reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)
> -			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD)))
> +			&& (boarddata->cd_type == ESDHC_CD_GPIO)))
>  		/*
>  		 * these interrupts won't work with a custom card_detect gpio
> -		 * (only applied to mx25/35)
>  		 */
>  		val &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
>  
> @@ -201,6 +197,18 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host)
>  	return clk_get_rate(pltfm_host->clk) / 256 / 16;
>  }
>  
> +static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
> +{
> +	struct esdhc_platform_data *boarddata =
> +			host->mmc->parent->platform_data;
> +
> +	if (gpio_is_valid(boarddata->wp_gpio))
> +		return gpio_get_value(boarddata->wp_gpio);
> +	else
> +		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
> +				SDHCI_WRITE_PROTECT);
> +}

Aren't you missing the NONE case here? Plus, I don't like having a
get_ro-function for the SIGNAL case, because in that case it is
superfluous. Though, I am not feeling strong about this if it makes the
rest of the code messier.

> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -212,6 +220,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_min_clock = esdhc_pltfm_get_min_clock,
>  	.get_max_blk_size = esdhc_pltfm_get_max_blk_size,
>  	.get_max_blk_count = esdhc_pltfm_get_max_blk_count,
> +	.get_ro = esdhc_pltfm_get_ro,
>  };
>  
>  static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> @@ -221,17 +230,6 @@ static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>  	.ops = &sdhci_esdhc_ops,
>  };
>  
> -static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
> -{
> -	struct esdhc_platform_data *boarddata =
> -			host->mmc->parent->platform_data;
> -
> -	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
> -		return gpio_get_value(boarddata->wp_gpio);
> -	else
> -		return -ENOSYS;
> -}
> -
>  static irqreturn_t cd_irq(int irq, void *data)
>  {
>  	struct sdhci_host *sdhost = (struct sdhci_host *)data;
> @@ -272,23 +270,33 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	if (!cpu_is_mx25())
>  		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>  
> -	if (cpu_is_mx25() || cpu_is_mx35()) {
> -		/* write_protect can't be routed to controller, use gpio */
> -		sdhci_esdhc_ops.get_ro = esdhc_pltfm_get_ro;
> -	}
> -
>  	if (!(cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51()))
>  		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
>  
>  	boarddata = host->mmc->parent->platform_data;
> -	if (boarddata) {
> +	if (!boarddata) {
> +		dev_err(mmc_dev(host->mmc), "no board data!\n");
> +		err = -EINVAL;
> +		goto no_board_data;
> +	}
> +
> +	/* write_protect */
> +	if (boarddata->wp_type == ESDHC_WP_GPIO) {
>  		err = gpio_request_one(boarddata->wp_gpio, GPIOF_IN, "ESDHC_WP");
>  		if (err) {
>  			dev_warn(mmc_dev(host->mmc),
> -				"no write-protect pin available!\n");
> -			boarddata->wp_gpio = err;
> +				 "no write-protect pin available!\n");
> +			boarddata->wp_gpio = -EINVAL;
>  		}
> +	} else
> +		boarddata->wp_gpio = -EINVAL;

else-block needs braces

>  
> +	/* card_detect */
> +	if (boarddata->cd_type != ESDHC_CD_GPIO)
> +		boarddata->cd_gpio = -EINVAL;
> +
> +	switch (boarddata->cd_type) {
> +	case ESDHC_CD_GPIO:
>  		err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
>  		if (err) {
>  			dev_warn(mmc_dev(host->mmc),
> @@ -296,10 +304,6 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  			goto no_card_detect_pin;
>  		}
>  
> -		/* i.MX5x has issues to be researched */
> -		if (!cpu_is_mx25() && !cpu_is_mx35())
> -			goto not_supported;
> -
>  		err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
>  				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>  				 mmc_hostname(host->mmc), host);
> @@ -307,10 +311,19 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  			dev_warn(mmc_dev(host->mmc), "request irq error\n");
>  			goto no_card_detect_irq;
>  		}
> +		/* fall through */
>  
> -		imx_data->flags |= ESDHC_FLAG_GPIO_FOR_CD;
> -		/* Now we have a working card_detect again */
> +	case ESDHC_CD_SIGNAL:
> +		/* we have a working card_detect back */
>  		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +		break;
> +
> +	case ESDHC_CD_PERMANENT:
> +		host->mmc->caps = MMC_CAP_NONREMOVABLE;
> +		break;
> +
> +	case ESDHC_CD_NONE:
> +		break;
>  	}
>  
>  	err = sdhci_add_host(host);
> @@ -319,16 +332,20 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> - no_card_detect_irq:
> -	gpio_free(boarddata->cd_gpio);
> - no_card_detect_pin:
> -	boarddata->cd_gpio = err;
> - not_supported:
> -	kfree(imx_data);
> - err_add_host:
> +err_add_host:
> +	if (gpio_is_valid(boarddata->cd_gpio))
> +		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
> +no_card_detect_irq:
> +	if (gpio_is_valid(boarddata->cd_gpio))
> +		gpio_free(boarddata->cd_gpio);
> +	if (gpio_is_valid(boarddata->wp_gpio))
> +		gpio_free(boarddata->wp_gpio);
> +no_card_detect_pin:
> +no_board_data:
>  	clk_disable(pltfm_host->clk);
>  	clk_put(pltfm_host->clk);
> - err_clk_get:
> +err_clk_get:
> +	kfree(imx_data);
>  	sdhci_pltfm_free(pdev);
>  	return err;
>  }
> @@ -343,14 +360,12 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev)
>  
>  	sdhci_remove_host(host, dead);
>  
> -	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
> +	if (gpio_is_valid(boarddata->wp_gpio))
>  		gpio_free(boarddata->wp_gpio);
>  
> -	if (boarddata && gpio_is_valid(boarddata->cd_gpio)) {
> +	if (gpio_is_valid(boarddata->cd_gpio)) {
> +		free_irq(gpio_to_irq(boarddata->cd_gpio), host);
>  		gpio_free(boarddata->cd_gpio);
> -
> -		if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION))
> -			free_irq(gpio_to_irq(boarddata->cd_gpio), host);
>  	}
>  
>  	clk_disable(pltfm_host->clk);
> -- 
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/92d43bf1/attachment-0001.sig>

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@freescale.com> writes:

Hi,

> On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
>> The card-present polling within sdhci based driver is very expensive
>> in terms of the impact to system performance.  We observe a few
>> system performance issues from Freescale and Linaro on mx5 platforms,
>> which have been proved card polling related.
>> 
>> The patch set extends the current sdhci-esdhc-imx card_detect and
>> write_protect support to cover mx5 platforms, and solves above
>> performance issues.
>> 
>> Shawn Guo (4):
>>       mmc: sdhci: fix interrupt storm from card detection
>>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
>>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
>>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
>> 
> Hi Arnaud,
>
> Any chance to play with it yet?

I tried applying the patch 4 (v2) on mmc git and Sascha Hauer's for-next
branch and failed. Can you please tell me on which tree should I apply
it ?

Thanks,
Arnaud

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 01:13:29PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@freescale.com> writes:
> 
> Hi,
> 
> > On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
> >> The card-present polling within sdhci based driver is very expensive
> >> in terms of the impact to system performance.  We observe a few
> >> system performance issues from Freescale and Linaro on mx5 platforms,
> >> which have been proved card polling related.
> >> 
> >> The patch set extends the current sdhci-esdhc-imx card_detect and
> >> write_protect support to cover mx5 platforms, and solves above
> >> performance issues.
> >> 
> >> Shawn Guo (4):
> >>       mmc: sdhci: fix interrupt storm from card detection
> >>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
> >>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
> >>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
> >> 
> > Hi Arnaud,
> >
> > Any chance to play with it yet?
> 
> I tried applying the patch 4 (v2) on mmc git and Sascha Hauer's for-next
> branch and failed. Can you please tell me on which tree should I apply
> it ?
> 
Sascha really should not picked up the following 3 patches.  They are
nonsense when driver is not even ready for the support.  These 3
patches have conflict with my patch set.  You should be able to
apply with these 3 removed.

commit 954548c68c2d478bf8ec0ca9d9eea2a7c45a4fcd
Author: Andre Silva <andre.silva@freescale.com>

    ARM:mach-mx5/board-mx51_babbage: Add CD and WP GPIOs

commit ba0c5064cb55aaac4cb5202ecad5afe9385139fc
Author: Andre Silva <andre.silva@freescale.com>

    ARM:mach-mx5/board-mx53_loco: Add CD and WP GPIOs

commit fabef7b7c62ba58604da22b6f79b7e9ad5ae8d74
Author: Andre Silva <andre.silva@freescale.com>

    ARM: mach-mx5/mx53_ard: Add support for i.MX53 ARD board

-- 
Regards,
Shawn

[PATCH 3/4] mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP

[Shawn Guo]

On Tue, Jun 14, 2011 at 11:28:13AM +0200, Wolfram Sang wrote:
> On Fri, Jun 10, 2011 at 06:42:51PM +0800, Shawn Guo wrote:
> > The use of flag ESDHC_FLAG_GPIO_FOR_CD_WP is all CD related.  It does
> > not necessarily need to bother WP in the flag name.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> 
> I'd just squash it into the next one?
> 
I'd like to have this patch to give the exact meaning of the flag,
and then the following patch removes the flag with proper CD only
case handling.

-- 
Regards,
Shawn

[PATCH 1/4] mmc: sdhci: fix interrupt storm from card detection

[Shawn Guo]

On Tue, Jun 14, 2011 at 11:24:10AM +0200, Wolfram Sang wrote:
> On Fri, Jun 10, 2011 at 06:42:49PM +0800, Shawn Guo wrote:
> > The issue was initially found by Eric Benard as below.
> > 
> > http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031
> > 
> > Not sure about other SDHCI based controller, but on Freescale eSDHC,
> > the SDHCI_INT_CARD_INSERT bits will be immediately set again when it
> > gets cleared, if a card is inserted. The driver need to mask the irq
> > to prevent interrupt storm which will freeze the system.  And the
> > SDHCI_INT_CARD_REMOVE gets the same situation.
> > 
> > The patch fixes the problem based on the initial idea from
> > Eric Benard.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Eric Benard <eric@eukrea.com>
> 
> Hmm, that should get enough testing on non-imx (and even non-ARM)
> devices. And a comment describing the situation.
> 
Agreed.  Will add something in commit message to mention the
situation.  That's why I hope we can get the patch on mmc-next at
early stage of the cycle, so everyone can start testing it and
report problems, if any.

-- 
Regards,
Shawn

[PATCH 1/4] mmc: sdhci: fix interrupt storm from card detection

[Wolfram Sang]

> > > The issue was initially found by Eric Benard as below.
> > > 
> > > http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031
> > > 
> > > Not sure about other SDHCI based controller, but on Freescale eSDHC,
> > > the SDHCI_INT_CARD_INSERT bits will be immediately set again when it
> > > gets cleared, if a card is inserted. The driver need to mask the irq
> > > to prevent interrupt storm which will freeze the system.  And the
> > > SDHCI_INT_CARD_REMOVE gets the same situation.
> > > 
> > > The patch fixes the problem based on the initial idea from
> > > Eric Benard.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Eric Benard <eric@eukrea.com>
> > 
> > Hmm, that should get enough testing on non-imx (and even non-ARM)
> > devices. And a comment describing the situation.
> > 
> Agreed.  Will add something in commit message to mention the
> situation.  That's why I hope we can get the patch on mmc-next at

I actually meant a comment in the code, so it will be obvious for later
hackers why the "extra" steps are in there...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/3e3c9a65/attachment.sig>

[PATCH 1/4] mmc: sdhci: fix interrupt storm from card detection

[Shawn Guo]

On Tue, Jun 14, 2011 at 02:02:58PM +0200, Wolfram Sang wrote:
> 
> > > > The issue was initially found by Eric Benard as below.
> > > > 
> > > > http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031
> > > > 
> > > > Not sure about other SDHCI based controller, but on Freescale eSDHC,
> > > > the SDHCI_INT_CARD_INSERT bits will be immediately set again when it
> > > > gets cleared, if a card is inserted. The driver need to mask the irq
> > > > to prevent interrupt storm which will freeze the system.  And the
> > > > SDHCI_INT_CARD_REMOVE gets the same situation.
> > > > 
> > > > The patch fixes the problem based on the initial idea from
> > > > Eric Benard.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Eric Benard <eric@eukrea.com>
> > > 
> > > Hmm, that should get enough testing on non-imx (and even non-ARM)
> > > devices. And a comment describing the situation.
> > > 
> > Agreed.  Will add something in commit message to mention the
> > situation.  That's why I hope we can get the patch on mmc-next at
> 
> I actually meant a comment in the code, so it will be obvious for later
> hackers why the "extra" steps are in there...
> 
OK.

-- 
Regards,
Shawn

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 11:51:29AM +0200, Wolfram Sang wrote:
> On Tue, Jun 14, 2011 at 02:47:39PM +0800, Shawn Guo wrote:
> > The patch extends card_detect and write_protect support to get mx5
> > family and more scenarios supported.  The changes include:
> > 
> >  * Turn platform_data from optional to mandatory
> >  * Add cd_types and wp_types into platform_data to cover more use
> >    cases
> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
> >  * Adjust some machine codes to adopt the platform_data changes
> > 
> > With this patch, card_detect and write_protect gets supported on
> > mx51_babbage, and other mx5 machines can easily get there with the
> > least board level configuration added.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> 
> No time for a full review, but it looks a lot like going into the right

The lack of CD support is causing critical system performance issue.
There is monthly Linaro release coming shortly, and this is one
important issue needs to be fixed in the release.  Please find some
time to do a full review, if possible.  We need your ack to know if
the patch stands :)  

> direction. Thanks.
> 
> A few comments:
> 
> > ---
> > Changes since v1:
> >  * Took the suggestion from Arnaud Patard to add default pdata
> >    in imx_add_sdhci_esdhc_imx(), to avoid touching every single
> >    board file for the platform_data changes
> > 
> >  arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c     |    3 +-
> >  arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c     |    3 +-
> >  arch/arm/mach-imx/mach-mx25_3ds.c                  |    2 +
> >  arch/arm/mach-imx/mach-pcm043.c                    |    2 +
> >  arch/arm/mach-mx5/board-mx51_babbage.c             |   27 +++++-
> >  .../plat-mxc/devices/platform-sdhci-esdhc-imx.c    |   12 ++
> >  arch/arm/plat-mxc/include/mach/esdhc.h             |   25 ++++-
> >  drivers/mmc/host/sdhci-esdhc-imx.c                 |  115 +++++++++++---------
> >  8 files changed, 130 insertions(+), 59 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> > index 01ebcb3..66e8726 100644
> > --- a/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> > +++ b/arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c
> > @@ -225,7 +225,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
> >  
> >  static struct esdhc_platform_data sd1_pdata = {
> >  	.cd_gpio = GPIO_SD1CD,
> > -	.wp_gpio = -EINVAL,
> > +	.cd_type = ESDHC_CD_GPIO,
> > +	.wp_type = ESDHC_WP_NONE,
> >  };
> >  
> >  /*
> > diff --git a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> > index 558eb52..0f0af02 100644
> > --- a/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> > +++ b/arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c
> > @@ -236,7 +236,8 @@ struct imx_ssi_platform_data eukrea_mbimxsd_ssi_pdata __initconst = {
> >  
> >  static struct esdhc_platform_data sd1_pdata = {
> >  	.cd_gpio = GPIO_SD1CD,
> > -	.wp_gpio = -EINVAL,
> > +	.cd_type = ESDHC_CD_GPIO,
> > +	.wp_type = ESDHC_WP_NONE,
> >  };
> >  
> >  /*
> > diff --git a/arch/arm/mach-imx/mach-mx25_3ds.c b/arch/arm/mach-imx/mach-mx25_3ds.c
> > index 01534bb..7f66a91 100644
> > --- a/arch/arm/mach-imx/mach-mx25_3ds.c
> > +++ b/arch/arm/mach-imx/mach-mx25_3ds.c
> > @@ -215,6 +215,8 @@ static const struct imxi2c_platform_data mx25_3ds_i2c0_data __initconst = {
> >  static const struct esdhc_platform_data mx25pdk_esdhc_pdata __initconst = {
> >  	.wp_gpio = SD1_GPIO_WP,
> >  	.cd_gpio = SD1_GPIO_CD,
> > +	.wp_type = ESDHC_WP_GPIO,
> > +	.cd_type = ESDHC_CD_GPIO,
> >  };
> >  
> >  static void __init mx25pdk_init(void)
> > diff --git a/arch/arm/mach-imx/mach-pcm043.c b/arch/arm/mach-imx/mach-pcm043.c
> > index 163cc31..660ec3e 100644
> > --- a/arch/arm/mach-imx/mach-pcm043.c
> > +++ b/arch/arm/mach-imx/mach-pcm043.c
> > @@ -349,6 +349,8 @@ __setup("otg_mode=", pcm043_otg_mode);
> >  static struct esdhc_platform_data sd1_pdata = {
> >  	.wp_gpio = SD1_GPIO_WP,
> >  	.cd_gpio = SD1_GPIO_CD,
> > +	.wp_type = ESDHC_WP_GPIO,
> > +	.cd_type = ESDHC_CD_GPIO,
> >  };
> >  
> >  /*
> > diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
> > index e54e4bf..4db6cf9 100644
> > --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> > +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> > @@ -34,6 +34,8 @@
> >  #include "devices.h"
> >  #include "cpu_op-mx51.h"
> >  
> > +#define BABBAGE_ESDHC2_WP	IMX_GPIO_NR(1, 5)
> > +#define BABBAGE_ESDHC2_CD	IMX_GPIO_NR(1, 6)
> >  #define BABBAGE_USB_HUB_RESET	IMX_GPIO_NR(1, 7)
> >  #define BABBAGE_USBH1_STP	IMX_GPIO_NR(1, 27)
> >  #define BABBAGE_USB_PHY_RESET	IMX_GPIO_NR(2, 5)
> > @@ -142,6 +144,10 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
> >  	MX51_PAD_SD1_DATA1__SD1_DATA1,
> >  	MX51_PAD_SD1_DATA2__SD1_DATA2,
> >  	MX51_PAD_SD1_DATA3__SD1_DATA3,
> > +	/* CD signal */
> > +	MX51_PAD_GPIO1_0__SD1_CD,
> > +	/* WP signal */
> > +	MX51_PAD_GPIO1_1__SD1_WP,
> >  
> >  	/* SD 2 */
> >  	MX51_PAD_SD2_CMD__SD2_CMD,
> > @@ -150,6 +156,11 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
> >  	MX51_PAD_SD2_DATA1__SD2_DATA1,
> >  	MX51_PAD_SD2_DATA2__SD2_DATA2,
> >  	MX51_PAD_SD2_DATA3__SD2_DATA3,
> > +	/* WP gpio */
> > +	MX51_PAD_GPIO1_5__GPIO1_5,
> > +	/* CD gpio */
> > +	MX51_PAD_GPIO1_6__GPIO1_6,
> > +
> >  
> >  	/* eCSPI1 */
> >  	MX51_PAD_CSPI1_MISO__ECSPI1_MISO,
> > @@ -331,6 +342,18 @@ static const struct spi_imx_master mx51_babbage_spi_pdata __initconst = {
> >  	.num_chipselect = ARRAY_SIZE(mx51_babbage_spi_cs),
> >  };
> >  
> > +static const struct esdhc_platform_data esdhc1_pdata __initconst = {
> > +	.wp_type = ESDHC_WP_SIGNAL,
> > +	.cd_type = ESDHC_CD_SIGNAL,
> > +};
> > +
> > +static const struct esdhc_platform_data esdhc2_pdata __initconst = {
> > +	.wp_gpio = BABBAGE_ESDHC2_WP,
> > +	.cd_gpio = BABBAGE_ESDHC2_CD,
> > +	.wp_type = ESDHC_WP_GPIO,
> > +	.cd_type = ESDHC_CD_GPIO,
> > +};
> > +
> >  /*
> >   * Board specific initialization.
> >   */
> > @@ -376,8 +399,8 @@ static void __init mx51_babbage_init(void)
> >  	mxc_iomux_v3_setup_pad(usbh1stp);
> >  	babbage_usbhub_reset();
> >  
> > -	imx51_add_sdhci_esdhc_imx(0, NULL);
> > -	imx51_add_sdhci_esdhc_imx(1, NULL);
> > +	imx51_add_sdhci_esdhc_imx(0, &esdhc1_pdata);
> > +	imx51_add_sdhci_esdhc_imx(1, &esdhc2_pdata);
> >  
> >  	spi_register_board_info(mx51_babbage_spi_board_info,
> >  		ARRAY_SIZE(mx51_babbage_spi_board_info));
> > diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> > index 6b2940b..a880f73 100644
> > --- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> > +++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
> > @@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
> >  };
> >  #endif /* ifdef CONFIG_SOC_IMX53 */
> >  
> > +static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
> > +	.wp_type = ESDHC_WP_NONE,
> > +	.cd_type = ESDHC_CD_NONE,
> > +};
> > +
> >  struct platform_device *__init imx_add_sdhci_esdhc_imx(
> >  		const struct imx_sdhci_esdhc_imx_data *data,
> >  		const struct esdhc_platform_data *pdata)
> > @@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
> >  		},
> >  	};
> >  
> > +	/*
> > +	 * If machine does not provide a pdata, use the default one
> 
> s/a pdata/pdata/ (not 100% sure, though)
> 
OK

> > +	 * which means none WP/CD support
> 
> s/none/no/
> 
OK

> > +	 */
> > +	if (!pdata)
> > +		pdata = &default_esdhc_pdata;
> > +
> >  	return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
> >  			ARRAY_SIZE(res), pdata, sizeof(*pdata));
> >  }
> > diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
> > index 86003f4..fced348 100644
> > --- a/arch/arm/plat-mxc/include/mach/esdhc.h
> > +++ b/arch/arm/plat-mxc/include/mach/esdhc.h
> > @@ -10,17 +10,32 @@
> >  #ifndef __ASM_ARCH_IMX_ESDHC_H
> >  #define __ASM_ARCH_IMX_ESDHC_H
> >  
> > +enum wp_types {
> > +	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
> > +	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */
> 
> I think SIGNAL is not descriptive enough. Maybe
> 
We have the document telling it's internal.

> ESDHC_WP_INTERNAL		/* WP routed directly to mmc controller */
> 
I thought about that.  If we use ESDHC_WP_INTERNAL, ESDHC_WP_GPIO
should probably be the ESDHC_WP_EXTERNAL for the couple naming.  But
I like ESDHC_WP_GPIO over ESDHC_WP_EXTERNAL, so chose the couple
naming of ESDHC_WP_GPIO vs. ESDHC_WP_SIGNAL.

> ? It should be mentioned on which SoCs this is not available?
> 

>From bogus@does.not.exist.com  Wed Jun  1 12:03:18 2011
From: bogus@does.not.exist.com ()
Date: Wed, 01 Jun 2011 16:03:18 -0000
Subject: No subject
Message-ID: <mailman.25.1308056456.24103.linux-arm-kernel@lists.infradead.org>

design choice to connect card CD/WP to the pads or not, which the
controller CD/WP signal/function is available on.

> > +	ESDHC_WP_GPIO,		/* external gpio pin for WP */
> > +};
> > +
> > +enum cd_types {
> > +	ESDHC_CD_NONE,		/* no CD, neither signal nor gpio */
> > +	ESDHC_CD_SIGNAL,	/* mmc internal CD signal */
> 
> ditto
> 
ditto

> > +	ESDHC_CD_GPIO,		/* external gpio pin for CD */
> > +	ESDHC_CD_PERMANENT,	/* no CD, card permanently wired to host */
> > +};
> > +
> >  /**
> > - * struct esdhc_platform_data - optional platform data for esdhc on i.MX
> > - *
> > - * strongly recommended for i.MX25/35, not needed for other variants
> > + * struct esdhc_platform_data - platform data for esdhc on i.MX
> >   *
> > - * @wp_gpio:	gpio for write_protect (-EINVAL if unused)
> > - * @cd_gpio:	gpio for card_detect interrupt (-EINVAL if unused)
> > + * @wp_gpio:	gpio for write_protect
> > + * @cd_gpio:	gpio for card_detect interrupt
> > + * @wp_type:	type of write_protect method (see wp_types enum above)
> > + * @cd_type:	type of card_detect method (see cd_types enum above)
> >   */
> >  
> >  struct esdhc_platform_data {
> >  	unsigned int wp_gpio;
> >  	unsigned int cd_gpio;
> > +	enum wp_types wp_type;
> > +	enum cd_types cd_type;
> >  };
> >  #endif /* __ASM_ARCH_IMX_ESDHC_H */
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 79b7a9a..9cba6eb 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -29,7 +29,6 @@
> >  #define SDHCI_VENDOR_SPEC		0xC0
> >  #define  SDHCI_VENDOR_SPEC_SDIO_QUIRK	0x00000002
> >  
> > -#define ESDHC_FLAG_GPIO_FOR_CD		(1 << 0)
> >  /*
> >   * The CMDTYPE of the CMD register (offset 0xE) should be set to
> >   * "11" when the STOP CMD12 is issued on imx53 to abort one
> > @@ -70,19 +69,15 @@ static inline void esdhc_clrset_le(struct sdhci_host *host, u32 mask, u32 val, i
> >  
> >  static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
> >  {
> > -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> > +	struct esdhc_platform_data *boarddata =
> > +			host->mmc->parent->platform_data;
> >  
> > -	/* fake CARD_PRESENT flag on mx25/35 */
> > +	/* fake CARD_PRESENT flag */
> >  	u32 val = readl(host->ioaddr + reg);
> >  
> >  	if (unlikely((reg == SDHCI_PRESENT_STATE)
> > -			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD))) {
> > -		struct esdhc_platform_data *boarddata =
> > -				host->mmc->parent->platform_data;
> > -
> > -		if (boarddata && gpio_is_valid(boarddata->cd_gpio)
> > -				&& gpio_get_value(boarddata->cd_gpio))
> > +			&& gpio_is_valid(boarddata->cd_gpio))) {
> > +		if (gpio_get_value(boarddata->cd_gpio))
> >  			/* no card, if a valid gpio says so... */
> >  			val &= ~SDHCI_CARD_PRESENT;
> >  		else
> > @@ -97,12 +92,13 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
> >  {
> >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> > +	struct esdhc_platform_data *boarddata =
> > +			host->mmc->parent->platform_data;
> >  
> >  	if (unlikely((reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)
> > -			&& (imx_data->flags & ESDHC_FLAG_GPIO_FOR_CD)))
> > +			&& (boarddata->cd_type == ESDHC_CD_GPIO)))
> >  		/*
> >  		 * these interrupts won't work with a custom card_detect gpio
> > -		 * (only applied to mx25/35)
> >  		 */
> >  		val &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
> >  
> > @@ -201,6 +197,18 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host)
> >  	return clk_get_rate(pltfm_host->clk) / 256 / 16;
> >  }
> >  
> > +static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
> > +{
> > +	struct esdhc_platform_data *boarddata =
> > +			host->mmc->parent->platform_data;
> > +
> > +	if (gpio_is_valid(boarddata->wp_gpio))
> > +		return gpio_get_value(boarddata->wp_gpio);
> > +	else
> > +		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
> > +				SDHCI_WRITE_PROTECT);
> > +}
> 
> Aren't you missing the NONE case here? Plus, I don't like having a
> get_ro-function for the SIGNAL case, because in that case it is
> superfluous. Though, I am not feeling strong about this if it makes the
> rest of the code messier.
> 
OK, I will go back to the existing one, which only handles gpio case
in esdhc_pltfm_get_ro and get it assigned to .get_ro only in case of
ESDHC_WP_GPIO.

> > +
> >  static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.read_l = esdhc_readl_le,
> >  	.read_w = esdhc_readw_le,
> > @@ -212,6 +220,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.get_min_clock = esdhc_pltfm_get_min_clock,
> >  	.get_max_blk_size = esdhc_pltfm_get_max_blk_size,
> >  	.get_max_blk_count = esdhc_pltfm_get_max_blk_count,
> > +	.get_ro = esdhc_pltfm_get_ro,
> >  };
> >  
> >  static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> > @@ -221,17 +230,6 @@ static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> >  	.ops = &sdhci_esdhc_ops,
> >  };
> >  
> > -static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
> > -{
> > -	struct esdhc_platform_data *boarddata =
> > -			host->mmc->parent->platform_data;
> > -
> > -	if (boarddata && gpio_is_valid(boarddata->wp_gpio))
> > -		return gpio_get_value(boarddata->wp_gpio);
> > -	else
> > -		return -ENOSYS;
> > -}
> > -
> >  static irqreturn_t cd_irq(int irq, void *data)
> >  {
> >  	struct sdhci_host *sdhost = (struct sdhci_host *)data;
> > @@ -272,23 +270,33 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
> >  	if (!cpu_is_mx25())
> >  		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
> >  
> > -	if (cpu_is_mx25() || cpu_is_mx35()) {
> > -		/* write_protect can't be routed to controller, use gpio */
> > -		sdhci_esdhc_ops.get_ro = esdhc_pltfm_get_ro;
> > -	}
> > -
> >  	if (!(cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51()))
> >  		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
> >  
> >  	boarddata = host->mmc->parent->platform_data;
> > -	if (boarddata) {
> > +	if (!boarddata) {
> > +		dev_err(mmc_dev(host->mmc), "no board data!\n");
> > +		err = -EINVAL;
> > +		goto no_board_data;
> > +	}
> > +
> > +	/* write_protect */
> > +	if (boarddata->wp_type == ESDHC_WP_GPIO) {
> >  		err = gpio_request_one(boarddata->wp_gpio, GPIOF_IN, "ESDHC_WP");
> >  		if (err) {
> >  			dev_warn(mmc_dev(host->mmc),
> > -				"no write-protect pin available!\n");
> > -			boarddata->wp_gpio = err;
> > +				 "no write-protect pin available!\n");
> > +			boarddata->wp_gpio = -EINVAL;
> >  		}
> > +	} else
> > +		boarddata->wp_gpio = -EINVAL;
> 
> else-block needs braces
> 
OK

-- 
Regards,
Shawn

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 07:39:03PM +0800, Shawn Guo wrote:
> On Tue, Jun 14, 2011 at 01:13:29PM +0200, Arnaud Patard wrote:
> > Shawn Guo <shawn.guo@freescale.com> writes:
> > 
> > Hi,
> > 
> > > On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
> > >> The card-present polling within sdhci based driver is very expensive
> > >> in terms of the impact to system performance.  We observe a few
> > >> system performance issues from Freescale and Linaro on mx5 platforms,
> > >> which have been proved card polling related.
> > >> 
> > >> The patch set extends the current sdhci-esdhc-imx card_detect and
> > >> write_protect support to cover mx5 platforms, and solves above
> > >> performance issues.
> > >> 
> > >> Shawn Guo (4):
> > >>       mmc: sdhci: fix interrupt storm from card detection
> > >>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
> > >>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
> > >>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
> > >> 
> > > Hi Arnaud,
> > >
> > > Any chance to play with it yet?
> > 
> > I tried applying the patch 4 (v2) on mmc git and Sascha Hauer's for-next
> > branch and failed. Can you please tell me on which tree should I apply
> > it ?
> > 
> Sascha really should not picked up the following 3 patches.  They are
> nonsense when driver is not even ready for the support.  These 3
> patches have conflict with my patch set.  You should be able to
> apply with these 3 removed.
> 
> commit 954548c68c2d478bf8ec0ca9d9eea2a7c45a4fcd
> Author: Andre Silva <andre.silva@freescale.com>
> 
>     ARM:mach-mx5/board-mx51_babbage: Add CD and WP GPIOs
> 
> commit ba0c5064cb55aaac4cb5202ecad5afe9385139fc
> Author: Andre Silva <andre.silva@freescale.com>
> 
>     ARM:mach-mx5/board-mx53_loco: Add CD and WP GPIOs
> 
> commit fabef7b7c62ba58604da22b6f79b7e9ad5ae8d74
> Author: Andre Silva <andre.silva@freescale.com>
> 
>     ARM: mach-mx5/mx53_ard: Add support for i.MX53 ARD board
> 
Sascha,

Since these 3 patches are still sitting on top of your tree, would
you mind backing out them?  Or you want me to do a rebase on your
tree?  I'm fine with either one.

-- 
Regards,
Shawn

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Wolfram Sang]

> important issue needs to be fixed in the release.  Please find some
> time to do a full review, if possible.  We need your ack to know if
> the patch stands :)  

Huh, my ack is needed for the linaro-kernel? :) Isn't it possible to
replace a V1 patch with a V2 patch in later kernels?

> > > diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
> > > index 86003f4..fced348 100644
> > > --- a/arch/arm/plat-mxc/include/mach/esdhc.h
> > > +++ b/arch/arm/plat-mxc/include/mach/esdhc.h
> > > @@ -10,17 +10,32 @@
> > >  #ifndef __ASM_ARCH_IMX_ESDHC_H
> > >  #define __ASM_ARCH_IMX_ESDHC_H
> > >  
> > > +enum wp_types {
> > > +	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
> > > +	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */
> > 
> > I think SIGNAL is not descriptive enough. Maybe
> > 
> We have the document telling it's internal.
> 
> > ESDHC_WP_INTERNAL		/* WP routed directly to mmc controller */
> > 
> I thought about that.  If we use ESDHC_WP_INTERNAL, ESDHC_WP_GPIO
> should probably be the ESDHC_WP_EXTERNAL for the couple naming.  But
> I like ESDHC_WP_GPIO over ESDHC_WP_EXTERNAL, so chose the couple
> naming of ESDHC_WP_GPIO vs. ESDHC_WP_SIGNAL.

ESDHC_WP_CONTROLLER? Or ESDHC_WP_CORE?

> > ? It should be mentioned on which SoCs this is not available?
> > 
> From SoC/controller POV, it's all available.  It's really a board
> design choice to connect card CD/WP to the pads or not, which the
> controller CD/WP signal/function is available on.

For mx25/35, I tried to find muxer settings but couldn't find them. It
seemed to me, GPIO is the only option there (I think I even asked
Richard). Can you route them on mx25/35, too?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110614/08c5168d/attachment.sig>

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@freescale.com> writes:

> On Tue, Jun 14, 2011 at 01:13:29PM +0200, Arnaud Patard wrote:
>> Shawn Guo <shawn.guo@freescale.com> writes:
>> 
>> Hi,
>> 
>> > On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
>> >> The card-present polling within sdhci based driver is very expensive
>> >> in terms of the impact to system performance.  We observe a few
>> >> system performance issues from Freescale and Linaro on mx5 platforms,
>> >> which have been proved card polling related.
>> >> 
>> >> The patch set extends the current sdhci-esdhc-imx card_detect and
>> >> write_protect support to cover mx5 platforms, and solves above
>> >> performance issues.
>> >> 
>> >> Shawn Guo (4):
>> >>       mmc: sdhci: fix interrupt storm from card detection
>> >>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
>> >>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
>> >>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
>> >> 
>> > Hi Arnaud,
>> >
>> > Any chance to play with it yet?
>> 
>> I tried applying the patch 4 (v2) on mmc git and Sascha Hauer's for-next
>> branch and failed. Can you please tell me on which tree should I apply
>> it ?
>> 
> Sascha really should not picked up the following 3 patches.  They are
> nonsense when driver is not even ready for the support.  These 3
> patches have conflict with my patch set.  You should be able to
> apply with these 3 removed.

They're still not applying. It's failing to failing
sdhci-esdhc-imx.c. I'm currently trying t make sure I didn't miss
something but it looks like the patch is not against for-next. For
instance, look at :
http://git.pengutronix.de/?p=imx/linux-2.6.git;a=blob;f=drivers/mmc/host/sdhci-esdhc-imx.c;h=a19967d0bfc48b0ce8216de0d1a727093ee9fa03;hb=refs/heads/for-next

It's already adding a esdhc_pltfm_get_ro() function, so I fail to see
how a patch adding a function with same name can work.

Thanks,
Arnaud

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 03:55:20PM +0200, Wolfram Sang wrote:
> 
> > important issue needs to be fixed in the release.  Please find some
> > time to do a full review, if possible.  We need your ack to know if
> > the patch stands :)  
> 
> Huh, my ack is needed for the linaro-kernel? :) Isn't it possible to
> replace a V1 patch with a V2 patch in later kernels?
> 
The linaro-kernel tree only accepts patches that have been on upstream
or are on the way to upstream (sitting on 'next' is a good proving).
Also since we are touching sdhci.c a little bit, it should be good for
us to get the patch on 'next' as early as we can, so that people can
start testing it.

Anyway, I will post v2 shortly.  Please help do a full review if
possible.  I'd appreciate it.

> > > > diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
> > > > index 86003f4..fced348 100644
> > > > --- a/arch/arm/plat-mxc/include/mach/esdhc.h
> > > > +++ b/arch/arm/plat-mxc/include/mach/esdhc.h
> > > > @@ -10,17 +10,32 @@
> > > >  #ifndef __ASM_ARCH_IMX_ESDHC_H
> > > >  #define __ASM_ARCH_IMX_ESDHC_H
> > > >  
> > > > +enum wp_types {
> > > > +	ESDHC_WP_NONE,		/* no WP, neither signal nor gpio */
> > > > +	ESDHC_WP_SIGNAL,	/* mmc internal WP signal */
> > > 
> > > I think SIGNAL is not descriptive enough. Maybe
> > > 
> > We have the document telling it's internal.
> > 
> > > ESDHC_WP_INTERNAL		/* WP routed directly to mmc controller */
> > > 
> > I thought about that.  If we use ESDHC_WP_INTERNAL, ESDHC_WP_GPIO
> > should probably be the ESDHC_WP_EXTERNAL for the couple naming.  But
> > I like ESDHC_WP_GPIO over ESDHC_WP_EXTERNAL, so chose the couple
> > naming of ESDHC_WP_GPIO vs. ESDHC_WP_SIGNAL.
> 
> ESDHC_WP_CONTROLLER? Or ESDHC_WP_CORE?
> 
Ok, going ESDHC_WP_CONTROLLER

> > > ? It should be mentioned on which SoCs this is not available?
> > > 
> > From SoC/controller POV, it's all available.  It's really a board
> > design choice to connect card CD/WP to the pads or not, which the
> > controller CD/WP signal/function is available on.
> 
> For mx25/35, I tried to find muxer settings but couldn't find them. It
> seemed to me, GPIO is the only option there (I think I even asked
> Richard). Can you route them on mx25/35, too?
> 
Sorry for confusion here.  It's all available from eSDHC controller
POV.  But no, it's not available on mx25/35 from SoC pinmux POV.  So
you are right.

-- 
Regards,
Shawn

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 04:29:57PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@freescale.com> writes:
> 
> > On Tue, Jun 14, 2011 at 01:13:29PM +0200, Arnaud Patard wrote:
> >> Shawn Guo <shawn.guo@freescale.com> writes:
> >> 
> >> Hi,
> >> 
> >> > On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
> >> >> The card-present polling within sdhci based driver is very expensive
> >> >> in terms of the impact to system performance.  We observe a few
> >> >> system performance issues from Freescale and Linaro on mx5 platforms,
> >> >> which have been proved card polling related.
> >> >> 
> >> >> The patch set extends the current sdhci-esdhc-imx card_detect and
> >> >> write_protect support to cover mx5 platforms, and solves above
> >> >> performance issues.
> >> >> 
> >> >> Shawn Guo (4):
> >> >>       mmc: sdhci: fix interrupt storm from card detection
> >> >>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
> >> >>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
> >> >>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
> >> >> 
> >> > Hi Arnaud,
> >> >
> >> > Any chance to play with it yet?
> >> 
> >> I tried applying the patch 4 (v2) on mmc git and Sascha Hauer's for-next
> >> branch and failed. Can you please tell me on which tree should I apply
> >> it ?
> >> 
> > Sascha really should not picked up the following 3 patches.  They are
> > nonsense when driver is not even ready for the support.  These 3
> > patches have conflict with my patch set.  You should be able to
> > apply with these 3 removed.
> 
> They're still not applying. It's failing to failing
> sdhci-esdhc-imx.c. I'm currently trying t make sure I didn't miss
> something but it looks like the patch is not against for-next. For
> instance, look at :
> http://git.pengutronix.de/?p=imx/linux-2.6.git;a=blob;f=drivers/mmc/host/sdhci-esdhc-imx.c;h=a19967d0bfc48b0ce8216de0d1a727093ee9fa03;hb=refs/heads/for-next
> 
> It's already adding a esdhc_pltfm_get_ro() function, so I fail to see
> how a patch adding a function with same name can work.
> 
My patch did not add esdhc_pltfm_get_ro() function but moved it a
little bit.  Anyway, I will go ahead to rebase the patch set on
the latest 'next' and send it as v2 shortly, since we have not heard
anything from Sascha.

-- 
Regards,
Shawn

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Shawn Guo]

On Tue, Jun 14, 2011 at 09:06:11PM +0800, Shawn Guo wrote:
[...]
> > >  
> > > +static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
> > > +{
> > > +	struct esdhc_platform_data *boarddata =
> > > +			host->mmc->parent->platform_data;
> > > +
> > > +	if (gpio_is_valid(boarddata->wp_gpio))
> > > +		return gpio_get_value(boarddata->wp_gpio);
> > > +	else
> > > +		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
> > > +				SDHCI_WRITE_PROTECT);
> > > +}
> > 
> > Aren't you missing the NONE case here? Plus, I don't like having a
> > get_ro-function for the SIGNAL case, because in that case it is
> > superfluous. Though, I am not feeling strong about this if it makes the
> > rest of the code messier.
> > 
> OK, I will go back to the existing one, which only handles gpio case
> in esdhc_pltfm_get_ro and get it assigned to .get_ro only in case of
> ESDHC_WP_GPIO.
> 
Sorry.  I had some reason for moving esdhc_pltfm_get_ro around.  We
can not keep the existing approach (what I said above).  For mx51
babbage example, esdhc1 uses internal WP while esdhc2 uses gpio WP.
If we have esdhc_pltfm_get_ro handle gpio only and assign it to
sdhci_esdhc_ops.get_ro in .probe only when wp_type is ESDHC_WP_GPIO,
that works for esdhc2 but breaks esdhc1 WP function.  So no, I will
not change my code except adding NONE case handling there.

-- 
Regards,
Shawn

[PATCH v2 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5

[Wolfram Sang]

> Sorry.  I had some reason for moving esdhc_pltfm_get_ro around.  We
> can not keep the existing approach (what I said above).  For mx51
> babbage example, esdhc1 uses internal WP while esdhc2 uses gpio WP.
> If we have esdhc_pltfm_get_ro handle gpio only and assign it to
> sdhci_esdhc_ops.get_ro in .probe only when wp_type is ESDHC_WP_GPIO,
> that works for esdhc2 but breaks esdhc1 WP function.  So no, I will
> not change my code except adding NONE case handling there.

Ok, fine with me. Thanks for checking!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110615/d5cff7fc/attachment.sig>

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Arnaud Patard (Rtp)]

Shawn Guo <shawn.guo@freescale.com> writes:

Hi,

> On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
>> The card-present polling within sdhci based driver is very expensive
>> in terms of the impact to system performance.  We observe a few
>> system performance issues from Freescale and Linaro on mx5 platforms,
>> which have been proved card polling related.
>> 
>> The patch set extends the current sdhci-esdhc-imx card_detect and
>> write_protect support to cover mx5 platforms, and solves above
>> performance issues.
>> 
>> Shawn Guo (4):
>>       mmc: sdhci: fix interrupt storm from card detection
>>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
>>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
>>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
>> 
> Hi Arnaud,
>
> Any chance to play with it yet?

Finally managed to build a kernel with this version of the
patchset. While I'm not polling anymore, I'm getting a lot of interrupts
if the card is not inserted. Theses interrupts are not happening if the
card is inserted. I can see things like this in the logs :
sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000080

[ of course, the bit of the present state register indicating card
presence is equal to 0 ]

I've tested the SIGNAL case only. Don't know if switch to GPIO may help.
Do you have same kind of issue on your side ?

Thanks,
Arnaud

[PATCH 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5

[Shawn Guo]

Hi Arnaud,

Sorry for the late replying.  Somehow I missed the message.

On Thu, Jun 16, 2011 at 08:32:33PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@freescale.com> writes:
> 
> Hi,
> 
> > On Fri, Jun 10, 2011 at 06:42:48PM +0800, Shawn Guo wrote:
> >> The card-present polling within sdhci based driver is very expensive
> >> in terms of the impact to system performance.  We observe a few
> >> system performance issues from Freescale and Linaro on mx5 platforms,
> >> which have been proved card polling related.
> >> 
> >> The patch set extends the current sdhci-esdhc-imx card_detect and
> >> write_protect support to cover mx5 platforms, and solves above
> >> performance issues.
> >> 
> >> Shawn Guo (4):
> >>       mmc: sdhci: fix interrupt storm from card detection
> >>       mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared
> >>       mmc: sdhci-esdhc-imx: remove "WP" from flag ESDHC_FLAG_GPIO_FOR_CD_WP
> >>       mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
> >> 
> > Hi Arnaud,
> >
> > Any chance to play with it yet?
> 
> Finally managed to build a kernel with this version of the
> patchset. While I'm not polling anymore, I'm getting a lot of interrupts
> if the card is not inserted. Theses interrupts are not happening if the
> card is inserted. I can see things like this in the logs :
> sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000080
> 
> [ of course, the bit of the present state register indicating card
> presence is equal to 0 ]
> 
> I've tested the SIGNAL case only. Don't know if switch to GPIO may help.
> Do you have same kind of issue on your side ?
> 
I reproduced the issue on my side with turn on debug switch (I should
do so from the beginning).

GPIO mode does not have this issue.  But you do not have to switch to
GPIO, as I have got it fixed in the v3 of the patch set (posted out
a few minutes ago).  It can apply on linux-next cleanly.  You need
to note that naming 'SIGNAL' has been changed to 'CONTROLLER' per
Wolfram's comment.

Thanks for testing.

-- 
Regards,
Shawn