[U-Boot] [PATCH v3 06/11] mmc: update the Faraday FTSDC010 driver to fix performance issue

[Kuo-Jung Su]

2013/5/4 Andy Fleming <afleming at gmail.com>:
>
>
>
> On Fri, Apr 26, 2013 at 3:02 AM, Kuo-Jung Su <dantesu at gmail.com> wrote:
>>
>> From: Kuo-Jung Su <dantesu at faraday-tech.com>
>>
>> Faraday FTSDC010 is a MMC/SD host controller.
>> Although there is already a driver in current u-boot release,
>> which is modified from eSHDC and contributed by Andes Tech.
>>
>> Its performance is too terrible on Faraday A36x SoC platforms,
>> so I turn to implement this new version of driver which is
>> 10+ times faster than the old one.
>>
>> It's carefully designed to be compatible to Andes chips,
>> so it should be safe to replace it.
>>
>> Signed-off-by: Kuo-Jung Su <dantesu at faraday-tech.com>
>> CC: Andy Fleming <afleming at gmail.com>
>>
>
>
>>
>> +
>>
>> +       if (chip->acmd) {
>> +               cmd |= FTSDC010_CMD_APP_CMD;
>> +               chip->acmd = 0;
>> +       }
>
>
> [...]
>
>>
>> +       if (ret) {
>> +               debug("ftsdc010: cmd timeout (op code=%d)\n",
>> +                       mmc_cmd->cmdidx);
>> +       } else if (mmc_cmd->cmdidx == MMC_CMD_APP_CMD) {
>> +               chip->acmd = 1;
>> +       }
>
>
>
> Interesting. I've not seen any other devices that needed to have the sub
> command of the APP command flagged as such. I can't think of a better way to
> do this right now, so it's fine. Perhaps if there were other examples, we
> could implement a separate callback for APP cmds? Or we could modify the
> higher-level code to set a flag for the subcommands? I feel like maintaining
> this kind of state shouldn't be necessary, but possibly this hardware is
> strange.
>
>
>>
>> +static void ftsdc010_set_ios(struct mmc *mmc)
>> +{
>> +       struct ftsdc010_chip *chip = mmc->priv;
>> +       struct ftsdc010_mmc __iomem *regs = chip->regs;
>> +
>> +       ftsdc010_clkset(chip, mmc->clock);
>> +
>> +       if (mmc->clock > 25000000)
>> +               setbits_le32(&regs->ccr, FTSDC010_CCR_CLK_HISPD);
>> +       else
>> +               clrbits_le32(&regs->ccr, FTSDC010_CCR_CLK_HISPD);
>
>
>
> Why not put this part in ftsdc010_clkset, and set ccr all at once?
>
>

Got it, thanks.
I'll merge it into ftsdc010_clkset().

>>
>> +int ftsdc010_mmc_init(int devid)
>> +{
>
>
> [...]
>
>>
>> +
>> +       sprintf(mmc->name, "ftsdc010");
>> +       mmc->send_cmd = ftsdc010_request;
>> +       mmc->set_ios  = ftsdc010_set_ios;
>> +       mmc->init     = ftsdc010_init;
>> +
>> +       switch ((readl(&regs->bwr) >> 3) & 3) {
>
>
>
> Could we get named constants for those mask/shift numbers?
>
>

Sure, it would be fixed in next version.

>>
>> +       case 1:
>> +               mmc->host_caps = MMC_MODE_HS | MMC_MODE_HS_52MHz
>> +                       | MMC_MODE_4BIT;
>> +               break;
>> +       case 2:
>
>
> Andy



--
Best wishes,
Kuo-Jung Su