mmc: sdhci / i.MX esdhc buswidth patches

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 <[email protected]>

---
 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(-)

drivers/mmc/host/sdhci-pci.c +2 -2

index 504da715..cf34b8d 100644
@@ -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,
 };
 

drivers/mmc/host/sdhci-pxav2.c +1 -1

index b6ee885..f380de8 100644
@@ -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

drivers/mmc/host/sdhci-s3c.c +4 -4

index a50c205..776ef3a 100644
@@ -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)

drivers/mmc/host/sdhci-tegra.c +2 -2

index 0810ccc..1f51667 100644
@@ -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,
 };
 

drivers/mmc/host/sdhci.c +0 -0

index 9a11dc3..9043a3c 100644
@@ -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,

drivers/mmc/host/sdhci.c +0 -0

 		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 		if (ios->bus_width == MMC_BUS_WIDTH_8) {
 			ctrl &= ~SDHCI_CTRL_4BITBUS;

drivers/mmc/host/sdhci.h +1 -1

index 97653ea..efce211 100644
@@ -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);

drivers/mmc/host/sdhci.c static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) · line 1379

Quoted 12 lines · Sascha Hauer

drivers/mmc/host/sdhci.c +4 -4

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

Quoted 14 lines · Jaehoon Chung

drivers/mmc/host/sdhci.c static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) · line 1379

Quoted 12 lines · Sascha Hauer

drivers/mmc/host/sdhci.c +4 -4

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

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 <[email protected]>

---
 drivers/mmc/host/sdhci-esdhc-imx.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

drivers/mmc/host/sdhci-esdhc-imx.c +13 -8

index e23f813..4fe7312 100644
@@ -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);

Signature · 6 lines

1.7.10.4

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

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

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 <[email protected]>

---
 .../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(-)

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt +0 -0

index 1dd6225..c989994 100644

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt +0 -0

 Examples:
 

arch/arm/plat-mxc/include/mach/esdhc.h +1 -0

index aaf9748..b4a0521 100644
@@ -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 */

drivers/mmc/host/sdhci-esdhc-imx.c +0 -0

index 4fe7312..b205fe9 100644

drivers/mmc/host/sdhci-esdhc-imx.c +25 -2

+#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;

drivers/mmc/host/sdhci-esdhc-imx.c +11 -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

drivers/mmc/host/sdhci-esdhc-imx.c +13 -0

 	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;

Signature · 6 lines

1.7.10.4

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

Quoted 13 lines · 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 <[email protected]>

---
 .../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(-)

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt Required properties: · line 16

Quoted 10 lines

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt +1 -0

index 1dd6225..c989994 100644
@@ -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.

Quoted 6 lines

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt +6 -0

+		break;
+	case MMC_BUS_WIDTH_4:
+		ctrl = FSL_SDHCI_CTRL_4BITBUS;
+		break;
+	default:
+		ctrl = 0;

...

		ctrl = FSL_SDHCI_CTRL_1BITBUS;

Any better?

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt

Quoted 6 lines

Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt +1 -1

 	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?

Quoted 5 lines

Quoted 5 lines

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

Quoted 3 lines

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 ;)

Quoted 9 lines · Sascha Hauer

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 4 lines · Sascha Hauer

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.

Quoted 6 lines · Sascha Hauer

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.

Quoted 3 lines

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

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

+- 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 ;)

Quoted 2 lines

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.

Quoted 5 lines · Chris Ball

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 :)

Hi,

Quoted 6 lines · Shawn Guo

Quoted 5 lines · Chris Ball

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.

Quoted 1 line

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.

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