This series adds support for SPI controller driver implemented on UniPhier SoCs.
Changes since v1:
- modify not to config SPI in master->setup method.
- fix internal bug of uniphier_spi_fill_tx_fifo().
- replace macro of BYTES_PER_WORD to inline function.
- use IRQ_NONE in irq handler.
- modify the handling of word size in uniphier_spi_send() and uniphier_spi_recv().
- modify help in Kconfig.
- other trivial change.
Keiji Hayashibara (1):
spi: add SPI controller driver for UniPhier SoC
Kunihiko Hayashi (1):
dt-bindings: spi: add DT bindings for UniPhier SPI controller
.../devicetree/bindings/spi/spi-uniphier.txt | 22 +
drivers/spi/Kconfig | 13 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-uniphier.c | 539 +++++++++++++++++++++
4 files changed, 575 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
create mode 100644 drivers/spi/spi-uniphier.c
--
2.7.4
Add SPI controller driver implemented in Socionext UniPhier SoCs.
UniPhier SoCs have two types SPI controllers; SCSSI supports a
single channel, and MCSSI supports multiple channels.
This driver supports SCSSI only.
This controller has 32bit TX/RX FIFO with depth of eight entry,
and supports the SPI master mode only.
This commit is implemented in PIO transfer mode, not DMA transfer.
Signed-off-by: Kunihiko Hayashi <[email protected]>
Signed-off-by: Keiji Hayashibara <[email protected]>
---
drivers/spi/Kconfig | 13 ++
drivers/spi/Makefile | 1 +
drivers/spi/spi-uniphier.c | 539 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 553 insertions(+)
create mode 100644 drivers/spi/spi-uniphier.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..671d078 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -688,6 +688,19 @@ config SPI_TXX9
help
SPI driver for Toshiba TXx9 MIPS SoCs
+config SPI_UNIPHIER
+ tristate "Socionext UniPhier SPI Controller"
+ depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+ help
+ This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
+
+ UniPhier SoCs have SCSSI and MCSSI SPI controllers.
+ Every UniPhier SoC has SCSSI which supports single channel.
+ Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
+ This driver supports SCSSI only.
+
+ If your SoC supports SCSSI, say Y here.
+
config SPI_XCOMM
tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..a90d559 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -101,6 +101,7 @@ spi-thunderx-objs := spi-cavium.o spi-cavium-thunderx.o
obj-$(CONFIG_SPI_THUNDERX) += spi-thunderx.o
obj-$(CONFIG_SPI_TOPCLIFF_PCH) += spi-topcliff-pch.o
obj-$(CONFIG_SPI_TXX9) += spi-txx9.o
+obj-$(CONFIG_SPI_UNIPHIER) += spi-uniphier.o
obj-$(CONFIG_SPI_XCOMM) += spi-xcomm.o
obj-$(CONFIG_SPI_XILINX) += spi-xilinx.o
obj-$(CONFIG_SPI_XLP) += spi-xlp.o
diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
new file mode 100644
index 0000000..2e80bb0
--- /dev/null
+++ b/drivers/spi/spi-uniphier.c
@@ -0,0 +1,539 @@
+// SPDX-License-Identifier: GPL-2.0
+// spi-uniphier.c - Socionext UniPhier SPI controller driver
+// Copyright 2012 Panasonic Corporation
+// Copyright 2016-2018 Socionext Inc.
+
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+#define SSI_TIMEOUT 2000 /* ms */
+#define SSI_MAX_CLK_DIVIDER 254
+#define SSI_MIN_CLK_DIVIDER 4
+
+struct uniphier_spi_priv {
+ void __iomem *base;
+ int irq;
+ struct clk *clk;
+ struct spi_master *master;
+ struct completion xfer_done;
+
+ int error;
+ unsigned int tx_bytes;
+ unsigned int rx_bytes;
+ const u8 *tx_buf;
+ u8 *rx_buf;
+
+ bool is_save_param;
+ u8 bits_per_word;
+ u16 mode;
+ u32 speed_hz;
+};
+
+#define SSI_CTL 0x0
+#define SSI_CTL_EN BIT(0)
+
+#define SSI_CKS 0x4
+#define SSI_CKS_CKRAT_MASK GENMASK(7, 0)
+#define SSI_CKS_CKPHS BIT(14)
+#define SSI_CKS_CKINIT BIT(13)
+#define SSI_CKS_CKDLY BIT(12)
+
+#define SSI_TXWDS 0x8
+#define SSI_TXWDS_WDLEN_MASK GENMASK(13, 8)
+#define SSI_TXWDS_TDTF_MASK GENMASK(7, 6)
+#define SSI_TXWDS_DTLEN_MASK GENMASK(5, 0)
+
+#define SSI_RXWDS 0xc
+#define SSI_RXWDS_DTLEN_MASK GENMASK(5, 0)
+
+#define SSI_FPS 0x10
+#define SSI_FPS_FSPOL BIT(15)
+#define SSI_FPS_FSTRT BIT(14)
+
+#define SSI_SR 0x14
+#define SSI_SR_RNE BIT(0)
+
+#define SSI_IE 0x18
+#define SSI_IE_RCIE BIT(3)
+#define SSI_IE_RORIE BIT(0)
+
+#define SSI_IS 0x1c
+#define SSI_IS_RXRS BIT(9)
+#define SSI_IS_RCID BIT(3)
+#define SSI_IS_RORID BIT(0)
+
+#define SSI_IC 0x1c
+#define SSI_IC_TCIC BIT(4)
+#define SSI_IC_RCIC BIT(3)
+#define SSI_IC_RORIC BIT(0)
+
+#define SSI_FC 0x20
+#define SSI_FC_TXFFL BIT(12)
+#define SSI_FC_TXFTH_MASK GENMASK(11, 8)
+#define SSI_FC_RXFFL BIT(4)
+#define SSI_FC_RXFTH_MASK GENMASK(3, 0)
+
+#define SSI_TXDR 0x24
+#define SSI_RXDR 0x24
+
+#define SSI_FIFO_DEPTH 8U
+
+static inline unsigned int bytes_per_word(unsigned int bits)
+{
+ return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
+}
+
+static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val;
+
+ val = readl(priv->base + SSI_IE);
+ val |= mask;
+ writel(val, priv->base + SSI_IE);
+}
+
+static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val;
+
+ val = readl(priv->base + SSI_IE);
+ val &= ~mask;
+ writel(val, priv->base + SSI_IE);
+}
+
+static void uniphier_spi_set_mode(struct spi_device *spi)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val1, val2;
+
+ /*
+ * clock setting
+ * CKPHS capture timing. 0:rising edge, 1:falling edge
+ * CKINIT clock initial level. 0:low, 1:high
+ * CKDLY clock delay. 0:no delay, 1:delay depending on FSTRT
+ * (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
+ *
+ * frame setting
+ * FSPOL frame signal porarity. 0: low, 1: high
+ * FSTRT start frame timing
+ * 0: rising edge of clock, 1: falling edge of clock
+ */
+ switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
+ case SPI_MODE_0:
+ /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
+ val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
+ val2 = 0;
+ break;
+ case SPI_MODE_1:
+ /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
+ val1 = 0;
+ val2 = SSI_FPS_FSTRT;
+ break;
+ case SPI_MODE_2:
+ /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
+ val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
+ val2 = SSI_FPS_FSTRT;
+ break;
+ case SPI_MODE_3:
+ /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
+ val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
+ val2 = 0;
+ break;
+ }
+
+ if (!(spi->mode & SPI_CS_HIGH))
+ val2 |= SSI_FPS_FSPOL;
+
+ writel(val1, priv->base + SSI_CKS);
+ writel(val2, priv->base + SSI_FPS);
+
+ val1 = 0;
+ if (spi->mode & SPI_LSB_FIRST)
+ val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
+ writel(val1, priv->base + SSI_TXWDS);
+ writel(val1, priv->base + SSI_RXWDS);
+}
+
+static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val;
+
+ val = readl(priv->base + SSI_TXWDS);
+ val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
+ val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
+ val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
+ writel(val, priv->base + SSI_TXWDS);
+
+ val = readl(priv->base + SSI_RXWDS);
+ val &= ~SSI_RXWDS_DTLEN_MASK;
+ val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
+ writel(val, priv->base + SSI_RXWDS);
+}
+
+static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val, ckrat;
+
+ /*
+ * the supported rates are even numbers from 4 to 254. (4,6,8...254)
+ * round up as we look for equal or less speed
+ */
+ ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
+ ckrat = roundup(ckrat, 2);
+
+ /* check if requested speed is too small */
+ if (ckrat > SSI_MAX_CLK_DIVIDER)
+ return -EINVAL;
+
+ if (ckrat < SSI_MIN_CLK_DIVIDER)
+ ckrat = SSI_MIN_CLK_DIVIDER;
+
+ val = readl(priv->base + SSI_CKS);
+ val &= ~SSI_CKS_CKRAT_MASK;
+ val |= ckrat & SSI_CKS_CKRAT_MASK;
+ writel(val, priv->base + SSI_CKS);
+
+ return 0;
+}
+
+static int uniphier_spi_setup_transfer(struct spi_device *spi,
+ struct spi_transfer *t)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val;
+ int ret;
+
+ priv->error = 0;
+ priv->tx_buf = t->tx_buf;
+ priv->rx_buf = t->rx_buf;
+ priv->tx_bytes = priv->rx_bytes = t->len;
+
+ if (!priv->is_save_param || priv->mode != spi->mode) {
+ uniphier_spi_set_mode(spi);
+ priv->mode = spi->mode;
+ }
+
+ if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
+ uniphier_spi_set_transfer_size(spi, t->bits_per_word);
+ priv->bits_per_word = t->bits_per_word;
+ }
+
+ if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
+ ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
+ if (ret)
+ return ret;
+ priv->speed_hz = t->speed_hz;
+ }
+
+ if (!priv->is_save_param)
+ priv->is_save_param = true;
+
+ /* reset FIFOs */
+ val = SSI_FC_TXFFL | SSI_FC_RXFFL;
+ writel(val, priv->base + SSI_FC);
+
+ return 0;
+}
+
+static void uniphier_spi_send(struct uniphier_spi_priv *priv)
+{
+ int wsize;
+ u32 val = 0;
+
+ wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
+ priv->tx_bytes -= wsize;
+
+ if (priv->tx_buf) {
+ switch (wsize) {
+ case 1:
+ val = *priv->tx_buf;
+ break;
+ case 2:
+ val = get_unaligned_le16(priv->tx_buf);
+ break;
+ case 4:
+ val = get_unaligned_le32(priv->tx_buf);
+ break;
+ }
+
+ priv->tx_buf += wsize;
+ }
+
+ writel(val, priv->base + SSI_TXDR);
+}
+
+static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
+{
+ int rsize;
+ u32 val;
+
+ rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
+ priv->rx_bytes -= rsize;
+
+ val = readl(priv->base + SSI_RXDR);
+
+ if (priv->rx_buf) {
+ switch (rsize) {
+ case 1:
+ *priv->rx_buf = (u8)val;
+ break;
+ case 2:
+ put_unaligned_le16(val, priv->rx_buf);
+ break;
+ case 4:
+ put_unaligned_le32(val, priv->rx_buf);
+ break;
+ }
+
+ priv->rx_buf += rsize;
+ }
+}
+
+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
+{
+ unsigned int tx_count;
+ u32 val;
+
+ tx_count = DIV_ROUND_UP(priv->tx_bytes,
+ bytes_per_word(priv->bits_per_word));
+ tx_count = min(tx_count, SSI_FIFO_DEPTH);
+
+ /* set fifo threthold */
+ val = readl(priv->base + SSI_FC);
+ val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
+ val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
+ val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
+ writel(val, priv->base + SSI_FC);
+
+ while (tx_count--)
+ uniphier_spi_send(priv);
+}
+
+static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+ u32 val;
+
+ val = readl(priv->base + SSI_FPS);
+
+ if (enable)
+ val |= SSI_FPS_FSPOL;
+ else
+ val &= ~SSI_FPS_FSPOL;
+
+ writel(val, priv->base + SSI_FPS);
+}
+
+static int uniphier_spi_transfer_one(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *t)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+ int status;
+
+ status = uniphier_spi_setup_transfer(spi, t);
+ if (status < 0)
+ return status;
+
+ reinit_completion(&priv->xfer_done);
+
+ uniphier_spi_fill_tx_fifo(priv);
+
+ uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+ status = wait_for_completion_timeout(&priv->xfer_done,
+ msecs_to_jiffies(SSI_TIMEOUT));
+
+ uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+ if (status < 0)
+ return status;
+
+ return priv->error;
+}
+
+static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+ writel(SSI_CTL_EN, priv->base + SSI_CTL);
+
+ return 0;
+}
+
+static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
+{
+ struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+ writel(0, priv->base + SSI_CTL);
+
+ return 0;
+}
+
+static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
+{
+ struct uniphier_spi_priv *priv = dev_id;
+ u32 val, stat;
+
+ stat = readl(priv->base + SSI_IS);
+ val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
+ writel(val, priv->base + SSI_IC);
+
+ /* rx fifo overrun */
+ if (stat & SSI_IS_RORID) {
+ priv->error = -EIO;
+ goto done;
+ }
+
+ /* rx complete */
+ if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
+ while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
+ (priv->rx_bytes - priv->tx_bytes) > 0)
+ uniphier_spi_recv(priv);
+
+ if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
+ (priv->rx_bytes != priv->tx_bytes)) {
+ priv->error = -EIO;
+ goto done;
+ } else if (priv->rx_bytes == 0)
+ goto done;
+
+ /* next tx transfer */
+ uniphier_spi_fill_tx_fifo(priv);
+
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+
+done:
+ complete(&priv->xfer_done);
+ return IRQ_HANDLED;
+}
+
+static int uniphier_spi_probe(struct platform_device *pdev)
+{
+ struct uniphier_spi_priv *priv;
+ struct spi_master *master;
+ struct resource *res;
+ unsigned long clksrc;
+ int ret;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(*priv));
+ if (!master)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, master);
+
+ priv = spi_master_get_devdata(master);
+ priv->master = master;
+ priv->is_save_param = false;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->base)) {
+ ret = PTR_ERR(priv->base);
+ goto out_master_put;
+ }
+
+ priv->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(&pdev->dev, "failed to get clock\n");
+ ret = PTR_ERR(priv->clk);
+ goto out_master_put;
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ goto out_master_put;
+
+ priv->irq = platform_get_irq(pdev, 0);
+ if (priv->irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ\n");
+ ret = -ENXIO;
+ goto out_disable_clk;
+ }
+
+ ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
+ 0, "uniphier-spi", priv);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request IRQ\n");
+ goto out_disable_clk;
+ }
+
+ init_completion(&priv->xfer_done);
+
+ clksrc = clk_get_rate(priv->clk);
+
+ master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
+ master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
+ master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+ master->dev.of_node = pdev->dev.of_node;
+ master->bus_num = pdev->id;
+ master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
+
+ master->set_cs = uniphier_spi_set_cs;
+ master->transfer_one = uniphier_spi_transfer_one;
+ master->prepare_transfer_hardware
+ = uniphier_spi_prepare_transfer_hardware;
+ master->unprepare_transfer_hardware
+ = uniphier_spi_unprepare_transfer_hardware;
+ master->num_chipselect = 1;
+
+ ret = devm_spi_register_master(&pdev->dev, master);
+ if (ret)
+ goto out_disable_clk;
+
+ return ret;
+
+out_disable_clk:
+ clk_disable_unprepare(priv->clk);
+
+out_master_put:
+ spi_master_put(master);
+ return ret;
+}
+
+static int uniphier_spi_remove(struct platform_device *pdev)
+{
+ struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static const struct of_device_id uniphier_spi_match[] = {
+ { .compatible = "socionext,uniphier-scssi", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uniphier_spi_match);
+
+static struct platform_driver uniphier_spi_driver = {
+ .probe = uniphier_spi_probe,
+ .remove = uniphier_spi_remove,
+ .driver = {
+ .name = "uniphier-spi",
+ .of_match_table = uniphier_spi_match,
+ },
+};
+module_platform_driver(uniphier_spi_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <[email protected]>");
+MODULE_AUTHOR("Keiji Hayashibara <[email protected]>");
+MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
From: Kunihiko Hayashi <[email protected]>
Add DT bindings for SPI controller implemented in UniPhier SoCs.
Signed-off-by: Kunihiko Hayashi <[email protected]>
Signed-off-by: Keiji Hayashibara <[email protected]>
---
.../devicetree/bindings/spi/spi-uniphier.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
diff --git a/Documentation/devicetree/bindings/spi/spi-uniphier.txt b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
new file mode 100644
index 0000000..504a4ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
@@ -0,0 +1,22 @@
+Socionext UniPhier SPI controller driver
+
+UniPhier SoCs have SCSSI which supports SPI single channel.
+
+Required properties:
+ - compatible: should be "socionext,uniphier-scssi"
+ - reg: address and length of the spi master registers
+ - #address-cells: must be <1>, see spi-bus.txt
+ - #size-cells: must be <0>, see spi-bus.txt
+ - clocks: A phandle to the clock for the device.
+ - resets: A phandle to the reset control for the device.
+
+Example:
+
+spi0: spi@54006000 {
+ compatible = "socionext,uniphier-scssi";
+ reg = <0x54006000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&peri_clk 11>;
+ resets = <&peri_rst 11>;
+};
--
2.7.4
On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara
<[email protected]> wrote:
> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>
> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> single channel, and MCSSI supports multiple channels.
> This driver supports SCSSI only.
>
> This controller has 32bit TX/RX FIFO with depth of eight entry,
> and supports the SPI master mode only.
>
> This commit is implemented in PIO transfer mode, not DMA transfer.
Few style realted comments.
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
Slightly better to keep them in order and put asm/* at the last.
> +#define SSI_TIMEOUT 2000 /* ms */
SSI_TIMEOUT_MS ?
> +#define SSI_CTL 0x0
Slightly better to keep same width for the addresses, like 0x00 here.
> +#define SSI_CKS 0x4
> +#define SSI_TXWDS 0x8
> +#define SSI_RXWDS 0xc
Ditto.
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val, ckrat;
> +
> + /*
> + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> + * round up as we look for equal or less speed
> + */
> + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> + ckrat = roundup(ckrat, 2);
ckrat += ckrat & 1;
?
> + /* check if requested speed is too small */
> + if (ckrat > SSI_MAX_CLK_DIVIDER)
> + return -EINVAL;
So, does this critical?
> +
> + if (ckrat < SSI_MIN_CLK_DIVIDER)
> + ckrat = SSI_MIN_CLK_DIVIDER;
clamp_val() / max() ?
> + val = readl(priv->base + SSI_CKS);
> + val &= ~SSI_CKS_CKRAT_MASK;
> + val |= ckrat & SSI_CKS_CKRAT_MASK;
> + writel(val, priv->base + SSI_CKS);
> +
> + return 0;
> +}
> + priv->irq = platform_get_irq(pdev, 0);
> + if (priv->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ\n");
> + ret = -ENXIO;
What's wrong with
ret = priv->irq;
?
> + goto out_disable_clk;
> + }
> +static const struct of_device_id uniphier_spi_match[] = {
> + { .compatible = "socionext,uniphier-scssi", },
> + { /* sentinel */ },
Slightly better without comma.
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
--
With Best Regards,
Andy Shevchenko
Hi.
2018-07-26 16:09 GMT+09:00 Keiji Hayashibara <[email protected]>:
> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>
> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> single channel, and MCSSI supports multiple channels.
> This driver supports SCSSI only.
>
> This controller has 32bit TX/RX FIFO with depth of eight entry,
> and supports the SPI master mode only.
>
> This commit is implemented in PIO transfer mode, not DMA transfer.
>
> Signed-off-by: Kunihiko Hayashi <[email protected]>
> Signed-off-by: Keiji Hayashibara <[email protected]>
> ---
> drivers/spi/Kconfig | 13 ++
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-uniphier.c | 539 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 553 insertions(+)
> create mode 100644 drivers/spi/spi-uniphier.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ad5d68e..671d078 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -688,6 +688,19 @@ config SPI_TXX9
> help
> SPI driver for Toshiba TXx9 MIPS SoCs
>
> +config SPI_UNIPHIER
> + tristate "Socionext UniPhier SPI Controller"
> + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> + help
> + This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
> +
> + UniPhier SoCs have SCSSI and MCSSI SPI controllers.
> + Every UniPhier SoC has SCSSI which supports single channel.
> + Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
> + This driver supports SCSSI only.
> +
> + If your SoC supports SCSSI, say Y here.
> +
> config SPI_XCOMM
> tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
> depends on I2C
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index cb1f437..a90d559 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -101,6 +101,7 @@ spi-thunderx-objs := spi-cavium.o spi-cavium-thunderx.o
> obj-$(CONFIG_SPI_THUNDERX) += spi-thunderx.o
> obj-$(CONFIG_SPI_TOPCLIFF_PCH) += spi-topcliff-pch.o
> obj-$(CONFIG_SPI_TXX9) += spi-txx9.o
> +obj-$(CONFIG_SPI_UNIPHIER) += spi-uniphier.o
> obj-$(CONFIG_SPI_XCOMM) += spi-xcomm.o
> obj-$(CONFIG_SPI_XILINX) += spi-xilinx.o
> obj-$(CONFIG_SPI_XLP) += spi-xlp.o
> diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
> new file mode 100644
> index 0000000..2e80bb0
> --- /dev/null
> +++ b/drivers/spi/spi-uniphier.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// spi-uniphier.c - Socionext UniPhier SPI controller driver
> +// Copyright 2012 Panasonic Corporation
> +// Copyright 2016-2018 Socionext Inc.
> +
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
> +
> +#define SSI_TIMEOUT 2000 /* ms */
> +#define SSI_MAX_CLK_DIVIDER 254
> +#define SSI_MIN_CLK_DIVIDER 4
> +
> +struct uniphier_spi_priv {
> + void __iomem *base;
> + int irq;
irq is unnecessary because it is only used in probe().
Use a stack variable.
> + struct clk *clk;
> + struct spi_master *master;
> + struct completion xfer_done;
> +
> + int error;
> + unsigned int tx_bytes;
> + unsigned int rx_bytes;
> + const u8 *tx_buf;
> + u8 *rx_buf;
> +
> + bool is_save_param;
> + u8 bits_per_word;
> + u16 mode;
> + u32 speed_hz;
> +};
> +
> +#define SSI_CTL 0x0
> +#define SSI_CTL_EN BIT(0)
> +
> +#define SSI_CKS 0x4
> +#define SSI_CKS_CKRAT_MASK GENMASK(7, 0)
> +#define SSI_CKS_CKPHS BIT(14)
> +#define SSI_CKS_CKINIT BIT(13)
> +#define SSI_CKS_CKDLY BIT(12)
> +
> +#define SSI_TXWDS 0x8
> +#define SSI_TXWDS_WDLEN_MASK GENMASK(13, 8)
> +#define SSI_TXWDS_TDTF_MASK GENMASK(7, 6)
> +#define SSI_TXWDS_DTLEN_MASK GENMASK(5, 0)
> +
> +#define SSI_RXWDS 0xc
> +#define SSI_RXWDS_DTLEN_MASK GENMASK(5, 0)
> +
> +#define SSI_FPS 0x10
> +#define SSI_FPS_FSPOL BIT(15)
> +#define SSI_FPS_FSTRT BIT(14)
> +
> +#define SSI_SR 0x14
> +#define SSI_SR_RNE BIT(0)
> +
> +#define SSI_IE 0x18
> +#define SSI_IE_RCIE BIT(3)
> +#define SSI_IE_RORIE BIT(0)
> +
> +#define SSI_IS 0x1c
> +#define SSI_IS_RXRS BIT(9)
> +#define SSI_IS_RCID BIT(3)
> +#define SSI_IS_RORID BIT(0)
> +
> +#define SSI_IC 0x1c
> +#define SSI_IC_TCIC BIT(4)
> +#define SSI_IC_RCIC BIT(3)
> +#define SSI_IC_RORIC BIT(0)
> +
> +#define SSI_FC 0x20
> +#define SSI_FC_TXFFL BIT(12)
> +#define SSI_FC_TXFTH_MASK GENMASK(11, 8)
> +#define SSI_FC_RXFFL BIT(4)
> +#define SSI_FC_RXFTH_MASK GENMASK(3, 0)
> +
> +#define SSI_TXDR 0x24
> +#define SSI_RXDR 0x24
> +
> +#define SSI_FIFO_DEPTH 8U
> +
> +static inline unsigned int bytes_per_word(unsigned int bits)
> +{
> + return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
> +}
> +
> +static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val;
> +
> + val = readl(priv->base + SSI_IE);
> + val |= mask;
> + writel(val, priv->base + SSI_IE);
> +}
> +
> +static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val;
> +
> + val = readl(priv->base + SSI_IE);
> + val &= ~mask;
> + writel(val, priv->base + SSI_IE);
> +}
> +
> +static void uniphier_spi_set_mode(struct spi_device *spi)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val1, val2;
> +
> + /*
> + * clock setting
> + * CKPHS capture timing. 0:rising edge, 1:falling edge
> + * CKINIT clock initial level. 0:low, 1:high
> + * CKDLY clock delay. 0:no delay, 1:delay depending on FSTRT
> + * (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
> + *
> + * frame setting
> + * FSPOL frame signal porarity. 0: low, 1: high
> + * FSTRT start frame timing
> + * 0: rising edge of clock, 1: falling edge of clock
> + */
> + switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
> + case SPI_MODE_0:
> + /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
> + val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
> + val2 = 0;
> + break;
> + case SPI_MODE_1:
> + /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
> + val1 = 0;
> + val2 = SSI_FPS_FSTRT;
> + break;
> + case SPI_MODE_2:
> + /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
> + val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
> + val2 = SSI_FPS_FSTRT;
> + break;
> + case SPI_MODE_3:
> + /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
> + val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
> + val2 = 0;
> + break;
> + }
> +
> + if (!(spi->mode & SPI_CS_HIGH))
> + val2 |= SSI_FPS_FSPOL;
> +
> + writel(val1, priv->base + SSI_CKS);
> + writel(val2, priv->base + SSI_FPS);
> +
> + val1 = 0;
> + if (spi->mode & SPI_LSB_FIRST)
> + val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> + writel(val1, priv->base + SSI_TXWDS);
> + writel(val1, priv->base + SSI_RXWDS);
> +}
> +
> +static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val;
> +
> + val = readl(priv->base + SSI_TXWDS);
> + val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> + val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> + val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> + writel(val, priv->base + SSI_TXWDS);
> +
> + val = readl(priv->base + SSI_RXWDS);
> + val &= ~SSI_RXWDS_DTLEN_MASK;
> + val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> + writel(val, priv->base + SSI_RXWDS);
> +}
> +
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val, ckrat;
> +
> + /*
> + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> + * round up as we look for equal or less speed
> + */
> + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> + ckrat = roundup(ckrat, 2);
> +
> + /* check if requested speed is too small */
> + if (ckrat > SSI_MAX_CLK_DIVIDER)
> + return -EINVAL;
> +
> + if (ckrat < SSI_MIN_CLK_DIVIDER)
> + ckrat = SSI_MIN_CLK_DIVIDER;
> +
> + val = readl(priv->base + SSI_CKS);
> + val &= ~SSI_CKS_CKRAT_MASK;
> + val |= ckrat & SSI_CKS_CKRAT_MASK;
> + writel(val, priv->base + SSI_CKS);
> +
> + return 0;
> +}
> +
> +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> + struct spi_transfer *t)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val;
> + int ret;
> +
> + priv->error = 0;
> + priv->tx_buf = t->tx_buf;
> + priv->rx_buf = t->rx_buf;
> + priv->tx_bytes = priv->rx_bytes = t->len;
> +
> + if (!priv->is_save_param || priv->mode != spi->mode) {
> + uniphier_spi_set_mode(spi);
> + priv->mode = spi->mode;
> + }
> +
> + if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
> + uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> + priv->bits_per_word = t->bits_per_word;
> + }
> +
> + if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
> + ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> + if (ret)
> + return ret;
> + priv->speed_hz = t->speed_hz;
> + }
> +
> + if (!priv->is_save_param)
> + priv->is_save_param = true;
> +
> + /* reset FIFOs */
> + val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> + writel(val, priv->base + SSI_FC);
> +
> + return 0;
> +}
> +
> +static void uniphier_spi_send(struct uniphier_spi_priv *priv)
> +{
> + int wsize;
> + u32 val = 0;
> +
> + wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> + priv->tx_bytes -= wsize;
> +
> + if (priv->tx_buf) {
> + switch (wsize) {
> + case 1:
> + val = *priv->tx_buf;
> + break;
> + case 2:
> + val = get_unaligned_le16(priv->tx_buf);
> + break;
> + case 4:
> + val = get_unaligned_le32(priv->tx_buf);
> + break;
> + }
> +
> + priv->tx_buf += wsize;
> + }
> +
> + writel(val, priv->base + SSI_TXDR);
> +}
> +
> +static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
> +{
> + int rsize;
> + u32 val;
> +
> + rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
> + priv->rx_bytes -= rsize;
> +
> + val = readl(priv->base + SSI_RXDR);
> +
> + if (priv->rx_buf) {
> + switch (rsize) {
> + case 1:
> + *priv->rx_buf = (u8)val;
Is this cast necessary?
> + break;
> + case 2:
> + put_unaligned_le16(val, priv->rx_buf);
> + break;
> + case 4:
> + put_unaligned_le32(val, priv->rx_buf);
> + break;
> + }
> +
> + priv->rx_buf += rsize;
> + }
> +}
> +
> +static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> +{
> + unsigned int tx_count;
> + u32 val;
> +
> + tx_count = DIV_ROUND_UP(priv->tx_bytes,
> + bytes_per_word(priv->bits_per_word));
> + tx_count = min(tx_count, SSI_FIFO_DEPTH);
> +
> + /* set fifo threthold */
Typo. threshold
> + val = readl(priv->base + SSI_FC);
> + val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> + val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> + val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> + writel(val, priv->base + SSI_FC);
> +
> + while (tx_count--)
> + uniphier_spi_send(priv);
> +}
> +
> +static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> + u32 val;
> +
> + val = readl(priv->base + SSI_FPS);
> +
> + if (enable)
> + val |= SSI_FPS_FSPOL;
> + else
> + val &= ~SSI_FPS_FSPOL;
> +
> + writel(val, priv->base + SSI_FPS);
> +}
> +
> +static int uniphier_spi_transfer_one(struct spi_master *master,
> + struct spi_device *spi,
> + struct spi_transfer *t)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> + int status;
> +
> + status = uniphier_spi_setup_transfer(spi, t);
> + if (status < 0)
> + return status;
> +
> + reinit_completion(&priv->xfer_done);
> +
> + uniphier_spi_fill_tx_fifo(priv);
> +
> + uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> +
> + status = wait_for_completion_timeout(&priv->xfer_done,
> + msecs_to_jiffies(SSI_TIMEOUT));
> +
> + uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> +
> + if (status < 0)
> + return status;
> +
> + return priv->error;
> +}
> +
> +static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> +
> + writel(SSI_CTL_EN, priv->base + SSI_CTL);
> +
> + return 0;
> +}
> +
> +static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
> +{
> + struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> +
> + writel(0, priv->base + SSI_CTL);
> +
> + return 0;
> +}
> +
> +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
> +{
> + struct uniphier_spi_priv *priv = dev_id;
> + u32 val, stat;
> +
> + stat = readl(priv->base + SSI_IS);
> + val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> + writel(val, priv->base + SSI_IC);
> +
> + /* rx fifo overrun */
> + if (stat & SSI_IS_RORID) {
> + priv->error = -EIO;
> + goto done;
> + }
> +
> + /* rx complete */
> + if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> + while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
> + (priv->rx_bytes - priv->tx_bytes) > 0)
> + uniphier_spi_recv(priv);
> +
> + if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
> + (priv->rx_bytes != priv->tx_bytes)) {
> + priv->error = -EIO;
> + goto done;
> + } else if (priv->rx_bytes == 0)
> + goto done;
> +
> + /* next tx transfer */
> + uniphier_spi_fill_tx_fifo(priv);
> +
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +
> +done:
> + complete(&priv->xfer_done);
> + return IRQ_HANDLED;
> +}
> +
> +static int uniphier_spi_probe(struct platform_device *pdev)
> +{
> + struct uniphier_spi_priv *priv;
> + struct spi_master *master;
> + struct resource *res;
> + unsigned long clksrc;
> + int ret;
> +
> + master = spi_alloc_master(&pdev->dev, sizeof(*priv));
> + if (!master)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, master);
> +
> + priv = spi_master_get_devdata(master);
> + priv->master = master;
> + priv->is_save_param = false;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->base)) {
> + ret = PTR_ERR(priv->base);
> + goto out_master_put;
> + }
> +
> + priv->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + dev_err(&pdev->dev, "failed to get clock\n");
> + ret = PTR_ERR(priv->clk);
> + goto out_master_put;
> + }
> +
> + ret = clk_prepare_enable(priv->clk);
> + if (ret)
> + goto out_master_put;
> +
> + priv->irq = platform_get_irq(pdev, 0);
> + if (priv->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ\n");
> + ret = -ENXIO;
> + goto out_disable_clk;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
> + 0, "uniphier-spi", priv);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request IRQ\n");
> + goto out_disable_clk;
> + }
> +
> + init_completion(&priv->xfer_done);
> +
> + clksrc = clk_get_rate(priv->clk);
How about 'clk_rate' ?
> + master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> + master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
> + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> + master->dev.of_node = pdev->dev.of_node;
> + master->bus_num = pdev->id;
> + master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> +
> + master->set_cs = uniphier_spi_set_cs;
> + master->transfer_one = uniphier_spi_transfer_one;
> + master->prepare_transfer_hardware
> + = uniphier_spi_prepare_transfer_hardware;
> + master->unprepare_transfer_hardware
> + = uniphier_spi_unprepare_transfer_hardware;
> + master->num_chipselect = 1;
> +
> + ret = devm_spi_register_master(&pdev->dev, master);
> + if (ret)
> + goto out_disable_clk;
> +
> + return ret;
I think
return 0;
will be clearer.
> +
> +out_disable_clk:
> + clk_disable_unprepare(priv->clk);
> +
> +out_master_put:
> + spi_master_put(master);
> + return ret;
> +}
> +
> +static int uniphier_spi_remove(struct platform_device *pdev)
> +{
> + struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(priv->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id uniphier_spi_match[] = {
> + { .compatible = "socionext,uniphier-scssi", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> +
> +static struct platform_driver uniphier_spi_driver = {
> + .probe = uniphier_spi_probe,
> + .remove = uniphier_spi_remove,
> + .driver = {
> + .name = "uniphier-spi",
> + .of_match_table = uniphier_spi_match,
> + },
> +};
> +module_platform_driver(uniphier_spi_driver);
> +
> +MODULE_AUTHOR("Kunihiko Hayashi <[email protected]>");
> +MODULE_AUTHOR("Keiji Hayashibara <[email protected]>");
> +MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
Hello Andy,
Thank you for your check!
> From: Andy Shevchenko [mailto:[email protected]]
> Sent: Thursday, July 26, 2018 5:46 PM
> To: Hayashibara, Keiji/林原 啓二 <[email protected]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>
> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[email protected]> wrote:
> > Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >
> > UniPhier SoCs have two types SPI controllers; SCSSI supports a single
> > channel, and MCSSI supports multiple channels.
> > This driver supports SCSSI only.
> >
> > This controller has 32bit TX/RX FIFO with depth of eight entry, and
> > supports the SPI master mode only.
> >
> > This commit is implemented in PIO transfer mode, not DMA transfer.
>
> Few style realted comments.
>
> > +#include <asm/unaligned.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spi/spi.h>
>
> Slightly better to keep them in order and put asm/* at the last.
I see. I will modify this.
> > +#define SSI_TIMEOUT 2000 /* ms */
>
> SSI_TIMEOUT_MS ?
>
> > +#define SSI_CTL 0x0
>
> Slightly better to keep same width for the addresses, like 0x00 here.
>
> > +#define SSI_CKS 0x4
>
> > +#define SSI_TXWDS 0x8
>
> > +#define SSI_RXWDS 0xc
>
> Ditto.
I will modify about above.
>
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val, ckrat;
> > +
> > + /*
> > + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > + * round up as we look for equal or less speed
> > + */
> > + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
>
> > + ckrat = roundup(ckrat, 2);
>
> ckrat += ckrat & 1;
>
> ?
It's simple. I will modify.
> > + /* check if requested speed is too small */
> > + if (ckrat > SSI_MAX_CLK_DIVIDER)
>
> > + return -EINVAL;
>
> So, does this critical?
If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
I don't change it to high frequency, and it is daringly an error.
On the other hand, when changing to low frequency, I will change it automatically.
> > +
> > + if (ckrat < SSI_MIN_CLK_DIVIDER)
> > + ckrat = SSI_MIN_CLK_DIVIDER;
>
> clamp_val() / max() ?
I will modify it to use max().
>
> > + val = readl(priv->base + SSI_CKS);
> > + val &= ~SSI_CKS_CKRAT_MASK;
> > + val |= ckrat & SSI_CKS_CKRAT_MASK;
> > + writel(val, priv->base + SSI_CKS);
> > +
> > + return 0;
> > +}
>
> > + priv->irq = platform_get_irq(pdev, 0);
> > + if (priv->irq < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ\n");
>
> > + ret = -ENXIO;
>
> What's wrong with
>
> ret = priv->irq;
>
> ?
I will modify it.
> > + goto out_disable_clk;
> > + }
>
> > +static const struct of_device_id uniphier_spi_match[] = {
> > + { .compatible = "socionext,uniphier-scssi", },
>
> > + { /* sentinel */ },
>
> Slightly better without comma.
OK. I will modify this.
-----------------
Best Regards,
Keiji Hayashibara
> > +};
> > +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
>
> --
> With Best Regards,
> Andy Shevchenko
Hello Yamada-san,
> From: Masahiro Yamada [mailto:[email protected]]
> Sent: Thursday, July 26, 2018 6:13 PM
> To: Hayashibara, Keiji/林原 啓二 <[email protected]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>
> Hi.
>
>
> 2018-07-26 16:09 GMT+09:00 Keiji Hayashibara <[email protected]>:
> > Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >
> > UniPhier SoCs have two types SPI controllers; SCSSI supports a single
> > channel, and MCSSI supports multiple channels.
> > This driver supports SCSSI only.
> >
> > This controller has 32bit TX/RX FIFO with depth of eight entry, and
> > supports the SPI master mode only.
> >
> > This commit is implemented in PIO transfer mode, not DMA transfer.
> >
> > Signed-off-by: Kunihiko Hayashi <[email protected]>
> > Signed-off-by: Keiji Hayashibara <[email protected]>
> > ---
> > drivers/spi/Kconfig | 13 ++
> > drivers/spi/Makefile | 1 +
> > drivers/spi/spi-uniphier.c | 539
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 553 insertions(+)
> > create mode 100644 drivers/spi/spi-uniphier.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > ad5d68e..671d078 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -688,6 +688,19 @@ config SPI_TXX9
> > help
> > SPI driver for Toshiba TXx9 MIPS SoCs
> >
> > +config SPI_UNIPHIER
> > + tristate "Socionext UniPhier SPI Controller"
> > + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > + help
> > + This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
> > +
> > + UniPhier SoCs have SCSSI and MCSSI SPI controllers.
> > + Every UniPhier SoC has SCSSI which supports single channel.
> > + Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
> > + This driver supports SCSSI only.
> > +
> > + If your SoC supports SCSSI, say Y here.
> > +
> > config SPI_XCOMM
> > tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
> > depends on I2C
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > cb1f437..a90d559 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -101,6 +101,7 @@ spi-thunderx-objs := spi-cavium.o spi-cavium-thunderx.o
> > obj-$(CONFIG_SPI_THUNDERX) += spi-thunderx.o
> > obj-$(CONFIG_SPI_TOPCLIFF_PCH) += spi-topcliff-pch.o
> > obj-$(CONFIG_SPI_TXX9) += spi-txx9.o
> > +obj-$(CONFIG_SPI_UNIPHIER) += spi-uniphier.o
> > obj-$(CONFIG_SPI_XCOMM) += spi-xcomm.o
> > obj-$(CONFIG_SPI_XILINX) += spi-xilinx.o
> > obj-$(CONFIG_SPI_XLP) += spi-xlp.o
> > diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
> > new file mode 100644 index 0000000..2e80bb0
> > --- /dev/null
> > +++ b/drivers/spi/spi-uniphier.c
> > @@ -0,0 +1,539 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// spi-uniphier.c - Socionext UniPhier SPI controller driver
> > +// Copyright 2012 Panasonic Corporation
> > +// Copyright 2016-2018 Socionext Inc.
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define SSI_TIMEOUT 2000 /* ms */
> > +#define SSI_MAX_CLK_DIVIDER 254
> > +#define SSI_MIN_CLK_DIVIDER 4
> > +
> > +struct uniphier_spi_priv {
> > + void __iomem *base;
> > + int irq;
>
>
> irq is unnecessary because it is only used in probe().
>
> Use a stack variable.
I see. I will modify it.
>
>
> > + struct clk *clk;
> > + struct spi_master *master;
> > + struct completion xfer_done;
> > +
> > + int error;
> > + unsigned int tx_bytes;
> > + unsigned int rx_bytes;
> > + const u8 *tx_buf;
> > + u8 *rx_buf;
> > +
> > + bool is_save_param;
> > + u8 bits_per_word;
> > + u16 mode;
> > + u32 speed_hz;
> > +};
> > +
> > +#define SSI_CTL 0x0
> > +#define SSI_CTL_EN BIT(0)
> > +
> > +#define SSI_CKS 0x4
> > +#define SSI_CKS_CKRAT_MASK GENMASK(7, 0)
> > +#define SSI_CKS_CKPHS BIT(14)
> > +#define SSI_CKS_CKINIT BIT(13)
> > +#define SSI_CKS_CKDLY BIT(12)
> > +
> > +#define SSI_TXWDS 0x8
> > +#define SSI_TXWDS_WDLEN_MASK GENMASK(13, 8)
> > +#define SSI_TXWDS_TDTF_MASK GENMASK(7, 6)
> > +#define SSI_TXWDS_DTLEN_MASK GENMASK(5, 0)
> > +
> > +#define SSI_RXWDS 0xc
> > +#define SSI_RXWDS_DTLEN_MASK GENMASK(5, 0)
> > +
> > +#define SSI_FPS 0x10
> > +#define SSI_FPS_FSPOL BIT(15)
> > +#define SSI_FPS_FSTRT BIT(14)
> > +
> > +#define SSI_SR 0x14
> > +#define SSI_SR_RNE BIT(0)
> > +
> > +#define SSI_IE 0x18
> > +#define SSI_IE_RCIE BIT(3)
> > +#define SSI_IE_RORIE BIT(0)
> > +
> > +#define SSI_IS 0x1c
> > +#define SSI_IS_RXRS BIT(9)
> > +#define SSI_IS_RCID BIT(3)
> > +#define SSI_IS_RORID BIT(0)
> > +
> > +#define SSI_IC 0x1c
> > +#define SSI_IC_TCIC BIT(4)
> > +#define SSI_IC_RCIC BIT(3)
> > +#define SSI_IC_RORIC BIT(0)
> > +
> > +#define SSI_FC 0x20
> > +#define SSI_FC_TXFFL BIT(12)
> > +#define SSI_FC_TXFTH_MASK GENMASK(11, 8)
> > +#define SSI_FC_RXFFL BIT(4)
> > +#define SSI_FC_RXFTH_MASK GENMASK(3, 0)
> > +
> > +#define SSI_TXDR 0x24
> > +#define SSI_RXDR 0x24
> > +
> > +#define SSI_FIFO_DEPTH 8U
> > +
> > +static inline unsigned int bytes_per_word(unsigned int bits) {
> > + return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); }
> > +
> > +static inline void uniphier_spi_irq_enable(struct spi_device *spi,
> > +u32 mask) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val;
> > +
> > + val = readl(priv->base + SSI_IE);
> > + val |= mask;
> > + writel(val, priv->base + SSI_IE); }
> > +
> > +static inline void uniphier_spi_irq_disable(struct spi_device *spi,
> > +u32 mask) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val;
> > +
> > + val = readl(priv->base + SSI_IE);
> > + val &= ~mask;
> > + writel(val, priv->base + SSI_IE); }
> > +
> > +static void uniphier_spi_set_mode(struct spi_device *spi) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val1, val2;
> > +
> > + /*
> > + * clock setting
> > + * CKPHS capture timing. 0:rising edge, 1:falling edge
> > + * CKINIT clock initial level. 0:low, 1:high
> > + * CKDLY clock delay. 0:no delay, 1:delay depending on FSTRT
> > + * (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
> > + *
> > + * frame setting
> > + * FSPOL frame signal porarity. 0: low, 1: high
> > + * FSTRT start frame timing
> > + * 0: rising edge of clock, 1: falling edge of clock
> > + */
> > + switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
> > + case SPI_MODE_0:
> > + /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
> > + val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
> > + val2 = 0;
> > + break;
> > + case SPI_MODE_1:
> > + /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
> > + val1 = 0;
> > + val2 = SSI_FPS_FSTRT;
> > + break;
> > + case SPI_MODE_2:
> > + /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
> > + val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
> > + val2 = SSI_FPS_FSTRT;
> > + break;
> > + case SPI_MODE_3:
> > + /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
> > + val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
> > + val2 = 0;
> > + break;
> > + }
> > +
> > + if (!(spi->mode & SPI_CS_HIGH))
> > + val2 |= SSI_FPS_FSPOL;
> > +
> > + writel(val1, priv->base + SSI_CKS);
> > + writel(val2, priv->base + SSI_FPS);
> > +
> > + val1 = 0;
> > + if (spi->mode & SPI_LSB_FIRST)
> > + val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > + writel(val1, priv->base + SSI_TXWDS);
> > + writel(val1, priv->base + SSI_RXWDS); }
> > +
> > +static void uniphier_spi_set_transfer_size(struct spi_device *spi,
> > +int size) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val;
> > +
> > + val = readl(priv->base + SSI_TXWDS);
> > + val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> > + val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> > + val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> > + writel(val, priv->base + SSI_TXWDS);
> > +
> > + val = readl(priv->base + SSI_RXWDS);
> > + val &= ~SSI_RXWDS_DTLEN_MASK;
> > + val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > + writel(val, priv->base + SSI_RXWDS); }
> > +
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val, ckrat;
> > +
> > + /*
> > + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > + * round up as we look for equal or less speed
> > + */
> > + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > + ckrat = roundup(ckrat, 2);
> > +
> > + /* check if requested speed is too small */
> > + if (ckrat > SSI_MAX_CLK_DIVIDER)
> > + return -EINVAL;
> > +
> > + if (ckrat < SSI_MIN_CLK_DIVIDER)
> > + ckrat = SSI_MIN_CLK_DIVIDER;
> > +
> > + val = readl(priv->base + SSI_CKS);
> > + val &= ~SSI_CKS_CKRAT_MASK;
> > + val |= ckrat & SSI_CKS_CKRAT_MASK;
> > + writel(val, priv->base + SSI_CKS);
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> > + struct spi_transfer *t) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val;
> > + int ret;
> > +
> > + priv->error = 0;
> > + priv->tx_buf = t->tx_buf;
> > + priv->rx_buf = t->rx_buf;
> > + priv->tx_bytes = priv->rx_bytes = t->len;
> > +
> > + if (!priv->is_save_param || priv->mode != spi->mode) {
> > + uniphier_spi_set_mode(spi);
> > + priv->mode = spi->mode;
> > + }
> > +
> > + if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
> > + uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> > + priv->bits_per_word = t->bits_per_word;
> > + }
> > +
> > + if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
> > + ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> > + if (ret)
> > + return ret;
> > + priv->speed_hz = t->speed_hz;
> > + }
> > +
> > + if (!priv->is_save_param)
> > + priv->is_save_param = true;
> > +
> > + /* reset FIFOs */
> > + val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > + writel(val, priv->base + SSI_FC);
> > +
> > + return 0;
> > +}
> > +
> > +static void uniphier_spi_send(struct uniphier_spi_priv *priv) {
> > + int wsize;
> > + u32 val = 0;
> > +
> > + wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> > + priv->tx_bytes -= wsize;
> > +
> > + if (priv->tx_buf) {
> > + switch (wsize) {
> > + case 1:
> > + val = *priv->tx_buf;
> > + break;
> > + case 2:
> > + val = get_unaligned_le16(priv->tx_buf);
> > + break;
> > + case 4:
> > + val = get_unaligned_le32(priv->tx_buf);
> > + break;
> > + }
> > +
> > + priv->tx_buf += wsize;
> > + }
> > +
> > + writel(val, priv->base + SSI_TXDR); }
> > +
> > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) {
> > + int rsize;
> > + u32 val;
> > +
> > + rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
> > + priv->rx_bytes -= rsize;
> > +
> > + val = readl(priv->base + SSI_RXDR);
> > +
> > + if (priv->rx_buf) {
> > + switch (rsize) {
> > + case 1:
> > + *priv->rx_buf = (u8)val;
>
> Is this cast necessary?
I added it explicitly, but I will modify it.
>
> > + break;
> > + case 2:
> > + put_unaligned_le16(val, priv->rx_buf);
> > + break;
> > + case 4:
> > + put_unaligned_le32(val, priv->rx_buf);
> > + break;
> > + }
> > +
> > + priv->rx_buf += rsize;
> > + }
> > +}
> > +
> > +static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> > +{
> > + unsigned int tx_count;
> > + u32 val;
> > +
> > + tx_count = DIV_ROUND_UP(priv->tx_bytes,
> > + bytes_per_word(priv->bits_per_word));
> > + tx_count = min(tx_count, SSI_FIFO_DEPTH);
> > +
> > + /* set fifo threthold */
>
>
> Typo. threshold
I will modify it.
>
>
> > + val = readl(priv->base + SSI_FC);
> > + val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > + val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> > + val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> > + writel(val, priv->base + SSI_FC);
> > +
> > + while (tx_count--)
> > + uniphier_spi_send(priv); }
> > +
> > +static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
> > +{
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > + u32 val;
> > +
> > + val = readl(priv->base + SSI_FPS);
> > +
> > + if (enable)
> > + val |= SSI_FPS_FSPOL;
> > + else
> > + val &= ~SSI_FPS_FSPOL;
> > +
> > + writel(val, priv->base + SSI_FPS); }
> > +
> > +static int uniphier_spi_transfer_one(struct spi_master *master,
> > + struct spi_device *spi,
> > + struct spi_transfer *t) {
> > + struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> > + int status;
> > +
> > + status = uniphier_spi_setup_transfer(spi, t);
> > + if (status < 0)
> > + return status;
> > +
> > + reinit_completion(&priv->xfer_done);
> > +
> > + uniphier_spi_fill_tx_fifo(priv);
> > +
> > + uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> > +
> > + status = wait_for_completion_timeout(&priv->xfer_done,
> > +
> > + msecs_to_jiffies(SSI_TIMEOUT));
> > +
> > + uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> > +
> > + if (status < 0)
> > + return status;
> > +
> > + return priv->error;
> > +}
> > +
> > +static int uniphier_spi_prepare_transfer_hardware(struct spi_master
> > +*master) {
> > + struct uniphier_spi_priv *priv =
> > +spi_master_get_devdata(master);
> > +
> > + writel(SSI_CTL_EN, priv->base + SSI_CTL);
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_unprepare_transfer_hardware(struct spi_master
> > +*master) {
> > + struct uniphier_spi_priv *priv =
> > +spi_master_get_devdata(master);
> > +
> > + writel(0, priv->base + SSI_CTL);
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id) {
> > + struct uniphier_spi_priv *priv = dev_id;
> > + u32 val, stat;
> > +
> > + stat = readl(priv->base + SSI_IS);
> > + val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> > + writel(val, priv->base + SSI_IC);
> > +
> > + /* rx fifo overrun */
> > + if (stat & SSI_IS_RORID) {
> > + priv->error = -EIO;
> > + goto done;
> > + }
> > +
> > + /* rx complete */
> > + if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> > + while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
> > + (priv->rx_bytes - priv->tx_bytes) > 0)
> > + uniphier_spi_recv(priv);
> > +
> > + if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
> > + (priv->rx_bytes != priv->tx_bytes)) {
> > + priv->error = -EIO;
> > + goto done;
> > + } else if (priv->rx_bytes == 0)
> > + goto done;
> > +
> > + /* next tx transfer */
> > + uniphier_spi_fill_tx_fifo(priv);
> > +
> > + return IRQ_HANDLED;
> > + }
> > +
> > + return IRQ_NONE;
> > +
> > +done:
> > + complete(&priv->xfer_done);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int uniphier_spi_probe(struct platform_device *pdev) {
> > + struct uniphier_spi_priv *priv;
> > + struct spi_master *master;
> > + struct resource *res;
> > + unsigned long clksrc;
> > + int ret;
> > +
> > + master = spi_alloc_master(&pdev->dev, sizeof(*priv));
> > + if (!master)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, master);
> > +
> > + priv = spi_master_get_devdata(master);
> > + priv->master = master;
> > + priv->is_save_param = false;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + priv->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv->base)) {
> > + ret = PTR_ERR(priv->base);
> > + goto out_master_put;
> > + }
> > +
> > + priv->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(priv->clk)) {
> > + dev_err(&pdev->dev, "failed to get clock\n");
> > + ret = PTR_ERR(priv->clk);
> > + goto out_master_put;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + goto out_master_put;
> > +
> > + priv->irq = platform_get_irq(pdev, 0);
> > + if (priv->irq < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ\n");
> > + ret = -ENXIO;
> > + goto out_disable_clk;
> > + }
> > +
> > + ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
> > + 0, "uniphier-spi", priv);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to request IRQ\n");
> > + goto out_disable_clk;
> > + }
> > +
> > + init_completion(&priv->xfer_done);
> > +
> > + clksrc = clk_get_rate(priv->clk);
>
>
> How about 'clk_rate' ?
OK. I will replace clksrc to clk_rate.
>
>
> > + master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> > + master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
> > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> > + master->dev.of_node = pdev->dev.of_node;
> > + master->bus_num = pdev->id;
> > + master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> > +
> > + master->set_cs = uniphier_spi_set_cs;
> > + master->transfer_one = uniphier_spi_transfer_one;
> > + master->prepare_transfer_hardware
> > + = uniphier_spi_prepare_transfer_hardware;
> > + master->unprepare_transfer_hardware
> > + = uniphier_spi_unprepare_transfer_hardware;
> > + master->num_chipselect = 1;
> > +
> > + ret = devm_spi_register_master(&pdev->dev, master);
> > + if (ret)
> > + goto out_disable_clk;
> > +
> > + return ret;
>
>
> I think
>
> return 0;
>
> will be clearer.
I think so. I will modify it.
Thank you.
-----------------
Best Regards,
Keiji Hayashibara
On 07/26/2018 12:38 PM, Keiji Hayashibara wrote:
> Hello Andy,
>
> Thank you for your check!
>
>
>> From: Andy Shevchenko [mailto:[email protected]]
>> Sent: Thursday, July 26, 2018 5:46 PM
>> To: Hayashibara, Keiji/林原 啓二 <[email protected]>
>> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>>
>> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[email protected]> wrote:
>>> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>>>
>>> UniPhier SoCs have two types SPI controllers; SCSSI supports a single
>>> channel, and MCSSI supports multiple channels.
>>> This driver supports SCSSI only.
>>>
>>> This controller has 32bit TX/RX FIFO with depth of eight entry, and
>>> supports the SPI master mode only.
>>>
>>> This commit is implemented in PIO transfer mode, not DMA transfer.
>>
>> Few style realted comments.
>>
>>> +#include <asm/unaligned.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/spi/spi.h>
>>
>> Slightly better to keep them in order and put asm/* at the last.
>
> I see. I will modify this.
>
>
>>> +#define SSI_TIMEOUT 2000 /* ms */
>>
>> SSI_TIMEOUT_MS ?
>>
>>> +#define SSI_CTL 0x0
>>
>> Slightly better to keep same width for the addresses, like 0x00 here.
>>
>>> +#define SSI_CKS 0x4
>>
>>> +#define SSI_TXWDS 0x8
>>
>>> +#define SSI_RXWDS 0xc
>>
>> Ditto.
>
> I will modify about above.
>
>>
>>> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
>>> +int speed) {
>>> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
>>> + u32 val, ckrat;
>>> +
>>> + /*
>>> + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
>>> + * round up as we look for equal or less speed
>>> + */
>>> + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
>>
>>> + ckrat = roundup(ckrat, 2);
>>
>> ckrat += ckrat & 1;
>>
>> ?
>
> It's simple. I will modify.
>
>
>>> + /* check if requested speed is too small */
>>> + if (ckrat > SSI_MAX_CLK_DIVIDER)
>>
>>> + return -EINVAL;
>>
>> So, does this critical?
>
> If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> I don't change it to high frequency, and it is daringly an error.
> On the other hand, when changing to low frequency, I will change it automatically.
>
>>> +
>>> + if (ckrat < SSI_MIN_CLK_DIVIDER)
>>> + ckrat = SSI_MIN_CLK_DIVIDER;
In fact you don't need this checks. You already set in probe function this.
master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
The SPI core will check if transfer speed is higher than controller
speed and if is, will set the transfer speed to master->max_speed_hz. In
case of master->min_speed_hz, if transfer speed is lower than
master->min_speed_hz __spi_validate(drivers/spi/spi.c) will return -EINVAL.
>>
>> clamp_val() / max() ?
>
> I will modify it to use max().
>
>>
>>> + val = readl(priv->base + SSI_CKS);
>>> + val &= ~SSI_CKS_CKRAT_MASK;
>>> + val |= ckrat & SSI_CKS_CKRAT_MASK;
>>> + writel(val, priv->base + SSI_CKS);
>>> +
>>> + return 0;
>>> +}
>>
>>> + priv->irq = platform_get_irq(pdev, 0);
>>> + if (priv->irq < 0) {
>>> + dev_err(&pdev->dev, "failed to get IRQ\n");
>>
>>> + ret = -ENXIO;
>>
>> What's wrong with
>>
>> ret = priv->irq;
>>
>> ?
>
> I will modify it.
>
>>> + goto out_disable_clk;
>>> + }
>>
>>> +static const struct of_device_id uniphier_spi_match[] = {
>>> + { .compatible = "socionext,uniphier-scssi", },
>>
>>> + { /* sentinel */ },
>>
>> Slightly better without comma.
>
> OK. I will modify this.
>
> -----------------
> Best Regards,
> Keiji Hayashibara
>
>
>
>>> +};
>>> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, Jul 26, 2018 at 12:38 PM, Keiji Hayashibara
<[email protected]> wrote:
>> > + /* check if requested speed is too small */
>> > + if (ckrat > SSI_MAX_CLK_DIVIDER)
>>
>> > + return -EINVAL;
>>
>> So, does this critical?
>
> If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> I don't change it to high frequency, and it is daringly an error.
> On the other hand, when changing to low frequency, I will change it automatically.
No. My point is, if somehow user asks for that condition to be
happened you bail out, while when using clamp_val() you may continue
to work at maximum limit, though issue warning to user, for example.
>> > +
>> > + if (ckrat < SSI_MIN_CLK_DIVIDER)
>> > + ckrat = SSI_MIN_CLK_DIVIDER;
>>
>> clamp_val() / max() ?
>
> I will modify it to use max().
See above.
--
With Best Regards,
Andy Shevchenko
On Thu, 2018-07-26 at 11:46 +0300, Andy Shevchenko wrote:
> > +
> > + /*
> > + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > + * round up as we look for equal or less speed
> > + */
> > + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > + ckrat = roundup(ckrat, 2);
>
> ckrat += ckrat & 1;
Either way, the compiler produces the same code:
add r0, r0, #1
bic r0, r0, #1
I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".
Might as well use the macro so it's clear. There is also round_up(x,y),
which I believe is intended to be used only with power of 2 values of
y.
Hello Radu,
> From: Radu Pirea [mailto:[email protected]]
> Sent: Thursday, July 26, 2018 7:58 PM
> To: Hayashibara, Keiji/林原 啓二 <[email protected]>; 'Andy Shevchenko'
> <[email protected]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>
>
>
> On 07/26/2018 12:38 PM, Keiji Hayashibara wrote:
> > Hello Andy,
> >
> > Thank you for your check!
> >
> >
> >> From: Andy Shevchenko [mailto:[email protected]]
> >> Sent: Thursday, July 26, 2018 5:46 PM
> >> To: Hayashibara, Keiji/林原 啓二 <[email protected]>
> >> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for
> >> UniPhier SoC
> >>
> >> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[email protected]> wrote:
> >>> Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >>>
> >>> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> >>> single channel, and MCSSI supports multiple channels.
> >>> This driver supports SCSSI only.
> >>>
> >>> This controller has 32bit TX/RX FIFO with depth of eight entry, and
> >>> supports the SPI master mode only.
> >>>
> >>> This commit is implemented in PIO transfer mode, not DMA transfer.
> >>
> >> Few style realted comments.
> >>
> >>> +#include <asm/unaligned.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/bitfield.h>
> >>> +#include <linux/bitops.h>
> >>> +#include <linux/clk.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/io.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/spi/spi.h>
> >>
> >> Slightly better to keep them in order and put asm/* at the last.
> >
> > I see. I will modify this.
> >
> >
> >>> +#define SSI_TIMEOUT 2000 /* ms */
> >>
> >> SSI_TIMEOUT_MS ?
> >>
> >>> +#define SSI_CTL 0x0
> >>
> >> Slightly better to keep same width for the addresses, like 0x00 here.
> >>
> >>> +#define SSI_CKS 0x4
> >>
> >>> +#define SSI_TXWDS 0x8
> >>
> >>> +#define SSI_RXWDS 0xc
> >>
> >> Ditto.
> >
> > I will modify about above.
> >
> >>
> >>> +static int uniphier_spi_set_baudrate(struct spi_device *spi,
> >>> +unsigned int speed) {
> >>> + struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> >>> + u32 val, ckrat;
> >>> +
> >>> + /*
> >>> + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> >>> + * round up as we look for equal or less speed
> >>> + */
> >>> + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> >>
> >>> + ckrat = roundup(ckrat, 2);
> >>
> >> ckrat += ckrat & 1;
> >>
> >> ?
> >
> > It's simple. I will modify.
> >
> >
> >>> + /* check if requested speed is too small */
> >>> + if (ckrat > SSI_MAX_CLK_DIVIDER)
> >>
> >>> + return -EINVAL;
> >>
> >> So, does this critical?
> >
> > If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> > I don't change it to high frequency, and it is daringly an error.
> > On the other hand, when changing to low frequency, I will change it automatically.
> >
> >>> +
> >>> + if (ckrat < SSI_MIN_CLK_DIVIDER)
> >>> + ckrat = SSI_MIN_CLK_DIVIDER;
>
> In fact you don't need this checks. You already set in probe function this.
> master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
>
> The SPI core will check if transfer speed is higher than controller speed and if is, will set the transfer speed
> to master->max_speed_hz. In case of master->min_speed_hz, if transfer speed is lower than
> master->min_speed_hz __spi_validate(drivers/spi/spi.c) will return -EINVAL.
I see.
I confirmed __spi_validate() and understood that this check code is unnecessary.
I will remove this check code.
Thank you.
> >>
> >> clamp_val() / max() ?
> >
> > I will modify it to use max().
> >
> >>
> >>> + val = readl(priv->base + SSI_CKS);
> >>> + val &= ~SSI_CKS_CKRAT_MASK;
> >>> + val |= ckrat & SSI_CKS_CKRAT_MASK;
> >>> + writel(val, priv->base + SSI_CKS);
> >>> +
> >>> + return 0;
> >>> +}
> >>
> >>> + priv->irq = platform_get_irq(pdev, 0);
> >>> + if (priv->irq < 0) {
> >>> + dev_err(&pdev->dev, "failed to get IRQ\n");
> >>
> >>> + ret = -ENXIO;
> >>
> >> What's wrong with
> >>
> >> ret = priv->irq;
> >>
> >> ?
> >
> > I will modify it.
> >
> >>> + goto out_disable_clk;
> >>> + }
> >>
> >>> +static const struct of_device_id uniphier_spi_match[] = {
> >>> + { .compatible = "socionext,uniphier-scssi", },
> >>
> >>> + { /* sentinel */ },
> >>
> >> Slightly better without comma.
> >
> > OK. I will modify this.
> >
> > -----------------
> > Best Regards,
> > Keiji Hayashibara
> >
> >
> >
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> >>
> >> --
> >> With Best Regards,
> >> Andy Shevchenko
> >
-----------------
Best Regards,
Keiji Hayashibara
Hello Andy,
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Andy
> Shevchenko
> Sent: Thursday, July 26, 2018 10:44 PM
> To: Hayashibara, Keiji/林原 啓二 <[email protected]>
> Cc: Mark Brown <[email protected]>; Rob Herring <[email protected]>; Mark Rutland <[email protected]>;
> Yamada, Masahiro/山田 真弘 <[email protected]>; linux-spi <[email protected]>; linux-arm
> Mailing List <[email protected]>; devicetree <[email protected]>; Masami Hiramatsu
> <[email protected]>; Jassi Brar <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; Hayashi, Kunihiko/林 邦彦 <[email protected]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>
> On Thu, Jul 26, 2018 at 12:38 PM, Keiji Hayashibara <[email protected]> wrote:
>
> >> > + /* check if requested speed is too small */
> >> > + if (ckrat > SSI_MAX_CLK_DIVIDER)
> >>
> >> > + return -EINVAL;
> >>
> >> So, does this critical?
> >
> > If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> > I don't change it to high frequency, and it is daringly an error.
> > On the other hand, when changing to low frequency, I will change it automatically.
>
> No. My point is, if somehow user asks for that condition to be happened you bail out, while when using clamp_val()
> you may continue to work at maximum limit, though issue warning to user, for example.
Thank you. I understood what you pointed out.
However, I found this code unnecessary as pointed out by Radu,
so I will delete it.
> >> > +
> >> > + if (ckrat < SSI_MIN_CLK_DIVIDER)
> >> > + ckrat = SSI_MIN_CLK_DIVIDER;
> >>
> >> clamp_val() / max() ?
> >
> > I will modify it to use max().
>
> See above.
This is also the same as above.
> --
> With Best Regards,
> Andy Shevchenko
-----------------
Best Regards,
Keiji Hayashibara
Hello Trent and Andy,
> From: Trent Piepho [mailto:[email protected]]
> Sent: Friday, July 27, 2018 2:02 AM
> To: [email protected]; Hayashibara, Keiji/林原 啓二 <[email protected]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>
> On Thu, 2018-07-26 at 11:46 +0300, Andy Shevchenko wrote:
> > > +
> > > + /*
> > > + * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > > + * round up as we look for equal or less speed
> > > + */
> > > + ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > > + ckrat = roundup(ckrat, 2);
> >
> > ckrat += ckrat & 1;
>
> Either way, the compiler produces the same code:
>
> add r0, r0, #1
> bic r0, r0, #1
>
> I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".
>
> Might as well use the macro so it's clear. There is also round_up(x,y), which I believe is intended to be used
> only with power of 2 values of y.
I confirmed round_up() macro.
This macro is optimized to power of 2 argument,
and it matches to this case and clear.
I will replace to round_up() macro.
Thank you.
-----------------
Best Regards,
Keiji Hayashibara
On Mon, Jul 30, 2018 at 8:30 AM, Keiji Hayashibara
<[email protected]> wrote:
> Hello Trent and Andy,
>> > > + ckrat = roundup(ckrat, 2);
>> >
>> > ckrat += ckrat & 1;
>>
>> Either way, the compiler produces the same code:
>>
>> add r0, r0, #1
>> bic r0, r0, #1
>>
>> I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".
>>
>> Might as well use the macro so it's clear. There is also round_up(x,y), which I believe is intended to be used
>> only with power of 2 values of y.
>
> I confirmed round_up() macro.
> This macro is optimized to power of 2 argument,
> and it matches to this case and clear.
>
> I will replace to round_up() macro.
Good!
--
With Best Regards,
Andy Shevchenko
On Thu, Jul 26, 2018 at 04:09:02PM +0900, Keiji Hayashibara wrote:
> From: Kunihiko Hayashi <[email protected]>
>
> Add DT bindings for SPI controller implemented in UniPhier SoCs.
>
> Signed-off-by: Kunihiko Hayashi <[email protected]>
> Signed-off-by: Keiji Hayashibara <[email protected]>
> ---
> .../devicetree/bindings/spi/spi-uniphier.txt | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
Please add acks/reviewed-bys when posting new versions.
Rob
Hello Rob,
I see.
I will add "Acked-by" and "Reviewed-by" tag by Rob in next version.
Thank you.
-----------------
Best Regards,
Keiji Hayashibara
> -----Original Message-----
> From: Rob Herring [mailto:[email protected]]
> Sent: Tuesday, July 31, 2018 6:47 AM
> To: Hayashibara, Keiji/$BNS86(B $B7<Fs(B <[email protected]>
> Subject: Re: [PATCH v2 1/2] dt-bindings: spi: add DT bindings for UniPhier SPI controller
>
> On Thu, Jul 26, 2018 at 04:09:02PM +0900, Keiji Hayashibara wrote:
> > From: Kunihiko Hayashi <[email protected]>
> >
> > Add DT bindings for SPI controller implemented in UniPhier SoCs.
> >
> > Signed-off-by: Kunihiko Hayashi <[email protected]>
> > Signed-off-by: Keiji Hayashibara <[email protected]>
> > ---
> > .../devicetree/bindings/spi/spi-uniphier.txt | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
>
> Please add acks/reviewed-bys when posting new versions.
>
> Rob