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

[Andy Fleming]

On Mon, Nov 28, 2011 at 2:07 AM, Macpaul Lin <macpaul at 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.