[U-Boot] [PATCH] ftsdc010: add support of ftsdc010 mmc controller

[Andy Fleming]

On Fri, Jul 8, 2011 at 3:01 AM, Macpaul Lin <macpaul at gmail.com> wrote:
> + */
> +static struct mmc mmc_dev[CONFIG_FTSDC010_NUMBER];
> +static struct mmc_host mmc_host[CONFIG_FTSDC010_NUMBER];

Probably should have less generic names for all of these.

> +
> +static struct ftsdc010_mmc *ftsdc010_get_base_mmc(int dev_index)
> +{
> +       unsigned long offset = dev_index * sizeof(struct ftsdc010_mmc);
> +       return (struct ftsdc010_mmc *)(CONFIG_FTSDC010_BASE + offset);

This might be clearer as straightforward pointer math:

return (struct ftsdc010_mmc *)(CONFIG_FTSDC010_BASE) + dev_index;


> +
> +static void mmc_pio_read(struct mmc_host *host, char *buf, unsigned int size)

This is a very generic name


> +
> +static int mmc_pio_check_status(struct mmc *mmc, struct mmc_cmd *cmd,
> +                       struct mmc_data *data)
> +{
> +       struct mmc_host *host = (struct mmc_host *)mmc->priv;

This cast is unnecessary. A void * can be cast to another pointer type
without a cast, and that is the preferred style.

> +
> +       unsigned int sta, clear;
> +       unsigned int i;
> +
> +       /* check response and hardware status */
> +       clear = 0;
> +
> +       /* chech CMD_SEND */
> +       for (i = 0; i < FTSDC010_CMD_RETRY; i++) {
> +               sta = readl(&host->reg->status);
> +               /* Command Complete */
> +               if (sta & FTSDC010_STATUS_CMD_SEND) {
> +                       if (!data)
> +                               clear |= FTSDC010_CLR_CMD_SEND;
> +                       break;
> +               }
> +       }
> +
> +       if (i > FTSDC010_CMD_RETRY) {
> +               printf("%s: send command timeout\n", __func__);
> +               return TIMEOUT;
> +       }
> +
> +       /* debug: print status register and command index*/
> +       debug("sta: %08x cmd %d\n", sta, cmd->cmdidx);
> +
> +       /* handle data FIFO */
> +       if ((sta & FTSDC010_STATUS_FIFO_ORUN) ||
> +               (sta & FTSDC010_STATUS_FIFO_URUN)) {
> +
> +               /* Wrong DATA FIFO Flag */
> +               if (data == NULL)
> +                       printf("%s, data fifo wrong: sta: %08x cmd %d\n",
> +                               __func__, sta, cmd->cmdidx);
> +
> +               if (sta & FTSDC010_STATUS_FIFO_ORUN)
> +                       clear |= FTSDC010_STATUS_FIFO_ORUN;
> +               if (sta & FTSDC010_STATUS_FIFO_URUN)
> +                       clear |= FTSDC010_STATUS_FIFO_URUN;
> +       }
> +
> +       /* check RSP TIMEOUT or FAIL */
> +       if (sta & FTSDC010_STATUS_RSP_TIMEOUT) {
> +               /* RSP TIMEOUT */
> +               debug("%s: RSP timeout: sta: %08x cmd %d\n",
> +                               __func__, sta, cmd->cmdidx);
> +
> +               clear |= FTSDC010_CLR_RSP_TIMEOUT;
> +               writel(clear, &host->reg->clr);
> +
> +               return TIMEOUT;
> +       } else if (sta & FTSDC010_STATUS_RSP_CRC_FAIL) {
> +               /* clear response fail bit */
> +               debug("%s: RSP CRC FAIL: sta: %08x cmd %d\n",
> +                               __func__, sta, cmd->cmdidx);
> +
> +               clear |= FTSDC010_CLR_RSP_CRC_FAIL;
> +               writel(clear, &host->reg->clr);
> +
> +               return -1;
> +       } else if (sta & FTSDC010_STATUS_RSP_CRC_OK) {
> +
> +               /* clear response CRC OK bit */
> +               clear |= FTSDC010_CLR_RSP_CRC_OK;
> +       }
> +
> +       /* check DATA TIMEOUT or FAIL */
> +       if (data) {
> +               if (sta & FTSDC010_STATUS_DATA_TIMEOUT) {
> +                       /* DATA TIMEOUT */
> +                       debug("%s: DATA TIMEOUT: sta: %08x\n",
> +                                       __func__, sta);
> +
> +                       clear |= FTSDC010_STATUS_DATA_TIMEOUT;
> +                       writel(sta, &host->reg->clr);
> +                       return TIMEOUT;
> +               } else if (sta & FTSDC010_STATUS_DATA_CRC_FAIL) {
> +                       /* Error Interrupt */
> +                       debug("%s: DATA CRC FAIL: sta: %08x\n",
> +                                       __func__, sta);
> +
> +                       clear |= FTSDC010_STATUS_DATA_CRC_FAIL;
> +                       writel(clear, &host->reg->clr);
> +
> +                       return -1;
> +               } else if (sta & FTSDC010_STATUS_DATA_END) {
> +                       /* Transfer Complete */
> +                       clear |= FTSDC010_STATUS_DATA_END;
> +               }
> +       }
> +
> +       /* transaction is success and clear status register */
> +       writel(sta, &host->reg->clr);


I'm confused by the use of the "clear" variable. Some of the if
clauses are totally useless, because "clear" is never read after being
written. And some of the users of clear don't clear all of the status
bits. Are you going to come back around and read the bits you didn't
clear? If it's always safe to write "sta" to "clr", then why not just
do that every time, and remove the "clear" variable?


> +
> +       return 0;
> +}
> +
> +static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> +                       struct mmc_data *data)


This name actually conflicts with the global mmc framework's command
name.  It would be much better if you named it something like
ftsdc010_send_cmd()



> +static int ftsdc010_setup_data(struct mmc *mmc, struct mmc_data *data)
> +{
> +       struct mmc_host *host = (struct mmc_host *)mmc->priv;
> +       unsigned int dcon, newmask;
> +
> +       /* configure data transfer paramter */
> +       if (!data)
> +               return 0;
> +
> +       if (((data->blocksize - 1) & data->blocksize) != 0) {
> +               printf("%s: can't do non-power-of 2 sized block transfers"
> +                       " (blksz %d)\n", __func__, data->blocksize);
> +               return -1;
> +       }
> +
> +       if (data->blocksize <= 2) {
> +               /* We cannot deal with unaligned blocks with more than
> +                * one block being transfered. */
> +
> +               if (data->blocks > 1) {
> +                       printf("%s: can't do non-word sized block transfers"
> +                               " (blksz %d)\n", __func__, data->blocksize);
> +                       return -1;
> +               }
> +       }

I'd put these two conditions in one if statement (if (size <= 2) &&
(blocks > 1)).



Make name more specific to this controller


> +
> +static void mmc_reset(struct mmc_host *host)

Change the name


> +static int mmc_core_init(struct mmc *mmc)

Change this name, too



> diff --git a/include/faraday/ftsdc010.h b/include/faraday/ftsdc010.h
> new file mode 100644
> index 0000000..b6e44a8
> --- /dev/null
> +++ b/include/faraday/ftsdc010.h

> +struct mmc_host {

Change this name

Andy