From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andy Fleming <afleming@gmail.com>
Date: Fri, 3 May 2013 17:35:11 -0500
Subject: [U-Boot] [PATCH v3 06/11] mmc: update the Faraday FTSDC010
 driver to fix performance issue
In-Reply-To: <1366963360-2987-7-git-send-email-dantesu@gmail.com>
References: <1366277139-29728-2-git-send-email-dantesu@gmail.com>
	<1366963360-2987-1-git-send-email-dantesu@gmail.com>
	<1366963360-2987-7-git-send-email-dantesu@gmail.com>
Message-ID: <CAKWjMd4mYxuh1+138bGoSzqFFW-XZxnRyHfLaZXV1PGXqpbKdg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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?



> +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?



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

Andy