[U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

[Tom Warren]

Mike,

On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
>> Signed-off-by: Tom Warren <twarren at nvidia.com>
>> ---
>> This patch adds support for the SPIFLASH peripheral on Tegra2 (SPI 1).
>> Probe, erase, read and write are all supported, as well as low-level
>> SPI commands via 'sspi'.
>>
>> Note that, at this time, only Seaboard has a SPI flash part (Winbond).
>> With this code, I've written U-Boot to SPI from within U-Boot, and
>> booted the system from that SPI image (boot sel jumpers must be set
>> to SPI [1000], and the UART ENABLE jumper must be set to GPIO CTRL).
>
> so this is a driver for a SPI bus, and the board you're working on just
> happens to have some SPI flashes connected to it ?  then the "SPIFLASH" part
> is irrelevant, as is the board in question.  please remove references to both
> and just refer to this as "a SPI driver for Tegra2 processors".
No, there are 4 general purpose SPI interfaces in Tegra2 (SLINK), and
1 dedicated SPI FLASH controller. From the Tegra2 TRM:

"The Serial Flash interface (SFLASH) Controller interfaces Tegra 2 to
SPI-capable devices such as Flash
memories and ADC/DAC devices. Tegra 2 supports only master mode of SPI
operation on this interface.
This interface is specifically intended for serial flash and similar
devices. For a general purpose SPI
interface, use the SLINK serial interface."

The register sets are similar, but not identical.

As to the board reference, it's in my comments so people wishing to
use this will know that only Seaboard is available w/a SPI chip -
there's no provision for that on the Harmony board.

>
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-tegra2/tegra2_spi.h
>
> if the only thing that uses this is the spi driver, then please put this in
> drivers/spi/tegra2_spi.h
Will do.
>
>> +#define      SPI_CMD_GO              (1 << 30)
>
> dont put a tab after the #define
>
OK. I'll fix 'em up.

>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>
> this change needs to be split out into a dedicated one
>
>> -
>
> this new line should not be deleted
>
>> +#if defined(CONFIG_CMD_SPI)
>> +#include <spi.h>
>> +#endif
>
> no need to protect the inclusion.  just always include it.
OK.
>
>>  #endif
>> -
>>  #if defined(CONFIG_CMD_NAND)
>
> nor should this newline be deleted
>
>>       puts ("NAND:  ");
>>       nand_init();            /* go init the NAND */
>>  #endif
>> -
>> +#if defined(CONFIG_CMD_SPI)
>
> nor this newline
I'm going to remove these changes from arm/lib/board.c and move
spi_init() to our common board file (nvidia/common/board.c.

>
>> +int spi_claim_bus(struct spi_slave *slave)
>> +{
>> +     /* Move bulk of spi_init code here? */
>
> yes, so do that
>
OK, I'll look into that.

>> +void spi_release_bus(struct spi_slave *slave)
>> +{
>
> this func should be disabling the spi bus
OK, I'll look into that, too.

>
>> +void spi_cs_activate(struct spi_slave *slave)
>> +{
>> +     struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE;
>> +     struct spi_tegra *spi = (struct spi_tegra *)TEGRA2_SPI_BASE;
>> +     u32 val;
>> +
>> +     /*
>> +      * Delay here to clean up comms - spurious chars seen around SPI xfers.
>> +      * Fine-tune later.
>> +      */
>> +     udelay(1000);
>
> fine tune now ?
The comment went in during development, and the 1000 values is the
final fine-tuned value (1ms). So I'll change the comment.

>
>> +     /*
>> +      * On Seaboard, MOSI/MISO are shared w/UART.
>> +      * Use GPIO I3 (UART_DISABLE) to tristate UART during SPI activity.
>> +      * Enable UART later (cs_deactivate) so we can use it for U-Boot comms.
>> +      */
>> +     gpio_direction_output(UART_DISABLE_GPIO, 1);
>
> board specific issues shouldnt be in a processor driver.
Please explain what you mean by a processor driver.  This does need an
ifdef, so any future board that doesn't have Seaboard's defiency of a
muxed SPIFLASH/UART won't have to mess with a GPIO, but for Seaboard,
we have to dynamically change GPIO_PI3 every SPI transaction to ensure
UART spew can continue (in DEBUG builds, etc.).
>
>> +     int numBytes = (bitlen + 7) / 8;
>
> no camel case.  use "num_bytes".
Sure, will do. This came over from the driver I ported from.

>
>> +                                     for (i = 0; i < bytes; ++i) {
>> +                                             ((u8 *)din)[i] = (tmpdin >> 24);
>
> create a dedicated pointer and deference that.  casts on the lhs are poor
> style.
Again, this was in the driver source I used as a porting reference.
I'll change it.

>
>> --- a/include/configs/seaboard.h
>> +++ b/include/configs/seaboard.h
>
> this should be split out into a dedicated patch
OK.

Thanks,

Tom
> -mike
>