From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuo-Jung Su Date: Mon, 6 May 2013 14:44:48 +0800 Subject: [U-Boot] [PATCH v3 06/11] mmc: update the Faraday FTSDC010 driver to fix performance issue In-Reply-To: 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: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 2013/5/4 Andy Fleming : > > > > On Fri, Apr 26, 2013 at 3:02 AM, Kuo-Jung Su wrote: >> >> From: Kuo-Jung Su >> >> 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 >> CC: Andy Fleming >> > > >> >> + >> >> + 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(®s->ccr, FTSDC010_CCR_CLK_HISPD); >> + else >> + clrbits_le32(®s->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(®s->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