Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Friday 25 July 2008, Grant Likely wrote:
From: Grant Likely [EMAIL PROTECTED]
Adds support for the dedicated SPI device on the Freescale MPC5200(b)
SoC.
It'd be a bit more clear if you said dedicated SPI controller;
device sounds like it's a struct spi_device.
Capsule summary: fault handling needs work. (Details below.)
Signed-off-by: Grant Likely [EMAIL PROTECTED]
---
drivers/spi/Kconfig |8 +
drivers/spi/Makefile|1
drivers/spi/mpc52xx_spi.c | 595
+++
include/linux/spi/mpc52xx_spi.h | 10 +
4 files changed, 614 insertions(+), 0 deletions(-)
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 2303521..68e4a4a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -116,6 +116,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 7fca043..340b878 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_PXA2XX)+= pxa2xx_spi.o
obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.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 000..453690f
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,595 @@
+/*
+ * MPC52xx SPI master driver.
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This is the driver for the MPC5200's dedicated SPI device (not for a
+ * PSC in SPI mode)
+ */
+
+#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 [EMAIL PROTECTED]);
+MODULE_DESCRIPTION(MPC52xx SPI (non-PSC) Driver);
+MODULE_LICENSE(GPL);
+
+/* Register offsets */
+#define SPI_CTRL10x00
+#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_CTRL20x01
+#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
+#define FSM_POLL 1
+#define FSM_CONTINUE 2
+
+/* 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;
+
+ /* Hooks for platform modification of behaviour */
+ void (*premessage)(struct spi_message *m, void *context);
+ void *premessage_context;
+
+ 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)
+{
+ if (value)
+ writeb(0, ms-regs + SPI_PORTDATA); /* Assert SS pin */
+ else
+ writeb(0x08, ms-regs + SPI_PORTDATA); /* Deassert SS pin */
+}
+
+/*
+ * Start a new transfer. This is called both by
Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote: + 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; I don't think doing list_empty outside the critical section is totally safe.. You might want to move it down inside the spin_lock() section. + /* Get the next message */ + spin_lock(ms-lock); The part that's a little confusing here is that the interrupt can actually activate the workqueue .. So I'm wondering if maybe you could have this interrupt driven any workqueue driven at the same time? If you could then you would need the above to be spin_lock_irq/spin_lock_irqsave .. Daniel ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker [EMAIL PROTECTED] wrote: On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote: + 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; I don't think doing list_empty outside the critical section is totally safe.. You might want to move it down inside the spin_lock() section. This should be fine. This is the only place where items are dequeued, and it will only ever be called from the ISR or the work queue. The work queue and IRQ will never be active at the same time (I'll add a comment to the fact). It also looks like list_empty is perfectly safe to call without the protection of a spin lock (but somebody correct me if I'm out to lunch). It doesn't dereference any of the pointers in the list_head structure. + /* Get the next message */ + spin_lock(ms-lock); The part that's a little confusing here is that the interrupt can actually activate the workqueue .. So I'm wondering if maybe you could have this interrupt driven any workqueue driven at the same time? If you could then you would need the above to be spin_lock_irq/spin_lock_irqsave .. Ditto here, since the irq and workqueue are not enabled at the same time there is no worry about collision. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Fri, 2008-07-25 at 22:45 -0400, Grant Likely wrote: On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker [EMAIL PROTECTED] wrote: On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote: + 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; I don't think doing list_empty outside the critical section is totally safe.. You might want to move it down inside the spin_lock() section. This should be fine. This is the only place where items are dequeued, and it will only ever be called from the ISR or the work queue. The work queue and IRQ will never be active at the same time (I'll add a comment to the fact). It also looks like list_empty is perfectly safe to call without the protection of a spin lock (but somebody correct me if I'm out to lunch). It doesn't dereference any of the pointers in the list_head structure. The list_empty wouldn't crash, but the result isn't necessarily accurate. + /* Get the next message */ + spin_lock(ms-lock); The part that's a little confusing here is that the interrupt can actually activate the workqueue .. So I'm wondering if maybe you could have this interrupt driven any workqueue driven at the same time? If you could then you would need the above to be spin_lock_irq/spin_lock_irqsave .. Ditto here, since the irq and workqueue are not enabled at the same time there is no worry about collision. Why are you waking up the work queue from inside the irq handler? You might also want to break out the handling from inside the irq handler and call that from the workqueue, instead of re-calling the irq handler function from the workqueue.. It's a little confusing from a context perspective. Daniel ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Sat, Jul 26, 2008 at 12:47 AM, Daniel Walker [EMAIL PROTECTED] wrote: On Fri, 2008-07-25 at 22:45 -0400, Grant Likely wrote: On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker [EMAIL PROTECTED] wrote: On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote: + 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; I don't think doing list_empty outside the critical section is totally safe.. You might want to move it down inside the spin_lock() section. This should be fine. This is the only place where items are dequeued, and it will only ever be called from the ISR or the work queue. The work queue and IRQ will never be active at the same time (I'll add a comment to the fact). It also looks like list_empty is perfectly safe to call without the protection of a spin lock (but somebody correct me if I'm out to lunch). It doesn't dereference any of the pointers in the list_head structure. The list_empty wouldn't crash, but the result isn't necessarily accurate. It doesn't need to be accurate because the spinlock is obtained before actually trying to dequeue anything. Even if it held the spinlock there would still be uncertainty depending on which routine ran first. If this function ran first, then it's going to stop the state machine regardless and the enqueue function will have to kick it off again anyway. But, I've taken a look at the code, and you're right, the spin lock does need to be held while running the state machine because I do have a case where the IRQ and workqueue could get executed at the same time. I'll fix it up. + /* Get the next message */ + spin_lock(ms-lock); The part that's a little confusing here is that the interrupt can actually activate the workqueue .. So I'm wondering if maybe you could have this interrupt driven any workqueue driven at the same time? If you could then you would need the above to be spin_lock_irq/spin_lock_irqsave .. Ditto here, since the irq and workqueue are not enabled at the same time there is no worry about collision. Why are you waking up the work queue from inside the irq handler? You might also want to break out the handling from inside the irq handler and call that from the workqueue, instead of re-calling the irq handler function from the workqueue.. It's a little confusing from a context perspective. Sure, I can rework that. The irq handler would then just simply be a straight call into the state machine runner. I just dropped the additional 4 lines of code for conciseness, but I can change it around for clarity... actually, with the spin lock stuff above I need to do this rework. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
