[PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal@microchip.com>

The PIC32 SPI driver is capable of performing SPI transfers either using
polling or based on interrupt-trigger. GPIO based CS support (necessary
for correct SPI operation) is available. It is dependent on availability
of "cs-gpios" property of spi node in board dts file.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
---
 drivers/spi/Kconfig     |    7 +
 drivers/spi/Makefile    |    1 +
 drivers/spi/spi-pic32.c | 1212 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1220 insertions(+)
 create mode 100644 drivers/spi/spi-pic32.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7706416..cda8b7a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -654,6 +654,13 @@ config SPI_NUC900
 	help
 	  SPI driver for Nuvoton NUC900 series ARM SoCs
 
+config SPI_PIC32
+	tristate "Microchip PIC32 series SPI"
+	depends on MACH_PIC32
+	help
+	  SPI driver for PIC32 SPI master controller.
+
+
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8991ffc..bb37fed 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -94,3 +94,4 @@ obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
 obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
 obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
+obj-$(CONFIG_SPI_PIC32)			+= spi-pic32.o
diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c
new file mode 100644
index 0000000..6658efb
--- /dev/null
+++ b/drivers/spi/spi-pic32.c
@@ -0,0 +1,1212 @@
+/*
+ * PIC32 SPI core controller driver (refer dw_spi.c)
+ *
+ * Purna Chandra Mandal <purna.mandal@microchip.com>
+ * Copyright (c) 2016, Microchip Technology Inc.
+ *
+ * This program is free software; you can distribute it and/or modify it
+ * under the terms of the GNU General Public License (Version 2) as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ */
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/highmem.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/of_address.h>
+#include <linux/dmaengine.h>
+#include <linux/sizes.h>
+
+/* SPI Register offsets */
+#define SPICON		0x00
+#define SPICON_CLR	0x04
+#define SPICON_SET	0x08
+#define SPISTAT		0x10
+#define SPISTAT_CLR	0x14
+#define SPIBUF		0x20
+#define SPIBRG		0x30
+#define SPICON2		0x40
+#define SPICON2_CLR	0x44
+#define SPICON2_SET	0x48
+
+/* Bit fields in SPICON Register */
+/* Rx interrupt generation condition */
+#define SPICON_RXI_SHIFT	0
+/* TX interrupt generation condition */
+#define SPICON_TXI_SHIFT	2
+/* Enable SPI Master */
+#define SPICON_MSTEN		BIT(5)
+/* active low */
+#define SPICON_CKP		BIT(6)
+/* Tx on falling edge */
+#define SPICON_CKE		BIT(8)
+/* Rx at middle or end of tx */
+#define SPICON_SMP		BIT(9)
+/* Bits per word/audio-sample */
+#define SPICON_BPW		0x03
+#define SPICON_BPW_SHIFT	10
+/* STOP on idle */
+#define SPICON_SIDL		BIT(13)
+/* Macro enable */
+#define SPICON_ON		BIT(15)
+/* Enable enhanced buffering */
+#define SPICON_ENHBUF		BIT(16)
+/* Select SPI Clock src */
+#define SPICON_MCLKSEL		BIT(23)
+/* SPI macro will drive SS */
+#define SPICON_MSSEN		BIT(28)
+/* SPI SS polarity */
+#define SPICON_FRMPOL		BIT(29)
+/* Enable framing mode */
+#define SPICON_FRMEN		BIT(31)
+
+/* Bit fields in SPISTAT Register */
+/* RX fifo full */
+#define STAT_RF_FULL		BIT(0)
+/* TX fifo full */
+#define STAT_TF_FULL		BIT(1)
+/* standard buffer mode */
+#define STAT_TX_EMPTY		BIT(3)
+/* RX Fifo empty */
+#define STAT_RF_EMPTY		BIT(5)
+/* err, s/w needs to clear */
+#define STAT_RX_OV		BIT(6)
+/* Internal shift-reg empty */
+#define STAT_SHIFT_REG_EMPTY	BIT(7)
+/* UR in Framed SPI modes */
+#define STAT_TX_UR		BIT(8)
+/* Macro is processing tx or rx */
+#define STAT_BUSY		BIT(11)
+/* Multiple Frame Sync pulse */
+#define STAT_FRM_ERR		BIT(12)
+#define STAT_TF_LVL_MASK	0x1F
+#define STAT_TF_LVL_SHIFT	16
+#define STAT_RF_LVL_MASK	0x1F
+#define STAT_RF_LVL_SHIFT	24
+
+/* Bit fields in SPIBRG Register */
+#define SPIBRG_MASK		0x1ff
+#define SPIBRG_SHIFT		0x0
+
+/* Bit fields in SPICON2 Register */
+/* Enable int on Tx under-run */
+#define SPI_INT_TX_UR_EN	BIT(10)
+/* Enable int on Rx over-run */
+#define SPI_INT_RX_OV_EN	BIT(11)
+/* Enable frame err int */
+#define SPI_INT_FRM_ERR_EN	BIT(12)
+
+/* Rx-fifo state for RX interrupt generation */
+#define SPI_RX_FIFO_EMTPY	0x0
+/* not empty */
+#define SPI_RX_FIFO_NOT_EMPTY	0x1
+/* full by one-half or more */
+#define SPI_RX_FIFO_HALF_FULL	0x2
+/* completely full */
+#define SPI_RX_FIFO_FULL	0x3
+
+/* TX-fifo state for TX interrupt generation */
+/* completely empty */
+#define SPI_TX_FIFO_ALL_EMPTY	0x0
+/* empty */
+#define SPI_TX_FIFO_EMTPY	0x1
+/* empty by half or more */
+#define SPI_TX_FIFO_HALF_EMPTY	0x2
+/* atleast one empty */
+#define SPI_TX_FIFO_NOT_FULL	0x3
+
+/* Transfer bits-per-word */
+#define SPI_BPW_8		0x0
+#define SPI_BPW_16		0x1
+#define SPI_BPW_32		0x2
+
+/* SPI clock sources */
+#define SPI_CLKSRC_PBCLK	0x0
+#define SPI_CLKSRC_MCLK		0x1
+
+/* Minimum DMA transfer size */
+#define SPI_DMA_LEN_MIN		SZ_64
+
+/* struct pic32_spi::flags */
+/* PIO Transfer based on polling */
+#define SPI_XFER_POLL	BIT(1)
+/* SPI master controlled SS */
+#define SPI_SS_MASTER	BIT(2)
+/* DMA is supported */
+#define SPI_DMA_CAP	BIT(3)
+/* DMA channels allocated */
+#define SPI_DMA_PREP	BIT(4)
+/* buffer mapped and ready for DMA */
+#define SPI_DMA_READY	BIT(5)
+
+struct pic32_spi {
+	void __iomem		*regs;
+	dma_addr_t		phys_base;
+	int			fault_irq;
+	int			rx_irq;
+	int			tx_irq;
+	u32			fifo_n_byte;
+	struct clk		*clk;
+	spinlock_t		lock;
+	struct spi_master	*master;
+
+	/* Current SPI device specific */
+	struct spi_device	*spi_dev;
+	u32			speed_hz;
+	u32			mode;
+	u32			flags;
+	u8			fifo_n_elm;
+	enum dma_slave_buswidth	dma_width;
+
+	/* Current message/transfer state */
+	struct spi_message	*mesg;
+	const void		*tx;
+	const void		*tx_end;
+	const void		*rx;
+	const void		*rx_end;
+	int			len;
+	struct completion	xfer_done;
+
+	void (*rx_fifo)(struct pic32_spi *);
+	void (*tx_fifo)(struct pic32_spi *);
+};
+
+static inline u32 spi_rx_fifo_level(struct pic32_spi *pic32s)
+{
+	u32 sr = readl(pic32s->regs + SPISTAT);
+
+	return (sr >> STAT_RF_LVL_SHIFT) & STAT_RF_LVL_MASK;
+}
+
+static inline u32 spi_tx_fifo_level(struct pic32_spi *pic32s)
+{
+	u32 sr = readl(pic32s->regs + SPISTAT);
+
+	return (sr >> STAT_TF_LVL_SHIFT) & STAT_TF_LVL_MASK;
+}
+
+static inline void spi_enable_chip(struct pic32_spi *pic32s)
+{
+	writel(SPICON_ON, pic32s->regs + SPICON_SET);
+}
+
+static inline void spi_disable_chip(struct pic32_spi *pic32s)
+{
+	writel(SPICON_ON, pic32s->regs + SPICON_CLR);
+	cpu_relax();
+}
+
+static inline void spi_set_clk_mode(struct pic32_spi *pic32s, int mode)
+{
+	u32 conset = 0, conclr = 0;
+
+	/* active low */
+	if (mode & SPI_CPOL)
+		conset |= SPICON_CKP;
+	else
+		conclr |= SPICON_CKP;
+
+	/* tx on rising edge of clk */
+	if (mode & SPI_CPHA)
+		conclr |= SPICON_CKE;
+	else
+		conset |= SPICON_CKE;
+
+	/* rx at end of tx */
+	conset |= SPICON_SMP;
+
+	writel(conclr, pic32s->regs + SPICON_CLR);
+	writel(conset, pic32s->regs + SPICON_SET);
+}
+
+static inline void spi_set_ws(struct pic32_spi *pic32s, int ws)
+{
+	writel(SPICON_BPW << SPICON_BPW_SHIFT, pic32s->regs + SPICON_CLR);
+	writel(ws << SPICON_BPW_SHIFT, pic32s->regs + SPICON_SET);
+}
+
+static inline void spi_drain_rx_buf(struct pic32_spi *pic32s)
+{
+	u32 sr;
+
+	/* drain rx bytes until empty */
+	for (;;) {
+		sr = readl(pic32s->regs + SPISTAT);
+		if (sr & STAT_RF_EMPTY)
+			break;
+
+		(void)readl(pic32s->regs + SPIBUF);
+	}
+
+	/* clear rx overflow */
+	writel(STAT_RX_OV, pic32s->regs + SPISTAT_CLR);
+}
+
+static inline void spi_set_clk_rate(struct pic32_spi *pic32s, u32 sck)
+{
+	u16 clk_div;
+
+	/* sck = clk_in / [2 * (clk_div + 1)]
+	 * ie. clk_div = [clk_in / (2 * sck)] - 1
+	 */
+	clk_div = (clk_get_rate(pic32s->clk) / (2 * sck)) - 1;
+	clk_div &= SPIBRG_MASK;
+
+	writel(clk_div, pic32s->regs + SPIBRG);
+}
+
+static inline void spi_set_clk(struct pic32_spi *pic32s, int clk_id)
+{
+	switch (clk_id) {
+	case SPI_CLKSRC_PBCLK:
+		writel(SPICON_MCLKSEL, pic32s->regs + SPICON_CLR);
+		break;
+	case SPI_CLKSRC_MCLK:
+		writel(SPICON_MCLKSEL, pic32s->regs + SPICON_SET);
+		break;
+	}
+}
+
+static inline void spi_set_ss_auto(struct pic32_spi *pic32s, u8 mst, u32 mode)
+{
+	u32 v;
+
+	/* spi controller can drive CS/SS during transfer depending on fifo
+	 * fill-level. SS will stay asserted as long as TX fifo has something
+	 * to transfer, else will be deasserted confirming completion of
+	 * the ongoing transfer.
+	 */
+
+	v = readl(pic32s->regs + SPICON);
+	v &= ~SPICON_MSSEN;
+	if (mst) {
+		v |= SPICON_MSSEN;
+		if (mode & SPI_CS_HIGH)
+			v |= SPICON_FRMPOL;
+		else
+			v &= ~SPICON_FRMPOL;
+	}
+
+	writel(v, pic32s->regs + SPICON);
+}
+
+static inline void spi_set_err_int(struct pic32_spi *pic32s)
+{
+	u32 mask;
+
+	mask = SPI_INT_TX_UR_EN | SPI_INT_RX_OV_EN | SPI_INT_FRM_ERR_EN;
+	writel(mask, pic32s->regs + SPICON2_SET);
+}
+
+/* Return the max entries we can fill into tx fifo */
+static inline u32 pic32_tx_max(struct pic32_spi *pic32s, int n_bytes)
+{
+	u32 tx_left, tx_room, rxtx_gap;
+
+	tx_left = (pic32s->tx_end - pic32s->tx) / n_bytes;
+	tx_room = pic32s->fifo_n_elm - spi_tx_fifo_level(pic32s);
+
+	/* Another concern is about the tx/rx mismatch, we
+	 * thought to use (pic32s->fifo_n_byte - rxfl - txfl) as
+	 * one maximum value for tx, but it doesn't cover the
+	 * data which is out of tx/rx fifo and inside the
+	 * shift registers. So a control from sw point of
+	 * view is taken.
+	 */
+	rxtx_gap = ((pic32s->rx_end - pic32s->rx) -
+		    (pic32s->tx_end - pic32s->tx)) / n_bytes;
+	return min3(tx_left, tx_room, (u32)(pic32s->fifo_n_elm - rxtx_gap));
+}
+
+/* Return the max entries we should read out of rx fifo */
+static inline u32 pic32_rx_max(struct pic32_spi *pic32s, int n_bytes)
+{
+	u32 rx_left = (pic32s->rx_end - pic32s->rx) / n_bytes;
+
+	return min_t(u32, rx_left, spi_rx_fifo_level(pic32s));
+}
+
+#define BUILD_SPI_FIFO_RW(__name, __type, __bwl)		\
+static void pic32_spi_rx_##__name(struct pic32_spi *pic32s)	\
+{								\
+	__type v;						\
+	u32 mx = pic32_rx_max(pic32s, sizeof(__type));		\
+	for (; mx; mx--) {					\
+		v = read##__bwl(pic32s->regs + SPIBUF);	\
+		if (pic32s->rx_end - pic32s->len)		\
+			*(__type *)(pic32s->rx) = v;		\
+		pic32s->rx += sizeof(__type);			\
+	}							\
+}								\
+								\
+static void pic32_spi_tx_##__name(struct pic32_spi *pic32s)	\
+{								\
+	__type v;						\
+	u32 mx = pic32_tx_max(pic32s, sizeof(__type));		\
+	for (; mx ; mx--) {					\
+		v = (__type)~0U;				\
+		if (pic32s->tx_end - pic32s->len)		\
+			v = *(__type *)(pic32s->tx);		\
+		write##__bwl(v, pic32s->regs + SPIBUF);	\
+		pic32s->tx += sizeof(__type);			\
+	}							\
+}
+
+BUILD_SPI_FIFO_RW(byte, u8, b);
+BUILD_SPI_FIFO_RW(word, u16, w);
+BUILD_SPI_FIFO_RW(dword, u32, l);
+
+static void pic32_err_stop(struct pic32_spi *pic32s, const char *msg)
+{
+	spi_disable_chip(pic32s);
+
+	/* Show err message and abort xfer with err */
+	dev_err(&pic32s->master->dev, "%s\n", msg);
+	pic32s->mesg->state = (void *)-1;
+	complete(&pic32s->xfer_done);
+
+	disable_irq_nosync(pic32s->fault_irq);
+	disable_irq_nosync(pic32s->rx_irq);
+	disable_irq_nosync(pic32s->tx_irq);
+	dev_err(&pic32s->master->dev, "irq: disable all\n");
+}
+
+static irqreturn_t pic32_spi_fault_irq(int irq, void *dev_id)
+{
+	u32 status;
+	struct pic32_spi *pic32s = dev_id;
+
+	spin_lock(&pic32s->lock);
+
+	status = readl(pic32s->regs + SPISTAT);
+
+	if (status & (STAT_RX_OV | STAT_FRM_ERR | STAT_TX_UR)) {
+		writel(STAT_RX_OV, pic32s->regs + SPISTAT_CLR);
+		writel(STAT_TX_UR, pic32s->regs + SPISTAT_CLR);
+		pic32_err_stop(pic32s, "err_irq: fifo ov/ur-run\n");
+		goto irq_handled;
+	}
+
+	if (status & STAT_FRM_ERR) {
+		pic32_err_stop(pic32s, "err_irq: frame error");
+		goto irq_handled;
+	}
+
+	if (!pic32s->mesg) {
+		pic32_err_stop(pic32s, "err_irq: mesg error");
+		goto irq_handled;
+	}
+
+irq_handled:
+	spin_unlock(&pic32s->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t pic32_spi_rx_irq(int irq, void *dev_id)
+{
+	struct pic32_spi *pic32s = dev_id;
+
+	spin_lock(&pic32s->lock);
+
+	pic32s->rx_fifo(pic32s);
+
+	/* rx complete? */
+	if (pic32s->rx_end == pic32s->rx) {
+		disable_irq_nosync(pic32s->fault_irq);
+		disable_irq_nosync(pic32s->rx_irq);
+
+		complete(&pic32s->xfer_done);
+	}
+
+	spin_unlock(&pic32s->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t pic32_spi_tx_irq(int irq, void *dev_id)
+{
+	struct pic32_spi *pic32s = dev_id;
+
+	spin_lock(&pic32s->lock);
+
+	pic32s->tx_fifo(pic32s);
+
+	/* tx complete? mask and disable tx interrupt */
+	if (pic32s->tx_end == pic32s->tx)
+		disable_irq_nosync(pic32s->tx_irq);
+
+	spin_unlock(&pic32s->lock);
+
+	return IRQ_HANDLED;
+}
+
+static inline void pic32_spi_cs_assert(struct pic32_spi *pic32s)
+{
+	int cs_high;
+	struct spi_device *spi_dev = pic32s->spi_dev;
+
+	if (pic32s->flags & SPI_SS_MASTER)
+		return;
+
+	cs_high = pic32s->mode & SPI_CS_HIGH;
+	gpio_set_value(spi_dev->cs_gpio, cs_high);
+}
+
+static inline void pic32_spi_cs_deassert(struct pic32_spi *pic32s)
+{
+	int cs_high;
+	struct spi_device *spi_dev = pic32s->spi_dev;
+
+	if (pic32s->flags & SPI_SS_MASTER)
+		return;
+
+	cs_high = pic32s->mode & SPI_CS_HIGH;
+	gpio_set_value(spi_dev->cs_gpio, !cs_high);
+}
+
+static int pic32_spi_poll_transfer(struct pic32_spi *pic32s,
+				   unsigned long timeout)
+{
+	unsigned long deadline;
+
+	deadline = timeout + jiffies;
+	for (;;) {
+		pic32s->tx_fifo(pic32s);
+		cpu_relax();
+
+		pic32s->rx_fifo(pic32s);
+		cpu_relax();
+
+		/* received sufficient data */
+		if (pic32s->rx >= pic32s->rx_end)
+			break;
+
+		if (time_after_eq(jiffies, deadline))
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static bool pic32_spi_can_dma(struct spi_master *master,
+			      struct spi_device *spi,
+			      struct spi_transfer *xfer)
+{
+	struct pic32_spi *pic32s = spi_master_get_devdata(master);
+
+	return (xfer->len >= SPI_DMA_LEN_MIN) && (pic32s->flags & SPI_DMA_PREP);
+}
+
+static inline bool pic32_spi_dma_is_ready(struct pic32_spi *pic32s)
+{
+	int ret;
+
+	ret = (pic32s->flags & SPI_DMA_PREP) &&
+		(pic32s->len >= SPI_DMA_LEN_MIN) && (pic32s->tx && pic32s->rx);
+
+	if (ret)
+		pic32s->flags |= SPI_DMA_READY;
+
+	return ret;
+}
+
+static inline void pic32_spi_dma_unmap(struct pic32_spi *pic32s)
+{
+	pic32s->flags &= ~SPI_DMA_READY;
+}
+
+static void pic32_spi_dma_rx_notify(void *data)
+{
+	struct pic32_spi *pic32s = data;
+
+	spin_lock(&pic32s->lock);
+	complete(&pic32s->xfer_done);
+	spin_unlock(&pic32s->lock);
+}
+
+static inline void pic32_spi_dma_abort(struct pic32_spi *pic32s)
+{
+	if (!(pic32s->flags & SPI_DMA_READY))
+		return;
+
+	if (pic32s->master->dma_rx)
+		dmaengine_terminate_all(pic32s->master->dma_rx);
+
+	if (pic32s->master->dma_tx)
+		dmaengine_terminate_all(pic32s->master->dma_tx);
+
+	dev_err(&pic32s->master->dev, "%s, aborted\n", __func__);
+}
+
+static int pic32_spi_dma_transfer(struct pic32_spi *pic32s,
+				  struct spi_transfer *xfer)
+{
+	dma_cookie_t cookie;
+	struct spi_master *master = pic32s->master;
+	struct dma_async_tx_descriptor *desc_rx;
+	struct dma_async_tx_descriptor *desc_tx;
+
+	if (!master->dma_rx || !master->dma_tx)
+		return -ENODEV;
+
+	desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+					  xfer->rx_sg.sgl,
+					  xfer->rx_sg.nents,
+					  DMA_FROM_DEVICE,
+					  DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc_rx)
+		goto err_dma;
+
+	desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+					  xfer->tx_sg.sgl,
+					  xfer->tx_sg.nents,
+					  DMA_TO_DEVICE,
+					  DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc_tx)
+		goto err_dma;
+
+	dev_vdbg(&master->dev, "dma_xfer %p: len %u, tx %p(%p), rx %p(%p)\n",
+		 xfer, xfer->len,
+		 xfer->tx_buf, (void *)xfer->tx_dma,
+		 xfer->rx_buf, (void *)xfer->rx_dma);
+
+	/* Put callback on the RX transfer, that should finish last */
+	desc_rx->callback = pic32_spi_dma_rx_notify;
+	desc_rx->callback_param = pic32s;
+
+	cookie = dmaengine_submit(desc_rx);
+	if (dma_submit_error(cookie))
+		goto err_dma;
+
+	cookie = dmaengine_submit(desc_tx);
+	if (dma_submit_error(cookie))
+		goto err_dma;
+
+	dma_async_issue_pending(master->dma_rx);
+	dma_async_issue_pending(master->dma_tx);
+
+	return 0;
+
+err_dma:
+	pic32_spi_dma_abort(pic32s);
+	return -ENOMEM;
+}
+
+static int pic32_spi_dma_config(struct pic32_spi *pic32s, u32 dma_width)
+{
+	int err;
+	struct dma_slave_config cfg;
+	struct spi_master *master = pic32s->master;
+
+	cfg.device_fc		= true;
+	cfg.dst_addr		= pic32s->phys_base + SPIBUF;
+	cfg.src_addr		= pic32s->phys_base + SPIBUF;
+	cfg.dst_addr_width	= dma_width;
+	cfg.src_addr_width	= dma_width;
+	cfg.src_maxburst	= pic32s->fifo_n_elm >> 1; /* fill one-half */
+	cfg.dst_maxburst	= pic32s->fifo_n_elm >> 1; /* drain one-half */
+
+	cfg.slave_id = pic32s->tx_irq;
+	cfg.direction = DMA_MEM_TO_DEV;
+	err = dmaengine_slave_config(master->dma_tx, &cfg);
+	if (err) {
+		dev_err(&master->dev, "configure tx dma channel failed\n");
+		goto out;
+	}
+
+	cfg.slave_id = pic32s->rx_irq;
+	cfg.direction = DMA_DEV_TO_MEM;
+	err = dmaengine_slave_config(master->dma_rx, &cfg);
+	if (err)
+		dev_err(&master->dev, "configure rx dma channel failed\n");
+out:
+	return err;
+}
+
+static void pic32_spi_set_word_size(struct pic32_spi *pic32s, u8 bpw)
+{
+	u8 spi_bpw;
+	enum dma_slave_buswidth busw;
+
+	switch (bpw) {
+	default:
+	case 8:
+		pic32s->rx_fifo = pic32_spi_rx_byte;
+		pic32s->tx_fifo = pic32_spi_tx_byte;
+		spi_bpw		= SPI_BPW_8;
+		busw		= DMA_SLAVE_BUSWIDTH_1_BYTE;
+		break;
+	case 16:
+		pic32s->rx_fifo = pic32_spi_rx_word;
+		pic32s->tx_fifo = pic32_spi_tx_word;
+		spi_bpw		= SPI_BPW_16;
+		busw		= DMA_SLAVE_BUSWIDTH_2_BYTES;
+		break;
+	case 32:
+		pic32s->rx_fifo = pic32_spi_rx_dword;
+		pic32s->tx_fifo = pic32_spi_tx_dword;
+		spi_bpw		= SPI_BPW_32;
+		busw		= DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
+	}
+	spi_set_ws(pic32s, spi_bpw);
+
+	/* calculate maximum elements fifo can hold */
+	pic32s->fifo_n_elm = DIV_ROUND_UP(pic32s->fifo_n_byte, bpw >> 3);
+
+	/* re-configure dma width, if required */
+	if ((pic32s->flags & SPI_DMA_PREP) && (busw != pic32s->dma_width)) {
+		pic32_spi_dma_config(pic32s, busw);
+		pic32s->dma_width = busw;
+	}
+}
+
+static int pic32_spi_one_transfer(struct pic32_spi *pic32s,
+				  struct spi_message *message,
+				  struct spi_transfer *transfer)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct spi_master *master = pic32s->master;
+
+	/* set current transfer information */
+	pic32s->tx = (const void *)transfer->tx_buf;
+	pic32s->rx = (const void *)transfer->rx_buf;
+	pic32s->tx_end = pic32s->tx + transfer->len;
+	pic32s->rx_end = pic32s->rx + transfer->len;
+	pic32s->len = transfer->len;
+
+	if (transfer->speed_hz && (transfer->speed_hz != pic32s->speed_hz)) {
+		spi_set_clk_rate(pic32s, transfer->speed_hz);
+		pic32s->speed_hz = transfer->speed_hz;
+	}
+
+	if (transfer->bits_per_word)
+		pic32_spi_set_word_size(pic32s, transfer->bits_per_word);
+
+	spin_lock_irqsave(&pic32s->lock, flags);
+
+	spi_enable_chip(pic32s);
+
+	/* polling mode? */
+	if (pic32s->flags & SPI_XFER_POLL) {
+		ret = pic32_spi_poll_transfer(pic32s, 2 * HZ);
+		spin_unlock_irqrestore(&pic32s->lock, flags);
+
+		if (ret) {
+			dev_err(&master->dev, "poll-xfer timedout\n");
+			message->status = ret;
+			goto err_xfer_done;
+		}
+		goto out_xfer_done;
+	}
+
+	reinit_completion(&pic32s->xfer_done);
+
+	/* DMA mode ? */
+	if (pic32_spi_dma_is_ready(pic32s)) {
+		spin_unlock_irqrestore(&pic32s->lock, flags);
+
+		ret = pic32_spi_dma_transfer(pic32s, transfer);
+		if (ret) {
+			dev_err(&master->dev, "dma xfer error\n");
+			message->status = ret;
+			spin_lock_irqsave(&pic32s->lock, flags);
+		} else {
+			goto out_wait_for_xfer;
+		}
+	}
+
+	/* enable interrupt */
+	enable_irq(pic32s->fault_irq);
+	enable_irq(pic32s->tx_irq);
+	enable_irq(pic32s->rx_irq);
+
+	spin_unlock_irqrestore(&pic32s->lock, flags);
+
+out_wait_for_xfer:
+
+	/* wait for completion */
+	ret = wait_for_completion_timeout(&pic32s->xfer_done, 2 * HZ);
+	if (ret <= 0) {
+		dev_err(&master->dev, "wait timedout/interrupted\n");
+		ret = -EIO;
+		message->status = ret;
+		pic32_spi_dma_abort(pic32s);
+		goto err_xfer_done;
+	}
+
+out_xfer_done:
+	/* Update total byte transferred */
+	message->actual_length += transfer->len;
+	ret = 0;
+
+err_xfer_done:
+	pic32_spi_dma_unmap(pic32s);
+	return ret;
+}
+
+static int pic32_spi_one_message(struct spi_master *master,
+				 struct spi_message *msg)
+{
+	int ret = 0;
+	int cs_active = 0;
+	struct pic32_spi *pic32s;
+	struct spi_transfer *xfer;
+	struct spi_device *spi = msg->spi;
+
+	dev_vdbg(&spi->dev, "new message %p submitted for %s\n",
+		 msg, dev_name(&spi->dev));
+
+	pic32s = spi_master_get_devdata(master);
+
+	msg->status = 0;
+	msg->state = (void *)-2; /* running */
+	msg->actual_length = 0;
+
+	pic32s->mesg = msg;
+
+	spi_disable_chip(pic32s);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (!cs_active) {
+			pic32_spi_cs_assert(pic32s);
+			cs_active = 1;
+		}
+
+		ret = pic32_spi_one_transfer(pic32s, msg, xfer);
+		if (ret) {
+			dev_err(&spi->dev, "xfer %p err\n", xfer);
+			goto xfer_done;
+		}
+
+		/* handle delay, if asked */
+		if (xfer->delay_usecs)
+			udelay(xfer->delay_usecs);
+
+		/* handle cs-change
+		 * - for terminal transfer of the list skips CS deassertion.
+		 * - for others deassert CS temporarily, before starting next
+		 *   transfer CS will be asserted as usual.
+		 */
+		if (!xfer->cs_change)
+			continue;
+
+		cs_active = 0;
+		if (!list_is_last(&xfer->transfer_list, &msg->transfers)) {
+			pic32_spi_cs_deassert(pic32s);
+			continue;
+		}
+	}
+
+	msg->state = NULL;
+	msg->status = 0;
+
+xfer_done:
+	/* deassert cs */
+	if (cs_active)
+		pic32_spi_cs_deassert(pic32s);
+
+	spi_disable_chip(pic32s);
+
+	spi_finalize_current_message(spi->master);
+
+	return ret;
+}
+
+static void pic32_spi_cleanup(struct spi_device *spi)
+{
+	int cs_high;
+	struct pic32_spi *pic32s;
+
+	pic32s = spi_master_get_devdata(spi->master);
+
+	/* diasable chip */
+	spi_disable_chip(pic32s);
+
+	/* release cs-gpio */
+	if (!(pic32s->flags & SPI_SS_MASTER)) {
+		cs_high = pic32s->mode & SPI_CS_HIGH;
+		gpio_direction_output(spi->cs_gpio, !cs_high);
+		gpio_free(spi->cs_gpio);
+	}
+
+	/* reset reference */
+	pic32s->spi_dev = NULL;
+	pic32s->speed_hz = 0;
+}
+
+/* This may be called multiple times by same spi dev */
+static int pic32_spi_setup(struct spi_device *spi)
+{
+	struct pic32_spi *pic32s;
+	int cs_high;
+	int ret;
+
+	pic32s = spi_master_get_devdata(spi->master);
+
+	/* SPI master supports only one spi-device at a time.
+	 * So multiple spi_setup() to different devices is not allowed.
+	 */
+	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {
+		dev_err(&spi->dev, "spi-master already associated with %s\n",
+			dev_name(&pic32s->spi_dev->dev));
+		return -EPERM;
+	}
+
+	if (pic32s->spi_dev == spi)
+		pic32_spi_cleanup(spi);
+
+	pic32s->spi_dev = spi;
+
+	/* set POLL mode, if invalid irq is provided */
+	if ((pic32s->fault_irq <= 0) || (pic32s->rx_irq <= 0) ||
+	    (pic32s->tx_irq <= 0))
+		pic32s->flags |= SPI_XFER_POLL;
+
+	if (!spi->bits_per_word) {
+		dev_err(&spi->dev, "No bits_per_word defined\n");
+		return -EINVAL;
+	}
+
+	if (!spi->max_speed_hz) {
+		dev_err(&spi->dev, "No max speed HZ parameter\n");
+		return -EINVAL;
+	}
+
+	spi_disable_chip(pic32s);
+
+	pic32_spi_set_word_size(pic32s, spi->bits_per_word);
+
+	pic32s->speed_hz = spi->max_speed_hz;
+	spi_set_clk_rate(pic32s, spi->max_speed_hz);
+
+	/* set spi mode */
+	spi_set_clk_mode(pic32s, spi->mode);
+	pic32s->mode = spi->mode;
+
+	/* configure master-ctl CS assert/deassert, if cs_gpio is invalid */
+	pic32s->flags |= SPI_SS_MASTER;
+
+	cs_high = pic32s->mode & SPI_CS_HIGH;
+	if (gpio_is_valid(spi->cs_gpio)) {
+		ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
+		if (!ret) {
+			gpio_direction_output(spi->cs_gpio, !cs_high);
+			pic32s->flags &= ~SPI_SS_MASTER;
+			dev_vdbg(&spi->dev,
+				 "gpio-%d configured for spics (%s)\n",
+				 spi->cs_gpio, cs_high ? "cs_high" : "cs_low");
+		}
+	}
+	spi_set_ss_auto(pic32s, pic32s->flags & SPI_SS_MASTER, spi->mode);
+
+	dev_vdbg(&spi->master->dev,
+		 "successfully registered spi-device %s\n",
+		 dev_name(&spi->dev));
+	return 0;
+}
+
+static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev)
+{
+	int err;
+	struct spi_master *master = pic32s->master;
+	dma_cap_mask_t mask;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	master->dma_rx = dma_request_slave_channel_compat(mask, NULL, NULL,
+							  dev, "spi-rx");
+	if (!master->dma_rx) {
+		dev_err(dev, "RX channel not found, SPI unable to use DMA\n");
+		err = -EBUSY;
+		goto out_err;
+	}
+
+	master->dma_tx = dma_request_slave_channel_compat(mask, NULL, NULL,
+							  dev, "spi-tx");
+	if (!master->dma_tx) {
+		dev_err(dev, "TX channel not found, SPI unable to use DMA\n");
+		err = -EBUSY;
+		goto out_err;
+	}
+
+	pic32s->dma_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	err = pic32_spi_dma_config(pic32s, pic32s->dma_width);
+	if (err)
+		goto out_err;
+
+	/* DMA chnls allocated & prepared */
+	pic32s->flags |= SPI_DMA_PREP;
+
+	dev_dbg(dev, "Using %s (tx) and %s (rx) for DMA transfers\n",
+		dma_chan_name(master->dma_tx),
+		dma_chan_name(master->dma_rx));
+	return 0;
+
+out_err:
+	if (master->dma_rx)
+		dma_release_channel(master->dma_rx);
+
+	if (master->dma_tx)
+		dma_release_channel(master->dma_tx);
+
+	return err;
+}
+
+static void pic32_spi_dma_unprep(struct pic32_spi *pic32s)
+{
+	if (!(pic32s->flags & SPI_DMA_PREP))
+		return;
+
+	pic32s->flags &= ~SPI_DMA_PREP;
+	if (pic32s->master->dma_rx)
+		dma_release_channel(pic32s->master->dma_rx);
+
+	if (pic32s->master->dma_tx)
+		dma_release_channel(pic32s->master->dma_tx);
+}
+
+static void pic32_spi_hw_init(struct pic32_spi *pic32s)
+{
+	u32 v;
+
+	/* disable module */
+	spi_disable_chip(pic32s);
+
+	/* drain rx buf */
+	spi_drain_rx_buf(pic32s);
+
+	v = readl(pic32s->regs + SPICON);
+
+	/* enable fifo: fifo-depth is fixed to 128bit(= 16B) */
+	v |= SPICON_ENHBUF;
+	pic32s->fifo_n_byte = 16;
+
+	/* disable framing mode */
+	v &= ~SPICON_FRMEN;
+
+	/* enable master mode while disabled */
+	v |= SPICON_MSTEN;
+
+	/* set tx fifo threshold interrupt */
+	v &= ~(0x3 << SPICON_TXI_SHIFT);
+	v |= (SPI_TX_FIFO_HALF_EMPTY << SPICON_TXI_SHIFT);
+
+	/* set rx fifo threshold interrupt */
+	v &= ~(0x3 << SPICON_RXI_SHIFT);
+	v |= (SPI_RX_FIFO_NOT_EMPTY << SPICON_RXI_SHIFT);
+
+	writel(v, pic32s->regs + SPICON);
+
+	spi_set_err_int(pic32s);
+}
+
+static int pic32_spi_hw_probe(struct platform_device *pdev,
+			      struct pic32_spi *pic32s)
+{
+	int ret, clk_id = SPI_CLKSRC_PBCLK;
+	struct resource *mem;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pic32s->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (!pic32s->regs) {
+		dev_err(&pdev->dev, "mem map failed\n");
+		return -ENOMEM;
+	}
+	pic32s->phys_base = mem->start;
+
+	/* get irq resources: err-irq, rx-irq, tx-irq */
+	pic32s->fault_irq = platform_get_irq_byname(pdev, "fault");
+	if (pic32s->fault_irq < 0)
+		dev_warn(&pdev->dev, "no fault-irq supplied\n");
+
+	pic32s->rx_irq = platform_get_irq_byname(pdev, "rx");
+	if (pic32s->rx_irq < 0)
+		dev_warn(&pdev->dev, "no rx-irq supplied\n");
+
+	pic32s->tx_irq = platform_get_irq_byname(pdev, "tx");
+	if (pic32s->tx_irq < 0)
+		dev_warn(&pdev->dev, "no tx-irq supplied\n");
+
+	pic32_spi_hw_init(pic32s);
+
+	/* any one of the two clk sources is mandatory; pbxclk:0, m_clk:1 */
+	pic32s->clk = devm_clk_get(&pdev->dev, "mck0");
+	if (IS_ERR(pic32s->clk)) {
+		pic32s->clk = devm_clk_get(&pdev->dev, "mck1");
+		if (IS_ERR(pic32s->clk)) {
+			ret = PTR_ERR(pic32s->clk);
+			dev_err(&pdev->dev, "no clk supplied\n");
+			goto err_unmap_mem;
+		}
+		clk_id = SPI_CLKSRC_MCLK;
+	}
+
+	clk_prepare_enable(pic32s->clk);
+
+	/* Select clk src */
+	spi_set_clk(pic32s, clk_id);
+
+	return 0;
+
+err_unmap_mem:
+	dev_err(&pdev->dev, "hw_probe failed, ret %d\n", ret);
+	return ret;
+}
+
+static int pic32_spi_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct spi_master *master;
+	struct pic32_spi *pic32s;
+	struct device *dev = &pdev->dev;
+	u32 max_spi_rate = 50000000;
+
+	master = spi_alloc_master(dev, sizeof(*pic32s));
+	if (!master)
+		return -ENOMEM;
+
+	pic32s = spi_master_get_devdata(master);
+	pic32s->master = master;
+	pic32s->flags = SPI_DMA_CAP;
+
+	ret = pic32_spi_hw_probe(pdev, pic32s);
+	if (ret) {
+		dev_err(&pdev->dev, "hw probe failed\n");
+		goto err_free_master;
+	}
+
+	if (dev->of_node) {
+		of_property_read_u32(dev->of_node,
+				     "max-clock-frequency", &max_spi_rate);
+
+		if (of_find_property(dev->of_node, "use-no-dma", NULL)) {
+			dev_warn(dev, "DMA support not requested\n");
+			pic32s->flags &= ~SPI_DMA_CAP;
+		}
+	}
+
+	if (pic32s->flags & SPI_DMA_CAP) {
+		ret = pic32_spi_dma_prep(pic32s, dev);
+		if (ret)
+			dev_warn(dev, "DMA support kept disabled\n");
+	}
+
+	master->dev.of_node	= of_node_get(pdev->dev.of_node);
+	master->mode_bits	= SPI_MODE_3 | SPI_MODE_0 | SPI_CS_HIGH;
+	master->num_chipselect	= 1;
+	master->max_speed_hz	= max_spi_rate;
+	master->setup		= pic32_spi_setup;
+	master->cleanup		= pic32_spi_cleanup;
+	master->flags		= SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX;
+	master->bits_per_word_mask	= SPI_BPW_RANGE_MASK(8, 32);
+	master->transfer_one_message	= pic32_spi_one_message;
+	if (pic32s->flags & SPI_DMA_PREP)
+		master->can_dma	= pic32_spi_can_dma;
+
+	init_completion(&pic32s->xfer_done);
+	spin_lock_init(&pic32s->lock);
+
+	irq_set_status_flags(pic32s->fault_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, pic32s->fault_irq,
+			       pic32_spi_fault_irq, IRQF_NO_THREAD,
+			       dev_name(dev), pic32s);
+	if (ret < 0) {
+		dev_warn(dev, "request fault-irq %d\n", pic32s->rx_irq);
+		pic32s->flags |= SPI_XFER_POLL;
+		goto irq_request_done;
+	}
+
+	irq_set_status_flags(pic32s->rx_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, pic32s->rx_irq,
+			       pic32_spi_rx_irq, IRQF_NO_THREAD, dev_name(dev),
+			       pic32s);
+	if (ret < 0) {
+		dev_warn(dev, "request rx-irq %d\n", pic32s->rx_irq);
+		pic32s->flags |= SPI_XFER_POLL;
+		goto irq_request_done;
+	}
+
+	irq_set_status_flags(pic32s->tx_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, pic32s->tx_irq,
+			       pic32_spi_tx_irq, IRQF_NO_THREAD, dev_name(dev),
+			       pic32s);
+	if (ret < 0) {
+		dev_warn(dev, "request tx-irq %d\n", pic32s->tx_irq);
+		pic32s->flags |= SPI_XFER_POLL;
+		goto irq_request_done;
+	}
+
+irq_request_done:
+	ret = devm_spi_register_master(dev, master);
+	if (ret) {
+		dev_err(&master->dev, "failed registering spi master\n");
+		goto err_hw_remove;
+	}
+
+	platform_set_drvdata(pdev, pic32s);
+
+	return 0;
+
+err_hw_remove:
+	spi_disable_chip(pic32s);
+
+	if (!IS_ERR_OR_NULL(pic32s->clk))
+		clk_disable_unprepare(pic32s->clk);
+err_free_master:
+	spi_master_put(master);
+	return ret;
+}
+
+static int pic32_spi_remove(struct platform_device *pdev)
+{
+	struct pic32_spi *pic32s;
+
+	pic32s = platform_get_drvdata(pdev);
+
+	spi_disable_chip(pic32s);
+	clk_disable_unprepare(pic32s->clk);
+	pic32_spi_dma_unprep(pic32s);
+
+	return 0;
+}
+
+static const struct of_device_id pic32_spi_of_match[] = {
+	{.compatible = "microchip,pic32mzda-spi",},
+	{},
+};
+MODULE_DEVICE_TABLE(of, pic32_spi_of_match);
+
+static struct platform_driver pic32_spi_driver = {
+	.driver = {
+		.name = "spi-pic32",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(pic32_spi_of_match),
+	},
+	.probe = pic32_spi_probe,
+	.remove = pic32_spi_remove,
+};
+
+module_platform_driver(pic32_spi_driver);
+
+MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
+MODULE_DESCRIPTION("Microchip PIC32 SPI Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 9303 bytes --]

On Mon, Feb 01, 2016 at 03:44:57PM -0700, Joshua Henderson wrote:

> The PIC32 SPI driver is capable of performing SPI transfers either using
> polling or based on interrupt-trigger. GPIO based CS support (necessary
> for correct SPI operation) is available. It is dependent on availability
> of "cs-gpios" property of spi node in board dts file.

There's quite a lot of issues here.  At a high level the big issues are
with duplicating core functionality, a large number of helper functions
that don't seem to add much and some interesting locking.  There's also
no DT binding documentation which is mandatory for new bindings.

> +config SPI_PIC32
> +	tristate "Microchip PIC32 series SPI"
> +	depends on MACH_PIC32
> +	help
> +	  SPI driver for PIC32 SPI master controller.
> +
> +
>  #
>  # Add new SPI master controllers in alphabetical order above this line
>  #

Note the comment here (yes, there are some things out of order but
that's no reason to add more).  Please also add an || COMPILE_TEST
unless there's an actual build time dependency.

> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 8991ffc..bb37fed 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -94,3 +94,4 @@ obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
>  obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
>  obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>  obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
> +obj-$(CONFIG_SPI_PIC32)			+= spi-pic32.o

Similarly here where things are better sorted already.

> +/*
> + * PIC32 SPI core controller driver (refer dw_spi.c)

What should we refer to dw_spi.c for?  If this is a Designware IP you
should share the same driver.

> +static inline void spi_disable_chip(struct pic32_spi *pic32s)
> +{
> +	writel(SPICON_ON, pic32s->regs + SPICON_CLR);
> +	cpu_relax();
> +}

What is the cpu_relax() intended to do here?

> +static inline void spi_set_clk_mode(struct pic32_spi *pic32s, int mode)
> +{
> +	u32 conset = 0, conclr = 0;
> +
> +	/* active low */
> +	if (mode & SPI_CPOL)
> +		conset |= SPICON_CKP;
> +	else
> +		conclr |= SPICON_CKP;
> +
> +	/* tx on rising edge of clk */
> +	if (mode & SPI_CPHA)
> +		conclr |= SPICON_CKE;
> +	else
> +		conset |= SPICON_CKE;
> +
> +	/* rx at end of tx */
> +	conset |= SPICON_SMP;
> +
> +	writel(conclr, pic32s->regs + SPICON_CLR);
> +	writel(conset, pic32s->regs + SPICON_SET);
> +}

This is large for an inline function and only has one caller anyway.
Why not just include it in that user?

> +static inline void spi_drain_rx_buf(struct pic32_spi *pic32s)
> +{
> +	u32 sr;
> +
> +	/* drain rx bytes until empty */
> +	for (;;) {
> +		sr = readl(pic32s->regs + SPISTAT);
> +		if (sr & STAT_RF_EMPTY)
> +			break;
> +
> +		(void)readl(pic32s->regs + SPIBUF);

Why do you need this cast to void?

> +	}

A busy waiting loop like this is more where I'd expect to see a
cpu_relax().  I'd also expect to see some kind of timeout here,
otherwise if something goes wrong we could lock up.

> +static inline void spi_set_clk_rate(struct pic32_spi *pic32s, u32 sck)

Essentially all the functions in this file appear to be marked as
inline.  Don't do this unless the function really *needs* to be inline,
just let the compiler inline it if it figures out that it's a good idea.

> +static inline void spi_set_ss_auto(struct pic32_spi *pic32s, u8 mst, u32 mode)
> +{
> +	u32 v;
> +
> +	/* spi controller can drive CS/SS during transfer depending on fifo
> +	 * fill-level. SS will stay asserted as long as TX fifo has something
> +	 * to transfer, else will be deasserted confirming completion of
> +	 * the ongoing transfer.
> +	 */
> +
> +	v = readl(pic32s->regs + SPICON);
> +	v &= ~SPICON_MSSEN;
> +	if (mst) {
> +		v |= SPICON_MSSEN;
> +		if (mode & SPI_CS_HIGH)
> +			v |= SPICON_FRMPOL;
> +		else
> +			v &= ~SPICON_FRMPOL;
> +	}
> +
> +	writel(v, pic32s->regs + SPICON);
> +}

I don't really know what the above is suposed to do just from looking at
it.  If it's enabling automatic /CS managment that doesn't sound like
something we can actually use in Linux based on the comment.

> +static inline void spi_set_err_int(struct pic32_spi *pic32s)
> +{
> +	u32 mask;
> +
> +	mask = SPI_INT_TX_UR_EN | SPI_INT_RX_OV_EN | SPI_INT_FRM_ERR_EN;
> +	writel(mask, pic32s->regs + SPICON2_SET);
> +}

There is no need for this to be a function - there is one caller and
it contains only a single function.  Just inline it and add a comment if
there is a need for documentation.  This applies to quite a lot of the
functions in this file, they are very small and have only one user so
mostly just make the code less direct.  Using spi_ as the prefix also
has a namespacing issue, that's the namespace for the core.

> +	pic32s->mesg->state = (void *)-1;

This is trying to munge a numeric return code into a pointer without
using PTR_ERR() and friends which is ugly and fragile.  If you really
need to do this use the standard macros, but since we already have an
error code in the message you can use that (though it seems nothing
actually ever reads these...).  You should also use real error codes not
magic numbers.

> +	complete(&pic32s->xfer_done);
> +
> +	disable_irq_nosync(pic32s->fault_irq);
> +	disable_irq_nosync(pic32s->rx_irq);
> +	disable_irq_nosync(pic32s->tx_irq);

This is racy, you are completing the transfer then disabling the
interrupts.  Something could come along and try and do another transfer
before the interrupts are disabled.

> +	if (!pic32s->mesg) {
> +		pic32_err_stop(pic32s, "err_irq: mesg error");
> +		goto irq_handled;
> +	}

There's nothing in this case that checks that the interrupt actually
fired.

> +	/* tx complete? mask and disable tx interrupt */
> +	if (pic32s->tx_end == pic32s->tx)
> +		disable_irq_nosync(pic32s->tx_irq);

It seems there's no interrupt masking in the actual IP and the
interrupts will scream when the IP is idle?

> +static inline void pic32_spi_cs_assert(struct pic32_spi *pic32s)
> +{
> +	int cs_high;
> +	struct spi_device *spi_dev = pic32s->spi_dev;
> +
> +	if (pic32s->flags & SPI_SS_MASTER)
> +		return;
> +
> +	cs_high = pic32s->mode & SPI_CS_HIGH;
> +	gpio_set_value(spi_dev->cs_gpio, cs_high);
> +}

Don't open code GPIO chip selects, use the core support.

> +static void pic32_spi_dma_rx_notify(void *data)
> +{
> +	struct pic32_spi *pic32s = data;
> +
> +	spin_lock(&pic32s->lock);
> +	complete(&pic32s->xfer_done);
> +	spin_unlock(&pic32s->lock);
> +}

Taking the spinlock here looks completely pointless - what is it
intended to protect?

> +err_dma:
> +	pic32_spi_dma_abort(pic32s);
> +	return -ENOMEM;

-ENOMEM looks completely random here, please use a sensible error code
(if you got one back from another funtion pass it through).

> +static int pic32_spi_one_transfer(struct pic32_spi *pic32s,
> +				  struct spi_message *message,
> +				  struct spi_transfer *transfer)

> +	if (transfer->speed_hz && (transfer->speed_hz != pic32s->speed_hz)) {

Just use the transfer, the core will ensure that it is set.

> +	spin_lock_irqsave(&pic32s->lock, flags);
> +
> +	spi_enable_chip(pic32s);
> +
> +	/* polling mode? */
> +	if (pic32s->flags & SPI_XFER_POLL) {
> +		ret = pic32_spi_poll_transfer(pic32s, 2 * HZ);
> +		spin_unlock_irqrestore(&pic32s->lock, flags);
> +
> +		if (ret) {
> +			dev_err(&master->dev, "poll-xfer timedout\n");
> +			message->status = ret;
> +			goto err_xfer_done;
> +		}
> +		goto out_xfer_done;
> +	}
> +
> +	reinit_completion(&pic32s->xfer_done);
> +
> +	/* DMA mode ? */
> +	if (pic32_spi_dma_is_ready(pic32s)) {
> +		spin_unlock_irqrestore(&pic32s->lock, flags);
> +
> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
> +		if (ret) {
> +			dev_err(&master->dev, "dma xfer error\n");
> +			message->status = ret;
> +			spin_lock_irqsave(&pic32s->lock, flags);
> +		} else {
> +			goto out_wait_for_xfer;
> +		}
> +	}
> +
> +	/* enable interrupt */
> +	enable_irq(pic32s->fault_irq);
> +	enable_irq(pic32s->tx_irq);
> +	enable_irq(pic32s->rx_irq);
> +
> +	spin_unlock_irqrestore(&pic32s->lock, flags);

This seems like a very large section of code to hold a spinlock for and
I'm not immediately seeing a reason why the locking has to be so
aggressive.  Among other things it's holding a spinlock with interrupts
disabled for up to two seconds over a polling transfer which is at best
needless.

> +static int pic32_spi_one_message(struct spi_master *master,
> +				 struct spi_message *msg)
> +{
> +	int ret = 0;
> +	int cs_active = 0;

Don't open code message parsing, use the core support and implement
transfer_one().

> +	clk_prepare_enable(pic32s->clk);

No error checking.  It'd also be better to have runtime power management
of this.

> +	if (dev->of_node) {

This has DT support but the DT binding is not documented.  Documentation
is mandatory for all new DT bindings.

> +		of_property_read_u32(dev->of_node,
> +				     "max-clock-frequency", &max_spi_rate);

This looks like it's duplicating something that should be in the clock
bindings.  As far as I can tell we never change the parent clock rate so
we can just look at that to identify the maximum clock rate.

> +		if (of_find_property(dev->of_node, "use-no-dma", NULL)) {
> +			dev_warn(dev, "DMA support not requested\n");
> +			pic32s->flags &= ~SPI_DMA_CAP;
> +		}

Why not just handle the DT binding information being missing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

Hi Mark,

On 02/02/2016 05:43 PM, Mark Brown wrote:

> On Mon, Feb 01, 2016 at 03:44:57PM -0700, Joshua Henderson wrote:
>
>> The PIC32 SPI driver is capable of performing SPI transfers either using
>> polling or based on interrupt-trigger. GPIO based CS support (necessary
>> for correct SPI operation) is available. It is dependent on availability
>> of "cs-gpios" property of spi node in board dts file.
> There's quite a lot of issues here.  At a high level the big issues are
> with duplicating core functionality, a large number of helper functions
> that don't seem to add much and some interesting locking.  There's also
> no DT binding documentation which is mandatory for new bindings.

Good summary, will remove the helper functions wherever required. Also fix
locking issue, and implement transfer_one().

Please find the binding documentation (missed adding you in CC).
https://patchwork.ozlabs.org/patch/576738/

>> +config SPI_PIC32
>> +	tristate "Microchip PIC32 series SPI"
>> +	depends on MACH_PIC32
>> +	help
>> +	  SPI driver for PIC32 SPI master controller.
>> +
>> +
>>  #
>>  # Add new SPI master controllers in alphabetical order above this line
>>  #
> Note the comment here (yes, there are some things out of order but
> that's no reason to add more).  Please also add an || COMPILE_TEST
> unless there's an actual build time dependency.

Ack, Will add in alphabetical order and add || COMPILE_TEST.

>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index 8991ffc..bb37fed 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -94,3 +94,4 @@ obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
>>  obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
>>  obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>>  obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>> +obj-$(CONFIG_SPI_PIC32)			+= spi-pic32.o
> Similarly here where things are better sorted already.

ack, Will add in sorted order.

>> +/*
>> + * PIC32 SPI core controller driver (refer dw_spi.c)
> What should we refer to dw_spi.c for?  If this is a Designware IP you
> should share the same driver.

This is Microchip proprietary SPI IP, not Designware. Reference to dw_spi.c here
is to mention 'inspired by'. Will update comment accordingly.

>> +static inline void spi_disable_chip(struct pic32_spi *pic32s)
>> +{
>> +	writel(SPICON_ON, pic32s->regs + SPICON_CLR);
>> +	cpu_relax();
>> +}
> What is the cpu_relax() intended to do here?

No SPI registers may be read or written in the CPU cycle immediately following
the instruction that clears the module’s ON bit.
Is there any better alternative than this? or udelay()? anyway, will add comment why this is here.

>> +static inline void spi_set_clk_mode(struct pic32_spi *pic32s, int mode)
>> +{
>> +	u32 conset = 0, conclr = 0;
>> +
>> +	/* active low */
>> +	if (mode & SPI_CPOL)
>> +		conset |= SPICON_CKP;
>> +	else
>> +		conclr |= SPICON_CKP;
>> +
>> +	/* tx on rising edge of clk */
>> +	if (mode & SPI_CPHA)
>> +		conclr |= SPICON_CKE;
>> +	else
>> +		conset |= SPICON_CKE;
>> +
>> +	/* rx at end of tx */
>> +	conset |= SPICON_SMP;
>> +
>> +	writel(conclr, pic32s->regs + SPICON_CLR);
>> +	writel(conset, pic32s->regs + SPICON_SET);
>> +}
> This is large for an inline function and only has one caller anyway.
> Why not just include it in that user?

ack. Merge accordingly,

>> +static inline void spi_drain_rx_buf(struct pic32_spi *pic32s)
>> +{
>> +	u32 sr;
>> +
>> +	/* drain rx bytes until empty */
>> +	for (;;) {
>> +		sr = readl(pic32s->regs + SPISTAT);
>> +		if (sr & STAT_RF_EMPTY)
>> +			break;
>> +
>> +		(void)readl(pic32s->regs + SPIBUF);
> Why do you need this cast to void?

Will drop.

>> +	}
> A busy waiting loop like this is more where I'd expect to see a
> cpu_relax().  I'd also expect to see some kind of timeout here,
> otherwise if something goes wrong we could lock up.

Will add usleep_range() and timeout check to avoid unbounded wait.

>> +static inline void spi_set_clk_rate(struct pic32_spi *pic32s, u32 sck)
> Essentially all the functions in this file appear to be marked as
> inline.  Don't do this unless the function really *needs* to be inline,
> just let the compiler inline it if it figures out that it's a good idea.

Will remove 'inline' keyword in larger functions.
Also as per my understanding compiler ignores 'inline' hint if it finds otherwise.

>> +static inline void spi_set_ss_auto(struct pic32_spi *pic32s, u8 mst, u32 mode)
>> +{
>> +	u32 v;
>> +
>> +	/* spi controller can drive CS/SS during transfer depending on fifo
>> +	 * fill-level. SS will stay asserted as long as TX fifo has something
>> +	 * to transfer, else will be deasserted confirming completion of
>> +	 * the ongoing transfer.
>> +	 */
>> +
>> +	v = readl(pic32s->regs + SPICON);
>> +	v &= ~SPICON_MSSEN;
>> +	if (mst) {
>> +		v |= SPICON_MSSEN;
>> +		if (mode & SPI_CS_HIGH)
>> +			v |= SPICON_FRMPOL;
>> +		else
>> +			v &= ~SPICON_FRMPOL;
>> +	}
>> +
>> +	writel(v, pic32s->regs + SPICON);
>> +}
> I don't really know what the above is suposed to do just from looking at
> it.  If it's enabling automatic /CS managment that doesn't sound like
> something we can actually use in Linux based on the comment.

The functions enables/disables automatic /CS management depending on 'mst'
argument which is govern by presence of valid cs_gpio.
Now we know, clearly, the SPI IP works correct only with GPIO controlled
/CS management. So will remove this logic and force disable-automatic /CS  handling.

>> +static inline void spi_set_err_int(struct pic32_spi *pic32s)
>> +{
>> +	u32 mask;
>> +
>> +	mask = SPI_INT_TX_UR_EN | SPI_INT_RX_OV_EN | SPI_INT_FRM_ERR_EN;
>> +	writel(mask, pic32s->regs + SPICON2_SET);
>> +}
> There is no need for this to be a function - there is one caller and
> it contains only a single function.  Just inline it and add a comment if
> there is a need for documentation.  This applies to quite a lot of the
> functions in this file, they are very small and have only one user so
> mostly just make the code less direct.  Using spi_ as the prefix also
> has a namespacing issue, that's the namespace for the core.

ack. Will replace accordingly.

>> +	pic32s->mesg->state = (void *)-1;
> This is trying to munge a numeric return code into a pointer without
> using PTR_ERR() and friends which is ugly and fragile.  If you really
> need to do this use the standard macros, but since we already have an
> error code in the message you can use that (though it seems nothing
> actually ever reads these...).  You should also use real error codes not
> magic numbers.

Ack, Will implement transfer_one() and there will be no need of handling
message state at all.

>> +	complete(&pic32s->xfer_done);
>> +
>> +	disable_irq_nosync(pic32s->fault_irq);
>> +	disable_irq_nosync(pic32s->rx_irq);
>> +	disable_irq_nosync(pic32s->tx_irq);
> This is racy, you are completing the transfer then disabling the
> interrupts.  Something could come along and try and do another transfer
> before the interrupts are disabled.

Good catch. Will change the order.

>> +	if (!pic32s->mesg) {
>> +		pic32_err_stop(pic32s, "err_irq: mesg error");
>> +		goto irq_handled;
>> +	}
> There's nothing in this case that checks that the interrupt actually
> fired.

ack. Will return with IRQ_NONE in this case.

>> +	/* tx complete? mask and disable tx interrupt */
>> +	if (pic32s->tx_end == pic32s->tx)
>> +		disable_irq_nosync(pic32s->tx_irq);
> It seems there's no interrupt masking in the actual IP and the
> interrupts will scream when the IP is idle?
>
At IP level there is no interrupt masking capability. So to disable
firing interrupt when IP is idle interrupts are masked/disabled at
interrupt controller level. 

>> +static inline void pic32_spi_cs_assert(struct pic32_spi *pic32s)
>> +{
>> +	int cs_high;
>> +	struct spi_device *spi_dev = pic32s->spi_dev;
>> +
>> +	if (pic32s->flags & SPI_SS_MASTER)
>> +		return;
>> +
>> +	cs_high = pic32s->mode & SPI_CS_HIGH;
>> +	gpio_set_value(spi_dev->cs_gpio, cs_high);
>> +}
> Don't open code GPIO chip selects, use the core support.

ack. Will implement .transfer_one() callback and use SPI core support.

>> +static void pic32_spi_dma_rx_notify(void *data)
>> +{
>> +	struct pic32_spi *pic32s = data;
>> +
>> +	spin_lock(&pic32s->lock);
>> +	complete(&pic32s->xfer_done);
>> +	spin_unlock(&pic32s->lock);
>> +}
> Taking the spinlock here looks completely pointless - what is it
> intended to protect?

Will drop.

>> +err_dma:
>> +	pic32_spi_dma_abort(pic32s);
>> +	return -ENOMEM;
> -ENOMEM looks completely random here, please use a sensible error code
> (if you got one back from another funtion pass it through).

ack. Will use same error code as returned by failed function call.

>> +static int pic32_spi_one_transfer(struct pic32_spi *pic32s,
>> +				  struct spi_message *message,
>> +				  struct spi_transfer *transfer)
>> +	if (transfer->speed_hz && (transfer->speed_hz != pic32s->speed_hz)) {
> Just use the transfer, the core will ensure that it is set.

ack.

>> +	spin_lock_irqsave(&pic32s->lock, flags);
>> +
>> +	spi_enable_chip(pic32s);
>> +
>> +	/* polling mode? */
>> +	if (pic32s->flags & SPI_XFER_POLL) {
>> +		ret = pic32_spi_poll_transfer(pic32s, 2 * HZ);
>> +		spin_unlock_irqrestore(&pic32s->lock, flags);
>> +
>> +		if (ret) {
>> +			dev_err(&master->dev, "poll-xfer timedout\n");
>> +			message->status = ret;
>> +			goto err_xfer_done;
>> +		}
>> +		goto out_xfer_done;
>> +	}
>> +
>> +	reinit_completion(&pic32s->xfer_done);
>> +
>> +	/* DMA mode ? */
>> +	if (pic32_spi_dma_is_ready(pic32s)) {
>> +		spin_unlock_irqrestore(&pic32s->lock, flags);
>> +
>> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
>> +		if (ret) {
>> +			dev_err(&master->dev, "dma xfer error\n");
>> +			message->status = ret;
>> +			spin_lock_irqsave(&pic32s->lock, flags);
>> +		} else {
>> +			goto out_wait_for_xfer;
>> +		}
>> +	}
>> +
>> +	/* enable interrupt */
>> +	enable_irq(pic32s->fault_irq);
>> +	enable_irq(pic32s->tx_irq);
>> +	enable_irq(pic32s->rx_irq);
>> +
>> +	spin_unlock_irqrestore(&pic32s->lock, flags);
> This seems like a very large section of code to hold a spinlock for and
> I'm not immediately seeing a reason why the locking has to be so
> aggressive.  Among other things it's holding a spinlock with interrupts
> disabled for up to two seconds over a polling transfer which is at best
> needless.

ack. Will remove locking requirement for polling-mode (needed to support
automatic /CS handling).

>> +static int pic32_spi_one_message(struct spi_master *master,
>> +				 struct spi_message *msg)
>> +{
>> +	int ret = 0;
>> +	int cs_active = 0;
> Don't open code message parsing, use the core support and implement
> transfer_one().

ack. Will implement.

>> +	clk_prepare_enable(pic32s->clk);
> No error checking.  It'd also be better to have runtime power management
> of this.

Will add error checking. Currently we don't have support for runtime power-management.

>> +	if (dev->of_node) {
> This has DT support but the DT binding is not documented.  Documentation
> is mandatory for all new DT bindings.

ack. See above for DT documentation.

>> +		of_property_read_u32(dev->of_node,
>> +				     "max-clock-frequency", &max_spi_rate);
> This looks like it's duplicating something that should be in the clock
> bindings.  As far as I can tell we never change the parent clock rate so
> we can just look at that to identify the maximum clock rate.

ack. Will drop this and derive maximum clock rate from the parent clock.

>> +		if (of_find_property(dev->of_node, "use-no-dma", NULL)) {
>> +			dev_warn(dev, "DMA support not requested\n");
>> +			pic32s->flags &= ~SPI_DMA_CAP;
>> +		}
> Why not just handle the DT binding information being missing?

Will drop this DT property (added mainly for debug purpose).


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html