Mike, On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger <vap...@gentoo.org> wrote: > On Thursday, April 28, 2011 10:55:13 Tom Warren wrote: >> Signed-off-by: Tom Warren <twar...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot