[PATCH] mmc: sdhci / i.MX esdhc buswidth patches

[PATCH] mmc: sdhci / i.MX esdhc buswidth patches

[Sascha Hauer]

The first patch is a generic sdhci driver cleanup change, the others
are i.MX specific, adding 8bit bus support and fix version register
readout.

Sascha

----------------------------------------------------------------
Sascha Hauer (3):
      mmc: sdhci: rename platform_8bit_width to platform_bus_width
      mmc: esdhc i.MX: Fix version register read
      mmc: esdhc i.MX: Support 8bit mode

 .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |    1 +
 arch/arm/plat-mxc/include/mach/esdhc.h             |    1 +
 drivers/mmc/host/sdhci-esdhc-imx.c                 |   77 +++++++++++++++++---
 drivers/mmc/host/sdhci-pci.c                       |    4 +-
 drivers/mmc/host/sdhci-pxav2.c                     |    2 +-
 drivers/mmc/host/sdhci-s3c.c                       |    8 +-
 drivers/mmc/host/sdhci-tegra.c                     |    4 +-
 drivers/mmc/host/sdhci.c                           |    8 +-
 drivers/mmc/host/sdhci.h                           |    2 +-
 9 files changed, 82 insertions(+), 25 deletions(-)

[PATCH 1/3] mmc: sdhci: rename platform_8bit_width to platform_bus_width

[Sascha Hauer]

The 8bit in the function name is misleading. When set, it will be
used to set the bus width, regardless of whether 8bit or another
bus width is requested, so change the function name to
platform_bus_width.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mmc/host/sdhci-pci.c   |    4 ++--
 drivers/mmc/host/sdhci-pxav2.c |    2 +-
 drivers/mmc/host/sdhci-s3c.c   |    8 ++++----
 drivers/mmc/host/sdhci-tegra.c |    4 ++--
 drivers/mmc/host/sdhci.c       |    8 ++++----
 drivers/mmc/host/sdhci.h       |    2 +-
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 504da715..cf34b8d 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -934,7 +934,7 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
 	return 0;
 }
 
-static int sdhci_pci_8bit_width(struct sdhci_host *host, int width)
+static int sdhci_pci_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
 
@@ -976,7 +976,7 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
 
 static struct sdhci_ops sdhci_pci_ops = {
 	.enable_dma	= sdhci_pci_enable_dma,
-	.platform_8bit_width	= sdhci_pci_8bit_width,
+	.platform_bus_width	= sdhci_pci_bus_width,
 	.hw_reset		= sdhci_pci_hw_reset,
 };
 
diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index b6ee885..f380de8 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -121,7 +121,7 @@ static u32 pxav2_get_max_clock(struct sdhci_host *host)
 static struct sdhci_ops pxav2_sdhci_ops = {
 	.get_max_clock = pxav2_get_max_clock,
 	.platform_reset_exit = pxav2_set_private_registers,
-	.platform_8bit_width = pxav2_mmc_set_width,
+	.platform_bus_width = pxav2_mmc_set_width,
 };
 
 #ifdef CONFIG_OF
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index a50c205..776ef3a 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -322,14 +322,14 @@ static void sdhci_cmu_set_clock(struct sdhci_host *host, unsigned int clock)
 }
 
 /**
- * sdhci_s3c_platform_8bit_width - support 8bit buswidth
+ * sdhci_s3c_platform_bus_width - support 8bit buswidth
  * @host: The SDHCI host being queried
  * @width: MMC_BUS_WIDTH_ macro for the bus width being requested
  *
  * We have 8-bit width support but is not a v3 controller.
- * So we add platform_8bit_width() and support 8bit width.
+ * So we add platform_bus_width() and support 8bit width.
  */
-static int sdhci_s3c_platform_8bit_width(struct sdhci_host *host, int width)
+static int sdhci_s3c_platform_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
 
@@ -359,7 +359,7 @@ static struct sdhci_ops sdhci_s3c_ops = {
 	.get_max_clock		= sdhci_s3c_get_max_clk,
 	.set_clock		= sdhci_s3c_set_clock,
 	.get_min_clock		= sdhci_s3c_get_min_clock,
-	.platform_8bit_width	= sdhci_s3c_platform_8bit_width,
+	.platform_bus_width	= sdhci_s3c_platform_bus_width,
 };
 
 static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 0810ccc..1f51667 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -144,7 +144,7 @@ static void tegra_sdhci_reset_exit(struct sdhci_host *host, u8 mask)
 	}
 }
 
-static int tegra_sdhci_8bit(struct sdhci_host *host, int bus_width)
+static int tegra_sdhci_buswidth(struct sdhci_host *host, int bus_width)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = pltfm_host->priv;
@@ -171,7 +171,7 @@ static struct sdhci_ops tegra_sdhci_ops = {
 	.read_l     = tegra_sdhci_readl,
 	.read_w     = tegra_sdhci_readw,
 	.write_l    = tegra_sdhci_writel,
-	.platform_8bit_width = tegra_sdhci_8bit,
+	.platform_bus_width = tegra_sdhci_buswidth,
 	.platform_reset_exit = tegra_sdhci_reset_exit,
 };
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9a11dc3..9043a3c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1372,11 +1372,11 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 	/*
 	 * If your platform has 8-bit width support but is not a v3 controller,
 	 * or if it requires special setup code, you should implement that in
-	 * platform_8bit_width().
+	 * platform_bus_width().
 	 */
-	if (host->ops->platform_8bit_width)
-		host->ops->platform_8bit_width(host, ios->bus_width);
-	else {
+	if (host->ops->platform_bus_width) {
+		host->ops->platform_bus_width(host, ios->bus_width);
+	} else {
 		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 		if (ios->bus_width == MMC_BUS_WIDTH_8) {
 			ctrl &= ~SDHCI_CTRL_4BITBUS;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 97653ea..efce211 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -267,7 +267,7 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
-	int		(*platform_8bit_width)(struct sdhci_host *host,
+	int		(*platform_bus_width)(struct sdhci_host *host,
 					       int width);
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
 					     u8 power_mode);
-- 
1.7.10.4

[PATCH 2/3] mmc: esdhc i.MX: Fix version register read

[Sascha Hauer]

The i.MX ESDHC controller version register is a mess:

- i.MX25 has a v1 controller which identifies itself as v2
- i.MX6Q has a v3 controller which identifies itself as v4
- i.MX35,51,53 have v2 controllers which identify themselves correctly

Additionally on i.MX the register is located at offset 0xfc instead of
0xfe. The i.MX6 had a quirk around it which converted v4 into v3. Instead
of reading the real version register, all other SoCs used to return
the value from 0xfe which contains 0x0 for all SoCs, so all controllers
except i.MX6q were identified as version v1.

This patch fixes this by returning the version based on the devtype
data leaving the useless version register untouched.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index e23f813..4fe7312 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -237,15 +237,20 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
 
 static u16 esdhc_readw_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;
+
 	if (unlikely(reg == SDHCI_HOST_VERSION)) {
-		u16 val = readw(host->ioaddr + (reg ^ 2));
-		/*
-		 * uSDHC supports SDHCI v3.0, but it's encoded as value
-		 * 0x3 in host controller version register, which violates
-		 * SDHCI_SPEC_300 definition.  Work it around here.
-		 */
-		if ((val & SDHCI_SPEC_VER_MASK) == 3)
-			return --val;
+		switch (imx_data->devtype) {
+		case IMX25_ESDHC:
+			return SDHCI_SPEC_100;
+		case IMX35_ESDHC:
+		case IMX51_ESDHC:
+		case IMX53_ESDHC:
+			return SDHCI_SPEC_200;
+		case IMX6Q_USDHC:
+			return SDHCI_SPEC_300;
+		}
 	}
 
 	return readw(host->ioaddr + reg);
-- 
1.7.10.4

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Sascha Hauer]

The i.MX esdhc has a nonstandard bit layout for the SDHCI_HOST_CONTROL
register. To support 8bit bus width on i.MX populate the platform_bus_width
callback. This is tested on an i.MX25, but should according to the datasheets
work on the other i.MX using this hardware aswell. The i.MX6, while having
a SDHCI_SPEC_300 controller, still uses the same nonstandard register layout.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |    1 +
 arch/arm/plat-mxc/include/mach/esdhc.h             |    1 +
 drivers/mmc/host/sdhci-esdhc-imx.c                 |   56 ++++++++++++++++++--
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
index 1dd6225..c989994 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
@@ -12,6 +12,7 @@ Required properties:
 Optional properties:
 - fsl,cd-controller : Indicate to use controller internal card detection
 - fsl,wp-controller : Indicate to use controller internal write protection
+- bus-width : Maximum supported bus width. Defaults to 4 if omitted
 
 Examples:
 
diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
index aaf9748..b4a0521 100644
--- a/arch/arm/plat-mxc/include/mach/esdhc.h
+++ b/arch/arm/plat-mxc/include/mach/esdhc.h
@@ -39,5 +39,6 @@ struct esdhc_platform_data {
 	unsigned int cd_gpio;
 	enum wp_types wp_type;
 	enum cd_types cd_type;
+	int max_bus_width;
 };
 #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 4fe7312..b205fe9 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -57,6 +57,13 @@
  */
 #define ESDHC_FLAG_MULTIBLK_NO_INT	(1 << 1)
 
+/*
+ * Freescale interpretation of the SDHCI_HOST_CONTROL register
+ */
+#define FSL_SDHCI_CTRL_4BITBUS			(0x1 << 1)
+#define FSL_SDHCI_CTRL_8BITBUS			(0x2 << 1)
+#define FSL_SDHCI_CTRL_BUSWIDTH_MASK		(0x3 << 1)
+
 enum imx_esdhc_type {
 	IMX25_ESDHC,
 	IMX35_ESDHC,
@@ -307,6 +314,7 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 	u32 new_val;
+	u32 mask;
 
 	switch (reg) {
 	case SDHCI_POWER_CONTROL:
@@ -318,7 +326,6 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 	case SDHCI_HOST_CONTROL:
 		/* FSL messed up here, so we can just keep those three */
 		new_val = val & (SDHCI_CTRL_LED | \
-				SDHCI_CTRL_4BITBUS | \
 				SDHCI_CTRL_D3CD);
 		/* ensure the endianess */
 		new_val |= ESDHC_HOST_CONTROL_LE;
@@ -328,7 +335,13 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 			new_val |= (val & SDHCI_CTRL_DMA_MASK) << 5;
 		}
 
-		esdhc_clrset_le(host, 0xffff, new_val, reg);
+		/*
+		 * Do not touch buswidth bits here. This is done in
+		 * esdhc_pltfm_buswidth.
+		 */
+		mask = 0xffff & ~FSL_SDHCI_CTRL_BUSWIDTH_MASK;
+
+		esdhc_clrset_le(host, mask, new_val, reg);
 		return;
 	}
 	esdhc_clrset_le(host, 0xff, val, reg);
@@ -379,6 +392,27 @@ static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
 	return -ENOSYS;
 }
 
+static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
+{
+	u32 ctrl;
+
+	switch (width) {
+	case MMC_BUS_WIDTH_8:
+		ctrl = FSL_SDHCI_CTRL_8BITBUS;
+		break;
+	case MMC_BUS_WIDTH_4:
+		ctrl = FSL_SDHCI_CTRL_4BITBUS;
+		break;
+	default:
+		ctrl = 0;
+		break;
+	}
+
+	esdhc_clrset_le(host, FSL_SDHCI_CTRL_BUSWIDTH_MASK, ctrl, SDHCI_HOST_CONTROL);
+
+	return 0;
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -389,6 +423,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_max_clock = esdhc_pltfm_get_max_clock,
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_ro = esdhc_pltfm_get_ro,
+	.platform_bus_width = esdhc_pltfm_bus_width,
 };
 
 static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
@@ -434,6 +469,8 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 	if (gpio_is_valid(boarddata->wp_gpio))
 		boarddata->wp_type = ESDHC_WP_GPIO;
 
+	of_property_read_u32(np, "bus-width", &boarddata->max_bus_width);
+
 	return 0;
 }
 #else
@@ -507,7 +544,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
 		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
 		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
-			| SDHCI_QUIRK_BROKEN_ADMA;
+			 | SDHCI_QUIRK_BROKEN_ADMA;
 
 	if (is_imx53_esdhc(imx_data))
 		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
@@ -577,6 +614,19 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 		break;
 	}
 
+	switch (boarddata->max_bus_width) {
+	case 8:
+		host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
+		break;
+	case 4:
+	default:
+		host->mmc->caps |= MMC_CAP_4_BIT_DATA;
+		break;
+	case 1:
+		host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
+		break;
+	}
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto err_add_host;
-- 
1.7.10.4

[PATCH 1/3] mmc: sdhci: rename platform_8bit_width to platform_bus_width

[Jaehoon Chung]

On 09/24/2012 04:22 PM, Sascha Hauer wrote:
> The 8bit in the function name is misleading. When set, it will be
> used to set the bus width, regardless of whether 8bit or another
> bus width is requested, so change the function name to
> platform_bus_width.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mmc/host/sdhci-pci.c   |    4 ++--
>  drivers/mmc/host/sdhci-pxav2.c |    2 +-
>  drivers/mmc/host/sdhci-s3c.c   |    8 ++++----
>  drivers/mmc/host/sdhci-tegra.c |    4 ++--
>  drivers/mmc/host/sdhci.c       |    8 ++++----
>  drivers/mmc/host/sdhci.h       |    2 +-
>  6 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 504da715..cf34b8d 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -934,7 +934,7 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
>  	return 0;
>  }
>  
> -static int sdhci_pci_8bit_width(struct sdhci_host *host, int width)
> +static int sdhci_pci_bus_width(struct sdhci_host *host, int width)
>  {
>  	u8 ctrl;
>  
> @@ -976,7 +976,7 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
>  
>  static struct sdhci_ops sdhci_pci_ops = {
>  	.enable_dma	= sdhci_pci_enable_dma,
> -	.platform_8bit_width	= sdhci_pci_8bit_width,
> +	.platform_bus_width	= sdhci_pci_bus_width,
>  	.hw_reset		= sdhci_pci_hw_reset,
>  };
>  
> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
> index b6ee885..f380de8 100644
> --- a/drivers/mmc/host/sdhci-pxav2.c
> +++ b/drivers/mmc/host/sdhci-pxav2.c
> @@ -121,7 +121,7 @@ static u32 pxav2_get_max_clock(struct sdhci_host *host)
>  static struct sdhci_ops pxav2_sdhci_ops = {
>  	.get_max_clock = pxav2_get_max_clock,
>  	.platform_reset_exit = pxav2_set_private_registers,
> -	.platform_8bit_width = pxav2_mmc_set_width,
> +	.platform_bus_width = pxav2_mmc_set_width,
>  };
>  
>  #ifdef CONFIG_OF
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index a50c205..776ef3a 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -322,14 +322,14 @@ static void sdhci_cmu_set_clock(struct sdhci_host *host, unsigned int clock)
>  }
>  
>  /**
> - * sdhci_s3c_platform_8bit_width - support 8bit buswidth
> + * sdhci_s3c_platform_bus_width - support 8bit buswidth
>   * @host: The SDHCI host being queried
>   * @width: MMC_BUS_WIDTH_ macro for the bus width being requested
>   *
>   * We have 8-bit width support but is not a v3 controller.
> - * So we add platform_8bit_width() and support 8bit width.
> + * So we add platform_bus_width() and support 8bit width.
>   */
> -static int sdhci_s3c_platform_8bit_width(struct sdhci_host *host, int width)
> +static int sdhci_s3c_platform_bus_width(struct sdhci_host *host, int width)
>  {
>  	u8 ctrl;
>  
> @@ -359,7 +359,7 @@ static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_max_clock		= sdhci_s3c_get_max_clk,
>  	.set_clock		= sdhci_s3c_set_clock,
>  	.get_min_clock		= sdhci_s3c_get_min_clock,
> -	.platform_8bit_width	= sdhci_s3c_platform_8bit_width,
> +	.platform_bus_width	= sdhci_s3c_platform_bus_width,
>  };
>  
>  static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 0810ccc..1f51667 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -144,7 +144,7 @@ static void tegra_sdhci_reset_exit(struct sdhci_host *host, u8 mask)
>  	}
>  }
>  
> -static int tegra_sdhci_8bit(struct sdhci_host *host, int bus_width)
> +static int tegra_sdhci_buswidth(struct sdhci_host *host, int bus_width)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_tegra *tegra_host = pltfm_host->priv;
> @@ -171,7 +171,7 @@ static struct sdhci_ops tegra_sdhci_ops = {
>  	.read_l     = tegra_sdhci_readl,
>  	.read_w     = tegra_sdhci_readw,
>  	.write_l    = tegra_sdhci_writel,
> -	.platform_8bit_width = tegra_sdhci_8bit,
> +	.platform_bus_width = tegra_sdhci_buswidth,
>  	.platform_reset_exit = tegra_sdhci_reset_exit,
>  };
>  
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9a11dc3..9043a3c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1372,11 +1372,11 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>  	/*
>  	 * If your platform has 8-bit width support but is not a v3 controller,
>  	 * or if it requires special setup code, you should implement that in
> -	 * platform_8bit_width().
> +	 * platform_bus_width().
>  	 */
> -	if (host->ops->platform_8bit_width)
> -		host->ops->platform_8bit_width(host, ios->bus_width);
> -	else {
> +	if (host->ops->platform_bus_width) {
> +		host->ops->platform_bus_width(host, ios->bus_width);
> +	} else {
Why add the brackets? 

Best Regards,
Jaehoon Chung
>  		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>  		if (ios->bus_width == MMC_BUS_WIDTH_8) {
>  			ctrl &= ~SDHCI_CTRL_4BITBUS;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 97653ea..efce211 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -267,7 +267,7 @@ struct sdhci_ops {
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> -	int		(*platform_8bit_width)(struct sdhci_host *host,
> +	int		(*platform_bus_width)(struct sdhci_host *host,
>  					       int width);
>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>  					     u8 power_mode);
> 

[PATCH 1/3] mmc: sdhci: rename platform_8bit_width to platform_bus_width

[Sascha Hauer]

On Tue, Sep 25, 2012 at 11:35:14AM +0900, Jaehoon Chung wrote:
> On 09/24/2012 04:22 PM, Sascha Hauer wrote:
> >  	/*
> >  	 * If your platform has 8-bit width support but is not a v3 controller,
> >  	 * or if it requires special setup code, you should implement that in
> > -	 * platform_8bit_width().
> > +	 * platform_bus_width().
> >  	 */
> > -	if (host->ops->platform_8bit_width)
> > -		host->ops->platform_8bit_width(host, ios->bus_width);
> > -	else {
> > +	if (host->ops->platform_bus_width) {
> > +		host->ops->platform_bus_width(host, ios->bus_width);
> > +	} else {
> Why add the brackets?

Took the chance to cleanup the codingstyle when changing these lines
anyway.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/3] mmc: esdhc i.MX: Fix version register read

[Shawn Guo]

On Mon, Sep 24, 2012 at 09:22:24AM +0200, Sascha Hauer wrote:
> The i.MX ESDHC controller version register is a mess:
> 
> - i.MX25 has a v1 controller which identifies itself as v2
> - i.MX6Q has a v3 controller which identifies itself as v4
> - i.MX35,51,53 have v2 controllers which identify themselves correctly
> 
> Additionally on i.MX the register is located at offset 0xfc instead of
> 0xfe. The i.MX6 had a quirk around it which converted v4 into v3. Instead

Ah, I added the quirk for i.MX6 without noticing that other SoCs have
the similar problem.

> of reading the real version register, all other SoCs used to return
> the value from 0xfe which contains 0x0 for all SoCs, so all controllers
> except i.MX6q were identified as version v1.
> 
> This patch fixes this by returning the version based on the devtype
> data leaving the useless version register untouched.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |   21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index e23f813..4fe7312 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -237,15 +237,20 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
>  
>  static u16 esdhc_readw_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;
> +
>  	if (unlikely(reg == SDHCI_HOST_VERSION)) {
> -		u16 val = readw(host->ioaddr + (reg ^ 2));
> -		/*
> -		 * uSDHC supports SDHCI v3.0, but it's encoded as value
> -		 * 0x3 in host controller version register, which violates
> -		 * SDHCI_SPEC_300 definition.  Work it around here.
> -		 */
> -		if ((val & SDHCI_SPEC_VER_MASK) == 3)
> -			return --val;
> +		switch (imx_data->devtype) {
> +		case IMX25_ESDHC:
> +			return SDHCI_SPEC_100;
> +		case IMX35_ESDHC:
> +		case IMX51_ESDHC:
> +		case IMX53_ESDHC:
> +			return SDHCI_SPEC_200;
> +		case IMX6Q_USDHC:
> +			return SDHCI_SPEC_300;
> +		}
>  	}
>  
>  	return readw(host->ioaddr + reg);
> -- 
> 1.7.10.4
> 
> --
> 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

[PATCH] mmc: sdhci / i.MX esdhc buswidth patches

[Wolfram Sang]

On Mon, Sep 24, 2012 at 09:22:22AM +0200, Sascha Hauer wrote:
> The first patch is a generic sdhci driver cleanup change, the others
> are i.MX specific, adding 8bit bus support and fix version register
> readout.
> 
> Sascha
> 
> ----------------------------------------------------------------
> Sascha Hauer (3):
>       mmc: sdhci: rename platform_8bit_width to platform_bus_width
>       mmc: esdhc i.MX: Fix version register read
>       mmc: esdhc i.MX: Support 8bit mode

Whole series:

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

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

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Mon, Sep 24, 2012 at 09:22:25AM +0200, Sascha Hauer wrote:
> The i.MX esdhc has a nonstandard bit layout for the SDHCI_HOST_CONTROL
> register. To support 8bit bus width on i.MX populate the platform_bus_width
> callback. This is tested on an i.MX25, but should according to the datasheets
> work on the other i.MX using this hardware aswell. The i.MX6, while having
> a SDHCI_SPEC_300 controller, still uses the same nonstandard register layout.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |    1 +
>  arch/arm/plat-mxc/include/mach/esdhc.h             |    1 +
>  drivers/mmc/host/sdhci-esdhc-imx.c                 |   56 ++++++++++++++++++--
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> index 1dd6225..c989994 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> @@ -12,6 +12,7 @@ Required properties:
>  Optional properties:
>  - fsl,cd-controller : Indicate to use controller internal card detection
>  - fsl,wp-controller : Indicate to use controller internal write protection
> +- bus-width : Maximum supported bus width. Defaults to 4 if omitted
>
This is a common mmc property documented in bindings/mmc/mmc.txt.
Instead of duplicating the documentation, we should try to make our
implementation conform to the common definition of the property.

>  Examples:
>  
> diff --git a/arch/arm/plat-mxc/include/mach/esdhc.h b/arch/arm/plat-mxc/include/mach/esdhc.h
> index aaf9748..b4a0521 100644
> --- a/arch/arm/plat-mxc/include/mach/esdhc.h
> +++ b/arch/arm/plat-mxc/include/mach/esdhc.h
> @@ -39,5 +39,6 @@ struct esdhc_platform_data {
>  	unsigned int cd_gpio;
>  	enum wp_types wp_type;
>  	enum cd_types cd_type;
> +	int max_bus_width;
>  };
>  #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 4fe7312..b205fe9 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -57,6 +57,13 @@
>   */
>  #define ESDHC_FLAG_MULTIBLK_NO_INT	(1 << 1)
>  
> +/*
> + * Freescale interpretation of the SDHCI_HOST_CONTROL register
> + */
#define FSL_SDHCI_CTRL_1BITBUS			(0x0 << 1)
...

> +#define FSL_SDHCI_CTRL_4BITBUS			(0x1 << 1)
> +#define FSL_SDHCI_CTRL_8BITBUS			(0x2 << 1)
> +#define FSL_SDHCI_CTRL_BUSWIDTH_MASK		(0x3 << 1)
> +
>  enum imx_esdhc_type {
>  	IMX25_ESDHC,
>  	IMX35_ESDHC,
> @@ -307,6 +314,7 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>  	u32 new_val;
> +	u32 mask;
>  
>  	switch (reg) {
>  	case SDHCI_POWER_CONTROL:
> @@ -318,7 +326,6 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>  	case SDHCI_HOST_CONTROL:
>  		/* FSL messed up here, so we can just keep those three */
>  		new_val = val & (SDHCI_CTRL_LED | \
> -				SDHCI_CTRL_4BITBUS | \
>  				SDHCI_CTRL_D3CD);
>  		/* ensure the endianess */
>  		new_val |= ESDHC_HOST_CONTROL_LE;
> @@ -328,7 +335,13 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>  			new_val |= (val & SDHCI_CTRL_DMA_MASK) << 5;
>  		}
>  
> -		esdhc_clrset_le(host, 0xffff, new_val, reg);
> +		/*
> +		 * Do not touch buswidth bits here. This is done in
> +		 * esdhc_pltfm_buswidth.
> +		 */
> +		mask = 0xffff & ~FSL_SDHCI_CTRL_BUSWIDTH_MASK;
> +
> +		esdhc_clrset_le(host, mask, new_val, reg);
>  		return;
>  	}
>  	esdhc_clrset_le(host, 0xff, val, reg);
> @@ -379,6 +392,27 @@ static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
>  	return -ENOSYS;
>  }
>  
> +static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
> +{
> +	u32 ctrl;
> +
> +	switch (width) {
> +	case MMC_BUS_WIDTH_8:
> +		ctrl = FSL_SDHCI_CTRL_8BITBUS;
> +		break;
> +	case MMC_BUS_WIDTH_4:
> +		ctrl = FSL_SDHCI_CTRL_4BITBUS;
> +		break;
> +	default:
> +		ctrl = 0;

... 
		ctrl = FSL_SDHCI_CTRL_1BITBUS;

Any better?

> +		break;
> +	}
> +
> +	esdhc_clrset_le(host, FSL_SDHCI_CTRL_BUSWIDTH_MASK, ctrl, SDHCI_HOST_CONTROL);
> +
> +	return 0;
> +}
> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -389,6 +423,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_max_clock = esdhc_pltfm_get_max_clock,
>  	.get_min_clock = esdhc_pltfm_get_min_clock,
>  	.get_ro = esdhc_pltfm_get_ro,
> +	.platform_bus_width = esdhc_pltfm_bus_width,
>  };
>  
>  static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> @@ -434,6 +469,8 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>  	if (gpio_is_valid(boarddata->wp_gpio))
>  		boarddata->wp_type = ESDHC_WP_GPIO;
>  
> +	of_property_read_u32(np, "bus-width", &boarddata->max_bus_width);
> +
>  	return 0;
>  }
>  #else
> @@ -507,7 +544,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
>  		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
>  		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
> -			| SDHCI_QUIRK_BROKEN_ADMA;
> +			 | SDHCI_QUIRK_BROKEN_ADMA;
>  

Why this change?

Shawn

>  	if (is_imx53_esdhc(imx_data))
>  		imx_data->flags |= ESDHC_FLAG_MULTIBLK_NO_INT;
> @@ -577,6 +614,19 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  		break;
>  	}
>  
> +	switch (boarddata->max_bus_width) {
> +	case 8:
> +		host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
> +		break;
> +	case 4:
> +	default:
> +		host->mmc->caps |= MMC_CAP_4_BIT_DATA;
> +		break;
> +	case 1:
> +		host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
> +		break;
> +	}
> +
>  	err = sdhci_add_host(host);
>  	if (err)
>  		goto err_add_host;
> -- 
> 1.7.10.4
> 
> --
> 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

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Sascha Hauer]

On Tue, Sep 25, 2012 at 03:15:08PM +0800, Shawn Guo wrote:
> On Mon, Sep 24, 2012 at 09:22:25AM +0200, Sascha Hauer wrote:
> > The i.MX esdhc has a nonstandard bit layout for the SDHCI_HOST_CONTROL
> > register. To support 8bit bus width on i.MX populate the platform_bus_width
> > callback. This is tested on an i.MX25, but should according to the datasheets
> > work on the other i.MX using this hardware aswell. The i.MX6, while having
> > a SDHCI_SPEC_300 controller, still uses the same nonstandard register layout.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |    1 +
> >  arch/arm/plat-mxc/include/mach/esdhc.h             |    1 +
> >  drivers/mmc/host/sdhci-esdhc-imx.c                 |   56 ++++++++++++++++++--
> >  3 files changed, 55 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> > index 1dd6225..c989994 100644
> > --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> > @@ -12,6 +12,7 @@ Required properties:
> >  Optional properties:
> >  - fsl,cd-controller : Indicate to use controller internal card detection
> >  - fsl,wp-controller : Indicate to use controller internal write protection
> > +- bus-width : Maximum supported bus width. Defaults to 4 if omitted
> >
> This is a common mmc property documented in bindings/mmc/mmc.txt.
> Instead of duplicating the documentation, we should try to make our
> implementation conform to the common definition of the property.

It is conform to the common definition, so all I have to do is drop the
line duplicating the docs. That's easy ;)

> > +		break;
> > +	case MMC_BUS_WIDTH_4:
> > +		ctrl = FSL_SDHCI_CTRL_4BITBUS;
> > +		break;
> > +	default:
> > +		ctrl = 0;
> 
> ... 
> 		ctrl = FSL_SDHCI_CTRL_1BITBUS;
> 
> Any better?

ok.

> >  	if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
> >  		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
> >  		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
> > -			| SDHCI_QUIRK_BROKEN_ADMA;
> > +			 | SDHCI_QUIRK_BROKEN_ADMA;
> >  
> 
> Why this change?

Accident. Will drop.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] mmc: sdhci: rename platform_8bit_width to platform_bus_width

[Jaehoon Chung]

On 09/25/2012 03:15 PM, Sascha Hauer wrote:
> On Tue, Sep 25, 2012 at 11:35:14AM +0900, Jaehoon Chung wrote:
>> On 09/24/2012 04:22 PM, Sascha Hauer wrote:
>>>  	/*
>>>  	 * If your platform has 8-bit width support but is not a v3 controller,
>>>  	 * or if it requires special setup code, you should implement that in
>>> -	 * platform_8bit_width().
>>> +	 * platform_bus_width().
>>>  	 */
>>> -	if (host->ops->platform_8bit_width)
>>> -		host->ops->platform_8bit_width(host, ios->bus_width);
>>> -	else {
>>> +	if (host->ops->platform_bus_width) {
>>> +		host->ops->platform_bus_width(host, ios->bus_width);
>>> +	} else {
>> Why add the brackets?
> 
> Took the chance to cleanup the codingstyle when changing these lines
> anyway.
Anyway, this patch is reasonable. Looks good to me.

Best Regards,
Jaehoon Chung
> 
> Sascha
> 
> 

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Tue, Sep 25, 2012 at 09:27:15AM +0200, Sascha Hauer wrote:
> > >  Optional properties:
> > >  - fsl,cd-controller : Indicate to use controller internal card detection
> > >  - fsl,wp-controller : Indicate to use controller internal write protection
> > > +- bus-width : Maximum supported bus width. Defaults to 4 if omitted
> > >
> > This is a common mmc property documented in bindings/mmc/mmc.txt.
> > Instead of duplicating the documentation, we should try to make our
> > implementation conform to the common definition of the property.
> 
> It is conform to the common definition, so all I have to do is drop the
> line duplicating the docs. That's easy ;)
> 
Maybe not.  The common binding defines it as a required property while
the patch implements it as an optional one.

Shawn

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Sascha Hauer]

On Tue, Sep 25, 2012 at 03:33:19PM +0800, Shawn Guo wrote:
> On Tue, Sep 25, 2012 at 09:27:15AM +0200, Sascha Hauer wrote:
> > > >  Optional properties:
> > > >  - fsl,cd-controller : Indicate to use controller internal card detection
> > > >  - fsl,wp-controller : Indicate to use controller internal write protection
> > > > +- bus-width : Maximum supported bus width. Defaults to 4 if omitted
> > > >
> > > This is a common mmc property documented in bindings/mmc/mmc.txt.
> > > Instead of duplicating the documentation, we should try to make our
> > > implementation conform to the common definition of the property.
> > 
> > It is conform to the common definition, so all I have to do is drop the
> > line duplicating the docs. That's easy ;)
> > 
> Maybe not.  The common binding defines it as a required property while
> the patch implements it as an optional one.

So you want to make the code conform to the common spec which has
bus-width as a required property. Is this really worth it to add an
incompatible change to our devicetrees?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Tue, Sep 25, 2012 at 09:38:08AM +0200, Sascha Hauer wrote:
> So you want to make the code conform to the common spec which has
> bus-width as a required property. Is this really worth it to add an
> incompatible change to our devicetrees?
> 
I do want to have our implementation conform to common spec.  At the
current stage devicetrees are not stable anyway, so we should make
them right as much as possible before they start getting maintained
separately from kernel tree, where we will be asked to maintain
devicetree compatibility a lot harder.

Shawn

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Chris Ball]

Hi,

On Tue, Sep 25 2012, Sascha Hauer wrote:
>> > > > +- bus-width : Maximum supported bus width. Defaults to 4 if omitted
>> > > >
>> > > This is a common mmc property documented in bindings/mmc/mmc.txt.
>> > > Instead of duplicating the documentation, we should try to make our
>> > > implementation conform to the common definition of the property.
>> > 
>> > It is conform to the common definition, so all I have to do is drop the
>> > line duplicating the docs. That's easy ;)
>> > 
>> Maybe not.  The common binding defines it as a required property while
>> the patch implements it as an optional one.
>
> So you want to make the code conform to the common spec which has
> bus-width as a required property. Is this really worth it to add an
> incompatible change to our devicetrees?

We could also break the stalemate by just changing the common spec to
have bus-width become optional, default 4.  It's not going to break the
code of anyone who's been treating it as required.  I don't think there
there was a principled reason behind making it required.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Tue, Sep 25, 2012 at 03:52:09AM -0400, Chris Ball wrote:
> We could also break the stalemate by just changing the common spec to
> have bus-width become optional, default 4.  It's not going to break the
> code of anyone who's been treating it as required.  I don't think there
> there was a principled reason behind making it required.
> 
That would be the best :)

Shawn

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Chris Ball]

Hi,

On Tue, Sep 25 2012, Shawn Guo wrote:
> On Tue, Sep 25, 2012 at 03:52:09AM -0400, Chris Ball wrote:
>> We could also break the stalemate by just changing the common spec to
>> have bus-width become optional, default 4.  It's not going to break the
>> code of anyone who's been treating it as required.  I don't think there
>> there was a principled reason behind making it required.
>> 
> That would be the best :)

Ah, but sdhci-s3c and dw_mmc have been assuming that bus-width defaults
1 when it's not supplied.  That's going to make it complicated.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Sascha Hauer]

On Tue, Sep 25, 2012 at 03:45:50PM +0800, Shawn Guo wrote:
> On Tue, Sep 25, 2012 at 09:38:08AM +0200, Sascha Hauer wrote:
> > So you want to make the code conform to the common spec which has
> > bus-width as a required property. Is this really worth it to add an
> > incompatible change to our devicetrees?
> > 
> I do want to have our implementation conform to common spec.  At the
> current stage devicetrees are not stable anyway, so we should make
> them right as much as possible before they start getting maintained
> separately from kernel tree, where we will be asked to maintain
> devicetree compatibility a lot harder.

Ok, how about this:

- I add a bus-width = <4> property to all i.MX dtsi files
- we merge the patch adding bus-width as an optional property
- Make the bus-width option mandatory in the next step.

This way we can merge the mmc related patches through the mmc tree and
the dts changes through arm-soc.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Tue, Sep 25, 2012 at 10:05:25AM +0200, Sascha Hauer wrote:
> Ok, how about this:
> 
> - I add a bus-width = <4> property to all i.MX dtsi files
> - we merge the patch adding bus-width as an optional property
> - Make the bus-width option mandatory in the next step.
> 
> This way we can merge the mmc related patches through the mmc tree and
> the dts changes through arm-soc.
> 
Sounds good to me.  How the last step should be done will depend
on the common spec.  Per Chris, there are already two drivers
implementing the property as optional.  Not sure if Chris will change
spec to have the property defined as optional (probably default to 1?).

Shawn

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Sascha Hauer]

On Tue, Sep 25, 2012 at 04:50:52PM +0800, Shawn Guo wrote:
> On Tue, Sep 25, 2012 at 10:05:25AM +0200, Sascha Hauer wrote:
> > Ok, how about this:
> > 
> > - I add a bus-width = <4> property to all i.MX dtsi files
> > - we merge the patch adding bus-width as an optional property
> > - Make the bus-width option mandatory in the next step.
> > 
> > This way we can merge the mmc related patches through the mmc tree and
> > the dts changes through arm-soc.
> > 
> Sounds good to me.  How the last step should be done will depend
> on the common spec.  Per Chris, there are already two drivers
> implementing the property as optional.  Not sure if Chris will change
> spec to have the property defined as optional (probably default to 1?).

It won't hurt if we add the property now for all i.MX. Then Chris is
free to do whatever he wants (at least from the i.MX perspective)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/3] mmc: esdhc i.MX: Fix version register read

[Shawn Guo]

Hi Sascha,

On Mon, Sep 24, 2012 at 09:22:24AM +0200, Sascha Hauer wrote:
> The i.MX ESDHC controller version register is a mess:
> 
> - i.MX25 has a v1 controller which identifies itself as v2
> - i.MX6Q has a v3 controller which identifies itself as v4
> - i.MX35,51,53 have v2 controllers which identify themselves correctly

When looking at these a second time, I start doubting the statement
about i.MX25.  Since i.MX25 is actually a newer SoC than i.MX35, it
sounds unreasonable that i.MX35 runs a v2 controller while i.MX25 runs
a v1.

The confirmation from designer tells that i.MX25 runs a v2 controller
too.  This also matches the statement of "SD Host Controller Standard
Specification Version 2.0" listed in IMX25RM 23.1.1 Features chapter.
You probably got the impression that i.MX25 has a v1 controller by
reading the HOSTVER_SVN bit description.  While it says 0x00 - SD Host
Specification Version 1.0, the register value is actually 0x01. 

So the esdhc host version register indeed encodes the correct value
for all i.MX SoCs except imx6q.  Thus, I prefer to replace the patch
with the one below.

Also I'm working on a big series to make esdhc driver more feature
complete for imx6q, including 8bit support, so I would like to pick
up your work from here.

Shawn

---8<------

[PATCH] mmc: sdhci-esdhc-imx: fix host version read

When commit 95a2482 (mmc: sdhci-esdhc-imx: add basic imx6q usdhc
support) works around host version issue on imx6q, it gets the
register address fixup "reg ^= 2" lost for imx25/35/51/53 esdhc.
Thus, the controller version on these SoCs is wrongly identified
as v1 while it's actually v2.

Add the address fixup back and take a different approach to correct
imx6q host version, so that the host version read gets back to work
for all SoCs.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: <stable@vger.kernel.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index e07df81..b503113 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -237,15 +237,18 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
 
 static u16 esdhc_readw_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;
+
 	if (unlikely(reg == SDHCI_HOST_VERSION)) {
-		u16 val = readw(host->ioaddr + (reg ^ 2));
-		/*
-		 * uSDHC supports SDHCI v3.0, but it's encoded as value
-		 * 0x3 in host controller version register, which violates
-		 * SDHCI_SPEC_300 definition.  Work it around here.
-		 */
-		if ((val & SDHCI_SPEC_VER_MASK) == 3)
-			return --val;
+		reg ^= 2;
+		if (is_imx6q_usdhc(imx_data)) {
+			/*
+			 * The usdhc register returns a wrong host version.
+			 * Correct it here.
+			 */
+			return SDHCI_SPEC_300;
+		}
 	}
 
 	return readw(host->ioaddr + reg);
-- 
1.7.9.5

[PATCH 3/3] mmc: esdhc i.MX: Support 8bit mode

[Shawn Guo]

On Tue, Sep 25, 2012 at 04:00:18AM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Sep 25 2012, Shawn Guo wrote:
> > On Tue, Sep 25, 2012 at 03:52:09AM -0400, Chris Ball wrote:
> >> We could also break the stalemate by just changing the common spec to
> >> have bus-width become optional, default 4.  It's not going to break the
> >> code of anyone who's been treating it as required.  I don't think there
> >> there was a principled reason behind making it required.
> >> 
> > That would be the best :)
> 
> Ah, but sdhci-s3c and dw_mmc have been assuming that bus-width defaults
> 1 when it's not supplied.  That's going to make it complicated.
> 
Okay, we will do the same for esdhc driver, and also submit a change to
the common binding to make it clear.

Shawn