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

Kuo-Jung Su
2013-05-06T06:44:48Z

2013/5/4 Andy Fleming <afleming@gmail.com>:
>
>
>
> On Fri, Apr 26, 2013 at 3:02 AM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>>
>> From: Kuo-Jung Su <dantesu@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@faraday-tech.com>
>> CC: Andy Fleming <afleming@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