Mike, On Mon, May 2, 2011 at 2:26 PM, Mike Frysinger <vap...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot