[U-Boot] [PATCH 3/8] tegra2: spi: Add SPI driver for SPIFLASH on Seaboard

[Mike Frysinger <vapier@gentoo.org>]

On Thursday 20 October 2011 15:03:24 Simon Glass wrote:
> This driver supports SPI on Tegra2, running at 48MHz.

the summary says "SPIFLASH" and "Seaboard".  sounds like this is a tegra2 SoC 
issue, and so driver/board specific info shouldn't be in the summary.

adding notes to the changelog as to what boards/setups have been tested is OK 
though ...

> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra2/tegra2_spi.h
>
> +struct spi_tegra {
> +	u32 command;	/* SPI_COMMAND_0 register  */
> +	u32 status;	/* SPI_STATUS_0 register */
> +	u32 rx_cmp;	/* SPI_RX_CMP_0 register  */
> +	u32 dma_ctl;	/* SPI_DMA_CTL_0 register */
> +	u32 tx_fifo;	/* SPI_TX_FIFO_0 register */
> +	u32 rsvd[3];	/* offsets 0x14 to 0x1F reserved */
> +	u32 rx_fifo;	/* SPI_RX_FIFO_0 register */
> +
> +};

don't need that newline before the closing brace

> --- /dev/null
> +++ b/drivers/spi/tegra2_spi.c
>
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +	/* Tegra2 SPI-Flash - only 1 device ('bus/cs') */
> +	if (bus > 0 && cs != 0)
> +		return 0;
> +	else
> +		return 1;
> +}
> +
> +

only need one new line here

> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +		unsigned int max_hz, unsigned int mode)
> +{
> +	struct spi_slave *slave;
> +
> +	if (!spi_cs_is_valid(bus, cs))
> +		return NULL;
> +
> +	slave = malloc(sizeof(struct spi_slave));
> +	if (!slave)
> +		return NULL;
> +
> +	slave->bus = bus;
> +	slave->cs = cs;
> +
> +	/*
> +	 * Currently, Tegra2 SFLASH uses mode 0 & a 24MHz clock.
> +	 * Use 'mode' and 'maz_hz' to change that here, if needed.
> +	 */
> +
> +	return slave;
> +}

this should be respecting hz/mode ...

> +void spi_init(void)
> +{
> +	struct spi_tegra *spi = (struct spi_tegra *)TEGRA2_SPI_BASE;
> +	u32 reg;
> +
> +	/* Change SPI clock to 48MHz, PLLP_OUT0 source */
> +	clock_start_periph_pll(PERIPH_ID_SPI1, CLOCK_ID_PERIPH, 48000000);
> +
> +	/* Clear stale status here */
> +	reg = SPI_STAT_RDY | SPI_STAT_RXF_FLUSH | SPI_STAT_TXF_FLUSH | \
> +		SPI_STAT_RXF_UNR | SPI_STAT_TXF_OVF;
> +	writel(reg, &spi->status);
> +	debug("spi_init: STATUS = %08x\n", readl(&spi->status));
> +
> +	/*
> +	 * Use sw-controlled CS, so we can clock in data after ReadID, etc.
> +	 */
> +	reg = readl(&spi->command);
> +	writel(reg | SPI_CMD_CS_SOFT, &spi->command);
> +	debug("spi_init: COMMAND = %08x\n", readl(&spi->command));
> +
> +	/*
> +	 * SPI pins on Tegra2 are muxed - change pinmux later due to UART
> +	 * issue.
> +	 */
> +}

shouldn't this be in spi_claim_bus() ?  and configured using the values 
requested at spi_setup_slave() ?

> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void
> +		*dout, void *din, unsigned long flags)
> +{
>
> +	int num_bytes = (bitlen + 7) / 8;

if you can only handle multiples of 8 bits, then return an error.  many (most) 
of the spi buses do just that.

> +	reg = readl(&spi->command);
> +	writel((reg |= (SPI_CMD_TXEN | SPI_CMD_RXEN)), &spi->command);

ugh!  pull that "|=" operation out and move it up to the readl() line above.

> +	debug("spi_xfer: COMMAND = %08x\n", readl(&spi->command));

the debug() should use "reg" rather than readl().

> +	debug("spi_xfer: slave %u:%u dout %08X din %08X bitlen %u\n",
> +	      slave->bus, slave->cs, *(u8 *)dout, *(u8 *)din, bitlen);
> ...
> +				tmpdout = (tmpdout << 8) | ((u8 *) dout)[i];
> ...
> +						((u8 *)din)[i] =
> +							(tmpdin & 0xff);

please setup local vars with u8* type and set din/dout to that to avoid all 
these inline casts

> +		writel((reg |= SPI_CMD_GO), &spi->command);

hoist that |= operation up and out
-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/20111020/ba282c19/attachment.pgp