* [PATCH v5 0/3] spi: support for Socionext Synquacer platform
@ 2019-05-21 11:59 Masahisa Kojima
2019-05-21 11:59 ` [PATCH v5 1/3] MAINTAINERS: Add entry for Synquacer SPI driver Masahisa Kojima
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Masahisa Kojima @ 2019-05-21 11:59 UTC (permalink / raw)
To: linux-spi, devicetree
Cc: broonie, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland,
ard.biesheuvel, jaswinder.singh, masami.hiramatsu,
okamoto.satoru, osaki.yoshitoyo, Masahisa Kojima
Hello,
Support for Socionext's FIP controller intended for flash device interfacing.
The controller can operate in 'direct' or 'command' mode. One mode directly
talks and provide a read/write i/f to the flash device. Other works as plain
SPI mode. This driver runs the controller as a SPI controller.
I took over this driver update from Jassi.
Although more than a year has passed from previous version,
it would be great if you could review this patch series.
Changes since v4:
# Supported interrupt based data handling instead of polling
# Added prefix "SYNQUACER_HSSPI_"
# Replaced data read/write access with readsx()/writesx()
# Updated clock source handling, explicitly specify "iHCLK" or "iPCKL"
and removed array of clk
Changes since v3:
# Convert IS_ERR returned from devm_clk_get(iPCLK) to NULL, that way
we can call clk_xxx(IPCLK) without first checking for it being valid.
Changes since v2:
# Made iHCLK clock property required in DT, and iPCLK an optional extra.
# Hardcode max number of slaves to 4, as specified in the manual.
Changes since v1:
# Changed licence header to C++ style comment.
# Removed redundant lock and transfer_mode backup member.
# Fixed divisor to allow upto 254.
Masahisa Kojima (3):
MAINTAINERS: Add entry for Synquacer SPI driver
dt-bindings: spi: Add DT bindings for Synquacer
spi: Add spi driver for Socionext Synquacer platform
.../devicetree/bindings/spi/spi-synquacer.txt | 27 +
MAINTAINERS | 8 +
drivers/spi/Kconfig | 11 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-synquacer.c | 731 +++++++++++++++++++++
5 files changed, 778 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-synquacer.txt
create mode 100644 drivers/spi/spi-synquacer.c
--
2.14.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 1/3] MAINTAINERS: Add entry for Synquacer SPI driver 2019-05-21 11:59 [PATCH v5 0/3] spi: support for Socionext Synquacer platform Masahisa Kojima @ 2019-05-21 11:59 ` Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 2/3] dt-bindings: spi: Add DT bindings for Synquacer Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima 2 siblings, 0 replies; 12+ messages in thread From: Masahisa Kojima @ 2019-05-21 11:59 UTC (permalink / raw) To: linux-spi, devicetree Cc: broonie, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, ard.biesheuvel, jaswinder.singh, masami.hiramatsu, okamoto.satoru, osaki.yoshitoyo, Masahisa Kojima Add entry for the Synquacer spi driver and DT bindings. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 0c55b0fedbe2..a4970349ef4c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14597,6 +14597,14 @@ S: Maintained F: drivers/net/ethernet/socionext/netsec.c F: Documentation/devicetree/bindings/net/socionext-netsec.txt +SOCIONEXT (SNI) Synquacer SPI DRIVER +M: Masahisa Kojima <masahisa.kojima@linaro.org> +M: Jassi Brar <jaswinder.singh@linaro.org> +L: linux-spi@vger.kernel.org +S: Maintained +F: drivers/spi/spi-synquacer.c +F: Documentation/devicetree/bindings/spi/spi-synquacer.txt + SOLIDRUN CLEARFOG SUPPORT M: Russell King <linux@armlinux.org.uk> S: Maintained -- 2.14.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 2/3] dt-bindings: spi: Add DT bindings for Synquacer 2019-05-21 11:59 [PATCH v5 0/3] spi: support for Socionext Synquacer platform Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 1/3] MAINTAINERS: Add entry for Synquacer SPI driver Masahisa Kojima @ 2019-05-21 11:59 ` Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima 2 siblings, 0 replies; 12+ messages in thread From: Masahisa Kojima @ 2019-05-21 11:59 UTC (permalink / raw) To: linux-spi, devicetree Cc: broonie, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, ard.biesheuvel, jaswinder.singh, masami.hiramatsu, okamoto.satoru, osaki.yoshitoyo, Masahisa Kojima This patch adds documentation for Device-Tree bindings for the Socionext Synquacer spi driver. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> Reviewed-by: Rob Herring <robh@kernel.org> --- .../devicetree/bindings/spi/spi-synquacer.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-synquacer.txt diff --git a/Documentation/devicetree/bindings/spi/spi-synquacer.txt b/Documentation/devicetree/bindings/spi/spi-synquacer.txt new file mode 100644 index 000000000000..291dfa692d0a --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-synquacer.txt @@ -0,0 +1,27 @@ +* Socionext Synquacer HS-SPI bindings + +Required Properties: +- compatible: should be "socionext,synquacer-spi" +- reg: physical base address of the controller and length of memory mapped + region. +- interrupts: should contain the "spi_rx", "spi_tx" and "spi_fault" interrupts. +- clocks: core clock iHCLK. Optional rate clock iPCLK (default is iHCLK) +- clock-names: Shall be "iHCLK" and "iPCLK" respectively + +Optional Properties: +- socionext,use-rtm: boolean, if required to use "retimed clock" for RX +- socionext,set-aces: boolean, if same active clock edges field to be set. + +Example: + + spi0: spi@ff110000 { + compatible = "socionext,synquacer-spi"; + reg = <0xff110000 0x1000>; + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk_hsspi>; + clock-names = "iHCLK"; + socionext,use-rtm; + socionext,set-aces; + }; -- 2.14.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 11:59 [PATCH v5 0/3] spi: support for Socionext Synquacer platform Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 1/3] MAINTAINERS: Add entry for Synquacer SPI driver Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 2/3] dt-bindings: spi: Add DT bindings for Synquacer Masahisa Kojima @ 2019-05-21 11:59 ` Masahisa Kojima 2019-05-21 13:55 ` Ard Biesheuvel ` (2 more replies) 2 siblings, 3 replies; 12+ messages in thread From: Masahisa Kojima @ 2019-05-21 11:59 UTC (permalink / raw) To: linux-spi, devicetree Cc: broonie, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, ard.biesheuvel, jaswinder.singh, masami.hiramatsu, okamoto.satoru, osaki.yoshitoyo, Masahisa Kojima This patch adds support for controller found on synquacer platforms. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- drivers/spi/Kconfig | 11 + drivers/spi/Makefile | 1 + drivers/spi/spi-synquacer.c | 731 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 743 insertions(+) create mode 100644 drivers/spi/spi-synquacer.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 0fba8f400c59..24a3eac72d12 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -732,6 +732,17 @@ config SPI_SUN6I help This enables using the SPI controller on the Allwinner A31 SoCs. +config SPI_SYNQUACER + tristate "Socionext's Synquacer HighSpeed SPI controller" + depends on ARCH_SYNQUACER || COMPILE_TEST + select SPI_BITBANG + help + SPI driver for Socionext's High speed SPI controller which provides + various operating modes for interfacing to serial peripheral devices + that use the de-facto standard SPI protocol. + + It also supports the new dual-bit and quad-bit SPI protocol. + config SPI_MXIC tristate "Macronix MX25F0A SPI controller" depends on SPI_MASTER diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index f2f78d03dc28..63dcab552bcb 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -106,6 +106,7 @@ obj-$(CONFIG_SPI_STM32_QSPI) += spi-stm32-qspi.o obj-$(CONFIG_SPI_ST_SSC4) += spi-st-ssc4.o obj-$(CONFIG_SPI_SUN4I) += spi-sun4i.o obj-$(CONFIG_SPI_SUN6I) += spi-sun6i.o +obj-$(CONFIG_SPI_SYNQUACER) += spi-synquacer.o obj-$(CONFIG_SPI_TEGRA114) += spi-tegra114.o obj-$(CONFIG_SPI_TEGRA20_SFLASH) += spi-tegra20-sflash.o obj-$(CONFIG_SPI_TEGRA20_SLINK) += spi-tegra20-slink.o diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c new file mode 100644 index 000000000000..04ba76609a12 --- /dev/null +++ b/drivers/spi/spi-synquacer.c @@ -0,0 +1,731 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Synquacer HSSPI controller driver +// +// Copyright (c) 2015-2018 Socionext Inc. +// Copyright (c) 2018-2019 Linaro Ltd. +// + +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/scatterlist.h> +#include <linux/slab.h> +#include <linux/spi/spi.h> +#include <linux/spinlock.h> +#include <linux/clk.h> + +/* HSSPI register address definitions */ +#define SYNQUACER_HSSPI_REG_MCTRL 0x00 +#define SYNQUACER_HSSPI_REG_PCC0 0x04 +#define SYNQUACER_HSSPI_REG_PCC(n) (SYNQUACER_HSSPI_REG_PCC0 + (n) * 4) +#define SYNQUACER_HSSPI_REG_TXF 0x14 +#define SYNQUACER_HSSPI_REG_TXE 0x18 +#define SYNQUACER_HSSPI_REG_TXC 0x1C +#define SYNQUACER_HSSPI_REG_RXF 0x20 +#define SYNQUACER_HSSPI_REG_RXE 0x24 +#define SYNQUACER_HSSPI_REG_RXC 0x28 +#define SYNQUACER_HSSPI_REG_FAULTF 0x2C +#define SYNQUACER_HSSPI_REG_FAULTC 0x30 +#define SYNQUACER_HSSPI_REG_DMCFG 0x34 +#define SYNQUACER_HSSPI_REG_DMSTART 0x38 +#define SYNQUACER_HSSPI_REG_DMBCC 0x3C +#define SYNQUACER_HSSPI_REG_DMSTATUS 0x40 +#define SYNQUACER_HSSPI_REG_FIFOCFG 0x4C +#define SYNQUACER_HSSPI_REG_TX_FIFO 0x50 +#define SYNQUACER_HSSPI_REG_RX_FIFO 0x90 +#define SYNQUACER_HSSPI_REG_MID 0xFC + +/* HSSPI register bit definitions */ +#define SYNQUACER_HSSPI_MCTRL_MEN BIT(0) +#define SYNQUACER_HSSPI_MCTRL_COMMAND_SEQUENCE_EN BIT(1) +#define SYNQUACER_HSSPI_MCTRL_CDSS BIT(3) +#define SYNQUACER_HSSPI_MCTRL_MES BIT(4) +#define SYNQUACER_HSSPI_MCTRL_SYNCON BIT(5) + +#define SYNQUACER_HSSPI_PCC_CPHA BIT(0) +#define SYNQUACER_HSSPI_PCC_CPOL BIT(1) +#define SYNQUACER_HSSPI_PCC_ACES BIT(2) +#define SYNQUACER_HSSPI_PCC_RTM BIT(3) +#define SYNQUACER_HSSPI_PCC_SSPOL BIT(4) +#define SYNQUACER_HSSPI_PCC_SDIR BIT(7) +#define SYNQUACER_HSSPI_PCC_SENDIAN BIT(8) +#define SYNQUACER_HSSPI_PCC_SAFESYNC BIT(16) +#define SYNQUACER_HSSPI_PCC_SS2CD_SHIFT 5 +#define SYNQUACER_HSSPI_PCC_CDRS_MASK 0x7f +#define SYNQUACER_HSSPI_PCC_CDRS_SHIFT 9 + +#define SYNQUACER_HSSPI_TXF_FIFO_FULL BIT(0) +#define SYNQUACER_HSSPI_TXF_FIFO_EMPTY BIT(1) +#define SYNQUACER_HSSPI_TXF_SLAVE_RELEASED BIT(6) + +#define SYNQUACER_HSSPI_TXE_FIFO_FULL BIT(0) +#define SYNQUACER_HSSPI_TXE_FIFO_EMPTY BIT(1) +#define SYNQUACER_HSSPI_TXE_SLAVE_RELEASED BIT(6) + +#define SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD BIT(5) +#define SYNQUACER_HSSPI_RXF_SLAVE_RELEASED BIT(6) + +#define SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD BIT(5) +#define SYNQUACER_HSSPI_RXE_SLAVE_RELEASED BIT(6) + +#define SYNQUACER_HSSPI_DMCFG_SSDC BIT(1) +#define SYNQUACER_HSSPI_DMCFG_MSTARTEN BIT(2) + +#define SYNQUACER_HSSPI_DMSTART_START BIT(0) +#define SYNQUACER_HSSPI_DMSTOP_STOP BIT(8) +#define SYNQUACER_HSSPI_DMPSEL_CS_MASK 0x3 +#define SYNQUACER_HSSPI_DMPSEL_CS_SHIFT 16 +#define SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT 24 +#define SYNQUACER_HSSPI_DMTRP_DATA_MASK 0x3 +#define SYNQUACER_HSSPI_DMTRP_DATA_SHIFT 26 +#define SYNQUACER_HSSPI_DMTRP_DATA_TXRX 0 +#define SYNQUACER_HSSPI_DMTRP_DATA_RX 1 +#define SYNQUACER_HSSPI_DMTRP_DATA_TX 2 + +#define SYNQUACER_HSSPI_DMSTATUS_RX_DATA_MASK 0x1f +#define SYNQUACER_HSSPI_DMSTATUS_RX_DATA_SHIFT 8 +#define SYNQUACER_HSSPI_DMSTATUS_TX_DATA_MASK 0x1f +#define SYNQUACER_HSSPI_DMSTATUS_TX_DATA_SHIFT 16 + +#define SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_MASK 0xf +#define SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT 0 +#define SYNQUACER_HSSPI_FIFOCFG_TX_THRESHOLD_MASK 0xf +#define SYNQUACER_HSSPI_FIFOCFG_TX_THRESHOLD_SHIFT 4 +#define SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_MASK 0x3 +#define SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT 8 +#define SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH BIT(11) +#define SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH BIT(12) + +#define SYNQUACER_HSSPI_FIFO_DEPTH 16 +#define SYNQUACER_HSSPI_FIFO_TX_THRESHOLD 4 +#define SYNQUACER_HSSPI_FIFO_RX_THRESHOLD \ + (SYNQUACER_HSSPI_FIFO_DEPTH - SYNQUACER_HSSPI_FIFO_TX_THRESHOLD) + +#define SYNQUACER_HSSPI_TRANSFER_MODE_TX BIT(1) +#define SYNQUACER_HSSPI_TRANSFER_MODE_RX BIT(2) +#define SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC 2000 + +#define SYNQUACER_HSSPI_CLOCK_SRC_IHCLK 0 +#define SYNQUACER_HSSPI_CLOCK_SRC_IPCLK 1 + +#define SYNQUACER_HSSPI_NUM_CHIP_SELECT 4 + +struct synquacer_spi { + struct device *dev; + struct completion transfer_done; + unsigned int cs; + unsigned int bpw; + unsigned int mode; + unsigned int speed; + bool aces, rtm; + void *rx_buf; + const void *tx_buf; + struct clk *clk; + int clk_src_type; + void __iomem *regs; + unsigned int tx_words, rx_words; + unsigned int bus_width; + unsigned int transfer_mode; +}; + +static void read_fifo(struct synquacer_spi *sspi) +{ + u32 len = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTATUS); + + len = (len >> SYNQUACER_HSSPI_DMSTATUS_RX_DATA_SHIFT) & + SYNQUACER_HSSPI_DMSTATUS_RX_DATA_MASK; + len = min_t(unsigned int, len, sspi->rx_words); + + switch (sspi->bpw) { + case 8: + { + u8 *buf = sspi->rx_buf; + + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); + sspi->rx_buf = buf + len; + break; + } + case 16: + { + u16 *buf = sspi->rx_buf; + + readsw(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); + sspi->rx_buf = buf + len; + break; + } + default: + { + u32 *buf = sspi->rx_buf; + + readsl(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); + sspi->rx_buf = buf + len; + break; + } + } + + sspi->rx_words -= len; +} + +static void write_fifo(struct synquacer_spi *sspi) +{ + u32 len = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTATUS); + + len = (len >> SYNQUACER_HSSPI_DMSTATUS_TX_DATA_SHIFT) & + SYNQUACER_HSSPI_DMSTATUS_TX_DATA_MASK; + len = min_t(unsigned int, SYNQUACER_HSSPI_FIFO_DEPTH - len, + sspi->tx_words); + + switch (sspi->bpw) { + case 8: + { + const u8 *buf = sspi->tx_buf; + + writesb(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); + sspi->tx_buf = buf + len; + break; + } + case 16: + { + const u16 *buf = sspi->tx_buf; + + writesw(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); + sspi->tx_buf = buf + len; + break; + } + default: + { + const u32 *buf = sspi->tx_buf; + + writesl(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); + sspi->tx_buf = buf + len; + break; + } + } + sspi->tx_words -= len; +} + +static int synquacer_spi_config(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct synquacer_spi *sspi = spi_master_get_devdata(master); + unsigned int speed, mode, bpw, cs, bus_width, transfer_mode; + u32 rate, val, div; + + /* Full Duplex only on 1-bit wide bus */ + if (xfer->rx_buf && xfer->tx_buf && + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { + dev_err(sspi->dev, + "RX and TX bus widths must match for Full-Duplex!\n"); + return -EINVAL; + } + + if (xfer->tx_buf) { + bus_width = xfer->tx_nbits; + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_TX; + } else { + bus_width = xfer->rx_nbits; + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_RX; + } + + mode = spi->mode; + cs = spi->chip_select; + speed = xfer->speed_hz; + bpw = xfer->bits_per_word; + + /* return if nothing to change */ + if (speed == sspi->speed && + bus_width == sspi->bus_width && bpw == sspi->bpw && + mode == sspi->mode && cs == sspi->cs && + transfer_mode == sspi->transfer_mode) { + return 0; + } + + sspi->transfer_mode = transfer_mode; + rate = master->max_speed_hz; + + div = DIV_ROUND_UP(rate, speed); + if (div > 254) { + dev_err(sspi->dev, "Requested rate too low (%u)\n", + sspi->speed); + return -EINVAL; + } + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_PCC(cs)); + val &= ~SYNQUACER_HSSPI_PCC_SAFESYNC; + if (bpw == 8 && (mode & (SPI_TX_DUAL | SPI_RX_DUAL)) && div < 3) + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; + if (bpw == 8 && (mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 6) + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; + if (bpw == 16 && (mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 3) + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; + + if (mode & SPI_CPHA) + val |= SYNQUACER_HSSPI_PCC_CPHA; + else + val &= ~SYNQUACER_HSSPI_PCC_CPHA; + + if (mode & SPI_CPOL) + val |= SYNQUACER_HSSPI_PCC_CPOL; + else + val &= ~SYNQUACER_HSSPI_PCC_CPOL; + + if (mode & SPI_CS_HIGH) + val |= SYNQUACER_HSSPI_PCC_SSPOL; + else + val &= ~SYNQUACER_HSSPI_PCC_SSPOL; + + if (mode & SPI_LSB_FIRST) + val |= SYNQUACER_HSSPI_PCC_SDIR; + else + val &= ~SYNQUACER_HSSPI_PCC_SDIR; + + if (sspi->aces) + val |= SYNQUACER_HSSPI_PCC_ACES; + else + val &= ~SYNQUACER_HSSPI_PCC_ACES; + + if (sspi->rtm) + val |= SYNQUACER_HSSPI_PCC_RTM; + else + val &= ~SYNQUACER_HSSPI_PCC_RTM; + + val |= (3 << SYNQUACER_HSSPI_PCC_SS2CD_SHIFT); + val |= SYNQUACER_HSSPI_PCC_SENDIAN; + + val &= ~(SYNQUACER_HSSPI_PCC_CDRS_MASK << + SYNQUACER_HSSPI_PCC_CDRS_SHIFT); + val |= ((div >> 1) << SYNQUACER_HSSPI_PCC_CDRS_SHIFT); + + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_PCC(cs)); + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + val &= ~(SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_MASK << + SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT); + val |= ((bpw / 8 - 1) << SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT); + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + val &= ~(SYNQUACER_HSSPI_DMTRP_DATA_MASK << + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); + + if (xfer->rx_buf) + val |= (SYNQUACER_HSSPI_DMTRP_DATA_RX << + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); + else + val |= (SYNQUACER_HSSPI_DMTRP_DATA_TX << + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); + + val &= ~(3 << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); + val |= ((bus_width >> 1) << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + + sspi->bpw = bpw; + sspi->mode = mode; + sspi->speed = speed; + sspi->cs = spi->chip_select; + sspi->bus_width = bus_width; + + return 0; +} + +static int synquacer_spi_transfer_one(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct synquacer_spi *sspi = spi_master_get_devdata(master); + int ret; + int status = 0; + unsigned int words; + u8 bpw; + u32 val; + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH; + val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + + /* + * See if we can transfer 4-bytes as 1 word + * to maximize the FIFO buffer effficiency + */ + bpw = xfer->bits_per_word; + if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST)) + xfer->bits_per_word = 32; + + ret = synquacer_spi_config(master, spi, xfer); + + /* restore */ + xfer->bits_per_word = bpw; + + if (ret) + return ret; + + reinit_completion(&sspi->transfer_done); + + sspi->tx_buf = xfer->tx_buf; + sspi->rx_buf = xfer->rx_buf; + + switch (sspi->bpw) { + case 8: + words = xfer->len; + break; + case 16: + words = xfer->len / 2; + break; + default: + words = xfer->len / 4; + break; + } + + if (xfer->tx_buf) + sspi->tx_words = words; + else + sspi->tx_words = 0; + + if (xfer->rx_buf) + sspi->rx_words = words; + else + sspi->rx_words = 0; + + if (xfer->tx_buf) + write_fifo(sspi); + + if (xfer->rx_buf) { + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + val &= ~(SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_MASK << + SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT); + val |= ((sspi->rx_words > SYNQUACER_HSSPI_FIFO_DEPTH ? + SYNQUACER_HSSPI_FIFO_RX_THRESHOLD : sspi->rx_words) << + SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT); + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); + } + + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_TXC); + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_RXC); + + /* Trigger */ + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + val |= SYNQUACER_HSSPI_DMSTART_START; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + + if (sspi->rx_words) { + val = SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD | + SYNQUACER_HSSPI_RXE_SLAVE_RELEASED; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_RXE); + status = wait_for_completion_timeout(&sspi->transfer_done, + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); + } + + if (xfer->tx_buf) { + val = SYNQUACER_HSSPI_TXE_FIFO_EMPTY; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_TXE); + status = wait_for_completion_timeout(&sspi->transfer_done, + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); + } + + if (status < 0) { + dev_err(sspi->dev, "failed to transfer\n"); + return status; + } + + return 0; +} + +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable) +{ + struct synquacer_spi *sspi = spi_master_get_devdata(spi->master); + u32 val; + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK << + SYNQUACER_HSSPI_DMPSEL_CS_SHIFT); + val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT; + + if (enable) { + val |= SYNQUACER_HSSPI_DMSTOP_STOP; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + + if (sspi->rx_buf) { + u32 buf[SYNQUACER_HSSPI_FIFO_DEPTH]; + + sspi->rx_buf = buf; + sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH; + read_fifo(sspi); + } + } else { + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + val &= ~SYNQUACER_HSSPI_DMSTOP_STOP; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); + } +} + +static int synquacer_spi_enable(struct spi_master *master) +{ + struct synquacer_spi *sspi = spi_master_get_devdata(master); + u32 val; + unsigned int retries = 0xfffff; + + /* Disable module */ + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); + while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) & + SYNQUACER_HSSPI_MCTRL_MES) && --retries) + cpu_relax(); + if (!retries) + return -EBUSY; + + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_TXC); + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_RXC); + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_FAULTC); + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMCFG); + val &= ~SYNQUACER_HSSPI_DMCFG_SSDC; + val &= ~SYNQUACER_HSSPI_DMCFG_MSTARTEN; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMCFG); + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); + if (sspi->clk_src_type == SYNQUACER_HSSPI_CLOCK_SRC_IPCLK) + val |= SYNQUACER_HSSPI_MCTRL_CDSS; + else + val &= ~SYNQUACER_HSSPI_MCTRL_CDSS; + + val &= ~SYNQUACER_HSSPI_MCTRL_COMMAND_SEQUENCE_EN; + val |= SYNQUACER_HSSPI_MCTRL_MEN; + val |= SYNQUACER_HSSPI_MCTRL_SYNCON; + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); + + return 0; +} + +static irqreturn_t sq_spi_rx_handler(int irq, void *priv) +{ + uint32_t val; + struct synquacer_spi *sspi = priv; + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_RXF); + if ((val & SYNQUACER_HSSPI_RXF_SLAVE_RELEASED) || + (val & SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD)) + read_fifo(sspi); + + if (sspi->rx_words == 0) { + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); + complete(&sspi->transfer_done); + } + + return 0; +} + +static irqreturn_t sq_spi_tx_handler(int irq, void *priv) +{ + uint32_t val; + struct synquacer_spi *sspi = priv; + + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF); + + if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) { + if (sspi->tx_words == 0) { + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); + complete(&sspi->transfer_done); + return 0; + } + write_fifo(sspi); + } + + return 0; +} + +static int synquacer_spi_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct spi_master *master; + struct synquacer_spi *sspi; + struct resource *res; + int ret; + int rx_irq, tx_irq; + + master = spi_alloc_master(&pdev->dev, sizeof(*sspi)); + if (!master) + return -ENOMEM; + + platform_set_drvdata(pdev, master); + + sspi = spi_master_get_devdata(master); + sspi->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + sspi->regs = devm_ioremap_resource(sspi->dev, res); + if (IS_ERR(sspi->regs)) { + ret = PTR_ERR(sspi->regs); + goto put_spi; + } + + if (of_property_match_string(np, "clock-names", "iHCLK") >= 0) { + sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IHCLK; + sspi->clk = devm_clk_get(sspi->dev, "iHCLK"); + } else if (of_property_match_string(np, "clock-names", "iPCLK") >= 0) { + sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IPCLK; + sspi->clk = devm_clk_get(sspi->dev, "iPCLK"); + } else { + dev_err(&pdev->dev, "specified wrong clock source\n"); + ret = -EINVAL; + goto put_spi; + } + if (IS_ERR(sspi->clk)) { + dev_err(&pdev->dev, "clock not found\n"); + ret = PTR_ERR(sspi->clk); + goto put_spi; + } + + sspi->aces = of_property_read_bool(np, "socionext,set-aces"); + sspi->rtm = of_property_read_bool(np, "socionext,use-rtm"); + + master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT; + + init_completion(&sspi->transfer_done); + + rx_irq = platform_get_irq(pdev, 0); + if (rx_irq < 0) + dev_err(&pdev->dev, "get rx_irq failed\n"); + + tx_irq = platform_get_irq(pdev, 1); + if (tx_irq < 0) + dev_err(&pdev->dev, "get tx_irq failed\n"); + + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, + 0, "synquacer-spi-rx", sspi); + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, + 0, "synquacer-spi-tx", sspi); + + ret = clk_prepare_enable(sspi->clk); + if (ret) + goto put_spi; + + master->dev.of_node = np; + master->auto_runtime_pm = true; + master->bus_num = pdev->id; + + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | + SPI_TX_QUAD | SPI_RX_QUAD; + master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) | + SPI_BPW_MASK(16) | SPI_BPW_MASK(8); + + master->max_speed_hz = clk_get_rate(sspi->clk); + master->min_speed_hz = master->max_speed_hz / 254; + + master->set_cs = synquacer_spi_set_cs; + master->transfer_one = synquacer_spi_transfer_one; + + ret = synquacer_spi_enable(master); + if (ret) + goto fail_enable; + + pm_runtime_set_active(sspi->dev); + pm_runtime_enable(sspi->dev); + + ret = devm_spi_register_master(sspi->dev, master); + if (ret) + goto disable_pm; + + return 0; + +disable_pm: + pm_runtime_disable(sspi->dev); +fail_enable: + clk_disable_unprepare(sspi->clk); +put_spi: + spi_master_put(master); + + return ret; +} + +static int synquacer_spi_remove(struct platform_device *pdev) +{ + struct spi_master *master = platform_get_drvdata(pdev); + struct synquacer_spi *sspi = spi_master_get_devdata(master); + + pm_runtime_disable(sspi->dev); + clk_disable_unprepare(sspi->clk); + return 0; +} + +static int __maybe_unused synquacer_spi_suspend(struct device *dev) +{ + struct spi_master *master = dev_get_drvdata(dev); + struct synquacer_spi *sspi = spi_master_get_devdata(master); + int ret; + + ret = spi_master_suspend(master); + if (ret) + return ret; + + if (!pm_runtime_suspended(dev)) + clk_disable_unprepare(sspi->clk); + + return ret; +} + +static int __maybe_unused synquacer_spi_resume(struct device *dev) +{ + struct spi_master *master = dev_get_drvdata(dev); + struct synquacer_spi *sspi = spi_master_get_devdata(master); + int ret; + + if (!pm_runtime_suspended(dev)) { + /* Ensure reconfigure during next xfer */ + sspi->speed = 0; + + ret = clk_prepare_enable(sspi->clk); + if (ret < 0) { + dev_err(dev, "failed to enable clk (%d)\n", ret); + return ret; + } + + ret = synquacer_spi_enable(master); + if (ret) { + dev_err(dev, "failed to enable spi (%d)\n", ret); + return ret; + } + } + + ret = spi_master_resume(master); + if (ret < 0) + clk_disable_unprepare(sspi->clk); + + return ret; +} + +static SIMPLE_DEV_PM_OPS(synquacer_spi_pm_ops, synquacer_spi_suspend, + synquacer_spi_resume); + +static const struct of_device_id synquacer_spi_of_match[] = { + {.compatible = "socionext,synquacer-spi"}, + {} +}; +MODULE_DEVICE_TABLE(of, synquacer_spi_of_match); + +static struct platform_driver synquacer_spi_driver = { + .driver = { + .name = "synquacer-spi", + .pm = &synquacer_spi_pm_ops, + .of_match_table = synquacer_spi_of_match, + }, + .probe = synquacer_spi_probe, + .remove = synquacer_spi_remove, +}; +module_platform_driver(synquacer_spi_driver); + +MODULE_DESCRIPTION("Socionext Synquacer HS-SPI controller driver"); +MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>"); +MODULE_AUTHOR("Jassi Brar <jaswinder.singh@linaro.org>"); +MODULE_LICENSE("GPL v2"); -- 2.14.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima @ 2019-05-21 13:55 ` Ard Biesheuvel 2019-05-21 16:27 ` Andy Shevchenko 2019-05-21 16:38 ` Andy Shevchenko 2019-05-21 18:16 ` Mark Brown 2 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2019-05-21 13:55 UTC (permalink / raw) To: Masahisa Kojima Cc: linux-spi, Devicetree List, Mark Brown, Geert Uytterhoeven, Trent Piepho, Andy Shevchenko, Rob Herring, Mark Rutland, Jaswinder Singh, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki On Tue, 21 May 2019 at 13:00, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > This patch adds support for controller found on synquacer platforms. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > --- > drivers/spi/Kconfig | 11 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-synquacer.c | 731 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 743 insertions(+) > create mode 100644 drivers/spi/spi-synquacer.c > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > index 0fba8f400c59..24a3eac72d12 100644 > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -732,6 +732,17 @@ config SPI_SUN6I > help > This enables using the SPI controller on the Allwinner A31 SoCs. > > +config SPI_SYNQUACER > + tristate "Socionext's Synquacer HighSpeed SPI controller" > + depends on ARCH_SYNQUACER || COMPILE_TEST > + select SPI_BITBANG > + help > + SPI driver for Socionext's High speed SPI controller which provides > + various operating modes for interfacing to serial peripheral devices > + that use the de-facto standard SPI protocol. > + > + It also supports the new dual-bit and quad-bit SPI protocol. > + > config SPI_MXIC > tristate "Macronix MX25F0A SPI controller" > depends on SPI_MASTER > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > index f2f78d03dc28..63dcab552bcb 100644 > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -106,6 +106,7 @@ obj-$(CONFIG_SPI_STM32_QSPI) += spi-stm32-qspi.o > obj-$(CONFIG_SPI_ST_SSC4) += spi-st-ssc4.o > obj-$(CONFIG_SPI_SUN4I) += spi-sun4i.o > obj-$(CONFIG_SPI_SUN6I) += spi-sun6i.o > +obj-$(CONFIG_SPI_SYNQUACER) += spi-synquacer.o > obj-$(CONFIG_SPI_TEGRA114) += spi-tegra114.o > obj-$(CONFIG_SPI_TEGRA20_SFLASH) += spi-tegra20-sflash.o > obj-$(CONFIG_SPI_TEGRA20_SLINK) += spi-tegra20-slink.o > diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c > new file mode 100644 > index 000000000000..04ba76609a12 > --- /dev/null > +++ b/drivers/spi/spi-synquacer.c > @@ -0,0 +1,731 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Synquacer HSSPI controller driver > +// > +// Copyright (c) 2015-2018 Socionext Inc. > +// Copyright (c) 2018-2019 Linaro Ltd. > +// > + > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/scatterlist.h> > +#include <linux/slab.h> > +#include <linux/spi/spi.h> > +#include <linux/spinlock.h> > +#include <linux/clk.h> > + > +/* HSSPI register address definitions */ > +#define SYNQUACER_HSSPI_REG_MCTRL 0x00 > +#define SYNQUACER_HSSPI_REG_PCC0 0x04 > +#define SYNQUACER_HSSPI_REG_PCC(n) (SYNQUACER_HSSPI_REG_PCC0 + (n) * 4) > +#define SYNQUACER_HSSPI_REG_TXF 0x14 > +#define SYNQUACER_HSSPI_REG_TXE 0x18 > +#define SYNQUACER_HSSPI_REG_TXC 0x1C > +#define SYNQUACER_HSSPI_REG_RXF 0x20 > +#define SYNQUACER_HSSPI_REG_RXE 0x24 > +#define SYNQUACER_HSSPI_REG_RXC 0x28 > +#define SYNQUACER_HSSPI_REG_FAULTF 0x2C > +#define SYNQUACER_HSSPI_REG_FAULTC 0x30 > +#define SYNQUACER_HSSPI_REG_DMCFG 0x34 > +#define SYNQUACER_HSSPI_REG_DMSTART 0x38 > +#define SYNQUACER_HSSPI_REG_DMBCC 0x3C > +#define SYNQUACER_HSSPI_REG_DMSTATUS 0x40 > +#define SYNQUACER_HSSPI_REG_FIFOCFG 0x4C > +#define SYNQUACER_HSSPI_REG_TX_FIFO 0x50 > +#define SYNQUACER_HSSPI_REG_RX_FIFO 0x90 > +#define SYNQUACER_HSSPI_REG_MID 0xFC > + > +/* HSSPI register bit definitions */ > +#define SYNQUACER_HSSPI_MCTRL_MEN BIT(0) > +#define SYNQUACER_HSSPI_MCTRL_COMMAND_SEQUENCE_EN BIT(1) > +#define SYNQUACER_HSSPI_MCTRL_CDSS BIT(3) > +#define SYNQUACER_HSSPI_MCTRL_MES BIT(4) > +#define SYNQUACER_HSSPI_MCTRL_SYNCON BIT(5) > + > +#define SYNQUACER_HSSPI_PCC_CPHA BIT(0) > +#define SYNQUACER_HSSPI_PCC_CPOL BIT(1) > +#define SYNQUACER_HSSPI_PCC_ACES BIT(2) > +#define SYNQUACER_HSSPI_PCC_RTM BIT(3) > +#define SYNQUACER_HSSPI_PCC_SSPOL BIT(4) > +#define SYNQUACER_HSSPI_PCC_SDIR BIT(7) > +#define SYNQUACER_HSSPI_PCC_SENDIAN BIT(8) > +#define SYNQUACER_HSSPI_PCC_SAFESYNC BIT(16) > +#define SYNQUACER_HSSPI_PCC_SS2CD_SHIFT 5 > +#define SYNQUACER_HSSPI_PCC_CDRS_MASK 0x7f > +#define SYNQUACER_HSSPI_PCC_CDRS_SHIFT 9 > + > +#define SYNQUACER_HSSPI_TXF_FIFO_FULL BIT(0) > +#define SYNQUACER_HSSPI_TXF_FIFO_EMPTY BIT(1) > +#define SYNQUACER_HSSPI_TXF_SLAVE_RELEASED BIT(6) > + > +#define SYNQUACER_HSSPI_TXE_FIFO_FULL BIT(0) > +#define SYNQUACER_HSSPI_TXE_FIFO_EMPTY BIT(1) > +#define SYNQUACER_HSSPI_TXE_SLAVE_RELEASED BIT(6) > + > +#define SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD BIT(5) > +#define SYNQUACER_HSSPI_RXF_SLAVE_RELEASED BIT(6) > + > +#define SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD BIT(5) > +#define SYNQUACER_HSSPI_RXE_SLAVE_RELEASED BIT(6) > + > +#define SYNQUACER_HSSPI_DMCFG_SSDC BIT(1) > +#define SYNQUACER_HSSPI_DMCFG_MSTARTEN BIT(2) > + > +#define SYNQUACER_HSSPI_DMSTART_START BIT(0) > +#define SYNQUACER_HSSPI_DMSTOP_STOP BIT(8) > +#define SYNQUACER_HSSPI_DMPSEL_CS_MASK 0x3 > +#define SYNQUACER_HSSPI_DMPSEL_CS_SHIFT 16 > +#define SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT 24 > +#define SYNQUACER_HSSPI_DMTRP_DATA_MASK 0x3 > +#define SYNQUACER_HSSPI_DMTRP_DATA_SHIFT 26 > +#define SYNQUACER_HSSPI_DMTRP_DATA_TXRX 0 > +#define SYNQUACER_HSSPI_DMTRP_DATA_RX 1 > +#define SYNQUACER_HSSPI_DMTRP_DATA_TX 2 > + > +#define SYNQUACER_HSSPI_DMSTATUS_RX_DATA_MASK 0x1f > +#define SYNQUACER_HSSPI_DMSTATUS_RX_DATA_SHIFT 8 > +#define SYNQUACER_HSSPI_DMSTATUS_TX_DATA_MASK 0x1f > +#define SYNQUACER_HSSPI_DMSTATUS_TX_DATA_SHIFT 16 > + > +#define SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_MASK 0xf > +#define SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT 0 > +#define SYNQUACER_HSSPI_FIFOCFG_TX_THRESHOLD_MASK 0xf > +#define SYNQUACER_HSSPI_FIFOCFG_TX_THRESHOLD_SHIFT 4 > +#define SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_MASK 0x3 > +#define SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT 8 > +#define SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH BIT(11) > +#define SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH BIT(12) > + > +#define SYNQUACER_HSSPI_FIFO_DEPTH 16 > +#define SYNQUACER_HSSPI_FIFO_TX_THRESHOLD 4 > +#define SYNQUACER_HSSPI_FIFO_RX_THRESHOLD \ > + (SYNQUACER_HSSPI_FIFO_DEPTH - SYNQUACER_HSSPI_FIFO_TX_THRESHOLD) > + > +#define SYNQUACER_HSSPI_TRANSFER_MODE_TX BIT(1) > +#define SYNQUACER_HSSPI_TRANSFER_MODE_RX BIT(2) > +#define SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC 2000 > + > +#define SYNQUACER_HSSPI_CLOCK_SRC_IHCLK 0 > +#define SYNQUACER_HSSPI_CLOCK_SRC_IPCLK 1 > + > +#define SYNQUACER_HSSPI_NUM_CHIP_SELECT 4 > + > +struct synquacer_spi { > + struct device *dev; > + struct completion transfer_done; > + unsigned int cs; > + unsigned int bpw; > + unsigned int mode; > + unsigned int speed; > + bool aces, rtm; > + void *rx_buf; > + const void *tx_buf; > + struct clk *clk; > + int clk_src_type; > + void __iomem *regs; > + unsigned int tx_words, rx_words; > + unsigned int bus_width; > + unsigned int transfer_mode; > +}; > + > +static void read_fifo(struct synquacer_spi *sspi) > +{ > + u32 len = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTATUS); > + > + len = (len >> SYNQUACER_HSSPI_DMSTATUS_RX_DATA_SHIFT) & > + SYNQUACER_HSSPI_DMSTATUS_RX_DATA_MASK; > + len = min_t(unsigned int, len, sspi->rx_words); > + Please use the same type for len and rx_words instead of relying on min_t() (and use min() instead) > + switch (sspi->bpw) { > + case 8: > + { > + u8 *buf = sspi->rx_buf; > + > + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } > + case 16: > + { > + u16 *buf = sspi->rx_buf; > + > + readsw(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } > + default: > + { > + u32 *buf = sspi->rx_buf; > + > + readsl(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } > + } > + > + sspi->rx_words -= len; > +} > + > +static void write_fifo(struct synquacer_spi *sspi) > +{ > + u32 len = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTATUS); > + > + len = (len >> SYNQUACER_HSSPI_DMSTATUS_TX_DATA_SHIFT) & > + SYNQUACER_HSSPI_DMSTATUS_TX_DATA_MASK; > + len = min_t(unsigned int, SYNQUACER_HSSPI_FIFO_DEPTH - len, > + sspi->tx_words); > + Same here > + switch (sspi->bpw) { > + case 8: > + { > + const u8 *buf = sspi->tx_buf; > + > + writesb(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); > + sspi->tx_buf = buf + len; > + break; > + } > + case 16: > + { > + const u16 *buf = sspi->tx_buf; > + > + writesw(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); > + sspi->tx_buf = buf + len; > + break; > + } > + default: > + { > + const u32 *buf = sspi->tx_buf; > + > + writesl(sspi->regs + SYNQUACER_HSSPI_REG_TX_FIFO, buf, len); > + sspi->tx_buf = buf + len; > + break; > + } > + } > + sspi->tx_words -= len; > +} > + > +static int synquacer_spi_config(struct spi_master *master, > + struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + unsigned int speed, mode, bpw, cs, bus_width, transfer_mode; > + u32 rate, val, div; > + > + /* Full Duplex only on 1-bit wide bus */ > + if (xfer->rx_buf && xfer->tx_buf && > + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { > + dev_err(sspi->dev, > + "RX and TX bus widths must match for Full-Duplex!\n"); > + return -EINVAL; > + } > + > + if (xfer->tx_buf) { > + bus_width = xfer->tx_nbits; > + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_TX; > + } else { > + bus_width = xfer->rx_nbits; > + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_RX; > + } > + > + mode = spi->mode; > + cs = spi->chip_select; > + speed = xfer->speed_hz; > + bpw = xfer->bits_per_word; > + > + /* return if nothing to change */ > + if (speed == sspi->speed && > + bus_width == sspi->bus_width && bpw == sspi->bpw && > + mode == sspi->mode && cs == sspi->cs && > + transfer_mode == sspi->transfer_mode) { > + return 0; > + } > + > + sspi->transfer_mode = transfer_mode; > + rate = master->max_speed_hz; > + > + div = DIV_ROUND_UP(rate, speed); > + if (div > 254) { > + dev_err(sspi->dev, "Requested rate too low (%u)\n", > + sspi->speed); > + return -EINVAL; > + } > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_PCC(cs)); > + val &= ~SYNQUACER_HSSPI_PCC_SAFESYNC; > + if (bpw == 8 && (mode & (SPI_TX_DUAL | SPI_RX_DUAL)) && div < 3) > + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; > + if (bpw == 8 && (mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 6) > + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; > + if (bpw == 16 && (mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 3) > + val |= SYNQUACER_HSSPI_PCC_SAFESYNC; > + > + if (mode & SPI_CPHA) > + val |= SYNQUACER_HSSPI_PCC_CPHA; > + else > + val &= ~SYNQUACER_HSSPI_PCC_CPHA; > + > + if (mode & SPI_CPOL) > + val |= SYNQUACER_HSSPI_PCC_CPOL; > + else > + val &= ~SYNQUACER_HSSPI_PCC_CPOL; > + > + if (mode & SPI_CS_HIGH) > + val |= SYNQUACER_HSSPI_PCC_SSPOL; > + else > + val &= ~SYNQUACER_HSSPI_PCC_SSPOL; > + > + if (mode & SPI_LSB_FIRST) > + val |= SYNQUACER_HSSPI_PCC_SDIR; > + else > + val &= ~SYNQUACER_HSSPI_PCC_SDIR; > + > + if (sspi->aces) > + val |= SYNQUACER_HSSPI_PCC_ACES; > + else > + val &= ~SYNQUACER_HSSPI_PCC_ACES; > + > + if (sspi->rtm) > + val |= SYNQUACER_HSSPI_PCC_RTM; > + else > + val &= ~SYNQUACER_HSSPI_PCC_RTM; > + > + val |= (3 << SYNQUACER_HSSPI_PCC_SS2CD_SHIFT); > + val |= SYNQUACER_HSSPI_PCC_SENDIAN; > + > + val &= ~(SYNQUACER_HSSPI_PCC_CDRS_MASK << > + SYNQUACER_HSSPI_PCC_CDRS_SHIFT); > + val |= ((div >> 1) << SYNQUACER_HSSPI_PCC_CDRS_SHIFT); > + > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_PCC(cs)); > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + val &= ~(SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_MASK << > + SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT); > + val |= ((bpw / 8 - 1) << SYNQUACER_HSSPI_FIFOCFG_FIFO_WIDTH_SHIFT); > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + val &= ~(SYNQUACER_HSSPI_DMTRP_DATA_MASK << > + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); > + > + if (xfer->rx_buf) > + val |= (SYNQUACER_HSSPI_DMTRP_DATA_RX << > + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); > + else > + val |= (SYNQUACER_HSSPI_DMTRP_DATA_TX << > + SYNQUACER_HSSPI_DMTRP_DATA_SHIFT); > + > + val &= ~(3 << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); > + val |= ((bus_width >> 1) << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + In general, you should use readl and writel everywhere, *unless* there is a reason you need the _relaxed variants, in which case you need to provide justification that it is safe to do so (and you still need a barrier somewhere) > + sspi->bpw = bpw; > + sspi->mode = mode; > + sspi->speed = speed; > + sspi->cs = spi->chip_select; > + sspi->bus_width = bus_width; > + > + return 0; > +} > + > +static int synquacer_spi_transfer_one(struct spi_master *master, > + struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + int ret; > + int status = 0; > + unsigned int words; > + u8 bpw; > + u32 val; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH; > + val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + > + /* > + * See if we can transfer 4-bytes as 1 word > + * to maximize the FIFO buffer effficiency > + */ > + bpw = xfer->bits_per_word; > + if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST)) > + xfer->bits_per_word = 32; > + > + ret = synquacer_spi_config(master, spi, xfer); > + > + /* restore */ > + xfer->bits_per_word = bpw; > + > + if (ret) > + return ret; > + > + reinit_completion(&sspi->transfer_done); > + > + sspi->tx_buf = xfer->tx_buf; > + sspi->rx_buf = xfer->rx_buf; > + > + switch (sspi->bpw) { > + case 8: > + words = xfer->len; > + break; > + case 16: > + words = xfer->len / 2; > + break; > + default: > + words = xfer->len / 4; > + break; > + } > + > + if (xfer->tx_buf) > + sspi->tx_words = words; > + else > + sspi->tx_words = 0; > + > + if (xfer->rx_buf) > + sspi->rx_words = words; > + else > + sspi->rx_words = 0; > + > + if (xfer->tx_buf) > + write_fifo(sspi); > + > + if (xfer->rx_buf) { > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + val &= ~(SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_MASK << > + SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT); > + val |= ((sspi->rx_words > SYNQUACER_HSSPI_FIFO_DEPTH ? > + SYNQUACER_HSSPI_FIFO_RX_THRESHOLD : sspi->rx_words) << > + SYNQUACER_HSSPI_FIFOCFG_RX_THRESHOLD_SHIFT); > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + } > + > + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_TXC); > + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_RXC); > + > + /* Trigger */ > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + val |= SYNQUACER_HSSPI_DMSTART_START; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + > + if (sspi->rx_words) { > + val = SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD | > + SYNQUACER_HSSPI_RXE_SLAVE_RELEASED; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + status = wait_for_completion_timeout(&sspi->transfer_done, > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + } > + > + if (xfer->tx_buf) { > + val = SYNQUACER_HSSPI_TXE_FIFO_EMPTY; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + status = wait_for_completion_timeout(&sspi->transfer_done, > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + } > + > + if (status < 0) { > + dev_err(sspi->dev, "failed to transfer\n"); > + return status; > + } > + > + return 0; > +} > + > +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(spi->master); > + u32 val; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK << > + SYNQUACER_HSSPI_DMPSEL_CS_SHIFT); > + val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT; > + > + if (enable) { > + val |= SYNQUACER_HSSPI_DMSTOP_STOP; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + > + if (sspi->rx_buf) { > + u32 buf[SYNQUACER_HSSPI_FIFO_DEPTH]; > + > + sspi->rx_buf = buf; > + sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH; > + read_fifo(sspi); > + } > + } else { > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + val &= ~SYNQUACER_HSSPI_DMSTOP_STOP; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + } > +} > + > +static int synquacer_spi_enable(struct spi_master *master) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + u32 val; > + unsigned int retries = 0xfffff; > + > + /* Disable module */ > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); > + while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) & > + SYNQUACER_HSSPI_MCTRL_MES) && --retries) > + cpu_relax(); > + if (!retries) > + return -EBUSY; > + > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_TXC); > + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_RXC); > + writel_relaxed(~0, sspi->regs + SYNQUACER_HSSPI_REG_FAULTC); > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMCFG); > + val &= ~SYNQUACER_HSSPI_DMCFG_SSDC; > + val &= ~SYNQUACER_HSSPI_DMCFG_MSTARTEN; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMCFG); > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); > + if (sspi->clk_src_type == SYNQUACER_HSSPI_CLOCK_SRC_IPCLK) > + val |= SYNQUACER_HSSPI_MCTRL_CDSS; > + else > + val &= ~SYNQUACER_HSSPI_MCTRL_CDSS; > + > + val &= ~SYNQUACER_HSSPI_MCTRL_COMMAND_SEQUENCE_EN; > + val |= SYNQUACER_HSSPI_MCTRL_MEN; > + val |= SYNQUACER_HSSPI_MCTRL_SYNCON; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); > + > + return 0; > +} > + > +static irqreturn_t sq_spi_rx_handler(int irq, void *priv) > +{ > + uint32_t val; > + struct synquacer_spi *sspi = priv; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_RXF); > + if ((val & SYNQUACER_HSSPI_RXF_SLAVE_RELEASED) || > + (val & SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD)) > + read_fifo(sspi); > + > + if (sspi->rx_words == 0) { > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + complete(&sspi->transfer_done); > + } > + > + return 0; > +} > + > +static irqreturn_t sq_spi_tx_handler(int irq, void *priv) > +{ > + uint32_t val; > + struct synquacer_spi *sspi = priv; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF); > + > + if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) { > + if (sspi->tx_words == 0) { > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + complete(&sspi->transfer_done); > + return 0; > + } > + write_fifo(sspi); > + } > + > + return 0; > +} > + > +static int synquacer_spi_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct spi_master *master; > + struct synquacer_spi *sspi; > + struct resource *res; > + int ret; > + int rx_irq, tx_irq; > + > + master = spi_alloc_master(&pdev->dev, sizeof(*sspi)); > + if (!master) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, master); > + > + sspi = spi_master_get_devdata(master); > + sspi->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sspi->regs = devm_ioremap_resource(sspi->dev, res); > + if (IS_ERR(sspi->regs)) { > + ret = PTR_ERR(sspi->regs); > + goto put_spi; > + } > + > + if (of_property_match_string(np, "clock-names", "iHCLK") >= 0) { > + sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IHCLK; > + sspi->clk = devm_clk_get(sspi->dev, "iHCLK"); > + } else if (of_property_match_string(np, "clock-names", "iPCLK") >= 0) { > + sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IPCLK; > + sspi->clk = devm_clk_get(sspi->dev, "iPCLK"); > + } else { > + dev_err(&pdev->dev, "specified wrong clock source\n"); > + ret = -EINVAL; > + goto put_spi; > + } > + if (IS_ERR(sspi->clk)) { > + dev_err(&pdev->dev, "clock not found\n"); > + ret = PTR_ERR(sspi->clk); > + goto put_spi; > + } > + Please make the clock handling dependent on ACPI vs DT probing (look at the netsec or I2C drivers for examples) > + sspi->aces = of_property_read_bool(np, "socionext,set-aces"); > + sspi->rtm = of_property_read_bool(np, "socionext,use-rtm"); Please use the device_property API instead of the of_property API so that this works in ACPI mode as well > + > + master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT; > + > + init_completion(&sspi->transfer_done); > + > + rx_irq = platform_get_irq(pdev, 0); > + if (rx_irq < 0) > + dev_err(&pdev->dev, "get rx_irq failed\n"); > + > + tx_irq = platform_get_irq(pdev, 1); > + if (tx_irq < 0) > + dev_err(&pdev->dev, "get tx_irq failed\n"); > + > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > + 0, "synquacer-spi-rx", sspi); > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > + 0, "synquacer-spi-tx", sspi); > + Can you use the dev_name() for the IRQ names? You may need to sprintf() it into a buffer to append the rx and tx. > + ret = clk_prepare_enable(sspi->clk); > + if (ret) > + goto put_spi; > + Make this depend on DT probing > + master->dev.of_node = np; > + master->auto_runtime_pm = true; > + master->bus_num = pdev->id; > + > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | > + SPI_TX_QUAD | SPI_RX_QUAD; > + master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) | > + SPI_BPW_MASK(16) | SPI_BPW_MASK(8); > + > + master->max_speed_hz = clk_get_rate(sspi->clk); > + master->min_speed_hz = master->max_speed_hz / 254; > + > + master->set_cs = synquacer_spi_set_cs; > + master->transfer_one = synquacer_spi_transfer_one; > + > + ret = synquacer_spi_enable(master); > + if (ret) > + goto fail_enable; > + > + pm_runtime_set_active(sspi->dev); > + pm_runtime_enable(sspi->dev); > + > + ret = devm_spi_register_master(sspi->dev, master); > + if (ret) > + goto disable_pm; > + > + return 0; > + > +disable_pm: > + pm_runtime_disable(sspi->dev); > +fail_enable: > + clk_disable_unprepare(sspi->clk); > +put_spi: > + spi_master_put(master); > + > + return ret; > +} > + > +static int synquacer_spi_remove(struct platform_device *pdev) > +{ > + struct spi_master *master = platform_get_drvdata(pdev); > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + > + pm_runtime_disable(sspi->dev); > + clk_disable_unprepare(sspi->clk); depend on DT > + return 0; > +} > + > +static int __maybe_unused synquacer_spi_suspend(struct device *dev) > +{ > + struct spi_master *master = dev_get_drvdata(dev); > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + int ret; > + > + ret = spi_master_suspend(master); > + if (ret) > + return ret; > + > + if (!pm_runtime_suspended(dev)) > + clk_disable_unprepare(sspi->clk); depend on DT > + > + return ret; > +} > + > +static int __maybe_unused synquacer_spi_resume(struct device *dev) > +{ > + struct spi_master *master = dev_get_drvdata(dev); > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + int ret; > + > + if (!pm_runtime_suspended(dev)) { > + /* Ensure reconfigure during next xfer */ > + sspi->speed = 0; > + > + ret = clk_prepare_enable(sspi->clk); > + if (ret < 0) { > + dev_err(dev, "failed to enable clk (%d)\n", ret); > + return ret; > + } > + > + ret = synquacer_spi_enable(master); > + if (ret) { > + dev_err(dev, "failed to enable spi (%d)\n", ret); > + return ret; > + } > + } > + > + ret = spi_master_resume(master); > + if (ret < 0) > + clk_disable_unprepare(sspi->clk); > + > + return ret; > +} > + > +static SIMPLE_DEV_PM_OPS(synquacer_spi_pm_ops, synquacer_spi_suspend, > + synquacer_spi_resume); > + > +static const struct of_device_id synquacer_spi_of_match[] = { > + {.compatible = "socionext,synquacer-spi"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, synquacer_spi_of_match); > + Add a device table for ACPI (SCX0004) > +static struct platform_driver synquacer_spi_driver = { > + .driver = { > + .name = "synquacer-spi", > + .pm = &synquacer_spi_pm_ops, > + .of_match_table = synquacer_spi_of_match, Add a .acpi_match_table member > + }, > + .probe = synquacer_spi_probe, > + .remove = synquacer_spi_remove, > +}; > +module_platform_driver(synquacer_spi_driver); > + > +MODULE_DESCRIPTION("Socionext Synquacer HS-SPI controller driver"); > +MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>"); > +MODULE_AUTHOR("Jassi Brar <jaswinder.singh@linaro.org>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.14.2 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 13:55 ` Ard Biesheuvel @ 2019-05-21 16:27 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2019-05-21 16:27 UTC (permalink / raw) To: Ard Biesheuvel Cc: Masahisa Kojima, linux-spi, Devicetree List, Mark Brown, Geert Uytterhoeven, Trent Piepho, Rob Herring, Mark Rutland, Jaswinder Singh, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki On Tue, May 21, 2019 at 4:55 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On Tue, 21 May 2019 at 13:00, Masahisa Kojima > <masahisa.kojima@linaro.org> wrote: > > + ret = clk_prepare_enable(sspi->clk); > > + if (ret) > > + goto put_spi; > > + > > Make this depend on DT probing We may use devm_clk_get_optional() and drop dependencies. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima 2019-05-21 13:55 ` Ard Biesheuvel @ 2019-05-21 16:38 ` Andy Shevchenko 2019-05-27 8:31 ` Masahisa Kojima 2019-05-21 18:16 ` Mark Brown 2 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2019-05-21 16:38 UTC (permalink / raw) To: Masahisa Kojima Cc: linux-spi, devicetree, Mark Brown, Geert Uytterhoeven, Trent Piepho, Rob Herring, Mark Rutland, Ard Biesheuvel, Jassi Brar, Masami Hiramatsu, okamoto.satoru, osaki.yoshitoyo On Tue, May 21, 2019 at 3:00 PM Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > This patch adds support for controller found on synquacer platforms. > + case 8: > + { > + u8 *buf = sspi->rx_buf; > + > + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } Slightly better style case FOO: { ... } > + /* Full Duplex only on 1-bit wide bus */ > + if (xfer->rx_buf && xfer->tx_buf && > + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { > + dev_err(sspi->dev, > + "RX and TX bus widths must match for Full-Duplex!\n"); The message is not telling full truth. Not only match, but also be equal 1. > + return -EINVAL; > + } > +static int synquacer_spi_transfer_one(struct spi_master *master, > + struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + int ret; > + int status = 0; > + unsigned int words; > + u8 bpw; > + u32 val; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH; > + val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > + > + /* > + * See if we can transfer 4-bytes as 1 word > + * to maximize the FIFO buffer effficiency Typo here, and period is missed. > + */ > + case 8: > + words = xfer->len; > + break; > + case 16: > + words = xfer->len / 2; > + break; > + default: > + words = xfer->len / 4; > + break; Hmm... Shouldn't be rather "less then or equal" comparisons? > + unsigned int retries = 0xfffff; Hmm... better to use decimal value. > + /* Disable module */ > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); > + while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) & > + SYNQUACER_HSSPI_MCTRL_MES) && --retries) > + cpu_relax(); > + if (!retries) > + return -EBUSY; And here something like do { } while (--retries) would look slightly better due to understanding that we do at least one iteration. Also, can readx_poll_timeout be used here? > +static irqreturn_t sq_spi_tx_handler(int irq, void *priv) > +{ > + uint32_t val; > + struct synquacer_spi *sspi = priv; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF); > + > + if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) { > + if (sspi->tx_words == 0) { > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + complete(&sspi->transfer_done); > + return 0; irqreturn_t type, please. We have corresponding defines. > + } > + write_fifo(sspi); > + } > + > + return 0; Ditto. > +} > +static int synquacer_spi_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct spi_master *master; > + struct synquacer_spi *sspi; > + struct resource *res; > + int ret; > + int rx_irq, tx_irq; > + > + master = spi_alloc_master(&pdev->dev, sizeof(*sspi)); > + if (!master) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, master); > + > + sspi = spi_master_get_devdata(master); > + sspi->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sspi->regs = devm_ioremap_resource(sspi->dev, res); devm_platform_ioremap_resource() > + if (IS_ERR(sspi->regs)) { > + ret = PTR_ERR(sspi->regs); > + goto put_spi; > + } > + } else { > + dev_err(&pdev->dev, "specified wrong clock source\n"); > + ret = -EINVAL; > + goto put_spi; > + } Not an issue for ACPI. > + if (IS_ERR(sspi->clk)) { > + dev_err(&pdev->dev, "clock not found\n"); > + ret = PTR_ERR(sspi->clk); > + goto put_spi; > + } > + > + sspi->aces = of_property_read_bool(np, "socionext,set-aces"); > + sspi->rtm = of_property_read_bool(np, "socionext,use-rtm"); > + > + master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT; > + > + init_completion(&sspi->transfer_done); > + > + rx_irq = platform_get_irq(pdev, 0); > + if (rx_irq < 0) > + dev_err(&pdev->dev, "get rx_irq failed\n"); > + > + tx_irq = platform_get_irq(pdev, 1); > + if (tx_irq < 0) > + dev_err(&pdev->dev, "get tx_irq failed\n"); > + > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > + 0, "synquacer-spi-rx", sspi); > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > + 0, "synquacer-spi-tx", sspi); > + > + ret = clk_prepare_enable(sspi->clk); > + if (ret) > + goto put_spi; > + > + master->dev.of_node = np; > + master->auto_runtime_pm = true; > + master->bus_num = pdev->id; > + > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | > + SPI_TX_QUAD | SPI_RX_QUAD; > + master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) | > + SPI_BPW_MASK(16) | SPI_BPW_MASK(8); > + > + master->max_speed_hz = clk_get_rate(sspi->clk); > + master->min_speed_hz = master->max_speed_hz / 254; > + > + master->set_cs = synquacer_spi_set_cs; > + master->transfer_one = synquacer_spi_transfer_one; > + > + ret = synquacer_spi_enable(master); > + if (ret) > + goto fail_enable; > + > + pm_runtime_set_active(sspi->dev); > + pm_runtime_enable(sspi->dev); > + > + ret = devm_spi_register_master(sspi->dev, master); > + if (ret) > + goto disable_pm; > + > + return 0; > + > +disable_pm: > + pm_runtime_disable(sspi->dev); > +fail_enable: > + clk_disable_unprepare(sspi->clk); > +put_spi: > + spi_master_put(master); > + > + return ret; > +} > + if (!pm_runtime_suspended(dev)) This is not enough to check. > + if (!pm_runtime_suspended(dev)) { Ditto. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 16:38 ` Andy Shevchenko @ 2019-05-27 8:31 ` Masahisa Kojima 0 siblings, 0 replies; 12+ messages in thread From: Masahisa Kojima @ 2019-05-27 8:31 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-spi, devicetree, Mark Brown, Geert Uytterhoeven, Trent Piepho, Rob Herring, Mark Rutland, Ard Biesheuvel, Jassi Brar, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki Thank you for your comments. On Wed, 22 May 2019 at 01:38, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, May 21, 2019 at 3:00 PM Masahisa Kojima > <masahisa.kojima@linaro.org> wrote: > > > > This patch adds support for controller found on synquacer platforms. > > > + case 8: > > + { > > + u8 *buf = sspi->rx_buf; > > + > > + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > > + sspi->rx_buf = buf + len; > > + break; > > + } > > Slightly better style > case FOO: { > ... > } > > > + /* Full Duplex only on 1-bit wide bus */ > > + if (xfer->rx_buf && xfer->tx_buf && > > + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { > > + dev_err(sspi->dev, > > + "RX and TX bus widths must match for Full-Duplex!\n"); > > The message is not telling full truth. Not only match, but also be equal 1. > > > + return -EINVAL; > > + } > > > +static int synquacer_spi_transfer_one(struct spi_master *master, > > + struct spi_device *spi, > > + struct spi_transfer *xfer) > > +{ > > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > > + int ret; > > + int status = 0; > > + unsigned int words; > > + u8 bpw; > > + u32 val; > > + > > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > > + val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH; > > + val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH; > > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG); > > + > > + /* > > + * See if we can transfer 4-bytes as 1 word > > + * to maximize the FIFO buffer effficiency > > Typo here, and period is missed. > > > + */ > > > + case 8: > > + words = xfer->len; > > + break; > > + case 16: > > + words = xfer->len / 2; > > + break; > > + default: > > + words = xfer->len / 4; > > + break; > > Hmm... Shouldn't be rather "less then or equal" comparisons? As Mark's comment, I will explicitly list the values(8, 16, 24 and 32). > > + unsigned int retries = 0xfffff; > > Hmm... better to use decimal value. Instead of implementing retry with counter, I will implement wait function with checking time_before(jiffies, timeout). > > > + /* Disable module */ > > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL); > > > + while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) & > > + SYNQUACER_HSSPI_MCTRL_MES) && --retries) > > + cpu_relax(); > > + if (!retries) > > + return -EBUSY; > > And here something like > do { > } while (--retries) > > would look slightly better due to understanding that we do at least > one iteration. > > Also, can readx_poll_timeout be used here? > > > +static irqreturn_t sq_spi_tx_handler(int irq, void *priv) > > +{ > > + uint32_t val; > > + struct synquacer_spi *sspi = priv; > > + > > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF); > > + > > + if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) { > > + if (sspi->tx_words == 0) { > > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > > + complete(&sspi->transfer_done); > > > + return 0; > > irqreturn_t type, please. We have corresponding defines. > > > + } > > + write_fifo(sspi); > > + } > > + > > + return 0; > > Ditto. > > > +} > > > +static int synquacer_spi_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct spi_master *master; > > + struct synquacer_spi *sspi; > > + struct resource *res; > > + int ret; > > + int rx_irq, tx_irq; > > + > > + master = spi_alloc_master(&pdev->dev, sizeof(*sspi)); > > + if (!master) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, master); > > + > > + sspi = spi_master_get_devdata(master); > > + sspi->dev = &pdev->dev; > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + sspi->regs = devm_ioremap_resource(sspi->dev, res); > > devm_platform_ioremap_resource() > > > + if (IS_ERR(sspi->regs)) { > > + ret = PTR_ERR(sspi->regs); > > + goto put_spi; > > + } > > > + } else { > > + dev_err(&pdev->dev, "specified wrong clock source\n"); > > + ret = -EINVAL; > > + goto put_spi; > > + } > > Not an issue for ACPI. > > > + if (IS_ERR(sspi->clk)) { > > + dev_err(&pdev->dev, "clock not found\n"); > > + ret = PTR_ERR(sspi->clk); > > + goto put_spi; > > + } > > + > > + sspi->aces = of_property_read_bool(np, "socionext,set-aces"); > > + sspi->rtm = of_property_read_bool(np, "socionext,use-rtm"); > > + > > + master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT; > > + > > + init_completion(&sspi->transfer_done); > > + > > + rx_irq = platform_get_irq(pdev, 0); > > + if (rx_irq < 0) > > + dev_err(&pdev->dev, "get rx_irq failed\n"); > > + > > + tx_irq = platform_get_irq(pdev, 1); > > + if (tx_irq < 0) > > + dev_err(&pdev->dev, "get tx_irq failed\n"); > > + > > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > > + 0, "synquacer-spi-rx", sspi); > > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > > + 0, "synquacer-spi-tx", sspi); > > + > > + ret = clk_prepare_enable(sspi->clk); > > + if (ret) > > + goto put_spi; > > + > > + master->dev.of_node = np; > > + master->auto_runtime_pm = true; > > + master->bus_num = pdev->id; > > + > > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | > > + SPI_TX_QUAD | SPI_RX_QUAD; > > + master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) | > > + SPI_BPW_MASK(16) | SPI_BPW_MASK(8); > > + > > + master->max_speed_hz = clk_get_rate(sspi->clk); > > + master->min_speed_hz = master->max_speed_hz / 254; > > + > > + master->set_cs = synquacer_spi_set_cs; > > + master->transfer_one = synquacer_spi_transfer_one; > > + > > + ret = synquacer_spi_enable(master); > > + if (ret) > > + goto fail_enable; > > + > > + pm_runtime_set_active(sspi->dev); > > + pm_runtime_enable(sspi->dev); > > + > > + ret = devm_spi_register_master(sspi->dev, master); > > + if (ret) > > + goto disable_pm; > > + > > + return 0; > > + > > +disable_pm: > > + pm_runtime_disable(sspi->dev); > > +fail_enable: > > + clk_disable_unprepare(sspi->clk); > > +put_spi: > > + spi_master_put(master); > > + > > + return ret; > > +} > > > > + if (!pm_runtime_suspended(dev)) > > This is not enough to check. I checked other drivers, but I could find what is missing. Could you kindly share more details? > > > + if (!pm_runtime_suspended(dev)) { > > Ditto. > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima 2019-05-21 13:55 ` Ard Biesheuvel 2019-05-21 16:38 ` Andy Shevchenko @ 2019-05-21 18:16 ` Mark Brown 2019-05-22 8:27 ` Masahisa Kojima 2 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2019-05-21 18:16 UTC (permalink / raw) To: Masahisa Kojima Cc: linux-spi, devicetree, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, ard.biesheuvel, jaswinder.singh, masami.hiramatsu, okamoto.satoru, osaki.yoshitoyo [-- Attachment #1: Type: text/plain, Size: 3623 bytes --] On Tue, May 21, 2019 at 08:59:58PM +0900, Masahisa Kojima wrote: > + switch (sspi->bpw) { > + case 8: > + { > + u8 *buf = sspi->rx_buf; > + > + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } Please indent these properly. > + default: > + { > + u32 *buf = sspi->rx_buf; > + > + readsl(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > + sspi->rx_buf = buf + len; > + break; > + } It'd be better to explicitly list the values this works for and return an error otherwise. > + if (sspi->rx_words) { > + val = SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD | > + SYNQUACER_HSSPI_RXE_SLAVE_RELEASED; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + status = wait_for_completion_timeout(&sspi->transfer_done, > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + } > + > + if (xfer->tx_buf) { > + val = SYNQUACER_HSSPI_TXE_FIFO_EMPTY; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + status = wait_for_completion_timeout(&sspi->transfer_done, > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > + } I guess the TX will complete before the RX usually so I'd kind of expect the waits to be in the other order? > + if (status < 0) { > + dev_err(sspi->dev, "failed to transfer\n"); > + return status; > + } Printing the error code could be helpful for users. > +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(spi->master); > + u32 val; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK << > + SYNQUACER_HSSPI_DMPSEL_CS_SHIFT); > + val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT; > + > + if (enable) { > + val |= SYNQUACER_HSSPI_DMSTOP_STOP; > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > + > + if (sspi->rx_buf) { > + u32 buf[SYNQUACER_HSSPI_FIFO_DEPTH]; > + > + sspi->rx_buf = buf; > + sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH; > + read_fifo(sspi); > + } This is doing things with the FIFO, that's completely inappropriate for a set_cs() operation. The set_cs() operation should set the chip select and nothing else. > +static irqreturn_t sq_spi_rx_handler(int irq, void *priv) > +{ > + uint32_t val; > + struct synquacer_spi *sspi = priv; > + > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_RXF); > + if ((val & SYNQUACER_HSSPI_RXF_SLAVE_RELEASED) || > + (val & SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD)) > + read_fifo(sspi); > + > + if (sspi->rx_words == 0) { > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > + complete(&sspi->transfer_done); > + } > + > + return 0; > +} 0 is not a valid return from an interrupt handler, IRQ_HANDLED or IRQ_NONE. > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > + 0, "synquacer-spi-rx", sspi); > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > + 0, "synquacer-spi-tx", sspi); The code looked awfully like we depend on having interrupts? > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | > + SPI_TX_QUAD | SPI_RX_QUAD; I don't see any code in the driver that configures dual or quad mode support other than setting _PCC_SAFESYNC, I'm not clear how the driver supports these modes? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-21 18:16 ` Mark Brown @ 2019-05-22 8:27 ` Masahisa Kojima 2019-05-22 11:09 ` Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Masahisa Kojima @ 2019-05-22 8:27 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, devicetree, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, Ard Biesheuvel, Jassi Brar, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki Thank you very much for your comments. On Wed, 22 May 2019 at 03:16, Mark Brown <broonie@kernel.org> wrote: > > On Tue, May 21, 2019 at 08:59:58PM +0900, Masahisa Kojima wrote: > > > + switch (sspi->bpw) { > > + case 8: > > > + { > > + u8 *buf = sspi->rx_buf; > > + > > + readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > > + sspi->rx_buf = buf + len; > > + break; > > + } > > Please indent these properly. > > > + default: > > + { > > + u32 *buf = sspi->rx_buf; > > + > > + readsl(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len); > > + sspi->rx_buf = buf + len; > > + break; > > + } > > It'd be better to explicitly list the values this works for and return > an error otherwise. > > > + if (sspi->rx_words) { > > + val = SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD | > > + SYNQUACER_HSSPI_RXE_SLAVE_RELEASED; > > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > > + status = wait_for_completion_timeout(&sspi->transfer_done, > > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > > + } > > + > > + if (xfer->tx_buf) { > > + val = SYNQUACER_HSSPI_TXE_FIFO_EMPTY; > > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > > + status = wait_for_completion_timeout(&sspi->transfer_done, > > + msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC)); > > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE); > > + } > > I guess the TX will complete before the RX usually so I'd kind of expect > the waits to be in the other order? > > > + if (status < 0) { > > + dev_err(sspi->dev, "failed to transfer\n"); > > + return status; > > + } > > Printing the error code could be helpful for users. > > > +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable) > > +{ > > + struct synquacer_spi *sspi = spi_master_get_devdata(spi->master); > > + u32 val; > > + > > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > > + val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK << > > + SYNQUACER_HSSPI_DMPSEL_CS_SHIFT); > > + val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT; > > + > > + if (enable) { > > + val |= SYNQUACER_HSSPI_DMSTOP_STOP; > > + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); > > + > > + if (sspi->rx_buf) { > > + u32 buf[SYNQUACER_HSSPI_FIFO_DEPTH]; > > + > > + sspi->rx_buf = buf; > > + sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH; > > + read_fifo(sspi); > > + } > > This is doing things with the FIFO, that's completely inappropriate for > a set_cs() operation. The set_cs() operation should set the chip select > and nothing else. > > > +static irqreturn_t sq_spi_rx_handler(int irq, void *priv) > > +{ > > + uint32_t val; > > + struct synquacer_spi *sspi = priv; > > + > > + val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_RXF); > > + if ((val & SYNQUACER_HSSPI_RXF_SLAVE_RELEASED) || > > + (val & SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD)) > > + read_fifo(sspi); > > + > > + if (sspi->rx_words == 0) { > > + writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE); > > + complete(&sspi->transfer_done); > > + } > > + > > + return 0; > > +} > > 0 is not a valid return from an interrupt handler, IRQ_HANDLED or > IRQ_NONE. > > > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > > + 0, "synquacer-spi-rx", sspi); > > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > > + 0, "synquacer-spi-tx", sspi); > > The code looked awfully like we depend on having interrupts? I"m not sure I correctly understand what this comment means, should driver assume the case interrupt is not available? Do I need to support both interrupt and polling handling? > > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL | > > + SPI_TX_QUAD | SPI_RX_QUAD; > > I don't see any code in the driver that configures dual or quad mode > support other than setting _PCC_SAFESYNC, I'm not clear how the driver > supports these modes? Configuring single, dual and quad mode is depending on the spi_transfer member from upper driver. +static int synquacer_spi_config(struct spi_master *master, <snip> + if (xfer->tx_buf) { + bus_width = xfer->tx_nbits; + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_TX; + } else { + bus_width = xfer->rx_nbits; + transfer_mode = SYNQUACER_HSSPI_TRANSFER_MODE_RX; + } <snip> + val &= ~(3 << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); + val |= ((bus_width >> 1) << SYNQUACER_HSSPI_DMTRP_BUS_WIDTH_SHIFT); + writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-22 8:27 ` Masahisa Kojima @ 2019-05-22 11:09 ` Mark Brown 2019-05-23 4:35 ` Masahisa Kojima 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2019-05-22 11:09 UTC (permalink / raw) To: Masahisa Kojima Cc: linux-spi, devicetree, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, Ard Biesheuvel, Jassi Brar, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki [-- Attachment #1: Type: text/plain, Size: 833 bytes --] On Wed, May 22, 2019 at 05:27:23PM +0900, Masahisa Kojima wrote: > On Wed, 22 May 2019 at 03:16, Mark Brown <broonie@kernel.org> wrote: > > On Tue, May 21, 2019 at 08:59:58PM +0900, Masahisa Kojima wrote: > > > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > > > + 0, "synquacer-spi-rx", sspi); > > > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > > > + 0, "synquacer-spi-tx", sspi); > > The code looked awfully like we depend on having interrupts? > I"m not sure I correctly understand what this comment means, > should driver assume the case interrupt is not available? > Do I need to support both interrupt and polling handling? If the driver requires interrupts it should not just ignore errors when it requests interrupts. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform 2019-05-22 11:09 ` Mark Brown @ 2019-05-23 4:35 ` Masahisa Kojima 0 siblings, 0 replies; 12+ messages in thread From: Masahisa Kojima @ 2019-05-23 4:35 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, devicetree, geert, tpiepho, andy.shevchenko, robh+dt, mark.rutland, Ard Biesheuvel, Jassi Brar, Masami Hiramatsu, Satoru Okamoto, Yoshitoyo Osaki On Wed, 22 May 2019 at 20:09, Mark Brown <broonie@kernel.org> wrote: > > On Wed, May 22, 2019 at 05:27:23PM +0900, Masahisa Kojima wrote: > > On Wed, 22 May 2019 at 03:16, Mark Brown <broonie@kernel.org> wrote: > > > On Tue, May 21, 2019 at 08:59:58PM +0900, Masahisa Kojima wrote: > > > > > + ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler, > > > > + 0, "synquacer-spi-rx", sspi); > > > > + ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler, > > > > + 0, "synquacer-spi-tx", sspi); > > > > The code looked awfully like we depend on having interrupts? > > > I"m not sure I correctly understand what this comment means, > > should driver assume the case interrupt is not available? > > Do I need to support both interrupt and polling handling? > > If the driver requires interrupts it should not just ignore errors when > it requests interrupts. I'm sorry, I misunderstood. Yes, interrupt is required. I will handle errors properly. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-05-27 8:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-21 11:59 [PATCH v5 0/3] spi: support for Socionext Synquacer platform Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 1/3] MAINTAINERS: Add entry for Synquacer SPI driver Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 2/3] dt-bindings: spi: Add DT bindings for Synquacer Masahisa Kojima 2019-05-21 11:59 ` [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform Masahisa Kojima 2019-05-21 13:55 ` Ard Biesheuvel 2019-05-21 16:27 ` Andy Shevchenko 2019-05-21 16:38 ` Andy Shevchenko 2019-05-27 8:31 ` Masahisa Kojima 2019-05-21 18:16 ` Mark Brown 2019-05-22 8:27 ` Masahisa Kojima 2019-05-22 11:09 ` Mark Brown 2019-05-23 4:35 ` Masahisa Kojima
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.