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

[Tom Warren]

Mike,

On Mon, May 2, 2011 at 2:26 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Monday, May 02, 2011 11:33:24 Tom Warren wrote:
>> On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger wrote:
>> > On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
>> >> 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.
>
> do you plan on writing a driver for the SLINK logic ?  would it be a new file,
> or would you keep the two integrated ?  if diff files, i'd name this one
> "tegra2_sflash_spi.c".  if you keep them merged, then "tegra2_spi.c" is fine.
No plan to write SLINK drivers for U-Boot.  Seaboard has a touchscreen
on SPI1, but U-Boot doesn't care about that.
So I'll keep it tegra2_spi.c for now.

>
>> 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.
>
> generally, processor drivers are not concerned with board issues.  if you want
> to document board-specific issues somewhere, add a doc/README.<boardname>.
>
Good idea. I'll gen one up for Seaboard.

>> >> --- a/arch/arm/lib/board.c
>> >> +++ b/arch/arm/lib/board.c
>> >> +#if defined(CONFIG_CMD_SPI)
>>
>> 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.
>
> i dont think you want to do this.  other than the style changes, and needing a
> sep changeset, having the arm board.c file call spi_init() is desirable.
OK. I'll fix up arm board.c with your comments in mind and put it in a
separate patch.

>
>> >> +      * 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.
>
> you're writing a driver for the Tegra2 SoC.  that is the only thing this
> driver should be doing.  that means no board-specific logic is allowed.
>
>> 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.).
>
> right, this is a board-specific issue.  thus it doesnt belong in a driver that
> is only concerned with the Tegra2 SoC.
>
> what you could do is:
> void spi_init_common(void) { ... only tegra2 code ... }
> void spi_init(void) __attribute__((weak, alias("spi_init_common")));
>
> and then in the seaboard directory, add:
> void spi_init(void)
> {
>        ... do random gpio/portmux stuff ...
>        spi_init_common();
> }
>
> or another style would be to:
> extern void spi_board_init(void) __weak;
> void spi_init(void)
> {
>        if (spi_board_init())
>                spi_board_init();
>        ... only tegra2 code ...
> }
>
> and then in the seaboard directory, add:
> void spi_board_init(void)
> {
>        ... do random gpio/portmux stuff ...
> }

I'll try out these ideas. Thanks.

Tom
> -mike
>