On Mon, Nov 22, 2010 at 3:17 AM, Philip Rakity <prakity@xxxxxxxxxxx> wrote:
> From d52213a33a71142134555793583b726501827827 Mon Sep 17 00:00:00 2001
> From: Philip Rakity <prakity@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Tested-by: Giuseppe Cavallaro <peppe.cavallaro@xxxxxx>
> Signed-off-by: Chris Ball <cjb@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx> <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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
