Re: [PATCH v5 1/1] davinci: spi: replace existing driver

[Michael Williamson <michael.williamson-wZX4cNJlHJ2sVWG7oymsAA@public.gmane.org>]

On 10/11/2010 12:57 PM, Nori, Sekhar wrote:
> Hi Mike,
> 
> On Sat, Oct 09, 2010 at 18:25:54, Michael Williamson wrote:
>> On 10/08/2010 03:22 PM, Michael Williamson wrote:
> 
>>> On 10/6/2010 11:37 AM, Nori, Sekhar wrote:
> 
>>>> On Mon, Sep 20, 2010 at 23:12:53, Michael Williamson wrote:
> 
>>>>
>>>> I just finished pushing the DMA related patches to the git branch[1].
>>>> I have not tested yet, but feel free to give it a go (maybe I will get
>>>> lucky again!).
>>>>
>>>
>>>
>>> I gave it a go for our platform.  The good news is, polled and interrupt driven mode still work!
>>> The bad news is that DMA mode hangs up the kernel on the first read attempt, stalled waiting
>>> for the transfer completion notification (hmmm.... I think this was a spot where things were
>>> tweaked a bit? :) )
>>>
>>> I'll see if I can narrow it down some more if you don't get to it first.
>>>
>>
>> I think I found it.  This patch (below, and on [2]) got it working for me. The Rx DMA
>> size on transfer requests with no receive buffer provided needs to match the requested
>> size (and the transmit size), not the size of the temporary buffer.  All good as the
>> DMA address increment is 0 in this case.
>>
>> I'll try to do some more testing next week, but so far so good.
> 
> Sorry, I couldn’t get to testing even today. Should be on it
> tomorrow.
> 
>>
>> ---
>> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
>> index 662ebbe..8206df1 100755
>> --- a/drivers/spi/davinci_spi.c
>> +++ b/drivers/spi/davinci_spi.c
>> @@ -632,13 +632,11 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>>                * source address never increments.
>>                */
>>
>> -             if (t->rx_buf) {
>> +             rx_buf_count = davinci_spi->rcount;
>> +             if (t->rx_buf)
>>                       rx_buf = t->rx_buf;
>> -                     rx_buf_count = davinci_spi->rcount;
>> -             } else {
>> +             else
>>                       rx_buf = davinci_spi->rx_tmp_buf;
>> -                     rx_buf_count = sizeof(davinci_spi->rx_tmp_buf);
>> -             }
>>
>>               t->rx_dma = dma_map_single(&spi->dev, rx_buf, rx_buf_count,
>>                                                       DMA_FROM_DEVICE);
> 
> Hmm, looks like the Tx and Rx DMA must always run for the same duration. I was
> under the impression Tx is required to keep the clock running, and so must always
> run for the length of transfer requested, but Rx DMA can finish earlier.
> 

That may be true.  The scenario I was seeing was a requested Tx length of 1 with no
Rx buffer supplied.  The length of the internal buffer was bigger than 1, so the resulting
Rx DMA size in the a_b_cnt was greater than the Tx size, and it stalled.

> I guess replacing the line:
> 
>                 param.a_b_cnt = rx_buf_count << 16 | data_type;
> 
> with
> 
>                 param.a_b_cnt = davinci_spi->rcount << 16 | data_type;
> 
> will also fix the issue?
> 

A better fix.  should be fine.

>>
>>>
>>>> Also, do you have patches adding SPI support for DA850 platform?
>>>> I can include these patches on this branch so others who will be
>>>> testing don\x19t have to repeat the work.
>>>>
>>>
>>>
>>> I cloned your repository at [1] and published the additions needed to made to test it on the
>>> mitydsp-l138 platform (da850 based) at [2] (never mind the UART one at the end).  I had to add
>>> clock support for the spi devices and define the platform SPI resources/registration routines.
>>> I'd appreciate a quick peek at those to make sure that I didn't make any errors.  The spi
>>> registration may be refactorable (sp?) to support da830, I didn't look to closely at that.
>>>
>>> If any of it is usable, great.
> 
> Sure it is! I checked-in your patches with some modifications to my
> branch. The most notable change being usage of da8xx instead of da850 since
> the same code should work on da830 as well. Apart from this there are
> some cosmetic changes (patches squashed, some non-relevant hunks dropped
> etc). Do update the patches in your tree if you are OK with the changes.
> 

OK.  I'll try to rebase off your tree and retest.  

-Mike
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source