Re: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
Liu Hui-R64343 wrote: Hi, Hi, static unsigned long spi_bases[] = { 0x43fa4000, 0x5001, 0x53f84000, }; Here hardcode the value in mx31, while in mx51 it use the macro. Which makes Code style not consistent. yes, agree, the driver already contains a lot of hard-coded values for mx.31 and I changed only according to the mx.51. As I see, in the imx-regs.h for MX.31 several defines are missing, and the drivers define theirselves the values. I will add the defines, at least for spi, to the mx31-regs.h in a separate patch. +static unsigned long spi_bases[] = { +CSPI1_BASE_ADDR, +CSPI2_BASE_ADDR, +CSPI3_BASE_ADDR, +}; See above comments. Ok, but this is correct. Only the MX31 part should be changed. struct mxc_spi_slave { struct spi_slave slave; unsigned long base; u32 ctrl_reg; +u32 cfg_reg; int gpio; }; Only MX51 use it, MX31 will not use it. However, I need a general structure to support both processors. Agree this register is available only on the MX.51 processor, I can surround the definition with an #ifdef CONFIG_MX51 statement. The function spi_cfg only used in MX51, will have compile warnings for MX31. Use CONFIG_MX51 to cover it. Agree, and as reported by Tom, there are other issues regarding the MX.31. I will check all of them globally and I will try to test on a MX.31 board, too. + * a single byte first, followed by 4 words. Comments is wrong, should be followed by 4 bytes Agree, it must be changed. + */ +if ((cnt_blk == 0) (bitlen % 32) +(j = ((bitlen % 32) / 8))) { +continue; +} +if (pbuf) +tmpdata = *pbuf++ | (tmpdata 8); +n_bytes--; +} +} +debug(writing TX FIFO 0x%x\n, tmpdata); +reg_write(mxcs-base + MXC_CSPITXDATA, tmpdata); } Can use word copy for the left part(cnt_blk !=0) to get high performance. Not sure, but it could be I have not get the real problem here. +if (din) +*pbuf++ = (tmpdata +((3 - spare_bytes - j) * 8)) The RX path should be logic wrong, spare_bytes not reset to zero and the data got not correct when data is not 4B alignement. Thanks, I will recheck the code, surely spare_bytes must be reset to zero. Regards, Stefano Babic -- = 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: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
Hi, -Original Message- From: u-boot-boun...@lists.denx.de [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Stefano Babic Sent: 2010年3月17日 0:22 To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi This patch add support for MX51 processor and supports transfer of multiple word in a single transation. Signed-off-by: Stefano Babic sba...@denx.de --- The patch adds support for the MX51 and wants to remove some limitation on the old driver. Actually, the buffer passed to the transfer function must be word-aligne, even if it is required to send a single byte. drivers/spi/mxc_spi.c | 357 + 1 files changed, 301 insertions(+), 56 deletions(-) diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 3a45200..b04fadc 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -24,6 +24,11 @@ #include asm/errno.h #include asm/io.h +#define MXC_CSPIRXDATA 0x00 +#define MXC_CSPITXDATA 0x04 +#define MXC_CSPICTRL 0x08 +#define MXC_CSPIPERIOD_32KHZ (1 15) + #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 */ @@ -31,13 +36,9 @@ #error i.MX27 CSPI not supported due to drastic differences in register definisions \ See linux mxc_spi driver from Freescale for details. -#else - +#elif defined(CONFIG_MX31) #include asm/arch/mx31.h -#define MXC_CSPIRXDATA 0x00 -#define MXC_CSPITXDATA 0x04 -#define MXC_CSPICTRL 0x08 #define MXC_CSPIINT 0x0C #define MXC_CSPIDMA 0x10 #define MXC_CSPISTAT 0x14 @@ -56,21 +57,63 @@ #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_CSPICTRL_MAXBITS 0x1f +#define MXC_CSPICTRL_TC (1 8) +#define MXC_CSPICTRL_RXOVF (1 6) #define MXC_CSPIPERIOD_32KHZ (1 15) +#define MAX_SPI_BYTES4 static unsigned long spi_bases[] = { 0x43fa4000, 0x5001, 0x53f84000, }; Here hardcode the value in mx31, while in mx51 it use the macro. Which makes Code style not consistent. +#elif defined(CONFIG_MX51) + +#define MXC_CSPICON 0x0C +#define MXC_CSPIINT 0x10 +#define MXC_CSPIDMA 0x14 +#define MXC_CSPISTAT 0x18 +#define MXC_CSPIPERIOD 0x1C +#define MXC_CSPIRESET0x00 +#include asm/arch/imx-regs.h +#include asm/arch/clock.h +#define MXC_CSPICTRL_EN (1 0) +#define MXC_CSPICTRL_MODE(1 1) +#define MXC_CSPICTRL_XCH (1 2) +#define MXC_CSPICTRL_CHIPSELECT(x) (((x) 0x3) 12) +#define MXC_CSPICTRL_BITCOUNT(x) (((x) 0xfff) 20) +#define MXC_CSPICTRL_PREDIV(x) (((x) 0xF) 12) +#define MXC_CSPICTRL_POSTDIV(x) (((x) 0xF) 8) +#define MXC_CSPICTRL_SELCHAN(x) (((x) 0x3) 18) +#define MXC_CSPICTRL_MAXBITS 0xfff +#define MXC_CSPICTRL_TC (1 7) +#define MXC_CSPICTRL_RXOVF (1 6) + +/* Bit position inside CTRL register to be associated with SS */ +#define MXC_CSPICTRL_CHAN18 + +/* Bit position inside CON register to be associated with SS */ +#define MXC_CSPICON_POL 4 +#define MXC_CSPICON_PHA 0 +#define MXC_CSPICON_SSPOL12 + +static unsigned long spi_bases[] = { + CSPI1_BASE_ADDR, + CSPI2_BASE_ADDR, + CSPI3_BASE_ADDR, +}; See above comments. +#else +#error Unsupported architecture #endif struct mxc_spi_slave { struct spi_slave slave; unsigned long base; u32 ctrl_reg; + u32 cfg_reg; int gpio; }; Only MX51 use it, MX31 will not use it. @@ -89,71 +132,262 @@ static inline void reg_write(unsigned long addr, u32 val) *(volatile unsigned long*)addr = val; } -static u32 spi_xchg_single(struct spi_slave *slave, u32 data, int bitlen, -unsigned long flags) +void spi_cs_activate(struct spi_slave *slave) { +#ifdef CONFIG_MX31 struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); - unsigned int cfg_reg = reg_read(mxcs-base + MXC_CSPICTRL); - - mxcs-ctrl_reg = (mxcs-ctrl_reg ~MXC_CSPICTRL_BITCOUNT(31)) | - MXC_CSPICTRL_BITCOUNT(bitlen - 1); + if (mxcs-gpio 0) + mx31_gpio_set(mxcs-gpio, mxcs-ctrl_reg MXC_CSPICTRL_SSPOL); +#endif } - if (cfg_reg != mxcs-ctrl_reg) - reg_write(mxcs-base + MXC_CSPICTRL, mxcs-ctrl_reg); +void spi_cs_deactivate(struct spi_slave *slave) { + struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); #ifdef +CONFIG_MX31 + if (mxcs-gpio 0) + mx31_gpio_set(mxcs-gpio
Re: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
Tom wrote: +#define MXC_CSPIRXDATA0x00 +#define MXC_CSPITXDATA0x04 +#define MXC_CSPICTRL0x08 +#define MXC_CSPIPERIOD_32KHZ(1 15) + Pulling these out and making them common may not be the best thing to do. Located here, it hides the #ifdef CONFIG_MX27 #elif defined (CONFIG_MX31) #elif defined (CONFIG_MX51) #else #endif I would prefer if you just kept the copies in the mx31, mx51 locations You are right - I will move them. In 'Add SPI support to mx51evk board' The MAX_SPI_BYTES was defined in the config file. Here it is defined for mx31 generally. You should be consistent. These would be a better place for the mx51 values as you only have to do it once. That is correct. And the value is not related to a particular board, setting this define in config file is definetely wrong. I will move it. Be constistent with mx31 Move the #includes to the first lines after the #elif The other #defines to follow. Ok, thanks. Stefano -- = 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: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
Stefano Babic wrote: This patch add support for MX51 processor and supports transfer of multiple word in a single transation. Signed-off-by: Stefano Babic sba...@denx.de --- The patch adds support for the MX51 and wants to remove some limitation on the old driver. Actually, the buffer passed to the transfer function must be word-aligne, even if it is required to send a single byte. It may be better to split this patch into 1. fixing the old driver 2. adding mx51 drivers/spi/mxc_spi.c | 357 + 1 files changed, 301 insertions(+), 56 deletions(-) diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 3a45200..b04fadc 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -24,6 +24,11 @@ #include asm/errno.h #include asm/io.h +#define MXC_CSPIRXDATA 0x00 +#define MXC_CSPITXDATA 0x04 +#define MXC_CSPICTRL 0x08 +#define MXC_CSPIPERIOD_32KHZ (1 15) + Pulling these out and making them common may not be the best thing to do. Located here, it hides the #ifdef CONFIG_MX27 #elif defined (CONFIG_MX31) #elif defined (CONFIG_MX51) #else #endif I would prefer if you just kept the copies in the mx31, mx51 locations #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 */ @@ -31,13 +36,9 @@ #error i.MX27 CSPI not supported due to drastic differences in register definisions \ See linux mxc_spi driver from Freescale for details. -#else - +#elif defined(CONFIG_MX31) #include asm/arch/mx31.h -#define MXC_CSPIRXDATA 0x00 -#define MXC_CSPITXDATA 0x04 -#define MXC_CSPICTRL 0x08 #define MXC_CSPIINT 0x0C #define MXC_CSPIDMA 0x10 #define MXC_CSPISTAT 0x14 @@ -56,21 +57,63 @@ #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_CSPICTRL_MAXBITS 0x1f +#define MXC_CSPICTRL_TC (1 8) +#define MXC_CSPICTRL_RXOVF (1 6) #define MXC_CSPIPERIOD_32KHZ (1 15) +#define MAX_SPI_BYTES4 In 'Add SPI support to mx51evk board' The MAX_SPI_BYTES was defined in the config file. Here it is defined for mx31 generally. You should be consistent. These would be a better place for the mx51 values as you only have to do it once. static unsigned long spi_bases[] = { 0x43fa4000, 0x5001, 0x53f84000, }; +#elif defined(CONFIG_MX51) + +#define MXC_CSPICON 0x0C +#define MXC_CSPIINT 0x10 +#define MXC_CSPIDMA 0x14 +#define MXC_CSPISTAT 0x18 +#define MXC_CSPIPERIOD 0x1C +#define MXC_CSPIRESET0x00 +#include asm/arch/imx-regs.h +#include asm/arch/clock.h Be constistent with mx31 Move the #includes to the first lines after the #elif The other #defines to follow. Tom +#define MXC_CSPICTRL_EN (1 0) +#define MXC_CSPICTRL_MODE(1 1) +#define MXC_CSPICTRL_XCH (1 2) +#define MXC_CSPICTRL_CHIPSELECT(x) (((x) 0x3) 12) +#define MXC_CSPICTRL_BITCOUNT(x) (((x) 0xfff) 20) +#define MXC_CSPICTRL_PREDIV(x) (((x) 0xF) 12) +#define MXC_CSPICTRL_POSTDIV(x) (((x) 0xF) 8) +#define MXC_CSPICTRL_SELCHAN(x) (((x) 0x3) 18) +#define MXC_CSPICTRL_MAXBITS 0xfff +#define MXC_CSPICTRL_TC (1 7) +#define MXC_CSPICTRL_RXOVF (1 6) + +/* Bit position inside CTRL register to be associated with SS */ +#define MXC_CSPICTRL_CHAN18 + +/* Bit position inside CON register to be associated with SS */ +#define MXC_CSPICON_POL 4 +#define MXC_CSPICON_PHA 0 +#define MXC_CSPICON_SSPOL12 + +static unsigned long spi_bases[] = { + CSPI1_BASE_ADDR, + CSPI2_BASE_ADDR, + CSPI3_BASE_ADDR, +}; +#else +#error Unsupported architecture #endif struct mxc_spi_slave { struct spi_slave slave; unsigned long base; u32 ctrl_reg; + u32 cfg_reg; int gpio; }; @@ -89,71 +132,262 @@ static inline void reg_write(unsigned long addr, u32 val) *(volatile unsigned long*)addr = val; } -static u32 spi_xchg_single(struct spi_slave *slave, u32 data, int bitlen, -unsigned long flags) +void spi_cs_activate(struct spi_slave *slave) { +#ifdef CONFIG_MX31 struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); - unsigned int cfg_reg = reg_read(mxcs-base + MXC_CSPICTRL); - - mxcs-ctrl_reg = (mxcs-ctrl_reg ~MXC_CSPICTRL_BITCOUNT(31)) | - MXC_CSPICTRL_BITCOUNT(bitlen - 1); + if (mxcs-gpio 0) + mx31_gpio_set(mxcs-gpio, mxcs-ctrl_reg MXC_CSPICTRL_SSPOL); +#endif +} - if (cfg_reg != mxcs-ctrl_reg) -
[U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
This patch add support for MX51 processor and supports transfer of multiple word in a single transation. Signed-off-by: Stefano Babic sba...@denx.de --- The patch adds support for the MX51 and wants to remove some limitation on the old driver. Actually, the buffer passed to the transfer function must be word-aligne, even if it is required to send a single byte. drivers/spi/mxc_spi.c | 357 + 1 files changed, 301 insertions(+), 56 deletions(-) diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 3a45200..b04fadc 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -24,6 +24,11 @@ #include asm/errno.h #include asm/io.h +#define MXC_CSPIRXDATA 0x00 +#define MXC_CSPITXDATA 0x04 +#define MXC_CSPICTRL 0x08 +#define MXC_CSPIPERIOD_32KHZ (1 15) + #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 */ @@ -31,13 +36,9 @@ #error i.MX27 CSPI not supported due to drastic differences in register definisions \ See linux mxc_spi driver from Freescale for details. -#else - +#elif defined(CONFIG_MX31) #include asm/arch/mx31.h -#define MXC_CSPIRXDATA 0x00 -#define MXC_CSPITXDATA 0x04 -#define MXC_CSPICTRL 0x08 #define MXC_CSPIINT0x0C #define MXC_CSPIDMA0x10 #define MXC_CSPISTAT 0x14 @@ -56,21 +57,63 @@ #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_CSPICTRL_MAXBITS 0x1f +#define MXC_CSPICTRL_TC(1 8) +#define MXC_CSPICTRL_RXOVF (1 6) #define MXC_CSPIPERIOD_32KHZ (1 15) +#define MAX_SPI_BYTES 4 static unsigned long spi_bases[] = { 0x43fa4000, 0x5001, 0x53f84000, }; +#elif defined(CONFIG_MX51) + +#define MXC_CSPICON0x0C +#define MXC_CSPIINT0x10 +#define MXC_CSPIDMA0x14 +#define MXC_CSPISTAT 0x18 +#define MXC_CSPIPERIOD 0x1C +#define MXC_CSPIRESET 0x00 +#include asm/arch/imx-regs.h +#include asm/arch/clock.h +#define MXC_CSPICTRL_EN(1 0) +#define MXC_CSPICTRL_MODE (1 1) +#define MXC_CSPICTRL_XCH (1 2) +#define MXC_CSPICTRL_CHIPSELECT(x) (((x) 0x3) 12) +#define MXC_CSPICTRL_BITCOUNT(x) (((x) 0xfff) 20) +#define MXC_CSPICTRL_PREDIV(x) (((x) 0xF) 12) +#define MXC_CSPICTRL_POSTDIV(x)(((x) 0xF) 8) +#define MXC_CSPICTRL_SELCHAN(x)(((x) 0x3) 18) +#define MXC_CSPICTRL_MAXBITS 0xfff +#define MXC_CSPICTRL_TC(1 7) +#define MXC_CSPICTRL_RXOVF (1 6) + +/* Bit position inside CTRL register to be associated with SS */ +#define MXC_CSPICTRL_CHAN 18 + +/* Bit position inside CON register to be associated with SS */ +#define MXC_CSPICON_POL4 +#define MXC_CSPICON_PHA0 +#define MXC_CSPICON_SSPOL 12 + +static unsigned long spi_bases[] = { + CSPI1_BASE_ADDR, + CSPI2_BASE_ADDR, + CSPI3_BASE_ADDR, +}; +#else +#error Unsupported architecture #endif struct mxc_spi_slave { struct spi_slave slave; unsigned long base; u32 ctrl_reg; + u32 cfg_reg; int gpio; }; @@ -89,71 +132,262 @@ static inline void reg_write(unsigned long addr, u32 val) *(volatile unsigned long*)addr = val; } -static u32 spi_xchg_single(struct spi_slave *slave, u32 data, int bitlen, - unsigned long flags) +void spi_cs_activate(struct spi_slave *slave) { +#ifdef CONFIG_MX31 struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); - unsigned int cfg_reg = reg_read(mxcs-base + MXC_CSPICTRL); - - mxcs-ctrl_reg = (mxcs-ctrl_reg ~MXC_CSPICTRL_BITCOUNT(31)) | - MXC_CSPICTRL_BITCOUNT(bitlen - 1); + if (mxcs-gpio 0) + mx31_gpio_set(mxcs-gpio, mxcs-ctrl_reg MXC_CSPICTRL_SSPOL); +#endif +} - if (cfg_reg != mxcs-ctrl_reg) - reg_write(mxcs-base + MXC_CSPICTRL, mxcs-ctrl_reg); +void spi_cs_deactivate(struct spi_slave *slave) +{ + struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); +#ifdef CONFIG_MX31 + if (mxcs-gpio 0) + mx31_gpio_set(mxcs-gpio, + !(mxcs-ctrl_reg MXC_CSPICTRL_SSPOL)); +#endif + reg_write(mxcs-base + MXC_CSPICTRL, 0); +} - if (mxcs-gpio 0 (flags SPI_XFER_BEGIN)) - mx31_gpio_set(mxcs-gpio, mxcs-ctrl_reg MXC_CSPICTRL_SSPOL); +static s32 spi_cfg(struct mxc_spi_slave *mxcs, unsigned int cs, + unsigned int max_hz, unsigned int mode) +{ + u32 clk_src = mxc_get_clock(MXC_CSPI_CLK); + s32 pre_div = 0, post_div = 0, i, reg_ctrl, reg_config; + u32 ss_pol = 0, sclkpol = 0, sclkpha =