[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver
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_ */
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
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
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@denx.de
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
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@denx.de
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
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@denx.de
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
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@denx.de
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
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@denx.de
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
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@denx.de
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
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
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
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@denx.de
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
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@denx.de
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
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
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
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
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
participants (4)
-
Ben Warren -
Guennadi Liakhovetski -
Haavard Skinnemoen -
Wolfgang Denk