[PATCH] spi: add support for SPI over SuperH SCI pins

[PATCH] spi: add support for SPI over SuperH SCI pins

[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 <damm@igel.co.jp>
---

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

--- 0001/drivers/spi/Kconfig
+++ work/drivers/spi/Kconfig	2008-01-21 16:59:39.000000000 +0900
@@ -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
--- 0001/drivers/spi/Makefile
+++ work/drivers/spi/Makefile	2008-01-21 16:59:39.000000000 +0900
@@ -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)
--- /dev/null
+++ work/drivers/spi/spi_sh_sci.c	2008-01-21 17:04:02.000000000 +0900
@@ -0,0 +1,217 @@
+/* linux/drivers/spi/spi_sh_sci.c
+ *
+ * SH SCI SPI interface
+ *
+ * Based on S3C24XX GPIO based SPI driver
+ *
+ * Copyright (c) 2006 Ben Dooks
+ * Copyright (c) 2006 Simtec Electronics
+ *
+ * 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>
+
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#include <asm/spi.h>
+#include <asm/io.h>
+
+struct sh_sci_spi {
+	struct spi_bitbang bitbang;
+
+	unsigned long mapbase;
+	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;
+}
+
+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));
+}
+
+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);
+}
+
+static inline u32 getmiso(struct spi_device *dev)
+{
+	return getbits(spidev_to_sg(dev), PIN_RXD) ? 1 : 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;
+
+	/* 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",
+			 &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);
+	}
+	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");

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[David Brownell]

On Monday 21 January 2008, Magnus Damm wrote:
> 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 <damm@igel.co.jp>
> ---
> 
>  drivers/spi/Kconfig      |    7 +
>  drivers/spi/Makefile     |    1 
>  drivers/spi/spi_sh_sci.c |  217 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
> 
> --- 0001/drivers/spi/Kconfig
> +++ work/drivers/spi/Kconfig	2008-01-21 16:59:39.000000000 +0900
> @@ -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
> --- 0001/drivers/spi/Makefile
> +++ work/drivers/spi/Makefile	2008-01-21 16:59:39.000000000 +0900
> @@ -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)
> --- /dev/null
> +++ work/drivers/spi/spi_sh_sci.c	2008-01-21 17:04:02.000000000 +0900
> @@ -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.

> + *
> + * 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...


> + *
> + * 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>
> +
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi_bitbang.h>
> +
> +#include <asm/spi.h>
> +#include <asm/io.h>
> +
> +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?


> +	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;
> +}
> +
> +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.


> +}
> +
> +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);
> +}
> +
> +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.


> +}
> +
> +#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;
> +
> +	/* 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",
> +			 &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.


> +	}
> +	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");
> 

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[Paul Mundt]

On Mon, Jan 21, 2008 at 02:29:29PM -0800, David Brownell wrote:
> On Monday 21 January 2008, Magnus Damm wrote:
> > +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?
> 
It should be ioremap(), yes. While for these parts ioremap() doesn't
really do anything, it still takes care of the void __iomem * casting
properly, and there are in fact parts where we have no choice but to go
through page tables anyways.

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

This is rather unorthodox..

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[Magnus Damm]

On Jan 22, 2008 7:29 AM, David Brownell <david-b@pacbell.net> wrote:
>
> On Monday 21 January 2008, Magnus Damm wrote:
> > 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 <damm@igel.co.jp>
> > ---
> >
> >  drivers/spi/Kconfig      |    7 +
> >  drivers/spi/Makefile     |    1
> >  drivers/spi/spi_sh_sci.c |  217 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 225 insertions(+)
> >
> > --- 0001/drivers/spi/Kconfig
> > +++ work/drivers/spi/Kconfig  2008-01-21 16:59:39.000000000 +0900
> > @@ -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
> > --- 0001/drivers/spi/Makefile
> > +++ work/drivers/spi/Makefile 2008-01-21 16:59:39.000000000 +0900
> > @@ -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)
> > --- /dev/null
> > +++ work/drivers/spi/spi_sh_sci.c     2008-01-21 17:04:02.000000000 +0900
> > @@ -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.

Ok, I'll remove that comment.

> > + *
> > + * 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!

Uhm, yes. I'll add that. =)

> 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.

Absolutely. Sorry about that.

> 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.

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

I just followed the good old sh-sci.c serial driver, but i'll rewrite that part.

> > +     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;
> > +}
> > +
> > +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.

The SCSPTR register is part of the serial controller block and this
driver should be the only user of that block. So no locks needed. I'll
add a comment that clarifies that.

> > +}
> > +
> > +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);
> > +}
> > +
> > +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.

No, I have not had look at the generated assembly code for this driver
yet, but I'll check it out.

> 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.

Right.

> > +     /* 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",
> > +                      &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!

/ magnus

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[David Brownell]

On Monday 21 January 2008, Paul Mundt wrote:

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

- Dave


000001cc <XX_spi_txrx_word_mode0>:
 1cc:	e1a0c00d 	mov	ip, sp
 1d0:	e92dd810 	push	{r4, fp, ip, lr, pc}
 1d4:	e24cb004 	sub	fp, ip, #4	; 0x4
 1d8:	e3a04008 	mov	r4, #8	; 0x8
 1dc:	e1a0ec02 	lsl	lr, r2, #24

 1e0:	e35e0000 	cmp	lr, #0	; 0x0
 1e4:	e59f303c 	ldr	r3, [pc, #60]	; 228
 1e8:	e3e01401 	mvn	r1, #16777216	; 0x1000000
 1ec:	a3a00034 	movge	r0, #52	; 0x34
 1f0:	b3a00030 	movlt	r0, #48	; 0x30
 1f4:	e3a0c004 	mov	ip, #4	; 0x4
 1f8:	e3a02002 	mov	r2, #2	; 0x2
 1fc:	e7802003 	str	r2, [r0, r3]
 200:	e501cdcf 	str	ip, [r1, #-3535]
 204:	e5113dc3 	ldr	r3, [r1, #-3523]
 208:	e2442001 	sub	r2, r4, #1	; 0x1
 20c:	e2033001 	and	r3, r3, #1	; 0x1
 210:	e21240ff 	ands	r4, r2, #255	; 0xff
 214:	e501cdcb 	str	ip, [r1, #-3531]
 218:	e183e08e 	orr	lr, r3, lr, lsl #1
 21c:	1affffef 	bne	1e0

 220:	e1a0000e 	mov	r0, lr
 224:	e89da810 	ldm	sp, {r4, fp, sp, pc}

 228:	fefff200 	.word	0xfefff200

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[David Brownell]

On Monday 21 January 2008, Magnus Damm wrote:
> On Jan 22, 2008 7:29 AM, David Brownell <david-b@pacbell.net> wrote:
> >
> > 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.

Fine by me.  


> > > +     /* 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",
> > > +                      &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,

Gaak.  How did those get in with such crap?

Aaaah, I see ... I never signed off on their original merge.  Hmm.



> 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!

Sure.

- Dave

Re: [spi-devel-general] [PATCH] spi: add support for SPI over SuperH SCI pins

[David Brownell]

On Monday 21 January 2008, David Brownell wrote:
> 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

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).

- Dave

Re: [PATCH] spi: add support for SPI over SuperH SCI pins

[Paul Mundt]

On Mon, Jan 21, 2008 at 08:22:22PM -0800, David Brownell wrote:
> On Monday 21 January 2008, Paul Mundt wrote:
> 
> > > > +#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.

On Mon, Jan 21, 2008 at 09:26:32PM -0800, David Brownell wrote:
> 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).
> 

The -ENOPATCH thing was intentional, I was more fishing for an
explanation of why this magical sequence existed than lobbying for its
replacement. drivers/spi/ isn't in danger of looking like crypto/ in the
name of code generation just yet, at least. ;-)

Thanks for the clarification.

[PATCH] spi: add support for SPI over SuperH SCI pins V2

[Magnus Damm]

spi: add support for SPI over SuperH SCI pins V2

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.

Changes since V1 include:
 - Comment and copyright fixes.
 - Use ioremap() together with ioread8()/iowrite8().
 - Store register value copy in sp->val.
 - Remove spi device registration code.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

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

--- 0001/drivers/spi/Kconfig
+++ work/drivers/spi/Kconfig	2008-01-23 12:40:15.000000000 +0900
@@ -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
--- 0001/drivers/spi/Makefile
+++ work/drivers/spi/Makefile	2008-01-23 12:40:15.000000000 +0900
@@ -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)
--- /dev/null
+++ work/drivers/spi/spi_sh_sci.c	2008-01-23 13:05:05.000000000 +0900
@@ -0,0 +1,206 @@
+/*
+ * SH SCI SPI interface
+ *
+ * Copyright (c) 2008 Magnus Damm
+ *
+ * Based on S3C24XX GPIO based SPI driver
+ *
+ * Copyright (c) 2006 Ben Dooks
+ * Copyright (c) 2006 Simtec Electronics
+ *
+ * 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>
+
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#include <asm/spi.h>
+#include <asm/io.h>
+
+struct sh_sci_spi {
+	struct spi_bitbang bitbang;
+
+	void __iomem *membase;
+	unsigned char val;
+	struct sh_spi_info *info;
+	struct platform_device *dev;
+};
+
+#define SCSPTR(sp) (sp->membase + 0x1c)
+#define PIN_SCK (1 << 2)
+#define PIN_TXD (1 << 0)
+#define PIN_RXD PIN_TXD
+#define PIN_INIT ((1 << 1) | (1 << 3) | PIN_SCK | PIN_TXD)
+
+static inline void setbits(struct sh_sci_spi *sp, int bits, int on)
+{
+	/*
+	 * We are the only user of SCSPTR so no locking is required.
+	 * Reading bit 2 and 0 in SCSPTR gives pin state as input.
+	 * Writing the same bits sets the output value.
+	 * This makes regular read-modify-write difficult so we
+	 * use sp->val to keep track of the latest register value.
+	 */
+
+	if (on)
+		sp->val |= bits;
+	else
+		sp->val &= ~bits;
+
+	iowrite8(sp->val, SCSPTR(sp));
+}
+
+static inline void setsck(struct spi_device *dev, int on)
+{
+	setbits(spi_master_get_devdata(dev->master), PIN_SCK, on);
+}
+
+static inline void setmosi(struct spi_device *dev, int on)
+{
+	setbits(spi_master_get_devdata(dev->master), PIN_TXD, on);
+}
+
+static inline u32 getmiso(struct spi_device *dev)
+{
+	struct sh_sci_spi *sp = spi_master_get_devdata(dev->master);
+
+	return (ioread8(SCSPTR(sp)) & PIN_RXD) ? 1 : 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 *sp = spi_master_get_devdata(dev->master);
+
+	if (sp->info && sp->info->chip_select)
+		(sp->info->chip_select)(sp->info, dev->chip_select, value);
+}
+
+static int sh_sci_spi_probe(struct platform_device *dev)
+{
+	struct resource	*r;
+	struct spi_master *master;
+	struct sh_sci_spi *sp;
+	int ret;
+
+	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);
+	sp->info = dev->dev.platform_data;
+
+	/* setup spi bitbang adaptor */
+	sp->bitbang.master = spi_master_get(master);
+	sp->bitbang.master->bus_num = sp->info->bus_num;
+	sp->bitbang.master->num_chipselect = sp->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->membase = ioremap(r->start, r->end - r->start + 1);
+	if (!sp->membase) {
+		ret = -ENXIO;
+		goto err1;
+	}
+	sp->val = ioread8(SCSPTR(sp));
+	setbits(sp, PIN_INIT, 1);
+
+	ret = spi_bitbang_start(&sp->bitbang);
+	if (!ret)
+		return 0;
+
+	setbits(sp, PIN_INIT, 0);
+	iounmap(sp->membase);
+ 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);
+
+	iounmap(sp->membase);
+	setbits(sp, PIN_INIT, 0);
+	spi_bitbang_stop(&sp->bitbang);
+	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 <damm@opensource.se>");
+MODULE_LICENSE("GPL");