Re: [PATCH] davinci: MMC: Adding support for 8bit MMC cards

[Kevin Hilman <khilman@deeprootsystems.com>]

Sergei Shtylyov <sshtylyov@ru.mvista.com> writes:

> Hello.
>
> Vipin Bhandari wrote:
>
>   [Re-replying to all, as I only replied to Vipin the first time.]
>
>> This patch adds the support for 8bit MMC cards. The controller
>> data width is configurable depending on the wires setting in the
>> platform data structure.
>>
>> MMC 8bit is tested on OMAPL137 and MMC 4bit is tested on OMAPL138 EVM.
>>
>> Signed-off-by: Vipin Bhandari <vipin.bhandari@ti.com>
>>   
>
>   This has been done by MV in the internal tree but as you code is
> significantly differenet from that one, I'm not asking you about the
> missing MV signoffs...
>
>> diff --git a/drivers/mmc/host/davinci_mmc.c
>> b/drivers/mmc/host/davinci_mmc.c
>> index dd45e7c..3bd0ba2 100644
>> --- a/drivers/mmc/host/davinci_mmc.c
>> +++ b/drivers/mmc/host/davinci_mmc.c
>> @@ -73,6 +73,7 @@
>>  /* DAVINCI_MMCCTL definitions */
>>  #define MMCCTL_DATRST         (1 << 0)
>>  #define MMCCTL_CMDRST         (1 << 1)
>> +#define MMCCTL_WIDTH_8_BIT    (1 << 8)
>>  #define MMCCTL_WIDTH_4_BIT    (1 << 2)
>>  #define MMCCTL_DATEG_DISABLED (0 << 6)
>>  #define MMCCTL_DATEG_RISING   (1 << 6)
>> @@ -791,22 +792,42 @@ static void calculate_clk_divider(struct
>> mmc_host *mmc, struct mmc_ios *ios)
>>   static void mmc_davinci_set_ios(struct mmc_host *mmc, struct
>> mmc_ios *ios)
>>  {
>> -    unsigned int mmc_pclk = 0;
>>      struct mmc_davinci_host *host = mmc_priv(mmc);
>>  -    mmc_pclk = host->mmc_input_clk;
>>      dev_dbg(mmc_dev(host->mmc),
>>          "clock %dHz busmode %d powermode %d Vdd %04x\n",
>>          ios->clock, ios->bus_mode, ios->power_mode,
>>          ios->vdd);
>> -    if (ios->bus_width == MMC_BUS_WIDTH_4) {
>> -        dev_dbg(mmc_dev(host->mmc), "Enabling 4 bit mode\n");
>> -        writel(readl(host->base + DAVINCI_MMCCTL) | MMCCTL_WIDTH_4_BIT,
>> -            host->base + DAVINCI_MMCCTL);
>> -    } else {
>> -        dev_dbg(mmc_dev(host->mmc), "Disabling 4 bit mode\n");
>> -        writel(readl(host->base + DAVINCI_MMCCTL) & ~MMCCTL_WIDTH_4_BIT,
>> +
>> +    switch (ios->bus_width) {
>> +    case MMC_BUS_WIDTH_8:
>> +        dev_dbg(mmc_dev(host->mmc), "Enabling 8 bit mode\n");
>> +        writel((readl(host->base + DAVINCI_MMCCTL) &
>> +            ~MMCCTL_WIDTH_4_BIT) | MMCCTL_WIDTH_8_BIT,
>>              host->base + DAVINCI_MMCCTL);
>> +        break;
>> +    case MMC_BUS_WIDTH_4:
>> +        dev_dbg(mmc_dev(host->mmc), "Enabling 4 bit mode\n");
>> +        if (host->version == MMC_CTLR_VERSION_2)
>> +            writel((readl(host->base + DAVINCI_MMCCTL) &
>> +                ~MMCCTL_WIDTH_8_BIT) | MMCCTL_WIDTH_4_BIT,
>> +                host->base + DAVINCI_MMCCTL);
>> +        else
>> +            writel(readl(host->base + DAVINCI_MMCCTL) |
>> +                MMCCTL_WIDTH_4_BIT,
>> +                host->base + DAVINCI_MMCCTL);
>>   
>
>   I don't think it makes sense to check for host->version just to not
> clear the bit which is reserved for original DaVinci. There's nothing
> criminal in clearing a reserved bit, so I'm suggesting that you remove
> the check.

There's nothing criminal... yet... until some new version of the
controller uses that bit for something else.  I think it's always best
to not touch reserved bits unless there is a good, well tested, reason
not to, such as performance etc.

Kevin


>> +        break;
>> +    case MMC_BUS_WIDTH_1:
>> +        dev_dbg(mmc_dev(host->mmc), "Enabling 1 bit mode\n");
>> +        if (host->version == MMC_CTLR_VERSION_2)
>> +            writel(readl(host->base + DAVINCI_MMCCTL) &
>> +                ~(MMCCTL_WIDTH_8_BIT | MMCCTL_WIDTH_4_BIT),
>> +                host->base + DAVINCI_MMCCTL);
>> +        else
>> +            writel(readl(host->base + DAVINCI_MMCCTL) &
>> +                ~MMCCTL_WIDTH_4_BIT,
>> +                host->base + DAVINCI_MMCCTL);
>>   
>
>   Same comment here...
>
>> +        break;
>>   
>
>   It'll result less code if you read/write the register only once -- 
> before/after the *switch* statement respectively.
>
>>      }
>>       calculate_clk_divider(mmc, ios);
>> @@ -1189,10 +1210,14 @@ static int __init davinci_mmcsd_probe(struct
>> platform_device *pdev)
>>       /* REVISIT:  someday, support IRQ-driven card detection.  */
>>      mmc->caps |= MMC_CAP_NEEDS_POLL;
>> +    mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>>   
>
>    Does this flag have to do with the bus width at all? :-/
>
> WBR, Sergei
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/