Hi Mike,
On Thu, Oct 20, 2011 at 1:03 PM, Mike Frysinger <vap...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
