Libertas with if_spi on pxa270

[Mike Rapoport]

Dan Williams wrote:
> On Tue, 2009-01-27 at 13:51 +0200, Mike Rapoport wrote:
>> Dan,
>>
>> Dan Williams wrote:
>>> On Mon, 2009-01-26 at 18:24 +0200, Mike Rapoport wrote:
>>>> Dan Williams wrote:
>>>>> On Mon, 2009-01-26 at 16:58 +0200, Mike Rapoport wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Have anybody tried to use 8686 with G-SPI on PXA270?
>>>>>> I've merged libertas-related changes from wireless-testing tree on top of
>>>>>> 2.6.29-rc2, added a "libertas_spi" device to my platform code, but all I could
>>>>>> get from the if_spi was "libertas: Can't read bus mode register." :(
>>>>> Have you set up the Chip Select stuff correctly for your board (see
>>>>> gpio_cs in the platform data)?  That seems to be board-dependent and
>>>>> there's not a good generic way of doing this in the current kernel
>>>>> sources.
>>>>>
>>>>> That error is the first error that will get reported when the driver
>>>>> actually tries to talk to the card, which requires that the Chip Select
>>>>> stuff be set up correctly.  If you think you've got that already set up,
>>>>> can you manually inspect whether or not the libertas device has actually
>>>>> been selected on the board?
>>>> The first thing I've done was to recheck GPIO setup and platform_data for all
>>>> SPI related stuff.
>>>> I've even inserted GPIO setup function at the very beginning of if_spi_probe.
>>>> The Marvell proprietary driver works well on the same board and exactly same
>>>> GPIOs setup.
>>> Ok, Colin McCabe will probably need to get involved here then since he
>>> wrote those bits :)
>> You were right about possible problems with my board setup. The 8686 requires
>> SPI clock pin to be high at power up to start with G-SPI rather than SDIO. That
>> part was missing from my board setup.
>> Still I needed to alter the if_spi.c to make it work. First of all, I think it
>> makes sence to add
>>
>> 	spi->bits_per_word = 16;
>> 	spi_setup(spi);
>>
>> at the beginning of if_spi_probe.
> 
> Is that a restriction of/optimization for the SPI controller?  See below
> if it is...  It seems like the SPI layer in the kernel has a bit of
> maturation to do.
> 
>> Another thing, it's possible worth adding ability to call platform specific
>> init/exit functions supplied with libertas_spi_platform_data.
>>
>> After the driver begun to work I've noted that it's performance is not as good
>> as I expected. The reason for it is that PXA SPI requires buffers to be 8-byte
>> aligned to use DMA. And, indeed, changing alignment from 4 to 8 throughout the
>> if_spi.c gave me x2 boost in large file transfer rate. I've thought about adding
>> a LIBERTAS_SPI_DMA_ALIGN option to drivers/net/wireless/Kconfig to allow
>> platforms to select required DMA alignment.
> 
> The alignment issue is a controller specific issue and should be quirked
> in the SPI controller code (where the restriction really lies),
> hopefully not in the libertas code itself.

Currently PXA SPI controller driver reverts to PIO mode when the buffer it gets
is not 8-byte aligned, so from it's point of view there's no issue here :)
It's possible to alter the SPI controller driver to use it's own properly
aligned buffers for DMA and copy the data it gets from the client into these
buffers. However, this approach also impacts overall performance.
Yet another possibility is to add something like 'get_dma_alignment' to SPI
layer and use the returned value for buffers alignment, but if_spi_packet uses
compile-time alignment attribute and I can't see how is it possible to make it
dynamic.

I think that since the G-SPI interface is used only on embedded systems and the
kernel configuration is created by the system vendor, adding a config option
that sets DMA alignment is not that bad after all.

> Colin must/may have had a reason to only use 4-byte alignment; perhaps
> his controller requires it.  But that re-inforces the point that if the
> controller has the quirks, they should be handled in the controller code
> as much as possible.
> 
> Dan
> 
> 

-- 
Sincerely yours,
Mike.