* [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral
@ 2016-02-01 22:44 Joshua Henderson
2016-02-01 22:44 ` [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver Joshua Henderson
2016-02-02 11:11 ` [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Sergei Shtylyov
0 siblings, 2 replies; 8+ messages in thread
From: Joshua Henderson @ 2016-02-01 22:44 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, Purna Chandra Mandal, Joshua Henderson, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree
From: Purna Chandra Mandal <purna.mandal@microchip.com>
Document the devicetree bindings for the SPI peripheral found
on Microchip PIC32 class devices.
Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
---
.../bindings/spi/microchip,spi-pic32.txt | 44 ++++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
new file mode 100644
index 0000000..a555618
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
@@ -0,0 +1,44 @@
+* Microchip PIC32 SPI device
+
+Required properties:
+- compatible: Should be "microchip,pic32mzda-spi".
+- reg: Address and length of the register set for the device
+- interrupts: Should contain all three spi interrupts in sequence
+ of <fault-irq>, <receive-irq>, <transmit-irq>.
+- interrupt-names: Should be "fault","rx","tx" in order.
+- clocks: phandles of baud generation clocks. Maximum two possible clocks
+ can be provided (&PBCLK2, &REFCLKO1).
+ See: Documentation/devicetree/bindings/clock/clock-bindings.txt
+- clock-names: Should be "mck0","mck1".
+ See: Documentation/devicetree/bindings/resource-names.txt
+- pinctrl-names: A pinctrl state names "default" must be defined.
+- pinctrl-0: Phandle referencing pin configuration of the SPI peripheral.
+ See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
+
+Optional properties:
+- cs-gpios: List of GPIO chip selects. See ../spi/spi-bus.txt
+ See: Documentation/devicetree/bindings/spi/spi-bus.txt
+- dmas: Two or more DMA channel specifiers following the convention outlined
+ in Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: Names for the dma channels. There must be at least one channel
+ named "spi-tx" for transmit and named "spi-rx" for receive.
+
+Example:
+
+ spi1:spi@0x1f821000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "microchip,pic32mzda-spi";
+ reg = <0x1f821000 0x200>;
+ interrupts = <109 IRQ_TYPE_LEVEL_HIGH>,
+ <110 IRQ_TYPE_LEVEL_HIGH>,
+ <111 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "fault", "rx", "tx";
+ clocks = <&PBCLK2>, <&REFCLKO1>;
+ clock-names = "mck0", "mck1";
+ dmas = <&dma 134>,
+ <&dma 135>;
+ dma-names = "spi-rx", "spi-tx";
+ cs-gpios = <&gpio3 0 GPIO_ACTIVE_LOW>,
+ <&gpio3 14 GPIO_ACTIVE_LOW>;
+ };
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver
2016-02-01 22:44 [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Joshua Henderson
@ 2016-02-01 22:44 ` Joshua Henderson
2016-02-02 12:13 ` Mark Brown
2016-02-02 11:11 ` [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Sergei Shtylyov
1 sibling, 1 reply; 8+ messages in thread
From: Joshua Henderson @ 2016-02-01 22:44 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, Purna Chandra Mandal, Joshua Henderson, Mark Brown,
linux-spi
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral
2016-02-01 22:44 [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Joshua Henderson
2016-02-01 22:44 ` [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver Joshua Henderson
@ 2016-02-02 11:11 ` Sergei Shtylyov
2016-02-05 5:08 ` Purna Chandra Mandal
1 sibling, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2016-02-02 11:11 UTC (permalink / raw)
To: Joshua Henderson, linux-kernel
Cc: linux-mips, Purna Chandra Mandal, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, devicetree
Hello.
On 2/2/2016 1:44 AM, Joshua Henderson wrote:
> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>
> Document the devicetree bindings for the SPI peripheral found
> on Microchip PIC32 class devices.
>
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
> ---
> .../bindings/spi/microchip,spi-pic32.txt | 44 ++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
> new file mode 100644
> index 0000000..a555618
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
> @@ -0,0 +1,44 @@
> +* Microchip PIC32 SPI device
> +
> +Required properties:
> +- compatible: Should be "microchip,pic32mzda-spi".
> +- reg: Address and length of the register set for the device
> +- interrupts: Should contain all three spi interrupts in sequence
> + of <fault-irq>, <receive-irq>, <transmit-irq>.
> +- interrupt-names: Should be "fault","rx","tx" in order.
Please insert spaces after commas.
> +- clocks: phandles of baud generation clocks. Maximum two possible clocks
Baud in the SPI context?
> + can be provided (&PBCLK2, &REFCLKO1).
Please align.
> + See: Documentation/devicetree/bindings/clock/clock-bindings.txt
> +- clock-names: Should be "mck0","mck1".
Please insert space after comma.
[...]
> +Example:
> +
> + spi1:spi@0x1f821000 {
Please insert spaces after colon.
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver
2016-02-01 22:44 ` [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver Joshua Henderson
@ 2016-02-02 12:13 ` Mark Brown
2016-02-05 11:57 ` Purna Chandra Mandal
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2016-02-02 12:13 UTC (permalink / raw)
To: Joshua Henderson
Cc: linux-kernel, linux-mips, Purna Chandra Mandal, linux-spi
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral
2016-02-02 11:11 ` [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Sergei Shtylyov
@ 2016-02-05 5:08 ` Purna Chandra Mandal
2016-02-05 13:32 ` Sergei Shtylyov
0 siblings, 1 reply; 8+ messages in thread
From: Purna Chandra Mandal @ 2016-02-05 5:08 UTC (permalink / raw)
To: Sergei Shtylyov, Joshua Henderson, linux-kernel
Cc: linux-mips, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, devicetree
On 02/02/2016 04:41 PM, Sergei Shtylyov wrote:
> Hello.
> On 2/2/2016 1:44 AM, Joshua Henderson wrote:
>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>> Document the devicetree bindings for the SPI peripheral found
>> on Microchip PIC32 class devices.
>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>> ---
>> .../bindings/spi/microchip,spi-pic32.txt | 44 ++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>> diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>> new file mode 100644
>> index 0000000..a555618
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>> @@ -0,0 +1,44 @@
>> +* Microchip PIC32 SPI device
>> +
>> +Required properties:
>> +- compatible: Should be "microchip,pic32mzda-spi".
>> +- reg: Address and length of the register set for the device
>> +- interrupts: Should contain all three spi interrupts in sequence
>> + of <fault-irq>, <receive-irq>, <transmit-irq>.
>> +- interrupt-names: Should be "fault","rx","tx" in order.
>>
> Please insert spaces after commas.
ack.
>> +- clocks: phandles of baud generation clocks. Maximum two possible clocks
>>
> Baud in the SPI context?
Yes, clock for generating spi clock (SCK). Will update the comment.
>> + can be provided (&PBCLK2, &REFCLKO1).
>>
> Please align.
ack.
>> + See: Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +- clock-names: Should be "mck0","mck1".
>>
> Please insert space after comma.
ack.
> [...]
>> +Example:
>> +
>> + spi1:spi@0x1f821000 {
> Please insert spaces after colon.
ack
> [...]
> MBR, Sergei
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver
2016-02-02 12:13 ` Mark Brown
@ 2016-02-05 11:57 ` Purna Chandra Mandal
0 siblings, 0 replies; 8+ messages in thread
From: Purna Chandra Mandal @ 2016-02-05 11:57 UTC (permalink / raw)
To: Mark Brown; +Cc: Joshua Henderson, linux-kernel, linux-mips, linux-spi
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).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral
2016-02-05 5:08 ` Purna Chandra Mandal
@ 2016-02-05 13:32 ` Sergei Shtylyov
2016-02-08 5:06 ` Purna Chandra Mandal
0 siblings, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2016-02-05 13:32 UTC (permalink / raw)
To: Purna Chandra Mandal, Joshua Henderson, linux-kernel
Cc: linux-mips, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, devicetree
Hello.
On 2/5/2016 8:08 AM, Purna Chandra Mandal wrote:
>>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>>> Document the devicetree bindings for the SPI peripheral found
>>> on Microchip PIC32 class devices.
>>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>>> ---
>>> .../bindings/spi/microchip,spi-pic32.txt | 44 ++++++++++++++++++++
>>> 1 file changed, 44 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>> diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>> new file mode 100644
>>> index 0000000..a555618
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>> @@ -0,0 +1,44 @@
>>> +* Microchip PIC32 SPI device
[...]
>>> +Example:
>>> +
>>> + spi1:spi@0x1f821000 {
>> Please insert spaces after colon.
> ack
And please drop "0x" from the <unit-address> part of the name.
MBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral
2016-02-05 13:32 ` Sergei Shtylyov
@ 2016-02-08 5:06 ` Purna Chandra Mandal
0 siblings, 0 replies; 8+ messages in thread
From: Purna Chandra Mandal @ 2016-02-08 5:06 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Joshua Henderson, linux-kernel, linux-mips, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree
On 02/05/2016 07:02 PM, Sergei Shtylyov wrote:
> Hello.
> On 2/5/2016 8:08 AM, Purna Chandra Mandal wrote:
>>>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>>>> Document the devicetree bindings for the SPI peripheral found
>>>> on Microchip PIC32 class devices.
>>>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>>>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>>>> ---
>>>> .../bindings/spi/microchip,spi-pic32.txt | 44 ++++++++++++++++++++
>>>> 1 file changed, 44 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>>> b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>>> new file mode 100644
>>>> index 0000000..a555618
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
>>>> @@ -0,0 +1,44 @@
>>>> +* Microchip PIC32 SPI device
> [...]
>>>> +Example:
>>>> +
>>>> + spi1:spi@0x1f821000 {
>>> Please insert spaces after colon.
>> ack
>>
> And please drop "0x" from the <unit-address> part of the name.
ack.
> MBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-08 5:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-01 22:44 [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Joshua Henderson
2016-02-01 22:44 ` [PATCH 2/2] spi: spi-pic32: Add PIC32 SPI master driver Joshua Henderson
2016-02-02 12:13 ` Mark Brown
2016-02-05 11:57 ` Purna Chandra Mandal
2016-02-02 11:11 ` [PATCH 1/2] dt/bindings: Add bindings for the PIC32 SPI peripheral Sergei Shtylyov
2016-02-05 5:08 ` Purna Chandra Mandal
2016-02-05 13:32 ` Sergei Shtylyov
2016-02-08 5:06 ` Purna Chandra Mandal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).