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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
