* [RFC] sdhci: 8 bit bus width changes
@ 2010-10-01 22:45 Philip Rakity
2010-11-05 7:13 ` Peppe CAVALLARO
2010-11-19 21:40 ` Chris Ball
0 siblings, 2 replies; 14+ messages in thread
From: Philip Rakity @ 2010-10-01 22:45 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org
>From 80c581d354df180a4c2d6aa50c21788e6c7593b9 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Fri, 1 Oct 2010 13:42:58 -0700
Subject: [RFC] sdhci: 8 bit bus width changes
Check for v3 controller controller when doing 8 bit bus width
add support for adaption layer to set 8 bit width.
controller may support 8 bit bus but NOT board design
allow non v3 controllers to support 8 bit bus
non v3 controller (platform_8bit_width)
=======================================
platform_8bit_width is used to support 8 bit mode using our v2
sd controller. We have private registers that are used for 8 bit support
v2 - v3 controller and 8 bit support (SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
=====================================================================
The quirk is needed since we support multiple SD controllers and
on the board sometimes 8 data lines are brought out and sometimes only 4.
The SD controller indicates it can support 8 bit (v3 controller) via the
capability field but has no knowledge of how the pins were configured
in the board design. There are only so many pins and the pins are multiplexed.
Signed-off-by: Philip Rakity <prakity@marvell.com>
---
drivers/mmc/host/sdhci.c | 44 +++++++++++++++++++++++++++++++++-----------
drivers/mmc/host/sdhci.h | 7 ++++++-
2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 73a94fe..ec103c3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1182,17 +1182,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
else
sdhci_set_power(host, ios->vdd);
- ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
- if (ios->bus_width == MMC_BUS_WIDTH_8)
- ctrl |= SDHCI_CTRL_8BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_8BITBUS;
+ /*
+ * use platform_8_bit_width if not v3 controller
+ * or if special hw/board specific processing is needed
+ */
+ if (host->ops->platform_8bit_width)
+ host->ops->platform_8bit_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;
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl |= SDHCI_CTRL_8BITBUS;
+ } else {
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl &= ~SDHCI_CTRL_8BITBUS;
+ if (ios->bus_width == MMC_BUS_WIDTH_4)
+ ctrl |= SDHCI_CTRL_4BITBUS;
+ else
+ ctrl &= ~SDHCI_CTRL_4BITBUS;
+ }
+ sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+ }
- if (ios->bus_width == MMC_BUS_WIDTH_4)
- ctrl |= SDHCI_CTRL_4BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_4BITBUS;
+ ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
if (ios->timing == MMC_TIMING_SD_HS &&
!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT))
@@ -1838,11 +1852,19 @@ int sdhci_add_host(struct sdhci_host *host)
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
else
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
+
mmc->f_max = host->max_clk;
mmc->caps |= MMC_CAP_SDIO_IRQ;
- if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
- mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
+ /* 8 bit width may be supported by v3 controller but not board
+ * so the safest thing is to let the adaption layer decide
+ * what to do by using the quirk
+ */
+ if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
+ mmc->caps |= MMC_CAP_4_BIT_DATA;
+ if (host->quirks & SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
+ mmc->caps |= MMC_CAP_8_BIT_DATA;
+ }
if (caps & SDHCI_CAN_DO_HISPD)
mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 112543a..b3288fd 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -72,7 +72,7 @@
#define SDHCI_CTRL_ADMA1 0x08
#define SDHCI_CTRL_ADMA32 0x10
#define SDHCI_CTRL_ADMA64 0x18
-#define SDHCI_CTRL_8BITBUS 0x20
+#define SDHCI_CTRL_8BITBUS 0x20
#define SDHCI_POWER_CONTROL 0x29
#define SDHCI_POWER_ON 0x01
@@ -148,6 +148,7 @@
#define SDHCI_CLOCK_BASE_SHIFT 8
#define SDHCI_MAX_BLOCK_MASK 0x00030000
#define SDHCI_MAX_BLOCK_SHIFT 16
+#define SDHCI_CAN_DO_8BIT 0x00040000
#define SDHCI_CAN_DO_ADMA2 0x00080000
#define SDHCI_CAN_DO_ADMA1 0x00100000
#define SDHCI_CAN_DO_HISPD 0x00200000
@@ -260,6 +261,8 @@ struct sdhci_host {
#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
/* Controller doesn't have HISPD bit field in HI-SPEED SD card */
#define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
+/* slot has 8 data pins going to eMMC/mmc card */
+#define SDHCI_QUIRK_SLOT_CAN_DO_8_BITS (1<<30)
int irq; /* Device IRQ */
void __iomem * ioaddr; /* Mapped address */
@@ -336,6 +339,8 @@ 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 width);
};
#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
--
1.6.0.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-10-01 22:45 [RFC] sdhci: 8 bit bus width changes Philip Rakity
@ 2010-11-05 7:13 ` Peppe CAVALLARO
2010-11-19 21:40 ` Chris Ball
1 sibling, 0 replies; 14+ messages in thread
From: Peppe CAVALLARO @ 2010-11-05 7:13 UTC (permalink / raw)
To: Philip Rakity; +Cc: linux-mmc@vger.kernel.org
Hello Philip,
On 10/2/2010 12:45 AM, Philip Rakity wrote:
>
> From 80c581d354df180a4c2d6aa50c21788e6c7593b9 Mon Sep 17 00:00:00 2001
> From: Philip Rakity <prakity@marvell.com>
> Date: Fri, 1 Oct 2010 13:42:58 -0700
> Subject: [RFC] sdhci: 8 bit bus width changes
>
> Check for v3 controller controller when doing 8 bit bus width
> add support for adaption layer to set 8 bit width.
> controller may support 8 bit bus but NOT board design
> allow non v3 controllers to support 8 bit bus
>
Patches ported and tested on our ST target and it works for me.
Thanks!
>
>
> non v3 controller (platform_8bit_width)
> =======================================
> platform_8bit_width is used to support 8 bit mode using our v2
> sd controller. We have private registers that are used for 8 bit support
>
> v2 - v3 controller and 8 bit support (SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
> =====================================================================
> The quirk is needed since we support multiple SD controllers and
> on the board sometimes 8 data lines are brought out and sometimes only 4.
> The SD controller indicates it can support 8 bit (v3 controller) via the
> capability field but has no knowledge of how the pins were configured
> in the board design. There are only so many pins and the pins are
> multiplexed.
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
>
Tested -by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Regards
Peppe
>
> ---
> drivers/mmc/host/sdhci.c | 44
> +++++++++++++++++++++++++++++++++-----------
> drivers/mmc/host/sdhci.h | 7 ++++++-
> 2 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 73a94fe..ec103c3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1182,17 +1182,31 @@ static void sdhci_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
> else
> sdhci_set_power(host, ios->vdd);
>
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> - if (ios->bus_width == MMC_BUS_WIDTH_8)
> - ctrl |= SDHCI_CTRL_8BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_8BITBUS;
> + /*
> + * use platform_8_bit_width if not v3 controller
> + * or if special hw/board specific processing is needed
> + */
> + if (host->ops->platform_8bit_width)
> + host->ops->platform_8bit_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;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (ios->bus_width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
>
> - if (ios->bus_width == MMC_BUS_WIDTH_4)
> - ctrl |= SDHCI_CTRL_4BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_4BITBUS;
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> if (ios->timing == MMC_TIMING_SD_HS &&
> !(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT))
> @@ -1838,11 +1852,19 @@ int sdhci_add_host(struct sdhci_host *host)
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
> else
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> +
> mmc->f_max = host->max_clk;
> mmc->caps |= MMC_CAP_SDIO_IRQ;
>
> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> + /* 8 bit width may be supported by v3 controller but not board
> + * so the safest thing is to let the adaption layer decide
> + * what to do by using the quirk
> + */
> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
> + if (host->quirks & SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
> + mmc->caps |= MMC_CAP_8_BIT_DATA;
> + }
>
> if (caps & SDHCI_CAN_DO_HISPD)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 112543a..b3288fd 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -72,7 +72,7 @@
> #define SDHCI_CTRL_ADMA1 0x08
> #define SDHCI_CTRL_ADMA32 0x10
> #define SDHCI_CTRL_ADMA64 0x18
> -#define SDHCI_CTRL_8BITBUS 0x20
> +#define SDHCI_CTRL_8BITBUS 0x20
>
> #define SDHCI_POWER_CONTROL 0x29
> #define SDHCI_POWER_ON 0x01
> @@ -148,6 +148,7 @@
> #define SDHCI_CLOCK_BASE_SHIFT 8
> #define SDHCI_MAX_BLOCK_MASK 0x00030000
> #define SDHCI_MAX_BLOCK_SHIFT 16
> +#define SDHCI_CAN_DO_8BIT 0x00040000
> #define SDHCI_CAN_DO_ADMA2 0x00080000
> #define SDHCI_CAN_DO_ADMA1 0x00100000
> #define SDHCI_CAN_DO_HISPD 0x00200000
> @@ -260,6 +261,8 @@ struct sdhci_host {
> #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
> /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
> #define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
> +/* slot has 8 data pins going to eMMC/mmc card */
> +#define SDHCI_QUIRK_SLOT_CAN_DO_8_BITS (1<<30)
>
> int irq; /* Device IRQ */
> void __iomem * ioaddr; /* Mapped address */
> @@ -336,6 +339,8 @@ 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 width);
> };
>
> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> --
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-10-01 22:45 [RFC] sdhci: 8 bit bus width changes Philip Rakity
2010-11-05 7:13 ` Peppe CAVALLARO
@ 2010-11-19 21:40 ` Chris Ball
2010-11-19 21:53 ` Chris Ball
1 sibling, 1 reply; 14+ messages in thread
From: Chris Ball @ 2010-11-19 21:40 UTC (permalink / raw)
To: Philip Rakity; +Cc: linux-mmc@vger.kernel.org
Hi Philip,
I'd like to take this patch. One comment, though:
On Fri, Oct 01, 2010 at 03:45:50PM -0700, Philip Rakity wrote:
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 73a94fe..ec103c3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1182,17 +1182,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> else
> sdhci_set_power(host, ios->vdd);
>
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> - if (ios->bus_width == MMC_BUS_WIDTH_8)
> - ctrl |= SDHCI_CTRL_8BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_8BITBUS;
> + /*
> + * use platform_8_bit_width if not v3 controller
> + * or if special hw/board specific processing is needed
> + */
> + if (host->ops->platform_8bit_width)
> + host->ops->platform_8bit_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;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (ios->bus_width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
>
> - if (ios->bus_width == MMC_BUS_WIDTH_4)
> - ctrl |= SDHCI_CTRL_4BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_4BITBUS;
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
I don't see why we should re-read ctrl here, since we've already written
it back to the device at this point, and we don't use it anywhere below
this line.
Thanks,
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-19 21:40 ` Chris Ball
@ 2010-11-19 21:53 ` Chris Ball
2010-11-20 12:35 ` Wolfram Sang
2010-11-20 16:37 ` Philip Rakity
0 siblings, 2 replies; 14+ messages in thread
From: Chris Ball @ 2010-11-19 21:53 UTC (permalink / raw)
To: Philip Rakity, linux-mmc@vger.kernel.org
On Fri, Nov 19, 2010 at 09:40:02PM +0000, Chris Ball wrote:
> I don't see why we should re-read ctrl here, since we've already written
> it back to the device at this point, and we don't use it anywhere below
> this line.
Ah, I see why now; please ignore this.
Here's a rebased version of the patch, with some more comments:
From: Philip Rakity <prakity@marvell.com>
Date: Fri, 19 Nov 2010 16:48:39 -0500
Subject: [PATCH] mmc: sdhci: 8-bit bus width changes
We now:
* check for a v3 controller before setting 8-bit bus width
* offer a callback for platform code to switch to 8-bit mode, which
allows non-v3 controllers to support it
* introduce a quirk to specify that the board designers have indeed
brought out all the pins for 8-bit to the slot.
We were previously relying only on whether the controller supported
8-bit, which doesn't tell us anything about the pin configuration in
the board design.
Signed-off-by: Philip Rakity <prakity@marvell.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Signed-off-by: Chris Ball <cjb@laptop.org>
---
drivers/mmc/host/sdhci.c | 48 +++++++++++++++++++++++++++++++++-----------
drivers/mmc/host/sdhci.h | 5 +++-
include/linux/mmc/sdhci.h | 2 +
3 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 154cbf8..f1f9658 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
if (host->ops->platform_send_init_74_clocks)
host->ops->platform_send_init_74_clocks(host, ios->power_mode);
- ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
-
- if (ios->bus_width == MMC_BUS_WIDTH_8)
- ctrl |= SDHCI_CTRL_8BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_8BITBUS;
+ /*
+ * 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().
+ */
+ if (host->ops->platform_8bit_width)
+ host->ops->platform_8bit_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;
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl |= SDHCI_CTRL_8BITBUS;
+ } else {
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl &= ~SDHCI_CTRL_8BITBUS;
+ if (ios->bus_width == MMC_BUS_WIDTH_4)
+ ctrl |= SDHCI_CTRL_4BITBUS;
+ else
+ ctrl &= ~SDHCI_CTRL_4BITBUS;
+ }
+ sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+ }
- if (ios->bus_width == MMC_BUS_WIDTH_4)
- ctrl |= SDHCI_CTRL_4BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_4BITBUS;
+ ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
if ((ios->timing == MMC_TIMING_SD_HS ||
ios->timing == MMC_TIMING_MMC_HS)
@@ -1855,11 +1869,21 @@ int sdhci_add_host(struct sdhci_host *host)
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
else
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
+
mmc->f_max = host->max_clk;
mmc->caps |= MMC_CAP_SDIO_IRQ;
- if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
- mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
+ /*
+ * A controller may support 8-bit width, but the board itself
+ * might not have the pins brought out. So, boards that support
+ * 8-bit width should set the below quirk, and we won't assume
+ * that devices without the quirk can use 8-bit width.
+ */
+ if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
+ mmc->caps |= MMC_CAP_4_BIT_DATA;
+ if (host->quirks & SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
+ mmc->caps |= MMC_CAP_8_BIT_DATA;
+ }
if (caps & SDHCI_CAN_DO_HISPD)
mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d52a716..e42d7f0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -76,7 +76,7 @@
#define SDHCI_CTRL_ADMA1 0x08
#define SDHCI_CTRL_ADMA32 0x10
#define SDHCI_CTRL_ADMA64 0x18
-#define SDHCI_CTRL_8BITBUS 0x20
+#define SDHCI_CTRL_8BITBUS 0x20
#define SDHCI_POWER_CONTROL 0x29
#define SDHCI_POWER_ON 0x01
@@ -155,6 +155,7 @@
#define SDHCI_CLOCK_BASE_SHIFT 8
#define SDHCI_MAX_BLOCK_MASK 0x00030000
#define SDHCI_MAX_BLOCK_SHIFT 16
+#define SDHCI_CAN_DO_8BIT 0x00040000
#define SDHCI_CAN_DO_ADMA2 0x00080000
#define SDHCI_CAN_DO_ADMA1 0x00100000
#define SDHCI_CAN_DO_HISPD 0x00200000
@@ -215,6 +216,8 @@ 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 width);
void (*platform_send_init_74_clocks)(struct sdhci_host *host,
u8 power_mode);
unsigned int (*get_ro)(struct sdhci_host *host);
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 1fdc673..7fdcfca 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -83,6 +83,8 @@ struct sdhci_host {
#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
/* Controller doesn't have HISPD bit field in HI-SPEED SD card */
#define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
+/* Slot has 8 data pins going to eMMC/MMC card */
+#define SDHCI_QUIRK_SLOT_CAN_DO_8_BITS (1<<30)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-19 21:53 ` Chris Ball
@ 2010-11-20 12:35 ` Wolfram Sang
2010-11-20 16:37 ` Philip Rakity
2010-11-20 18:24 ` Chris Ball
2010-11-20 16:37 ` Philip Rakity
1 sibling, 2 replies; 14+ messages in thread
From: Wolfram Sang @ 2010-11-20 12:35 UTC (permalink / raw)
To: Chris Ball; +Cc: Philip Rakity, linux-mmc@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]
I know it's too late, but...
On Fri, Nov 19, 2010 at 09:53:33PM +0000, Chris Ball wrote:
> On Fri, Nov 19, 2010 at 09:40:02PM +0000, Chris Ball wrote:
> > I don't see why we should re-read ctrl here, since we've already written
> > it back to the device at this point, and we don't use it anywhere below
> > this line.
>
> Ah, I see why now; please ignore this.
>
> Here's a rebased version of the patch, with some more comments:
>
> From: Philip Rakity <prakity@marvell.com>
> Date: Fri, 19 Nov 2010 16:48:39 -0500
> Subject: [PATCH] mmc: sdhci: 8-bit bus width changes
>
> We now:
> * check for a v3 controller before setting 8-bit bus width
> * offer a callback for platform code to switch to 8-bit mode, which
> allows non-v3 controllers to support it
What does the platform_-prefix of the callback indicate?
> * introduce a quirk to specify that the board designers have indeed
> brought out all the pins for 8-bit to the slot.
This is not a quirk, this is platform_data, no?
> We were previously relying only on whether the controller supported
> 8-bit, which doesn't tell us anything about the pin configuration in
> the board design.
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Signed-off-by: Chris Ball <cjb@laptop.org>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-20 12:35 ` Wolfram Sang
@ 2010-11-20 16:37 ` Philip Rakity
2010-11-20 18:24 ` Chris Ball
1 sibling, 0 replies; 14+ messages in thread
From: Philip Rakity @ 2010-11-20 16:37 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Chris Ball, linux-mmc@vger.kernel.org
On Nov 20, 2010, at 4:35 AM, Wolfram Sang wrote:
> I know it's too late, but...
>
> On Fri, Nov 19, 2010 at 09:53:33PM +0000, Chris Ball wrote:
>> On Fri, Nov 19, 2010 at 09:40:02PM +0000, Chris Ball wrote:
>>> I don't see why we should re-read ctrl here, since we've already written
>>> it back to the device at this point, and we don't use it anywhere below
>>> this line.
>>
>> Ah, I see why now; please ignore this.
>>
>> Here's a rebased version of the patch, with some more comments:
>>
>> From: Philip Rakity <prakity@marvell.com>
>> Date: Fri, 19 Nov 2010 16:48:39 -0500
>> Subject: [PATCH] mmc: sdhci: 8-bit bus width changes
>>
>> We now:
>> * check for a v3 controller before setting 8-bit bus width
>> * offer a callback for platform code to switch to 8-bit mode, which
>> allows non-v3 controllers to support it
>
> What does the platform_-prefix of the callback indicate?
used for controllers that can support 8 bits and NOT v3
>
>> * introduce a quirk to specify that the board designers have indeed
>> brought out all the pins for 8-bit to the slot.
>
> This is not a quirk, this is platform_data, no?
No opinion -- if it makes sense to move to a platform define I am happy to do that
but
I would like to see some standard platform defines
eg
This one and the one for CARD_PRESENT that can be used consistently across sdhci.c
>
>> We were previously relying only on whether the controller supported
>> 8-bit, which doesn't tell us anything about the pin configuration in
>> the board design.
>>
>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Signed-off-by: Chris Ball <cjb@laptop.org>
>
> --
> Pengutronix e.K. | Wolfram Sang |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-19 21:53 ` Chris Ball
2010-11-20 12:35 ` Wolfram Sang
@ 2010-11-20 16:37 ` Philip Rakity
1 sibling, 0 replies; 14+ messages in thread
From: Philip Rakity @ 2010-11-20 16:37 UTC (permalink / raw)
To: Chris Ball; +Cc: linux-mmc@vger.kernel.org
code is fine.
On Nov 19, 2010, at 1:53 PM, Chris Ball wrote:
> On Fri, Nov 19, 2010 at 09:40:02PM +0000, Chris Ball wrote:
>> I don't see why we should re-read ctrl here, since we've already written
>> it back to the device at this point, and we don't use it anywhere below
>> this line.
>
> Ah, I see why now; please ignore this.
>
> Here's a rebased version of the patch, with some more comments:
>
> From: Philip Rakity <prakity@marvell.com>
> Date: Fri, 19 Nov 2010 16:48:39 -0500
> Subject: [PATCH] mmc: sdhci: 8-bit bus width changes
>
> We now:
> * check for a v3 controller before setting 8-bit bus width
> * offer a callback for platform code to switch to 8-bit mode, which
> allows non-v3 controllers to support it
> * introduce a quirk to specify that the board designers have indeed
> brought out all the pins for 8-bit to the slot.
>
> We were previously relying only on whether the controller supported
> 8-bit, which doesn't tell us anything about the pin configuration in
> the board design.
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
> drivers/mmc/host/sdhci.c | 48 +++++++++++++++++++++++++++++++++-----------
> drivers/mmc/host/sdhci.h | 5 +++-
> include/linux/mmc/sdhci.h | 2 +
> 3 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 154cbf8..f1f9658 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (host->ops->platform_send_init_74_clocks)
> host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> -
> - if (ios->bus_width == MMC_BUS_WIDTH_8)
> - ctrl |= SDHCI_CTRL_8BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_8BITBUS;
> + /*
> + * 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().
> + */
> + if (host->ops->platform_8bit_width)
> + host->ops->platform_8bit_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;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (ios->bus_width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
>
> - if (ios->bus_width == MMC_BUS_WIDTH_4)
> - ctrl |= SDHCI_CTRL_4BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_4BITBUS;
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> if ((ios->timing == MMC_TIMING_SD_HS ||
> ios->timing == MMC_TIMING_MMC_HS)
> @@ -1855,11 +1869,21 @@ int sdhci_add_host(struct sdhci_host *host)
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
> else
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> +
> mmc->f_max = host->max_clk;
> mmc->caps |= MMC_CAP_SDIO_IRQ;
>
> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> + /*
> + * A controller may support 8-bit width, but the board itself
> + * might not have the pins brought out. So, boards that support
> + * 8-bit width should set the below quirk, and we won't assume
> + * that devices without the quirk can use 8-bit width.
> + */
> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
> + if (host->quirks & SDHCI_QUIRK_SLOT_CAN_DO_8_BITS)
> + mmc->caps |= MMC_CAP_8_BIT_DATA;
> + }
>
> if (caps & SDHCI_CAN_DO_HISPD)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d52a716..e42d7f0 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -76,7 +76,7 @@
> #define SDHCI_CTRL_ADMA1 0x08
> #define SDHCI_CTRL_ADMA32 0x10
> #define SDHCI_CTRL_ADMA64 0x18
> -#define SDHCI_CTRL_8BITBUS 0x20
> +#define SDHCI_CTRL_8BITBUS 0x20
>
> #define SDHCI_POWER_CONTROL 0x29
> #define SDHCI_POWER_ON 0x01
> @@ -155,6 +155,7 @@
> #define SDHCI_CLOCK_BASE_SHIFT 8
> #define SDHCI_MAX_BLOCK_MASK 0x00030000
> #define SDHCI_MAX_BLOCK_SHIFT 16
> +#define SDHCI_CAN_DO_8BIT 0x00040000
> #define SDHCI_CAN_DO_ADMA2 0x00080000
> #define SDHCI_CAN_DO_ADMA1 0x00100000
> #define SDHCI_CAN_DO_HISPD 0x00200000
> @@ -215,6 +216,8 @@ 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 width);
> void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> u8 power_mode);
> unsigned int (*get_ro)(struct sdhci_host *host);
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 1fdc673..7fdcfca 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -83,6 +83,8 @@ struct sdhci_host {
> #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
> /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
> #define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
> +/* Slot has 8 data pins going to eMMC/MMC card */
> +#define SDHCI_QUIRK_SLOT_CAN_DO_8_BITS (1<<30)
>
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-20 12:35 ` Wolfram Sang
2010-11-20 16:37 ` Philip Rakity
@ 2010-11-20 18:24 ` Chris Ball
2010-11-21 19:17 ` Philip Rakity
1 sibling, 1 reply; 14+ messages in thread
From: Chris Ball @ 2010-11-20 18:24 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Philip Rakity, linux-mmc@vger.kernel.org
Hi,
On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
> I know it's too late, but...
It's late, but it's not too late. I'd like to send a fix for MMC cards
to Linus within the next week. If we decide to make some small changes
here and can do it quickly, that should be fine.
> What does the platform_-prefix of the callback indicate?
That it's a hook for code (whether that's a -pltfm driver or an SDHCI
driver) that knows more about the board-level setup to implement.
> > * introduce a quirk to specify that the board designers have indeed
> > brought out all the pins for 8-bit to the slot.
>
> This is not a quirk, this is platform_data, no?
Yes, I agree that platform code would be more correct than the quirk.
Thanks,
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-20 18:24 ` Chris Ball
@ 2010-11-21 19:17 ` Philip Rakity
2010-11-22 10:16 ` zhangfei gao
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Philip Rakity @ 2010-11-21 19:17 UTC (permalink / raw)
To: Chris Ball; +Cc: Wolfram Sang, linux-mmc@vger.kernel.org
>From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Sun, 21 Nov 2010 11:08:21 -0800
Subject: [PATCH] [PATCH] sdhci: resubmit 8 bit support based on feedback from list
a) QUIRK removed for 8 bit support since platform issue - not quirk
b) platform Flag defined for sdhci-pxa.c and plat-pxa
c) comments added to sdhci.c on usage
Signed-off-by: Philip Rakity <prakity@marvell.com>
comments from previous submission
We now:
* check for a v3 controller before setting 8-bit bus width
* offer a callback for platform code to switch to 8-bit mode, which
allows non-v3 controllers to support it
* introduce a quirk to specify that the board designers have indeed
brought out all the pins for 8-bit to the slot.
We were previously relying only on whether the controller supported
8-bit, which doesn't tell us anything about the pin configuration in
the board design.
Signed-off-by: Philip Rakity <prakity@marvell.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Signed-off-by: Chris Ball <cjb@laptop.org>
---
arch/arm/plat-pxa/include/plat/sdhci.h | 3 ++
drivers/mmc/host/sdhci-pxa.c | 4 ++
drivers/mmc/host/sdhci.c | 49 ++++++++++++++++++++++++--------
drivers/mmc/host/sdhci.h | 4 ++-
4 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
index e49c5b6..e85b58f 100644
--- a/arch/arm/plat-pxa/include/plat/sdhci.h
+++ b/arch/arm/plat-pxa/include/plat/sdhci.h
@@ -17,6 +17,9 @@
/* Require clock free running */
#define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
+/* board design supports 8 bit data on SD/SDIO BUS */
+#define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2)
+
/*
* struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
* @max_speed: the maximum speed supported
diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
index fc406ac..f609f4a 100644
--- a/drivers/mmc/host/sdhci-pxa.c
+++ b/drivers/mmc/host/sdhci-pxa.c
@@ -141,6 +141,10 @@ static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
if (pdata->quirks)
host->quirks |= pdata->quirks;
+ /* if slot design supports 8 bit data - indicate this */
+ if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
+ host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+
ret = sdhci_add_host(host);
if (ret) {
dev_err(&pdev->dev, "failed to add host\n");
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 154cbf8..03c801b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
if (host->ops->platform_send_init_74_clocks)
host->ops->platform_send_init_74_clocks(host, ios->power_mode);
- ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
-
- if (ios->bus_width == MMC_BUS_WIDTH_8)
- ctrl |= SDHCI_CTRL_8BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_8BITBUS;
+ /*
+ * 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().
+ */
+ if (host->ops->platform_8bit_width)
+ host->ops->platform_8bit_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;
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl |= SDHCI_CTRL_8BITBUS;
+ } else {
+ if (host->version >= SDHCI_SPEC_300)
+ ctrl &= ~SDHCI_CTRL_8BITBUS;
+ if (ios->bus_width == MMC_BUS_WIDTH_4)
+ ctrl |= SDHCI_CTRL_4BITBUS;
+ else
+ ctrl &= ~SDHCI_CTRL_4BITBUS;
+ }
+ sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+ }
- if (ios->bus_width == MMC_BUS_WIDTH_4)
- ctrl |= SDHCI_CTRL_4BITBUS;
- else
- ctrl &= ~SDHCI_CTRL_4BITBUS;
+ ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
if ((ios->timing == MMC_TIMING_SD_HS ||
ios->timing == MMC_TIMING_MMC_HS)
@@ -1855,11 +1869,22 @@ int sdhci_add_host(struct sdhci_host *host)
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
else
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
+
mmc->f_max = host->max_clk;
mmc->caps |= MMC_CAP_SDIO_IRQ;
- if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
- mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
+ /*
+ * A controller may support 8-bit width, but the board itself
+ * might not have the pins brought out. So, boards that support
+ * 8-bit width should set the MMC_CAP_8_BIT_DATA and we won't assume
+ * that devices without the quirk can use 8-bit width.
+ *
+ * set mmc->caps |= MMC_CAP_8_BIT_DATA in platfrom code
+ * before call for_add_host to enable 8 bit support
+ */
+ if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
+ mmc->caps |= MMC_CAP_4_BIT_DATA;
+ }
if (caps & SDHCI_CAN_DO_HISPD)
mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d52a716..ff18eaa 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -76,7 +76,7 @@
#define SDHCI_CTRL_ADMA1 0x08
#define SDHCI_CTRL_ADMA32 0x10
#define SDHCI_CTRL_ADMA64 0x18
-#define SDHCI_CTRL_8BITBUS 0x20
+#define SDHCI_CTRL_8BITBUS 0x20
#define SDHCI_POWER_CONTROL 0x29
#define SDHCI_POWER_ON 0x01
@@ -215,6 +215,8 @@ 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 width);
void (*platform_send_init_74_clocks)(struct sdhci_host *host,
u8 power_mode);
unsigned int (*get_ro)(struct sdhci_host *host);
--
1.6.0.4
On Nov 20, 2010, at 10:24 AM, Chris Ball wrote:
> Hi,
>
> On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
>> I know it's too late, but...
>
> It's late, but it's not too late. I'd like to send a fix for MMC cards
> to Linus within the next week. If we decide to make some small changes
> here and can do it quickly, that should be fine.
>
>> What does the platform_-prefix of the callback indicate?
>
> That it's a hook for code (whether that's a -pltfm driver or an SDHCI
> driver) that knows more about the board-level setup to implement.
>
>>> * introduce a quirk to specify that the board designers have indeed
>>> brought out all the pins for 8-bit to the slot.
>>
>> This is not a quirk, this is platform_data, no?
>
> Yes, I agree that platform code would be more correct than the quirk.
>
> Thanks,
>
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-21 19:17 ` Philip Rakity
@ 2010-11-22 10:16 ` zhangfei gao
2010-11-22 10:36 ` zhangfei gao
2010-11-24 17:35 ` Chris Ball
2 siblings, 0 replies; 14+ messages in thread
From: zhangfei gao @ 2010-11-22 10:16 UTC (permalink / raw)
To: Philip Rakity; +Cc: Chris Ball, Wolfram Sang, linux-mmc@vger.kernel.org
On Mon, Nov 22, 2010 at 3:17 AM, Philip Rakity <prakity@marvell.com> wrote:
> From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
> From: Philip Rakity <prakity@marvell.com>
> Date: Sun, 21 Nov 2010 11:08:21 -0800
> Subject: [PATCH] [PATCH] sdhci: resubmit 8 bit support based on feedback from list
>
> a) QUIRK removed for 8 bit support since platform issue - not quirk
> b) platform Flag defined for sdhci-pxa.c and plat-pxa
> c) comments added to sdhci.c on usage
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
>
> comments from previous submission
>
> We now:
> * check for a v3 controller before setting 8-bit bus width
> * offer a callback for platform code to switch to 8-bit mode, which
> allows non-v3 controllers to support it
> * introduce a quirk to specify that the board designers have indeed
> brought out all the pins for 8-bit to the slot.
>
> We were previously relying only on whether the controller supported
> 8-bit, which doesn't tell us anything about the pin configuration in
> the board design.
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
> arch/arm/plat-pxa/include/plat/sdhci.h | 3 ++
> drivers/mmc/host/sdhci-pxa.c | 4 ++
> drivers/mmc/host/sdhci.c | 49 ++++++++++++++++++++++++--------
> drivers/mmc/host/sdhci.h | 4 ++-
> 4 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
> index e49c5b6..e85b58f 100644
> --- a/arch/arm/plat-pxa/include/plat/sdhci.h
> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
> @@ -17,6 +17,9 @@
> /* Require clock free running */
> #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
>
> +/* board design supports 8 bit data on SD/SDIO BUS */
> +#define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2)
> +
> /*
> * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
> * @max_speed: the maximum speed supported
> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> index fc406ac..f609f4a 100644
> --- a/drivers/mmc/host/sdhci-pxa.c
> +++ b/drivers/mmc/host/sdhci-pxa.c
> @@ -141,6 +141,10 @@ static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
> if (pdata->quirks)
> host->quirks |= pdata->quirks;
>
> + /* if slot design supports 8 bit data - indicate this */
> + if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +
> ret = sdhci_add_host(host);
> if (ret) {
> dev_err(&pdev->dev, "failed to add host\n");
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 154cbf8..03c801b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (host->ops->platform_send_init_74_clocks)
> host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> -
> - if (ios->bus_width == MMC_BUS_WIDTH_8)
> - ctrl |= SDHCI_CTRL_8BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_8BITBUS;
> + /*
> + * 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().
> + */
> + if (host->ops->platform_8bit_width)
> + host->ops->platform_8bit_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;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (ios->bus_width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
>
> - if (ios->bus_width == MMC_BUS_WIDTH_4)
> - ctrl |= SDHCI_CTRL_4BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_4BITBUS;
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> if ((ios->timing == MMC_TIMING_SD_HS ||
> ios->timing == MMC_TIMING_MMC_HS)
> @@ -1855,11 +1869,22 @@ int sdhci_add_host(struct sdhci_host *host)
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
> else
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> +
> mmc->f_max = host->max_clk;
> mmc->caps |= MMC_CAP_SDIO_IRQ;
>
> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> + /*
> + * A controller may support 8-bit width, but the board itself
> + * might not have the pins brought out. So, boards that support
> + * 8-bit width should set the MMC_CAP_8_BIT_DATA and we won't assume
> + * that devices without the quirk can use 8-bit width.
> + *
> + * set mmc->caps |= MMC_CAP_8_BIT_DATA in platfrom code
> + * before call for_add_host to enable 8 bit support
> + */
> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
> + }
>
> if (caps & SDHCI_CAN_DO_HISPD)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d52a716..ff18eaa 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -76,7 +76,7 @@
> #define SDHCI_CTRL_ADMA1 0x08
> #define SDHCI_CTRL_ADMA32 0x10
> #define SDHCI_CTRL_ADMA64 0x18
> -#define SDHCI_CTRL_8BITBUS 0x20
> +#define SDHCI_CTRL_8BITBUS 0x20
>
> #define SDHCI_POWER_CONTROL 0x29
> #define SDHCI_POWER_ON 0x01
> @@ -215,6 +215,8 @@ 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 width);
> void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> u8 power_mode);
> unsigned int (*get_ro)(struct sdhci_host *host);
> --
> 1.6.0.4
>
> On Nov 20, 2010, at 10:24 AM, Chris Ball wrote:
>
>> Hi,
>>
>> On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
>>> I know it's too late, but...
>>
>> It's late, but it's not too late. I'd like to send a fix for MMC cards
>> to Linus within the next week. If we decide to make some small changes
>> here and can do it quickly, that should be fine.
>>
>>> What does the platform_-prefix of the callback indicate?
>>
>> That it's a hook for code (whether that's a -pltfm driver or an SDHCI
>> driver) that knows more about the board-level setup to implement.
>>
>>>> * introduce a quirk to specify that the board designers have indeed
>>>> brought out all the pins for 8-bit to the slot.
>>>
>>> This is not a quirk, this is platform_data, no?
>>
>> Yes, I agree that platform code would be more correct than the quirk.
>>
>> Thanks,
>>
>> --
>> Chris Ball <cjb@laptop.org> <http://printf.net/>
>> One Laptop Per Child
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi, Chris
Philip has update a new patch without new quirk.
I think the patch applied mmc-next should be reverted, since
SDHCI_QUIRK_SLOT_CAN_DO_8_BITS is not needed at all.
Thanks
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-21 19:17 ` Philip Rakity
2010-11-22 10:16 ` zhangfei gao
@ 2010-11-22 10:36 ` zhangfei gao
2010-11-22 16:13 ` Philip Rakity
2010-11-24 17:35 ` Chris Ball
2 siblings, 1 reply; 14+ messages in thread
From: zhangfei gao @ 2010-11-22 10:36 UTC (permalink / raw)
To: Philip Rakity; +Cc: Chris Ball, Wolfram Sang, linux-mmc@vger.kernel.org
On Mon, Nov 22, 2010 at 3:17 AM, Philip Rakity <prakity@marvell.com> wrote:
> From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
> From: Philip Rakity <prakity@marvell.com>
> Date: Sun, 21 Nov 2010 11:08:21 -0800
> Subject: [PATCH] [PATCH] sdhci: resubmit 8 bit support based on feedback from list
>
> a) QUIRK removed for 8 bit support since platform issue - not quirk
> b) platform Flag defined for sdhci-pxa.c and plat-pxa
> c) comments added to sdhci.c on usage
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
>
> comments from previous submission
>
> We now:
> * check for a v3 controller before setting 8-bit bus width
> * offer a callback for platform code to switch to 8-bit mode, which
> allows non-v3 controllers to support it
> * introduce a quirk to specify that the board designers have indeed
> brought out all the pins for 8-bit to the slot.
>
> We were previously relying only on whether the controller supported
> 8-bit, which doesn't tell us anything about the pin configuration in
> the board design.
>
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
> arch/arm/plat-pxa/include/plat/sdhci.h | 3 ++
> drivers/mmc/host/sdhci-pxa.c | 4 ++
> drivers/mmc/host/sdhci.c | 49 ++++++++++++++++++++++++--------
> drivers/mmc/host/sdhci.h | 4 ++-
> 4 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
> index e49c5b6..e85b58f 100644
> --- a/arch/arm/plat-pxa/include/plat/sdhci.h
> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
> @@ -17,6 +17,9 @@
> /* Require clock free running */
> #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
>
> +/* board design supports 8 bit data on SD/SDIO BUS */
> +#define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2)
> +
> /*
> * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
> * @max_speed: the maximum speed supported
> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> index fc406ac..f609f4a 100644
> --- a/drivers/mmc/host/sdhci-pxa.c
> +++ b/drivers/mmc/host/sdhci-pxa.c
> @@ -141,6 +141,10 @@ static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
> if (pdata->quirks)
> host->quirks |= pdata->quirks;
>
> + /* if slot design supports 8 bit data - indicate this */
> + if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +
> ret = sdhci_add_host(host);
how about
/* if BOARD NOT support 8 BIT */
if (pdata->flags & PXA_FLAG_SD_CANT_8_BIT)
host->mmc->caps &= ~MMC_CAP_8_BIT_DATA;
> if (ret) {
> dev_err(&pdev->dev, "failed to add host\n");
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 154cbf8..03c801b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (host->ops->platform_send_init_74_clocks)
> host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> -
> - if (ios->bus_width == MMC_BUS_WIDTH_8)
> - ctrl |= SDHCI_CTRL_8BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_8BITBUS;
> + /*
> + * 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().
> + */
> + if (host->ops->platform_8bit_width)
> + host->ops->platform_8bit_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;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (ios->bus_width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
>
> - if (ios->bus_width == MMC_BUS_WIDTH_4)
> - ctrl |= SDHCI_CTRL_4BITBUS;
> - else
> - ctrl &= ~SDHCI_CTRL_4BITBUS;
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
> if ((ios->timing == MMC_TIMING_SD_HS ||
> ios->timing == MMC_TIMING_MMC_HS)
> @@ -1855,11 +1869,22 @@ int sdhci_add_host(struct sdhci_host *host)
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
> else
> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> +
> mmc->f_max = host->max_clk;
> mmc->caps |= MMC_CAP_SDIO_IRQ;
>
> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> + /*
> + * A controller may support 8-bit width, but the board itself
> + * might not have the pins brought out. So, boards that support
> + * 8-bit width should set the MMC_CAP_8_BIT_DATA and we won't assume
> + * that devices without the quirk can use 8-bit width.
> + *
> + * set mmc->caps |= MMC_CAP_8_BIT_DATA in platfrom code
> + * before call for_add_host to enable 8 bit support
> + */
> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
> + }
i am afraid this will impact all platform supporting 8 bit emmc, if
this logic is correct, why assume board could support 4-bit width.
i think sdhci.c should describe controller behavior, instead of board
specific behavior.
>
> if (caps & SDHCI_CAN_DO_HISPD)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d52a716..ff18eaa 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -76,7 +76,7 @@
> #define SDHCI_CTRL_ADMA1 0x08
> #define SDHCI_CTRL_ADMA32 0x10
> #define SDHCI_CTRL_ADMA64 0x18
> -#define SDHCI_CTRL_8BITBUS 0x20
> +#define SDHCI_CTRL_8BITBUS 0x20
>
> #define SDHCI_POWER_CONTROL 0x29
> #define SDHCI_POWER_ON 0x01
> @@ -215,6 +215,8 @@ 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 width);
> void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> u8 power_mode);
> unsigned int (*get_ro)(struct sdhci_host *host);
> --
> 1.6.0.4
>
> On Nov 20, 2010, at 10:24 AM, Chris Ball wrote:
>
>> Hi,
>>
>> On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
>>> I know it's too late, but...
>>
>> It's late, but it's not too late. I'd like to send a fix for MMC cards
>> to Linus within the next week. If we decide to make some small changes
>> here and can do it quickly, that should be fine.
>>
>>> What does the platform_-prefix of the callback indicate?
>>
>> That it's a hook for code (whether that's a -pltfm driver or an SDHCI
>> driver) that knows more about the board-level setup to implement.
>>
>>>> * introduce a quirk to specify that the board designers have indeed
>>>> brought out all the pins for 8-bit to the slot.
>>>
>>> This is not a quirk, this is platform_data, no?
>>
>> Yes, I agree that platform code would be more correct than the quirk.
>>
>> Thanks,
>>
>> --
>> Chris Ball <cjb@laptop.org> <http://printf.net/>
>> One Laptop Per Child
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-22 10:36 ` zhangfei gao
@ 2010-11-22 16:13 ` Philip Rakity
2010-11-23 2:45 ` zhangfei gao
0 siblings, 1 reply; 14+ messages in thread
From: Philip Rakity @ 2010-11-22 16:13 UTC (permalink / raw)
To: zhangfei gao; +Cc: Chris Ball, Wolfram Sang, linux-mmc@vger.kernel.org
On Nov 22, 2010, at 2:36 AM, zhangfei gao wrote:
> On Mon, Nov 22, 2010 at 3:17 AM, Philip Rakity <prakity@marvell.com> wrote:
>> From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
>> From: Philip Rakity <prakity@marvell.com>
>> Date: Sun, 21 Nov 2010 11:08:21 -0800
>> Subject: [PATCH] [PATCH] sdhci: resubmit 8 bit support based on feedback from list
>>
>> a) QUIRK removed for 8 bit support since platform issue - not quirk
>> b) platform Flag defined for sdhci-pxa.c and plat-pxa
>> c) comments added to sdhci.c on usage
>>
>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>>
>> comments from previous submission
>>
>> We now:
>> * check for a v3 controller before setting 8-bit bus width
>> * offer a callback for platform code to switch to 8-bit mode, which
>> allows non-v3 controllers to support it
>> * introduce a quirk to specify that the board designers have indeed
>> brought out all the pins for 8-bit to the slot.
>>
>> We were previously relying only on whether the controller supported
>> 8-bit, which doesn't tell us anything about the pin configuration in
>> the board design.
>>
>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Signed-off-by: Chris Ball <cjb@laptop.org>
>> ---
>> arch/arm/plat-pxa/include/plat/sdhci.h | 3 ++
>> drivers/mmc/host/sdhci-pxa.c | 4 ++
>> drivers/mmc/host/sdhci.c | 49 ++++++++++++++++++++++++--------
>> drivers/mmc/host/sdhci.h | 4 ++-
>> 4 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
>> index e49c5b6..e85b58f 100644
>> --- a/arch/arm/plat-pxa/include/plat/sdhci.h
>> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
>> @@ -17,6 +17,9 @@
>> /* Require clock free running */
>> #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
>>
>> +/* board design supports 8 bit data on SD/SDIO BUS */
>> +#define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2)
>> +
>> /*
>> * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
>> * @max_speed: the maximum speed supported
>> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
>> index fc406ac..f609f4a 100644
>> --- a/drivers/mmc/host/sdhci-pxa.c
>> +++ b/drivers/mmc/host/sdhci-pxa.c
>> @@ -141,6 +141,10 @@ static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
>> if (pdata->quirks)
>> host->quirks |= pdata->quirks;
>>
>> + /* if slot design supports 8 bit data - indicate this */
>> + if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
>> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
>> +
>> ret = sdhci_add_host(host);
>
> how about
> /* if BOARD NOT support 8 BIT */
> if (pdata->flags & PXA_FLAG_SD_CANT_8_BIT)
> host->mmc->caps &= ~MMC_CAP_8_BIT_DATA;
A number of folks have found that enabling 8 bit by default breaks existing drivers.
Having it enabled by default is not a good idea. New features should enabled by the
platform code and not enabled by default.
>
>
>> if (ret) {
>> dev_err(&pdev->dev, "failed to add host\n");
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 154cbf8..03c801b 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> if (host->ops->platform_send_init_74_clocks)
>> host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>>
>> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> -
>> - if (ios->bus_width == MMC_BUS_WIDTH_8)
>> - ctrl |= SDHCI_CTRL_8BITBUS;
>> - else
>> - ctrl &= ~SDHCI_CTRL_8BITBUS;
>> + /*
>> + * 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().
>> + */
>> + if (host->ops->platform_8bit_width)
>> + host->ops->platform_8bit_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;
>> + if (host->version >= SDHCI_SPEC_300)
>> + ctrl |= SDHCI_CTRL_8BITBUS;
>> + } else {
>> + if (host->version >= SDHCI_SPEC_300)
>> + ctrl &= ~SDHCI_CTRL_8BITBUS;
>> + if (ios->bus_width == MMC_BUS_WIDTH_4)
>> + ctrl |= SDHCI_CTRL_4BITBUS;
>> + else
>> + ctrl &= ~SDHCI_CTRL_4BITBUS;
>> + }
>> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> + }
>>
>> - if (ios->bus_width == MMC_BUS_WIDTH_4)
>> - ctrl |= SDHCI_CTRL_4BITBUS;
>> - else
>> - ctrl &= ~SDHCI_CTRL_4BITBUS;
>> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>>
>> if ((ios->timing == MMC_TIMING_SD_HS ||
>> ios->timing == MMC_TIMING_MMC_HS)
>> @@ -1855,11 +1869,22 @@ int sdhci_add_host(struct sdhci_host *host)
>> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
>> else
>> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
>> +
>> mmc->f_max = host->max_clk;
>> mmc->caps |= MMC_CAP_SDIO_IRQ;
>>
>> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
>> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
>> + /*
>> + * A controller may support 8-bit width, but the board itself
>> + * might not have the pins brought out. So, boards that support
>> + * 8-bit width should set the MMC_CAP_8_BIT_DATA and we won't assume
>> + * that devices without the quirk can use 8-bit width.
>> + *
>> + * set mmc->caps |= MMC_CAP_8_BIT_DATA in platfrom code
>> + * before call for_add_host to enable 8 bit support
>> + */
>> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
>> + mmc->caps |= MMC_CAP_4_BIT_DATA;
>> + }
>
> i am afraid this will impact all platform supporting 8 bit emmc, if
> this logic is correct, why assume board could support 4-bit width.
> i think sdhci.c should describe controller behavior, instead of board
> specific behavior.
Concur about 4 bit support and said this on the mailing list earlier. The original code by Pierre
assumes 4 bit bus width is always available.
sdhci.c should only assume 1 bit mode works and the platform code should enable 4 and 8 bit support.
Did not want to change the original assumptions as it impacts lots of platform code. If the list thinks this
is a good idea I am open to preparing the patch to do this.
8 bit support is new -- I have not seen the impact that you are referring too.
>
>>
>> if (caps & SDHCI_CAN_DO_HISPD)
>> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index d52a716..ff18eaa 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -76,7 +76,7 @@
>> #define SDHCI_CTRL_ADMA1 0x08
>> #define SDHCI_CTRL_ADMA32 0x10
>> #define SDHCI_CTRL_ADMA64 0x18
>> -#define SDHCI_CTRL_8BITBUS 0x20
>> +#define SDHCI_CTRL_8BITBUS 0x20
>>
>> #define SDHCI_POWER_CONTROL 0x29
>> #define SDHCI_POWER_ON 0x01
>> @@ -215,6 +215,8 @@ 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 width);
>> void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>> u8 power_mode);
>> unsigned int (*get_ro)(struct sdhci_host *host);
>> --
>> 1.6.0.4
>>
>> On Nov 20, 2010, at 10:24 AM, Chris Ball wrote:
>>
>>> Hi,
>>>
>>> On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
>>>> I know it's too late, but...
>>>
>>> It's late, but it's not too late. I'd like to send a fix for MMC cards
>>> to Linus within the next week. If we decide to make some small changes
>>> here and can do it quickly, that should be fine.
>>>
>>>> What does the platform_-prefix of the callback indicate?
>>>
>>> That it's a hook for code (whether that's a -pltfm driver or an SDHCI
>>> driver) that knows more about the board-level setup to implement.
>>>
>>>>> * introduce a quirk to specify that the board designers have indeed
>>>>> brought out all the pins for 8-bit to the slot.
>>>>
>>>> This is not a quirk, this is platform_data, no?
>>>
>>> Yes, I agree that platform code would be more correct than the quirk.
>>>
>>> Thanks,
>>>
>>> --
>>> Chris Ball <cjb@laptop.org> <http://printf.net/>
>>> One Laptop Per Child
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-22 16:13 ` Philip Rakity
@ 2010-11-23 2:45 ` zhangfei gao
0 siblings, 0 replies; 14+ messages in thread
From: zhangfei gao @ 2010-11-23 2:45 UTC (permalink / raw)
To: Philip Rakity; +Cc: Chris Ball, Wolfram Sang, linux-mmc@vger.kernel.org
On Mon, Nov 22, 2010 at 11:13 AM, Philip Rakity <prakity@marvell.com> wrote:
>
> On Nov 22, 2010, at 2:36 AM, zhangfei gao wrote:
>
>> On Mon, Nov 22, 2010 at 3:17 AM, Philip Rakity <prakity@marvell.com> wrote:
>>> From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
>>> From: Philip Rakity <prakity@marvell.com>
>>> Date: Sun, 21 Nov 2010 11:08:21 -0800
>>> Subject: [PATCH] [PATCH] sdhci: resubmit 8 bit support based on feedback from list
>>>
>>> a) QUIRK removed for 8 bit support since platform issue - not quirk
>>> b) platform Flag defined for sdhci-pxa.c and plat-pxa
>>> c) comments added to sdhci.c on usage
>>>
>>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>>>
>>> comments from previous submission
>>>
>>> We now:
>>> * check for a v3 controller before setting 8-bit bus width
>>> * offer a callback for platform code to switch to 8-bit mode, which
>>> allows non-v3 controllers to support it
>>> * introduce a quirk to specify that the board designers have indeed
>>> brought out all the pins for 8-bit to the slot.
>>>
>>> We were previously relying only on whether the controller supported
>>> 8-bit, which doesn't tell us anything about the pin configuration in
>>> the board design.
>>>
>>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>>> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>>> Signed-off-by: Chris Ball <cjb@laptop.org>
>>> ---
>>> arch/arm/plat-pxa/include/plat/sdhci.h | 3 ++
>>> drivers/mmc/host/sdhci-pxa.c | 4 ++
>>> drivers/mmc/host/sdhci.c | 49 ++++++++++++++++++++++++--------
>>> drivers/mmc/host/sdhci.h | 4 ++-
>>> 4 files changed, 47 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
>>> index e49c5b6..e85b58f 100644
>>> --- a/arch/arm/plat-pxa/include/plat/sdhci.h
>>> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
>>> @@ -17,6 +17,9 @@
>>> /* Require clock free running */
>>> #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
>>>
>>> +/* board design supports 8 bit data on SD/SDIO BUS */
>>> +#define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2)
>>> +
>>> /*
>>> * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
>>> * @max_speed: the maximum speed supported
>>> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
>>> index fc406ac..f609f4a 100644
>>> --- a/drivers/mmc/host/sdhci-pxa.c
>>> +++ b/drivers/mmc/host/sdhci-pxa.c
>>> @@ -141,6 +141,10 @@ static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
>>> if (pdata->quirks)
>>> host->quirks |= pdata->quirks;
>>>
>>> + /* if slot design supports 8 bit data - indicate this */
>>> + if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
>>> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
>>> +
>>> ret = sdhci_add_host(host);
>>
>> how about
>> /* if BOARD NOT support 8 BIT */
>> if (pdata->flags & PXA_FLAG_SD_CANT_8_BIT)
>> host->mmc->caps &= ~MMC_CAP_8_BIT_DATA;
>
>
> A number of folks have found that enabling 8 bit by default breaks existing drivers.
> Having it enabled by default is not a good idea. New features should enabled by the
> platform code and not enabled by default.
>
>>
>>
>>> if (ret) {
>>> dev_err(&pdev->dev, "failed to add host\n");
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 154cbf8..03c801b 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1185,17 +1185,31 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>> if (host->ops->platform_send_init_74_clocks)
>>> host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>>>
>>> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>>> -
>>> - if (ios->bus_width == MMC_BUS_WIDTH_8)
>>> - ctrl |= SDHCI_CTRL_8BITBUS;
>>> - else
>>> - ctrl &= ~SDHCI_CTRL_8BITBUS;
>>> + /*
>>> + * 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().
>>> + */
>>> + if (host->ops->platform_8bit_width)
>>> + host->ops->platform_8bit_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;
>>> + if (host->version >= SDHCI_SPEC_300)
>>> + ctrl |= SDHCI_CTRL_8BITBUS;
>>> + } else {
>>> + if (host->version >= SDHCI_SPEC_300)
>>> + ctrl &= ~SDHCI_CTRL_8BITBUS;
>>> + if (ios->bus_width == MMC_BUS_WIDTH_4)
>>> + ctrl |= SDHCI_CTRL_4BITBUS;
>>> + else
>>> + ctrl &= ~SDHCI_CTRL_4BITBUS;
>>> + }
>>> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>>> + }
>>>
>>> - if (ios->bus_width == MMC_BUS_WIDTH_4)
>>> - ctrl |= SDHCI_CTRL_4BITBUS;
>>> - else
>>> - ctrl &= ~SDHCI_CTRL_4BITBUS;
>>> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>>>
>>> if ((ios->timing == MMC_TIMING_SD_HS ||
>>> ios->timing == MMC_TIMING_MMC_HS)
>>> @@ -1855,11 +1869,22 @@ int sdhci_add_host(struct sdhci_host *host)
>>> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_300;
>>> else
>>> mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
>>> +
>>> mmc->f_max = host->max_clk;
>>> mmc->caps |= MMC_CAP_SDIO_IRQ;
>>>
>>> - if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
>>> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
>>> + /*
>>> + * A controller may support 8-bit width, but the board itself
>>> + * might not have the pins brought out. So, boards that support
>>> + * 8-bit width should set the MMC_CAP_8_BIT_DATA and we won't assume
>>> + * that devices without the quirk can use 8-bit width.
>>> + *
>>> + * set mmc->caps |= MMC_CAP_8_BIT_DATA in platfrom code
>>> + * before call for_add_host to enable 8 bit support
>>> + */
>>> + if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA)) {
>>> + mmc->caps |= MMC_CAP_4_BIT_DATA;
>>> + }
>>
>> i am afraid this will impact all platform supporting 8 bit emmc, if
>> this logic is correct, why assume board could support 4-bit width.
>> i think sdhci.c should describe controller behavior, instead of board
>> specific behavior.
>
>
> Concur about 4 bit support and said this on the mailing list earlier. The original code by Pierre
> assumes 4 bit bus width is always available.
>
> sdhci.c should only assume 1 bit mode works and the platform code should enable 4 and 8 bit support.
Disagree, i think there is no assumption.
sdhci.c describe the capability of the controller aligning to sdh
standard, instead of board limitation.
if (caps & SDHCI_CAN_DO_8BIT)
mmc->caps |= SDHCI_CAN_DO_8BIT;
While platfrom specific driver add platfrom limitation, including
specific controller limitation and board limitation.
Though counting on platfrom driver add "mmc->caps |=
MMC_CAP_8_BIT_DATA;" is also workable.
> Did not want to change the original assumptions as it impacts lots of platform code. If the list thinks this
> is a good idea I am open to preparing the patch to do this.
>
> 8 bit support is new -- I have not seen the impact that you are referring too.
>
>
>>
>>>
>>> if (caps & SDHCI_CAN_DO_HISPD)
>>> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index d52a716..ff18eaa 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -76,7 +76,7 @@
>>> #define SDHCI_CTRL_ADMA1 0x08
>>> #define SDHCI_CTRL_ADMA32 0x10
>>> #define SDHCI_CTRL_ADMA64 0x18
>>> -#define SDHCI_CTRL_8BITBUS 0x20
>>> +#define SDHCI_CTRL_8BITBUS 0x20
>>>
>>> #define SDHCI_POWER_CONTROL 0x29
>>> #define SDHCI_POWER_ON 0x01
>>> @@ -215,6 +215,8 @@ 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 width);
>>> void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>>> u8 power_mode);
>>> unsigned int (*get_ro)(struct sdhci_host *host);
>>> --
>>> 1.6.0.4
>>>
>>> On Nov 20, 2010, at 10:24 AM, Chris Ball wrote:
>>>
>>>> Hi,
>>>>
>>>> On Sat, Nov 20, 2010 at 01:35:53PM +0100, Wolfram Sang wrote:
>>>>> I know it's too late, but...
>>>>
>>>> It's late, but it's not too late. I'd like to send a fix for MMC cards
>>>> to Linus within the next week. If we decide to make some small changes
>>>> here and can do it quickly, that should be fine.
>>>>
>>>>> What does the platform_-prefix of the callback indicate?
>>>>
>>>> That it's a hook for code (whether that's a -pltfm driver or an SDHCI
>>>> driver) that knows more about the board-level setup to implement.
>>>>
>>>>>> * introduce a quirk to specify that the board designers have indeed
>>>>>> brought out all the pins for 8-bit to the slot.
>>>>>
>>>>> This is not a quirk, this is platform_data, no?
>>>>
>>>> Yes, I agree that platform code would be more correct than the quirk.
>>>>
>>>> Thanks,
>>>>
>>>> --
>>>> Chris Ball <cjb@laptop.org> <http://printf.net/>
>>>> One Laptop Per Child
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] sdhci: 8 bit bus width changes
2010-11-21 19:17 ` Philip Rakity
2010-11-22 10:16 ` zhangfei gao
2010-11-22 10:36 ` zhangfei gao
@ 2010-11-24 17:35 ` Chris Ball
2 siblings, 0 replies; 14+ messages in thread
From: Chris Ball @ 2010-11-24 17:35 UTC (permalink / raw)
To: Philip Rakity; +Cc: Wolfram Sang, linux-mmc
Hi Philip,
On Sun, Nov 21, 2010 at 11:17:09AM -0800, Philip Rakity wrote:
> a) QUIRK removed for 8 bit support since platform issue - not quirk
> b) platform Flag defined for sdhci-pxa.c and plat-pxa
> c) comments added to sdhci.c on usage
Thanks very much, I'll plan on sending this to Linus tomorrow, along
with the rest of my .37 queue, unless there are any more review
comments from anyone.
http://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commit;h=15ec44611904be0dcc97b84c29fbf964e5e2b36f
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-11-24 17:35 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-01 22:45 [RFC] sdhci: 8 bit bus width changes Philip Rakity
2010-11-05 7:13 ` Peppe CAVALLARO
2010-11-19 21:40 ` Chris Ball
2010-11-19 21:53 ` Chris Ball
2010-11-20 12:35 ` Wolfram Sang
2010-11-20 16:37 ` Philip Rakity
2010-11-20 18:24 ` Chris Ball
2010-11-21 19:17 ` Philip Rakity
2010-11-22 10:16 ` zhangfei gao
2010-11-22 10:36 ` zhangfei gao
2010-11-22 16:13 ` Philip Rakity
2010-11-23 2:45 ` zhangfei gao
2010-11-24 17:35 ` Chris Ball
2010-11-20 16:37 ` Philip Rakity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox