mxc_spi refactoring (for mx6q)

Signed-off-by Eric Nelson <[email protected]>

---
 arch/arm/include/asm/arch-mx6/imx-regs.h |   44

++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644

arch/arm/include/asm/arch-mx6/imx-regs.h +1 -0

@@ -190,6 +190,50 @@ struct src {
 	u32     gpr10;
 };
 
+/* ECSPI registers */

arch/arm/include/asm/arch-mx6/imx-regs.h +33 -0

+
+/*
+ * CSPI register definitions
+ */
+#define MXC_ECSPI
+#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)
+#define MXC_CSPIPERIOD_32KHZ	(1 << 15)
+#define MAX_SPI_BYTES	32
+
+/* 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_POL		4
+#define MXC_CSPICON_PHA		0
+#define MXC_CSPICON_SSPOL	12
+#define MXC_SPI_BASE_ADDRESSES \
+	ECSPI1_BASE_ADDR, \
+	ECSPI2_BASE_ADDR, \
+	ECSPI3_BASE_ADDR, \
+	ECSPI4_BASE_ADDR, \
+	ECSPI5_BASE_ADDR
+
 struct iim_regs {
 	u32	ctrl;
 	u32	ctrl_set;

Quoted 15 lines

Signed-off-by Eric Nelson<[email protected]>

---
  arch/arm/include/asm/arch-mx6/imx-regs.h |   44

++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644

arch/arm/include/asm/arch-mx6/imx-regs.h +1 -0

@@ -190,6 +190,50 @@ struct src {
  	u32     gpr10;
  };
 
+/* ECSPI registers */

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 203

Quoted 10 lines

arch/arm/include/asm/arch-mx6/imx-regs.h +10 -0

+struct cspi_regs {
+	u32 rxdata;
+	u32 txdata;
+	u32 ctrl;
+	u32 cfg;
+	u32 intr;
+	u32 dma;
+	u32 stat;
+	u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Is this part common for all imx-es ?

Quoted 32 lines

Quoted 19 lines

Signed-off-by Eric Nelson<[email protected]>

---

  arch/arm/include/asm/arch-mx6/imx-regs.h |   44

++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644

arch/arm/include/asm/arch-mx6/imx-regs.h +1 -0

@@ -190,6 +190,50 @@ struct src {
 
  	u32     gpr10;
 
  };
 
+/* ECSPI registers */

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 205

Quoted 10 lines

arch/arm/include/asm/arch-mx6/imx-regs.h +10 -0

+struct cspi_regs {
+	u32 rxdata;
+	u32 txdata;
+	u32 ctrl;
+	u32 cfg;
+	u32 intr;
+	u32 dma;
+	u32 stat;
+	u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Quoted 1 line

Is this part common for all imx-es ?

All i.MX6's

This is a cut & paste from MX51.

I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.

Quoted 22 lines · Marek Vasut

Quoted 15 lines

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 203

Quoted 12 lines

Signed-off-by Eric Nelson<[email protected]>

arch/arm/include/asm/arch-mx6/imx-regs.h +11 -0

+/* ECSPI registers */
+struct cspi_regs {
+	u32 rxdata;
+	u32 txdata;
+	u32 ctrl;
+	u32 cfg;
+	u32 intr;
+	u32 dma;
+	u32 stat;
+	u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Quoted 1 line

Is this part common for all imx-es ?

All i.MX6's

This is a cut& paste from MX51.

Quoted 3 lines

I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.

But you don't even need this outside of the spi driver so just put it into the spi driver and be done with it. That'll solve your duplication issue.

M

Right - and we already discussed in the past how to avoid to put specific SOC code inside the driver. In fact, the cspi_regs structure was already moved into the specific SOC header (imx-regs.h) - but the definitions of the single bits of the registers are still inside the driver, as well as the base address of the (e)cspi controllers.

They should also be moved - take into acoount by implementing your changes for i.mx6

Quoted 26 lines

Quoted 18 lines · Marek Vasut

Quoted 15 lines

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 203

Quoted 12 lines

Signed-off-by Eric Nelson<[email protected]>

arch/arm/include/asm/arch-mx6/imx-regs.h +11 -0

+/* ECSPI registers */
+struct cspi_regs {
+	u32 rxdata;
+	u32 txdata;
+	u32 ctrl;
+	u32 cfg;
+	u32 intr;
+	u32 dma;
+	u32 stat;
+	u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Quoted 1 line

Is this part common for all imx-es ?

Quoted 7 lines

All i.MX6's

This is a cut& paste from MX51.

I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.

Quoted 5 lines

But you don't even need this outside of the spi driver so just put it into the spi driver and be done with it. That'll solve your duplication issue.

M

I'll defer to Stefano on this one, since I did this in response

to his request:

Quoted 6 lines

Right - and we already discussed in the past how to avoid to put specific SOC code inside the driver. In fact, the cspi_regs structure was already moved into the specific SOC header (imx-regs.h) - but the definitions of the single bits of the registers are still inside the driver, as well as the base address of the (e)cspi controllers.

Quoted 2 lines

They should also be moved - take into acoount by implementing your changes for i.mx6

The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations.

	http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-

mx-regs.h;h=6a517ddd931ca0d1e598bd7456c4c611741602eb;hb=HEAD#l60
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx35/i
mx-regs.h;h=df74508a93ee87ae986f7c2f48f6c5fb36626070;hb=HEAD#l279
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx5/im
x-regs.h;h=0ee88d25b7800ae9e6aed809d02dd19d9cac9c82;hb=HEAD#l423

My interpretation of Stefano's intent is to clean up the driver at the expense of extra defines in the arch-specific headers.

Regards,

Eric

Quoted 18 lines · Marek Vasut

I'll defer to Stefano on this one, since I did this in response to his request:

Quoted 8 lines

Right - and we already discussed in the past how to avoid to put specific SOC code inside the driver. In fact, the cspi_regs structure was already moved into the specific SOC header (imx-regs.h) - but the definitions of the single bits of the registers are still inside the driver, as well as the base address of the (e)cspi controllers.

They should also be moved - take into acoount by implementing your changes for i.mx6

The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations. <snip>

Quoted 2 lines

My interpretation of Stefano's intent is to clean up the driver at the expense of extra defines in the arch-specific headers.

But they're all the same, right? So we have now the same structure defined thrice?

Quoted 30 lines · Marek Vasut

Quoted 18 lines · Marek Vasut

Quoted 15 lines

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 203

Quoted 12 lines

Signed-off-by Eric Nelson<[email protected]>

arch/arm/include/asm/arch-mx6/imx-regs.h +11 -0

+/* ECSPI registers */
+struct cspi_regs {
+    u32 rxdata;
+    u32 txdata;
+    u32 ctrl;
+    u32 cfg;
+    u32 intr;
+    u32 dma;
+    u32 stat;
+    u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Quoted 1 line

Is this part common for all imx-es ?

Quoted 7 lines

All i.MX6's

This is a cut& paste from MX51.

I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.

But you don't even need this outside of the spi driver so just put it into the
spi driver and be done with it. That'll solve your duplication issue.

Quoted 2 lines

M

I'll defer to Stefano on this one, since I did this in response to his request:

The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations.

My interpretation of Stefano's intent is to clean up the driver at the expense
of extra defines in the arch-specific headers.

Quoted 31 lines · Marek Vasut

Quoted 30 lines

Quoted 18 lines · Marek Vasut

Quoted 15 lines

arch/arm/include/asm/arch-mx6/imx-regs.h struct src { · line 203

Quoted 12 lines

Signed-off-by Eric Nelson<[email protected]>

arch/arm/include/asm/arch-mx6/imx-regs.h +11 -0

+/* ECSPI registers */
+struct cspi_regs {
+    u32 rxdata;
+    u32 txdata;
+    u32 ctrl;
+    u32 cfg;
+    u32 intr;
+    u32 dma;
+    u32 stat;
+    u32 period;
+};

Sigh ... it's no fun I can have only one remark :-)

Quoted 1 line

Is this part common for all imx-es ?

Quoted 7 lines

All i.MX6's

This is a cut& paste from MX51.

I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.

But you don't even need this outside of the spi driver so just put it into the
spi driver and be done with it. That'll solve your duplication issue.

Quoted 5 lines

Quoted 1 line

M

I'll defer to Stefano on this one, since I did this in response

to his request:

Yes, I admit I am guilty about this !

The layout of the CSPI registers is not exactly the same for all SOCs. For example, the MXC_CSPICTRL_TC has a different position, as well as the BITCOUNT (because the MX31 can send less bits in one shot) and MXC_CSPICTRL_CHIPSELECT.

So they are similar, but not exactly the same.

Then we have the MX5 registers. Even if we check the CSPI (not eCSPI) controller, the layout of the registers is different compared to the MX3 SOCs.

Quoted 2 lines

The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations.

Right, I see the same. The cspi_regs structure is already defined into the imx_regs.h, only the bit layout was moved. And as the bit layout is SOC dependent, I think the right place for it is inside the imx-regs.h registers.

Quoted 3 lines

My interpretation of Stefano's intent is to clean up the driver at the expense
of extra defines in the arch-specific headers.

Yes, you're right - of course, I am open also to other solutions if they are proofed to be better ;-).

Best regards,
Stefano

Ok guys, I see ... Stefano, you're ok with putting the reg structures into these header files?

Quoted 2 lines · Eric Nelson

I'll defer to Stefano on this one, since I did this in response to his request:

Yes, I admit I am guilty about this !

The layout of the CSPI registers is not exactly the same for all SOCs. For example, the MXC_CSPICTRL_TC has a different position, as well as the BITCOUNT (because the MX31 can send less bits in one shot) and MXC_CSPICTRL_CHIPSELECT.

So they are similar, but not exactly the same.

Then we have the MX5 registers. Even if we check the CSPI (not eCSPI) controller, the layout of the registers is different compared to the MX3 SOCs.

Quoted 2 lines

The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations.

Right, I see the same. The cspi_regs structure is already defined into the imx_regs.h, only the bit layout was moved. And as the bit layout is SOC dependent, I think the right place for it is inside the imx-regs.h registers.

Quoted 2 lines

My interpretation of Stefano's intent is to clean up the driver at the expense of extra defines in the arch-specific headers.

Yes, you're right - of course, I am open also to other solutions if they are proofed to be better ;-).

Quoted 4 lines

Yes, you're right - of course, I am open also to other solutions if they are proofed to be better ;-).

I think this is about as good as things get with the current code base. I would argue that the driver would be better if it explicitly supported ECSPI and CSPI at the same time since the mx5x CPUs support it.

Implememting that would likely require a de-structuring (removing the use of structs to represent the register set). IOW, a re-write.

That's probably not worth the effort unless someone's built hardware that needs it (I'm not aware of any).

On our boards that use more than one channel of SPI (for PMIC and SF), we're using ECSPI on both. I think the same was true on the MX51 EVK.

Signed-off-by Eric Nelson <[email protected]>

---
?include/configs/mx6qsabrelite.h | ? 12 +++++++++++- ?1 files changed, 11 insertions(+), 1 deletions(-)

include/configs/mx6qsabrelite.h +0 -0

index 44b028a..160894c 100644
@@ -174,10 +174,20 @@

?/* FLASH and environment organization */
?#define CONFIG_SYS_NO_FLASH

-#define CONFIG_ENV_OFFSET ? ? ? ? ? ? ?(6 * 64 * 1024) ?#define CONFIG_ENV_SIZE ? ? ? ? ? ? ? ? ? ? ? ?(8 * 1024) +
?#define CONFIG_ENV_IS_IN_MMC
+/* #define CONFIG_ENV_IS_IN_SPI_FLASH */

Quoted 13 lines · Eric Nelson

Signed-off-by Eric Nelson <[email protected]>

---
 include/configs/mx6qsabrelite.h |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

include/configs/mx6qsabrelite.h +0 -0

index 44b028a..160894c 100644
@@ -174,10 +174,20 @@
 /* FLASH and environment organization */
 #define CONFIG_SYS_NO_FLASH
 

include/configs/mx6qsabrelite.h

Quoted 7 lines

include/configs/mx6qsabrelite.h +2 -1

-#define CONFIG_ENV_OFFSET              (6 * 64 * 1024)
 #define CONFIG_ENV_SIZE                        (8 * 1024)
+
 #define CONFIG_ENV_IS_IN_MMC
+/* #define CONFIG_ENV_IS_IN_SPI_FLASH */
 
 From the commit log, it says the default is in serial flash,

but
apparently in the code
the env is default to MMC, which mismatch. please fix it.

include/configs/mx6qsabrelite.h line 188

Quoted 8 lines

include/configs/mx6qsabrelite.h +7 -0

+
+#if defined(CONFIG_ENV_IS_IN_MMC)
+#define CONFIG_ENV_OFFSET              (6 * 64 * 1024)
 #define CONFIG_SYS_MMC_ENV_DEV         0
+#elif defined(CONFIG_ENV_IS_IN_SPI_FLASH)
+#define CONFIG_ENV_OFFSET              (768 * 1024)
+#define CONFIG_ENV_SECT_SIZE           (8 * 1024)
+#define CONFIG_ENV_SPI_CS              0x5300

I'm wondering how the CONFIG_ENV_SPI_CS could be 0x5300? Vague?

Quoted 8 lines

include/configs/mx6qsabrelite.h +2 -0

+#define CONFIG_ENV_SPI_MODE            SPI_MODE_0
+#endif
 

#define CONFIG_OF_LIBFDT

--
1.7.1

Quoted 18 lines · Jason Hui

Quoted 15 lines · Eric Nelson

Signed-off-by Eric Nelson <[email protected]>

---
?include/configs/mx6qsabrelite.h | ? 12 +++++++++++- ?1 files changed, 11 insertions(+), 1 deletions(-)

include/configs/mx6qsabrelite.h +0 -0

index 44b028a..160894c 100644
@@ -174,10 +174,20 @@

?/* FLASH and environment organization */
?#define CONFIG_SYS_NO_FLASH

Quoted 8 lines

Quoted 8 lines

-#define CONFIG_ENV_OFFSET ? ? ? ? ? ? ?(6 * 64 * 1024) ?#define CONFIG_ENV_SIZE ? ? ? ? ? ? ? ? ? ? ? ?(8 * 1024) +
?#define CONFIG_ENV_IS_IN_MMC
+/* #define CONFIG_ENV_IS_IN_SPI_FLASH */

From the commit log, it says the default is in serial flash,

No, the commit log doesn't say this. It doesn't say 'it is'. It says it 'provides the defaults'. But it doesn't say that it actually uses these defaults.

Quoted 3 lines

but
apparently in the code
the env is default to MMC, which mismatch. please fix it.

As mentioned above, I understand this differently. While I reviewed it some days ago, I found the description and the doing here quite fine.

It enables the MMC env and additionally _provides_ everything needed to easily switch to SPI env by uncommenting one switch. This is fine and quite helpful, see e.g. [1].

I like this, please keep it as is.

Quoted 4 lines

Quoted 1 line

+#define CONFIG_ENV_SPI_CS ? ? ? ? ? ? ?0x5300

I'm wondering how the CONFIG_ENV_SPI_CS ?could be 0x5300? Vague?

Quoted 2 lines

Quoted 2 lines

I'm wondering how the CONFIG_ENV_SPI_CS could be 0x5300? Vague?

Then the left open question is only above one.

Quoted 4 lines · Jason Hui

Quoted 2 lines

Quoted 2 lines

I'm wondering how the CONFIG_ENV_SPI_CS could be 0x5300? Vague?

Then the left open question is only above one.

The SPI driver can take as chip select the controller's chip selects as well as an external GPIO. The LSB byte has the value of the internal chip select, the highest (thought as 16-bit value) contains the GPIO number. Reading this configuration, the GPIO used on this board should be the number 83 (0x53).

Stefano

Quoted 13 lines · Eric Nelson

Signed-off-by Eric Nelson<[email protected]>

---
  include/configs/mx6qsabrelite.h |   12 +++++++++++-
  1 files changed, 11 insertions(+), 1 deletions(-)

include/configs/mx6qsabrelite.h +0 -0

index 44b028a..160894c 100644
@@ -174,10 +174,20 @@
  /* FLASH and environment organization */
  #define CONFIG_SYS_NO_FLASH
 

include/configs/mx6qsabrelite.h line 180

Quoted 5 lines

include/configs/mx6qsabrelite.h +2 -1

-#define CONFIG_ENV_OFFSET              (6 * 64 * 1024)
  #define CONFIG_ENV_SIZE                        (8 * 1024)
+
  #define CONFIG_ENV_IS_IN_MMC
+/* #define CONFIG_ENV_IS_IN_SPI_FLASH */

From the commit log, it says the default is in serial flash, but apparently in the code the env is default to MMC, which mismatch. please fix it.

include/configs/mx6qsabrelite.h line 188

Quoted 8 lines

include/configs/mx6qsabrelite.h +7 -0

+
+#if defined(CONFIG_ENV_IS_IN_MMC)
+#define CONFIG_ENV_OFFSET              (6 * 64 * 1024)
  #define CONFIG_SYS_MMC_ENV_DEV         0
+#elif defined(CONFIG_ENV_IS_IN_SPI_FLASH)
+#define CONFIG_ENV_OFFSET              (768 * 1024)
+#define CONFIG_ENV_SECT_SIZE           (8 * 1024)
+#define CONFIG_ENV_SPI_CS              0x5300

I'm wondering how the CONFIG_ENV_SPI_CS could be 0x5300? Vague?

This patch set refactors mxc_spi as described in

    http://lists.denx.de/pipermail/u-boot/2010-March/068791.html
and requested in
    http://lists.denx.de/pipermail/u-boot/2012-January/116023.html

in order to add support for the MX6Q in general and the mx6qsabrelite specifically.

Patch 1 simply moves the conditional parts of mxc_spi.c into the respective CPU-specific imx-regs.h files.

Patch 2 adds general support for SPI to the i.MX6.

Patch 3 adds support to the mx6qsabrelite board

Patch 4 modifies the 'sf' command to allow a default chip-select to be specified by board headers as is done on efika et al. This allows a bare 'sf' probe command:
U-Boot> sf probe
instead of the more cumbersome usage when a GPIO is tacked onto the chip-select. Otherwise, this command-line would be needed to specify GP3:19 on SabreLite:

     U-Boot> sf probe 0x5300

Patch 5 adds default chip-select values for mx6qsabrelite platform.

Patch 6 adds reference macros for use in storing the environment in serial flash to match the use on Freescale's U-Boot release

This patch set has been compiled against the following configurations, but only tested on mx6qsabrelite:

	mx6qsabrelite
	mx51evk
	mx31pdk
	mx35pdk

This patch set refactors mxc_spi as described in

    http://lists.denx.de/pipermail/u-boot/2010-March/068791.html
and requested in 
    http://lists.denx.de/pipermail/u-boot/2012-January/116023.html

in order to add support for the MX6Q in general and the mx6qsabrelite specifically.