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

[Macpaul Lin]

Hi Andy

2011/11/29 Andy Fleming 

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

-- 
Best regards,
Macpaul Lin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Andy Fleming]

On Mon, Nov 28, 2011 at 2:07 AM, Macpaul Lin  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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Macpaul Lin]

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.


-- 
Best regards,
Macpaul Lin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Andy Fleming]

On Thu, Nov 17, 2011 at 3:34 AM, Macpaul Lin  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 
> ---
> 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 */
> +                      

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

[Macpaul Lin]

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 
---
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(-)

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