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

[Sudhakar Rajashekhara <sudhakar.raj@ti.com>]

Hi,

On Tue, Jan 05, 2010 at 19:49:19, Tom wrote:
> 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>
> > ---

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

I'll remove extra lines between the header file inclusions.

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

I'll remove the static variable.

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

OK.

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

OK.

> > +
> > +	/* 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.
> 

NULL values for din and dout is normal and I'll document it. SPI peripheral
is a transceiver kind of a peripheral on DaVinci. It requires TX for RX to
work. Hence they are coupled like this.

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

SPI_XFER_END flag is being set if bitlen is not 8 bit aligned. This has been
documented above.

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

Agree, I'll remove it.

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

OK.

Thanks for your comments. I'll submit the updated patch soon.

Regards,
Sudhakar