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

[Mike Frysinger]

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.

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

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

> >> +      * 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 ...
}
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110502/c210f689/attachment.pgp