[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

This is an SPI driver for i.MX and MXC based SoCs from Freescale. So far
only implemented and tested on i.MX31, can with a modified register layout
and definitions be used for i.MX27, I think, MXC CPUs have similar SPI
controllers too.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>

---

Changes since v1: fix a copy-paste error

 README                                |    5 +
 drivers/spi/Makefile                  |    1 +
 drivers/spi/mxc_spi.c                 |  166 +++++++++++++++++++++++++++++++++
 include/asm-arm/arch-mx31/mx31-regs.h |    7 +-
 include/spi.h                         |   16 +++-
 5 files changed, 193 insertions(+), 2 deletions(-)
 create mode 100644 drivers/spi/mxc_spi.c

diff --git a/README b/README
index 7c16345..d201b92 100644
--- a/README
+++ b/README
@@ -1414,6 +1414,11 @@ The following options need to be configured:
 		Currently supported on some MPC8xxx processors.  For an
 		example, see include/configs/mpc8349emds.h.
 
+		CONFIG_MXC_SPI
+
+		Enables the driver for the SPI controllers on i.MX and MXC
+		SoCs. Currently only i.MX31 is supported.
+
 - FPGA Support: CONFIG_FPGA
 
 		Enables FPGA subsystem.
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 0b7a2df..bc8a104 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
 LIB	:= $(obj)libspi.a
 
 COBJS-y += mpc8xxx_spi.o
+COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
new file mode 100644
index 0000000..b2e3ab9
--- /dev/null
+++ b/drivers/spi/mxc_spi.c
@@ -0,0 +1,166 @@
+/*
+ * Copyright (C) 2008, Guennadi Liakhovetski <lg@denx.de>
+ *
+ * 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 <asm/io.h>
+
+#ifdef CONFIG_MX27
+/* i.MX27 has a completely wrong register layout and register definitions in the
+ * datasheet, the correct one is in the Freescale's Linux driver */
+
+#error "i.MX27 CSPI not supported due to drastic differences in register definisions" \
+"See linux mxc_spi driver from Freescale for details."
+
+#else
+
+#define MXC_CSPIRXDATA		0x00
+#define MXC_CSPITXDATA		0x04
+#define MXC_CSPICTRL		0x08
+#define MXC_CSPIINT		0x0C
+#define MXC_CSPIDMA		0x10
+#define MXC_CSPISTAT		0x14
+#define MXC_CSPIPERIOD		0x18
+#define MXC_CSPITEST		0x1C
+#define MXC_CSPIRESET		0x00
+
+#define MXC_CSPICTRL_EN		(1 << 0)
+#define MXC_CSPICTRL_MODE	(1 << 1)
+#define MXC_CSPICTRL_XCH	(1 << 2)
+#define MXC_CSPICTRL_SMC	(1 << 3)
+#define MXC_CSPICTRL_POL	(1 << 4)
+#define MXC_CSPICTRL_PHA	(1 << 5)
+#define MXC_CSPICTRL_SSCTL	(1 << 6)
+#define MXC_CSPICTRL_SSPOL 	(1 << 7)
+#define MXC_CSPICTRL_CHIPSELECT(x)	(((x) & 0x3) << 24)
+#define MXC_CSPICTRL_BITCOUNT(x)	(((x) & 0x1f) << 8)
+#define MXC_CSPICTRL_DATARATE(x)	(((x) & 0x7) << 16)
+
+#define MXC_CSPIPERIOD_32KHZ	(1 << 15)
+
+static unsigned long spi_bases[] = {
+	0x43fa4000,
+	0x50010000,
+	0x53f84000,
+};
+
+static unsigned long spi_base;
+
+#endif
+
+spi_chipsel_type spi_chipsel[] = {
+	(spi_chipsel_type)0,
+	(spi_chipsel_type)1,
+	(spi_chipsel_type)2,
+	(spi_chipsel_type)3,
+};
+int spi_chipsel_cnt = sizeof(spi_chipsel) / sizeof(spi_chipsel[0]);
+
+static inline u32 reg_read(unsigned long addr)
+{
+	return *(volatile unsigned long*)addr;
+}
+
+static inline void reg_write(unsigned long addr, u32 val)
+{
+	*(volatile unsigned long*)addr = val;
+}
+
+static u32 spi_xchg_single(u32 data, int bitlen)
+{
+
+	unsigned int cfg_reg = reg_read(spi_base + MXC_CSPICTRL);
+
+	if (MXC_CSPICTRL_BITCOUNT(bitlen - 1) != (cfg_reg & MXC_CSPICTRL_BITCOUNT(31))) {
+		cfg_reg = (cfg_reg & ~MXC_CSPICTRL_BITCOUNT(31)) |
+			MXC_CSPICTRL_BITCOUNT(bitlen - 1);
+		reg_write(spi_base + MXC_CSPICTRL, cfg_reg);
+	}
+
+	reg_write(spi_base + MXC_CSPITXDATA, data);
+
+	cfg_reg |= MXC_CSPICTRL_XCH;
+
+	reg_write(spi_base + MXC_CSPICTRL, cfg_reg);
+
+	while (reg_read(spi_base + MXC_CSPICTRL) & MXC_CSPICTRL_XCH)
+		;
+
+	return reg_read(spi_base + MXC_CSPIRXDATA);
+}
+
+int spi_xfer(spi_chipsel_type chipsel, int bitlen, uchar *dout, uchar *din)
+{
+	int n_blks = (bitlen + 31) / 32;
+	u32 *out_l, *in_l;
+	int i;
+
+	if ((int)dout & 3 || (int)din & 3) {
+		printf("Error: unaligned buffers in: %p, out: %p\n", din, dout);
+		return 1;
+	}
+
+	if (!spi_base)
+		spi_select(CONFIG_MXC_SPI_IFACE, (int)chipsel, SPI_MODE_2 | SPI_CS_HIGH);
+
+	for (i = 0, in_l = (u32 *)din, out_l = (u32 *)dout;
+	     i < n_blks;
+	     i++, in_l++, out_l++, bitlen -= 32)
+		*in_l = spi_xchg_single(*out_l, bitlen);
+
+	return 0;
+}
+
+void spi_init(void)
+{
+}
+
+int spi_select(unsigned int bus, unsigned int dev, unsigned long mode)
+{
+	unsigned int ctrl_reg;
+
+	if (bus >= sizeof(spi_bases) / sizeof(spi_bases[0]) ||
+	    dev > 3)
+		return 1;
+
+	spi_base = spi_bases[bus];
+
+	ctrl_reg = MXC_CSPICTRL_CHIPSELECT(dev) |
+		MXC_CSPICTRL_BITCOUNT(31) |
+		MXC_CSPICTRL_DATARATE(7) | /* FIXME: calculate data rate */
+		MXC_CSPICTRL_EN |
+		MXC_CSPICTRL_MODE;
+
+	if (mode & SPI_CPHA)
+		ctrl_reg |= MXC_CSPICTRL_PHA;
+	if (!(mode & SPI_CPOL))
+		ctrl_reg |= MXC_CSPICTRL_POL;
+	if (mode & SPI_CS_HIGH)
+		ctrl_reg |= MXC_CSPICTRL_SSPOL;
+
+	reg_write(spi_base + MXC_CSPIRESET, 1);
+	udelay(1);
+	reg_write(spi_base + MXC_CSPICTRL, ctrl_reg);
+	reg_write(spi_base + MXC_CSPIPERIOD,
+		  MXC_CSPIPERIOD_32KHZ);
+	reg_write(spi_base + MXC_CSPIINT, 0);
+
+	return 0;
+}
diff --git a/include/asm-arm/arch-mx31/mx31-regs.h b/include/asm-arm/arch-mx31/mx31-regs.h
index 380b401..d04072e 100644
--- a/include/asm-arm/arch-mx31/mx31-regs.h
+++ b/include/asm-arm/arch-mx31/mx31-regs.h
@@ -37,6 +37,9 @@
 #define CCM_UPCTL	(CCM_BASE + 0x10)
 #define CCM_SPCTL	(CCM_BASE + 0x18)
 #define CCM_COSR	(CCM_BASE + 0x1C)
+#define CCM_CGR0	(CCM_BASE + 0x20)
+#define CCM_CGR1	(CCM_BASE + 0x24)
+#define CCM_CGR2	(CCM_BASE + 0x28)
 
 #define CCMR_MDS	(1 << 7)
 #define CCMR_SBYCS	(1 << 4)
@@ -118,7 +121,9 @@
 #define MUX_CTL_RXD1		0x82
 #define MUX_CTL_TXD1		0x83
 #define MUX_CTL_CSPI2_MISO	0x84
-/* 0x85 .. 0x8a */
+#define MUX_CTL_CSPI2_SS0	0x85
+#define MUX_CTL_CSPI2_SS1	0x86
+#define MUX_CTL_CSPI2_SS2	0x87
 #define MUX_CTL_CSPI2_MOSI	0x8b
 
 /* The modes a specific pin can be in
diff --git a/include/spi.h b/include/spi.h
index 03dc5bc..3a55a68 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -24,6 +24,18 @@
 #ifndef _SPI_H_
 #define _SPI_H_
 
+/* SPI mode flags */
+#define	SPI_CPHA	0x01			/* clock phase */
+#define	SPI_CPOL	0x02			/* clock polarity */
+#define	SPI_MODE_0	(0|0)			/* (original MicroWire) */
+#define	SPI_MODE_1	(0|SPI_CPHA)
+#define	SPI_MODE_2	(SPI_CPOL|0)
+#define	SPI_MODE_3	(SPI_CPOL|SPI_CPHA)
+#define	SPI_CS_HIGH	0x04			/* chipselect active high? */
+#define	SPI_LSB_FIRST	0x08			/* per-word bits-on-wire */
+#define	SPI_3WIRE	0x10			/* SI/SO signals shared */
+#define	SPI_LOOP	0x20			/* loopback mode */
+
 /*
  * The function call pointer type used to drive the chip select.
  */
@@ -68,6 +80,8 @@ void spi_init(void);
  *
  *   Returns: 0 on success, not 0 on failure
  */
-int  spi_xfer(spi_chipsel_type chipsel, int bitlen, uchar *dout, uchar *din);
+int spi_xfer(spi_chipsel_type chipsel, int bitlen, uchar *dout, uchar *din);
+
+int spi_select(unsigned int bus, unsigned int dev, unsigned long mode);
 
 #endif	/* _SPI_H_ */
-- 
1.5.4

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Wolfgang Denk]

In message <Pine.LNX.4.64.0804151412120.5159@axis700.grange> you wrote:
> This is an SPI driver for i.MX and MXC based SoCs from Freescale. So far
> only implemented and tested on i.MX31, can with a modified register layout
> and definitions be used for i.MX27, I think, MXC CPUs have similar SPI
> controllers too.
> 
> Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
> 
> ---
> 
> Changes since v1: fix a copy-paste error
> 
>  README                                |    5 +
>  drivers/spi/Makefile                  |    1 +
>  drivers/spi/mxc_spi.c                 |  166 +++++++++++++++++++++++++++++++++
>  include/asm-arm/arch-mx31/mx31-regs.h |    7 +-
>  include/spi.h                         |   16 +++-
>  5 files changed, 193 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/spi/mxc_spi.c

Applied, thanks.

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
If ignorance is bliss, why aren't there more happy people?

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <Pine.LNX.4.64.0804151412120.5159@axis700.grange> you wrote:
> > This is an SPI driver for i.MX and MXC based SoCs from Freescale. So far
> > only implemented and tested on i.MX31, can with a modified register layout
> > and definitions be used for i.MX27, I think, MXC CPUs have similar SPI
> > controllers too.
> > 
> > Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
> > 
> > ---
> > 
> > Changes since v1: fix a copy-paste error
> > 
> >  README                                |    5 +
> >  drivers/spi/Makefile                  |    1 +
> >  drivers/spi/mxc_spi.c                 |  166 +++++++++++++++++++++++++++++++++
> >  include/asm-arm/arch-mx31/mx31-regs.h |    7 +-
> >  include/spi.h                         |   16 +++-
> >  5 files changed, 193 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/spi/mxc_spi.c
> 
> Applied, thanks.

Oh great. We can do API changes without even mentioning it in the
change log now?

I'm beginning to get tired of rebasing my SPI patches. Maybe it was a
mistake to be open about the API changes I wanted to make...it would
have been much easier to just slam them in without saying anything or
caring about any breakage :-(

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> Wolfgang Denk <wd@denx.de> wrote:
> > In message <Pine.LNX.4.64.0804151412120.5159@axis700.grange> you wrote:
> > > This is an SPI driver for i.MX and MXC based SoCs from Freescale. So far
> > > only implemented and tested on i.MX31, can with a modified register layout
> > > and definitions be used for i.MX27, I think, MXC CPUs have similar SPI
> > > controllers too.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
> > > 
> > > ---
> > > 
> > > Changes since v1: fix a copy-paste error
> > > 
> > >  README                                |    5 +
> > >  drivers/spi/Makefile                  |    1 +
> > >  drivers/spi/mxc_spi.c                 |  166 +++++++++++++++++++++++++++++++++
> > >  include/asm-arm/arch-mx31/mx31-regs.h |    7 +-
> > >  include/spi.h                         |   16 +++-
> > >  5 files changed, 193 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/spi/mxc_spi.c
> > 
> > Applied, thanks.
> 
> Oh great. We can do API changes without even mentioning it in the
> change log now?

Right, sorry, should have mentioned it. Although, the API change is one 
added function spi_select(), which you do not have to implement. So, I 
don't think I have broken anything.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Wolfgang Denk]

In message <20080508134735.0f777149@hskinnemo-gx620.norway.atmel.com> you wrote:
>
> > Applied, thanks.
> 
> Oh great. We can do API changes without even mentioning it in the
> change log now?

Did I miss any complaints or NAK against this  patch?  I  didn't  see
any.

That was version 2 of the patch, so it seems  there  was  opportunity
for feedback before? You certainly do not expect me to check each and
every piece of code myself. That's why we post patches here - to give
you and others a chance for review.


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
A meeting is an event at which the minutes are kept and the hours are
lost.

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> > Oh great. We can do API changes without even mentioning it in the
> > change log now?  
> 
> Right, sorry, should have mentioned it. Although, the API change is one 
> added function spi_select(), which you do not have to implement. So, I 
> don't think I have broken anything.

The problem is that it is completely undocumented, and it appears to
have a lot of overlap with the spi_setup() function I proposed several
times a while back.

Besides, it's only optional up to the point where drivers start using
it...and the mc13783-rtc driver does appear to be using it, so it isn't
really optional anymore. And I suspect lots of other drivers really
_do_ need this kind of thing, which is why I proposed the spi_setup()
interface to begin with. You can always get around this by adding
tweaks to the board/cpu/driver code for your particular setup, but I
think very few drivers work as expected out of the box on a new board
or platform. So I don't think an interface like this _should_ be
optional.

Therefore, I'm going to remove it in the next version of my patchset.
If you can tell me how it's supposed to work, I can try to minimize the
breakage.

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <20080508134735.0f777149@hskinnemo-gx620.norway.atmel.com> you wrote:
> >
> > > Applied, thanks.
> > 
> > Oh great. We can do API changes without even mentioning it in the
> > change log now?
> 
> Did I miss any complaints or NAK against this  patch?  I  didn't  see
> any.

No. I had very limited access to my e-mail during most of April, so I
didn't see it until now.

> That was version 2 of the patch, so it seems  there  was  opportunity
> for feedback before? You certainly do not expect me to check each and
> every piece of code myself. That's why we post patches here - to give
> you and others a chance for review.

Yeah, I know, I should have complained before. But my main complaint is
that API changes are easy to miss when they aren't mentioned in the
change log, so I certainly understand if you didn't see it either.

I'd really prefer API changes being posted as separate patches so that
they can be discussed separately, and they should _at least_ be
mentioned in the change log. Even small, "optional" ones; IMO, u-boot
has serious issues with feature creep, so I try to at least put up some
resistance whenever I see something that may not have been properly
thought through.

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> Guennadi Liakhovetski <lg@denx.de> wrote:
> > > Oh great. We can do API changes without even mentioning it in the
> > > change log now?  
> > 
> > Right, sorry, should have mentioned it. Although, the API change is one 
> > added function spi_select(), which you do not have to implement. So, I 
> > don't think I have broken anything.
> 
> The problem is that it is completely undocumented, and it appears to
> have a lot of overlap with the spi_setup() function I proposed several
> times a while back.
> 
> Besides, it's only optional up to the point where drivers start using
> it...and the mc13783-rtc driver does appear to be using it, so it isn't
> really optional anymore. And I suspect lots of other drivers really
> _do_ need this kind of thing, which is why I proposed the spi_setup()
> interface to begin with. You can always get around this by adding
> tweaks to the board/cpu/driver code for your particular setup, but I
> think very few drivers work as expected out of the box on a new board
> or platform. So I don't think an interface like this _should_ be
> optional.
> 
> Therefore, I'm going to remove it in the next version of my patchset.
> If you can tell me how it's supposed to work, I can try to minimize the
> breakage.

Would be better if we could avoid any breakage completely, please.

I added this function, because 1) the current spi_xfer() doesn't support 
multiple SPI busses, and 2) is not particularly friendly when the SPI 
controller itself controls chipselects. As far as I can see your new 
proposed API doesn't solve the former of these problems either. One could 
get around this problem by numbering all chipselects on all busses 
through, but that would be too ugly. So, the spi_select does just that - 
selects a bus and a device to talk to. Of course this is racy, but as long 
as there's no multitasking, it should be ok.

As you certainly noticed while working on your API improvements, the 
current API is very unflexible. But as I didn't have resources to rework 
it completely and change multiple existing drivers, I chose the lesser 
evil - added an auxiliary function.

Wouldn't an API like

struct spi_slave *spi_attach(int bus, int cs); /* also does init */
int spi_xfer(struct spi_slave *slave, bitlen, dout, din);
void spi_detach(struct spi_slave *slave);

(approximately) be better?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > Therefore, I'm going to remove it in the next version of my patchset.
> > If you can tell me how it's supposed to work, I can try to minimize the
> > breakage.
> 
> Would be better if we could avoid any breakage completely, please.

That's certainly what I'm aiming for, but I need someone to help me
test it.

> I added this function, because 1) the current spi_xfer() doesn't support 
> multiple SPI busses, and 2) is not particularly friendly when the SPI 
> controller itself controls chipselects. As far as I can see your new 
> proposed API doesn't solve the former of these problems either. One could 
> get around this problem by numbering all chipselects on all busses 
> through, but that would be too ugly.

I don't think global chip select numbering would be _that_ ugly, but
yeah, maybe we should add a bus parameter as well.

> So, the spi_select does just that - 
> selects a bus and a device to talk to. Of course this is racy, but as long 
> as there's no multitasking, it should be ok.

Yeah, multitasking isn't an issue.

> As you certainly noticed while working on your API improvements, the 
> current API is very unflexible. But as I didn't have resources to rework 
> it completely and change multiple existing drivers, I chose the lesser 
> evil - added an auxiliary function.

Lesser evil compared to what exactly? Than participating in the
discussion about the new API?

> Wouldn't an API like
> 
> struct spi_slave *spi_attach(int bus, int cs); /* also does init */
> int spi_xfer(struct spi_slave *slave, bitlen, dout, din);
> void spi_detach(struct spi_slave *slave);
> 
> (approximately) be better?

I did propose something similar a while ago:

/* Set slave-specific parameters and enable SPI */
int spi_claim_bus(int cs, unsigned int max_hz, unsigned int mode);

/* Disable SPI */
void spi_release_bus(int cs);

But I have to say I like the idea about passing a spi_slave
"handle" around...

How about something like this?

/*
 * Set up communication parameters for a SPI slave. This doesn't
 * necessarily enable the controller.
 */
struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
		max_hz, unsigned int mode);

/*
 * Get exclusive access to the SPI bus for communicating with the given
 * slave. Returns a negative value if the bus is busy (drivers may omit
 * such checks if they don't want the extra code/data overhead.)
 */
int spi_claim_bus(struct spi_slave *slave);

/*
 * Release the SPI bus. This may disable the controller and/or put it
 * in low-power mode
 */
void spi_release_bus(struct spi_slave *slave);

/*
 * Transfer data on the SPI bus.
 *
 * flags can be a combination of any of the following bits:
 *   SPI_XFER_BEGIN: Assert CS before starting the transfer
 *   SPI_XFER_END: Deassert CS after the transfer
 */
int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
		void *din, unsigned long flags);

What do you think?

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> But I have to say I like the idea about passing a spi_slave
> "handle" around...
> 
> How about something like this?
> 
> /*
>  * Set up communication parameters for a SPI slave. This doesn't
>  * necessarily enable the controller.
>  */
> struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
> 		max_hz, unsigned int mode);

Don't quite like the "create" name. To me it sounds like "we just create a 
slave object for you, do what you want with it." Whereas, I was thinking 
about an "attach" method, where the host driver adds the slave pointer to 
its driver list, possibly initializes the hardware, and can always verify 
whether the slave pointer it is handed in by one of further method is 
indeed one of the drivers on the list.

> /*
>  * Get exclusive access to the SPI bus for communicating with the given
>  * slave. Returns a negative value if the bus is busy (drivers may omit
>  * such checks if they don't want the extra code/data overhead.)
>  */
> int spi_claim_bus(struct spi_slave *slave);

Is this needed? Getting the spi_slave handle you gain exclusive access to 
the spi device, but claiming the whole bus? Drivers may be lazy releasing 
the chip-select between transfers, but are there cases where you _really_ 
must have exclusive access to the bus or cannot release the cs?

> /*
>  * Release the SPI bus. This may disable the controller and/or put it
>  * in low-power mode
>  */
> void spi_release_bus(struct spi_slave *slave);
> 
> /*
>  * Transfer data on the SPI bus.
>  *
>  * flags can be a combination of any of the following bits:
>  *   SPI_XFER_BEGIN: Assert CS before starting the transfer
>  *   SPI_XFER_END: Deassert CS after the transfer
>  */
> int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
> 		void *din, unsigned long flags);

See above - would anything break if we just deassert the cs between 
transfers?

> What do you think?

Yes, looks good, just let's iron out my remarks above, and see if anyone 
else has any comments.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> 
> > But I have to say I like the idea about passing a spi_slave
> > "handle" around...
> > 
> > How about something like this?
> > 
> > /*
> >  * Set up communication parameters for a SPI slave. This doesn't
> >  * necessarily enable the controller.
> >  */
> > struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
> > 		max_hz, unsigned int mode);
> 
> Don't quite like the "create" name. To me it sounds like "we just create a 
> slave object for you, do what you want with it." Whereas, I was thinking 
> about an "attach" method, where the host driver adds the slave pointer to 
> its driver list, possibly initializes the hardware, and can always verify 
> whether the slave pointer it is handed in by one of further method is 
> indeed one of the drivers on the list.

Ok. spi_setup_slave()? spi_init_slave()? I'm not a huge fan of "attach"
since it implies that some global state changes. It should be perfectly
acceptable for a driver to simply initialize a spi_slave struct and
return it, without updating any internal lists or hardware state.

> > /*
> >  * Get exclusive access to the SPI bus for communicating with the given
> >  * slave. Returns a negative value if the bus is busy (drivers may omit
> >  * such checks if they don't want the extra code/data overhead.)
> >  */
> > int spi_claim_bus(struct spi_slave *slave);
> 
> Is this needed? Getting the spi_slave handle you gain exclusive access to 
> the spi device, but claiming the whole bus? Drivers may be lazy releasing 
> the chip-select between transfers, but are there cases where you _really_ 
> must have exclusive access to the bus or cannot release the cs?

For some controllers, I believe claiming/releasing the bus explicitly
makes a lot of sense. For example, the SPI controller on AT91 and AVR32
allows you to select a given slave by writing to a register, and then
start transferring data, toggling chip selects as needed.

If a driver is sloppy with the chip select control, it's buggy. Many
SPI devices have very strict (and often strange) requirements about
when the chip select should be asserted and when it should not.

I think a claim/release interface is the best way to ensure that a
device driver can do multiple transfers more or less atomically (i.e.
without releasing the chip select and without having any traffic on the
bus in between) in a flexible way.

> > /*
> >  * Release the SPI bus. This may disable the controller and/or put it
> >  * in low-power mode
> >  */
> > void spi_release_bus(struct spi_slave *slave);
> > 
> > /*
> >  * Transfer data on the SPI bus.
> >  *
> >  * flags can be a combination of any of the following bits:
> >  *   SPI_XFER_BEGIN: Assert CS before starting the transfer
> >  *   SPI_XFER_END: Deassert CS after the transfer
> >  */
> > int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
> > 		void *din, unsigned long flags);
> 
> See above - would anything break if we just deassert the cs between 
> transfers?

Yes.

We've had so many chipselect-related problems in Linux that it's
not even funny. Controllers, drivers, devices, everything seems to be
making assumptions about how the chip select is supposed to behave -- I
think we should at least try not to make any assumptions on the
interface level.

Maybe we can do it in a different way, but simply saying that the chip
select should be deasserted between transfers is broken.

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> Guennadi Liakhovetski <lg@denx.de> wrote:
> > On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > 
> > > But I have to say I like the idea about passing a spi_slave
> > > "handle" around...
> > > 
> > > How about something like this?
> > > 
> > > /*
> > >  * Set up communication parameters for a SPI slave. This doesn't
> > >  * necessarily enable the controller.
> > >  */
> > > struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
> > > 		max_hz, unsigned int mode);
> > 
> > Don't quite like the "create" name. To me it sounds like "we just create a 
> > slave object for you, do what you want with it." Whereas, I was thinking 
> > about an "attach" method, where the host driver adds the slave pointer to 
> > its driver list, possibly initializes the hardware, and can always verify 
> > whether the slave pointer it is handed in by one of further method is 
> > indeed one of the drivers on the list.
> 
> Ok. spi_setup_slave()? spi_init_slave()? I'm not a huge fan of "attach"
> since it implies that some global state changes. It should be perfectly
> acceptable for a driver to simply initialize a spi_slave struct and
> return it, without updating any internal lists or hardware state.

Ok, let's go for spi_setup_slave then:-)

> > > /*
> > >  * Get exclusive access to the SPI bus for communicating with the given
> > >  * slave. Returns a negative value if the bus is busy (drivers may omit
> > >  * such checks if they don't want the extra code/data overhead.)
> > >  */
> > > int spi_claim_bus(struct spi_slave *slave);
> > 
> > Is this needed? Getting the spi_slave handle you gain exclusive access to 
> > the spi device, but claiming the whole bus? Drivers may be lazy releasing 
> > the chip-select between transfers, but are there cases where you _really_ 
> > must have exclusive access to the bus or cannot release the cs?
> 
> For some controllers, I believe claiming/releasing the bus explicitly
> makes a lot of sense. For example, the SPI controller on AT91 and AVR32
> allows you to select a given slave by writing to a register, and then
> start transferring data, toggling chip selects as needed.
> 
> If a driver is sloppy with the chip select control, it's buggy. Many
> SPI devices have very strict (and often strange) requirements about
> when the chip select should be asserted and when it should not.
> 
> I think a claim/release interface is the best way to ensure that a
> device driver can do multiple transfers more or less atomically (i.e.
> without releasing the chip select and without having any traffic on the
> bus in between) in a flexible way.

Is it compulsory then to claim / release a bus or not? Looks like it is 
not from your description above. But - even if you don't claim a bus for 
exclusive access, you still have to release it, right? Otherwise, if 
someone has setup a slave without claiming the bus, noone will ever be 
able to claim it afterwards? Because you shouldn't be able to claim it as 
long as there are even non-exclusive users on the bus? Actually, what's 
the mode variable in the setup method for? Maybe we could just use it to 
configure exclusive / non-exclusive access? Then you get a simple API like

slave = spi_setup_slave(host, dev, hz, 0 /*| SPI_ACCESS_EXCLUSIVE */);
if (!slave)
	return;
/* Now you MUST release the slave / host */
spi_xfer();
...
spi_release();

And you get access to the bus if there are no exclusive users, and you get 
exclusive access, only if there are no users currently at all.

> > > /*
> > >  * Release the SPI bus. This may disable the controller and/or put it
> > >  * in low-power mode
> > >  */
> > > void spi_release_bus(struct spi_slave *slave);
> > > 
> > > /*
> > >  * Transfer data on the SPI bus.
> > >  *
> > >  * flags can be a combination of any of the following bits:
> > >  *   SPI_XFER_BEGIN: Assert CS before starting the transfer
> > >  *   SPI_XFER_END: Deassert CS after the transfer
> > >  */
> > > int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
> > > 		void *din, unsigned long flags);
> > 
> > See above - would anything break if we just deassert the cs between 
> > transfers?
> 
> Yes.
> 
> We've had so many chipselect-related problems in Linux that it's
> not even funny. Controllers, drivers, devices, everything seems to be
> making assumptions about how the chip select is supposed to behave -- I
> think we should at least try not to make any assumptions on the
> interface level.
> 
> Maybe we can do it in a different way, but simply saying that the chip
> select should be deasserted between transfers is broken.

What do we do, if there are several non-exclusive users on a bus, and one 
of them doesn't release its cs between transfers? Return an error to 
others if they try to communicate at this time?

Looks like most of this API implementation is hardware independent, and 
should go into an spi.c?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > I think a claim/release interface is the best way to ensure that a
> > device driver can do multiple transfers more or less atomically (i.e.
> > without releasing the chip select and without having any traffic on the
> > bus in between) in a flexible way.
> 
> Is it compulsory then to claim / release a bus or not? Looks like it is 
> not from your description above. But - even if you don't claim a bus for 
> exclusive access, you still have to release it, right? Otherwise, if 
> someone has setup a slave without claiming the bus, noone will ever be 
> able to claim it afterwards? Because you shouldn't be able to claim it as 
> long as there are even non-exclusive users on the bus? Actually, what's 
> the mode variable in the setup method for? Maybe we could just use it to 
> configure exclusive / non-exclusive access? Then you get a simple API like

I'd say we should either make it compulsory or forget about it. You
always have exclusive access to the bus while doing a transfer, so
distinguishing between "exclusive" and "non-exclusive" users just
complicates things.

So we should just define spi_claim_bus() as a way of letting the
controller driver know that "I'm going to start talking to this
particular slave now", and let it handle it any way it chooses. We
could also use the interface to add debugging checks to verify that
we're not trying to access a different slaves while some driver thinks
it has exclusive access to the bus.

The "mode" variable in the setup function means exactly the same as it
does in your spi_select() function -- a bitwise combination of various
communication parameters like clock polarity, clock phase, etc.

Not all drivers may actually _need_ to do anything in the claim/release
functions though. I just think the interface makes sense conceptually,
and I also think it might improve performance with some drivers.

Another reason to have the claim/release interface is to allow
controller drivers to aggressively turn the controller on/off in order
to save power, or to ensure that everything is properly switched off
when booting the OS.

> slave = spi_setup_slave(host, dev, hz, 0 /*| SPI_ACCESS_EXCLUSIVE */);
> if (!slave)
> 	return;
> /* Now you MUST release the slave / host */
> spi_xfer();
> ...
> spi_release();

I think you should set up a slave exactly once before starting to
communicate with it. Once you've done that, you just execute sequences
of

spi_claim_bus(slave);
spi_xfer(slave, ...);
/* possibly more transfers */
spi_release_bus(slave);

We could also define commonly-used "canned" sequences, like spi_w8r8()
(write 8 bits, read 8 bits).

> And you get access to the bus if there are no exclusive users, and you get 
> exclusive access, only if there are no users currently at all.

I don't see how having multiple active users on the bus at the same
time can possibly make any sense...once you start a transfer, you
better make sure nobody else is doing a transfer at the same time since
you're using the same MOSI/MISO/SCK lines...

> What do we do, if there are several non-exclusive users on a bus, and one 
> of them doesn't release its cs between transfers? Return an error to 
> others if they try to communicate at this time?

That's a bug, but there are several ways the controller driver can try
to recover. The best one is probably to just deselect the CS that was
left active, possibly complaining a bit about it to the console.

spi_release_bus() could deactivate CS implicitly, making sure this
situation never happens in the first place.

> Looks like most of this API implementation is hardware independent, and 
> should go into an spi.c?

Not really...what "claiming" the bus really means is highly hardware
dependent. And SPI slave setup is mostly about decoding
hardware-independent parameters like SCK rate and mode bits into
hardware register values. But any convenience wrappers like spi_w8r8()
probably belongs somewhere hardware-independent.

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> Guennadi Liakhovetski <lg@denx.de> wrote:
> 
> > slave = spi_setup_slave(host, dev, hz, 0 /*| SPI_ACCESS_EXCLUSIVE */);
> > if (!slave)
> > 	return;
> > /* Now you MUST release the slave / host */
> > spi_xfer();
> > ...
> > spi_release();
> 
> I think you should set up a slave exactly once before starting to
> communicate with it. Once you've done that, you just execute sequences
> of
> 
> spi_claim_bus(slave);
> spi_xfer(slave, ...);
> /* possibly more transfers */
> spi_release_bus(slave);
> 
> We could also define commonly-used "canned" sequences, like spi_w8r8()
> (write 8 bits, read 8 bits).

Ok, I see what you mean now. But then we need another function - an 
opposite to spi_setup, to free any allocated RAM, etc. I thought this was 
going to happen in release_bus, but after this explanation this doesn't 
seem to be the case.

So, just add an spi_free(), and, as a counterpart to it, an spi_init() 
might sound better than spi_setup:-)

> > And you get access to the bus if there are no exclusive users, and you get 
> > exclusive access, only if there are no users currently at all.
> 
> I don't see how having multiple active users on the bus at the same
> time can possibly make any sense...once you start a transfer, you
> better make sure nobody else is doing a transfer at the same time since
> you're using the same MOSI/MISO/SCK lines...

No, not "active." Multiple users having set up different slaves. But not 
communicating simultaneously:-)

> > Looks like most of this API implementation is hardware independent, and 
> > should go into an spi.c?
> 
> Not really...what "claiming" the bus really means is highly hardware
> dependent. And SPI slave setup is mostly about decoding
> hardware-independent parameters like SCK rate and mode bits into
> hardware register values. But any convenience wrappers like spi_w8r8()
> probably belongs somewhere hardware-independent.

I thought, like (pseudocode)

static struct spi_host busses[SPI_BUSSES];

struct spi_slave *spi_init()
{
	list_for_each_entry(slave, &busses[bus].slaves, list) {
		if (slave->device == device)
			return (struct spi_slave *)-EBUSY;
	}
	slave = malloc();
	slave->bus = bus;
	slave->device = device;
	ret = busses[bus].init(slave);
	if (ret) {
		free(slave);
		return (struct spi_slave *)ret;
	}
	return slave;
}

int spi_xfer()
{
	list_for_each_entry(ix, &busses[bus].slaves, list) {
		if (ix == slave)
			break;
	}
	if (ix != slave)
		return -EINVAL;

	if (slave->bus->busy)
		return -EBUSY;

	return slave->bus->xfer();
}

...and so on, which is all quite hardware-independent.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> Ok, I see what you mean now. But then we need another function - an 
> opposite to spi_setup, to free any allocated RAM, etc. I thought this was 
> going to happen in release_bus, but after this explanation this doesn't 
> seem to be the case.
> 
> So, just add an spi_free(), and, as a counterpart to it, an spi_init() 
> might sound better than spi_setup:-)

Yes, perhaps we need that. But I was sort of thinking that once you
initialize a slave, it will just stick around until you boot an OS or
reset. Most drivers don't seem to have any cleanup hooks anyway, and
some don't even have init hooks. In the latter case, you can simply
keep a static struct spi_slave * around and if it's NULL, you
initialize it.

Normally, having an allocation function without a corresponding free
would seem like a bad design, but in U-Boot's case, I'm not so sure...

> > > And you get access to the bus if there are no exclusive users, and you get 
> > > exclusive access, only if there are no users currently at all.
> > 
> > I don't see how having multiple active users on the bus at the same
> > time can possibly make any sense...once you start a transfer, you
> > better make sure nobody else is doing a transfer at the same time since
> > you're using the same MOSI/MISO/SCK lines...
> 
> No, not "active." Multiple users having set up different slaves. But not 
> communicating simultaneously:-)

Ah, but they would only claim the bus when they're actually going to
communicate :-)

> > > Looks like most of this API implementation is hardware independent, and 
> > > should go into an spi.c?
> > 
> > Not really...what "claiming" the bus really means is highly hardware
> > dependent. And SPI slave setup is mostly about decoding
> > hardware-independent parameters like SCK rate and mode bits into
> > hardware register values. But any convenience wrappers like spi_w8r8()
> > probably belongs somewhere hardware-independent.
> 
> I thought, like (pseudocode)
> 
> static struct spi_host busses[SPI_BUSSES];
> 
> struct spi_slave *spi_init()
> {
> 	list_for_each_entry(slave, &busses[bus].slaves, list) {
> 		if (slave->device == device)
> 			return (struct spi_slave *)-EBUSY;
> 	}
> 	slave = malloc();
> 	slave->bus = bus;
> 	slave->device = device;
> 	ret = busses[bus].init(slave);
> 	if (ret) {
> 		free(slave);
> 		return (struct spi_slave *)ret;
> 	}
> 	return slave;
> }
>
> int spi_xfer()
> {
> 	list_for_each_entry(ix, &busses[bus].slaves, list) {
> 		if (ix == slave)
> 			break;
> 	}
> 	if (ix != slave)
> 		return -EINVAL;
> 
> 	if (slave->bus->busy)
> 		return -EBUSY;
> 
> 	return slave->bus->xfer();
> }

I was thinking about something much simpler:

struct spi_slave *spi_init_slave(bus, cs, max_hz, mode)
{
	slave = malloc();
	slave->regs = get_spi_controller_base_address(bus);
	slave->mode_reg = get_suitable_settings_for(cs, max_hz, mode);
	return slave;
}

int spi_xfer(slave, ...)
{
	__raw_writel(slave->mode_reg, slave->regs + SPI_MR);
	if (flags & SPI_XFER_BEGIN)
		assert_chip_select();

	do_the_actual_spi_transfer();

	if (flags & SPI_XFER_END)
		deassert_chip_select();

	return whatever;
}

Of course, your solution will work with multiple, different SPI
controllers while mine won't, but is that really necessary?

Your solution comes with more error checking as well, which might be a
good thing, but since it comes with a cost of additional memory and
flash footprint, I think it should be optional. Maybe we could provide
some library functions to simplify the drivers that want this?

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> > I thought, like (pseudocode)
> > 
> > static struct spi_host busses[SPI_BUSSES];
> > 
> > struct spi_slave *spi_init()
> > {
> > 	list_for_each_entry(slave, &busses[bus].slaves, list) {
> > 		if (slave->device == device)
> > 			return (struct spi_slave *)-EBUSY;
> > 	}
> > 	slave = malloc();
> > 	slave->bus = bus;
> > 	slave->device = device;
> > 	ret = busses[bus].init(slave);
> > 	if (ret) {
> > 		free(slave);
> > 		return (struct spi_slave *)ret;
> > 	}
> > 	return slave;
> > }
> >
> > int spi_xfer()
> > {
> > 	list_for_each_entry(ix, &busses[bus].slaves, list) {
> > 		if (ix == slave)
> > 			break;
> > 	}
> > 	if (ix != slave)
> > 		return -EINVAL;
> > 
> > 	if (slave->bus->busy)
> > 		return -EBUSY;
> > 
> > 	return slave->bus->xfer();
> > }
> 
> I was thinking about something much simpler:
> 
> struct spi_slave *spi_init_slave(bus, cs, max_hz, mode)
> {
> 	slave = malloc();
> 	slave->regs = get_spi_controller_base_address(bus);
> 	slave->mode_reg = get_suitable_settings_for(cs, max_hz, mode);
> 	return slave;
> }
> 
> int spi_xfer(slave, ...)
> {
> 	__raw_writel(slave->mode_reg, slave->regs + SPI_MR);
> 	if (flags & SPI_XFER_BEGIN)
> 		assert_chip_select();
> 
> 	do_the_actual_spi_transfer();
> 
> 	if (flags & SPI_XFER_END)
> 		deassert_chip_select();
> 
> 	return whatever;
> }
> 
> Of course, your solution will work with multiple, different SPI
> controllers while mine won't, but is that really necessary?
> 
> Your solution comes with more error checking as well, which might be a
> good thing, but since it comes with a cost of additional memory and
> flash footprint, I think it should be optional. Maybe we could provide
> some library functions to simplify the drivers that want this?

I see. Well, I don't have a strong preference. So, either we need more 
votes, or the one who implements it decides:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg@denx.de> wrote:
> I see. Well, I don't have a strong preference. So, either we need more 
> votes, or the one who implements it decides:-)

I'll see if I can come up with a patch tomorrow, and Cc a few people
that have shown some interest earlier...I think we've reached the point
where we need some actual code to discuss things further :-)

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Wolfgang Denk]

In message <20080508181708.60e8b3e9@hskinnemo-gx620.norway.atmel.com> you wrote:
>
> Yes, perhaps we need that. But I was sort of thinking that once you
> initialize a slave, it will just stick around until you boot an OS or
> reset. Most drivers don't seem to have any cleanup hooks anyway, and
> some don't even have init hooks. In the latter case, you can simply
> keep a static struct spi_slave * around and if it's NULL, you
> initialize it.

Even if there is lots of bad examples, it is a design requirement to
disable a hardware interface after use. See bullet 6 at
http://www.denx.de/wiki/UBoot/DesignRequirements

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
If the odds are a million to one against something occuring,  chances
are 50-50 it will.

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Wolfgang Denk]

In message <Pine.LNX.4.64.0805081824310.5839@axis700.grange> you wrote:
> 
> > Of course, your solution will work with multiple, different SPI
> > controllers while mine won't, but is that really necessary?
> > 
> > Your solution comes with more error checking as well, which might be a
> > good thing, but since it comes with a cost of additional memory and
> > flash footprint, I think it should be optional. Maybe we could provide
> > some library functions to simplify the drivers that want this?
> 
> I see. Well, I don't have a strong preference. So, either we need more 
> votes, or the one who implements it decides:-)

That was two pros - did I miss any cons ?

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
I don't know if it's what you want, but it's what you get.  :-)
                      - Larry Wall in <10502@jpl-devvax.JPL.NASA.GOV>

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Wolfgang Denk wrote:

> In message <Pine.LNX.4.64.0805081824310.5839@axis700.grange> you wrote:
> > 
> > > Of course, your solution will work with multiple, different SPI
> > > controllers while mine won't, but is that really necessary?
> > > 
> > > Your solution comes with more error checking as well, which might be a
> > > good thing, but since it comes with a cost of additional memory and
> > > flash footprint, I think it should be optional. Maybe we could provide
> > > some library functions to simplify the drivers that want this?
> > 
> > I see. Well, I don't have a strong preference. So, either we need more 
> > votes, or the one who implements it decides:-)
> 
> That was two pros - did I miss any cons ?

I think, those were two pros - but for two somewhat different solutions.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Ben Warren]

Haavard Skinnemoen wrote:
> Guennadi Liakhovetski <lg@denx.de> wrote:
>   
>> I see. Well, I don't have a strong preference. So, either we need more 
>> votes, or the one who implements it decides:-)
>>     
>
> I'll see if I can come up with a patch tomorrow, and Cc a few people
> that have shown some interest earlier...I think we've reached the point
> where we need some actual code to discuss things further :-)
>   
Being able to compare code would be good.  I can't keep track of this 
conversation.  The only impression is that a very simple interface (SPI) 
seems to be getting very complicated.  Let's remember this is a 
bootloader that typically runs for a few seconds.

cheers,
Ben

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Wolfgang Denk]

In message <Pine.LNX.4.64.0805082103120.8518@axis700.grange> you wrote:
> 
> > > > Of course, your solution will work with multiple, different SPI
> > > > controllers while mine won't, but is that really necessary?
> > > > 
> > > > Your solution comes with more error checking as well, which might be a
> > > > good thing, but since it comes with a cost of additional memory and
> > > > flash footprint, I think it should be optional. Maybe we could provide
> > > > some library functions to simplify the drivers that want this?
> > > 
> > > I see. Well, I don't have a strong preference. So, either we need more 
> > > votes, or the one who implements it decides:-)
> > 
> > That was two pros - did I miss any cons ?
> 
> I think, those were two pros - but for two somewhat different solutions.

Oops?

"your solution will work with multiple, different SPI controllers" and
"Your solution comes with more error checking as well"
seem to me as if it were 2 x pro for your code.

Am I missing something?

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
What's the sound a name makes when it's dropped?

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Wolfgang Denk wrote:

> In message <Pine.LNX.4.64.0805082103120.8518@axis700.grange> you wrote:
> > 
> > > > > Of course, your solution will work with multiple, different SPI
> > > > > controllers while mine won't, but is that really necessary?
> > > > > 
> > > > > Your solution comes with more error checking as well, which might be a
> > > > > good thing, but since it comes with a cost of additional memory and
> > > > > flash footprint, I think it should be optional. Maybe we could provide
> > > > > some library functions to simplify the drivers that want this?
> > > > 
> > > > I see. Well, I don't have a strong preference. So, either we need more 
> > > > votes, or the one who implements it decides:-)
> > > 
> > > That was two pros - did I miss any cons ?
> > 
> > I think, those were two pros - but for two somewhat different solutions.
> 
> Oops?
> 
> "your solution will work with multiple, different SPI controllers" and
> "Your solution comes with more error checking as well"
> seem to me as if it were 2 x pro for your code.
> 
> Am I missing something?

Haavard also named disadvantages of my proposal - like larger storage and 
memory footprint, higher complexity, etc. And as he is going to implement 
it, I think, he has the final say on this - until we see the code at 
least:-) He also has more experience with SPI than I. Of course, I feel a 
bit uncomfartable building restrictions in directly during design, like 
inability to use different SPI controllers, but I cannot estimate how 
probable it is, that we ever have to deal with this in U-Boot.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <Pine.LNX.4.64.0805081824310.5839@axis700.grange> you wrote:
> >   
> > > Of course, your solution will work with multiple, different SPI
> > > controllers while mine won't, but is that really necessary?
> > > 
> > > Your solution comes with more error checking as well, which might be a
> > > good thing, but since it comes with a cost of additional memory and
> > > flash footprint, I think it should be optional. Maybe we could provide
> > > some library functions to simplify the drivers that want this?  
> > 
> > I see. Well, I don't have a strong preference. So, either we need more 
> > votes, or the one who implements it decides:-)  
> 
> That was two pros - did I miss any cons ?

Simplicity, flash and RAM footprint. Also, you missed my question: "is
that really necessary?". If it is, we should probably take the extra
effort to support it. If it isn't, we should keep the code simple.

So is it necessary? It's only a pro if it is actually useful to someone.

As for error handling, we can add a small, optional SPI slave registry
library which can be used to debug new SPI slave drivers, but that may
be turned off on stable systems to save a bit of flash and RAM.

But my main argument is that we should keep everything simple enough so
that we don't _need_ any mandatory mid-layer.

Haavard

[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <20080508181708.60e8b3e9@hskinnemo-gx620.norway.atmel.com> you wrote:
> >
> > Yes, perhaps we need that. But I was sort of thinking that once you
> > initialize a slave, it will just stick around until you boot an OS or
> > reset. Most drivers don't seem to have any cleanup hooks anyway, and
> > some don't even have init hooks. In the latter case, you can simply
> > keep a static struct spi_slave * around and if it's NULL, you
> > initialize it.  
> 
> Even if there is lots of bad examples, it is a design requirement to
> disable a hardware interface after use. See bullet 6 at
> http://www.denx.de/wiki/UBoot/DesignRequirements

I know. That's what the spi_claim_bus() and spi_release_bus() hooks are
supposed to do. I just don't see the point of deallocating the
spi_slave struct memory; this has nothing to do with disabling hardware.

Haavard