spi: add support for SPI over SuperH SCI pins

spi: add support for SPI over SuperH SCI pins

This patch adds support for SPI over SCI pins. SCI is a very simple serial controller block that can be found on older SuperH processors. In theory it is possible to use the SCI hardware block in syncronous mode, but this version of the driver simply hooks up the bit banging code on the SCI pins.

Signed-off-by Magnus Damm <[email protected]>

---

 drivers/spi/Kconfig      |    7 +
 drivers/spi/Makefile     |    1 
 drivers/spi/spi_sh_sci.c |  217 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)

drivers/spi/Kconfig +7 -0

@@ -175,6 +175,13 @@ config SPI_S3C24XX_GPIO
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
 
+config SPI_SH_SCI
+	tristate "SuperH SCI SPI controller"
+	depends on SPI_MASTER && SUPERH
+	select SPI_BITBANG
+	help
+	  SPI driver for SuperH SCI blocks.
+
 config SPI_TXX9
 	tristate "Toshiba TXx9 SPI controller"
 	depends on SPI_MASTER && GENERIC_GPIO && CPU_TX49XX

drivers/spi/Makefile +1 -0

@@ -27,6 +27,7 @@ obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
 obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
+obj-$(CONFIG_SPI_SH_SCI)		+= spi_sh_sci.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)

drivers/spi/spi_sh_sci.c new file +14 -0

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+*/
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+

drivers/spi/spi_sh_sci.c new file +15 -0

+	struct sh_spi_info *info;
+	struct platform_device *dev;
+};
+
+#define SCSPTR(sg) (sg->mapbase + 0x1c)
+#define PIN_SCK (1 << 2)
+#define PIN_TXD (1 << 0)
+#define PIN_RXD PIN_TXD
+#define PIN_INIT ((1 << 1) | (1 << 3))
+
+static inline struct sh_sci_spi *spidev_to_sg(struct spi_device *spi)
+{
+	return spi->controller_data;
+}
+

drivers/spi/spi_sh_sci.c new file +10 -0

+}
+
+static inline unsigned char getbits(struct sh_sci_spi *sg, int bits)
+{
+	return ctrl_inb(SCSPTR(sg)) & bits;
+}
+
+static inline void setsck(struct spi_device *dev, int on)
+{
+	setbits(spidev_to_sg(dev), PIN_SCK, on);

drivers/spi/spi_sh_sci.c new file +85 -0

+}
+
+#define spidelay(x) ndelay(x)
+
+#define	EXPAND_BITBANG_TXRX
+#include <linux/spi/spi_bitbang.h>
+
+static u32 sh_sci_spi_txrx_mode0(struct spi_device *spi,
+				      unsigned nsecs, u32 word, u8 bits)
+{
+	return bitbang_txrx_be_cpha0(spi, nsecs, 0, word, bits);
+}
+
+static u32 sh_sci_spi_txrx_mode1(struct spi_device *spi,
+				      unsigned nsecs, u32 word, u8 bits)
+{
+	return bitbang_txrx_be_cpha1(spi, nsecs, 0, word, bits);
+}
+
+static u32 sh_sci_spi_txrx_mode2(struct spi_device *spi,
+				      unsigned nsecs, u32 word, u8 bits)
+{
+	return bitbang_txrx_be_cpha0(spi, nsecs, 1, word, bits);
+}
+
+static u32 sh_sci_spi_txrx_mode3(struct spi_device *spi,
+				      unsigned nsecs, u32 word, u8 bits)
+{
+	return bitbang_txrx_be_cpha1(spi, nsecs, 1, word, bits);
+}
+
+
+static void sh_sci_spi_chipselect(struct spi_device *dev, int value)
+{
+	struct sh_sci_spi *sg = spidev_to_sg(dev);
+
+	if (sg->info && sg->info->chip_select)
+		(sg->info->chip_select)(sg->info, dev->chip_select, value);
+}
+
+static int sh_sci_spi_probe(struct platform_device *dev)
+{
+	struct resource	*r;
+	struct sh_spi_info *info;
+	struct spi_master *master;
+	struct sh_sci_spi *sp;
+	int ret;
+	int i;
+
+	master = spi_alloc_master(&dev->dev, sizeof(struct sh_sci_spi));
+	if (master = NULL) {
+		dev_err(&dev->dev, "failed to allocate spi master\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	sp = spi_master_get_devdata(master);
+
+	platform_set_drvdata(dev, sp);
+
+	/* copy in the platform data */
+	info = sp->info = dev->dev.platform_data;
+
+	/* setup spi bitbang adaptor */
+	sp->bitbang.master = spi_master_get(master);
+	sp->bitbang.master->bus_num = info->bus_num;
+	sp->bitbang.master->num_chipselect = info->num_chipselect;
+	sp->bitbang.chipselect = sh_sci_spi_chipselect;
+
+	sp->bitbang.txrx_word[SPI_MODE_0] = sh_sci_spi_txrx_mode0;
+	sp->bitbang.txrx_word[SPI_MODE_1] = sh_sci_spi_txrx_mode1;
+	sp->bitbang.txrx_word[SPI_MODE_2] = sh_sci_spi_txrx_mode2;
+	sp->bitbang.txrx_word[SPI_MODE_3] = sh_sci_spi_txrx_mode3;
+
+	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (r = NULL) {
+		ret = -ENOENT;
+		goto err1;
+	}
+	sp->mapbase = r->start;
+	setbits(sp, PIN_INIT, 1);
+
+	ret = spi_bitbang_start(&sp->bitbang);
+	if (ret)
+		goto err2;

drivers/spi/spi_sh_sci.c new file +45 -0

+	}
+	return 0;
+ err2:
+	setbits(sp, PIN_INIT, 0);
+ err1:
+	spi_master_put(sp->bitbang.master);
+ err0:
+	return ret;
+}
+
+static int sh_sci_spi_remove(struct platform_device *dev)
+{
+	struct sh_sci_spi *sp = platform_get_drvdata(dev);
+
+	spi_bitbang_stop(&sp->bitbang);
+	setbits(sp, PIN_INIT, 0);
+	spi_master_put(sp->bitbang.master);
+	return 0;
+}
+
+static struct platform_driver sh_sci_spi_drv = {
+	.probe		= sh_sci_spi_probe,
+	.remove		= sh_sci_spi_remove,
+	.driver		= {
+		.name	= "spi_sh_sci",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init sh_sci_spi_init(void)
+{
+	return platform_driver_register(&sh_sci_spi_drv);
+}
+
+static void __exit sh_sci_spi_exit(void)
+{
+	platform_driver_unregister(&sh_sci_spi_drv);
+}
+
+module_init(sh_sci_spi_init);
+module_exit(sh_sci_spi_exit);
+
+MODULE_DESCRIPTION("SH SCI SPI Driver");
+MODULE_AUTHOR("Magnus Damm");
+MODULE_LICENSE("GPL");

drivers/spi/spi_sh_sci.c line 32

Quoted 4 lines · Magnus Damm

drivers/spi/spi_sh_sci.c new file +4 -0

+struct sh_sci_spi {
+	struct spi_bitbang bitbang;
+
+	unsigned long mapbase;

Not "void __iomem *mapbase"?

I notice that you're not doing an ioremap() to get this address. Is the platform device properly registering a *physical* address in the IORESOURCE_MEM record for the device?

drivers/spi/spi_sh_sci.c line 84

Quoted 5 lines

drivers/spi/spi_sh_sci.c new file +5 -0

+#define spidelay(x) ndelay(x)
+
+#define	EXPAND_BITBANG_TXRX
+#include <linux/spi/spi_bitbang.h>
+

Quoted 5 lines

drivers/spi/spi_sh_sci.c line 84

Quoted 5 lines

drivers/spi/spi_sh_sci.c new file +5 -0

+#define spidelay(x) ndelay(x)
+
+#define	EXPAND_BITBANG_TXRX
+#include <linux/spi/spi_bitbang.h>
+

This is rather unorthodox..

then it's not bad.  And it's twice as fast as going through a subroutine call for each bitop.  That's easily visible on

Quoted 40 lines · Magnus Damm

spi: add support for SPI over SuperH SCI pins

This patch adds support for SPI over SCI pins. SCI is a very simple serial controller block that can be found on older SuperH processors. In theory it is possible to use the SCI hardware block in syncronous mode, but this version of the driver simply hooks up the bit banging code on the SCI pins.

Signed-off-by Magnus Damm <[email protected]>

---

 drivers/spi/Kconfig      |    7 +
 drivers/spi/Makefile     |    1
 drivers/spi/spi_sh_sci.c |  217 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)

drivers/spi/Kconfig 2008-01-21 16:59:39.000000000 +0900 renamed from drivers/spi/Kconfig +7 -0

@@ -175,6 +175,13 @@ config SPI_S3C24XX_GPIO
        the inbuilt hardware cannot provide the transfer mode, or
        where the board is using non hardware connected pins.
 
+config SPI_SH_SCI
+     tristate "SuperH SCI SPI controller"
+     depends on SPI_MASTER && SUPERH
+     select SPI_BITBANG
+     help
+       SPI driver for SuperH SCI blocks.
+
 config SPI_TXX9
      tristate "Toshiba TXx9 SPI controller"
      depends on SPI_MASTER && GENERIC_GPIO && CPU_TX49XX

drivers/spi/Makefile 2008-01-21 16:59:39.000000000 +0900 renamed from drivers/spi/Makefile +1 -0

@@ -27,6 +27,7 @@ obj-$(CONFIG_SPI_S3C24XX_GPIO)              += spi_s
 obj-$(CONFIG_SPI_S3C24XX)            += spi_s3c24xx.o
 obj-$(CONFIG_SPI_TXX9)                       += spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)             += xilinx_spi.o
+obj-$(CONFIG_SPI_SH_SCI)             += spi_sh_sci.o
 #    ... add above this line ...
 

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 1

Quoted 5 lines

drivers/spi/Makefile 2008-01-21 16:59:39.000000000 +0900 renamed from drivers/spi/Makefile +0 -0

 # SPI protocol drivers (device/link on bus)

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +1 -0

@@ -0,0 +1,217 @@
+/* linux/drivers/spi/spi_sh_sci.c

The general policy is to not include the whole driver pathname, since paths change over time. Better yet, I don't see any need for a pathname here ... just remove it.

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 8

Quoted 7 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +7 -0

+ *
+ * SH SCI SPI interface
+ *
+ * Based on S3C24XX GPIO based SPI driver
+ *
+ * Copyright (c) 2006 Ben Dooks
+ * Copyright (c) 2006 Simtec Electronics

I see a missing copyright there: yours!

I'd be relatively certain Ben and Simtek don't want to be maintain a driver they never had anything to do with, so please make sure it doesn't look like they wrote it.

You should consider adding yourself to MAINTAINERS for this driver...

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 12

Quoted 4 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +4 -0

+struct sh_sci_spi {
+     struct spi_bitbang bitbang;
+
+     unsigned long mapbase;

Not "void __iomem *mapbase"?

I notice that you're not doing an ioremap() to get this address. Is the platform device properly registering a *physical* address in the IORESOURCE_MEM record for the device?

Quoted 15 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +15 -0

+     struct sh_spi_info *info;
+     struct platform_device *dev;
+};
+
+#define SCSPTR(sg) (sg->mapbase + 0x1c)
+#define PIN_SCK (1 << 2)
+#define PIN_TXD (1 << 0)
+#define PIN_RXD PIN_TXD
+#define PIN_INIT ((1 << 1) | (1 << 3))
+
+static inline struct sh_sci_spi *spidev_to_sg(struct spi_device *spi)
+{
+     return spi->controller_data;
+}
+

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 37

Quoted 10 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +10 -0

+static inline void setbits(struct sh_sci_spi *sg, int bits, int on)
+{
+     unsigned char val;
+
+     val = ctrl_inb(SCSPTR(sg));
+     if (on)
+             val |= bits;
+     else
+             val &= ~bits;
+     ctrl_outb(val, SCSPTR(sg));

I notice there are no locks around this read/modify/write operation.

As a rule that's unsafe for GPIO registers -- since another activity (IRQ handler or whatever) can do the same thing, and then you'd have one stomping all over the other. Is there something that would make it safe in this case? If not, fix; if so, comment.

Quoted 17 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +17 -0

+}
+
+static inline unsigned char getbits(struct sh_sci_spi *sg, int bits)
+{
+     return ctrl_inb(SCSPTR(sg)) & bits;
+}
+
+static inline void setsck(struct spi_device *dev, int on)
+{
+     setbits(spidev_to_sg(dev), PIN_SCK, on);
+}
+
+static inline void setmosi(struct spi_device *dev, int on)
+{
+     setbits(spidev_to_sg(dev), PIN_TXD, on);
+}
+

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 57

Quoted 3 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +3 -0

+static inline u32 getmiso(struct spi_device *dev)
+{
+     return getbits(spidev_to_sg(dev), PIN_RXD) ? 1 : 0;

I worry about all those spidev_to_sg(dev) calls ... did you look at the generated code to make sure they get pulled out of the inner loops? Declaring some stuff "const" might help if that's an issue.

Ideally the setsck(), setmosi(), and getmiso() calls optimize to a couple instructions each, at least inside the I/O loops below. It works that way on some systems, and I'd think it might be possible on this one too.

Quoted 4 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +4 -0

+     /* register the chips to go with the board */
+
+     for (i = 0; i < sp->info->board_size; i++) {
+             dev_info(&dev->dev, "registering %p: %s\n",

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 line 66

Quoted 5 lines

drivers/spi/spi_sh_sci.c 2008-01-21 17:04:02.000000000 +0900 new file +5 -0

+                      &sp->info->board_info[i],
+                      sp->info->board_info[i].modalias);
+
+             sp->info->board_info[i].controller_data = sp;
+             spi_new_device(master, sp->info->board_info + i);

NO -- this doesn't belong here at all. Such registration is handled by the SPI core code, according to what the board init code told it.

Quoted 3 lines · David Brownell

You should consider adding yourself to MAINTAINERS for this driver...

Yes. If it's ok with you then i'll send a separate patch for that. I should probably add more than one entry there.

Quoted 4 lines

Quoted 4 lines

drivers/spi/spi_sh_sci.c new file +4 -0

+     /* register the chips to go with the board */
+
+     for (i = 0; i < sp->info->board_size; i++) {
+             dev_info(&dev->dev, "registering %p: %s\n",

Quoted 8 lines

drivers/spi/spi_sh_sci.c line 172

Quoted 5 lines

drivers/spi/spi_sh_sci.c new file +5 -0

+                      &sp->info->board_info[i],
+                      sp->info->board_info[i].modalias);
+
+             sp->info->board_info[i].controller_data = sp;
+             spi_new_device(master, sp->info->board_info + i);

NO -- this doesn't belong here at all. Such registration is handled by the SPI core code, according to what the board init code told it.

Hehe. My gut reaction was the same when i read the s3c24xx drivers,

but then I decided to follow their style. I'll remove that part then and instead register the spi devices from the board code using spi_register_board_info().

I'll fix up the code and repost in a bit. Thanks for your review!

Quoted 7 lines · Paul Mundt

Quoted 5 lines

drivers/spi/spi_sh_sci.c line 84

Quoted 5 lines

drivers/spi/spi_sh_sci.c new file +5 -0

+#define spidelay(x) ndelay(x)
+
+#define	EXPAND_BITBANG_TXRX
+#include <linux/spi/spi_bitbang.h>
+

This is rather unorthodox..

return -ENOPATCH;)

I've seen similar idioms used for years. It's not dissimilar to "#ifdef __KERNEL__" except for the code audiences: two different drivers, vs two different address spaces.

Similar approaches have been used to expand bit manipulation algorithms in other contexts ... like some X11 servers I once had to cope with. In fact that was infamous for breaking CPP on new platforms; that was before C grew inlines! But the rationale is still the same: make sure the compiler has every opportunity to optimize each variant of those inner loops, since they're performance-critical.

And it works. I append the code generated on one ARM which inlines the bitops ... the inner loop isn't as fast as one could code by hand, but if you consider it's four GPIO ops then it's not bad. And it's twice as fast as going through a subroutine call for each bitop. That's easily visible on block I/O paths that unfortunately can't use hardware SPI on that system.

Minor clarification: twice as fast for loads involving lots of medium-size buffers (1K+). Down from one minute(!) to put new kernels to the only place that they can boot from ... it's the usual case where a 5x speedup at one place is very visible, but doesn't add up to 5x overall; other issues start to show up more (like unchanged per-byte overheads).