[U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller

Tom
2010-01-05T14:19:19Z

Sudhakar Rajashekhara wrote:
> From: Sekhar Nori <nsekhar@ti.com>
>
> This adds a driver for the SPI controller found on davinci
> based SoCs from Texas Instruments.
>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
> ---
> From the previous version following have been modified:
> 1. Sorted the entries in drivers/spi/Makefile alphabetically.
> 2. Implemented dummy functions for spi_cs_is_valid(),
> spi_cs_activate() and spi_cs_deactivate().
> 3. Added GPL header for drivers/spi/davinci_spi.h file.
> 4. Added protection against multiple inclusion of SPI header
> file.
> 5. Replaced the macro based register offsets in SPI header
> file with structure.
> 6. Replaced the spi_readl and spi_writel functions with
> readl and writel respectively.
>
> drivers/spi/Makefile | 1 +
> drivers/spi/davinci_spi.c | 221 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/spi/davinci_spi.h | 102 ++++++++++++++++++++
> include/configs/da830evm.h | 2 +-
> 4 files changed, 325 insertions(+), 1 deletions(-)
> create mode 100644 drivers/spi/davinci_spi.c
> create mode 100644 drivers/spi/davinci_spi.h
>
<snip>
>
> new file mode 100644
> index 0000000..c3f1810
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.c
> @@ -0,0 +1,221 @@
> +/*
> + * Copyright (C) 2009 Sekhar Nori, Texas Instruments, Inc <www.ti.com>
> + *
> + * Driver for SPI controller on DaVinci. Based on atmel_spi.c
> + * by Atmel Corporation
> + *
> + * Copyright (C) 2007 Atmel Corporation
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +#include <common.h>
> +#include <spi.h>
> +#include <malloc.h>
> +
> +#include <asm/io.h>
> +
> +#include <asm/arch/hardware.h>
> +
> +#include "davinci_spi.h"
> +

Please remove the extra spaces

> +static unsigned int data1_reg_val;

Why is this static value used instead of reading
ds->regs->dat1 ?
Depending on the order of the function calling, this
value may not mirror what is in the register.

> +
> +void spi_init()
> +{
> + /* do nothing */
> +}
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> + unsigned int max_hz, unsigned int mode)
> +{
> + struct davinci_spi_slave *ds;
> +
> + ds = malloc(sizeof(struct davinci_spi_slave));
> + if (!ds)
> + return NULL;
> +
> + ds->slave.bus = bus;
> + ds->slave.cs = cs;
> + ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE;
> + ds->freq = max_hz;
> +
> + return &ds->slave;
> +}
> +
> +void spi_free_slave(struct spi_slave *slave)
> +{
> + struct davinci_spi_slave *ds = to_davinci_spi(slave);
> +
Check before you free.
It would be nice if you could poison the pointer.

> + free(ds);
> +}
> +
> +int spi_claim_bus(struct spi_slave *slave)
> +{
> + struct davinci_spi_slave *ds = to_davinci_spi(slave);
> + unsigned int scalar;
> +
> + /* Enable the SPI hardware */
> + writel(SPIGCR0_SPIRST_MASK, &ds->regs->gcr0);
> + udelay(1000);
> + writel(SPIGCR0_SPIENA_MASK, &ds->regs->gcr0);
> +
> + /* Set master mode, powered up and not activated */
> + writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, &ds->regs->gcr1);
> +
> + /* CS, CLK, SIMO and SOMI are functional pins */
> + writel((SPIPC0_EN0FUN_MASK) | (SPIPC0_CLKFUN_MASK) |
> + (SPIPC0_DOFUN_MASK) | (SPIPC0_DIFUN_MASK), &ds->regs->pc0);
> +
> + /* setup format */
> + scalar = ((CONFIG_SYS_SPI_CLK / ds->freq) - 1) & 0xFF;
> +
> + writel(8 | /* character length */
> + (scalar << SPIFMT_PRESCALE_SHIFT) |
> + /* clock signal delayed by half clk cycle */
> + (1 << SPIFMT_PHASE_SHIFT) |
> + /* clock low in idle state - Mode 0 */
> + (0 << SPIFMT_POLARITY_SHIFT) |
> + /* MSB shifted out first */
> + (0 << SPIFMT_SHIFTDIR_SHIFT), &ds->regs->fmt0);
Shifting 0's..
This could be improved
> +
> + /* hold cs active at end of transfer until explicitly de-asserted */
> + data1_reg_val = (1 << SPIDAT1_CSHOLD_SHIFT) |
> + (slave->cs << SPIDAT1_CSNR_SHIFT);
> + writel(data1_reg_val, &ds->regs->dat1);
> +
> + /*
> + * Including a minor delay. No science here. Should be good even with
> + * no delay
> + */
> + writel((50 << SPI_C2TDELAY_SHIFT) |
> + (50 << SPI_T2CDELAY_SHIFT), &ds->regs->delay);
> +
> + /* default chip select register */
> + writel(SPIDEF_CSDEF0_MASK, &ds->regs->def);
> +
> + /* no interrupts */
> + writel(0, &ds->regs->int0);
> + writel(0, &ds->regs->lvl);
> +
> + /* enable SPI */
> + writel(readl(&ds->regs->gcr1) | (SPIGCR1_SPIENA_MASK), &ds->regs->gcr1);
> +
> + return 0;
> +}
> +
> +void spi_release_bus(struct spi_slave *slave)
> +{
> + struct davinci_spi_slave *ds = to_davinci_spi(slave);
> +
> + /* Disable the SPI hardware */
> + writel(SPIGCR0_SPIRST_MASK, &ds->regs->gcr0);
> +}
> +
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> + const void *dout, void *din, unsigned long flags)
> +{
> + struct davinci_spi_slave *ds = to_davinci_spi(slave);
> + unsigned int len;
> + int ret, i;
> + const u8 *txp = dout;
> + u8 *rxp = din;
It is unclear if passing in NULL values for din and dout is normal
behaviour. Please add a comment if it is. Also break transfer loop
into a tx / rx parts.

> +
> + ret = 0;
> +
> + if (bitlen == 0)
> + /* Finish any previously submitted transfers */
> + goto out;
> +
> + /*
> + * It's not clear how non-8-bit-aligned transfers are supposed to be
> + * represented as a stream of bytes...this is a limitation of
> + * the current SPI interface - here we terminate on receiving such a
> + * transfer request.
> + */
> + if (bitlen % 8) {
> + /* Errors always terminate an ongoing transfer */
> + flags |= SPI_XFER_END;
It is unclear if you are forcing a flag state that may also be passed in.
Please add a comment

> + goto out;
> + }
> +
> + len = bitlen / 8;
> +
> + /* do an empty read to clear the current contents */
> + readl(&ds->regs->buf);
> +
> + /* keep writing and reading 1 byte until done */
> + for (i = 0; i < len; i++) {
> + /* wait till TXFULL is asserted */
> + while (readl(&ds->regs->buf) & (SPIBUF_TXFULL_MASK));
> +
> + /* write the data */
> + data1_reg_val &= ~0xFFFF;
> + if (txp) {

Checking for a valid pointer should happen earlier.
If an error happens here, a bogus value of the old
data1_reg_val will be used.
Move the check higher

> + data1_reg_val |= *txp & 0xFF;

Adding 0xff should not be necessary for a u8.

> + txp++;
> + }
> +
<snip>

> +
> diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> new file mode 100644
> index 0000000..5b9a832
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.h
> @@ -0,0 +1,102 @@
> +/*
<snip>
> + * Copyright (C) 2009 Sekhar Nori, Texas Instruments, Inc <www.ti.com>

> +
> +static inline struct davinci_spi_slave *to_davinci_spi(struct spi_slave *slave)
> +{
Check before calling

> + return container_of(slave, struct davinci_spi_slave, slave);
> +}
> +
> +#endif /* _DAVINCI_SPI_H_ */

Tom