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 =
