* [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 2:55 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-18 2:55 UTC (permalink / raw)
To: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel; +Cc: jonsmirl
From: Grant Likely <grant.likely@secretlab.ca>
Adds support for the dedicated SPI device on the Freescale MPC5200(b)
SoC.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
Hi David,
It's been a while since I've posted this, but I believe I've addressed
all the outstanding comments against v3, and people are asking me for it.
The pre-message stuff is all gone and the error handling should be
better now.
BTW, the premessage stuff was to handle a platform I had where some of
the CS lines were controlled with a separate SPI transaction to the
same SPI controller. It almost looks like an SPI bridge. It requires
more thought before I try again.
Being a new driver, it would be nice to get it into 2.6.31, but
definitely not critical.
g.
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1
drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
include/linux/spi/mpc52xx_spi.h | 10 +
4 files changed, 539 insertions(+), 0 deletions(-)
create mode 100644 drivers/spi/mpc52xx_spi.c
create mode 100644 include/linux/spi/mpc52xx_spi.h
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83a185d..1994bcd 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -132,6 +132,14 @@ config SPI_LM70_LLP
which interfaces to an LM70 temperature sensor using
a parallel port.
+config SPI_MPC52xx
+ tristate "Freescale MPC52xx SPI (non-PSC) controller support"
+ depends on PPC_MPC52xx && SPI
+ select SPI_MASTER_OF
+ help
+ This drivers supports the MPC52xx SPI controller in master SPI
+ mode.
+
config SPI_MPC52xx_PSC
tristate "Freescale MPC52xx PSC SPI controller"
depends on PPC_MPC52xx && EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5d04519..8de32c7 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
obj-$(CONFIG_SPI_ORION) += orion_spi.o
obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
+obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
new file mode 100644
index 0000000..ef8379b
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,520 @@
+/*
+ * MPC52xx SPI bus driver.
+ *
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This file is released under the GPLv2
+ *
+ * This is the driver for the MPC5200's dedicated SPI controller.
+ *
+ * Note: this driver does not support the MPC5200 PSC in SPI mode. For
+ * that driver see drivers/spi/mpc52xx_psc_spi.c
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/of_platform.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mpc52xx_spi.h>
+#include <linux/of_spi.h>
+#include <linux/io.h>
+#include <asm/time.h>
+#include <asm/mpc52xx.h>
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
+MODULE_LICENSE("GPL");
+
+/* Register offsets */
+#define SPI_CTRL1 0x00
+#define SPI_CTRL1_SPIE (1 << 7)
+#define SPI_CTRL1_SPE (1 << 6)
+#define SPI_CTRL1_MSTR (1 << 4)
+#define SPI_CTRL1_CPOL (1 << 3)
+#define SPI_CTRL1_CPHA (1 << 2)
+#define SPI_CTRL1_SSOE (1 << 1)
+#define SPI_CTRL1_LSBFE (1 << 0)
+
+#define SPI_CTRL2 0x01
+#define SPI_BRR 0x04
+
+#define SPI_STATUS 0x05
+#define SPI_STATUS_SPIF (1 << 7)
+#define SPI_STATUS_WCOL (1 << 6)
+#define SPI_STATUS_MODF (1 << 4)
+
+#define SPI_DATA 0x09
+#define SPI_PORTDATA 0x0d
+#define SPI_DATADIR 0x10
+
+/* FSM state return values */
+#define FSM_STOP 0 /* Nothing more for the state machine to */
+ /* do. If something interesting happens */
+ /* then and IRQ will be received */
+#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
+ /* not expected */
+#define FSM_CONTINUE 2 /* Keep iterating the state machine */
+
+/* Driver internal data */
+struct mpc52xx_spi {
+ struct spi_master *master;
+ u32 sysclk;
+ void __iomem *regs;
+ int irq0; /* MODF irq */
+ int irq1; /* SPIF irq */
+ int ipb_freq;
+
+ /* Statistics */
+ int msg_count;
+ int wcol_count;
+ int wcol_ticks;
+ u32 wcol_tx_timestamp;
+ int modf_count;
+ int byte_count;
+
+ struct list_head queue; /* queue of pending messages */
+ spinlock_t lock;
+ struct work_struct work;
+
+
+ /* Details of current transfer (length, and buffer pointers) */
+ struct spi_message *message; /* current message */
+ struct spi_transfer *transfer; /* current transfer */
+ int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
+ int len;
+ int timestamp;
+ u8 *rx_buf;
+ const u8 *tx_buf;
+ int cs_change;
+};
+
+/*
+ * CS control function
+ */
+static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
+{
+ out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
+}
+
+/*
+ * Start a new transfer. This is called both by the idle state
+ * for the first transfer in a message, and by the wait state when the
+ * previous transfer in a message is complete.
+ */
+static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
+{
+ ms->rx_buf = ms->transfer->rx_buf;
+ ms->tx_buf = ms->transfer->tx_buf;
+ ms->len = ms->transfer->len;
+
+ /* Activate the chip select */
+ if (ms->cs_change)
+ mpc52xx_spi_chipsel(ms, 1);
+ ms->cs_change = ms->transfer->cs_change;
+
+ /* Write out the first byte */
+ ms->wcol_tx_timestamp = get_tbl();
+ if (ms->tx_buf)
+ out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+ else
+ out_8(ms->regs + SPI_DATA, 0);
+}
+
+/* Forward declaration of state handlers */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data);
+static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data);
+
+/*
+ * IDLE state
+ *
+ * No transfers are in progress; if another transfer is pending then retrieve
+ * it and kick it off. Otherwise, stop processing the state machine
+ */
+static int
+mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+ struct spi_device *spi;
+ int spr, sppr;
+ u8 ctrl1;
+
+ if (status && (irq != NO_IRQ))
+ dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+ status);
+
+ /* Check if there is another transfer waiting. */
+ if (list_empty(&ms->queue))
+ return FSM_STOP;
+
+ /* get the head of the queue */
+ ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
+ list_del_init(&ms->message->queue);
+
+ /* Setup the controller parameters */
+ ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
+ spi = ms->message->spi;
+ if (spi->mode & SPI_CPHA)
+ ctrl1 |= SPI_CTRL1_CPHA;
+ if (spi->mode & SPI_CPOL)
+ ctrl1 |= SPI_CTRL1_CPOL;
+ if (spi->mode & SPI_LSB_FIRST)
+ ctrl1 |= SPI_CTRL1_LSBFE;
+ out_8(ms->regs + SPI_CTRL1, ctrl1);
+
+ /* Setup the controller speed */
+ /* minimum divider is '2'. Also, add '1' to force rounding the
+ * divider up. */
+ sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
+ spr = 0;
+ if (sppr < 1)
+ sppr = 1;
+ while (((sppr - 1) & ~0x7) != 0) {
+ sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
+ spr++;
+ }
+ sppr--; /* sppr quantity in register is offset by 1 */
+ if (spr > 7) {
+ /* Don't overrun limits of SPI baudrate register */
+ spr = 7;
+ sppr = 7;
+ }
+ out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
+
+ ms->cs_change = 1;
+ ms->transfer = container_of(ms->message->transfers.next,
+ struct spi_transfer, transfer_list);
+
+ mpc52xx_spi_start_transfer(ms);
+ ms->state = mpc52xx_spi_fsmstate_transfer;
+
+ return FSM_CONTINUE;
+}
+
+/*
+ * TRANSFER state
+ *
+ * In the middle of a transfer. If the SPI core has completed processing
+ * a byte, then read out the received data and write out the next byte
+ * (unless this transfer is finished; in which case go on to the wait
+ * state)
+ */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data)
+{
+ if (!status)
+ return ms->irq0 ? FSM_STOP : FSM_POLL;
+
+ if (status & SPI_STATUS_WCOL) {
+ /* The SPI controller is stoopid. At slower speeds, it may
+ * raise the SPIF flag before the state machine is actually
+ * finished, which causes a collision (internal to the state
+ * machine only). The manual recommends inserting a delay
+ * between receiving the interrupt and sending the next byte,
+ * but it can also be worked around simply by retrying the
+ * transfer which is what we do here. */
+ ms->wcol_count++;
+ ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
+ ms->wcol_tx_timestamp = get_tbl();
+ data = 0;
+ if (ms->tx_buf)
+ data = *(ms->tx_buf-1);
+ out_8(ms->regs + SPI_DATA, data); /* try again */
+ return FSM_CONTINUE;
+ } else if (status & SPI_STATUS_MODF) {
+ ms->modf_count++;
+ dev_err(&ms->master->dev, "mode fault\n");
+ mpc52xx_spi_chipsel(ms, 0);
+ ms->message->status = -EIO;
+ ms->message->complete(ms->message->context);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ return FSM_CONTINUE;
+ }
+
+ /* Read data out of the spi device */
+ ms->byte_count++;
+ if (ms->rx_buf)
+ *ms->rx_buf++ = data;
+
+ /* Is the transfer complete? */
+ ms->len--;
+ if (ms->len == 0) {
+ ms->timestamp = get_tbl();
+ ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
+ ms->state = mpc52xx_spi_fsmstate_wait;
+ return FSM_CONTINUE;
+ }
+
+ /* Write out the next byte */
+ ms->wcol_tx_timestamp = get_tbl();
+ if (ms->tx_buf)
+ out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+ else
+ out_8(ms->regs + SPI_DATA, 0);
+
+ return FSM_CONTINUE;
+}
+
+/*
+ * WAIT state
+ *
+ * A transfer has completed; need to wait for the delay period to complete
+ * before starting the next transfer
+ */
+static int
+mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+ if (status && irq)
+ dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+ status);
+
+ if (((int)get_tbl()) - ms->timestamp < 0)
+ return FSM_POLL;
+
+ ms->message->actual_length += ms->transfer->len;
+
+ /* Check if there is another transfer in this message. If there
+ * aren't then deactivate CS, notify sender, and drop back to idle
+ * to start the next message. */
+ if (ms->transfer->transfer_list.next == &ms->message->transfers) {
+ ms->msg_count++;
+ mpc52xx_spi_chipsel(ms, 0);
+ ms->message->status = 0;
+ ms->message->complete(ms->message->context);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ return FSM_CONTINUE;
+ }
+
+ /* There is another transfer; kick it off */
+
+ if (ms->cs_change)
+ mpc52xx_spi_chipsel(ms, 0);
+
+ ms->transfer = container_of(ms->transfer->transfer_list.next,
+ struct spi_transfer, transfer_list);
+ mpc52xx_spi_start_transfer(ms);
+ ms->state = mpc52xx_spi_fsmstate_transfer;
+ return FSM_CONTINUE;
+}
+
+/**
+ * mpc52xx_spi_fsm_process - Finite State Machine iteration function
+ * @irq: irq number that triggered the FSM or 0 for polling
+ * @ms: pointer to mpc52xx_spi driver data
+ */
+static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
+{
+ int rc = FSM_CONTINUE;
+ u8 status, data;
+
+ while (rc == FSM_CONTINUE) {
+ /* Interrupt cleared by read of STATUS followed by
+ * read of DATA registers */
+ status = in_8(ms->regs + SPI_STATUS);
+ data = in_8(ms->regs + SPI_DATA);
+ rc = ms->state(irq, ms, status, data);
+ }
+
+ if (rc == FSM_POLL)
+ schedule_work(&ms->work);
+}
+
+/**
+ * mpc52xx_spi_irq - IRQ handler
+ */
+static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
+{
+ struct mpc52xx_spi *ms = _ms;
+ spin_lock(&ms->lock);
+ mpc52xx_spi_fsm_process(irq, ms);
+ spin_unlock(&ms->lock);
+ return IRQ_HANDLED;
+}
+
+/**
+ * mpc52xx_spi_wq - Workqueue function for polling the state machine
+ */
+static void mpc52xx_spi_wq(struct work_struct *work)
+{
+ struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ms->lock, flags);
+ mpc52xx_spi_fsm_process(0, ms);
+ spin_unlock_irqrestore(&ms->lock, flags);
+}
+
+/*
+ * spi_master ops
+ */
+
+static int mpc52xx_spi_setup(struct spi_device *spi)
+{
+ if (spi->bits_per_word % 8)
+ return -EINVAL;
+
+ if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
+ return -EINVAL;
+
+ if (spi->chip_select >= spi->master->num_chipselect)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+ struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
+ unsigned long flags;
+
+ m->actual_length = 0;
+ m->status = -EINPROGRESS;
+
+ spin_lock_irqsave(&ms->lock, flags);
+ list_add_tail(&m->queue, &ms->queue);
+ spin_unlock_irqrestore(&ms->lock, flags);
+ schedule_work(&ms->work);
+
+ return 0;
+}
+
+/*
+ * OF Platform Bus Binding
+ */
+static int __devinit mpc52xx_spi_probe(struct of_device *op,
+ const struct of_device_id *match)
+{
+ struct spi_master *master;
+ struct mpc52xx_spi *ms;
+ void __iomem *regs;
+ int rc;
+
+ /* MMIO registers */
+ dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
+ regs = of_iomap(op->node, 0);
+ if (!regs)
+ return -ENODEV;
+
+ /* initialize the device */
+ out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
+ out_8(regs + SPI_CTRL2, 0x0);
+ out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
+ out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
+
+ /* Clear the status register and re-read it to check for a MODF
+ * failure. This driver cannot currently handle multiple masters
+ * on the SPI bus. This fault will also occur if the SPI signals
+ * are not connected to any pins (port_config setting) */
+ in_8(regs + SPI_STATUS);
+ in_8(regs + SPI_DATA);
+ if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
+ dev_err(&op->dev, "mode fault; is port_config correct?\n");
+ rc = -EIO;
+ goto err_init;
+ }
+
+ dev_dbg(&op->dev, "allocating spi_master struct\n");
+ master = spi_alloc_master(&op->dev, sizeof *ms);
+ if (!master) {
+ rc = -ENOMEM;
+ goto err_alloc;
+ }
+ master->bus_num = -1;
+ master->num_chipselect = 1;
+ master->setup = mpc52xx_spi_setup;
+ master->transfer = mpc52xx_spi_transfer;
+ dev_set_drvdata(&op->dev, master);
+
+ ms = spi_master_get_devdata(master);
+ ms->master = master;
+ ms->regs = regs;
+ ms->irq0 = irq_of_parse_and_map(op->node, 0);
+ ms->irq1 = irq_of_parse_and_map(op->node, 1);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
+ spin_lock_init(&ms->lock);
+ INIT_LIST_HEAD(&ms->queue);
+ INIT_WORK(&ms->work, mpc52xx_spi_wq);
+
+ /* Decide if interrupts can be used */
+ if (ms->irq0 && ms->irq1) {
+ rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+ "mpc5200-spi-modf", ms);
+ rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+ "mpc5200-spi-spiF", ms);
+ if (rc) {
+ free_irq(ms->irq0, ms);
+ free_irq(ms->irq1, ms);
+ ms->irq0 = ms->irq1 = 0;
+ }
+ } else {
+ /* operate in polled mode */
+ ms->irq0 = ms->irq1 = 0;
+ }
+
+ if (!ms->irq0)
+ dev_info(&op->dev, "using polled mode\n");
+
+ dev_dbg(&op->dev, "registering spi_master struct\n");
+ rc = spi_register_master(master);
+ if (rc)
+ goto err_register;
+
+ of_register_spi_devices(master, op->node);
+ dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
+
+ return rc;
+
+ err_register:
+ dev_err(&ms->master->dev, "initialization failed\n");
+ spi_master_put(master);
+ err_alloc:
+ err_init:
+ iounmap(regs);
+ return rc;
+}
+
+static int __devexit mpc52xx_spi_remove(struct of_device *op)
+{
+ struct spi_master *master = dev_get_drvdata(&op->dev);
+ struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+
+ free_irq(ms->irq0, ms);
+ free_irq(ms->irq1, ms);
+
+ spi_unregister_master(master);
+ spi_master_put(master);
+ iounmap(ms->regs);
+
+ return 0;
+}
+
+static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
+ { .compatible = "fsl,mpc5200-spi", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
+
+static struct of_platform_driver mpc52xx_spi_of_driver = {
+ .owner = THIS_MODULE,
+ .name = "mpc52xx-spi",
+ .match_table = mpc52xx_spi_match,
+ .probe = mpc52xx_spi_probe,
+ .remove = __exit_p(mpc52xx_spi_remove),
+};
+
+static int __init mpc52xx_spi_init(void)
+{
+ return of_register_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_init(mpc52xx_spi_init);
+
+static void __exit mpc52xx_spi_exit(void)
+{
+ of_unregister_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_exit(mpc52xx_spi_exit);
+
diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
new file mode 100644
index 0000000..d1004cf
--- /dev/null
+++ b/include/linux/spi/mpc52xx_spi.h
@@ -0,0 +1,10 @@
+
+#ifndef INCLUDE_MPC5200_SPI_H
+#define INCLUDE_MPC5200_SPI_H
+
+extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
+ void (*hook)(struct spi_message *m,
+ void *context),
+ void *hook_context);
+
+#endif
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 2:55 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-18 2:55 UTC (permalink / raw)
To: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel
From: Grant Likely <grant.likely@secretlab.ca>
Adds support for the dedicated SPI device on the Freescale MPC5200(b)
SoC.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
Hi David,
It's been a while since I've posted this, but I believe I've addressed
all the outstanding comments against v3, and people are asking me for it.
The pre-message stuff is all gone and the error handling should be
better now.
BTW, the premessage stuff was to handle a platform I had where some of
the CS lines were controlled with a separate SPI transaction to the
same SPI controller. It almost looks like an SPI bridge. It requires
more thought before I try again.
Being a new driver, it would be nice to get it into 2.6.31, but
definitely not critical.
g.
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1
drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
include/linux/spi/mpc52xx_spi.h | 10 +
4 files changed, 539 insertions(+), 0 deletions(-)
create mode 100644 drivers/spi/mpc52xx_spi.c
create mode 100644 include/linux/spi/mpc52xx_spi.h
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83a185d..1994bcd 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -132,6 +132,14 @@ config SPI_LM70_LLP
which interfaces to an LM70 temperature sensor using
a parallel port.
+config SPI_MPC52xx
+ tristate "Freescale MPC52xx SPI (non-PSC) controller support"
+ depends on PPC_MPC52xx && SPI
+ select SPI_MASTER_OF
+ help
+ This drivers supports the MPC52xx SPI controller in master SPI
+ mode.
+
config SPI_MPC52xx_PSC
tristate "Freescale MPC52xx PSC SPI controller"
depends on PPC_MPC52xx && EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5d04519..8de32c7 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
obj-$(CONFIG_SPI_ORION) += orion_spi.o
obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
+obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
new file mode 100644
index 0000000..ef8379b
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,520 @@
+/*
+ * MPC52xx SPI bus driver.
+ *
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This file is released under the GPLv2
+ *
+ * This is the driver for the MPC5200's dedicated SPI controller.
+ *
+ * Note: this driver does not support the MPC5200 PSC in SPI mode. For
+ * that driver see drivers/spi/mpc52xx_psc_spi.c
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/of_platform.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mpc52xx_spi.h>
+#include <linux/of_spi.h>
+#include <linux/io.h>
+#include <asm/time.h>
+#include <asm/mpc52xx.h>
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
+MODULE_LICENSE("GPL");
+
+/* Register offsets */
+#define SPI_CTRL1 0x00
+#define SPI_CTRL1_SPIE (1 << 7)
+#define SPI_CTRL1_SPE (1 << 6)
+#define SPI_CTRL1_MSTR (1 << 4)
+#define SPI_CTRL1_CPOL (1 << 3)
+#define SPI_CTRL1_CPHA (1 << 2)
+#define SPI_CTRL1_SSOE (1 << 1)
+#define SPI_CTRL1_LSBFE (1 << 0)
+
+#define SPI_CTRL2 0x01
+#define SPI_BRR 0x04
+
+#define SPI_STATUS 0x05
+#define SPI_STATUS_SPIF (1 << 7)
+#define SPI_STATUS_WCOL (1 << 6)
+#define SPI_STATUS_MODF (1 << 4)
+
+#define SPI_DATA 0x09
+#define SPI_PORTDATA 0x0d
+#define SPI_DATADIR 0x10
+
+/* FSM state return values */
+#define FSM_STOP 0 /* Nothing more for the state machine to */
+ /* do. If something interesting happens */
+ /* then and IRQ will be received */
+#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
+ /* not expected */
+#define FSM_CONTINUE 2 /* Keep iterating the state machine */
+
+/* Driver internal data */
+struct mpc52xx_spi {
+ struct spi_master *master;
+ u32 sysclk;
+ void __iomem *regs;
+ int irq0; /* MODF irq */
+ int irq1; /* SPIF irq */
+ int ipb_freq;
+
+ /* Statistics */
+ int msg_count;
+ int wcol_count;
+ int wcol_ticks;
+ u32 wcol_tx_timestamp;
+ int modf_count;
+ int byte_count;
+
+ struct list_head queue; /* queue of pending messages */
+ spinlock_t lock;
+ struct work_struct work;
+
+
+ /* Details of current transfer (length, and buffer pointers) */
+ struct spi_message *message; /* current message */
+ struct spi_transfer *transfer; /* current transfer */
+ int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
+ int len;
+ int timestamp;
+ u8 *rx_buf;
+ const u8 *tx_buf;
+ int cs_change;
+};
+
+/*
+ * CS control function
+ */
+static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
+{
+ out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
+}
+
+/*
+ * Start a new transfer. This is called both by the idle state
+ * for the first transfer in a message, and by the wait state when the
+ * previous transfer in a message is complete.
+ */
+static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
+{
+ ms->rx_buf = ms->transfer->rx_buf;
+ ms->tx_buf = ms->transfer->tx_buf;
+ ms->len = ms->transfer->len;
+
+ /* Activate the chip select */
+ if (ms->cs_change)
+ mpc52xx_spi_chipsel(ms, 1);
+ ms->cs_change = ms->transfer->cs_change;
+
+ /* Write out the first byte */
+ ms->wcol_tx_timestamp = get_tbl();
+ if (ms->tx_buf)
+ out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+ else
+ out_8(ms->regs + SPI_DATA, 0);
+}
+
+/* Forward declaration of state handlers */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data);
+static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data);
+
+/*
+ * IDLE state
+ *
+ * No transfers are in progress; if another transfer is pending then retrieve
+ * it and kick it off. Otherwise, stop processing the state machine
+ */
+static int
+mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+ struct spi_device *spi;
+ int spr, sppr;
+ u8 ctrl1;
+
+ if (status && (irq != NO_IRQ))
+ dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+ status);
+
+ /* Check if there is another transfer waiting. */
+ if (list_empty(&ms->queue))
+ return FSM_STOP;
+
+ /* get the head of the queue */
+ ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
+ list_del_init(&ms->message->queue);
+
+ /* Setup the controller parameters */
+ ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
+ spi = ms->message->spi;
+ if (spi->mode & SPI_CPHA)
+ ctrl1 |= SPI_CTRL1_CPHA;
+ if (spi->mode & SPI_CPOL)
+ ctrl1 |= SPI_CTRL1_CPOL;
+ if (spi->mode & SPI_LSB_FIRST)
+ ctrl1 |= SPI_CTRL1_LSBFE;
+ out_8(ms->regs + SPI_CTRL1, ctrl1);
+
+ /* Setup the controller speed */
+ /* minimum divider is '2'. Also, add '1' to force rounding the
+ * divider up. */
+ sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
+ spr = 0;
+ if (sppr < 1)
+ sppr = 1;
+ while (((sppr - 1) & ~0x7) != 0) {
+ sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
+ spr++;
+ }
+ sppr--; /* sppr quantity in register is offset by 1 */
+ if (spr > 7) {
+ /* Don't overrun limits of SPI baudrate register */
+ spr = 7;
+ sppr = 7;
+ }
+ out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
+
+ ms->cs_change = 1;
+ ms->transfer = container_of(ms->message->transfers.next,
+ struct spi_transfer, transfer_list);
+
+ mpc52xx_spi_start_transfer(ms);
+ ms->state = mpc52xx_spi_fsmstate_transfer;
+
+ return FSM_CONTINUE;
+}
+
+/*
+ * TRANSFER state
+ *
+ * In the middle of a transfer. If the SPI core has completed processing
+ * a byte, then read out the received data and write out the next byte
+ * (unless this transfer is finished; in which case go on to the wait
+ * state)
+ */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+ u8 status, u8 data)
+{
+ if (!status)
+ return ms->irq0 ? FSM_STOP : FSM_POLL;
+
+ if (status & SPI_STATUS_WCOL) {
+ /* The SPI controller is stoopid. At slower speeds, it may
+ * raise the SPIF flag before the state machine is actually
+ * finished, which causes a collision (internal to the state
+ * machine only). The manual recommends inserting a delay
+ * between receiving the interrupt and sending the next byte,
+ * but it can also be worked around simply by retrying the
+ * transfer which is what we do here. */
+ ms->wcol_count++;
+ ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
+ ms->wcol_tx_timestamp = get_tbl();
+ data = 0;
+ if (ms->tx_buf)
+ data = *(ms->tx_buf-1);
+ out_8(ms->regs + SPI_DATA, data); /* try again */
+ return FSM_CONTINUE;
+ } else if (status & SPI_STATUS_MODF) {
+ ms->modf_count++;
+ dev_err(&ms->master->dev, "mode fault\n");
+ mpc52xx_spi_chipsel(ms, 0);
+ ms->message->status = -EIO;
+ ms->message->complete(ms->message->context);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ return FSM_CONTINUE;
+ }
+
+ /* Read data out of the spi device */
+ ms->byte_count++;
+ if (ms->rx_buf)
+ *ms->rx_buf++ = data;
+
+ /* Is the transfer complete? */
+ ms->len--;
+ if (ms->len == 0) {
+ ms->timestamp = get_tbl();
+ ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
+ ms->state = mpc52xx_spi_fsmstate_wait;
+ return FSM_CONTINUE;
+ }
+
+ /* Write out the next byte */
+ ms->wcol_tx_timestamp = get_tbl();
+ if (ms->tx_buf)
+ out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+ else
+ out_8(ms->regs + SPI_DATA, 0);
+
+ return FSM_CONTINUE;
+}
+
+/*
+ * WAIT state
+ *
+ * A transfer has completed; need to wait for the delay period to complete
+ * before starting the next transfer
+ */
+static int
+mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+ if (status && irq)
+ dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+ status);
+
+ if (((int)get_tbl()) - ms->timestamp < 0)
+ return FSM_POLL;
+
+ ms->message->actual_length += ms->transfer->len;
+
+ /* Check if there is another transfer in this message. If there
+ * aren't then deactivate CS, notify sender, and drop back to idle
+ * to start the next message. */
+ if (ms->transfer->transfer_list.next == &ms->message->transfers) {
+ ms->msg_count++;
+ mpc52xx_spi_chipsel(ms, 0);
+ ms->message->status = 0;
+ ms->message->complete(ms->message->context);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ return FSM_CONTINUE;
+ }
+
+ /* There is another transfer; kick it off */
+
+ if (ms->cs_change)
+ mpc52xx_spi_chipsel(ms, 0);
+
+ ms->transfer = container_of(ms->transfer->transfer_list.next,
+ struct spi_transfer, transfer_list);
+ mpc52xx_spi_start_transfer(ms);
+ ms->state = mpc52xx_spi_fsmstate_transfer;
+ return FSM_CONTINUE;
+}
+
+/**
+ * mpc52xx_spi_fsm_process - Finite State Machine iteration function
+ * @irq: irq number that triggered the FSM or 0 for polling
+ * @ms: pointer to mpc52xx_spi driver data
+ */
+static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
+{
+ int rc = FSM_CONTINUE;
+ u8 status, data;
+
+ while (rc == FSM_CONTINUE) {
+ /* Interrupt cleared by read of STATUS followed by
+ * read of DATA registers */
+ status = in_8(ms->regs + SPI_STATUS);
+ data = in_8(ms->regs + SPI_DATA);
+ rc = ms->state(irq, ms, status, data);
+ }
+
+ if (rc == FSM_POLL)
+ schedule_work(&ms->work);
+}
+
+/**
+ * mpc52xx_spi_irq - IRQ handler
+ */
+static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
+{
+ struct mpc52xx_spi *ms = _ms;
+ spin_lock(&ms->lock);
+ mpc52xx_spi_fsm_process(irq, ms);
+ spin_unlock(&ms->lock);
+ return IRQ_HANDLED;
+}
+
+/**
+ * mpc52xx_spi_wq - Workqueue function for polling the state machine
+ */
+static void mpc52xx_spi_wq(struct work_struct *work)
+{
+ struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ms->lock, flags);
+ mpc52xx_spi_fsm_process(0, ms);
+ spin_unlock_irqrestore(&ms->lock, flags);
+}
+
+/*
+ * spi_master ops
+ */
+
+static int mpc52xx_spi_setup(struct spi_device *spi)
+{
+ if (spi->bits_per_word % 8)
+ return -EINVAL;
+
+ if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
+ return -EINVAL;
+
+ if (spi->chip_select >= spi->master->num_chipselect)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+ struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
+ unsigned long flags;
+
+ m->actual_length = 0;
+ m->status = -EINPROGRESS;
+
+ spin_lock_irqsave(&ms->lock, flags);
+ list_add_tail(&m->queue, &ms->queue);
+ spin_unlock_irqrestore(&ms->lock, flags);
+ schedule_work(&ms->work);
+
+ return 0;
+}
+
+/*
+ * OF Platform Bus Binding
+ */
+static int __devinit mpc52xx_spi_probe(struct of_device *op,
+ const struct of_device_id *match)
+{
+ struct spi_master *master;
+ struct mpc52xx_spi *ms;
+ void __iomem *regs;
+ int rc;
+
+ /* MMIO registers */
+ dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
+ regs = of_iomap(op->node, 0);
+ if (!regs)
+ return -ENODEV;
+
+ /* initialize the device */
+ out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
+ out_8(regs + SPI_CTRL2, 0x0);
+ out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
+ out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
+
+ /* Clear the status register and re-read it to check for a MODF
+ * failure. This driver cannot currently handle multiple masters
+ * on the SPI bus. This fault will also occur if the SPI signals
+ * are not connected to any pins (port_config setting) */
+ in_8(regs + SPI_STATUS);
+ in_8(regs + SPI_DATA);
+ if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
+ dev_err(&op->dev, "mode fault; is port_config correct?\n");
+ rc = -EIO;
+ goto err_init;
+ }
+
+ dev_dbg(&op->dev, "allocating spi_master struct\n");
+ master = spi_alloc_master(&op->dev, sizeof *ms);
+ if (!master) {
+ rc = -ENOMEM;
+ goto err_alloc;
+ }
+ master->bus_num = -1;
+ master->num_chipselect = 1;
+ master->setup = mpc52xx_spi_setup;
+ master->transfer = mpc52xx_spi_transfer;
+ dev_set_drvdata(&op->dev, master);
+
+ ms = spi_master_get_devdata(master);
+ ms->master = master;
+ ms->regs = regs;
+ ms->irq0 = irq_of_parse_and_map(op->node, 0);
+ ms->irq1 = irq_of_parse_and_map(op->node, 1);
+ ms->state = mpc52xx_spi_fsmstate_idle;
+ ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
+ spin_lock_init(&ms->lock);
+ INIT_LIST_HEAD(&ms->queue);
+ INIT_WORK(&ms->work, mpc52xx_spi_wq);
+
+ /* Decide if interrupts can be used */
+ if (ms->irq0 && ms->irq1) {
+ rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+ "mpc5200-spi-modf", ms);
+ rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+ "mpc5200-spi-spiF", ms);
+ if (rc) {
+ free_irq(ms->irq0, ms);
+ free_irq(ms->irq1, ms);
+ ms->irq0 = ms->irq1 = 0;
+ }
+ } else {
+ /* operate in polled mode */
+ ms->irq0 = ms->irq1 = 0;
+ }
+
+ if (!ms->irq0)
+ dev_info(&op->dev, "using polled mode\n");
+
+ dev_dbg(&op->dev, "registering spi_master struct\n");
+ rc = spi_register_master(master);
+ if (rc)
+ goto err_register;
+
+ of_register_spi_devices(master, op->node);
+ dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
+
+ return rc;
+
+ err_register:
+ dev_err(&ms->master->dev, "initialization failed\n");
+ spi_master_put(master);
+ err_alloc:
+ err_init:
+ iounmap(regs);
+ return rc;
+}
+
+static int __devexit mpc52xx_spi_remove(struct of_device *op)
+{
+ struct spi_master *master = dev_get_drvdata(&op->dev);
+ struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+
+ free_irq(ms->irq0, ms);
+ free_irq(ms->irq1, ms);
+
+ spi_unregister_master(master);
+ spi_master_put(master);
+ iounmap(ms->regs);
+
+ return 0;
+}
+
+static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
+ { .compatible = "fsl,mpc5200-spi", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
+
+static struct of_platform_driver mpc52xx_spi_of_driver = {
+ .owner = THIS_MODULE,
+ .name = "mpc52xx-spi",
+ .match_table = mpc52xx_spi_match,
+ .probe = mpc52xx_spi_probe,
+ .remove = __exit_p(mpc52xx_spi_remove),
+};
+
+static int __init mpc52xx_spi_init(void)
+{
+ return of_register_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_init(mpc52xx_spi_init);
+
+static void __exit mpc52xx_spi_exit(void)
+{
+ of_unregister_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_exit(mpc52xx_spi_exit);
+
diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
new file mode 100644
index 0000000..d1004cf
--- /dev/null
+++ b/include/linux/spi/mpc52xx_spi.h
@@ -0,0 +1,10 @@
+
+#ifndef INCLUDE_MPC5200_SPI_H
+#define INCLUDE_MPC5200_SPI_H
+
+extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
+ void (*hook)(struct spi_message *m,
+ void *context),
+ void *hook_context);
+
+#endif
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 2:55 ` Grant Likely
(?)
@ 2009-06-18 6:58 ` Wolfram Sang
-1 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 6:58 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl
[-- Attachment #1: Type: text/plain, Size: 20824 bytes --]
Hi Grant,
some comments below:
(by the way, have you tested this patch on hardware? I wonder because of the
SSOE-issue, but maybe it works despite the documentation.)
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Hi David,
>
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
>
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller. It almost looks like an SPI bridge. It requires
> more thought before I try again.
>
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
>
> g.
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 539 insertions(+), 0 deletions(-)
> create mode 100644 drivers/spi/mpc52xx_spi.c
> create mode 100644 include/linux/spi/mpc52xx_spi.h
>
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_ORION) += orion_spi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode. For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
Is this still needed? See last comment...
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
Really asm?
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0 /* Nothing more for the state machine to */
> + /* do. If something interesting happens */
> + /* then and IRQ will be received */
s/and/an/? Throughout the comments, there is sometimes a double space.
> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
> + /* not expected */
> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
unused?
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
unsigned int will suit it better IMHO.
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
them will be ugly. Well, I can't make up if this is just overhead or useful
debugging aid, so no real objection :)
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
Magic value.
But I wonder more about the usage of the SS pin and if this chipsel is needed
at all (sadly I cannot test as I don't have any board with SPI connected to
that device). You define the SS-pin as output, but do not set the SSOE-bit.
More, you use the MODF-feature, so the SS-pin should be defined as input?
According to Table 17.3 in the PM, you have that pin defined as generic purpose
output.
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting. */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
> +
> + /* get the head of the queue */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> + list_del_init(&ms->message->queue);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding the
> + * divider up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
> + }
> + out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI controller is stoopid. At slower speeds, it may
> + * raise the SPIF flag before the state machine is actually
> + * finished, which causes a collision (internal to the state
> + * machine only). The manual recommends inserting a delay
> + * between receiving the interrupt and sending the next byte,
> + * but it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
spaces around operator.
> + out_8(ms->regs + SPI_DATA, data); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mode fault\n");
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers */
> + status = in_8(ms->regs + SPI_STATUS);
> + data = in_8(ms->regs + SPI_DATA);
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + mpc52xx_spi_fsm_process(0, ms);
> + spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + if (spi->bits_per_word % 8)
> + return -EINVAL;
> +
> + if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> + return -EINVAL;
> +
> + if (spi->chip_select >= spi->master->num_chipselect)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
> +
> + return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + int rc;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
spaces around operator.
> + out_8(regs + SPI_CTRL2, 0x0);
> + out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
> + out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + in_8(regs + SPI_STATUS);
> + in_8(regs + SPI_DATA);
> + if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + rc = -EIO;
> + goto err_init;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + /* Decide if interrupts can be used */
> + if (ms->irq0 && ms->irq1) {
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
Capital 'F' wanted?
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = 0;
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = 0;
> + }
> +
> + if (!ms->irq0)
> + dev_info(&op->dev, "using polled mode\n");
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc)
> + goto err_register;
> +
> + of_register_spi_devices(master, op->node);
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + err_alloc:
> + err_init:
> + iounmap(regs);
> + return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +
> + return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_match,
> + .probe = mpc52xx_spi_probe,
> + .remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
This can be dropped for now and reintroduced when needed, I think.
>
>
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 6:58 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 6:58 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1.1: Type: text/plain, Size: 20824 bytes --]
Hi Grant,
some comments below:
(by the way, have you tested this patch on hardware? I wonder because of the
SSOE-issue, but maybe it works despite the documentation.)
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Hi David,
>
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
>
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller. It almost looks like an SPI bridge. It requires
> more thought before I try again.
>
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
>
> g.
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 539 insertions(+), 0 deletions(-)
> create mode 100644 drivers/spi/mpc52xx_spi.c
> create mode 100644 include/linux/spi/mpc52xx_spi.h
>
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_ORION) += orion_spi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode. For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
Is this still needed? See last comment...
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
Really asm?
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0 /* Nothing more for the state machine to */
> + /* do. If something interesting happens */
> + /* then and IRQ will be received */
s/and/an/? Throughout the comments, there is sometimes a double space.
> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
> + /* not expected */
> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
unused?
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
unsigned int will suit it better IMHO.
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
them will be ugly. Well, I can't make up if this is just overhead or useful
debugging aid, so no real objection :)
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
Magic value.
But I wonder more about the usage of the SS pin and if this chipsel is needed
at all (sadly I cannot test as I don't have any board with SPI connected to
that device). You define the SS-pin as output, but do not set the SSOE-bit.
More, you use the MODF-feature, so the SS-pin should be defined as input?
According to Table 17.3 in the PM, you have that pin defined as generic purpose
output.
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting. */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
> +
> + /* get the head of the queue */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> + list_del_init(&ms->message->queue);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding the
> + * divider up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
> + }
> + out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI controller is stoopid. At slower speeds, it may
> + * raise the SPIF flag before the state machine is actually
> + * finished, which causes a collision (internal to the state
> + * machine only). The manual recommends inserting a delay
> + * between receiving the interrupt and sending the next byte,
> + * but it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
spaces around operator.
> + out_8(ms->regs + SPI_DATA, data); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mode fault\n");
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers */
> + status = in_8(ms->regs + SPI_STATUS);
> + data = in_8(ms->regs + SPI_DATA);
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + mpc52xx_spi_fsm_process(0, ms);
> + spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + if (spi->bits_per_word % 8)
> + return -EINVAL;
> +
> + if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> + return -EINVAL;
> +
> + if (spi->chip_select >= spi->master->num_chipselect)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
> +
> + return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + int rc;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
spaces around operator.
> + out_8(regs + SPI_CTRL2, 0x0);
> + out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
> + out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + in_8(regs + SPI_STATUS);
> + in_8(regs + SPI_DATA);
> + if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + rc = -EIO;
> + goto err_init;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + /* Decide if interrupts can be used */
> + if (ms->irq0 && ms->irq1) {
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
Capital 'F' wanted?
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = 0;
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = 0;
> + }
> +
> + if (!ms->irq0)
> + dev_info(&op->dev, "using polled mode\n");
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc)
> + goto err_register;
> +
> + of_register_spi_devices(master, op->node);
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + err_alloc:
> + err_init:
> + iounmap(regs);
> + return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +
> + return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_match,
> + .probe = mpc52xx_spi_probe,
> + .remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
This can be dropped for now and reintroduced when needed, I think.
>
>
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 6:58 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 6:58 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1: Type: text/plain, Size: 20824 bytes --]
Hi Grant,
some comments below:
(by the way, have you tested this patch on hardware? I wonder because of the
SSOE-issue, but maybe it works despite the documentation.)
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Hi David,
>
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
>
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller. It almost looks like an SPI bridge. It requires
> more thought before I try again.
>
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
>
> g.
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 539 insertions(+), 0 deletions(-)
> create mode 100644 drivers/spi/mpc52xx_spi.c
> create mode 100644 include/linux/spi/mpc52xx_spi.h
>
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_ORION) += orion_spi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode. For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
Is this still needed? See last comment...
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
Really asm?
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0 /* Nothing more for the state machine to */
> + /* do. If something interesting happens */
> + /* then and IRQ will be received */
s/and/an/? Throughout the comments, there is sometimes a double space.
> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
> + /* not expected */
> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
unused?
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
unsigned int will suit it better IMHO.
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
them will be ugly. Well, I can't make up if this is just overhead or useful
debugging aid, so no real objection :)
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
Magic value.
But I wonder more about the usage of the SS pin and if this chipsel is needed
at all (sadly I cannot test as I don't have any board with SPI connected to
that device). You define the SS-pin as output, but do not set the SSOE-bit.
More, you use the MODF-feature, so the SS-pin should be defined as input?
According to Table 17.3 in the PM, you have that pin defined as generic purpose
output.
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting. */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
> +
> + /* get the head of the queue */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> + list_del_init(&ms->message->queue);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding the
> + * divider up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
> + }
> + out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI controller is stoopid. At slower speeds, it may
> + * raise the SPIF flag before the state machine is actually
> + * finished, which causes a collision (internal to the state
> + * machine only). The manual recommends inserting a delay
> + * between receiving the interrupt and sending the next byte,
> + * but it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
spaces around operator.
> + out_8(ms->regs + SPI_DATA, data); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mode fault\n");
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers */
> + status = in_8(ms->regs + SPI_STATUS);
> + data = in_8(ms->regs + SPI_DATA);
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + mpc52xx_spi_fsm_process(0, ms);
> + spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + if (spi->bits_per_word % 8)
> + return -EINVAL;
> +
> + if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> + return -EINVAL;
> +
> + if (spi->chip_select >= spi->master->num_chipselect)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
> +
> + return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + int rc;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
spaces around operator.
> + out_8(regs + SPI_CTRL2, 0x0);
> + out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
> + out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + in_8(regs + SPI_STATUS);
> + in_8(regs + SPI_DATA);
> + if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + rc = -EIO;
> + goto err_init;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + /* Decide if interrupts can be used */
> + if (ms->irq0 && ms->irq1) {
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
Capital 'F' wanted?
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = 0;
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = 0;
> + }
> +
> + if (!ms->irq0)
> + dev_info(&op->dev, "using polled mode\n");
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc)
> + goto err_register;
> +
> + of_register_spi_devices(master, op->node);
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + err_alloc:
> + err_init:
> + iounmap(regs);
> + return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +
> + return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_match,
> + .probe = mpc52xx_spi_probe,
> + .remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
This can be dropped for now and reintroduced when needed, I think.
>
>
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 6:58 ` Wolfram Sang
(?)
@ 2009-06-18 13:35 ` Grant Likely
-1 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-18 13:35 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl
On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> some comments below:
Hi Wolfram. Thanks for the review.
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...
No, not needed at all. Will remove.
>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?
Yes, really asm. Needed for get_tlb()
>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1 0x00
>> +#define SPI_CTRL1_SPIE (1 << 7)
>> +#define SPI_CTRL1_SPE (1 << 6)
>> +#define SPI_CTRL1_MSTR (1 << 4)
>> +#define SPI_CTRL1_CPOL (1 << 3)
>> +#define SPI_CTRL1_CPHA (1 << 2)
>> +#define SPI_CTRL1_SSOE (1 << 1)
>> +#define SPI_CTRL1_LSBFE (1 << 0)
>> +
>> +#define SPI_CTRL2 0x01
>> +#define SPI_BRR 0x04
>> +
>> +#define SPI_STATUS 0x05
>> +#define SPI_STATUS_SPIF (1 << 7)
>> +#define SPI_STATUS_WCOL (1 << 6)
>> +#define SPI_STATUS_MODF (1 << 4)
>> +
>> +#define SPI_DATA 0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR 0x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP 0 /* Nothing more for the state machine to */
>> + /* do. If something interesting happens */
>> + /* then and IRQ will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.
The double spaces at the end of sentences are intentional. It is
perhaps a bit quaint and old fashioned, but it is my writing style.
>
>> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
>> + /* not expected */
>> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> + struct spi_master *master;
>> + u32 sysclk;
>
> unused?
yes, fixed.
>> + void __iomem *regs;
>> + int irq0; /* MODF irq */
>> + int irq1; /* SPIF irq */
>> + int ipb_freq;
>
> unsigned int will suit it better IMHO.
right.
>> +
>> + /* Statistics */
>> + int msg_count;
>> + int wcol_count;
>> + int wcol_ticks;
>> + u32 wcol_tx_timestamp;
>> + int modf_count;
>> + int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> them will be ugly. Well, I can't make up if this is just overhead or useful
> debugging aid, so no real objection :)
There used to be a sysfs interface for dumping these, but it was an
ugly misuse. I'd like to leave these in. I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.
> But I wonder more about the usage of the SS pin and if this chipsel is needed
> at all (sadly I cannot test as I don't have any board with SPI connected to
> that device). You define the SS-pin as output, but do not set the SSOE-bit.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic purpose
> output.
That's right. The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.
>> + /* initialize the device */
>> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
>
> spaces around operator.
Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators. I think it is easier to
read that way.
>> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> + void (*hook)(struct spi_message *m,
>> + void *context),
>> + void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.
Yeah, this is cruft. Removed.
Thanks!
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 13:35 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-18 13:35 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> some comments below:
Hi Wolfram. Thanks for the review.
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...
No, not needed at all. Will remove.
>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?
Yes, really asm. Needed for get_tlb()
>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1 0x00
>> +#define SPI_CTRL1_SPIE (1 << 7)
>> +#define SPI_CTRL1_SPE (1 << 6)
>> +#define SPI_CTRL1_MSTR (1 << 4)
>> +#define SPI_CTRL1_CPOL (1 << 3)
>> +#define SPI_CTRL1_CPHA (1 << 2)
>> +#define SPI_CTRL1_SSOE (1 << 1)
>> +#define SPI_CTRL1_LSBFE (1 << 0)
>> +
>> +#define SPI_CTRL2 0x01
>> +#define SPI_BRR 0x04
>> +
>> +#define SPI_STATUS 0x05
>> +#define SPI_STATUS_SPIF (1 << 7)
>> +#define SPI_STATUS_WCOL (1 << 6)
>> +#define SPI_STATUS_MODF (1 << 4)
>> +
>> +#define SPI_DATA 0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR 0x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP 0 /* Nothing more for the state machine to */
>> + /* do. If something interesting happens */
>> + /* then and IRQ will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.
The double spaces at the end of sentences are intentional. It is
perhaps a bit quaint and old fashioned, but it is my writing style.
>
>> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
>> + /* not expected */
>> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> + struct spi_master *master;
>> + u32 sysclk;
>
> unused?
yes, fixed.
>> + void __iomem *regs;
>> + int irq0; /* MODF irq */
>> + int irq1; /* SPIF irq */
>> + int ipb_freq;
>
> unsigned int will suit it better IMHO.
right.
>> +
>> + /* Statistics */
>> + int msg_count;
>> + int wcol_count;
>> + int wcol_ticks;
>> + u32 wcol_tx_timestamp;
>> + int modf_count;
>> + int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> them will be ugly. Well, I can't make up if this is just overhead or useful
> debugging aid, so no real objection :)
There used to be a sysfs interface for dumping these, but it was an
ugly misuse. I'd like to leave these in. I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.
> But I wonder more about the usage of the SS pin and if this chipsel is needed
> at all (sadly I cannot test as I don't have any board with SPI connected to
> that device). You define the SS-pin as output, but do not set the SSOE-bit.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic purpose
> output.
That's right. The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.
>> + /* initialize the device */
>> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
>
> spaces around operator.
Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators. I think it is easier to
read that way.
>> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> + void (*hook)(struct spi_message *m,
>> + void *context),
>> + void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.
Yeah, this is cruft. Removed.
Thanks!
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 13:35 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-18 13:35 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.sang@pengutronix.de> wrote=
:
> Hi Grant,
>
> some comments below:
Hi Wolfram. Thanks for the review.
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...
No, not needed at all. Will remove.
>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?
Yes, really asm. Needed for get_tlb()
>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1 =A0 =A00x00
>> +#define SPI_CTRL1_SPIE =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 7)
>> +#define SPI_CTRL1_SPE =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 6)
>> +#define SPI_CTRL1_MSTR =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 4)
>> +#define SPI_CTRL1_CPOL =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 3)
>> +#define SPI_CTRL1_CPHA =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 2)
>> +#define SPI_CTRL1_SSOE =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 1)
>> +#define SPI_CTRL1_LSBFE =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 0)
>> +
>> +#define SPI_CTRL2 =A0 =A00x01
>> +#define SPI_BRR =A0 =A0 =A0 =A0 =A0 =A0 =A00x04
>> +
>> +#define SPI_STATUS =A0 0x05
>> +#define SPI_STATUS_SPIF =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 7)
>> +#define SPI_STATUS_WCOL =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 6)
>> +#define SPI_STATUS_MODF =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 4)
>> +
>> +#define SPI_DATA =A0 =A0 0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR =A00x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP =A0 =A0 0 =A0 =A0 =A0 /* Nothing more for the state ma=
chine to */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* do. =A0If s=
omething interesting happens */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* then and IR=
Q will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.
The double spaces at the end of sentences are intentional. It is
perhaps a bit quaint and old fashioned, but it is my writing style.
>
>> +#define FSM_POLL =A0 =A0 1 =A0 =A0 =A0 /* need to poll for completion, =
an IRQ is */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* not expecte=
d */
>> +#define FSM_CONTINUE 2 =A0 =A0 =A0 /* Keep iterating the state machine =
*/
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> + =A0 =A0 struct spi_master *master;
>> + =A0 =A0 u32 sysclk;
>
> unused?
yes, fixed.
>> + =A0 =A0 void __iomem *regs;
>> + =A0 =A0 int irq0; =A0 =A0 =A0 /* MODF irq */
>> + =A0 =A0 int irq1; =A0 =A0 =A0 /* SPIF irq */
>> + =A0 =A0 int ipb_freq;
>
> unsigned int will suit it better IMHO.
right.
>> +
>> + =A0 =A0 /* Statistics */
>> + =A0 =A0 int msg_count;
>> + =A0 =A0 int wcol_count;
>> + =A0 =A0 int wcol_ticks;
>> + =A0 =A0 u32 wcol_tx_timestamp;
>> + =A0 =A0 int modf_count;
>> + =A0 =A0 int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG ar=
ound
> them will be ugly. Well, I can't make up if this is just overhead or usef=
ul
> debugging aid, so no real objection :)
There used to be a sysfs interface for dumping these, but it was an
ugly misuse. I'd like to leave these in. I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.
> But I wonder more about the usage of the SS pin and if this chipsel is ne=
eded
> at all (sadly I cannot test as I don't have any board with SPI connected =
to
> that device). You define the SS-pin as output, but do not set the SSOE-bi=
t.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic p=
urpose
> output.
That's right. The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.
>> + =A0 =A0 /* initialize the device */
>> + =A0 =A0 out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTR=
L1_MSTR);
>
> spaces around operator.
Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators. I think it is easier to
read that way.
>> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx=
_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 void (*hook)(struct spi_message *m,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void *context),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.
Yeah, this is cruft. Removed.
Thanks!
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 13:35 ` Grant Likely
(?)
@ 2009-06-18 14:26 ` Wolfram Sang
-1 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 14:26 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl
[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]
Hi Grant,
> The double spaces at the end of sentences are intentional. It is
> perhaps a bit quaint and old fashioned, but it is my writing style.
Ah, okay.
> >> +
> >> + /* Statistics */
> >> + int msg_count;
> >> + int wcol_count;
> >> + int wcol_ticks;
> >> + u32 wcol_tx_timestamp;
> >> + int modf_count;
> >> + int byte_count;
> >
> > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> > them will be ugly. Well, I can't make up if this is just overhead or useful
> > debugging aid, so no real objection :)
>
> There used to be a sysfs interface for dumping these, but it was an
> ugly misuse. I'd like to leave these in. I still have the sysfs bits
> in a private patch and I'm going to rework them for debugfs.
Okay. Maybe a comment stating the future use will be nice.
>
> > But I wonder more about the usage of the SS pin and if this chipsel is needed
> > at all (sadly I cannot test as I don't have any board with SPI connected to
> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
> > More, you use the MODF-feature, so the SS-pin should be defined as input?
> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
> > output.
>
> That's right. The SS handling by the SPI device is completely
> useless, so this driver uses it as a GPIO and asserts it manually.
That definately needs a comment :D (perhaps with some more details if you know them).
> The MODF irq is probably irrelevant, but I'd like to leave it in for
> completeness.
But it won't work if the pin is set to output, no? Are you sure there are no
side-effects?
> >> + /* initialize the device */
> >> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> >
> > spaces around operator.
>
> Intentional to keep from spilling past column 80; but I'll move the
> missing spaces to around the | operators. I think it is easier to
> read that way.
Ah, I remember, we had this topic a while ago :D
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 14:26 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 14:26 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1.1: Type: text/plain, Size: 2272 bytes --]
Hi Grant,
> The double spaces at the end of sentences are intentional. It is
> perhaps a bit quaint and old fashioned, but it is my writing style.
Ah, okay.
> >> +
> >> + /* Statistics */
> >> + int msg_count;
> >> + int wcol_count;
> >> + int wcol_ticks;
> >> + u32 wcol_tx_timestamp;
> >> + int modf_count;
> >> + int byte_count;
> >
> > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> > them will be ugly. Well, I can't make up if this is just overhead or useful
> > debugging aid, so no real objection :)
>
> There used to be a sysfs interface for dumping these, but it was an
> ugly misuse. I'd like to leave these in. I still have the sysfs bits
> in a private patch and I'm going to rework them for debugfs.
Okay. Maybe a comment stating the future use will be nice.
>
> > But I wonder more about the usage of the SS pin and if this chipsel is needed
> > at all (sadly I cannot test as I don't have any board with SPI connected to
> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
> > More, you use the MODF-feature, so the SS-pin should be defined as input?
> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
> > output.
>
> That's right. The SS handling by the SPI device is completely
> useless, so this driver uses it as a GPIO and asserts it manually.
That definately needs a comment :D (perhaps with some more details if you know them).
> The MODF irq is probably irrelevant, but I'd like to leave it in for
> completeness.
But it won't work if the pin is set to output, no? Are you sure there are no
side-effects?
> >> + /* initialize the device */
> >> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> >
> > spaces around operator.
>
> Intentional to keep from spilling past column 80; but I'll move the
> missing spaces to around the | operators. I think it is easier to
> read that way.
Ah, I remember, we had this topic a while ago :D
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18 14:26 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-06-18 14:26 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]
Hi Grant,
> The double spaces at the end of sentences are intentional. It is
> perhaps a bit quaint and old fashioned, but it is my writing style.
Ah, okay.
> >> +
> >> + /* Statistics */
> >> + int msg_count;
> >> + int wcol_count;
> >> + int wcol_ticks;
> >> + u32 wcol_tx_timestamp;
> >> + int modf_count;
> >> + int byte_count;
> >
> > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> > them will be ugly. Well, I can't make up if this is just overhead or useful
> > debugging aid, so no real objection :)
>
> There used to be a sysfs interface for dumping these, but it was an
> ugly misuse. I'd like to leave these in. I still have the sysfs bits
> in a private patch and I'm going to rework them for debugfs.
Okay. Maybe a comment stating the future use will be nice.
>
> > But I wonder more about the usage of the SS pin and if this chipsel is needed
> > at all (sadly I cannot test as I don't have any board with SPI connected to
> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
> > More, you use the MODF-feature, so the SS-pin should be defined as input?
> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
> > output.
>
> That's right. The SS handling by the SPI device is completely
> useless, so this driver uses it as a GPIO and asserts it manually.
That definately needs a comment :D (perhaps with some more details if you know them).
> The MODF irq is probably irrelevant, but I'd like to leave it in for
> completeness.
But it won't work if the pin is set to output, no? Are you sure there are no
side-effects?
> >> + /* initialize the device */
> >> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> >
> > spaces around operator.
>
> Intentional to keep from spilling past column 80; but I'll move the
> missing spaces to around the | operators. I think it is easier to
> read that way.
Ah, I remember, we had this topic a while ago :D
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 2:55 ` Grant Likely
(?)
(?)
@ 2009-06-23 14:40 ` Kári Davíðsson
2009-06-23 15:53 ` Grant Likely
-1 siblings, 1 reply; 26+ messages in thread
From: Kári Davíðsson @ 2009-06-23 14:40 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
Hi,
I have been testing this driver a little bit (on 2.6.29.3)
What happens for me is that the driver starves the system while sending
data over the SPI interface.
I think the problem is in the function (interrupt handler)
mpc52xx_spi_irq() that calls the function mpc52xx_spi_fsm_process()
which again is an loop that iterates over the whole data to be sent.
rg
kd
Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Hi David,
>
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
>
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller. It almost looks like an SPI bridge. It requires
> more thought before I try again.
>
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
>
> g.
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 539 insertions(+), 0 deletions(-)
> create mode 100644 drivers/spi/mpc52xx_spi.c
> create mode 100644 include/linux/spi/mpc52xx_spi.h
>
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_ORION) += orion_spi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode. For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0 /* Nothing more for the state machine to */
> + /* do. If something interesting happens */
> + /* then and IRQ will be received */
> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
> + /* not expected */
> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting. */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
> +
> + /* get the head of the queue */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> + list_del_init(&ms->message->queue);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding the
> + * divider up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
> + }
> + out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI controller is stoopid. At slower speeds, it may
> + * raise the SPIF flag before the state machine is actually
> + * finished, which causes a collision (internal to the state
> + * machine only). The manual recommends inserting a delay
> + * between receiving the interrupt and sending the next byte,
> + * but it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
> + out_8(ms->regs + SPI_DATA, data); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mode fault\n");
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers */
> + status = in_8(ms->regs + SPI_STATUS);
> + data = in_8(ms->regs + SPI_DATA);
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + mpc52xx_spi_fsm_process(0, ms);
> + spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + if (spi->bits_per_word % 8)
> + return -EINVAL;
> +
> + if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> + return -EINVAL;
> +
> + if (spi->chip_select >= spi->master->num_chipselect)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
> +
> + return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + int rc;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> + out_8(regs + SPI_CTRL2, 0x0);
> + out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
> + out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + in_8(regs + SPI_STATUS);
> + in_8(regs + SPI_DATA);
> + if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + rc = -EIO;
> + goto err_init;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + /* Decide if interrupts can be used */
> + if (ms->irq0 && ms->irq1) {
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = 0;
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = 0;
> + }
> +
> + if (!ms->irq0)
> + dev_info(&op->dev, "using polled mode\n");
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc)
> + goto err_register;
> +
> + of_register_spi_devices(master, op->node);
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + err_alloc:
> + err_init:
> + iounmap(regs);
> + return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +
> + return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_match,
> + .probe = mpc52xx_spi_probe,
> + .remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-23 14:40 ` Kári Davíðsson
@ 2009-06-23 15:53 ` Grant Likely
2009-06-23 16:31 ` Kári Davíðsson
0 siblings, 1 reply; 26+ messages in thread
From: Grant Likely @ 2009-06-23 15:53 UTC (permalink / raw)
To: Kári Davíðsson; +Cc: linuxppc-dev
On Tue, Jun 23, 2009 at 8:40 AM, K=E1ri Dav=ED=F0sson<kari.davidsson@marel.=
com> wrote:
> Hi,
>
> I have been testing this driver a little bit (on 2.6.29.3)
>
> What happens for me is that the driver starves the system while sending
> data over the SPI interface.
>
> I think the problem is in the function (interrupt handler)
> mpc52xx_spi_irq() that calls the function mpc52xx_spi_fsm_process()
> which again is an loop that iterates over the whole data to be sent.
Hmmm, the state machine is supposed to yield after each byte sent, but
on fast transfers I could see it becoming ready to run again
immediately.
Unfortunately it is not an easy problem. The SPI device can only
handle 1 byte at a time. Maybe it would be better for CPU fairness if
all the work was done in a thread. I didn't originally because I
didn't want to introduce a huge amount of scheduling overhead every
time a byte was transfered, and I wanted to keep transfers fast. I
need to think about this some more.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-23 15:53 ` Grant Likely
@ 2009-06-23 16:31 ` Kári Davíðsson
2009-06-23 16:41 ` Grant Likely
0 siblings, 1 reply; 26+ messages in thread
From: Kári Davíðsson @ 2009-06-23 16:31 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
Lowered my SPI clock down to 1MHz (from 50MHz (in steps of 10MHz).
At 1Mhz the driver stops starving the rest of the kernel.
What is interesting is that due to the low duty cycle of the SPI port
the transfer rate at 1MHz and 50Mhz is almost the same.
I suggest that the driver simply limits max frequency to 1Mhz.
This (very limited) SPI pherperial does not handle any more anyways.
rg
kd
Grant Likely wrote:
> On Tue, Jun 23, 2009 at 8:40 AM, K=E1ri Dav=ED=F0sson<kari.davidsson@mare=
l.com> wrote:
>> Hi,
>>
>> I have been testing this driver a little bit (on 2.6.29.3)
>>
>> What happens for me is that the driver starves the system while sending
>> data over the SPI interface.
>>
>> I think the problem is in the function (interrupt handler)
>> mpc52xx_spi_irq() that calls the function mpc52xx_spi_fsm_process()
>> which again is an loop that iterates over the whole data to be sent.
>=20
> Hmmm, the state machine is supposed to yield after each byte sent, but
> on fast transfers I could see it becoming ready to run again
> immediately.
>=20
> Unfortunately it is not an easy problem. The SPI device can only
> handle 1 byte at a time. Maybe it would be better for CPU fairness if
> all the work was done in a thread. I didn't originally because I
> didn't want to introduce a huge amount of scheduling overhead every
> time a byte was transfered, and I wanted to keep transfers fast. I
> need to think about this some more.
>=20
> g.
>=20
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-23 16:31 ` Kári Davíðsson
@ 2009-06-23 16:41 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-06-23 16:41 UTC (permalink / raw)
To: Kári Davíðsson; +Cc: linuxppc-dev
On Tue, Jun 23, 2009 at 10:31 AM, K=E1ri
Dav=ED=F0sson<kari.davidsson@marel.com> wrote:
> Lowered my SPI clock down to 1MHz (from 50MHz (in steps of 10MHz).
> At 1Mhz the driver stops starving the rest of the kernel.
>
> What is interesting is that due to the low duty cycle of the SPI port
> the transfer rate at 1MHz and 50Mhz is almost the same.
>
> I suggest that the driver simply limits max frequency to 1Mhz.
Sounds good to me. Thanks for the testing!
> This (very limited) SPI pherperial does not handle any more anyways.
indeed.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 14:26 ` Wolfram Sang
(?)
@ 2009-07-03 7:01 ` Grant Likely
-1 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-07-03 7:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl
On Thu, Jun 18, 2009 at 8:26 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>> There used to be a sysfs interface for dumping these, but it was an
>> ugly misuse. I'd like to leave these in. I still have the sysfs bits
>> in a private patch and I'm going to rework them for debugfs.
>
> Okay. Maybe a comment stating the future use will be nice.
okay
>> > But I wonder more about the usage of the SS pin and if this chipsel is needed
>> > at all (sadly I cannot test as I don't have any board with SPI connected to
>> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
>> > More, you use the MODF-feature, so the SS-pin should be defined as input?
>> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
>> > output.
>>
>> That's right. The SS handling by the SPI device is completely
>> useless, so this driver uses it as a GPIO and asserts it manually.
>
> That definately needs a comment :D (perhaps with some more details if you know them).
>
>> The MODF irq is probably irrelevant, but I'd like to leave it in for
>> completeness.
>
> But it won't work if the pin is set to output, no?
yes
> Are you sure there are no side-effects?
I'm sure.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-07-03 7:01 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-07-03 7:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl
On Thu, Jun 18, 2009 at 8:26 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>> There used to be a sysfs interface for dumping these, but it was an
>> ugly misuse. I'd like to leave these in. I still have the sysfs bits
>> in a private patch and I'm going to rework them for debugfs.
>
> Okay. Maybe a comment stating the future use will be nice.
okay
>> > But I wonder more about the usage of the SS pin and if this chipsel is needed
>> > at all (sadly I cannot test as I don't have any board with SPI connected to
>> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
>> > More, you use the MODF-feature, so the SS-pin should be defined as input?
>> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
>> > output.
>>
>> That's right. The SS handling by the SPI device is completely
>> useless, so this driver uses it as a GPIO and asserts it manually.
>
> That definately needs a comment :D (perhaps with some more details if you know them).
>
>> The MODF irq is probably irrelevant, but I'd like to leave it in for
>> completeness.
>
> But it won't work if the pin is set to output, no?
yes
> Are you sure there are no side-effects?
I'm sure.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-07-03 7:01 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-07-03 7:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
On Thu, Jun 18, 2009 at 8:26 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>> There used to be a sysfs interface for dumping these, but it was an
>> ugly misuse. =A0I'd like to leave these in. =A0I still have the sysfs bi=
ts
>> in a private patch and I'm going to rework them for debugfs.
>
> Okay. Maybe a comment stating the future use will be nice.
okay
>> > But I wonder more about the usage of the SS pin and if this chipsel is=
needed
>> > at all (sadly I cannot test as I don't have any board with SPI connect=
ed to
>> > that device). You define the SS-pin as output, but do not set the SSOE=
-bit.
>> > More, you use the MODF-feature, so the SS-pin should be defined as inp=
ut?
>> > According to Table 17.3 in the PM, you have that pin defined as generi=
c purpose
>> > output.
>>
>> That's right. =A0The SS handling by the SPI device is completely
>> useless, so this driver uses it as a GPIO and asserts it manually.
>
> That definately needs a comment :D (perhaps with some more details if you=
know them).
>
>> The MODF irq is probably irrelevant, but I'd like to leave it in for
>> completeness.
>
> But it won't work if the pin is set to output, no?
yes
> Are you sure there are no side-effects?
I'm sure.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-06-18 2:55 ` Grant Likely
(?)
@ 2009-10-21 13:17 ` Wolfram Sang
-1 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 13:17 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
Hi Grant,
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
do you have an updated version to share? Or is V4 still 'status quo'?
Genki de ;)
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-10-21 13:17 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 13:17 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1.1: Type: text/plain, Size: 535 bytes --]
Hi Grant,
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
do you have an updated version to share? Or is V4 still 'status quo'?
Genki de ;)
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-10-21 13:17 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 13:17 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
Hi Grant,
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
do you have an updated version to share? Or is V4 still 'status quo'?
Genki de ;)
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-10-21 13:17 ` Wolfram Sang
@ 2009-10-21 14:45 ` Grant Likely
-1 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-10-21 14:45 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel
On Wed, Oct 21, 2009 at 10:17 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
>> SoC.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> do you have an updated version to share? Or is V4 still 'status quo'?
V4 was supposed to go in during the merge window. But I haven't heard
anything from David (and I didn't pursue it either). Unless David
objects, I'll put it into either my -merge or my -next tree; depending
on what Ben and Linus prefer.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-10-21 14:45 ` Grant Likely
0 siblings, 0 replies; 26+ messages in thread
From: Grant Likely @ 2009-10-21 14:45 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
On Wed, Oct 21, 2009 at 10:17 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
>> SoC.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> do you have an updated version to share? Or is V4 still 'status quo'?
V4 was supposed to go in during the merge window. But I haven't heard
anything from David (and I didn't pursue it either). Unless David
objects, I'll put it into either my -merge or my -next tree; depending
on what Ben and Linus prefer.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
2009-10-21 14:45 ` Grant Likely
(?)
@ 2009-10-21 17:06 ` Wolfram Sang
-1 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 17:06 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 560 bytes --]
> V4 was supposed to go in during the merge window. But I haven't heard
> anything from David (and I didn't pursue it either). Unless David
> objects, I'll put it into either my -merge or my -next tree; depending
> on what Ben and Linus prefer.
I wondered if there was going to be a V5 as you said you fixed a few things
according to my review and couldn't find an updated patch for that.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-10-21 17:06 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 17:06 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1.1: Type: text/plain, Size: 560 bytes --]
> V4 was supposed to go in during the merge window. But I haven't heard
> anything from David (and I didn't pursue it either). Unless David
> objects, I'll put it into either my -merge or my -next tree; depending
> on what Ben and Linus prefer.
I wondered if there was going to be a V5 as you said you fixed a few things
according to my review and couldn't find an updated patch for that.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-10-21 17:06 ` Wolfram Sang
0 siblings, 0 replies; 26+ messages in thread
From: Wolfram Sang @ 2009-10-21 17:06 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
[-- Attachment #1: Type: text/plain, Size: 560 bytes --]
> V4 was supposed to go in during the merge window. But I haven't heard
> anything from David (and I didn't pursue it either). Unless David
> objects, I'll put it into either my -merge or my -next tree; depending
> on what Ben and Linus prefer.
I wondered if there was going to be a V5 as you said you fixed a few things
according to my review and couldn't find an updated patch for that.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-10-21 17:06 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-18 2:55 [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver Grant Likely
2009-06-18 2:55 ` Grant Likely
2009-06-18 6:58 ` [spi-devel-general] " Wolfram Sang
2009-06-18 6:58 ` Wolfram Sang
2009-06-18 6:58 ` Wolfram Sang
2009-06-18 13:35 ` Grant Likely
2009-06-18 13:35 ` Grant Likely
2009-06-18 13:35 ` Grant Likely
2009-06-18 14:26 ` Wolfram Sang
2009-06-18 14:26 ` Wolfram Sang
2009-06-18 14:26 ` Wolfram Sang
2009-07-03 7:01 ` Grant Likely
2009-07-03 7:01 ` Grant Likely
2009-07-03 7:01 ` Grant Likely
2009-06-23 14:40 ` Kári Davíðsson
2009-06-23 15:53 ` Grant Likely
2009-06-23 16:31 ` Kári Davíðsson
2009-06-23 16:41 ` Grant Likely
2009-10-21 13:17 ` Wolfram Sang
2009-10-21 13:17 ` Wolfram Sang
2009-10-21 13:17 ` Wolfram Sang
2009-10-21 14:45 ` Grant Likely
2009-10-21 14:45 ` Grant Likely
2009-10-21 17:06 ` Wolfram Sang
2009-10-21 17:06 ` Wolfram Sang
2009-10-21 17:06 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.