From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sudhakar Rajashekhara Date: Wed, 6 Jan 2010 16:56:30 +0530 Subject: [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller In-Reply-To: <4B434A67.3040609@windriver.com> References: <1262666822-27247-1-git-send-email-sudhakar.raj@ti.com> <4B434A67.3040609@windriver.com> Message-ID: <00a801ca8ec3$17b53940$471fabc0$@raj@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On Tue, Jan 05, 2010 at 19:49:19, Tom wrote: > Sudhakar Rajashekhara wrote: > > From: Sekhar Nori > > > > This adds a driver for the SPI controller found on davinci > > based SoCs from Texas Instruments. > > > > Signed-off-by: Sekhar Nori > > Signed-off-by: Sudhakar Rajashekhara > > --- > > > > 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 > > + * > > + * 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 > > +#include > > +#include > > + > > +#include > > + > > +#include > > + > > +#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++; > > + } > > + > > > > + > > 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 @@ > > +/* > > > + * Copyright (C) 2009 Sekhar Nori, Texas Instruments, Inc > > > + > > +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