On Mon, Nov 22, 2010 at 11:13 AM, Philip Rakity <prakity@xxxxxxxxxxx> wrote:
>
> On Nov 22, 2010, at 2:36 AM, zhangfei gao wrote:
>
>> 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;
>
>
> 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@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
