[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