Libertas with if_spi on pxa270

[Andrey Yurovsky]

On Tue, Jan 27, 2009 at 10:50 AM, Dan Williams <dcbw at redhat.com> 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.

Yeah, it's likely a CPU architecture / DMA issue (alignment for
transfers between system memory and the controller) so it will be
different for ARM vs. Blackfin, etc.

>> 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.
>
> 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.

4-byte alignment is the 'minimum' required for the card's firmware.
For what it's worth, I tried swapping it out for 8-byte alignment as
Mike used and it didn't have any negative (or positive) side effects
on the Blackfin, and it of course still works with the firmware.  I
suppose that we could use 8-byte alignment as a more general case but
I agree that it would be better to solve this in the controller code
if possible.

  -Andrey