Skip to contentSkip to conversation

Adds driver for Xilinx' xps_spi SPI controller.

PATCH New Graeme Smecher 6 participants 12 messages Aug 3, 2010 – Apr 2, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Adds driver for Xilinx' xps_spi SPI controller.

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Documentation for the SPI core is available here:

http://www.xilinx.com/support/documentation/ip_documentation/xps_spi.pdf

---
 drivers/spi/Makefile     |    1 +
 drivers/spi/xilinx_spi.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/xilinx_spi.c
drivers/spi/Makefile +1 -0
index dfcbb8b..34e0f31 100644
@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
 LIB	:= $(obj)libspi.a
 
 COBJS-$(CONFIG_ALTERA_SPI) += altera_spi.o
+COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 COBJS-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o
 COBJS-$(CONFIG_ATMEL_SPI) += atmel_spi.o
 COBJS-$(CONFIG_BFIN_SPI) += bfin_spi.o
drivers/spi/xilinx_spi.c new file +173 -0

173 lines not on this page · raw message

Signature · 1 line
1.6.3.3
#1 lore raw

PATCH 1/1 Adds driver for Xilinx' xps_spi SPI controller.

drivers/spi/Makefile include $(TOPDIR)/config.mk · line 30

Quoted 6 lines · On Tuesday, August 03, 2010 11:47:42 Graeme Smecher wrote:
drivers/spi/Makefile +1 -0
 
 COBJS-$(CONFIG_ALTERA_SPI) += altera_spi.o
+COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 COBJS-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o

this is a sorted list. please keep it that way

Quoted 7 lines
drivers/spi/xilinx_spi.c new file +4 -0
 
+static ulong xilinx_spi_base_list[] = {
+#ifdef XPAR_SPI_0_BASEADDR
+	XPAR_SPI_0_BASEADDR,
+#endif

drivers/spi/xilinx_spi.c line 44

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+#ifdef XPAR_SPI_1_BASEADDR
+	XPAR_SPI_1_BASEADDR,
+#endif
+#ifdef XPAR_SPI_2_BASEADDR
+	XPAR_SPI_2_BASEADDR,
+#endif
+#ifdef XPAR_SPI_3_BASEADDR
+	XPAR_SPI_3_BASEADDR,
+#endif
+};

if this is only ever read, you might want to declare it const so it ends up in the rodata section.

what if someone defines just XPAR_SPI_3_BASEADDR ? do you want to be consistent in bus id 3 always corresponds to XPAR_SPI_3_BASEADDR ? or are you OK with the numbers always being relative ? doesnt matter to me, just highlighting some things that might have been missed.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100803/1987da11/attachment.pgp

#2 lore raw

PATCH 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi Mike,

Thanks again for reviewing! Comments in-line; an updated patch will follow.

Quoted 11 lines · Mike Frysinger

drivers/spi/Makefile include $(TOPDIR)/config.mk · line 30

Quoted 7 lines · On Tuesday, August 03, 2010 11:47:42 Graeme Smecher wrote:
drivers/spi/Makefile +1 -0
 
  COBJS-$(CONFIG_ALTERA_SPI) += altera_spi.o
+COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
  COBJS-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o

this is a sorted list. please keep it that way

Whoops -- OK.

Quoted 12 lines
Quoted 12 lines
drivers/spi/xilinx_spi.c new file +9 -0
 
+static ulong xilinx_spi_base_list[] = {
+#ifdef XPAR_SPI_0_BASEADDR
+	XPAR_SPI_0_BASEADDR,
+#endif
+#ifdef XPAR_SPI_1_BASEADDR
+	XPAR_SPI_1_BASEADDR,
+#endif
+#ifdef XPAR_SPI_2_BASEADDR
+	XPAR_SPI_2_BASEADDR,
Quoted 9 lines

drivers/spi/xilinx_spi.c line 44

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +5 -0
+#endif
+#ifdef XPAR_SPI_3_BASEADDR
+	XPAR_SPI_3_BASEADDR,
+#endif
+};

if this is only ever read, you might want to declare it const so it ends up in the rodata section.

Done.

Quoted 5 lines

what if someone defines just XPAR_SPI_3_BASEADDR ? do you want to be consistent in bus id 3 always corresponds to XPAR_SPI_3_BASEADDR ? or are you OK with the numbers always being relative ? doesnt matter to me, just highlighting some things that might have been missed.

These numbers come from autogenerated headers and are (as far as I know) always numbered from 0. Offhand, the above code seems like the least confusing approach. I don't have a particularly strong opinion either.

cheers,
Graeme

#3 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

In reply to Mike Frysinger

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Documentation for the SPI core is available here:

http://www.xilinx.com/support/documentation/ip_documentation/xps_spi.pdf

---
 drivers/spi/Makefile     |    1 +
 drivers/spi/xilinx_spi.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/xilinx_spi.c
drivers/spi/Makefile +1 -0
index dfcbb8b..eeba5ef 100644
@@ -36,6 +36,7 @@ COBJS-$(CONFIG_MPC52XX_SPI) += mpc52xx_spi.o
 COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
 COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
 COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
+COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
drivers/spi/xilinx_spi.c new file +173 -0

173 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#4 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi all,

Quoted 5 lines · Graeme Smecher

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Documentation for the SPI core is available here:

Quoted 9 lines

http://www.xilinx.com/support/documentation/ip_documentation/xps_spi.pdf

---
  drivers/spi/Makefile     |    1 +
  drivers/spi/xilinx_spi.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 174 insertions(+), 0 deletions(-)

create mode 100644 drivers/spi/xilinx_spi.c

Just a friendly reminder -- I'd be really happy to see this, and my other patch (July 29, "Add support for Winbond W25Q64 SPI flash"), integrated into the tree. Please let me know if there's anything I can do to grease the skids.

cheers,
Graeme

#5 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi,

Quoted 16 lines · On Tue, 17 Aug 2010 09:27:10 -0700 Graeme Smecher…
Quoted 5 lines · Graeme Smecher

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Documentation for the SPI core is available here:

Quoted 9 lines

http://www.xilinx.com/support/documentation/ip_documentation/xps_spi.pdf

---
  drivers/spi/Makefile     |    1 +
  drivers/spi/xilinx_spi.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 174 insertions(+), 0 deletions(-)

create mode 100644 drivers/spi/xilinx_spi.c

Quoted 4 lines

Just a friendly reminder -- I'd be really happy to see this, and my other patch (July 29, "Add support for Winbond W25Q64 SPI flash"), integrated into the tree. Please let me know if there's anything I can do to grease the skids.

Thanks! But both, this and your other patch have been submitted when the merge window for next v2010.09 release was closed. So the patches probably won't be integrated before the next merge window opens, since these are not bug fixes. Normally you do not have to resubmit the patches when the next merge window opens again, maybe send a reminder again. Some info about release cycle can be found here [1].

Best regards,
Anatolij

[1] http://www.denx.de/wiki/U-Boot/ReleaseCycle

#6 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi Anatolij,

Quoted 35 lines · Anatolij Gustschin

Hi,

Quoted 17 lines · On Tue, 17 Aug 2010 09:27:10 -0700 Graeme…
Quoted 5 lines · Graeme Smecher

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Documentation for the SPI core is available here:

Quoted 10 lines

http://www.xilinx.com/support/documentation/ip_documentation/xps_spi.pdf

---
   drivers/spi/Makefile     |    1 +
   drivers/spi/xilinx_spi.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
   2 files changed, 174 insertions(+), 0 deletions(-)

create mode 100644 drivers/spi/xilinx_spi.c

Quoted 5 lines

Just a friendly reminder -- I'd be really happy to see this, and my other patch (July 29, "Add support for Winbond W25Q64 SPI flash"), integrated into the tree. Please let me know if there's anything I can do to grease the skids.

Thanks! But both, this and your other patch have been submitted when the merge window for next v2010.09 release was closed. So the patches probably won't be integrated before the next merge window opens, since these are not bug fixes. Normally you do not have to resubmit the patches when the next merge window opens again, maybe send a reminder again. Some info about release cycle can be found here [1].

Quoted 5 lines

Best regards,
Anatolij

[1] http://www.denx.de/wiki/U-Boot/ReleaseCycle

Ah, of course -- I should have guessed. I'll send along another prod next month if it's necessary.

thanks,
Graeme

#7 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

In reply to Graeme Smecher

Dear Graeme Smecher,

Quoted 2 lines · In message <[email protected]>…

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Hm... if the core really differs in only trivial ways from the altera_spi driver, then why do we need a duplication of that code?

Can we plase have a single driver source that supports both instead?

Best regards,

Wolfgang Denk

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Harrisberger's Fourth Law of the Lab:
	Experience is directly proportional to the
	amount of equipment ruined.
#8 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi Wolfgang,

Quoted 4 lines · Wolfgang Denk

Dear Graeme Smecher,

In message<[email protected]> you wrote:

Quoted 8 lines
Quoted 3 lines

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Hm... if the core really differs in only trivial ways from the altera_spi driver, then why do we need a duplication of that code?

Can we plase have a single driver source that supports both instead?

Hm... It's possible to combine xilinx_spi.c and altera_spi.c. However, I suspect joining them will make maintenance more complicated rather than simpler. I can't, for example, test a combined driver on Altera hardware (and the Altera maintainer will likely have the same problem with Xilinx hardware.)

My guess is that most SPI interfaces are nearly identical at a register level, especially for drivers that don't support interrupts and other complications. (See mxc_spi.c for another example.) Xilinx and Altera's SPI interfaces are just two examples that happen to both be FPGA-based -- I could probably have adapted any of the other SPI drivers instead.

Are you sure combining drivers is the most logical approach? Let me know, and I'll have a crack at it.

thanks,
Graeme

#9 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Dear Graeme Smecher,

Quoted 6 lines

Hi Wolfgang,

Quoted 3 lines · Wolfgang Denk

Dear Graeme Smecher,

In message<[email protected]>

Quoted 21 lines · you
Quoted 8 lines
Quoted 3 lines

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Hm... if the core really differs in only trivial ways from the altera_spi driver, then why do we need a duplication of that code?

Can we plase have a single driver source that supports both instead?

Hm... It's possible to combine xilinx_spi.c and altera_spi.c. However, I suspect joining them will make maintenance more complicated rather than simpler. I can't, for example, test a combined driver on Altera hardware (and the Altera maintainer will likely have the same problem with Xilinx hardware.)

My guess is that most SPI interfaces are nearly identical at a register level, especially for drivers that don't support interrupts and other complications. (See mxc_spi.c for another example.) Xilinx and Altera's SPI interfaces are just two examples that happen to both be FPGA-based -- I could probably have adapted any of the other SPI drivers instead.

Quoted 5 lines

Are you sure combining drivers is the most logical approach? Let me know, and I'll have a crack at it.

thanks,
Graeme

What was the conclusion here? Shall I drop the patch or will you submit a rebased version?

Best regards,
Marek Vasut

#10 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Hi Marek,

Quoted 28 lines · Marek Vasut

Dear Graeme Smecher,

Quoted 6 lines

Hi Wolfgang,

Quoted 3 lines · Wolfgang Denk

Dear Graeme Smecher,

In message<[email protected]>

Quoted 19 lines · you
Quoted 7 lines
Quoted 3 lines

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Hm... if the core really differs in only trivial ways from the altera_spi driver, then why do we need a duplication of that code?

Can we plase have a single driver source that supports both instead?

Hm... It's possible to combine xilinx_spi.c and altera_spi.c. However, I suspect joining them will make maintenance more complicated rather than simpler. I can't, for example, test a combined driver on Altera hardware (and the Altera maintainer will likely have the same problem with Xilinx hardware.)

My guess is that most SPI interfaces are nearly identical at a register level, especially for drivers that don't support interrupts and other complications. (See mxc_spi.c for another example.) Xilinx and Altera's SPI interfaces are just two examples that happen to both be FPGA-based -- I could probably have adapted any of the other SPI drivers instead.

Quoted 7 lines
Quoted 5 lines

Are you sure combining drivers is the most logical approach? Let me know, and I'll have a crack at it.

thanks,
Graeme

What was the conclusion here? Shall I drop the patch or will you submit a rebased version?

Please drop the patch for now, since I'm not in a position to maintain it. The trail of bread-crumbs on the mailing list is a good compromise until someone (me, later on?) steps up.

thanks,
Graeme

#11 lore raw

PATCH v2 1/1 Adds driver for Xilinx' xps_spi SPI controller.

Dear Graeme Smecher,

Quoted 42 lines

Hi Marek,

Quoted 39 lines · Marek Vasut

Dear Graeme Smecher,

Quoted 8 lines

Hi Wolfgang,

Quoted 5 lines · Wolfgang Denk

Dear Graeme Smecher,

In message<[email protected]

Quoted 1 line
Quoted 26 lines · you
Quoted 8 lines
Quoted 3 lines

This code differs in only trivial ways from the altera_spi driver. It plays nice with Thomas Chou's mmc_spi driver, as well as with SPI flash.

Hm... if the core really differs in only trivial ways from the altera_spi driver, then why do we need a duplication of that code?

Can we plase have a single driver source that supports both instead?

Hm... It's possible to combine xilinx_spi.c and altera_spi.c. However, I suspect joining them will make maintenance more complicated rather than simpler. I can't, for example, test a combined driver on Altera hardware (and the Altera maintainer will likely have the same problem with Xilinx hardware.)

My guess is that most SPI interfaces are nearly identical at a register level, especially for drivers that don't support interrupts and other complications. (See mxc_spi.c for another example.) Xilinx and Altera's SPI interfaces are just two examples that happen to both be FPGA-based -- I could probably have adapted any of the other SPI drivers instead.

Are you sure combining drivers is the most logical approach? Let me know, and I'll have a crack at it.

thanks,
Graeme

Quoted 6 lines
Quoted 2 lines

What was the conclusion here? Shall I drop the patch or will you submit a rebased version?

Please drop the patch for now, since I'm not in a position to maintain it. The trail of bread-crumbs on the mailing list is a good compromise until someone (me, later on?) steps up.

Sorry to hear that, though thanks for updating me on the situation :)

Quoted 2 lines

thanks,
Graeme

Best regards,
Marek Vasut