Re: [PATCH 3/5] dw_spi: rework message processing

[Grant Likely]

On Thu, Jun 16, 2011 at 11:28 AM, Dirk Brandewie
<dirk.brandewie@xxxxxxxxx> wrote:
> On 06/16/2011 07:00 AM, Feng Tang wrote:
>>
>> Hi Dirk,
>>
>> IMHO, the patch is too big, it contains too many changes to the original
>> drivers, and we can't see clearly what you've changed to each logical
>> code part or section, If possible, could you separate this patch to
>> several small ones in a logical way.
>>
>> First, I have some questions, what devices have you tested with this
>> patch?
>> high speed, low speed? Do you have any performance data to show the
>> benefit
>> of this change? Current dw_spi driver has been tested with many devices,
>> so
>> to not break them or cause obvious regression, we have to be cautious.
>
> See Thread with grant for list of environments where it has been tested.
> The boot time of the platforms it is being used on decreased 2-5 seconds
> with no regressions reported.  It has been in use/under test for ~3 months
> in various Meego and Android builds.  It clears all the bugs reported
> against the driver that I am aware of.  If you can give me pointers to
> teams/projects that are using v2.6.37+ kernels I would be more that happy to
> provide them with patches for their kernel to ensure we get the most
> comprehensive test possible.
>
>>
>> Here are some general comments according to the commit logs:
>> 1. I think the threaded irq handling is a good idea. And let driver chose
>> to
>>    use poll or interrupt is good, some other spi controller driver has
>> used
>>    that way for a long time
>> 2. Why you remove the cs_change code, in some case, the controller is only
>>    be used by one device, we don't need do the config for every single
>>    spi_transfer
>
> There is no guarantee that all the transfers in a given spi_message have the
> same values for  speed_hz, bits_per_word, cs_change and delay_usecs atleast
> nothing I could find put that restriction in place. Since we need to deal
> with possible changes (although unlikely) it gets rid some state we need to
> maintain and makes the code path common for all transfers.
>
>> 3. Why do you remove the chip select control code, dw_spi controller hw
>> has
>>    some problem in chip select controller by SER, and thus many devices
>> has
>>    to use external GPIO has their chip select, this is real world usages!
>
> Which devices/platforms are you referring to?  I was unable to find any
> platforms or client drivers using this functionality. If they are not public
> please respond internally.  In any case it is mute since I already agreed to
> put is back in in my response to Grant.
>
>> 4. I saw you enable both TX/RX interrupt when doing interrupt transfer,
>> spi
>>    devices' TX/RX are born to be simutaneous, when one word is sent over
>>    TX line, a RX word will be received from RX line, so both the orignal
>>    interrupt transfer handling written by me and the later optimization
>>    from Alek Du only enable TX interrupt, which will only generate half of
>>    IRQs comparing to enble both TX/RX, this is huge when the data rate is
>>    several Mb per second
>
> I the current driver the txfltr register is set to 0 (FIFO empty) in the
> interrupt transfer case which will drop chip select every FIFO length bytes.
>
> In my transfer setup the RX FIFO interrupt is set to a value lower than the
> TX threshold which will keep both interrupts from firing at the same time.
>
> The TX interrupt will drive the transfer until there are less than
> tx_threshold bytes left to transfer then by the RX interrupt to drive the
> remainder of the transfer without dropping chip select.

Be careful here.  Can you guarantee that the kernel will process the
IRQ before the FIFO drains?  If not, then you'll need something more
reliable.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/