diff mbox

SPI: EMMA Mobile SPI master driver

Message ID 1378386669-9920-2-git-send-email-ian.molton@codethink.co.uk (mailing list archive)
State Changes Requested
Headers show

Commit Message

Ian Molton Sept. 5, 2013, 1:11 p.m. UTC 
This patch provides support for the EMMA mobile SPI master controller.

At present, only PIO mode is supported, and unless GPIOs are used for
chipselect, transfers are limited to 4 bytes maximum.

RW transfers are not supported, and only 8 bit words are supported at present.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
---
 drivers/spi/Kconfig  |    8 +
 drivers/spi/Makefile |    1 +
 drivers/spi/spi-em.c |  486 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 495 insertions(+)
 create mode 100644 drivers/spi/spi-em.c

Comments

Simon Horman Sept. 6, 2013, 2:16 a.m. UTC | #1 
On Thu, Sep 05, 2013 at 02:11:09PM +0100, Ian Molton wrote:
> This patch provides support for the EMMA mobile SPI master controller.
> 
> At present, only PIO mode is supported, and unless GPIOs are used for
> chipselect, transfers are limited to 4 bytes maximum.
> 
> RW transfers are not supported, and only 8 bit words are supported at present.
> 
> Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>

Hi Ian,

I suspect that Magnus is the best person to review this code
(although he may have other ideas). Nonetheless, I have made
a few comments below.

> ---
>  drivers/spi/Kconfig  |    8 +
>  drivers/spi/Makefile |    1 +
>  drivers/spi/spi-em.c |  486 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 495 insertions(+)
>  create mode 100644 drivers/spi/spi-em.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 89cbbab..4a648c0 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -164,6 +164,14 @@ config SPI_EP93XX
>  	  This enables using the Cirrus EP93xx SPI controller in master
>  	  mode.
>  
> +config SPI_EM
> +        tristate "EMXX built in SPI controller"
> +        depends on ARCH_EMEV2
> +        help
> +          This enables the emxx SPI driver in master mode. Only short transfers
> +	  are supported without extra chipselect logic in PIO mode. DMA is not
> +	  currently supported.
> +
>  config SPI_FALCON
>  	tristate "Falcon SPI controller support"
>  	depends on SOC_FALCON
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 33f9c09..41c7a23 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
>  obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
>  spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
>  obj-$(CONFIG_SPI_EP93XX)		+= spi-ep93xx.o
> +obj-$(CONFIG_SPI_EM)			+= spi-em.o
>  obj-$(CONFIG_SPI_FALCON)		+= spi-falcon.o
>  obj-$(CONFIG_SPI_FSL_CPM)		+= spi-fsl-cpm.o
>  obj-$(CONFIG_SPI_FSL_LIB)		+= spi-fsl-lib.o
> diff --git a/drivers/spi/spi-em.c b/drivers/spi/spi-em.c
> new file mode 100644
> index 0000000..2c3e980
> --- /dev/null
> +++ b/drivers/spi/spi-em.c
> @@ -0,0 +1,486 @@
> +/*
> + * EMXX SPI Master driver
> + * Based on spi_sh.
> + *
> + * Copyright (C) 2013 Codethink Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/errno.h>
> +#include <linux/clk.h>
> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <linux/list.h>
> +#include <linux/workqueue.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/io.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/gpio.h>
> +
> +struct em_spi_data {
> +	void __iomem *addr;
> +	int irq;
> +	struct clk *clk;
> +	struct clk *sclk;
> +	struct spi_master *master;
> +	wait_queue_head_t wait;
> +	unsigned int status;
> +	unsigned int chip_select;
> +	unsigned int hard_chip_select;
> +	unsigned int pol_mask;
> +	unsigned int pol_data;
> +	unsigned int mode;
> +	unsigned int *cs_gpio;
> +};
> +
> +#define EMXX_SPI_BLOCK_MODE_SWITCH_EN	0x0000
> +#define EMXX_SPI_BLOCK_MODE		0x0004
> +
> +#define EMXX_SPI_MODE			0x1000
> +#define EMXX_SPI_POL			0x1004
> +#define EMXX_SPI_CTL			0x1008
> +#define EMXX_SPI_TX			0x1010
> +#define EMXX_SPI_RX			0x1014
> +#define EMXX_SPI_STAT			0x1018
> +#define EMXX_SPI_RAWSTAT		0x101c
> +#define EMXX_SPI_ENSET			0x1020
> +#define EMXX_SPI_ENCLR			0x1024
> +#define EMXX_SPI_FFCLR			0x1028
> +
> +#define EMXX_SPI_TX_STOP		(1 << 6)
> +#define EMXX_SPI_RX_STOP		(1 << 5)
> +#define EMXX_SPI_TERR			(1 << 4)
> +#define EMXX_SPI_RDV			(1 << 3)
> +#define EMXX_SPI_END			(1 << 2)
> +#define EMXX_SPI_TX_UDR			(1 << 1)
> +#define EMXX_SPI_RX_OVR			(1 << 0)
> +
> +#define EMXX_SPI_ERR_STAT	(EMXX_SPI_RX_OVR | EMXX_SPI_TX_UDR | \
> +					EMXX_SPI_TERR)
> +
> +#define EMXX_SPI_ALL_IRQ	(EMXX_SPI_TERR | EMXX_SPI_RDV | EMXX_SPI_END | \
> +					EMXX_SPI_TX_UDR | EMXX_SPI_RX_OVR)
> +#define EMXX_SPI_RX_IRQS	(EMXX_SPI_RX_OVR | EMXX_SPI_RDV)
> +#define EMXX_SPI_TX_IRQS	(EMXX_SPI_TERR | EMXX_SPI_END | EMXX_SPI_TX_UDR)
> +
> +#define EMXX_SPI_FLAG_START			(1 << 0)
> +#define EMXX_SPI_FLAG_RX		(1 << 2)
> +#define EMXX_SPI_FLAG_TX		(1 << 3)
> +
> +static void em_spi_set_cs(struct em_spi_data *em_spi, int cs, int state)
> +{
> +	gpio_direction_output(em_spi->cs_gpio[cs], state);
> +}
> +
> +/*
> + * Soft reset. Set bit 8 of the control register.
> + * The chip will reset the interrupt status register, so no need to
> + * disable IRQs.
> + *
> + */
> +static void em_spi_soft_reset(struct em_spi_data *em_spi)
> +{
> +	writel(1 << 8, em_spi->addr + EMXX_SPI_CTL);
> +	msleep(1);
> +	writel(0, em_spi->addr + EMXX_SPI_CTL);
> +	msleep(1);
> +}
> +
> +void em_spi_set_clock(struct em_spi_data *em_spi, unsigned long freq)
> +{
> +	int rate = clk_round_rate(em_spi->sclk, freq);
> +
> +	clk_set_rate(em_spi->sclk, rate);
> +
> +	/* Plenty long enough for clocks to stabilise */
> +	msleep(1);
> +}
> +
> +static void em_spi_setup_transfer(struct em_spi_data *em_spi)
> +{
> +	unsigned long pol_reg;
> +
> +	pol_reg = readl(em_spi->addr + EMXX_SPI_POL);
> +	pol_reg &= em_spi->pol_mask;
> +	pol_reg |= em_spi->pol_data;
> +	writel(pol_reg, em_spi->addr + EMXX_SPI_POL);
> +
> +	writel(em_spi->mode, em_spi->addr + EMXX_SPI_MODE);
> +
> +}
> +
> +static int em_spi_start_transfer(struct em_spi_data *em_spi,
> +				unsigned int flags)
> +{
> +	unsigned int irqs = 0;
> +
> +	if (flags == EMXX_SPI_FLAG_RX)
> +		irqs |= EMXX_SPI_RX_IRQS;
> +
> +	if (flags == EMXX_SPI_FLAG_TX)
> +		irqs |= EMXX_SPI_TX_IRQS;
> +
> +	/* Enable IRQs */
> +	em_spi->status = 0;
> +	writel(irqs, em_spi->addr + EMXX_SPI_ENSET);
> +
> +	/* Kick off the transfer */
> +	writel(flags | EMXX_SPI_FLAG_START, em_spi->addr + EMXX_SPI_CTL);
> +
> +	return 0;
> +}
> +
> +static int em_spi_tx(struct em_spi_data *em_spi, struct spi_transfer *t)
> +{
> +	const char *tx_buf = t->tx_buf;
> +	int i, ret;
> +
> +	for (i = 0; i < t->len; i++) {
> +

Minor nit, there is no need for a blank line here.

> +		writel(tx_buf[i], em_spi->addr + EMXX_SPI_TX);
> +
> +		em_spi_start_transfer(em_spi, EMXX_SPI_FLAG_TX);
> +
> +		ret = wait_event_interruptible_timeout(em_spi->wait,
> +			em_spi->status & EMXX_SPI_TX_IRQS, HZ);
> +
> +		/*
> +		 * We're abusing the chip a bit so that we can do
> +		 * multi-byte operations in PIO mode.
> +		 * As a result we need to spin until the START bit drops here.
> +		 *
> +		 * This only works with GPIO based chipselects, unless the
> +		 * transfer size is 4 bytes or less. Verified on a scope.
> +		 *
> +		 * DMA mode appears not to have this restriction.
> +		 */
> +
> +		while (readl(em_spi->addr + EMXX_SPI_CTL) & 1)
> +			;

		Please impose some kind of limit on this loop so
		that it can never be endless.

		My naïve implementation would be something like this.

		int i;
		bool started = false;
		/* 1000 is an arbitrary limit to prevent infinite loop */
		for (i = 0; i < 1000; i++) {
			if (!readl(em_spi->addr + EMXX_SPI_CTL) & 1)) {
				started = true;
				break;
			}
		}

		if (!started || ret < 0)
			return -ETIMEDOUT;


		Also, should the readl loop occor even if ret is less than 0?
> +
> +		if (ret < 0)
> +			return -ETIMEDOUT;
> +
> +		if (em_spi->status & EMXX_SPI_ERR_STAT)
> +			return -EINVAL;
> +
> +		writel(EMXX_SPI_END, em_spi->addr + EMXX_SPI_FFCLR);
> +	}
> +
> +	return 0;
> +}
> +
> +static int em_spi_rx(struct em_spi_data *em_spi, struct spi_transfer *t)
> +{
> +	char *rx_buf = t->rx_buf;
> +	int i, ret;
> +
> +	for (i = 0; i < t->len; i++) {
> +		em_spi_start_transfer(em_spi, EMXX_SPI_FLAG_RX);
> +
> +		/* We might want to try reading more than one word per irq */
> +		ret = wait_event_interruptible_timeout(em_spi->wait,
> +			em_spi->status & EMXX_SPI_RX_IRQS, HZ);
> +
> +		if (ret < 0)
> +			return -ETIMEDOUT;
> +
> +		if (em_spi->status & EMXX_SPI_ERR_STAT)
> +			return -EINVAL;
> +
> +		rx_buf[i] = readl(em_spi->addr + EMXX_SPI_RX);
> +
> +		writel(EMXX_SPI_RDV, em_spi->addr + EMXX_SPI_FFCLR);
> +	}
> +
> +	return 0;
> +}
> +
> +static int em_spi_setup(struct spi_device *spi);
> +
> +static int em_spi_transfer_one_message(struct spi_master *master,
> +					struct spi_message *mesg)
> +{
> +	struct em_spi_data *em_spi = spi_master_get_devdata(master);
> +	struct spi_transfer *t;
> +	int ret;
> +
> +	em_spi_setup(mesg->spi);
> +
> +	em_spi_set_cs(em_spi, em_spi->chip_select, 1);
> +
> +	em_spi_setup_transfer(em_spi);
> +
> +	list_for_each_entry(t, &mesg->transfers, transfer_list) {
> +

I'm not a fan of this blank line.

> +		if (t->tx_buf && t->rx_buf) {
> +			/* Do not yet support concurrent transfers */
> +			BUG();

Is this really a bug?
Can it actually happen?
If so I think it should be handled as an error.

I understand that this driver is missing some features
and I assume this is one of them. But none the less
BUG() seems a bit extreme.

> +			goto error;
> +		}
> +
> +		if (t->speed_hz)
> +			em_spi_set_clock(em_spi, t->speed_hz);
> +
> +		if (t->tx_buf)
> +			ret = em_spi_tx(em_spi, t);
> +		else if (t->rx_buf)
> +			ret = em_spi_rx(em_spi, t);
> +
> +		if (ret)
> +			goto error;
> +
> +		mesg->actual_length += t->len;
> +	}
> +
> +	em_spi_set_cs(em_spi, em_spi->chip_select, 0);
> +
> +	mesg->status = 0;
> +	spi_finalize_current_message(master);
> +
> +	return 0;
> +
> +error:
> +	dev_err(&master->dev, "SPI status = %08x, ret = %d\n",
> +		em_spi->status, ret);
> +
> +	em_spi_soft_reset(em_spi);
> +
> +	mesg->status = ret;
> +	spi_finalize_current_message(master);
> +
> +	return ret;
> +}
> +
> +static int em_spi_setup(struct spi_device *spi)
> +{
> +	struct em_spi_data *em_spi = spi_master_get_devdata(spi->master);
> +	unsigned long pol = 0, mode = 0, shift, actual_cs;
> +
> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;
> +
> +	if (spi->bits_per_word != 8)
> +		BUG(); /* We dont support >8 bit transfers */

Likewise, I think this would be better handled as an error.

> +
> +	if (spi->max_speed_hz)
> +		em_spi_set_clock(em_spi, spi->max_speed_hz);
> +
> +	/* Select clock and cs polarity */
> +	pol |= !(spi->mode & SPI_CPHA)   ? (1 << 2) : 0;
> +	pol |= (spi->mode & SPI_CPOL)    ? (1 << 1) : 0;
> +	pol |= (spi->mode & SPI_CS_HIGH) ? (1 << 0) : 0;
> +
> +	if (em_spi->cs_gpio)
> +		actual_cs = em_spi->hard_chip_select;
> +	else
> +		actual_cs = spi->chip_select;
> +
> +	shift = actual_cs * 3;
> +
> +	/* Take into account gap in reg for CSW */
> +	if (shift > 9)
> +		shift += 4;
> +
> +	mode |= ((spi->bits_per_word - 1) & 0x1f) << 8;
> +	mode |= (actual_cs & 0x7) << 4;
> +
> +	em_spi->pol_mask = ~((0x7 << shift) | (0xf << 12));
> +	em_spi->pol_data = pol << shift;
> +	em_spi->mode = mode;
> +	em_spi->chip_select = spi->chip_select;
> +
> +	return 0;
> +}
> +
> +static void em_spi_power_on(struct em_spi_data *em_spi)
> +{
> +	clk_enable(em_spi->clk);
> +	clk_enable(em_spi->sclk);
> +}
> +
> +static void em_spi_power_off(struct em_spi_data *em_spi)
> +{
> +	clk_disable(em_spi->sclk);
> +	clk_disable(em_spi->clk);
> +}
> +
> +static irqreturn_t em_spi_irq(int irq, void *data)
> +{
> +	struct em_spi_data *em_spi = (struct em_spi_data *)data;
> +	unsigned long status;
> +
> +	status = readl(em_spi->addr + EMXX_SPI_STAT);
> +
> +	if (status) {
> +		em_spi->status = status;
> +		writel(status, em_spi->addr + EMXX_SPI_ENCLR);
> +
> +		wake_up(&em_spi->wait);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int em_spi_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
> +	struct spi_master *master;
> +	struct em_spi_data *em_spi;
> +	int ret, irq, n_cs_gpio;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(res == NULL)) {
> +		dev_err(&pdev->dev, "invalid resource\n");
> +		return -EINVAL;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "platform_get_irq error\n");
> +		return -ENODEV;
> +	}
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(struct em_spi_data));
> +	if (master == NULL) {
> +		dev_err(&pdev->dev, "spi_alloc_master error.\n");
> +		return -ENOMEM;
> +	}
> +
> +	em_spi = spi_master_get_devdata(master);
> +	platform_set_drvdata(pdev, em_spi);
> +
> +	em_spi->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (!IS_ERR(em_spi->clk))
> +		clk_prepare(em_spi->clk);
> +
> +	em_spi->sclk = devm_clk_get(&pdev->dev, "sclk");
> +	if (!IS_ERR(em_spi->sclk))
> +		clk_prepare(em_spi->sclk);
> +
> +	em_spi->irq = irq;
> +	em_spi->master = master;
> +	em_spi->addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (em_spi->addr == NULL) {
> +		dev_err(&pdev->dev, "ioremap error.\n");
> +		ret = -ENOMEM;
> +		goto error1;
> +	}
> +
> +	n_cs_gpio = of_gpio_named_count(np, "cs-gpios");
> +
> +	if (n_cs_gpio > 0) {
> +		int i;
> +
> +		master->num_chipselect = n_cs_gpio;
> +
> +		em_spi->cs_gpio = devm_kzalloc(&pdev->dev,
> +				sizeof(unsigned int) * n_cs_gpio,
> +				GFP_KERNEL);

Should this memory be freed at some point?

> +
> +		for (i = 0; i < n_cs_gpio; i++) {
> +			em_spi->cs_gpio[i] =
> +				of_get_named_gpio(np, "cs-gpios", i);
> +			devm_gpio_request(&pdev->dev, em_spi->cs_gpio[i], NULL);
> +		}
> +
> +		of_property_read_u32(np, "hard-chipselect",
> +					&em_spi->hard_chip_select);
> +
> +	} else {
> +		of_property_read_u16(np, "num-chipselects",
> +					&master->num_chipselect);
> +	}
> +
> +	init_waitqueue_head(&em_spi->wait);
> +
> +	em_spi_power_on(em_spi);
> +
> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE_SWITCH_EN);
> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE);
> +
> +	/* Reset, int off */
> +	em_spi_soft_reset(em_spi);
> +
> +	ret = devm_request_irq(&pdev->dev, irq, em_spi_irq, 0, "spi-em",
> +				em_spi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not request IRQ.\n");
> +		goto error1;
> +	}
> +
> +	master->dev.parent = &pdev->dev;
> +	master->dev.of_node = pdev->dev.of_node;
> +	master->bus_num = -1;
> +	master->setup = em_spi_setup;
> +	master->transfer_one_message = em_spi_transfer_one_message;
> +	master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH;
> +
> +	ret = spi_register_master(master);
> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not register SPI master.\n");
> +		goto error1;
> +	}
> +
> +	return 0;
> +
> +error1:
> +	spi_master_put(master);
> +
> +	return ret;
> +}
> +
> +static int em_spi_remove(struct platform_device *pdev)
> +{
> +	struct em_spi_data *em_spi = platform_get_drvdata(pdev);
> +
> +	em_spi_power_off(em_spi);
> +	spi_master_put(em_spi->master);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id em_spi_ids[] = {
> +	{ .compatible = "renesas,em-spi", },
> +	{ }
> +};
> +
> +static struct platform_driver em_spi_driver = {
> +	.probe = em_spi_probe,
> +	.remove = em_spi_remove,
> +	.driver = {
> +		.name = "em-spi",
> +		.owner = THIS_MODULE,
> +		.of_match_table = em_spi_ids,
> +	},
> +};
> +
> +module_platform_driver(em_spi_driver);
> +MODULE_DEVICE_TABLE(of, em_spi_ids);
> +
> +MODULE_DESCRIPTION("EMXX SPI bus driver");
> +MODULE_LICENSE("GPLv2");
> +MODULE_AUTHOR("Ian Molton");
> +MODULE_ALIAS("platform:spi-em");
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 16, 2013, 11:21 p.m. UTC | #2 
On Fri, Sep 06, 2013 at 11:16:14AM +0900, Simon Horman wrote:

> I suspect that Magnus is the best person to review this code
> (although he may have other ideas). Nonetheless, I have made
> a few comments below.

You also need to send it to the SPI maintainer (me)...
Mark Brown Sept. 17, 2013, 1:23 p.m. UTC | #3 
On Thu, Sep 05, 2013 at 02:11:09PM +0100, Ian Molton wrote:

> +void em_spi_set_clock(struct em_spi_data *em_spi, unsigned long freq)
> +{
> +	int rate = clk_round_rate(em_spi->sclk, freq);
> +
> +	clk_set_rate(em_spi->sclk, rate);
> +
> +	/* Plenty long enough for clocks to stabilise */
> +	msleep(1);
> +}

The clock API ought to be making sure the clock is ready here.

> +		/*
> +		 * We're abusing the chip a bit so that we can do
> +		 * multi-byte operations in PIO mode.
> +		 * As a result we need to spin until the START bit drops here.
> +		 *
> +		 * This only works with GPIO based chipselects, unless the
> +		 * transfer size is 4 bytes or less. Verified on a scope.
> +		 *
> +		 * DMA mode appears not to have this restriction.
> +		 */
> +
> +		while (readl(em_spi->addr + EMXX_SPI_CTL) & 1)
> +			;

The comment leaves me none the wiser as to what this is actually doing.
As Simon noted some sort of timeout would seem to be in order too.

> +		if (em_spi->status & EMXX_SPI_ERR_STAT)
> +			return -EINVAL;

The driver should really tell people what the error was, there's a
bitmask there.

> +	list_for_each_entry(t, &mesg->transfers, transfer_list) {
> +
> +		if (t->tx_buf && t->rx_buf) {
> +			/* Do not yet support concurrent transfers */
> +			BUG();

WARN_ON() at most.

> +	em_spi_set_cs(em_spi, em_spi->chip_select, 0);

This is ignoring any /CS fiddling options in the transfer.  We really
ought to be factoring this GPIO bashing out of the drivers...

> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;

The core ought to do this for you.

> +	if (spi->bits_per_word != 8)
> +		BUG(); /* We dont support >8 bit transfers */

Again this is too much, you're also not setting bits_per_word_mask.

> +
> +	if (em_spi->cs_gpio)
> +		actual_cs = em_spi->hard_chip_select;
> +	else
> +		actual_cs = spi->chip_select;

This test looks the wrong way round or hard_chip_select seems misnamed?
Or some comments might be in order.

> +static void em_spi_power_on(struct em_spi_data *em_spi)
> +{
> +	clk_enable(em_spi->clk);
> +	clk_enable(em_spi->sclk);
> +}

> +static void em_spi_power_off(struct em_spi_data *em_spi)

These look like they should be runtime PM callbacks.

> +static irqreturn_t em_spi_irq(int irq, void *data)
> +{
> +	struct em_spi_data *em_spi = (struct em_spi_data *)data;
> +	unsigned long status;
> +
> +	status = readl(em_spi->addr + EMXX_SPI_STAT);
> +
> +	if (status) {
> +		em_spi->status = status;
> +		writel(status, em_spi->addr + EMXX_SPI_ENCLR);
> +
> +		wake_up(&em_spi->wait);
> +	}
> +
> +	return IRQ_HANDLED;

So if status isn't set we say the IRQ was handled...  should it not be
returning IRQ_NONE?

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(res == NULL)) {
> +		dev_err(&pdev->dev, "invalid resource\n");
> +		return -EINVAL;
> +	}

> +	em_spi->addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (em_spi->addr == NULL) {
> +		dev_err(&pdev->dev, "ioremap error.\n");
> +		ret = -ENOMEM;
> +		goto error1;
> +	}

devm_ioremap_resource().

> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE_SWITCH_EN);
> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE);

> +	/* Reset, int off */
> +	em_spi_soft_reset(em_spi);

We initialise some registers and then we do a soft reset - that looks
odd?

> +	ret = devm_request_irq(&pdev->dev, irq, em_spi_irq, 0, "spi-em",
> +				em_spi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not request IRQ.\n");

Say what the error was.

> +	ret = spi_register_master(master);

devm_spi_register_master().

> +static struct of_device_id em_spi_ids[] = {
> +	{ .compatible = "renesas,em-spi", },
> +	{ }
> +};

There is no DT binding documentation for this, all new bindings must be
documented.
Ian Molton Sept. 17, 2013, 2:30 p.m. UTC | #4 
On 17/09/13 14:23, Mark Brown wrote:
> On Thu, Sep 05, 2013 at 02:11:09PM +0100, Ian Molton wrote:
>
>> +void em_spi_set_clock(struct em_spi_data *em_spi, unsigned long freq)
>> +{
>> +	int rate = clk_round_rate(em_spi->sclk, freq);
>> +
>> +	clk_set_rate(em_spi->sclk, rate);
>> +
>> +	/* Plenty long enough for clocks to stabilise */
>> +	msleep(1);
>> +}
>
> The clock API ought to be making sure the clock is ready here.

Im not sure the clock API should be taking into account quirks of the 
SPI block. the incomming clock is, I'm sure, fine. The block takes time 
to sync.

>> +		/*
>> +		 * We're abusing the chip a bit so that we can do
>> +		 * multi-byte operations in PIO mode.
>> +		 * As a result we need to spin until the START bit drops here.
>> +		 *
>> +		 * This only works with GPIO based chipselects, unless the
>> +		 * transfer size is 4 bytes or less. Verified on a scope.
>> +		 *
>> +		 * DMA mode appears not to have this restriction.
>> +		 */
>> +
>> +		while (readl(em_spi->addr + EMXX_SPI_CTL) & 1)
>> +			;
>
> The comment leaves me none the wiser as to what this is actually doing.
> As Simon noted some sort of timeout would seem to be in order too.

Hm, how much more is appropriate to comment within the driver?

What we're doing, is setting a GPIO CS just before the transfer and 
resetting it after.

using the dedicated CS lines hits a chip limit of 4 bytes. If you ignore 
it and keep sending data, the chip behaves correctly, except that 
automatic start/stop doesnt work properly. (it may not work properly, 
period, but I have no way to test).

>> +		if (em_spi->status & EMXX_SPI_ERR_STAT)
>> +			return -EINVAL;
>
> The driver should really tell people what the error was, there's a
> bitmask there.

Fair, will fix.

>> +	list_for_each_entry(t, &mesg->transfers, transfer_list) {
>> +
>> +		if (t->tx_buf && t->rx_buf) {
>> +			/* Do not yet support concurrent transfers */
>> +			BUG();
>
> WARN_ON() at most.

Already fixed locally.

>> +	em_spi_set_cs(em_spi, em_spi->chip_select, 0);
>
> This is ignoring any /CS fiddling options in the transfer.  We really
> ought to be factoring this GPIO bashing out of the drivers...

Perhaps, but how?

>> +	if (!spi->bits_per_word)
>> +		spi->bits_per_word = 8;
>
> The core ought to do this for you.

Paranoia ported forward... will nuke.

>> +	if (spi->bits_per_word != 8)
>> +		BUG(); /* We dont support >8 bit transfers */
>
> Again this is too much, you're also not setting bits_per_word_mask.

Will fix.

>> +
>> +	if (em_spi->cs_gpio)
>> +		actual_cs = em_spi->hard_chip_select;
>> +	else
>> +		actual_cs = spi->chip_select;
>
> This test looks the wrong way round or hard_chip_select seems misnamed?
> Or some comments might be in order.

No, its correct. When using GPIO lines, even if the real CS lines arent 
used, you still must program them properly, or the chip does not respect 
polarity/phase options.

hard_chip_select is used to choose which "real" chipselect is being used 
to back the GPIO based CS.

the idea is that chip_select is always used to refer to the CS as seen 
from the attached devices perspective. when not using GPIOs, it is == 
hard_chip_select.

perhaps s/hard/underlying/ ?
or s/hard/real/ ?

>> +static void em_spi_power_on(struct em_spi_data *em_spi)
>> +{
>> +	clk_enable(em_spi->clk);
>> +	clk_enable(em_spi->sclk);
>> +}
>
>> +static void em_spi_power_off(struct em_spi_data *em_spi)
>
> These look like they should be runtime PM callbacks.

Deleted locally as I cant test this. Im not sure what happened to the 
latest patch, it appears not to have hit the mailinglist.

>> +static irqreturn_t em_spi_irq(int irq, void *data)
>> +{
>> +	struct em_spi_data *em_spi = (struct em_spi_data *)data;
>> +	unsigned long status;
>> +
>> +	status = readl(em_spi->addr + EMXX_SPI_STAT);
>> +
>> +	if (status) {
>> +		em_spi->status = status;
>> +		writel(status, em_spi->addr + EMXX_SPI_ENCLR);
>> +
>> +		wake_up(&em_spi->wait);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>
> So if status isn't set we say the IRQ was handled...  should it not be
> returning IRQ_NONE?

Probably.

>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (unlikely(res == NULL)) {
>> +		dev_err(&pdev->dev, "invalid resource\n");
>> +		return -EINVAL;
>> +	}
>
>> +	em_spi->addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +	if (em_spi->addr == NULL) {
>> +		dev_err(&pdev->dev, "ioremap error.\n");
>> +		ret = -ENOMEM;
>> +		goto error1;
>> +	}
>
> devm_ioremap_resource().

Nice. will change.

>> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE_SWITCH_EN);
>> +	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE);
>
>> +	/* Reset, int off */
>> +	em_spi_soft_reset(em_spi);
>
> We initialise some registers and then we do a soft reset - that looks
> odd?

But correct. Those registers control function of the block as either SPI 
or IIS.

>> +	ret = devm_request_irq(&pdev->dev, irq, em_spi_irq, 0, "spi-em",
>> +				em_spi);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Could not request IRQ.\n");
>
> Say what the error was.

OK.

>> +	ret = spi_register_master(master);
>
> devm_spi_register_master().

OK.

>> +static struct of_device_id em_spi_ids[] = {
>> +	{ .compatible = "renesas,em-spi", },
>> +	{ }
>> +};
>
> There is no DT binding documentation for this, all new bindings must be
> documented.

No prob.

I'll fix this lot up and resubmit. Thanks for the review.

-Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 17, 2013, 4:41 p.m. UTC | #5 
On Tue, Sep 17, 2013 at 03:30:48PM +0100, Ian Molton wrote:
> On 17/09/13 14:23, Mark Brown wrote:

> >>+		/*
> >>+		 * We're abusing the chip a bit so that we can do
> >>+		 * multi-byte operations in PIO mode.
> >>+		 * As a result we need to spin until the START bit drops here.
> >>+		 *
> >>+		 * This only works with GPIO based chipselects, unless the
> >>+		 * transfer size is 4 bytes or less. Verified on a scope.
> >>+		 *
> >>+		 * DMA mode appears not to have this restriction.
> >>+		 */

> >>+		while (readl(em_spi->addr + EMXX_SPI_CTL) & 1)
> >>+			;

> >The comment leaves me none the wiser as to what this is actually doing.
> >As Simon noted some sort of timeout would seem to be in order too.

> Hm, how much more is appropriate to comment within the driver?

> What we're doing, is setting a GPIO CS just before the transfer and
> resetting it after.

Expanding the "this" would be a big help here, there's a lot of text
there but it doesn't say anything about what the operation being done
is.

> >>+	em_spi_set_cs(em_spi, em_spi->chip_select, 0);

> >This is ignoring any /CS fiddling options in the transfer.  We really
> >ought to be factoring this GPIO bashing out of the drivers...

> Perhaps, but how?

Implementing the /CS fiddling options is fairly straightforward.  For
the factoring out I'm mostly waiting for the Samsung guys to respin the
patch series they have pending for their driver since that is my primary
development platform and I don't want to collide with that work.

Most transfer_one_message() functions are very similar, especially those
where the /CS is done with a GPIO - the iteration and /CS fiddling is
all common, it's just the data transfer that varies.

> >>+	if (em_spi->cs_gpio)
> >>+		actual_cs = em_spi->hard_chip_select;
> >>+	else
> >>+		actual_cs = spi->chip_select;

> >This test looks the wrong way round or hard_chip_select seems misnamed?
> >Or some comments might be in order.

> No, its correct. When using GPIO lines, even if the real CS lines
> arent used, you still must program them properly, or the chip does
> not respect polarity/phase options.

> hard_chip_select is used to choose which "real" chipselect is being
> used to back the GPIO based CS.

> the idea is that chip_select is always used to refer to the CS as
> seen from the attached devices perspective. when not using GPIOs, it
> is == hard_chip_select.

> perhaps s/hard/underlying/ ?
> or s/hard/real/ ?

Something explaining that you still need to control the controller /CS
even without it being connected to the device would be a start.

> >>+	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE_SWITCH_EN);
> >>+	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE);

> >>+	/* Reset, int off */
> >>+	em_spi_soft_reset(em_spi);

> >We initialise some registers and then we do a soft reset - that looks
> >odd?

> But correct. Those registers control function of the block as either
> SPI or IIS.

Again a comment would be in order - one would expect that resetting the
block would reset the selection.
diff mbox

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 89cbbab..4a648c0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -164,6 +164,14 @@  config SPI_EP93XX
 	  This enables using the Cirrus EP93xx SPI controller in master
 	  mode.
 
+config SPI_EM
+        tristate "EMXX built in SPI controller"
+        depends on ARCH_EMEV2
+        help
+          This enables the emxx SPI driver in master mode. Only short transfers
+	  are supported without extra chipselect logic in PIO mode. DMA is not
+	  currently supported.
+
 config SPI_FALCON
 	tristate "Falcon SPI controller support"
 	depends on SOC_FALCON
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 33f9c09..41c7a23 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -28,6 +28,7 @@  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
 obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
 spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
 obj-$(CONFIG_SPI_EP93XX)		+= spi-ep93xx.o
+obj-$(CONFIG_SPI_EM)			+= spi-em.o
 obj-$(CONFIG_SPI_FALCON)		+= spi-falcon.o
 obj-$(CONFIG_SPI_FSL_CPM)		+= spi-fsl-cpm.o
 obj-$(CONFIG_SPI_FSL_LIB)		+= spi-fsl-lib.o
diff --git a/drivers/spi/spi-em.c b/drivers/spi/spi-em.c
new file mode 100644
index 0000000..2c3e980
--- /dev/null
+++ b/drivers/spi/spi-em.c
@@ -0,0 +1,486 @@ 
+/*
+ * EMXX SPI Master driver
+ * Based on spi_sh.
+ *
+ * Copyright (C) 2013 Codethink Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/errno.h>
+#include <linux/clk.h>
+#include <linux/timer.h>
+#include <linux/delay.h>
+#include <linux/list.h>
+#include <linux/workqueue.h>
+#include <linux/interrupt.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/of_gpio.h>
+#include <linux/io.h>
+#include <linux/spi/spi.h>
+
+#include <linux/gpio.h>
+
+struct em_spi_data {
+	void __iomem *addr;
+	int irq;
+	struct clk *clk;
+	struct clk *sclk;
+	struct spi_master *master;
+	wait_queue_head_t wait;
+	unsigned int status;
+	unsigned int chip_select;
+	unsigned int hard_chip_select;
+	unsigned int pol_mask;
+	unsigned int pol_data;
+	unsigned int mode;
+	unsigned int *cs_gpio;
+};
+
+#define EMXX_SPI_BLOCK_MODE_SWITCH_EN	0x0000
+#define EMXX_SPI_BLOCK_MODE		0x0004
+
+#define EMXX_SPI_MODE			0x1000
+#define EMXX_SPI_POL			0x1004
+#define EMXX_SPI_CTL			0x1008
+#define EMXX_SPI_TX			0x1010
+#define EMXX_SPI_RX			0x1014
+#define EMXX_SPI_STAT			0x1018
+#define EMXX_SPI_RAWSTAT		0x101c
+#define EMXX_SPI_ENSET			0x1020
+#define EMXX_SPI_ENCLR			0x1024
+#define EMXX_SPI_FFCLR			0x1028
+
+#define EMXX_SPI_TX_STOP		(1 << 6)
+#define EMXX_SPI_RX_STOP		(1 << 5)
+#define EMXX_SPI_TERR			(1 << 4)
+#define EMXX_SPI_RDV			(1 << 3)
+#define EMXX_SPI_END			(1 << 2)
+#define EMXX_SPI_TX_UDR			(1 << 1)
+#define EMXX_SPI_RX_OVR			(1 << 0)
+
+#define EMXX_SPI_ERR_STAT	(EMXX_SPI_RX_OVR | EMXX_SPI_TX_UDR | \
+					EMXX_SPI_TERR)
+
+#define EMXX_SPI_ALL_IRQ	(EMXX_SPI_TERR | EMXX_SPI_RDV | EMXX_SPI_END | \
+					EMXX_SPI_TX_UDR | EMXX_SPI_RX_OVR)
+#define EMXX_SPI_RX_IRQS	(EMXX_SPI_RX_OVR | EMXX_SPI_RDV)
+#define EMXX_SPI_TX_IRQS	(EMXX_SPI_TERR | EMXX_SPI_END | EMXX_SPI_TX_UDR)
+
+#define EMXX_SPI_FLAG_START			(1 << 0)
+#define EMXX_SPI_FLAG_RX		(1 << 2)
+#define EMXX_SPI_FLAG_TX		(1 << 3)
+
+static void em_spi_set_cs(struct em_spi_data *em_spi, int cs, int state)
+{
+	gpio_direction_output(em_spi->cs_gpio[cs], state);
+}
+
+/*
+ * Soft reset. Set bit 8 of the control register.
+ * The chip will reset the interrupt status register, so no need to
+ * disable IRQs.
+ *
+ */
+static void em_spi_soft_reset(struct em_spi_data *em_spi)
+{
+	writel(1 << 8, em_spi->addr + EMXX_SPI_CTL);
+	msleep(1);
+	writel(0, em_spi->addr + EMXX_SPI_CTL);
+	msleep(1);
+}
+
+void em_spi_set_clock(struct em_spi_data *em_spi, unsigned long freq)
+{
+	int rate = clk_round_rate(em_spi->sclk, freq);
+
+	clk_set_rate(em_spi->sclk, rate);
+
+	/* Plenty long enough for clocks to stabilise */
+	msleep(1);
+}
+
+static void em_spi_setup_transfer(struct em_spi_data *em_spi)
+{
+	unsigned long pol_reg;
+
+	pol_reg = readl(em_spi->addr + EMXX_SPI_POL);
+	pol_reg &= em_spi->pol_mask;
+	pol_reg |= em_spi->pol_data;
+	writel(pol_reg, em_spi->addr + EMXX_SPI_POL);
+
+	writel(em_spi->mode, em_spi->addr + EMXX_SPI_MODE);
+
+}
+
+static int em_spi_start_transfer(struct em_spi_data *em_spi,
+				unsigned int flags)
+{
+	unsigned int irqs = 0;
+
+	if (flags == EMXX_SPI_FLAG_RX)
+		irqs |= EMXX_SPI_RX_IRQS;
+
+	if (flags == EMXX_SPI_FLAG_TX)
+		irqs |= EMXX_SPI_TX_IRQS;
+
+	/* Enable IRQs */
+	em_spi->status = 0;
+	writel(irqs, em_spi->addr + EMXX_SPI_ENSET);
+
+	/* Kick off the transfer */
+	writel(flags | EMXX_SPI_FLAG_START, em_spi->addr + EMXX_SPI_CTL);
+
+	return 0;
+}
+
+static int em_spi_tx(struct em_spi_data *em_spi, struct spi_transfer *t)
+{
+	const char *tx_buf = t->tx_buf;
+	int i, ret;
+
+	for (i = 0; i < t->len; i++) {
+
+		writel(tx_buf[i], em_spi->addr + EMXX_SPI_TX);
+
+		em_spi_start_transfer(em_spi, EMXX_SPI_FLAG_TX);
+
+		ret = wait_event_interruptible_timeout(em_spi->wait,
+			em_spi->status & EMXX_SPI_TX_IRQS, HZ);
+
+		/*
+		 * We're abusing the chip a bit so that we can do
+		 * multi-byte operations in PIO mode.
+		 * As a result we need to spin until the START bit drops here.
+		 *
+		 * This only works with GPIO based chipselects, unless the
+		 * transfer size is 4 bytes or less. Verified on a scope.
+		 *
+		 * DMA mode appears not to have this restriction.
+		 */
+
+		while (readl(em_spi->addr + EMXX_SPI_CTL) & 1)
+			;
+
+		if (ret < 0)
+			return -ETIMEDOUT;
+
+		if (em_spi->status & EMXX_SPI_ERR_STAT)
+			return -EINVAL;
+
+		writel(EMXX_SPI_END, em_spi->addr + EMXX_SPI_FFCLR);
+	}
+
+	return 0;
+}
+
+static int em_spi_rx(struct em_spi_data *em_spi, struct spi_transfer *t)
+{
+	char *rx_buf = t->rx_buf;
+	int i, ret;
+
+	for (i = 0; i < t->len; i++) {
+		em_spi_start_transfer(em_spi, EMXX_SPI_FLAG_RX);
+
+		/* We might want to try reading more than one word per irq */
+		ret = wait_event_interruptible_timeout(em_spi->wait,
+			em_spi->status & EMXX_SPI_RX_IRQS, HZ);
+
+		if (ret < 0)
+			return -ETIMEDOUT;
+
+		if (em_spi->status & EMXX_SPI_ERR_STAT)
+			return -EINVAL;
+
+		rx_buf[i] = readl(em_spi->addr + EMXX_SPI_RX);
+
+		writel(EMXX_SPI_RDV, em_spi->addr + EMXX_SPI_FFCLR);
+	}
+
+	return 0;
+}
+
+static int em_spi_setup(struct spi_device *spi);
+
+static int em_spi_transfer_one_message(struct spi_master *master,
+					struct spi_message *mesg)
+{
+	struct em_spi_data *em_spi = spi_master_get_devdata(master);
+	struct spi_transfer *t;
+	int ret;
+
+	em_spi_setup(mesg->spi);
+
+	em_spi_set_cs(em_spi, em_spi->chip_select, 1);
+
+	em_spi_setup_transfer(em_spi);
+
+	list_for_each_entry(t, &mesg->transfers, transfer_list) {
+
+		if (t->tx_buf && t->rx_buf) {
+			/* Do not yet support concurrent transfers */
+			BUG();
+			goto error;
+		}
+
+		if (t->speed_hz)
+			em_spi_set_clock(em_spi, t->speed_hz);
+
+		if (t->tx_buf)
+			ret = em_spi_tx(em_spi, t);
+		else if (t->rx_buf)
+			ret = em_spi_rx(em_spi, t);
+
+		if (ret)
+			goto error;
+
+		mesg->actual_length += t->len;
+	}
+
+	em_spi_set_cs(em_spi, em_spi->chip_select, 0);
+
+	mesg->status = 0;
+	spi_finalize_current_message(master);
+
+	return 0;
+
+error:
+	dev_err(&master->dev, "SPI status = %08x, ret = %d\n",
+		em_spi->status, ret);
+
+	em_spi_soft_reset(em_spi);
+
+	mesg->status = ret;
+	spi_finalize_current_message(master);
+
+	return ret;
+}
+
+static int em_spi_setup(struct spi_device *spi)
+{
+	struct em_spi_data *em_spi = spi_master_get_devdata(spi->master);
+	unsigned long pol = 0, mode = 0, shift, actual_cs;
+
+	if (!spi->bits_per_word)
+		spi->bits_per_word = 8;
+
+	if (spi->bits_per_word != 8)
+		BUG(); /* We dont support >8 bit transfers */
+
+	if (spi->max_speed_hz)
+		em_spi_set_clock(em_spi, spi->max_speed_hz);
+
+	/* Select clock and cs polarity */
+	pol |= !(spi->mode & SPI_CPHA)   ? (1 << 2) : 0;
+	pol |= (spi->mode & SPI_CPOL)    ? (1 << 1) : 0;
+	pol |= (spi->mode & SPI_CS_HIGH) ? (1 << 0) : 0;
+
+	if (em_spi->cs_gpio)
+		actual_cs = em_spi->hard_chip_select;
+	else
+		actual_cs = spi->chip_select;
+
+	shift = actual_cs * 3;
+
+	/* Take into account gap in reg for CSW */
+	if (shift > 9)
+		shift += 4;
+
+	mode |= ((spi->bits_per_word - 1) & 0x1f) << 8;
+	mode |= (actual_cs & 0x7) << 4;
+
+	em_spi->pol_mask = ~((0x7 << shift) | (0xf << 12));
+	em_spi->pol_data = pol << shift;
+	em_spi->mode = mode;
+	em_spi->chip_select = spi->chip_select;
+
+	return 0;
+}
+
+static void em_spi_power_on(struct em_spi_data *em_spi)
+{
+	clk_enable(em_spi->clk);
+	clk_enable(em_spi->sclk);
+}
+
+static void em_spi_power_off(struct em_spi_data *em_spi)
+{
+	clk_disable(em_spi->sclk);
+	clk_disable(em_spi->clk);
+}
+
+static irqreturn_t em_spi_irq(int irq, void *data)
+{
+	struct em_spi_data *em_spi = (struct em_spi_data *)data;
+	unsigned long status;
+
+	status = readl(em_spi->addr + EMXX_SPI_STAT);
+
+	if (status) {
+		em_spi->status = status;
+		writel(status, em_spi->addr + EMXX_SPI_ENCLR);
+
+		wake_up(&em_spi->wait);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int em_spi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *res;
+	struct spi_master *master;
+	struct em_spi_data *em_spi;
+	int ret, irq, n_cs_gpio;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (unlikely(res == NULL)) {
+		dev_err(&pdev->dev, "invalid resource\n");
+		return -EINVAL;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "platform_get_irq error\n");
+		return -ENODEV;
+	}
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct em_spi_data));
+	if (master == NULL) {
+		dev_err(&pdev->dev, "spi_alloc_master error.\n");
+		return -ENOMEM;
+	}
+
+	em_spi = spi_master_get_devdata(master);
+	platform_set_drvdata(pdev, em_spi);
+
+	em_spi->clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(em_spi->clk))
+		clk_prepare(em_spi->clk);
+
+	em_spi->sclk = devm_clk_get(&pdev->dev, "sclk");
+	if (!IS_ERR(em_spi->sclk))
+		clk_prepare(em_spi->sclk);
+
+	em_spi->irq = irq;
+	em_spi->master = master;
+	em_spi->addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (em_spi->addr == NULL) {
+		dev_err(&pdev->dev, "ioremap error.\n");
+		ret = -ENOMEM;
+		goto error1;
+	}
+
+	n_cs_gpio = of_gpio_named_count(np, "cs-gpios");
+
+	if (n_cs_gpio > 0) {
+		int i;
+
+		master->num_chipselect = n_cs_gpio;
+
+		em_spi->cs_gpio = devm_kzalloc(&pdev->dev,
+				sizeof(unsigned int) * n_cs_gpio,
+				GFP_KERNEL);
+
+		for (i = 0; i < n_cs_gpio; i++) {
+			em_spi->cs_gpio[i] =
+				of_get_named_gpio(np, "cs-gpios", i);
+			devm_gpio_request(&pdev->dev, em_spi->cs_gpio[i], NULL);
+		}
+
+		of_property_read_u32(np, "hard-chipselect",
+					&em_spi->hard_chip_select);
+
+	} else {
+		of_property_read_u16(np, "num-chipselects",
+					&master->num_chipselect);
+	}
+
+	init_waitqueue_head(&em_spi->wait);
+
+	em_spi_power_on(em_spi);
+
+	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE_SWITCH_EN);
+	writel(0x1, em_spi->addr + EMXX_SPI_BLOCK_MODE);
+
+	/* Reset, int off */
+	em_spi_soft_reset(em_spi);
+
+	ret = devm_request_irq(&pdev->dev, irq, em_spi_irq, 0, "spi-em",
+				em_spi);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not request IRQ.\n");
+		goto error1;
+	}
+
+	master->dev.parent = &pdev->dev;
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = -1;
+	master->setup = em_spi_setup;
+	master->transfer_one_message = em_spi_transfer_one_message;
+	master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH;
+
+	ret = spi_register_master(master);
+
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register SPI master.\n");
+		goto error1;
+	}
+
+	return 0;
+
+error1:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int em_spi_remove(struct platform_device *pdev)
+{
+	struct em_spi_data *em_spi = platform_get_drvdata(pdev);
+
+	em_spi_power_off(em_spi);
+	spi_master_put(em_spi->master);
+
+	return 0;
+}
+
+static struct of_device_id em_spi_ids[] = {
+	{ .compatible = "renesas,em-spi", },
+	{ }
+};
+
+static struct platform_driver em_spi_driver = {
+	.probe = em_spi_probe,
+	.remove = em_spi_remove,
+	.driver = {
+		.name = "em-spi",
+		.owner = THIS_MODULE,
+		.of_match_table = em_spi_ids,
+	},
+};
+
+module_platform_driver(em_spi_driver);
+MODULE_DEVICE_TABLE(of, em_spi_ids);
+
+MODULE_DESCRIPTION("EMXX SPI bus driver");
+MODULE_LICENSE("GPLv2");
+MODULE_AUTHOR("Ian Molton");
+MODULE_ALIAS("platform:spi-em");