* [PATCH 0/2] IMG SPFI driver @ 2014-11-12 21:37 Andrew Bresticker 2014-11-12 21:37 ` [PATCH 1/2] of: Add binding document for IMG SPFI controller Andrew Bresticker 2014-11-12 21:37 ` [PATCH 2/2] spi: Add driver " Andrew Bresticker 0 siblings, 2 replies; 8+ messages in thread From: Andrew Bresticker @ 2014-11-12 21:37 UTC (permalink / raw) To: Mark Brown Cc: Andrew Bresticker, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, James Hartley, Ezequiel Garcia, devicetree, linux-kernel, linux-spi This series adds support for the Sychronous Peripheral Flash Interface master found on IMG SoCs. The controller supports up to 5 chip-select lines and single, dual, and optionally quad, mode transfers. Tested on a platform using the MIPS-based Pistachio SoC with additional out-of-tree patches - support for Pistachio will be submitted soon. Based on 3.18-rc4. Andrew Bresticker (2): of: Add binding document for IMG SPFI controller spi: Add driver for IMG SPFI controller Documentation/devicetree/bindings/spi/spi-img.txt | 37 ++ drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi-img.c | 703 ++++++++++++++++++++++ 4 files changed, 748 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-img.txt create mode 100644 drivers/spi/spi-img.c -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] of: Add binding document for IMG SPFI controller 2014-11-12 21:37 [PATCH 0/2] IMG SPFI driver Andrew Bresticker @ 2014-11-12 21:37 ` Andrew Bresticker 2014-11-12 22:09 ` Mark Brown 2014-11-12 21:37 ` [PATCH 2/2] spi: Add driver " Andrew Bresticker 1 sibling, 1 reply; 8+ messages in thread From: Andrew Bresticker @ 2014-11-12 21:37 UTC (permalink / raw) To: Mark Brown Cc: Andrew Bresticker, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, James Hartley, Ezequiel Garcia, devicetree, linux-kernel, linux-spi The Synchronous Peripheral Flash Interface (SPFI) controller found on IMG SoCs supports single, dual, and (optionally) quad mode SPI transfers. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- Documentation/devicetree/bindings/spi/spi-img.txt | 37 +++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-img.txt diff --git a/Documentation/devicetree/bindings/spi/spi-img.txt b/Documentation/devicetree/bindings/spi/spi-img.txt new file mode 100644 index 0000000..c7dd50f --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-img.txt @@ -0,0 +1,37 @@ +IMG Synchronous Peripheral Flash Interface (SPFI) controller + +Required properties: +- compatible: Must be "img,spfi". +- reg: Must contain the base address and length of the SPFI registers. +- interrupts: Must contain the SPFI interrupt. +- clocks: Must contain an entry for each entry in clock-names. + See ../clock/clock-bindings.txt for details. +- clock-names: Must include the following entries: + - spfi: SPI operating clock + - sys: SPI system interface clock +- dmas: Must contain an entry for each entry in dma-names. + See ../dma/dma.txt for details. +- dma-names: Must include the following entries: + - rx + - tx +- #address-cells: Must be 1. +- #size-cells: Must be 0. + +Optional properties: +- img,supports-quad-mode: Should be set if the interface supports quad mode + SPI transfers. + +Example: + +spi@18100f00 { + compatible = "img,spfi"; + reg = <0x18100f00 0x100>; + interrupts = <GIC_SHARED 22 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&spi_clk>, <&system_clk>; + clock-names = "spfi", "sys"; + dmas = <&mdc 9 0xffffffff 0>, <&mdc 10 0xffffffff 0>; + dma-names = "rx", "tx"; + + #address-cells = <1>; + #size-cells = <0>; +}; -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] of: Add binding document for IMG SPFI controller 2014-11-12 21:37 ` [PATCH 1/2] of: Add binding document for IMG SPFI controller Andrew Bresticker @ 2014-11-12 22:09 ` Mark Brown 0 siblings, 0 replies; 8+ messages in thread From: Mark Brown @ 2014-11-12 22:09 UTC (permalink / raw) To: Andrew Bresticker Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, James Hartley, Ezequiel Garcia, devicetree, linux-kernel, linux-spi [-- Attachment #1: Type: text/plain, Size: 513 bytes --] On Wed, Nov 12, 2014 at 01:37:53PM -0800, Andrew Bresticker wrote: > The Synchronous Peripheral Flash Interface (SPFI) controller found > on IMG SoCs supports single, dual, and (optionally) quad mode SPI > transfers. This is OK but please use subject lines matching the style for the subsystem; people often filter on subject line when describing what to read. I do note that the binding lists the DMA as mandatory but the implementation tolerates it being missing, this is fine since the code is more liberal. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] spi: Add driver for IMG SPFI controller 2014-11-12 21:37 [PATCH 0/2] IMG SPFI driver Andrew Bresticker 2014-11-12 21:37 ` [PATCH 1/2] of: Add binding document for IMG SPFI controller Andrew Bresticker @ 2014-11-12 21:37 ` Andrew Bresticker 2014-11-12 22:07 ` Mark Brown 1 sibling, 1 reply; 8+ messages in thread From: Andrew Bresticker @ 2014-11-12 21:37 UTC (permalink / raw) To: Mark Brown Cc: Andrew Bresticker, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, James Hartley, Ezequiel Garcia, devicetree, linux-kernel, linux-spi Add support for the Synchronous Peripheral Flash Interface (SPFI) master controller found on IMG SoCs. The SPFI controller supports 5 chip-select lines and single/dual/quad mode SPI transfers. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi-img.c | 703 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 711 insertions(+) create mode 100644 drivers/spi/spi-img.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 84e7c9e..c64fd6c 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -225,6 +225,13 @@ config SPI_GPIO GPIO operations, you should be able to leverage that for better speed with a custom version of this driver; see the source code. +config SPI_IMG + tristate "IMG SPFI controller" + depends on MIPS || COMPILE_TEST + help + This enables support for the SPFI master controller found on + IMG SoCs. + config SPI_IMX tristate "Freescale i.MX SPI controllers" depends on ARCH_MXC || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 78f24ca..75cf823 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -40,6 +40,7 @@ obj-$(CONFIG_SPI_FSL_LIB) += spi-fsl-lib.o obj-$(CONFIG_SPI_FSL_ESPI) += spi-fsl-espi.o obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o obj-$(CONFIG_SPI_GPIO) += spi-gpio.o +obj-$(CONFIG_SPI_IMG) += spi-img.o obj-$(CONFIG_SPI_IMX) += spi-imx.o obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o diff --git a/drivers/spi/spi-img.c b/drivers/spi/spi-img.c new file mode 100644 index 0000000..8c87dbf --- /dev/null +++ b/drivers/spi/spi-img.c @@ -0,0 +1,703 @@ +/* + * IMG SPFI controller driver + * + * Copyright (C) 2007,2008,2013 Imagination Technologies Ltd. + * Copyright (C) 2014 Google, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/dmaengine.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/scatterlist.h> +#include <linux/slab.h> +#include <linux/spi/spi.h> +#include <linux/spinlock.h> + +#define SPFI_DEVICE_PARAMETER(x) (0x00 + 0x4 * (x)) +#define SPFI_DEVICE_PARAMETER_BITCLK_SHIFT 24 +#define SPFI_DEVICE_PARAMETER_BITCLK_MASK 0xff +#define SPFI_DEVICE_PARAMETER_CSSETUP_SHIFT 16 +#define SPFI_DEVICE_PARAMETER_CSSETUP_MASK 0xff +#define SPFI_DEVICE_PARAMETER_CSHOLD_SHIFT 8 +#define SPFI_DEVICE_PARAMETER_CSHOLD_MASK 0xff +#define SPFI_DEVICE_PARAMETER_CSDELAY_SHIFT 0 +#define SPFI_DEVICE_PARAMETER_CSDELAY_MASK 0xff + +#define SPFI_CONTROL 0x14 +#define SPFI_CONTROL_CONTINUE BIT(12) +#define SPFI_CONTROL_SOFT_RESET BIT(11) +#define SPFI_CONTROL_SEND_DMA BIT(10) +#define SPFI_CONTROL_GET_DMA BIT(9) +#define SPFI_CONTROL_TMODE_SHIFT 5 +#define SPFI_CONTROL_TMODE_MASK 0x7 +#define SPFI_CONTROL_TMODE_SINGLE 0 +#define SPFI_CONTROL_TMODE_DUAL 1 +#define SPFI_CONTROL_TMODE_QUAD 2 +#define SPFI_CONTROL_SPFI_EN BIT(0) + +#define SPFI_TRANSACTION 0x18 +#define SPFI_TRANSACTION_TSIZE_SHIFT 16 +#define SPFI_TRANSACTION_TSIZE_MASK 0xffff + +#define SPFI_PORT_STATE 0x1c +#define SPFI_PORT_STATE_DEV_SEL_SHIFT 20 +#define SPFI_PORT_STATE_DEV_SEL_MASK 0x7 +#define SPFI_PORT_STATE_CK_POL(x) BIT(19 - (x)) +#define SPFI_PORT_STATE_CK_PHASE(x) BIT(14 - (x)) + +#define SPFI_TX_32BIT_VALID_DATA 0x20 +#define SPFI_TX_8BIT_VALID_DATA 0x24 +#define SPFI_RX_32BIT_VALID_DATA 0x28 +#define SPFI_RX_8BIT_VALID_DATA 0x2c + +#define SPFI_INTERRUPT_STATUS 0x30 +#define SPFI_INTERRUPT_ENABLE 0x34 +#define SPFI_INTERRUPT_CLEAR 0x38 +#define SPFI_INTERRUPT_IACCESS BIT(12) +#define SPFI_INTERRUPT_GDEX8BIT BIT(11) +#define SPFI_INTERRUPT_ALLDONETRIG BIT(9) +#define SPFI_INTERRUPT_GDFUL BIT(8) +#define SPFI_INTERRUPT_GDHF BIT(7) +#define SPFI_INTERRUPT_GDEX32BIT BIT(6) +#define SPFI_INTERRUPT_GDTRIG BIT(5) +#define SPFI_INTERRUPT_SDFUL BIT(3) +#define SPFI_INTERRUPT_SDHF BIT(2) +#define SPFI_INTERRUPT_SDE BIT(1) +#define SPFI_INTERRUPT_SDTRIG BIT(0) + +/* + * There are four parallel FIFOs of 16 bytes each. The word buffer + * (*_32BIT_VALID_DATA) accesses all four FIFOs at once, resulting in an + * effective FIFO size of 64 bytes. The byte buffer (*_8BIT_VALID_DATA) + * accesses only a single FIFO, resulting in an effective FIFO size of + * 16 bytes. + */ +#define SPFI_32BIT_FIFO_SIZE 64 +#define SPFI_8BIT_FIFO_SIZE 16 + +struct img_spfi { + struct device *dev; + struct spi_master *master; + spinlock_t lock; + + void __iomem *regs; + phys_addr_t phys; + int irq; + struct clk *spfi_clk; + struct clk *sys_clk; + + struct dma_chan *rx_ch; + struct dma_chan *tx_ch; + bool tx_dma_busy; + bool rx_dma_busy; +}; + +static inline u32 spfi_readl(struct img_spfi *spfi, u32 reg) +{ + return readl(spfi->regs + reg); +} + +static inline void spfi_writel(struct img_spfi *spfi, u32 val, u32 reg) +{ + writel(val, spfi->regs + reg); +} + +static inline void spfi_enable(struct img_spfi *spfi) +{ + u32 val; + + val = spfi_readl(spfi, SPFI_CONTROL); + val |= SPFI_CONTROL_SPFI_EN; + spfi_writel(spfi, val, SPFI_CONTROL); +} + +static inline void spfi_disable(struct img_spfi *spfi) +{ + u32 val; + + val = spfi_readl(spfi, SPFI_CONTROL); + val &= ~SPFI_CONTROL_SPFI_EN; + spfi_writel(spfi, val, SPFI_CONTROL); +} + +static inline void spfi_reset(struct img_spfi *spfi) +{ + spfi_writel(spfi, SPFI_CONTROL_SOFT_RESET, SPFI_CONTROL); + udelay(1); + spfi_writel(spfi, 0, SPFI_CONTROL); +} + +static void spfi_flush_tx_fifo(struct img_spfi *spfi) +{ + spfi_writel(spfi, SPFI_INTERRUPT_SDE, SPFI_INTERRUPT_CLEAR); + while (!(spfi_readl(spfi, SPFI_INTERRUPT_STATUS) & + SPFI_INTERRUPT_SDE)) + cpu_relax(); +} + +static unsigned int spfi_pio_write32(struct img_spfi *spfi, const u32 *buf, + unsigned int max) +{ + unsigned int count = 0; + u32 status; + + while (count < max) { + spfi_writel(spfi, SPFI_INTERRUPT_SDFUL, SPFI_INTERRUPT_CLEAR); + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); + if (status & SPFI_INTERRUPT_SDFUL) + break; + spfi_writel(spfi, buf[count / 4], SPFI_TX_32BIT_VALID_DATA); + count += 4; + } + + return count; +} + +static unsigned int spfi_pio_write8(struct img_spfi *spfi, const u8 *buf, + unsigned int max) +{ + unsigned int count = 0; + u32 status; + + while (count < max) { + spfi_writel(spfi, SPFI_INTERRUPT_SDFUL, SPFI_INTERRUPT_CLEAR); + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); + if (status & SPFI_INTERRUPT_SDFUL) + break; + spfi_writel(spfi, buf[count], SPFI_TX_8BIT_VALID_DATA); + count++; + } + + return count; +} + +static unsigned int spfi_pio_read32(struct img_spfi *spfi, u32 *buf, + unsigned int max) +{ + unsigned int count = 0; + u32 status; + + while (count < max) { + spfi_writel(spfi, SPFI_INTERRUPT_GDEX32BIT, + SPFI_INTERRUPT_CLEAR); + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); + if (!(status & SPFI_INTERRUPT_GDEX32BIT)) + break; + buf[count / 4] = spfi_readl(spfi, SPFI_RX_32BIT_VALID_DATA); + count += 4; + } + + return count; +} + +static unsigned int spfi_pio_read8(struct img_spfi *spfi, u8 *buf, + unsigned int max) +{ + unsigned int count = 0; + u32 status; + + while (count < max) { + spfi_writel(spfi, SPFI_INTERRUPT_GDEX8BIT, + SPFI_INTERRUPT_CLEAR); + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); + if (!(status & SPFI_INTERRUPT_GDEX8BIT)) + break; + buf[count] = spfi_readl(spfi, SPFI_RX_8BIT_VALID_DATA); + count++; + } + + return count; +} + +static int img_spfi_start_pio(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct img_spfi *spfi = spi_master_get_devdata(spi->master); + unsigned int tx_bytes = 0, rx_bytes = 0; + const void *tx_buf = xfer->tx_buf; + void *rx_buf = xfer->rx_buf; + unsigned long timeout; + + if (tx_buf) + tx_bytes = xfer->len; + if (rx_buf) + rx_bytes = xfer->len; + + spfi_enable(spfi); + + timeout = jiffies + + msecs_to_jiffies(xfer->len * 8 * 1000 / xfer->speed_hz + 100); + while ((tx_bytes > 0 || rx_bytes > 0) && + time_before(jiffies, timeout)) { + unsigned int tx_count, rx_count; + + if (xfer->bits_per_word == 32) { + tx_count = spfi_pio_write32(spfi, tx_buf, tx_bytes); + rx_count = spfi_pio_read32(spfi, rx_buf, rx_bytes); + } else { + tx_count = spfi_pio_write8(spfi, tx_buf, tx_bytes); + rx_count = spfi_pio_read8(spfi, rx_buf, rx_bytes); + } + + tx_buf += tx_count; + rx_buf += rx_count; + tx_bytes -= tx_count; + rx_bytes -= rx_count; + + cpu_relax(); + } + + if (rx_bytes > 0 || tx_bytes > 0) { + dev_err(spfi->dev, "PIO transfer timed out\n"); + spfi_reset(spfi); + return -ETIMEDOUT; + } + + if (tx_buf) + spfi_flush_tx_fifo(spfi); + spfi_disable(spfi); + + return 0; +} + +static void img_spfi_dma_rx_cb(void *data) +{ + struct img_spfi *spfi = data; + unsigned long flags; + + spin_lock_irqsave(&spfi->lock, flags); + + spfi->rx_dma_busy = false; + if (!spfi->tx_dma_busy) { + spfi_disable(spfi); + spi_finalize_current_transfer(spfi->master); + } + + spin_unlock_irqrestore(&spfi->lock, flags); +} + +static void img_spfi_dma_tx_cb(void *data) +{ + struct img_spfi *spfi = data; + unsigned long flags; + + spfi_flush_tx_fifo(spfi); + + spin_lock_irqsave(&spfi->lock, flags); + + spfi->tx_dma_busy = false; + if (!spfi->rx_dma_busy) { + spfi_disable(spfi); + spi_finalize_current_transfer(spfi->master); + } + + spin_unlock_irqrestore(&spfi->lock, flags); +} + +static int img_spfi_start_dma(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct img_spfi *spfi = spi_master_get_devdata(spi->master); + struct dma_async_tx_descriptor *rxdesc = NULL, *txdesc = NULL; + struct dma_slave_config rxconf, txconf; + + spfi->rx_dma_busy = false; + spfi->tx_dma_busy = false; + + if (xfer->rx_buf) { + rxconf.direction = DMA_DEV_TO_MEM; + if (xfer->bits_per_word == 32) { + rxconf.src_addr = spfi->phys + SPFI_RX_32BIT_VALID_DATA; + rxconf.src_addr_width = 4; + rxconf.src_maxburst = 4; + } else { + rxconf.src_addr = spfi->phys + SPFI_RX_8BIT_VALID_DATA; + rxconf.src_addr_width = 1; + rxconf.src_maxburst = 1; + } + dmaengine_slave_config(spfi->rx_ch, &rxconf); + + rxdesc = dmaengine_prep_slave_sg(spfi->rx_ch, xfer->rx_sg.sgl, + xfer->rx_sg.nents, + DMA_DEV_TO_MEM, + DMA_PREP_INTERRUPT); + if (!rxdesc) + goto stop_dma; + + rxdesc->callback = img_spfi_dma_rx_cb; + rxdesc->callback_param = spfi; + } + + if (xfer->tx_buf) { + txconf.direction = DMA_MEM_TO_DEV; + if (xfer->bits_per_word == 32) { + txconf.dst_addr = spfi->phys + SPFI_TX_32BIT_VALID_DATA; + txconf.dst_addr_width = 4; + txconf.dst_maxburst = 4; + } else { + txconf.dst_addr = spfi->phys + SPFI_TX_8BIT_VALID_DATA; + txconf.dst_addr_width = 1; + txconf.dst_maxburst = 1; + } + dmaengine_slave_config(spfi->tx_ch, &txconf); + + txdesc = dmaengine_prep_slave_sg(spfi->tx_ch, xfer->tx_sg.sgl, + xfer->tx_sg.nents, + DMA_MEM_TO_DEV, + DMA_PREP_INTERRUPT); + if (!txdesc) + goto stop_dma; + + txdesc->callback = img_spfi_dma_tx_cb; + txdesc->callback_param = spfi; + } + + if (xfer->rx_buf) { + spfi->rx_dma_busy = true; + dmaengine_submit(rxdesc); + dma_async_issue_pending(spfi->rx_ch); + } + + if (xfer->tx_buf) { + spfi->tx_dma_busy = true; + dmaengine_submit(txdesc); + dma_async_issue_pending(spfi->tx_ch); + } + + spfi_enable(spfi); + + return 1; + +stop_dma: + dmaengine_terminate_all(spfi->rx_ch); + dmaengine_terminate_all(spfi->tx_ch); + return -EIO; +} + +static void img_spfi_config(struct spi_master *master, struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct img_spfi *spfi = spi_master_get_devdata(spi->master); + u32 val, div; + + /* + * output = spfi_clk * (BITCLK / 512), where BITCLK must be a + * power of 2 up to 256 (where 255 == 256 since BITCLK is 8 bits) + */ + div = DIV_ROUND_UP(master->max_speed_hz, xfer->speed_hz); + div = clamp(512 / (1 << get_count_order(div)), 1, 255); + + val = spfi_readl(spfi, SPFI_DEVICE_PARAMETER(spi->chip_select)); + val &= ~(SPFI_DEVICE_PARAMETER_BITCLK_MASK << + SPFI_DEVICE_PARAMETER_BITCLK_SHIFT); + val |= div << SPFI_DEVICE_PARAMETER_BITCLK_SHIFT; + spfi_writel(spfi, val, SPFI_DEVICE_PARAMETER(spi->chip_select)); + + val = spfi_readl(spfi, SPFI_CONTROL); + val &= ~(SPFI_CONTROL_SEND_DMA | SPFI_CONTROL_GET_DMA); + if (xfer->tx_buf) + val |= SPFI_CONTROL_SEND_DMA; + if (xfer->rx_buf) + val |= SPFI_CONTROL_GET_DMA; + val &= ~(SPFI_CONTROL_TMODE_MASK << SPFI_CONTROL_TMODE_SHIFT); + if (xfer->tx_nbits == SPI_NBITS_DUAL || + xfer->rx_nbits == SPI_NBITS_DUAL) + val |= SPFI_CONTROL_TMODE_DUAL << SPFI_CONTROL_TMODE_SHIFT; + else if (xfer->tx_nbits == SPI_NBITS_QUAD || + xfer->rx_nbits == SPI_NBITS_QUAD) + val |= SPFI_CONTROL_TMODE_QUAD << SPFI_CONTROL_TMODE_SHIFT; + val &= ~SPFI_CONTROL_CONTINUE; + if (!xfer->cs_change && !list_is_last(&xfer->transfer_list, + &master->cur_msg->transfers)) + val |= SPFI_CONTROL_CONTINUE; + spfi_writel(spfi, val, SPFI_CONTROL); + + val = spfi_readl(spfi, SPFI_PORT_STATE); + if (spi->mode & SPI_CPHA) + val |= SPFI_PORT_STATE_CK_PHASE(spi->chip_select); + else + val &= ~SPFI_PORT_STATE_CK_PHASE(spi->chip_select); + if (spi->mode & SPI_CPOL) + val |= SPFI_PORT_STATE_CK_POL(spi->chip_select); + else + val &= ~SPFI_PORT_STATE_CK_POL(spi->chip_select); + spfi_writel(spfi, val, SPFI_PORT_STATE); + + spfi_writel(spfi, xfer->len << SPFI_TRANSACTION_TSIZE_SHIFT, + SPFI_TRANSACTION); +} + +static int img_spfi_transfer_one(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct img_spfi *spfi = spi_master_get_devdata(spi->master); + bool dma_reset = false; + unsigned long flags; + int ret; + + /* + * Stop all DMA and reset the controller if the previous transaction + * timed-out and never completed it's DMA. + */ + spin_lock_irqsave(&spfi->lock, flags); + if (spfi->tx_dma_busy || spfi->rx_dma_busy) { + dev_err(spfi->dev, "SPI DMA still busy\n"); + dma_reset = true; + } + spin_unlock_irqrestore(&spfi->lock, flags); + + if (dma_reset) { + dmaengine_terminate_all(spfi->tx_ch); + dmaengine_terminate_all(spfi->rx_ch); + spfi_reset(spfi); + } + + img_spfi_config(master, spi, xfer); + if (master->can_dma && master->can_dma(master, spi, xfer)) + ret = img_spfi_start_dma(master, spi, xfer); + else + ret = img_spfi_start_pio(master, spi, xfer); + + return ret; +} + +static void img_spfi_set_cs(struct spi_device *spi, bool enable) +{ + struct img_spfi *spfi = spi_master_get_devdata(spi->master); + u32 val; + + val = spfi_readl(spfi, SPFI_PORT_STATE); + val &= ~(SPFI_PORT_STATE_DEV_SEL_MASK << SPFI_PORT_STATE_DEV_SEL_SHIFT); + val |= spi->chip_select << SPFI_PORT_STATE_DEV_SEL_SHIFT; + spfi_writel(spfi, val, SPFI_PORT_STATE); +} + +static bool img_spfi_can_dma(struct spi_master *master, struct spi_device *spi, + struct spi_transfer *xfer) +{ + if (xfer->bits_per_word == 8 && xfer->len > SPFI_8BIT_FIFO_SIZE) + return true; + if (xfer->bits_per_word == 32 && xfer->len > SPFI_32BIT_FIFO_SIZE) + return true; + return false; +} + +static irqreturn_t img_spfi_irq(int irq, void *dev_id) +{ + struct img_spfi *spfi = (struct img_spfi *)dev_id; + u32 status; + + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); + if (status & SPFI_INTERRUPT_IACCESS) { + spfi_writel(spfi, SPFI_INTERRUPT_IACCESS, SPFI_INTERRUPT_CLEAR); + dev_err(spfi->dev, "Illegal access interrupt"); + } + + return IRQ_HANDLED; +} + +static int img_spfi_probe(struct platform_device *pdev) +{ + struct spi_master *master; + struct img_spfi *spfi; + struct resource *res; + int ret; + + master = spi_alloc_master(&pdev->dev, sizeof(*spfi)); + if (!master) + return -ENOMEM; + platform_set_drvdata(pdev, master); + + spfi = spi_master_get_devdata(master); + spfi->dev = &pdev->dev; + spfi->master = master; + spin_lock_init(&spfi->lock); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + spfi->regs = devm_ioremap_resource(spfi->dev, res); + if (IS_ERR(spfi->regs)) { + ret = PTR_ERR(spfi->regs); + goto put_spi; + } + spfi->phys = res->start; + + spfi->irq = platform_get_irq(pdev, 0); + if (spfi->irq < 0) { + ret = spfi->irq; + goto put_spi; + } + ret = devm_request_irq(spfi->dev, spfi->irq, img_spfi_irq, + IRQ_TYPE_LEVEL_HIGH, dev_name(spfi->dev), spfi); + if (ret) + goto put_spi; + + spfi->sys_clk = devm_clk_get(spfi->dev, "sys"); + if (IS_ERR(spfi->sys_clk)) { + ret = PTR_ERR(spfi->sys_clk); + goto put_spi; + } + spfi->spfi_clk = devm_clk_get(spfi->dev, "spfi"); + if (IS_ERR(spfi->spfi_clk)) { + ret = PTR_ERR(spfi->spfi_clk); + goto put_spi; + } + + ret = clk_prepare_enable(spfi->sys_clk); + if (ret) + goto put_spi; + ret = clk_prepare_enable(spfi->spfi_clk); + if (ret) + goto disable_pclk; + + spfi_reset(spfi); + /* + * Only enable the error (IACCESS) interrupt. In PIO mode we'll + * poll the status of the FIFOs. + */ + spfi_writel(spfi, SPFI_INTERRUPT_IACCESS, SPFI_INTERRUPT_ENABLE); + + master->bus_num = pdev->id; + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL; + if (of_property_read_bool(spfi->dev->of_node, "img,supports-quad-mode")) + master->mode_bits |= SPI_TX_QUAD | SPI_RX_QUAD; + master->num_chipselect = 5; + master->dev.of_node = pdev->dev.of_node; + master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(8); + master->max_speed_hz = clk_get_rate(spfi->spfi_clk); + master->min_speed_hz = master->max_speed_hz / 512; + + master->set_cs = img_spfi_set_cs; + master->transfer_one = img_spfi_transfer_one; + + spfi->tx_ch = dma_request_slave_channel(spfi->dev, "tx"); + spfi->rx_ch = dma_request_slave_channel(spfi->dev, "rx"); + if (!spfi->tx_ch || !spfi->rx_ch) { + if (spfi->tx_ch) + dma_release_channel(spfi->tx_ch); + if (spfi->rx_ch) + dma_release_channel(spfi->rx_ch); + dev_warn(spfi->dev, "Failed to get DMA channels, falling back to PIO mode\n"); + } else { + master->dma_tx = spfi->tx_ch; + master->dma_rx = spfi->rx_ch; + master->can_dma = img_spfi_can_dma; + } + + ret = devm_spi_register_master(spfi->dev, master); + if (ret) + goto put_dma; + + return 0; + +put_dma: + if (spfi->rx_ch) + dma_release_channel(spfi->rx_ch); + if (spfi->tx_ch) + dma_release_channel(spfi->tx_ch); + clk_disable_unprepare(spfi->spfi_clk); +disable_pclk: + clk_disable_unprepare(spfi->sys_clk); +put_spi: + spi_master_put(master); + + return ret; +} + +static int img_spfi_remove(struct platform_device *pdev) +{ + struct spi_master *master = platform_get_drvdata(pdev); + struct img_spfi *spfi = spi_master_get_devdata(master); + + if (spfi->tx_ch) + dma_release_channel(spfi->tx_ch); + if (spfi->rx_ch) + dma_release_channel(spfi->rx_ch); + + clk_disable_unprepare(spfi->spfi_clk); + clk_disable_unprepare(spfi->sys_clk); + + spi_master_put(master); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int img_spfi_suspend(struct device *dev) +{ + struct spi_master *master = dev_get_drvdata(dev); + struct img_spfi *spfi = spi_master_get_devdata(master); + int ret; + + ret = spi_master_suspend(master); + if (ret) + return ret; + + clk_disable_unprepare(spfi->spfi_clk); + clk_disable_unprepare(spfi->sys_clk); + + return 0; +} + +static int img_spfi_resume(struct device *dev) +{ + struct spi_master *master = dev_get_drvdata(dev); + struct img_spfi *spfi = spi_master_get_devdata(master); + int ret; + + ret = clk_prepare_enable(spfi->sys_clk); + if (ret) + return ret; + ret = clk_prepare_enable(spfi->spfi_clk); + if (ret) { + clk_disable_unprepare(spfi->sys_clk); + return ret; + } + + spfi_reset(spfi); + + ret = spi_master_resume(master); + if (ret) { + clk_disable_unprepare(spfi->spfi_clk); + clk_disable_unprepare(spfi->sys_clk); + } + + return ret; +} +#endif /* CONFIG_PM_SLEEP */ + +static const struct dev_pm_ops img_spfi_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(img_spfi_suspend, img_spfi_resume) +}; + +static const struct of_device_id img_spfi_of_match[] = { + { .compatible = "img,spfi", }, + { }, +}; +MODULE_DEVICE_TABLE(of, img_spfi_of_match); + +static struct platform_driver img_spfi_driver = { + .driver = { + .name = "img-spfi", + .pm = &img_spfi_pm_ops, + .of_match_table = of_match_ptr(img_spfi_of_match), + }, + .probe = img_spfi_probe, + .remove = img_spfi_remove, +}; +module_platform_driver(img_spfi_driver); + +MODULE_DESCRIPTION("IMG SPFI controller driver"); +MODULE_AUTHOR("Andrew Bresticker <abrestic@chromium.org>"); +MODULE_LICENSE("GPL v2"); -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller 2014-11-12 21:37 ` [PATCH 2/2] spi: Add driver " Andrew Bresticker @ 2014-11-12 22:07 ` Mark Brown 2014-11-12 22:54 ` Andrew Bresticker 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2014-11-12 22:07 UTC (permalink / raw) To: Andrew Bresticker Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, James Hartley, Ezequiel Garcia, devicetree, linux-kernel, linux-spi [-- Attachment #1: Type: text/plain, Size: 3390 bytes --] On Wed, Nov 12, 2014 at 01:37:54PM -0800, Andrew Bresticker wrote: > Add support for the Synchronous Peripheral Flash Interface (SPFI) master > controller found on IMG SoCs. The SPFI controller supports 5 chip-select > lines and single/dual/quad mode SPI transfers. > drivers/spi/spi-img.c | 703 ++++++++++++++++++++++++++++++++++++++++++++++++++ How about spi-img-spfi? That way if someone makes another SPI controller (say a more generic one, this one seems flash specialized) there won't be a collision. > +static void spfi_flush_tx_fifo(struct img_spfi *spfi) > +{ > + spfi_writel(spfi, SPFI_INTERRUPT_SDE, SPFI_INTERRUPT_CLEAR); > + while (!(spfi_readl(spfi, SPFI_INTERRUPT_STATUS) & > + SPFI_INTERRUPT_SDE)) > + cpu_relax(); > +} This will busy loop for ever if we don't get the response we want from the hardware... some cap on the number of spins would be good. > + while ((tx_bytes > 0 || rx_bytes > 0) && > + time_before(jiffies, timeout)) { > + unsigned int tx_count, rx_count; > + > + if (xfer->bits_per_word == 32) { > + tx_count = spfi_pio_write32(spfi, tx_buf, tx_bytes); > + rx_count = spfi_pio_read32(spfi, rx_buf, rx_bytes); > + } else { > + tx_count = spfi_pio_write8(spfi, tx_buf, tx_bytes); > + rx_count = spfi_pio_read8(spfi, rx_buf, rx_bytes); > + } switch statement please (here and elsewhere). Apart from the defensivenes it means that we'll do the right thing if someone decides to add 16 bit support to the hardware. > + tx_buf += tx_count; > + rx_buf += rx_count; > + tx_bytes -= tx_count; > + rx_bytes -= rx_count; No errors possible? > + > + cpu_relax(); Seems random - we already relax in the data transfer? > + if (tx_buf) > + spfi_flush_tx_fifo(spfi); > + spfi_disable(spfi); What does the enable and disable actually do? Should this be runtime PM? > + if (xfer->tx_nbits == SPI_NBITS_DUAL || > + xfer->rx_nbits == SPI_NBITS_DUAL) > + val |= SPFI_CONTROL_TMODE_DUAL << SPFI_CONTROL_TMODE_SHIFT; > + else if (xfer->tx_nbits == SPI_NBITS_QUAD || > + xfer->rx_nbits == SPI_NBITS_QUAD) > + val |= SPFI_CONTROL_TMODE_QUAD << SPFI_CONTROL_TMODE_SHIFT; This suggests that dual and quad mode must be symmetric but doesn't enforce that; I rather suspect that in reality these modes are only supported on the transmit side. > +static irqreturn_t img_spfi_irq(int irq, void *dev_id) > +{ > + struct img_spfi *spfi = (struct img_spfi *)dev_id; > + u32 status; > + > + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); > + if (status & SPFI_INTERRUPT_IACCESS) { > + spfi_writel(spfi, SPFI_INTERRUPT_IACCESS, SPFI_INTERRUPT_CLEAR); > + dev_err(spfi->dev, "Illegal access interrupt"); > + } > + > + return IRQ_HANDLED; > +} This will unconditionally claim to have handled an interrupt even if it didn't get any interrupt status it has ever heard of. At the very least it should log unknown interrupts, ideally return IRQ_NONE though since it seems to be a clear on read interrupt that's a bit misleading. > + ret = clk_prepare_enable(spfi->sys_clk); > + if (ret) > + goto put_spi; > + ret = clk_prepare_enable(spfi->spfi_clk); > + if (ret) > + goto disable_pclk; We should have runtime PM callbacks to enable these clocks only when the controller is active, this will improve power consumption slightly - the core can manage the runtime PM for you. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller 2014-11-12 22:07 ` Mark Brown @ 2014-11-12 22:54 ` Andrew Bresticker 2014-11-12 23:06 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Andrew Bresticker @ 2014-11-12 22:54 UTC (permalink / raw) To: Mark Brown, James Hartley Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, Ezequiel Garcia, devicetree, linux-kernel, linux-spi On Wed, Nov 12, 2014 at 2:07 PM, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Nov 12, 2014 at 01:37:54PM -0800, Andrew Bresticker wrote: >> Add support for the Synchronous Peripheral Flash Interface (SPFI) master >> controller found on IMG SoCs. The SPFI controller supports 5 chip-select >> lines and single/dual/quad mode SPI transfers. > >> drivers/spi/spi-img.c | 703 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > How about spi-img-spfi? That way if someone makes another SPI > controller (say a more generic one, this one seems flash specialized) > there won't be a collision. Despite the name, I believe this controller is used for generic SPI stuff as well. I'm not sure if there is a separate one which is more generic (James?). >> +static void spfi_flush_tx_fifo(struct img_spfi *spfi) >> +{ >> + spfi_writel(spfi, SPFI_INTERRUPT_SDE, SPFI_INTERRUPT_CLEAR); >> + while (!(spfi_readl(spfi, SPFI_INTERRUPT_STATUS) & >> + SPFI_INTERRUPT_SDE)) >> + cpu_relax(); >> +} > > This will busy loop for ever if we don't get the response we want from > the hardware... some cap on the number of spins would be good. Ok, I'll add a timeout or something here. >> + while ((tx_bytes > 0 || rx_bytes > 0) && >> + time_before(jiffies, timeout)) { >> + unsigned int tx_count, rx_count; >> + >> + if (xfer->bits_per_word == 32) { >> + tx_count = spfi_pio_write32(spfi, tx_buf, tx_bytes); >> + rx_count = spfi_pio_read32(spfi, rx_buf, rx_bytes); >> + } else { >> + tx_count = spfi_pio_write8(spfi, tx_buf, tx_bytes); >> + rx_count = spfi_pio_read8(spfi, rx_buf, rx_bytes); >> + } > > switch statement please (here and elsewhere). Apart from the > defensivenes it means that we'll do the right thing if someone decides > to add 16 bit support to the hardware. Ok. >> + tx_buf += tx_count; >> + rx_buf += rx_count; >> + tx_bytes -= tx_count; >> + rx_bytes -= rx_count; I don't think so. The spfi_pio_* functions just read/write from the FIFO until it's empty/full - they don't check for any errors, nor are there really any to check for. The loop will time-out eventually if we are unable to transmit/receive the requested number of bytes. >> + >> + cpu_relax(); > > Seems random - we already relax in the data transfer? We don't relax in the transfers - would that be a better place to put it? I thought it was better here since we reach this point once the TX FIFO has filled or RX FIFO has emptied. >> + if (tx_buf) >> + spfi_flush_tx_fifo(spfi); >> + spfi_disable(spfi); > > What does the enable and disable actually do? Should this be runtime > PM? It starts/stops the transfer. The control registers (bit clock, transfer mode, etc.) must be programmed before the enable bit is set and the bit does not clear itself upon completion of the transfer. I don't think it makes sense to have this be part of runtime PM. >> + if (xfer->tx_nbits == SPI_NBITS_DUAL || >> + xfer->rx_nbits == SPI_NBITS_DUAL) >> + val |= SPFI_CONTROL_TMODE_DUAL << SPFI_CONTROL_TMODE_SHIFT; >> + else if (xfer->tx_nbits == SPI_NBITS_QUAD || >> + xfer->rx_nbits == SPI_NBITS_QUAD) >> + val |= SPFI_CONTROL_TMODE_QUAD << SPFI_CONTROL_TMODE_SHIFT; > > This suggests that dual and quad mode must be symmetric but doesn't > enforce that; I rather suspect that in reality these modes are only > supported on the transmit side. Actually it looks like they are symmetric. I'll update these checks to enforce that. >> +static irqreturn_t img_spfi_irq(int irq, void *dev_id) >> +{ >> + struct img_spfi *spfi = (struct img_spfi *)dev_id; >> + u32 status; >> + >> + status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS); >> + if (status & SPFI_INTERRUPT_IACCESS) { >> + spfi_writel(spfi, SPFI_INTERRUPT_IACCESS, SPFI_INTERRUPT_CLEAR); >> + dev_err(spfi->dev, "Illegal access interrupt"); >> + } >> + >> + return IRQ_HANDLED; >> +} > > This will unconditionally claim to have handled an interrupt even if it > didn't get any interrupt status it has ever heard of. At the very least > it should log unknown interrupts, ideally return IRQ_NONE though since > it seems to be a clear on read interrupt that's a bit misleading. There's a clear register actually (see the writel() above), but yes, an error and returning IRQ_NONE sound appropriate in the event of an unexpected interrupt. >> + ret = clk_prepare_enable(spfi->sys_clk); >> + if (ret) >> + goto put_spi; >> + ret = clk_prepare_enable(spfi->spfi_clk); >> + if (ret) >> + goto disable_pclk; > > We should have runtime PM callbacks to enable these clocks only when the > controller is active, this will improve power consumption slightly - the > core can manage the runtime PM for you. Ok, will do. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller 2014-11-12 22:54 ` Andrew Bresticker @ 2014-11-12 23:06 ` Mark Brown 2014-11-13 15:07 ` James Hartley 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2014-11-12 23:06 UTC (permalink / raw) To: Andrew Bresticker Cc: James Hartley, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, Ezequiel Garcia, devicetree, linux-kernel, linux-spi [-- Attachment #1: Type: text/plain, Size: 2244 bytes --] On Wed, Nov 12, 2014 at 02:54:57PM -0800, Andrew Bresticker wrote: > On Wed, Nov 12, 2014 at 2:07 PM, Mark Brown <broonie@kernel.org> wrote: > >> drivers/spi/spi-img.c | 703 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > How about spi-img-spfi? That way if someone makes another SPI > > controller (say a more generic one, this one seems flash specialized) > > there won't be a collision. > Despite the name, I believe this controller is used for generic SPI > stuff as well. I'm not sure if there is a separate one which is more > generic (James?). It would still be better to use a name less impressively generic - this is an entire company, not even a product line. > >> + cpu_relax(); > > Seems random - we already relax in the data transfer? > We don't relax in the transfers - would that be a better place to put > it? I thought it was better here since we reach this point once the > TX FIFO has filled or RX FIFO has emptied. Oh, that was the FIFO drain I was thinking of. I guess here is fine. > >> + if (tx_buf) > >> + spfi_flush_tx_fifo(spfi); > >> + spfi_disable(spfi); > > What does the enable and disable actually do? Should this be runtime > > PM? > It starts/stops the transfer. The control registers (bit clock, > transfer mode, etc.) must be programmed before the enable bit is set > and the bit does not clear itself upon completion of the transfer. I > don't think it makes sense to have this be part of runtime PM. Perhaps these functions need to be called start() and stop() then - the names sound like they gate the IP? > > This will unconditionally claim to have handled an interrupt even if it > > didn't get any interrupt status it has ever heard of. At the very least > > it should log unknown interrupts, ideally return IRQ_NONE though since > > it seems to be a clear on read interrupt that's a bit misleading. > There's a clear register actually (see the writel() above), but yes, > an error and returning IRQ_NONE sound appropriate in the event of an > unexpected interrupt. Don't add the error print - the interrupt core has diagnostics already and it won't be nice if the interrupt ends up shared. My recommendation was intended as an either/or. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] spi: Add driver for IMG SPFI controller 2014-11-12 23:06 ` Mark Brown @ 2014-11-13 15:07 ` James Hartley 0 siblings, 0 replies; 8+ messages in thread From: James Hartley @ 2014-11-13 15:07 UTC (permalink / raw) To: Mark Brown, Andrew Bresticker Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, Ezequiel Garcia, devicetree, linux-kernel, linux-spi, James Hogan > -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: 12 November 2014 23:06 > To: Andrew Bresticker > Cc: James Hartley; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; > Kumar Gala; Grant Likely; Ezequiel Garcia; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org > Subject: Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller > > On Wed, Nov 12, 2014 at 02:54:57PM -0800, Andrew Bresticker wrote: > > On Wed, Nov 12, 2014 at 2:07 PM, Mark Brown <broonie@kernel.org> > wrote: > > > >> drivers/spi/spi-img.c | 703 > > >> ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > How about spi-img-spfi? That way if someone makes another SPI > > > controller (say a more generic one, this one seems flash > > > specialized) there won't be a collision. > > > Despite the name, I believe this controller is used for generic SPI > > stuff as well. I'm not sure if there is a separate one which is more > > generic (James?). There is another generic img spi hardware block which is not register compatible and doesn't have the flash interface which is also in existing SoC's. There is a separate img-spi driver for that which has not yet been mainlined. I think spi-img-spfi would be ok for this driver. There is also another sfc (Serial Flash Controller), block in the pipeline, so that could then be called spi-img-sfc. > > It would still be better to use a name less impressively generic - this is an > entire company, not even a product line. > > > >> + cpu_relax(); > > > > Seems random - we already relax in the data transfer? > > > We don't relax in the transfers - would that be a better place to put > > it? I thought it was better here since we reach this point once the > > TX FIFO has filled or RX FIFO has emptied. > > Oh, that was the FIFO drain I was thinking of. I guess here is fine. > > > >> + if (tx_buf) > > >> + spfi_flush_tx_fifo(spfi); > > >> + spfi_disable(spfi); > > > > What does the enable and disable actually do? Should this be > > > runtime PM? > > > It starts/stops the transfer. The control registers (bit clock, > > transfer mode, etc.) must be programmed before the enable bit is set > > and the bit does not clear itself upon completion of the transfer. I > > don't think it makes sense to have this be part of runtime PM. > > Perhaps these functions need to be called start() and stop() then - the > names sound like they gate the IP? > > > > This will unconditionally claim to have handled an interrupt even if > > > it didn't get any interrupt status it has ever heard of. At the > > > very least it should log unknown interrupts, ideally return IRQ_NONE > > > though since it seems to be a clear on read interrupt that's a bit > misleading. > > > There's a clear register actually (see the writel() above), but yes, > > an error and returning IRQ_NONE sound appropriate in the event of an > > unexpected interrupt. > > Don't add the error print - the interrupt core has diagnostics already and it > won't be nice if the interrupt ends up shared. My recommendation was > intended as an either/or. James. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-11-13 15:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-12 21:37 [PATCH 0/2] IMG SPFI driver Andrew Bresticker 2014-11-12 21:37 ` [PATCH 1/2] of: Add binding document for IMG SPFI controller Andrew Bresticker 2014-11-12 22:09 ` Mark Brown 2014-11-12 21:37 ` [PATCH 2/2] spi: Add driver " Andrew Bresticker 2014-11-12 22:07 ` Mark Brown 2014-11-12 22:54 ` Andrew Bresticker 2014-11-12 23:06 ` Mark Brown 2014-11-13 15:07 ` James Hartley
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 NNTP newsgroup(s).