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

[Simon Glass]

Hi Mike,

On Thu, Oct 20, 2011 at 1:03 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> 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 ...

Thanks for the comments.

OK I will need to implement this. There will be a short delay....

Regards,
Simon

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