[U-Boot] [PATCH v3] ftsdc010: improve performance and capability

[Andy Fleming]

On Thu, Nov 17, 2011 at 3:34 AM, Macpaul Lin <macpaul at andestech.com> wrote:
> This patch improve the performance by spliting flag examination code
> in ftsdc010_send_cmd() into 3 functions.
> This patch also reordered the function which made better capability to
> some high performance cards against to the next version of ftsdc010
> hardware.
>
> Signed-off-by: Macpaul Lin <macpaul at andestech.com>
> ---
> Changes for v2:
>  - Fix the problem if we read status register too fast in FTSDC010_CMD_RETRY
>    loop. If we read status register here too fast, the hardware will report
>    RSP_TIMEOUT incorrectly.
> Changes for v3:
>  - Remove host high speed capability due to hardware limitation.
>  - Remove unused variables.
>
>  drivers/mmc/ftsdc010_esdhc.c |  184 +++++++++++++++++++++++-------------------
>  1 files changed, 101 insertions(+), 83 deletions(-)

> @@ -146,7 +151,7 @@ static void ftsdc010_pio_write(struct mmc_host *host, const char *buf,
>        while (size) {
>                status = readl(&host->reg->status);
>
> -               if (status & FTSDC010_STATUS_FIFO_ORUN) {
> +               if (status & FTSDC010_STATUS_FIFO_URUN) {


Was this a bug before? If so, it should be mentioned in the changelog
that you fixed it.


> -static int ftsdc010_pio_check_status(struct mmc *mmc, struct mmc_cmd *cmd,
> +static int ftsdc010_check_rsp(struct mmc *mmc, struct mmc_cmd *cmd,
>                        struct mmc_data *data)
>  {
>        struct mmc_host *host = mmc->priv;
> -
>        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;
> -       }
> +       sta = readl(&host->reg->status);
> +       debug("%s: sta: %08x cmd %d\n", __func__, sta, cmd->cmdidx);
>
>        /* 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);
> +               debug("%s: RSP timeout: sta: %08x\n", __func__, sta);
>
>                clear |= FTSDC010_CLR_RSP_TIMEOUT;
>                writel(clear, &host->reg->clr);
> @@ -226,47 +193,62 @@ static int ftsdc010_pio_check_status(struct mmc *mmc, struct mmc_cmd *cmd,
>                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);
> +               debug("%s: RSP CRC FAIL: sta: %08x\n", __func__, sta);
>
>                clear |= FTSDC010_CLR_RSP_CRC_FAIL;
>                writel(clear, &host->reg->clr);
>
> -               return 0;
> +               return COMM_ERR;
>        } else if (sta & FTSDC010_STATUS_RSP_CRC_OK) {
>
>                /* clear response CRC OK bit */
>                clear |= FTSDC010_CLR_RSP_CRC_OK;
>        }
>
> +       writel(clear, &host->reg->clr);
> +       return 0;
> +}
> +
> +static int ftsdc010_check_data(struct mmc *mmc, struct mmc_cmd *cmd,
> +                       struct mmc_data *data)
> +{
> +       struct mmc_host *host = mmc->priv;
> +       unsigned int sta, clear;
> +
> +       sta = readl(&host->reg->status);
> +       debug("%s: sta: %08x cmd %d\n", __func__, sta, cmd->cmdidx);
> +
>        /* check DATA TIMEOUT or FAIL */
>        if (data) {
> +               if (sta & FTSDC010_STATUS_DATA_END) {
> +                       /* Transfer Complete */
> +                       clear |= FTSDC010_STATUS_DATA_END;
> +               }
> +
> +               if (sta & FTSDC010_STATUS_DATA_CRC_OK) {
> +                       /* Data CRC_OK */
> +                       clear |= FTSDC010_STATUS_DATA_CRC_OK;
> +               }


Instead of:

if (foo) {
  /* comment */
  bar;
}

It's better, I think to do:

/* comment */
if (foo)
  bar;

However, aside from that, the interrupt clearing confuses me. Usually,
you read the event register, and then write it back to clear
it. If there is more than one error, some of the status bits will be
left uncleared. If you only want to clear the bits being dealt with in
a particular section, I think it would be clearer and safer to set
"clear" up-front based on a MASK of bits that are being dealt with in
that section.

> +
>                if (sta & FTSDC010_STATUS_DATA_TIMEOUT) {
>                        /* DATA TIMEOUT */
> -                       debug("%s: DATA TIMEOUT: sta: %08x\n",
> -                                       __func__, sta);
> +                       debug("%s: DATA TIMEOUT: sta: %08x\n", __func__, sta);
>
>                        clear |= FTSDC010_STATUS_DATA_TIMEOUT;

Why set clear? This code returns before clear is written.
>                        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);
> +                       debug("%s: DATA CRC FAIL: sta: %08x\n", __func__, sta);
>
>                        clear |= FTSDC010_STATUS_DATA_CRC_FAIL;
>                        writel(clear, &host->reg->clr);

Ok, here "clear" is actually written to the register, but doesn't it
leave open the possibility that DATA_END is still there? Maybe it
doesn't matter for this, but it seems very fragile.

Andy