diff mbox

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

Message ID 1321522466-22222-1-git-send-email-macpaul@andestech.com
State Changes Requested
Delegated to: Andy Fleming
Headers show

Commit Message

Macpaul Lin Nov. 17, 2011, 9:34 a.m. UTC
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@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(-)

Comments

Andy Fleming Nov. 26, 2011, 12:07 a.m. UTC | #1
On Thu, Nov 17, 2011 at 3:34 AM, Macpaul Lin <macpaul@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@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
Macpaul Lin Nov. 28, 2011, 8:07 a.m. UTC | #2
Hi Andy,

> 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.
> > -               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.
>
>
Thanks. I missed this modification to be marked in the change log.


>
> >        /* 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;
>

 Okay, I'll fix it in patch v4.


> 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.
>

The MASK bits doesn't really work at all. :-(
If I've disabled some of the interrupt flags by mask, these disabled flag
will still raise
(I think is a design flaw in hardware) if the error or success has happened.

The event (status) register is different from the clear register.
They are 2 different register with slightly different definition of their
bit fields,
these 2 registers are only partially identical of the meaning of the flags.

The flags indeed can be separate to different stage during one command
transaction.
 Actually, not all the error status bit will raise at the same time.
Some flags will only be raised exclusively.
For example, there will be only one flag raised on time for the following 3
flags,
FTSDC010_STATUS_RSP_TIMEOUT, FTSDC010_STATUS_RSP_CRC_FAIL, and
FTSDC010_STATUS_RSP_CRC_OK.
If one of the flag didn't be cleared right now, say, RSP_TIMEOUT,  the
hardware will clear it if RSP_CRC_FAIL must be
raised in the next time.


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

Why did you say the code "returns before clear is written"?
FTSDC010_STATUS_DATA_TIMEOUT and FTSDC010_STATUS_DATA_CRC_FAIL are
exclusive just like
RSP_TIMEOUT and RSP_CRC_FAIL managed by hardware.
The driver will indeed clear DATA_TIMEOUT when the flag of clear has been
set and then will be wrote into
clear register on the next line of the code "writel(sta, &host->reg->clr);",
Finally we return TIMEOUT since the DATA_TIMEOUT has been detected.

We have a COMM_ERR returned after the DATA_CRC_FAIL has been detected and
cleared also.


>  >                } 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.
>
>
The clear here is used to clear FTSDC010_STATUS_DATA_END and
FTSDC010_STATUS_DATA_CRC_OK.
Only these 2 flags are independent to other DATA related flags.
The last writel() is used to clear up these 2 DATA_END and DATA_CRC_OK if
TIMEOUT and CRC_ERR didn't happened.
Andy Fleming Nov. 28, 2011, 4:02 p.m. UTC | #3
On Mon, Nov 28, 2011 at 2:07 AM, Macpaul Lin <macpaul@gmail.com> wrote:
>> 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.
>
> The MASK bits doesn't really work at all. :-(
> If I've disabled some of the interrupt flags by mask, these disabled flag
> will still raise
> (I think is a design flaw in hardware) if the error or success has happened.
>
> The event (status) register is different from the clear register.
> They are 2 different register with slightly different definition of their
> bit fields,
> these 2 registers are only partially identical of the meaning of the flags.
>
> The flags indeed can be separate to different stage during one command
> transaction.
>  Actually, not all the error status bit will raise at the same time.
> Some flags will only be raised exclusively.
> For example, there will be only one flag raised on time for the following 3
> flags,
> FTSDC010_STATUS_RSP_TIMEOUT, FTSDC010_STATUS_RSP_CRC_FAIL, and
> FTSDC010_STATUS_RSP_CRC_OK.
> If one of the flag didn't be cleared right now, say, RSP_TIMEOUT,  the
> hardware will clear it if RSP_CRC_FAIL must be
> raised in the next time.

Alright, if you say the bits aren't all the same, and they will be
cleared by the hardware before the next interrupt, then I'll withdraw
that issue.

>
>>
>> > +
>> >                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;
>
> Why did you say the code "returns before clear is written"?

I'm saying this sets "clear" to FTSDC010_STATUS_DATA_TIMEOUT, but then
it writes "sta" to the clr register, and returns.

"clear" is never used after being set in this case.


> FTSDC010_STATUS_DATA_TIMEOUT and FTSDC010_STATUS_DATA_CRC_FAIL are exclusive
> just like
> RSP_TIMEOUT and RSP_CRC_FAIL managed by hardware.
> The driver will indeed clear DATA_TIMEOUT when the flag of clear has been
> set and then will be wrote into
> clear register on the next line of the code "writel(sta, &host->reg->clr);",
> Finally we return TIMEOUT since the DATA_TIMEOUT has been detected.
>
> We have a COMM_ERR returned after the DATA_CRC_FAIL has been detected and
> cleared also.
Macpaul Lin Nov. 29, 2011, 2:52 a.m. UTC | #4
Hi Andy

2011/11/29 Andy Fleming <afleming@gmail.com>

>  >> >
> >> >                        clear |= FTSDC010_STATUS_DATA_TIMEOUT;
> >>
> >> Why set clear? This code returns before clear is written.
> >> >                        writel(sta, &host->reg->clr);
> >> > +
> >> >                        return TIMEOUT;
> >
> > Why did you say the code "returns before clear is written"?
>
> I'm saying this sets "clear" to FTSDC010_STATUS_DATA_TIMEOUT, but then
> it writes "sta" to the clr register, and returns.
>
> "clear" is never used after being set in this case.


You're really caught a bug. Sorry I didn't aware what you meant before.
I thought I've fix this bug at the 1st version of the patch. This is weird.
Anyway, I'll fix this in patch v4 also.
Thanks!
diff mbox

Patch

diff --git a/drivers/mmc/ftsdc010_esdhc.c b/drivers/mmc/ftsdc010_esdhc.c
index e38dd87..23d21ad 100644
--- a/drivers/mmc/ftsdc010_esdhc.c
+++ b/drivers/mmc/ftsdc010_esdhc.c
@@ -90,8 +90,13 @@  static void ftsdc010_pio_read(struct mmc_host *host, char *buf, unsigned int siz
 
 	while (size) {
 		status = readl(&host->reg->status);
+		debug("%s: size: %08x\n", __func__, size);
 
 		if (status & FTSDC010_STATUS_FIFO_ORUN) {
+
+			debug("%s: FIFO OVERRUN: sta: %08x\n",
+					__func__, status);
+
 			fifo = host->fifo_len > size ?
 				size : host->fifo_len;
 
@@ -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) {
 			fifo = host->fifo_len > size ?
 				size : host->fifo_len;
 
@@ -158,7 +163,6 @@  static void ftsdc010_pio_write(struct mmc_host *host, const char *buf,
 				writel(*ptr, &host->reg->dwr);
 				ptr++;
 			}
-
 		} else {
 			udelay(1);
 			if (++retry >= FTSDC010_PIO_RETRY) {
@@ -169,56 +173,19 @@  static void ftsdc010_pio_write(struct mmc_host *host, const char *buf,
 	}
 }
 
-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;
+		}
+
 		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;
 			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);
 
-			return 0;
-		} else if (sta & FTSDC010_STATUS_DATA_END) {
-			/* Transfer Complete */
-			clear |= FTSDC010_STATUS_DATA_END;
+			return COMM_ERR;
 		}
+		writel(clear, &host->reg->clr);
 	}
-
-	/* transaction is success and clear status register */
-	writel(clear, &host->reg->clr);
-
 	return 0;
 }
 
@@ -281,6 +263,9 @@  static int ftsdc010_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	unsigned int ccon;
 	unsigned int mask, tmpmask;
 	unsigned int ret;
+	unsigned int sta, i;
+
+	ret = 0;
 
 	if (data)
 		mask = FTSDC010_INT_MASK_RSP_TIMEOUT;
@@ -290,13 +275,9 @@  static int ftsdc010_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 		mask = FTSDC010_INT_MASK_CMD_SEND;
 
 	/* write argu reg */
-	debug("%s: cmd->arg: %08x\n", __func__, cmd->cmdarg);
+	debug("%s: argu: %08x\n", __func__, host->reg->argu);
 	writel(cmd->cmdarg, &host->reg->argu);
 
-	/* setup cmd reg */
-	debug("cmd: %d\n", cmd->cmdidx);
-	debug("resp: %08x\n", cmd->resp_type);
-
 	/* setup commnad */
 	ccon = FTSDC010_CMD_IDX(cmd->cmdidx);
 
@@ -340,7 +321,51 @@  static int ftsdc010_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	/* write cmd reg */
 	debug("%s: ccon: %08x\n", __func__, ccon);
 	writel(ccon, &host->reg->cmd);
-	udelay(4*FTSDC010_DELAY_UNIT);
+
+	/* check CMD_SEND */
+	for (i = 0; i < FTSDC010_CMD_RETRY; i++) {
+		/*
+		 * If we read status register too fast
+		 * will lead hardware error and the RSP_TIMEOUT
+		 * flag will be raised incorrectly.
+		 */
+		udelay(16*FTSDC010_DELAY_UNIT);
+		sta = readl(&host->reg->status);
+
+		/* Command Complete */
+		/*
+		 * Note:
+		 *	Do not clear FTSDC010_CLR_CMD_SEND flag.
+		 *	(by writing FTSDC010_CLR_CMD_SEND bit to clear register)
+		 *	It will make the driver becomes very slow.
+		 *	If the operation hasn't been finished, hardware will
+		 *	clear this bit automatically.
+		 *	In origin, the driver will clear this flag if there is
+		 *	no data need to be read.
+		 */
+		if (sta & FTSDC010_STATUS_CMD_SEND)
+			break;
+	}
+
+	if (i > FTSDC010_CMD_RETRY) {
+		printf("%s: send command timeout\n", __func__);
+		return TIMEOUT;
+	}
+
+	/* check rsp status */
+	ret = ftsdc010_check_rsp(mmc, cmd, data);
+	if (ret)
+		return ret;
+
+	/* read response if we have RSP_OK */
+	if (ccon & FTSDC010_CMD_LONG_RSP) {
+		cmd->response[0] = readl(&host->reg->rsp3);
+		cmd->response[1] = readl(&host->reg->rsp2);
+		cmd->response[2] = readl(&host->reg->rsp1);
+		cmd->response[3] = readl(&host->reg->rsp0);
+	} else {
+		cmd->response[0] = readl(&host->reg->rsp0);
+	}
 
 	/* read/write data */
 	if (data && (data->flags & MMC_DATA_READ)) {
@@ -351,19 +376,11 @@  static int ftsdc010_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 				data->blocksize * data->blocks);
 	}
 
-	/* pio check response status */
-	ret = ftsdc010_pio_check_status(mmc, cmd, data);
-	if (!ret) {
-		/* if it is long response */
-		if (ccon & FTSDC010_CMD_LONG_RSP) {
-			cmd->response[0] = readl(&host->reg->rsp3);
-			cmd->response[1] = readl(&host->reg->rsp2);
-			cmd->response[2] = readl(&host->reg->rsp1);
-			cmd->response[3] = readl(&host->reg->rsp0);
-
-		} else {
-			cmd->response[0] = readl(&host->reg->rsp0);
-		}
+	/* check data status */
+	if (data) {
+		ret = ftsdc010_check_data(mmc, cmd, data);
+		if (ret)
+			return ret;
 	}
 
 	udelay(FTSDC010_DELAY_UNIT);
@@ -431,8 +448,6 @@  static int ftsdc010_setup_data(struct mmc *mmc, struct mmc_data *data)
 	/* always reset fifo since last transfer may fail */
 	dcon |= FTSDC010_DCR_FIFO_RST;
 
-	/* handle sdio */
-	dcon = data->blocksize | data->blocks << 15;
 	if (data->blocks > 1)
 		dcon |= FTSDC010_SDIO_CTRL1_SDIO_BLK_MODE;
 #endif
@@ -497,7 +512,7 @@  static void ftsdc010_set_clk(struct mmc *mmc)
 {
 	struct mmc_host *host = mmc->priv;
 	unsigned char clk_div;
-	unsigned char real_rate;
+	unsigned int real_rate;
 	unsigned int clock;
 
 	debug("%s: mmc_set_clock: %x\n", __func__, mmc->clock);
@@ -518,7 +533,7 @@  static void ftsdc010_set_clk(struct mmc *mmc)
 				break;
 		}
 
-		debug("%s: computed real_rete: %x, clk_div: %x\n",
+		debug("%s: computed real_rate: %x, clk_div: %x\n",
 			 __func__, real_rate, clk_div);
 
 		if (clk_div > 127)
@@ -579,6 +594,7 @@  static void ftsdc010_set_ios(struct mmc *mmc)
 static void ftsdc010_reset(struct mmc_host *host)
 {
 	unsigned int timeout;
+	unsigned int sta;
 
 	/* Do SDC_RST: Software reset for all register */
 	writel(FTSDC010_CMD_SDC_RST, &host->reg->cmd);
@@ -598,6 +614,10 @@  static void ftsdc010_reset(struct mmc_host *host)
 		timeout--;
 		udelay(10*FTSDC010_DELAY_UNIT);
 	}
+
+	sta = readl(&host->reg->status);
+	if (sta & FTSDC010_STATUS_CARD_CHANGE)
+		writel(FTSDC010_CLR_CARD_CHANGE, &host->reg->clr);
 }
 
 static int ftsdc010_core_init(struct mmc *mmc)
@@ -650,8 +670,6 @@  int ftsdc010_mmc_init(int dev_index)
 
 	mmc->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
 
-	mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
-
 	mmc->f_min = CONFIG_SYS_CLK_FREQ / 2 / (2*128);
 	mmc->f_max = CONFIG_SYS_CLK_FREQ / 2 / 2;