Patchwork [U-Boot,6/7] mxc_spi: add support for i.MX35 processor

login
register
mail settings
Submitter Stefano Babic
Date Jan. 14, 2011, 1:35 p.m.
Message ID <1295012124-15551-6-git-send-email-sbabic@denx.de>
Download mbox | patch
Permalink /patch/78903/
State Superseded
Delegated to: Stefano Babic
Headers show

Comments

Stefano Babic - Jan. 14, 2011, 1:35 p.m.
Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 drivers/spi/mxc_spi.c |   96 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 73 insertions(+), 23 deletions(-)
Wolfgang Denk - Jan. 19, 2011, 7:48 a.m.
Dear Stefano Babic,

In message <1295012124-15551-6-git-send-email-sbabic@denx.de> you wrote:
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/spi/mxc_spi.c |   96 +++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 73 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> index d558137..b353c83 100644
> --- a/drivers/spi/mxc_spi.c
> +++ b/drivers/spi/mxc_spi.c
> @@ -70,6 +70,8 @@ static unsigned long spi_bases[] = {
>  	0x53f84000,
>  };
>  
> +#define spi_cfg	spi_cfg_mx3
...
> +#define spi_cfg	spi_cfg_mx51

Hm... this repeats below, but in the end both spi_cfg_mx3() and
spi_cfg_mx51() are just static functions within the same source file,
with #ifdef's around them so only one can ever be enabled at a time.

I suggest you omit all these "#define spi_cfg" lines and rename both
versions of these functions into spi_cfg_mx().

> +#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

As mentioned before: please use a C struct.



Best regards,

Wolfgang Denk
Stefano Babic - Jan. 19, 2011, 10:09 a.m.
On 01/19/2011 08:48 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 
> In message <1295012124-15551-6-git-send-email-sbabic@denx.de> you wrote:
>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>> ---
>>  drivers/spi/mxc_spi.c |   96 +++++++++++++++++++++++++++++++++++++------------
>>  1 files changed, 73 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
>> index d558137..b353c83 100644
>> --- a/drivers/spi/mxc_spi.c
>> +++ b/drivers/spi/mxc_spi.c
>> @@ -70,6 +70,8 @@ static unsigned long spi_bases[] = {
>>  	0x53f84000,
>>  };
>>  
>> +#define spi_cfg	spi_cfg_mx3
> ...
>> +#define spi_cfg	spi_cfg_mx51
> 
> Hm... this repeats below, but in the end both spi_cfg_mx3() and
> spi_cfg_mx51() are just static functions within the same source file,
> with #ifdef's around them so only one can ever be enabled at a time.

You are right, there is already an #ifdef. I think I had in mind to
remove the #ifdef surrounding the functions, but I give up because I
have added unneeded code (mx51 code for mx3 and viceversa). I will fix it.

> 
> I suggest you omit all these "#define spi_cfg" lines and rename both
> versions of these functions into spi_cfg_mx().
> 
>> +#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
> 
> As mentioned before: please use a C struct.

This is another issue. I agree that is ugly code, but it comes from the
originally written driver for the i.MX31. This issue should be fixed,
but in a separate patch, and for all supported processors
(MX.31/MX.25/MX.51/MX.35).

There are at the moment two other patches by Anatolji regarding this
driver. I have already rebased one of them and post to the ML, but I
admit that, as they are not in the same patchset, it is quite difficult
to have an overview of the changes. My proposal is that I will add these
other two patches to my patchset to simplify review.

Best regards,.
Stefano Babic
Wolfgang Denk - Jan. 19, 2011, 11:40 a.m.
Dear Stefano Babic,

In message <4D36B845.1000908@denx.de> you wrote:
>
...
> There are at the moment two other patches by Anatolji regarding this
> driver. I have already rebased one of them and post to the ML, but I
> admit that, as they are not in the same patchset, it is quite difficult
> to have an overview of the changes. My proposal is that I will add these
> other two patches to my patchset to simplify review.

That's fine with me, then.

Best regards,

Wolfgang Denk

Patch

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index d558137..b353c83 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -70,6 +70,8 @@  static unsigned long spi_bases[] = {
 	0x53f84000,
 };
 
+#define spi_cfg	spi_cfg_mx3
+
 #elif defined(CONFIG_MX51)
 #include <asm/arch/imx-regs.h>
 #include <asm/arch/clock.h>
@@ -111,6 +113,47 @@  static unsigned long spi_bases[] = {
 	CSPI2_BASE_ADDR,
 	CSPI3_BASE_ADDR,
 };
+#define spi_cfg	spi_cfg_mx51
+
+#elif defined(CONFIG_MX35)
+
+#include <asm/arch/imx-regs.h>
+#include <asm/arch/clock.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
+#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) << 12)
+#define MXC_CSPICTRL_BITCOUNT(x)	(((x) & 0xfff) << 20)
+#define MXC_CSPICTRL_DATARATE(x)	(((x) & 0x7) << 16)
+#define MXC_CSPICTRL_TC		(1 << 7)
+#define MXC_CSPICTRL_RXOVF	(1 << 6)
+#define MXC_CSPICTRL_MAXBITS	0xfff
+
+#define MXC_CSPIPERIOD_32KHZ	(1 << 15)
+#define MAX_SPI_BYTES	4
+
+static unsigned long spi_bases[] = {
+	0x43fa4000,
+	0x50010000,
+};
+#define spi_cfg	spi_cfg_mx3
+
 #else
 #error "Unsupported architecture"
 #endif
@@ -158,8 +201,35 @@  void spi_cs_deactivate(struct spi_slave *slave)
 			      !(mxcs->ss_pol));
 }
 
-#ifdef CONFIG_MX51
-static s32 spi_cfg(struct mxc_spi_slave *mxcs, unsigned int cs,
+#if defined(CONFIG_MX31) || defined(CONFIG_MX35)
+static s32 spi_cfg_mx3(struct mxc_spi_slave *mxcs, unsigned int cs,
+		unsigned int max_hz, unsigned int mode)
+{
+	unsigned int ctrl_reg;
+
+	ctrl_reg = MXC_CSPICTRL_CHIPSELECT(cs) |
+		MXC_CSPICTRL_BITCOUNT(MXC_CSPICTRL_MAXBITS) |
+		MXC_CSPICTRL_DATARATE(7) | /* FIXME: calculate data rate */
+		MXC_CSPICTRL_EN |
+#ifdef CONFIG_MX35
+		MXC_CSPICTRL_SSCTL |
+#endif
+		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;
+	mxcs->ctrl_reg = ctrl_reg;
+
+	return 0;
+}
+#endif
+
+#if defined(CONFIG_MX51)
+static s32 spi_cfg_mx51(struct mxc_spi_slave *mxcs, unsigned int cs,
 		unsigned int max_hz, unsigned int mode)
 {
 	u32 clk_src = mxc_get_clock(MXC_CSPI_CLK);
@@ -227,7 +297,7 @@  static s32 spi_cfg(struct mxc_spi_slave *mxcs, unsigned int cs,
 
 	/*
 	 * Configuration register setup
-	 * The MX51 has support different setup for each SS
+	 * The MX51 supports different setup for each SS
 	 */
 	reg_config = (reg_config & ~(1 << (cs + MXC_CSPICON_SSPOL))) |
 		(ss_pol << (cs + MXC_CSPICON_SSPOL));
@@ -363,7 +433,6 @@  int spi_xchg_single(struct spi_slave *slave, unsigned int bitlen,
 
 }
 
-
 int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 		void *din, unsigned long flags)
 {
@@ -441,7 +510,6 @@  static int decode_cs(struct mxc_spi_slave *mxcs, unsigned int cs)
 struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 			unsigned int max_hz, unsigned int mode)
 {
-	unsigned int ctrl_reg;
 	struct mxc_spi_slave *mxcs;
 	int ret;
 
@@ -467,30 +535,12 @@  struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	mxcs->base = spi_bases[bus];
 	mxcs->ss_pol = (mode & SPI_CS_HIGH) ? 1 : 0;
 
-#ifdef CONFIG_MX51
-	/* Can be used for i.MX31 too ? */
-	ctrl_reg = 0;
 	ret = spi_cfg(mxcs, cs, max_hz, mode);
 	if (ret) {
 		printf("mxc_spi: cannot setup SPI controller\n");
 		free(mxcs);
 		return NULL;
 	}
-#else
-	ctrl_reg = MXC_CSPICTRL_CHIPSELECT(cs) |
-		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;
-	mxcs->ctrl_reg = ctrl_reg;
-#endif
 	return &mxcs->slave;
 }