All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
@ 2009-12-23  7:44 Sudhakar Rajashekhara
  2009-12-24  1:34 ` Mike Frysinger
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Sudhakar Rajashekhara @ 2009-12-23  7:44 UTC (permalink / raw)
  To: u-boot

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>
---
 drivers/spi/Makefile      |    1 +
 drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
 3 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/davinci_spi.c
 create mode 100644 drivers/spi/davinci_spi.h

diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 824d8e7..07b9611 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
 COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
 COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
 COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
+COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
new file mode 100644
index 0000000..1c988bb
--- /dev/null
+++ b/drivers/spi/davinci_spi.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright (C) 2009 Texas Instruments Incorporated - http://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"
+
+static unsigned int data1_reg_val;
+
+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 = (void *)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);
+
+	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 */
+	spi_writel(ds, GCR0, SPIGCR0_SPIRST_MASK);
+	udelay(1000);
+	spi_writel(ds, GCR0, SPIGCR0_SPIENA_MASK);
+
+	/* Set master mode, powered up and not activated */
+	spi_writel(ds, GCR1, SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK);
+
+	/* CS, CLK, SIMO and SOMI are functional pins */
+	spi_writel(ds, PC0, (SPIPC0_EN0FUN_MASK) | (SPIPC0_CLKFUN_MASK) |
+				(SPIPC0_DOFUN_MASK) | (SPIPC0_DIFUN_MASK));
+
+	/* setup format */
+	scalar = ((CONFIG_SYS_SPI_CLK / ds->freq) - 1) & 0xFF;
+
+	spi_writel(ds, FMT0, 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));
+
+	/* hold cs active@end of transfer until explicitly de-asserted */
+	data1_reg_val = (1 << SPIDAT1_CSHOLD_SHIFT) |
+			(slave->cs << SPIDAT1_CSNR_SHIFT);
+	spi_writel(ds, DAT1, data1_reg_val);
+
+	/*
+	 * Including a minor delay. No science here. Should be good even with
+	 * no delay
+	 */
+	spi_writel(ds, DELAY, (50 << SPI_C2TDELAY_SHIFT) |
+				(50 << SPI_T2CDELAY_SHIFT));
+
+	/* default chip select register */
+	spi_writel(ds, DEF, SPIDEF_CSDEF0_MASK);
+
+	/* no interrupts */
+	spi_writel(ds, INT0, 0);
+	spi_writel(ds, LVL, 0);
+
+	/* enable SPI */
+	spi_writel(ds, GCR1, spi_readl(ds, GCR1) | (SPIGCR1_SPIENA_MASK));
+
+	return 0;
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+	struct davinci_spi_slave *ds = to_davinci_spi(slave);
+
+	/* Disable the SPI hardware */
+	spi_writel(ds, GCR0, SPIGCR0_SPIRST_MASK);
+}
+
+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;
+
+	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;
+		goto out;
+	}
+
+	len = bitlen / 8;
+
+	/* do an empty read to clear the current contents */
+	spi_readl(ds, BUF);
+
+	/* keep writing and reading 1 byte until done */
+	for (i = 0; i < len; i++) {
+		/* wait till TXFULL is asserted */
+		while (spi_readl(ds, BUF) & (SPIBUF_TXFULL_MASK));
+
+		/* write the data */
+		data1_reg_val &= ~0xFFFF;
+		if (txp) {
+			data1_reg_val |= *txp & 0xFF;
+			txp++;
+		}
+
+		/*
+		 * Write to DAT1 is required to keep the serial transfer going.
+		 * We just terminate when we reach the end.
+		 */
+		if ((i == (len - 1)) && (flags & SPI_XFER_END)) {
+			/* clear CS hold */
+			spi_writel(ds, DAT1, data1_reg_val &
+					~(1 << SPIDAT1_CSHOLD_SHIFT));
+		} else {
+			spi_writel(ds, DAT1, data1_reg_val);
+		}
+
+		/* read the data - wait for data availability */
+		while (spi_readl(ds, BUF) & (SPIBUF_RXEMPTY_MASK));
+
+		if (rxp) {
+			*rxp = spi_readl(ds, BUF) & 0xFF;
+			rxp++;
+		} else {
+			spi_readl(ds, BUF); /* simply drop the read character */
+		}
+	}
+	return 0;
+
+out:
+	if (flags & SPI_XFER_END) {
+		spi_writel(ds, DAT1, data1_reg_val &
+				~(1 << SPIDAT1_CSHOLD_SHIFT));
+	}
+	return 0;
+}
+
diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
new file mode 100644
index 0000000..b3bf916
--- /dev/null
+++ b/drivers/spi/davinci_spi.h
@@ -0,0 +1,84 @@
+/*
+ * Register definitions for the DaVinci SPI Controller
+ */
+
+/* Register offsets */
+#define DAVINCI_SPI_GCR0	0x0000
+#define DAVINCI_SPI_GCR1	0x0004
+#define DAVINCI_SPI_INT0	0x0008
+#define DAVINCI_SPI_LVL		0x000c
+#define DAVINCI_SPI_FLG		0x0010
+#define DAVINCI_SPI_PC0		0x0014
+#define DAVINCI_SPI_PC1		0x0018
+#define DAVINCI_SPI_PC2		0x001c
+#define DAVINCI_SPI_PC3		0x0020
+#define DAVINCI_SPI_PC4		0x0024
+#define DAVINCI_SPI_PC5		0x0028
+#define DAVINCI_SPI_DAT0	0x0038
+#define DAVINCI_SPI_DAT1	0x003c
+#define DAVINCI_SPI_BUF		0x0040
+#define DAVINCI_SPI_EMU		0x0044
+#define DAVINCI_SPI_DELAY	0x0048
+#define DAVINCI_SPI_DEF		0x004c
+#define DAVINCI_SPI_FMT0	0x0050
+#define DAVINCI_SPI_FMT1	0x0054
+#define DAVINCI_SPI_FMT2	0x0058
+#define DAVINCI_SPI_FMT3	0x005c
+#define DAVINCI_SPI_INTVEC0	0x0060
+#define DAVINCI_SPI_INTVEC1	0x0064
+
+#define BIT(x)			(1 << (x))
+
+/* SPIGCR0 */
+#define SPIGCR0_SPIENA_MASK	0x1
+#define SPIGCR0_SPIRST_MASK	0x0
+
+/* SPIGCR0 */
+#define SPIGCR1_CLKMOD_MASK	BIT(1)
+#define SPIGCR1_MASTER_MASK	BIT(0)
+#define SPIGCR1_SPIENA_MASK	BIT(24)
+
+/* SPIPC0 */
+#define SPIPC0_DIFUN_MASK	BIT(11)		/* SIMO */
+#define SPIPC0_DOFUN_MASK	BIT(10)		/* SOMI */
+#define SPIPC0_CLKFUN_MASK	BIT(9)		/* CLK */
+#define SPIPC0_EN0FUN_MASK	BIT(0)
+
+/* SPIFMT0 */
+#define SPIFMT_SHIFTDIR_SHIFT	20
+#define SPIFMT_POLARITY_SHIFT	17
+#define SPIFMT_PHASE_SHIFT	16
+#define SPIFMT_PRESCALE_SHIFT	8
+
+/* SPIDAT1 */
+#define SPIDAT1_CSHOLD_SHIFT	28
+#define SPIDAT1_CSNR_SHIFT	16
+
+/* SPIDELAY */
+#define SPI_C2TDELAY_SHIFT	24
+#define SPI_T2CDELAY_SHIFT	16
+
+/* SPIBUF */
+#define SPIBUF_RXEMPTY_MASK	BIT(31)
+#define SPIBUF_TXFULL_MASK	BIT(29)
+
+/* SPIDEF */
+#define SPIDEF_CSDEF0_MASK	BIT(0)
+
+struct davinci_spi_slave {
+	struct spi_slave slave;
+	void		*regs;
+	u32		mr;
+	unsigned int freq;
+};
+
+static inline struct davinci_spi_slave *to_davinci_spi(struct spi_slave *slave)
+{
+	return container_of(slave, struct davinci_spi_slave, slave);
+}
+
+#define spi_readl(ds, reg)					\
+	readl(ds->regs + DAVINCI_SPI_##reg)
+#define spi_writel(ds, reg, value)				\
+	writel(value, ds->regs + DAVINCI_SPI_##reg)
+
-- 
1.5.6

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-23  7:44 [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
@ 2009-12-24  1:34 ` Mike Frysinger
  2009-12-24  2:52   ` Sudhakar Rajashekhara
  2009-12-25 17:11 ` Dirk Behme
  2010-01-04  9:47 ` [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Nick Thompson
  2 siblings, 1 reply; 22+ messages in thread
From: Mike Frysinger @ 2009-12-24  1:34 UTC (permalink / raw)
  To: u-boot

On Wednesday 23 December 2009 02:44:36 Sudhakar Rajashekhara wrote:
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
>  COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
>  COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
>  COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o

this is a sorted list

> --- /dev/null
> +++ b/drivers/spi/davinci_spi.h
> @@ -0,0 +1,84 @@
> +/*
> + * Register definitions for the DaVinci SPI Controller
> + */

missing license/copyright

> +#define spi_readl(ds, reg)					\
> +	readl(ds->regs + DAVINCI_SPI_##reg)
> +#define spi_writel(ds, reg, value)				\
> +	writel(value, ds->regs + DAVINCI_SPI_##reg)

should be (ds)->regs
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091223/a699402e/attachment.pgp 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-24  1:34 ` Mike Frysinger
@ 2009-12-24  2:52   ` Sudhakar Rajashekhara
  0 siblings, 0 replies; 22+ messages in thread
From: Sudhakar Rajashekhara @ 2009-12-24  2:52 UTC (permalink / raw)
  To: u-boot

Hi,

On Thu, Dec 24, 2009 at 07:04:18, Mike Frysinger wrote:
> On Wednesday 23 December 2009 02:44:36 Sudhakar Rajashekhara wrote:
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
> >  COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
> >  COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
> >  COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> > +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> 
> this is a sorted list
> 

Will fix it. Thanks for pointing it out.

> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Register definitions for the DaVinci SPI Controller  */
> 
> missing license/copyright
> 

Will add copyright/license.

> > +#define spi_readl(ds, reg)					\
> > +	readl(ds->regs + DAVINCI_SPI_##reg)
> > +#define spi_writel(ds, reg, value)				\
> > +	writel(value, ds->regs + DAVINCI_SPI_##reg)
> 
> should be (ds)->regs

I'll modify it and resubmit the patch.

Thanks,
Sudhakar

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-23  7:44 [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
  2009-12-24  1:34 ` Mike Frysinger
@ 2009-12-25 17:11 ` Dirk Behme
  2010-01-04  9:42   ` Sudhakar Rajashekhara
  2010-01-04  9:46   ` Sudhakar Rajashekhara
  2010-01-04  9:47 ` [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Nick Thompson
  2 siblings, 2 replies; 22+ messages in thread
From: Dirk Behme @ 2009-12-25 17:11 UTC (permalink / raw)
  To: u-boot

On 23.12.2009 08:44, 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>
> ---
>   drivers/spi/Makefile      |    1 +
>   drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
>   drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
>   3 files changed, 290 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/spi/davinci_spi.c
>   create mode 100644 drivers/spi/davinci_spi.h
>
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 824d8e7..07b9611 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
>   COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
>   COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
>   COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o

Should this list be kept sorted alphabetically? I.e.

...
COBJS-$(CONFIG_BFIN_SPI) += bfin_spi.o
COBJS-$(CONFIG_CF_SPI) += cf_spi.o
  +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
...

?

>   COBJS	:= $(COBJS-y)
>   SRCS	:= $(COBJS:.o=.c)
> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
> new file mode 100644
> index 0000000..1c988bb
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.c
> @@ -0,0 +1,205 @@
...
> +void spi_init()
...
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +			unsigned int max_hz, unsigned int mode)
...
> +void spi_free_slave(struct spi_slave *slave)
....
> +int spi_claim_bus(struct spi_slave *slave)
....
> +void spi_release_bus(struct spi_slave *slave)
....
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> +		const void *dout, void *din, unsigned long flags)

I wonder if all (SPI API) functions from include/spi.h should be 
implemented? I.e. do we need (dummy) functions for spi_cs_is_valid(), 
spi_cs_activate() and spi_cs_deactivate(), too? Just in case a driver 
does use them.

> diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> new file mode 100644
> index 0000000..b3bf916
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.h
> @@ -0,0 +1,84 @@
> +/*
> + * Register definitions for the DaVinci SPI Controller
> + */

Do we want a complete GPL header, like in the .c file, here, too?

Do we want a protection against multiple inclusion

#ifndef _DAVINCI_SPI_H_
#define _DAVINCI_SPI_H_

in this header file?

Best regards

Dirk

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-25 17:11 ` Dirk Behme
@ 2010-01-04  9:42   ` Sudhakar Rajashekhara
  2010-01-04  9:46   ` Sudhakar Rajashekhara
  1 sibling, 0 replies; 22+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-04  9:42 UTC (permalink / raw)
  To: u-boot

Hi,

On Fri, Dec 25, 2009 at 22:41:13, Dirk Behme wrote:
> On 23.12.2009 08:44, 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>
> > ---
> >   drivers/spi/Makefile      |    1 +
> >   drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
> >   drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
> >   3 files changed, 290 insertions(+), 0 deletions(-)
> >   create mode 100644 drivers/spi/davinci_spi.c
> >   create mode 100644 drivers/spi/davinci_spi.h
> >
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 824d8e7..07b9611 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
> >   COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
> >   COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
> >   COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> > +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> 
> Should this list be kept sorted alphabetically? I.e.
> 

I'll arrange them alphabetically.

> ...
> COBJS-$(CONFIG_BFIN_SPI) += bfin_spi.o
> COBJS-$(CONFIG_CF_SPI) += cf_spi.o
>   +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
> ...
> 
> ?
> 
> >   COBJS	:= $(COBJS-y)
> >   SRCS	:= $(COBJS:.o=.c)
> > diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
> > new file mode 100644
> > index 0000000..1c988bb
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.c
> > @@ -0,0 +1,205 @@
> ...
> > +void spi_init()
> ...
> > +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> > +			unsigned int max_hz, unsigned int mode)
> ...
> > +void spi_free_slave(struct spi_slave *slave)
> ....
> > +int spi_claim_bus(struct spi_slave *slave)
> ....
> > +void spi_release_bus(struct spi_slave *slave)
> ....
> > +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> > +		const void *dout, void *din, unsigned long flags)
> 
> I wonder if all (SPI API) functions from include/spi.h should be 
> implemented? I.e. do we need (dummy) functions for spi_cs_is_valid(), 
> spi_cs_activate() and spi_cs_deactivate(), too? Just in case a driver 
> does use them.
> 
> > diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> > new file mode 100644
> > index 0000000..b3bf916
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Register definitions for the DaVinci SPI Controller
> > + */
> 
> Do we want a complete GPL header, like in the .c file, here, too?
> 
> Do we want a protection against multiple inclusion
> 
> #ifndef _DAVINCI_SPI_H_
> #define _DAVINCI_SPI_H_
> 
> in this header file?
> 
> Best regards
> 
> Dirk
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-25 17:11 ` Dirk Behme
  2010-01-04  9:42   ` Sudhakar Rajashekhara
@ 2010-01-04  9:46   ` Sudhakar Rajashekhara
  2010-01-05  6:01     ` Mike Frysinger
  1 sibling, 1 reply; 22+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-04  9:46 UTC (permalink / raw)
  To: u-boot

Hi,

On Fri, Dec 25, 2009 at 22:41:13, Dirk Behme wrote:
> On 23.12.2009 08:44, 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>
> > ---
> >   drivers/spi/Makefile      |    1 +
> >   drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
> >   drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
> >   3 files changed, 290 insertions(+), 0 deletions(-)
> >   create mode 100644 drivers/spi/davinci_spi.c
> >   create mode 100644 drivers/spi/davinci_spi.h
> >
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 824d8e7..07b9611 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
> >   COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
> >   COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
> >   COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> > +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> 
> Should this list be kept sorted alphabetically? I.e.
> 

I'll arrange them alphabetically.

> ...
> COBJS-$(CONFIG_BFIN_SPI) += bfin_spi.o
> COBJS-$(CONFIG_CF_SPI) += cf_spi.o
>   +COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
> ...
> 
> ?
> 
> >   COBJS	:= $(COBJS-y)
> >   SRCS	:= $(COBJS:.o=.c)
> > diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
> > new file mode 100644
> > index 0000000..1c988bb
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.c
> > @@ -0,0 +1,205 @@
> ...
> > +void spi_init()
> ...
> > +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> > +			unsigned int max_hz, unsigned int mode)
> ...
> > +void spi_free_slave(struct spi_slave *slave)
> ....
> > +int spi_claim_bus(struct spi_slave *slave)
> ....
> > +void spi_release_bus(struct spi_slave *slave)
> ....
> > +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> > +		const void *dout, void *din, unsigned long flags)
> 
> I wonder if all (SPI API) functions from include/spi.h should be 
> implemented? I.e. do we need (dummy) functions for spi_cs_is_valid(), 
> spi_cs_activate() and spi_cs_deactivate(), too? Just in case a driver 
> does use them.
> 

Currently none of the SPI Flash drivers are using the above functions. I'll 
implement the above functions as dummy and later as and when the need arises,
these functions will be implemented.

> > diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> > new file mode 100644
> > index 0000000..b3bf916
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Register definitions for the DaVinci SPI Controller
> > + */
> 
> Do we want a complete GPL header, like in the .c file, here, too?
> 
> Do we want a protection against multiple inclusion
> 
> #ifndef _DAVINCI_SPI_H_
> #define _DAVINCI_SPI_H_
> 
> in this header file?

I'll add the GPL header and guard against multiple inclusion.

Thanks for the comments. I'll submit an updated version of the patch soon.

Regards,
Sudhakar

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2009-12-23  7:44 [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
  2009-12-24  1:34 ` Mike Frysinger
  2009-12-25 17:11 ` Dirk Behme
@ 2010-01-04  9:47 ` Nick Thompson
  2010-01-04 10:42   ` Sudhakar Rajashekhara
  2 siblings, 1 reply; 22+ messages in thread
From: Nick Thompson @ 2010-01-04  9:47 UTC (permalink / raw)
  To: u-boot

On 23/12/09 07:44, 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>
> ---
>  drivers/spi/Makefile      |    1 +
>  drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
>  3 files changed, 290 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/spi/davinci_spi.c
>  create mode 100644 drivers/spi/davinci_spi.h

...

> diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> new file mode 100644
> index 0000000..b3bf916
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.h
> @@ -0,0 +1,84 @@
> +/*
> + * Register definitions for the DaVinci SPI Controller
> + */
> +
> +/* Register offsets */
> +#define DAVINCI_SPI_GCR0	0x0000
> +#define DAVINCI_SPI_GCR1	0x0004
> +#define DAVINCI_SPI_INT0	0x0008
> +#define DAVINCI_SPI_LVL		0x000c
> +#define DAVINCI_SPI_FLG		0x0010
> +#define DAVINCI_SPI_PC0		0x0014
> +#define DAVINCI_SPI_PC1		0x0018
> +#define DAVINCI_SPI_PC2		0x001c
> +#define DAVINCI_SPI_PC3		0x0020
> +#define DAVINCI_SPI_PC4		0x0024
> +#define DAVINCI_SPI_PC5		0x0028
> +#define DAVINCI_SPI_DAT0	0x0038
> +#define DAVINCI_SPI_DAT1	0x003c
> +#define DAVINCI_SPI_BUF		0x0040
> +#define DAVINCI_SPI_EMU		0x0044
> +#define DAVINCI_SPI_DELAY	0x0048
> +#define DAVINCI_SPI_DEF		0x004c
> +#define DAVINCI_SPI_FMT0	0x0050
> +#define DAVINCI_SPI_FMT1	0x0054
> +#define DAVINCI_SPI_FMT2	0x0058
> +#define DAVINCI_SPI_FMT3	0x005c
> +#define DAVINCI_SPI_INTVEC0	0x0060
> +#define DAVINCI_SPI_INTVEC1	0x0064

I think this ought to be a C structure, rather than register offsets?

...

> +
> +struct davinci_spi_slave {
> +	struct spi_slave slave;
> +	void		*regs;

This should have the type of the C structure to be defined above.

> +	u32		mr;
> +	unsigned int freq;
> +};
> +

...

> +
> +#define spi_readl(ds, reg)					\
> +	readl(ds->regs + DAVINCI_SPI_##reg)
> +#define spi_writel(ds, reg, value)				\
> +	writel(value, ds->regs + DAVINCI_SPI_##reg)

These can be rewritten (and the usages changed slightly) to access via the structure.
You will then not be circumventing any type checking.

Maybe these defs then become too trivial and can be dropped altogether?

Regards,
Nick.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2010-01-04  9:47 ` [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Nick Thompson
@ 2010-01-04 10:42   ` Sudhakar Rajashekhara
  2010-01-04 19:50     ` Dirk Behme
  0 siblings, 1 reply; 22+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-04 10:42 UTC (permalink / raw)
  To: u-boot

Hi Nick,

On Mon, Jan 04, 2010 at 15:17:51, Nick Thompson wrote:
> On 23/12/09 07:44, 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>
> > ---
> >  drivers/spi/Makefile      |    1 +
> >  drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
> >  3 files changed, 290 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/spi/davinci_spi.c
> >  create mode 100644 drivers/spi/davinci_spi.h
> 
> ...
> 
> > diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
> > new file mode 100644
> > index 0000000..b3bf916
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Register definitions for the DaVinci SPI Controller
> > + */
> > +
> > +/* Register offsets */
> > +#define DAVINCI_SPI_GCR0	0x0000
> > +#define DAVINCI_SPI_GCR1	0x0004
> > +#define DAVINCI_SPI_INT0	0x0008
> > +#define DAVINCI_SPI_LVL		0x000c
> > +#define DAVINCI_SPI_FLG		0x0010
> > +#define DAVINCI_SPI_PC0		0x0014
> > +#define DAVINCI_SPI_PC1		0x0018
> > +#define DAVINCI_SPI_PC2		0x001c
> > +#define DAVINCI_SPI_PC3		0x0020
> > +#define DAVINCI_SPI_PC4		0x0024
> > +#define DAVINCI_SPI_PC5		0x0028
> > +#define DAVINCI_SPI_DAT0	0x0038
> > +#define DAVINCI_SPI_DAT1	0x003c
> > +#define DAVINCI_SPI_BUF		0x0040
> > +#define DAVINCI_SPI_EMU		0x0044
> > +#define DAVINCI_SPI_DELAY	0x0048
> > +#define DAVINCI_SPI_DEF		0x004c
> > +#define DAVINCI_SPI_FMT0	0x0050
> > +#define DAVINCI_SPI_FMT1	0x0054
> > +#define DAVINCI_SPI_FMT2	0x0058
> > +#define DAVINCI_SPI_FMT3	0x005c
> > +#define DAVINCI_SPI_INTVEC0	0x0060
> > +#define DAVINCI_SPI_INTVEC1	0x0064
> 
> I think this ought to be a C structure, rather than register offsets?
> 

I see that existing SPI drivers are not using structures for register defines.
Off-late is there a shift in u-boot in favor of structures than macros for
register definitions?

Regards,
Sudhakar

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2010-01-04 10:42   ` Sudhakar Rajashekhara
@ 2010-01-04 19:50     ` Dirk Behme
  2010-01-05  0:20       ` Mike Frysinger
  2010-03-05  6:47       ` Mansoor
  0 siblings, 2 replies; 22+ messages in thread
From: Dirk Behme @ 2010-01-04 19:50 UTC (permalink / raw)
  To: u-boot

On 04.01.2010 11:42, Sudhakar Rajashekhara wrote:
> Hi Nick,
>
> On Mon, Jan 04, 2010 at 15:17:51, Nick Thompson wrote:
>> On 23/12/09 07:44, 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>
>>> ---
>>>   drivers/spi/Makefile      |    1 +
>>>   drivers/spi/davinci_spi.c |  205 +++++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/spi/davinci_spi.h |   84 ++++++++++++++++++
>>>   3 files changed, 290 insertions(+), 0 deletions(-)
>>>   create mode 100644 drivers/spi/davinci_spi.c
>>>   create mode 100644 drivers/spi/davinci_spi.h
>>
>> ...
>>
>>> diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h
>>> new file mode 100644
>>> index 0000000..b3bf916
>>> --- /dev/null
>>> +++ b/drivers/spi/davinci_spi.h
>>> @@ -0,0 +1,84 @@
>>> +/*
>>> + * Register definitions for the DaVinci SPI Controller
>>> + */
>>> +
>>> +/* Register offsets */
>>> +#define DAVINCI_SPI_GCR0	0x0000
>>> +#define DAVINCI_SPI_GCR1	0x0004
>>> +#define DAVINCI_SPI_INT0	0x0008
>>> +#define DAVINCI_SPI_LVL		0x000c
>>> +#define DAVINCI_SPI_FLG		0x0010
>>> +#define DAVINCI_SPI_PC0		0x0014
>>> +#define DAVINCI_SPI_PC1		0x0018
>>> +#define DAVINCI_SPI_PC2		0x001c
>>> +#define DAVINCI_SPI_PC3		0x0020
>>> +#define DAVINCI_SPI_PC4		0x0024
>>> +#define DAVINCI_SPI_PC5		0x0028
>>> +#define DAVINCI_SPI_DAT0	0x0038
>>> +#define DAVINCI_SPI_DAT1	0x003c
>>> +#define DAVINCI_SPI_BUF		0x0040
>>> +#define DAVINCI_SPI_EMU		0x0044
>>> +#define DAVINCI_SPI_DELAY	0x0048
>>> +#define DAVINCI_SPI_DEF		0x004c
>>> +#define DAVINCI_SPI_FMT0	0x0050
>>> +#define DAVINCI_SPI_FMT1	0x0054
>>> +#define DAVINCI_SPI_FMT2	0x0058
>>> +#define DAVINCI_SPI_FMT3	0x005c
>>> +#define DAVINCI_SPI_INTVEC0	0x0060
>>> +#define DAVINCI_SPI_INTVEC1	0x0064
>>
>> I think this ought to be a C structure, rather than register offsets?
>>
>
> I see that existing SPI drivers are not using structures for register defines.
> Off-late is there a shift in u-boot in favor of structures than macros for
> register definitions?

Yes.

E.g. what I did for OMAP3 SPI driver:

/* OMAP3 McSPI registers */
struct mcspi_channel {
	unsigned int chconf;		/* 0x2C, 0x40, 0x54, 0x68 */
	unsigned int chstat;		/* 0x30, 0x44, 0x58, 0x6C */
	unsigned int chctrl;		/* 0x34, 0x48, 0x5C, 0x70 */
	unsigned int tx;		/* 0x38, 0x4C, 0x60, 0x74 */
	unsigned int rx;		/* 0x3C, 0x50, 0x64, 0x78 */
};

struct mcspi {
	unsigned char res1[0x10];
	unsigned int sysconfig;		/* 0x10 */
	unsigned int sysstatus;		/* 0x14 */
	unsigned int irqstatus;		/* 0x18 */
	unsigned int irqenable;		/* 0x1C */
	unsigned int wakeupenable;	/* 0x20 */
	unsigned int syst;		/* 0x24 */
	unsigned int modulctrl;		/* 0x28 */
	struct mcspi_channel channel[4];
}

regs = (struct mcspi *)OMAP3_MCSPI1_BASE;

writel(value, regs->modulctrl);

Best regards

Dirk

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2010-01-04 19:50     ` Dirk Behme
@ 2010-01-05  0:20       ` Mike Frysinger
  2010-03-05  6:47       ` Mansoor
  1 sibling, 0 replies; 22+ messages in thread
From: Mike Frysinger @ 2010-01-05  0:20 UTC (permalink / raw)
  To: u-boot

On Monday 04 January 2010 14:50:54 Dirk Behme wrote:
> struct mcspi {
> 	unsigned char res1[0x10];
> 	unsigned int sysconfig;		/* 0x10 */
> 	unsigned int sysstatus;		/* 0x14 */
> 	unsigned int irqstatus;		/* 0x18 */
> 	unsigned int irqenable;		/* 0x1C */
> 	unsigned int wakeupenable;	/* 0x20 */
> 	unsigned int syst;		/* 0x24 */
> 	unsigned int modulctrl;		/* 0x28 */
> 	struct mcspi_channel channel[4];
> }
> 
> regs = (struct mcspi *)OMAP3_MCSPI1_BASE;
> 
> writel(value, regs->modulctrl);

i think you mean &regs->modulctrl
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100104/1e061348/attachment.pgp 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2010-01-04  9:46   ` Sudhakar Rajashekhara
@ 2010-01-05  6:01     ` Mike Frysinger
  2010-01-05  7:14       ` [U-Boot] NS16550 Register structure Pedanekar, Hemant
  0 siblings, 1 reply; 22+ messages in thread
From: Mike Frysinger @ 2010-01-05  6:01 UTC (permalink / raw)
  To: u-boot

On Monday 04 January 2010 04:46:19 Sudhakar Rajashekhara wrote:
> On Fri, Dec 25, 2009 at 22:41:13, Dirk Behme wrote:
> > I wonder if all (SPI API) functions from include/spi.h should be
> > implemented? I.e. do we need (dummy) functions for spi_cs_is_valid(),
> > spi_cs_activate() and spi_cs_deactivate(), too? Just in case a driver
> > does use them.
> 
> Currently none of the SPI Flash drivers are using the above functions. I'll
> implement the above functions as dummy and later as and when the need
>  arises, these functions will be implemented.

considering the SPI protocol, the implementation for the spi_cs_* functions 
should fall naturally into place if the driver is architected properly.  in 
other words, your bus driver should already be relying on these functions --  
you call them when you begin/end a transfer.

same goes for spi_cs_is_valid ... your spi_setup_slave function shouldnt be 
accepting any random cs value given to it.

the Blackfin driver shows that these are all pretty trivial to get right.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100105/be908eaf/attachment.pgp 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-05  6:01     ` Mike Frysinger
@ 2010-01-05  7:14       ` Pedanekar, Hemant
  2010-01-05  7:35         ` Wolfgang Denk
  0 siblings, 1 reply; 22+ messages in thread
From: Pedanekar, Hemant @ 2010-01-05  7:14 UTC (permalink / raw)
  To: u-boot

Hello,

I have observed that in the recent commit (file include/ns16550.h), the ns16550 UART register structure declaration is changed so that irrespective of the CONFIG_SYS_NS16550_REG_SIZE setting the register access will be done at byte level. 

This crates problem when UART space access is required to be word-aligned. I am particularly referring to DaVinci DM6467, where STRB over UART register space is not supported and requires 32-bit access (L/STR).

If I go ahead and add a specific handling in ns16550.h file (structure register members as "long"), I would end up making code similar to the one earlier to last commit. Is there a better way to handle such case?

Thanks.
-
Hemant

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-05  7:14       ` [U-Boot] NS16550 Register structure Pedanekar, Hemant
@ 2010-01-05  7:35         ` Wolfgang Denk
  2010-01-05  8:55           ` Pedanekar, Hemant
  0 siblings, 1 reply; 22+ messages in thread
From: Wolfgang Denk @ 2010-01-05  7:35 UTC (permalink / raw)
  To: u-boot

Dear "Pedanekar, Hemant",

In message <2A3DCF3DA181AD40BDE86A3150B27B6B03094B2982@dbde02.ent.ti.com> you wrote:
> 
> I have observed that in the recent commit (file include/ns16550.h), the ns16550 UART register structure declaration is changed so that irrespective of the CONFIG_SYS_NS16550_REG_SIZE setting the register access will be done at byte level. 

This seems to be working on all systems so far, which is not so much
of a surprise as the registers are 8 bit wide only.

> This crates problem when UART space access is required to be word-aligned. I am particularly referring to DaVinci DM6467, where STRB over UART register space is not supported and requires 32-bit access (L/STR).

Please make sure to use exact terms. Alignment is one thing, and  bus
width  is  another  one.  Alignment  can be easily adjusted using the
prepad_* or postpad_* settings.

> If I go ahead and add a specific handling in ns16550.h file (structure register members as "long"), I would end up making code similar to the one earlier to last commit. Is there a better way to handle such case?

No such changes should be needed, as far as I understand the code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In theory, there is no difference between  theory  and  practice.  In
practice, however, there is.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-05  7:35         ` Wolfgang Denk
@ 2010-01-05  8:55           ` Pedanekar, Hemant
  2010-01-05  9:58             ` Wolfgang Denk
  0 siblings, 1 reply; 22+ messages in thread
From: Pedanekar, Hemant @ 2010-01-05  8:55 UTC (permalink / raw)
  To: u-boot

Wolfgang,

Yes, that is not just word-aligned, instead, the Dm6467 UART access need to be "Word LOAD/STORE". Thus, if I use STRB <THR> instead of STR <THR> to transmit a character, it doesn't work.

As you said, using prepad/postpad, we can define the alignment but my understanding is that the final register access will be byte access only as the register will be declared as "unsigned char". 

For generating 32-bit load/store, probably we need to declare register members as "unsigned long". Or can we achieve the same without any code changes?

Thanks
-
Hemant
 

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Tuesday, January 05, 2010 1:05 PM
> To: Pedanekar, Hemant
> Cc: u-boot at lists.denx.de; dzu at denx.de
> Subject: Re: [U-Boot] NS16550 Register structure
> 
> Dear "Pedanekar, Hemant",
> 
> In message <2A3DCF3DA181AD40BDE86A3150B27B6B03094B2982@dbde02.ent.ti.com>
> you wrote:
> >
> > I have observed that in the recent commit (file include/ns16550.h), the
> ns16550 UART register structure declaration is changed so that
> irrespective of the CONFIG_SYS_NS16550_REG_SIZE setting the register
> access will be done at byte level.
> 
> This seems to be working on all systems so far, which is not so much
> of a surprise as the registers are 8 bit wide only.
> 
> > This crates problem when UART space access is required to be word-
> aligned. I am particularly referring to DaVinci DM6467, where STRB over
> UART register space is not supported and requires 32-bit access (L/STR).
> 
> Please make sure to use exact terms. Alignment is one thing, and  bus
> width  is  another  one.  Alignment  can be easily adjusted using the
> prepad_* or postpad_* settings.
> 
> > If I go ahead and add a specific handling in ns16550.h file (structure
> register members as "long"), I would end up making code similar to the one
> earlier to last commit. Is there a better way to handle such case?
> 
> No such changes should be needed, as far as I understand the code.
> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> In theory, there is no difference between  theory  and  practice.  In
> practice, however, there is.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-05  8:55           ` Pedanekar, Hemant
@ 2010-01-05  9:58             ` Wolfgang Denk
  2010-01-18 14:51               ` Pedanekar, Hemant
  0 siblings, 1 reply; 22+ messages in thread
From: Wolfgang Denk @ 2010-01-05  9:58 UTC (permalink / raw)
  To: u-boot

Dear "Pedanekar, Hemant",

In message <2A3DCF3DA181AD40BDE86A3150B27B6B030959490E@dbde02.ent.ti.com> you wrote:
> 
> Yes, that is not just word-aligned, instead, the Dm6467 UART access need to=
>  be "Word LOAD/STORE". Thus, if I use STRB <THR> instead of STR <THR> to tr=
> ansmit a character, it doesn't work.

Please use shorter lines (something like < 70 characters).

Can you please explain why byte I/O does not work for you?  Is this a
hardware problem on your specific board?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
It is necessary to have purpose.
	-- Alice #1, "I, Mudd", stardate 4513.3

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-05  9:58             ` Wolfgang Denk
@ 2010-01-18 14:51               ` Pedanekar, Hemant
  2010-01-19 13:04                 ` Detlev Zundel
  0 siblings, 1 reply; 22+ messages in thread
From: Pedanekar, Hemant @ 2010-01-18 14:51 UTC (permalink / raw)
  To: u-boot

Hi Wolfgang,

Only 32-bit access is supported for UART MMRs on DM6467 and hence need to use STR/LDR instead of STRB/LDRB.

Thanks
-
Hemant
 

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Tuesday, January 05, 2010 3:29 PM
> To: Pedanekar, Hemant
> Cc: u-boot at lists.denx.de; dzu at denx.de
> Subject: Re: [U-Boot] NS16550 Register structure
> 
> Dear "Pedanekar, Hemant",
> 
> In message <2A3DCF3DA181AD40BDE86A3150B27B6B030959490E@dbde02.ent.ti.com>
> you wrote:
> >
> > Yes, that is not just word-aligned, instead, the Dm6467 UART access need
> to=
> >  be "Word LOAD/STORE". Thus, if I use STRB <THR> instead of STR <THR> to
> tr=
> > ansmit a character, it doesn't work.
> 
> Please use shorter lines (something like < 70 characters).
> 
> Can you please explain why byte I/O does not work for you?  Is this a
> hardware problem on your specific board?
> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> It is necessary to have purpose.
> 	-- Alice #1, "I, Mudd", stardate 4513.3

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-18 14:51               ` Pedanekar, Hemant
@ 2010-01-19 13:04                 ` Detlev Zundel
  2010-01-21 11:58                   ` Pedanekar, Hemant
  2010-01-28  9:54                   ` Pedanekar, Hemant
  0 siblings, 2 replies; 22+ messages in thread
From: Detlev Zundel @ 2010-01-19 13:04 UTC (permalink / raw)
  To: u-boot

Hi Hemant,

> Only 32-bit access is supported for UART MMRs on DM6467 and hence need
> to use STR/LDR instead of STRB/LDRB.

Argh.  Why do hw people always think software can fix anything?  So out
of interest - do you get a bus fault or what?  Maybe we can fixup such
non-aligned accesses outside of the serial driver?

I'm really reluctant to topple over an infratstructure working on _many_
differenty hardware architectures and platforms only for one special
case which may be considered to be buggy hardware...

Cheers
  Detlev

-- 
While  the list of  different methods is not  endless, it is certainly
pretty long, with new ones being dreamed up all the time. Fortunately,
space limitations prevent us from looking at all of them.
                                            -- Andrew S. Tanenbaum
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-19 13:04                 ` Detlev Zundel
@ 2010-01-21 11:58                   ` Pedanekar, Hemant
  2010-01-28  9:54                   ` Pedanekar, Hemant
  1 sibling, 0 replies; 22+ messages in thread
From: Pedanekar, Hemant @ 2010-01-21 11:58 UTC (permalink / raw)
  To: u-boot

Hi Detlev,

There is no abort on ARM and also nothing gets to the console when UART register is accessed.

Do you think fixing this outside uart driver is possible? Maybe overriding UART register structure declaration or using some compiler magic? Please suggest.


Thanks
-
Hemant
 

> -----Original Message-----
> From: Detlev Zundel [mailto:dzu at denx.de]
> Sent: Tuesday, January 19, 2010 6:34 PM
> To: Pedanekar, Hemant
> Cc: Wolfgang Denk; u-boot at lists.denx.de
> Subject: Re: [U-Boot] NS16550 Register structure
> 
> Hi Hemant,
> 
> > Only 32-bit access is supported for UART MMRs on DM6467 and hence need
> > to use STR/LDR instead of STRB/LDRB.
> 
> Argh.  Why do hw people always think software can fix anything?  So out
> of interest - do you get a bus fault or what?  Maybe we can fixup such
> non-aligned accesses outside of the serial driver?
> 
> I'm really reluctant to topple over an infratstructure working on _many_
> differenty hardware architectures and platforms only for one special
> case which may be considered to be buggy hardware...
> 
> Cheers
>   Detlev
> 
> --
> While  the list of  different methods is not  endless, it is certainly
> pretty long, with new ones being dreamed up all the time. Fortunately,
> space limitations prevent us from looking at all of them.
>                                             -- Andrew S. Tanenbaum
> --
> DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-19 13:04                 ` Detlev Zundel
  2010-01-21 11:58                   ` Pedanekar, Hemant
@ 2010-01-28  9:54                   ` Pedanekar, Hemant
  2010-01-28 11:00                     ` Wolfgang Denk
  1 sibling, 1 reply; 22+ messages in thread
From: Pedanekar, Hemant @ 2010-01-28  9:54 UTC (permalink / raw)
  To: u-boot

Hi Detlev, Wolfgang,

Any suggestions on this?

Thanks
-
Hemant
 
> -----Original Message-----
> From: Pedanekar, Hemant
> Sent: Thursday, January 21, 2010 4:34 PM
> To: 'Detlev Zundel'
> Cc: Wolfgang Denk; u-boot at lists.denx.de
> Subject: RE: [U-Boot] NS16550 Register structure
> 
> Hi Detlev,
> 
> There is no abort on ARM and also nothing gets to the console when UART
> register is accessed.
> 
> Do you think fixing this outside uart driver is possible? Maybe overriding
> UART register structure declaration or using some compiler magic? Please
> suggest.
> 
> 
> Thanks
> -
> Hemant
> 
> 
> > -----Original Message-----
> > From: Detlev Zundel [mailto:dzu at denx.de]
> > Sent: Tuesday, January 19, 2010 6:34 PM
> > To: Pedanekar, Hemant
> > Cc: Wolfgang Denk; u-boot at lists.denx.de
> > Subject: Re: [U-Boot] NS16550 Register structure
> >
> > Hi Hemant,
> >
> > > Only 32-bit access is supported for UART MMRs on DM6467 and hence need
> > > to use STR/LDR instead of STRB/LDRB.
> >
> > Argh.  Why do hw people always think software can fix anything?  So out
> > of interest - do you get a bus fault or what?  Maybe we can fixup such
> > non-aligned accesses outside of the serial driver?
> >
> > I'm really reluctant to topple over an infratstructure working on _many_
> > differenty hardware architectures and platforms only for one special
> > case which may be considered to be buggy hardware...
> >
> > Cheers
> >   Detlev
> >
> > --
> > While  the list of  different methods is not  endless, it is certainly
> > pretty long, with new ones being dreamed up all the time. Fortunately,
> > space limitations prevent us from looking at all of them.
> >                                             -- Andrew S. Tanenbaum
> > --
> > DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> > HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] NS16550 Register structure
  2010-01-28  9:54                   ` Pedanekar, Hemant
@ 2010-01-28 11:00                     ` Wolfgang Denk
  0 siblings, 0 replies; 22+ messages in thread
From: Wolfgang Denk @ 2010-01-28 11:00 UTC (permalink / raw)
  To: u-boot

Dear Hemant,

In message <2A3DCF3DA181AD40BDE86A3150B27B6B03097EC8CA@dbde02.ent.ti.com> you wrote:
> 
> Any suggestions on this?

Well, the best solution would obviously be to fix the hardware...

> > There is no abort on ARM and also nothing gets to the console when UART
> > register is accessed.

Then what exactly is happening on your hardware when you read the
register? Did you really try out all options, i. e. reading as a 8
bit access with 0, 1, 2 and 3 bytes offset, etc.?

If you don't see any ecxceptions, then these accesses must be somehow
visible on the bus.  Find out (using a scope / logic analyzer) which
byte lanes get accessed when, and show us a table here what happens
when.

Only after we understand what is happening on your system we can come
up with any recommendations.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Alliance: In international politics, the union  of  two  thieves  who
have  their hands so deeply inserted in each other's pocket that they
cannot separately plunder a third.                   - Ambrose Bierce

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller
  2010-01-04 19:50     ` Dirk Behme
  2010-01-05  0:20       ` Mike Frysinger
@ 2010-03-05  6:47       ` Mansoor
  2010-03-07 12:38         ` [U-Boot] OMAP3 SPI driver ( was Re: [PATCH] TI DaVinci: Driver for the davinci SPI controller) Dirk Behme
  1 sibling, 1 reply; 22+ messages in thread
From: Mansoor @ 2010-03-05  6:47 UTC (permalink / raw)
  To: u-boot

Dirk Behme <dirk.behme <at> googlemail.com> writes:



> 
> E.g. what I did for OMAP3 SPI driver:
> 
> /* OMAP3 McSPI registers */
> struct mcspi_channel {
> 	unsigned int chconf;		/* 0x2C, 0x40, 0x54, 0x68 */
> 	unsigned int chstat;		/* 0x30, 0x44, 0x58, 0x6C */
> 	unsigned int chctrl;		/* 0x34, 0x48, 0x5C, 0x70 */
> 	unsigned int tx;		/* 0x38, 0x4C, 0x60, 0x74 */
> 	unsigned int rx;		/* 0x3C, 0x50, 0x64, 0x78 */
> };
> 

[...]

> writel(value, regs->modulctrl);
> 

Hi Dirk,

I could not find this code in any of the repositories. Could you share the
u-boot omap3 spi driver? 

Thanks
Mansoor

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] OMAP3 SPI driver ( was Re: [PATCH] TI DaVinci: Driver for the davinci SPI controller)
  2010-03-05  6:47       ` Mansoor
@ 2010-03-07 12:38         ` Dirk Behme
  0 siblings, 0 replies; 22+ messages in thread
From: Dirk Behme @ 2010-03-07 12:38 UTC (permalink / raw)
  To: u-boot

Hi Mansoor,

On 05.03.2010 07:47, Mansoor wrote:
> Dirk Behme<dirk.behme<at>  googlemail.com>  writes:
>
>
>
>>
>> E.g. what I did for OMAP3 SPI driver:
>>
>> /* OMAP3 McSPI registers */
>> struct mcspi_channel {
>> 	unsigned int chconf;		/* 0x2C, 0x40, 0x54, 0x68 */
>> 	unsigned int chstat;		/* 0x30, 0x44, 0x58, 0x6C */
>> 	unsigned int chctrl;		/* 0x34, 0x48, 0x5C, 0x70 */
>> 	unsigned int tx;		/* 0x38, 0x4C, 0x60, 0x74 */
>> 	unsigned int rx;		/* 0x3C, 0x50, 0x64, 0x78 */
>> };
>>
>
> [...]
>
>> writel(value, regs->modulctrl);
>>
>
> Hi Dirk,
>
> I could not find this code in any of the repositories. Could you share the
> u-boot omap3 spi driver?

I used the SPI driver for DaVinci (which this thread is about) as 
starting point for an U-Boot OMAP3 driver. The goal was to be able to 
access SPI ethernet on Zippy expansion board for Beagle. 
Unfortunately, I never found the time to finalize it.

Please find my latest patch from beginning of January 2010 in attachment.

I really hope it might help to finalize it!

Many thanks for asking and best regards

Dirk

P.S.: You might notice that the patch contains changes for the SPI 
ethernet, too. This should be moved to a separate patch.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: omap3_spi_patch.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20100307/3ea76be9/attachment.txt 

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2010-03-07 12:38 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-23  7:44 [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
2009-12-24  1:34 ` Mike Frysinger
2009-12-24  2:52   ` Sudhakar Rajashekhara
2009-12-25 17:11 ` Dirk Behme
2010-01-04  9:42   ` Sudhakar Rajashekhara
2010-01-04  9:46   ` Sudhakar Rajashekhara
2010-01-05  6:01     ` Mike Frysinger
2010-01-05  7:14       ` [U-Boot] NS16550 Register structure Pedanekar, Hemant
2010-01-05  7:35         ` Wolfgang Denk
2010-01-05  8:55           ` Pedanekar, Hemant
2010-01-05  9:58             ` Wolfgang Denk
2010-01-18 14:51               ` Pedanekar, Hemant
2010-01-19 13:04                 ` Detlev Zundel
2010-01-21 11:58                   ` Pedanekar, Hemant
2010-01-28  9:54                   ` Pedanekar, Hemant
2010-01-28 11:00                     ` Wolfgang Denk
2010-01-04  9:47 ` [U-Boot] [PATCH] TI DaVinci: Driver for the davinci SPI controller Nick Thompson
2010-01-04 10:42   ` Sudhakar Rajashekhara
2010-01-04 19:50     ` Dirk Behme
2010-01-05  0:20       ` Mike Frysinger
2010-03-05  6:47       ` Mansoor
2010-03-07 12:38         ` [U-Boot] OMAP3 SPI driver ( was Re: [PATCH] TI DaVinci: Driver for the davinci SPI controller) Dirk Behme

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.