Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH] spi: add driver for the SiFive SPI controller
@ 2018-11-12 14:27 kernel
  2018-11-12 14:27 ` Emil Renner Berthing
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: kernel @ 2018-11-12 14:27 UTC (permalink / raw)
  To: linux-riscv

From: Palmer Dabbelt <palmer@sifive.com>

Add driver for the SiFive SPI controller
on the HiFive Unleashed board.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
---
 .../devicetree/bindings/spi/spi-sifive.txt    |  29 ++
 drivers/spi/Kconfig                           |   6 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-sifive.c                      | 442 ++++++++++++++++++
 4 files changed, 478 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sifive.txt
 create mode 100644 drivers/spi/spi-sifive.c

Hi all,

I know the discussions about the sifive devicetree compatible
strings haven't come to a conclusion, so I'm sending this as
an RFC to get some feedback on the rest of the code.

Compared to the original[1] I've done the following:

- Update register names and bit fields to the ones in the
  FU540-C000 documentation.

- Change the optional devicetree property from "sifive,buffer-size"
  to "sifive,fifo-depth". The string "fifo-depth" seems to have more
  hits in the existing devicetree bindings and is IMHO a little more
  descriptive.

- Change the optional devicetree property from "sifive,bits-per-word"
  to "sifive,max-bits-per-word". For a long time I wondered why the
  SPI word size was a property on the controller and not the device.
  This change makes the meaning a little more clear.

- Be honest about only supporting 8bit SPI words in the driver.
  Without SPI_LSB_FIRST and bits_per_word < 8 we need some code
  to shift the bits in each byte which is not yet there.

- Program the csdef, csid and sckmode registers from prepare_message
  rather than transfer_one. This way we only do it once pr. message
  rather than every transfer.

- Drop the irq field from driver data. With devm_request_irq we
  don't need to remember it after requesting the irq.

- Drop hz = t->speed_hz ? t->speed_hz : device->max_speed_hz;
  The SPI framework handles this for us, so we can just always
  use t->speed_hz.

- Fix most checkpatch warnings.

[1]: https://github.com/riscv/riscv-linux/commit/801805694740ad0895e21d10b8f124d138beefbb

/Emil


diff --git a/Documentation/devicetree/bindings/spi/spi-sifive.txt b/Documentation/devicetree/bindings/spi/spi-sifive.txt
new file mode 100644
index 000000000000..96339afcc74f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sifive.txt
@@ -0,0 +1,29 @@
+SiFive SPI controller Device Tree Bindings
+------------------------------------------
+
+Required properties:
+- compatible		: Should be "sifive,spi0"
+- reg			: Physical base address and size of SPI registers map
+			  A second (optional) range can indicate memory mapped flash
+- interrupts		: Must contain one entry
+- interrupt-parent	: Must be core interrupt controller
+- clocks		: Must reference the frequency given to the controller
+- #address-cells	: Must be '1', indicating which CS to use
+- #size-cells		: Must be '0'
+
+Optional properties:
+- sifive,fifo-depth		: Depth of hardware queues; defaults to 8
+- sifive,max-bits-per-word	: Maximum bits per word; defaults to 8
+
+Example:
+	spi: spi at 10040000 {
+		compatible = "sifive,spi0";
+		reg = <0x0 0x10040000 0x0 0x1000 0x0 0x20000000 0x0 0x10000000>;
+		interrupt-parent = <&plic>;
+		interrupts = <51>;
+		clocks = <&tlclk>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		sifive,fifo-depth = <8>;
+		sifive,max-bits-per-word = <8>;
+	};
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c94727e..50bfadd24c32 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -615,6 +615,12 @@ config SPI_SH_HSPI
 	help
 	  SPI driver for SuperH HSPI blocks.
 
+config SPI_SIFIVE
+	tristate "SiFive SPI controller"
+	depends on HAS_IOMEM
+	help
+	  This exposes the SPI controller IP from SiFive.
+
 config SPI_SIRF
 	tristate "CSR SiRFprimaII SPI controller"
 	depends on SIRF_DMA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 3575205c5c27..76216f1861df 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_SPI_SH)			+= spi-sh.o
 obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
 obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
+obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
 obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
 obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c
new file mode 100644
index 000000000000..8df19d2b8b7f
--- /dev/null
+++ b/drivers/spi/spi-sifive.c
@@ -0,0 +1,442 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiFive SPI controller driver (master mode only)
+ *
+ * Author: SiFive, Inc.
+ * sifive at sifive.com
+ *
+ * Copyright 2018 SiFive, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+
+/* for consistency we need this symbol */
+#ifdef REG_FMT
+#undef REG_FMT
+#endif
+
+#define SIFIVE_SPI_MAX_CS        32
+
+#define SIFIVE_SPI_NAME          "sifive_spi"
+
+#define SIFIVE_SPI_DEFAULT_DEPTH 8
+#define SIFIVE_SPI_DEFAULT_BITS  8
+
+/* register offsets */
+#define REG_SCKDIV               0x00 /* Serial clock divisor */
+#define REG_SCKMODE              0x04 /* Serial clock mode */
+#define REG_CSID                 0x10 /* Chip select ID */
+#define REG_CSDEF                0x14 /* Chip select default */
+#define REG_CSMODE               0x18 /* Chip select mode */
+#define REG_DELAY0               0x28 /* Delay control 0 */
+#define REG_DELAY1               0x2c /* Delay control 1 */
+#define REG_FMT                  0x40 /* Frame format */
+#define REG_TXDATA               0x48 /* Tx FIFO data */
+#define REG_RXDATA               0x4c /* Rx FIFO data */
+#define REG_TXMARK               0x50 /* Tx FIFO watermark */
+#define REG_RXMARK               0x54 /* Rx FIFO watermark */
+#define REG_FCTRL                0x60 /* SPI flash interface control */
+#define REG_FFMT                 0x64 /* SPI flash instruction format */
+#define REG_IE                   0x70 /* Interrupt Enable Register */
+#define REG_IP                   0x74 /* Interrupt Pendings Register */
+
+/* sckdiv bits */
+#define SCKDIV_DIV_MASK          0xfffU
+
+/* sckmode bits */
+#define SCKMODE_PHA              (1U << 0)
+#define SCKMODE_POL              (1U << 1)
+#define SCKMODE_MODE_MASK        (SCKMODE_PHA | SCKMODE_POL)
+
+/* csmode bits */
+#define CSMODE_MODE_AUTO         0U
+#define CSMODE_MODE_HOLD         2U
+#define CSMODE_MODE_OFF          3U
+
+/* delay0 bits */
+#define DELAY0_CSSCK_MASK        0xffU
+#define DELAY0_SCKCS_MASK        (0xffU << 16)
+
+/* delay1 bits */
+#define DELAY1_INTERCS_MASK      0xffU
+#define DELAY1_INTERXFR_MASK     (0xffU << 16)
+
+/* fmt bits */
+#define FMT_PROTO_SINGLE         0U
+#define FMT_PROTO_DUAL           1U
+#define FMT_PROTO_QUAD           2U
+#define FMT_PROTO_MASK           3U
+#define FMT_ENDIAN               (1U << 2)
+#define FMT_DIR                  (1U << 3)
+#define FMT_LEN(x)               ((u32)(x) << 16)
+#define FMT_LEN_MASK             (0xfU << 16)
+
+/* txdata bits */
+#define TXDATA_DATA_MASK         0xffU
+#define TXDATA_FULL              (1U << 31)
+
+/* rxdata bits */
+#define RXDATA_DATA_MASK         0xffU
+#define RXDATA_EMPTY             (1U << 31)
+
+/* ie and ip bits */
+#define IP_TXWM                  (1U << 0)
+#define IP_RXWM                  (1U << 1)
+
+struct sifive_spi {
+	void __iomem      *regs;        /* virt. address of control registers */
+	struct clk        *clk;         /* bus clock */
+	unsigned int      fifo_depth;   /* fifo depth in words */
+	u32               cs_inactive;  /* level of the CS pins when inactive */
+	struct completion done;         /* wake-up from interrupt */
+};
+
+static void sifive_spi_write(struct sifive_spi *spi, int offset, u32 value)
+{
+	iowrite32(value, spi->regs + offset);
+}
+
+static u32 sifive_spi_read(struct sifive_spi *spi, int offset)
+{
+	return ioread32(spi->regs + offset);
+}
+
+static void sifive_spi_init(struct sifive_spi *spi)
+{
+	/* Watermark interrupts are disabled by default */
+	sifive_spi_write(spi, REG_IE, 0);
+
+	/* Default watermark FIFO threshold values */
+	sifive_spi_write(spi, REG_TXMARK, 1);
+	sifive_spi_write(spi, REG_RXMARK, 0);
+
+	/* Set CS/SCK Delays and Inactive Time to defaults */
+
+	/* Exit specialized memory-mapped SPI flash mode */
+	sifive_spi_write(spi, REG_FCTRL, 0);
+}
+
+static int sifive_spi_prepare_message(struct spi_master *master,
+		struct spi_message *msg)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+	struct spi_device *device = msg->spi;
+
+	/* Update the chip select polarity */
+	if (device->mode & SPI_CS_HIGH)
+		spi->cs_inactive &= ~BIT(device->chip_select);
+	else
+		spi->cs_inactive |= BIT(device->chip_select);
+	sifive_spi_write(spi, REG_CSDEF, spi->cs_inactive);
+
+	/* Select the correct device */
+	sifive_spi_write(spi, REG_CSID, device->chip_select);
+
+	/* Set clock mode */
+	sifive_spi_write(spi, REG_SCKMODE, device->mode & SCKMODE_MODE_MASK);
+
+	return 0;
+}
+
+static int sifive_spi_prep_transfer(struct sifive_spi *spi,
+		struct spi_device *device, struct spi_transfer *t)
+{
+	u32 cr;
+	unsigned int mode;
+
+	/* Calculate and program the clock rate */
+	cr = DIV_ROUND_UP(clk_get_rate(spi->clk) >> 1, t->speed_hz) - 1;
+	cr &= SCKDIV_DIV_MASK;
+	sifive_spi_write(spi, REG_SCKDIV, cr);
+
+	mode = max_t(unsigned int, t->rx_nbits, t->tx_nbits);
+
+	/* Set frame format */
+	cr = FMT_LEN(t->bits_per_word);
+	switch (mode) {
+	case SPI_NBITS_QUAD:
+		cr |= FMT_PROTO_QUAD;
+		break;
+	case SPI_NBITS_DUAL:
+		cr |= FMT_PROTO_DUAL;
+		break;
+	default:
+		cr |= FMT_PROTO_SINGLE;
+		break;
+	}
+	if (device->mode & SPI_LSB_FIRST)
+		cr |= FMT_ENDIAN;
+	if (!t->rx_buf)
+		cr |= FMT_DIR;
+	sifive_spi_write(spi, REG_FMT, cr);
+
+	/* We will want to poll if the time we need to wait is
+	 * less than the context switching time.
+	 * Let's call that threshold 5us. The operation will take:
+	 *    (8/mode) * fifo_depth / hz <= 5 * 10^-6
+	 *    1600000 * fifo_depth <= hz * mode
+	 */
+	return 1600000 * spi->fifo_depth <= t->speed_hz * mode;
+}
+
+static void sifive_spi_tx(struct sifive_spi *spi, const u8 *tx_ptr)
+{
+	WARN_ON_ONCE((sifive_spi_read(spi, REG_TXDATA) & TXDATA_FULL) != 0);
+	sifive_spi_write(spi, REG_TXDATA, *tx_ptr & TXDATA_DATA_MASK);
+}
+
+static void sifive_spi_rx(struct sifive_spi *spi, u8 *rx_ptr)
+{
+	u32 data = sifive_spi_read(spi, REG_RXDATA);
+
+	WARN_ON_ONCE((data & RXDATA_EMPTY) != 0);
+	*rx_ptr = data & RXDATA_DATA_MASK;
+}
+
+static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
+{
+	if (poll) {
+		u32 cr;
+		do cr = sifive_spi_read(spi, REG_IP);
+		while (!(cr & bit));
+	} else {
+		reinit_completion(&spi->done);
+		sifive_spi_write(spi, REG_IE, bit);
+		wait_for_completion(&spi->done);
+	}
+}
+
+static void sifive_spi_execute(struct sifive_spi *spi,
+		struct spi_transfer *t, int poll)
+{
+	const u8 *tx_ptr = t->tx_buf;
+	u8 *rx_ptr = t->rx_buf;
+	unsigned int remaining_words = t->len;
+
+	while (remaining_words) {
+		unsigned int n_words = min(remaining_words, spi->fifo_depth);
+		unsigned int i;
+
+		/* Enqueue n_words for transmission */
+		for (i = 0; i < n_words; i++)
+			sifive_spi_tx(spi, tx_ptr++);
+
+		if (rx_ptr) {
+			/* Wait for transmission + reception to complete */
+			sifive_spi_write(spi, REG_RXMARK, n_words-1);
+			sifive_spi_wait(spi, IP_RXWM, poll);
+
+			/* Read out all the data from the RX FIFO */
+			for (i = 0; i < n_words; i++)
+				sifive_spi_rx(spi, rx_ptr++);
+		} else {
+			/* Wait for transmission to complete */
+			sifive_spi_wait(spi, IP_TXWM, poll);
+		}
+
+		remaining_words -= n_words;
+	}
+}
+
+static int sifive_spi_transfer_one(struct spi_master *master,
+		struct spi_device *device, struct spi_transfer *t)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+	int poll = sifive_spi_prep_transfer(spi, device, t);
+
+	sifive_spi_execute(spi, t, poll);
+
+	return 0;
+}
+
+static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(device->master);
+
+	/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
+	if (device->mode & SPI_CS_HIGH)
+		is_high = !is_high;
+
+	sifive_spi_write(spi, REG_CSMODE,
+			is_high ? CSMODE_MODE_AUTO : CSMODE_MODE_HOLD);
+}
+
+static irqreturn_t sifive_spi_irq(int irq, void *dev_id)
+{
+	struct sifive_spi *spi = dev_id;
+	u32 ip = sifive_spi_read(spi, REG_IP);
+
+	if (ip & (IP_TXWM | IP_RXWM)) {
+		/* Disable interrupts until next transfer */
+		sifive_spi_write(spi, REG_IE, 0);
+		complete(&spi->done);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int sifive_spi_probe(struct platform_device *pdev)
+{
+	struct sifive_spi *spi;
+	struct resource *res;
+	int ret, irq, num_cs;
+	u32 cs_bits, max_bits_per_word;
+	struct spi_master *master;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct sifive_spi));
+	if (!master) {
+		dev_err(&pdev->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+
+	spi = spi_master_get_devdata(master);
+	init_completion(&spi->done);
+	platform_set_drvdata(pdev, master);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	spi->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(spi->regs)) {
+		dev_err(&pdev->dev, "Unable to map IO resources\n");
+		ret = PTR_ERR(spi->regs);
+		goto put_master;
+	}
+
+	spi->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(spi->clk)) {
+		dev_err(&pdev->dev, "Unable to find bus clock\n");
+		ret = PTR_ERR(spi->clk);
+		goto put_master;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Unable to find interrupt\n");
+		ret = irq;
+		goto put_master;
+	}
+
+	/* Optional parameters */
+	ret = of_property_read_u32(pdev->dev.of_node,
+			"sifive,fifo-depth", &spi->fifo_depth);
+	if (ret < 0)
+		spi->fifo_depth = SIFIVE_SPI_DEFAULT_DEPTH;
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+			"sifive,max-bits-per-word", &max_bits_per_word);
+	if (!ret && max_bits_per_word < 8) {
+		dev_err(&pdev->dev, "Only 8bit SPI words are supported by the driver\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	/* Spin up the bus clock before hitting registers */
+	ret = clk_prepare_enable(spi->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable bus clock\n");
+		goto put_master;
+	}
+
+	/* probe the number of CS lines */
+	spi->cs_inactive = sifive_spi_read(spi, REG_CSDEF);
+	sifive_spi_write(spi, REG_CSDEF, 0xffffffffU);
+	cs_bits = sifive_spi_read(spi, REG_CSDEF);
+	sifive_spi_write(spi, REG_CSDEF, spi->cs_inactive);
+	if (!cs_bits) {
+		dev_err(&pdev->dev, "Could not auto probe CS lines\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	num_cs = ilog2(cs_bits) + 1;
+	if (num_cs > SIFIVE_SPI_MAX_CS) {
+		dev_err(&pdev->dev, "Invalid number of spi slaves\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	/* Define our master */
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->num_chipselect = num_cs;
+	master->mode_bits = SPI_CPHA | SPI_CPOL
+	                  | SPI_CS_HIGH | SPI_LSB_FIRST
+	                  | SPI_TX_DUAL | SPI_TX_QUAD
+	                  | SPI_RX_DUAL | SPI_RX_QUAD;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->flags = SPI_CONTROLLER_MUST_TX | SPI_MASTER_GPIO_SS;
+	master->prepare_message = sifive_spi_prepare_message;
+	master->set_cs = sifive_spi_set_cs;
+	master->transfer_one = sifive_spi_transfer_one;
+
+	/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
+	 * Probably it should rely on the SPI core auto mapping instead.
+	 */
+	pdev->dev.dma_mask = NULL;
+
+	/* Configure the SPI master hardware */
+	sifive_spi_init(spi);
+
+	/* Register for SPI Interrupt */
+	ret = devm_request_irq(&pdev->dev, irq, sifive_spi_irq, 0,
+				dev_name(&pdev->dev), spi);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to bind to interrupt\n");
+		goto put_master;
+	}
+
+	dev_info(&pdev->dev, "mapped; irq=%d, cs=%d\n",
+		irq, master->num_chipselect);
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "spi_register_master failed\n");
+		goto put_master;
+	}
+
+	return 0;
+
+put_master:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int sifive_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+
+	/* Disable all the interrupts just in case */
+	sifive_spi_write(spi, REG_IE, 0);
+	spi_master_put(master);
+
+	return 0;
+}
+
+static const struct of_device_id sifive_spi_of_match[] = {
+	{ .compatible = "sifive,spi0", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sifive_spi_of_match);
+
+static struct platform_driver sifive_spi_driver = {
+	.probe = sifive_spi_probe,
+	.remove = sifive_spi_remove,
+	.driver = {
+		.name = SIFIVE_SPI_NAME,
+		.of_match_table = sifive_spi_of_match,
+	},
+};
+module_platform_driver(sifive_spi_driver);
+
+MODULE_AUTHOR("SiFive, Inc. <sifive@sifive.com>");
+MODULE_DESCRIPTION("SiFive SPI driver");
+MODULE_LICENSE("GPL");
-- 
2.19.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-12 14:27 [RFC PATCH] spi: add driver for the SiFive SPI controller kernel
@ 2018-11-12 14:27 ` Emil Renner Berthing
  2018-11-13 18:35 ` broonie
  2019-02-13 10:03 ` Yash Shah
  2 siblings, 0 replies; 10+ messages in thread
From: Emil Renner Berthing @ 2018-11-12 14:27 UTC (permalink / raw)
  To: linux-spi
  Cc: Mark Rutland, devicetree, Emil Renner Berthing, Palmer Dabbelt,
	linux-kernel, Rob Herring, Mark Brown, linux-riscv

From: Palmer Dabbelt <palmer@sifive.com>

Add driver for the SiFive SPI controller
on the HiFive Unleashed board.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
---
 .../devicetree/bindings/spi/spi-sifive.txt    |  29 ++
 drivers/spi/Kconfig                           |   6 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-sifive.c                      | 442 ++++++++++++++++++
 4 files changed, 478 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sifive.txt
 create mode 100644 drivers/spi/spi-sifive.c

Hi all,

I know the discussions about the sifive devicetree compatible
strings haven't come to a conclusion, so I'm sending this as
an RFC to get some feedback on the rest of the code.

Compared to the original[1] I've done the following:

- Update register names and bit fields to the ones in the
  FU540-C000 documentation.

- Change the optional devicetree property from "sifive,buffer-size"
  to "sifive,fifo-depth". The string "fifo-depth" seems to have more
  hits in the existing devicetree bindings and is IMHO a little more
  descriptive.

- Change the optional devicetree property from "sifive,bits-per-word"
  to "sifive,max-bits-per-word". For a long time I wondered why the
  SPI word size was a property on the controller and not the device.
  This change makes the meaning a little more clear.

- Be honest about only supporting 8bit SPI words in the driver.
  Without SPI_LSB_FIRST and bits_per_word < 8 we need some code
  to shift the bits in each byte which is not yet there.

- Program the csdef, csid and sckmode registers from prepare_message
  rather than transfer_one. This way we only do it once pr. message
  rather than every transfer.

- Drop the irq field from driver data. With devm_request_irq we
  don't need to remember it after requesting the irq.

- Drop hz = t->speed_hz ? t->speed_hz : device->max_speed_hz;
  The SPI framework handles this for us, so we can just always
  use t->speed_hz.

- Fix most checkpatch warnings.

[1]: https://github.com/riscv/riscv-linux/commit/801805694740ad0895e21d10b8f124d138beefbb

/Emil


diff --git a/Documentation/devicetree/bindings/spi/spi-sifive.txt b/Documentation/devicetree/bindings/spi/spi-sifive.txt
new file mode 100644
index 000000000000..96339afcc74f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sifive.txt
@@ -0,0 +1,29 @@
+SiFive SPI controller Device Tree Bindings
+------------------------------------------
+
+Required properties:
+- compatible		: Should be "sifive,spi0"
+- reg			: Physical base address and size of SPI registers map
+			  A second (optional) range can indicate memory mapped flash
+- interrupts		: Must contain one entry
+- interrupt-parent	: Must be core interrupt controller
+- clocks		: Must reference the frequency given to the controller
+- #address-cells	: Must be '1', indicating which CS to use
+- #size-cells		: Must be '0'
+
+Optional properties:
+- sifive,fifo-depth		: Depth of hardware queues; defaults to 8
+- sifive,max-bits-per-word	: Maximum bits per word; defaults to 8
+
+Example:
+	spi: spi@10040000 {
+		compatible = "sifive,spi0";
+		reg = <0x0 0x10040000 0x0 0x1000 0x0 0x20000000 0x0 0x10000000>;
+		interrupt-parent = <&plic>;
+		interrupts = <51>;
+		clocks = <&tlclk>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		sifive,fifo-depth = <8>;
+		sifive,max-bits-per-word = <8>;
+	};
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c94727e..50bfadd24c32 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -615,6 +615,12 @@ config SPI_SH_HSPI
 	help
 	  SPI driver for SuperH HSPI blocks.
 
+config SPI_SIFIVE
+	tristate "SiFive SPI controller"
+	depends on HAS_IOMEM
+	help
+	  This exposes the SPI controller IP from SiFive.
+
 config SPI_SIRF
 	tristate "CSR SiRFprimaII SPI controller"
 	depends on SIRF_DMA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 3575205c5c27..76216f1861df 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_SPI_SH)			+= spi-sh.o
 obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
 obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
+obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
 obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
 obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c
new file mode 100644
index 000000000000..8df19d2b8b7f
--- /dev/null
+++ b/drivers/spi/spi-sifive.c
@@ -0,0 +1,442 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiFive SPI controller driver (master mode only)
+ *
+ * Author: SiFive, Inc.
+ * sifive@sifive.com
+ *
+ * Copyright 2018 SiFive, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+
+/* for consistency we need this symbol */
+#ifdef REG_FMT
+#undef REG_FMT
+#endif
+
+#define SIFIVE_SPI_MAX_CS        32
+
+#define SIFIVE_SPI_NAME          "sifive_spi"
+
+#define SIFIVE_SPI_DEFAULT_DEPTH 8
+#define SIFIVE_SPI_DEFAULT_BITS  8
+
+/* register offsets */
+#define REG_SCKDIV               0x00 /* Serial clock divisor */
+#define REG_SCKMODE              0x04 /* Serial clock mode */
+#define REG_CSID                 0x10 /* Chip select ID */
+#define REG_CSDEF                0x14 /* Chip select default */
+#define REG_CSMODE               0x18 /* Chip select mode */
+#define REG_DELAY0               0x28 /* Delay control 0 */
+#define REG_DELAY1               0x2c /* Delay control 1 */
+#define REG_FMT                  0x40 /* Frame format */
+#define REG_TXDATA               0x48 /* Tx FIFO data */
+#define REG_RXDATA               0x4c /* Rx FIFO data */
+#define REG_TXMARK               0x50 /* Tx FIFO watermark */
+#define REG_RXMARK               0x54 /* Rx FIFO watermark */
+#define REG_FCTRL                0x60 /* SPI flash interface control */
+#define REG_FFMT                 0x64 /* SPI flash instruction format */
+#define REG_IE                   0x70 /* Interrupt Enable Register */
+#define REG_IP                   0x74 /* Interrupt Pendings Register */
+
+/* sckdiv bits */
+#define SCKDIV_DIV_MASK          0xfffU
+
+/* sckmode bits */
+#define SCKMODE_PHA              (1U << 0)
+#define SCKMODE_POL              (1U << 1)
+#define SCKMODE_MODE_MASK        (SCKMODE_PHA | SCKMODE_POL)
+
+/* csmode bits */
+#define CSMODE_MODE_AUTO         0U
+#define CSMODE_MODE_HOLD         2U
+#define CSMODE_MODE_OFF          3U
+
+/* delay0 bits */
+#define DELAY0_CSSCK_MASK        0xffU
+#define DELAY0_SCKCS_MASK        (0xffU << 16)
+
+/* delay1 bits */
+#define DELAY1_INTERCS_MASK      0xffU
+#define DELAY1_INTERXFR_MASK     (0xffU << 16)
+
+/* fmt bits */
+#define FMT_PROTO_SINGLE         0U
+#define FMT_PROTO_DUAL           1U
+#define FMT_PROTO_QUAD           2U
+#define FMT_PROTO_MASK           3U
+#define FMT_ENDIAN               (1U << 2)
+#define FMT_DIR                  (1U << 3)
+#define FMT_LEN(x)               ((u32)(x) << 16)
+#define FMT_LEN_MASK             (0xfU << 16)
+
+/* txdata bits */
+#define TXDATA_DATA_MASK         0xffU
+#define TXDATA_FULL              (1U << 31)
+
+/* rxdata bits */
+#define RXDATA_DATA_MASK         0xffU
+#define RXDATA_EMPTY             (1U << 31)
+
+/* ie and ip bits */
+#define IP_TXWM                  (1U << 0)
+#define IP_RXWM                  (1U << 1)
+
+struct sifive_spi {
+	void __iomem      *regs;        /* virt. address of control registers */
+	struct clk        *clk;         /* bus clock */
+	unsigned int      fifo_depth;   /* fifo depth in words */
+	u32               cs_inactive;  /* level of the CS pins when inactive */
+	struct completion done;         /* wake-up from interrupt */
+};
+
+static void sifive_spi_write(struct sifive_spi *spi, int offset, u32 value)
+{
+	iowrite32(value, spi->regs + offset);
+}
+
+static u32 sifive_spi_read(struct sifive_spi *spi, int offset)
+{
+	return ioread32(spi->regs + offset);
+}
+
+static void sifive_spi_init(struct sifive_spi *spi)
+{
+	/* Watermark interrupts are disabled by default */
+	sifive_spi_write(spi, REG_IE, 0);
+
+	/* Default watermark FIFO threshold values */
+	sifive_spi_write(spi, REG_TXMARK, 1);
+	sifive_spi_write(spi, REG_RXMARK, 0);
+
+	/* Set CS/SCK Delays and Inactive Time to defaults */
+
+	/* Exit specialized memory-mapped SPI flash mode */
+	sifive_spi_write(spi, REG_FCTRL, 0);
+}
+
+static int sifive_spi_prepare_message(struct spi_master *master,
+		struct spi_message *msg)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+	struct spi_device *device = msg->spi;
+
+	/* Update the chip select polarity */
+	if (device->mode & SPI_CS_HIGH)
+		spi->cs_inactive &= ~BIT(device->chip_select);
+	else
+		spi->cs_inactive |= BIT(device->chip_select);
+	sifive_spi_write(spi, REG_CSDEF, spi->cs_inactive);
+
+	/* Select the correct device */
+	sifive_spi_write(spi, REG_CSID, device->chip_select);
+
+	/* Set clock mode */
+	sifive_spi_write(spi, REG_SCKMODE, device->mode & SCKMODE_MODE_MASK);
+
+	return 0;
+}
+
+static int sifive_spi_prep_transfer(struct sifive_spi *spi,
+		struct spi_device *device, struct spi_transfer *t)
+{
+	u32 cr;
+	unsigned int mode;
+
+	/* Calculate and program the clock rate */
+	cr = DIV_ROUND_UP(clk_get_rate(spi->clk) >> 1, t->speed_hz) - 1;
+	cr &= SCKDIV_DIV_MASK;
+	sifive_spi_write(spi, REG_SCKDIV, cr);
+
+	mode = max_t(unsigned int, t->rx_nbits, t->tx_nbits);
+
+	/* Set frame format */
+	cr = FMT_LEN(t->bits_per_word);
+	switch (mode) {
+	case SPI_NBITS_QUAD:
+		cr |= FMT_PROTO_QUAD;
+		break;
+	case SPI_NBITS_DUAL:
+		cr |= FMT_PROTO_DUAL;
+		break;
+	default:
+		cr |= FMT_PROTO_SINGLE;
+		break;
+	}
+	if (device->mode & SPI_LSB_FIRST)
+		cr |= FMT_ENDIAN;
+	if (!t->rx_buf)
+		cr |= FMT_DIR;
+	sifive_spi_write(spi, REG_FMT, cr);
+
+	/* We will want to poll if the time we need to wait is
+	 * less than the context switching time.
+	 * Let's call that threshold 5us. The operation will take:
+	 *    (8/mode) * fifo_depth / hz <= 5 * 10^-6
+	 *    1600000 * fifo_depth <= hz * mode
+	 */
+	return 1600000 * spi->fifo_depth <= t->speed_hz * mode;
+}
+
+static void sifive_spi_tx(struct sifive_spi *spi, const u8 *tx_ptr)
+{
+	WARN_ON_ONCE((sifive_spi_read(spi, REG_TXDATA) & TXDATA_FULL) != 0);
+	sifive_spi_write(spi, REG_TXDATA, *tx_ptr & TXDATA_DATA_MASK);
+}
+
+static void sifive_spi_rx(struct sifive_spi *spi, u8 *rx_ptr)
+{
+	u32 data = sifive_spi_read(spi, REG_RXDATA);
+
+	WARN_ON_ONCE((data & RXDATA_EMPTY) != 0);
+	*rx_ptr = data & RXDATA_DATA_MASK;
+}
+
+static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
+{
+	if (poll) {
+		u32 cr;
+		do cr = sifive_spi_read(spi, REG_IP);
+		while (!(cr & bit));
+	} else {
+		reinit_completion(&spi->done);
+		sifive_spi_write(spi, REG_IE, bit);
+		wait_for_completion(&spi->done);
+	}
+}
+
+static void sifive_spi_execute(struct sifive_spi *spi,
+		struct spi_transfer *t, int poll)
+{
+	const u8 *tx_ptr = t->tx_buf;
+	u8 *rx_ptr = t->rx_buf;
+	unsigned int remaining_words = t->len;
+
+	while (remaining_words) {
+		unsigned int n_words = min(remaining_words, spi->fifo_depth);
+		unsigned int i;
+
+		/* Enqueue n_words for transmission */
+		for (i = 0; i < n_words; i++)
+			sifive_spi_tx(spi, tx_ptr++);
+
+		if (rx_ptr) {
+			/* Wait for transmission + reception to complete */
+			sifive_spi_write(spi, REG_RXMARK, n_words-1);
+			sifive_spi_wait(spi, IP_RXWM, poll);
+
+			/* Read out all the data from the RX FIFO */
+			for (i = 0; i < n_words; i++)
+				sifive_spi_rx(spi, rx_ptr++);
+		} else {
+			/* Wait for transmission to complete */
+			sifive_spi_wait(spi, IP_TXWM, poll);
+		}
+
+		remaining_words -= n_words;
+	}
+}
+
+static int sifive_spi_transfer_one(struct spi_master *master,
+		struct spi_device *device, struct spi_transfer *t)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+	int poll = sifive_spi_prep_transfer(spi, device, t);
+
+	sifive_spi_execute(spi, t, poll);
+
+	return 0;
+}
+
+static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
+{
+	struct sifive_spi *spi = spi_master_get_devdata(device->master);
+
+	/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
+	if (device->mode & SPI_CS_HIGH)
+		is_high = !is_high;
+
+	sifive_spi_write(spi, REG_CSMODE,
+			is_high ? CSMODE_MODE_AUTO : CSMODE_MODE_HOLD);
+}
+
+static irqreturn_t sifive_spi_irq(int irq, void *dev_id)
+{
+	struct sifive_spi *spi = dev_id;
+	u32 ip = sifive_spi_read(spi, REG_IP);
+
+	if (ip & (IP_TXWM | IP_RXWM)) {
+		/* Disable interrupts until next transfer */
+		sifive_spi_write(spi, REG_IE, 0);
+		complete(&spi->done);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int sifive_spi_probe(struct platform_device *pdev)
+{
+	struct sifive_spi *spi;
+	struct resource *res;
+	int ret, irq, num_cs;
+	u32 cs_bits, max_bits_per_word;
+	struct spi_master *master;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct sifive_spi));
+	if (!master) {
+		dev_err(&pdev->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+
+	spi = spi_master_get_devdata(master);
+	init_completion(&spi->done);
+	platform_set_drvdata(pdev, master);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	spi->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(spi->regs)) {
+		dev_err(&pdev->dev, "Unable to map IO resources\n");
+		ret = PTR_ERR(spi->regs);
+		goto put_master;
+	}
+
+	spi->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(spi->clk)) {
+		dev_err(&pdev->dev, "Unable to find bus clock\n");
+		ret = PTR_ERR(spi->clk);
+		goto put_master;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Unable to find interrupt\n");
+		ret = irq;
+		goto put_master;
+	}
+
+	/* Optional parameters */
+	ret = of_property_read_u32(pdev->dev.of_node,
+			"sifive,fifo-depth", &spi->fifo_depth);
+	if (ret < 0)
+		spi->fifo_depth = SIFIVE_SPI_DEFAULT_DEPTH;
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+			"sifive,max-bits-per-word", &max_bits_per_word);
+	if (!ret && max_bits_per_word < 8) {
+		dev_err(&pdev->dev, "Only 8bit SPI words are supported by the driver\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	/* Spin up the bus clock before hitting registers */
+	ret = clk_prepare_enable(spi->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable bus clock\n");
+		goto put_master;
+	}
+
+	/* probe the number of CS lines */
+	spi->cs_inactive = sifive_spi_read(spi, REG_CSDEF);
+	sifive_spi_write(spi, REG_CSDEF, 0xffffffffU);
+	cs_bits = sifive_spi_read(spi, REG_CSDEF);
+	sifive_spi_write(spi, REG_CSDEF, spi->cs_inactive);
+	if (!cs_bits) {
+		dev_err(&pdev->dev, "Could not auto probe CS lines\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	num_cs = ilog2(cs_bits) + 1;
+	if (num_cs > SIFIVE_SPI_MAX_CS) {
+		dev_err(&pdev->dev, "Invalid number of spi slaves\n");
+		ret = -EINVAL;
+		goto put_master;
+	}
+
+	/* Define our master */
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->num_chipselect = num_cs;
+	master->mode_bits = SPI_CPHA | SPI_CPOL
+	                  | SPI_CS_HIGH | SPI_LSB_FIRST
+	                  | SPI_TX_DUAL | SPI_TX_QUAD
+	                  | SPI_RX_DUAL | SPI_RX_QUAD;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->flags = SPI_CONTROLLER_MUST_TX | SPI_MASTER_GPIO_SS;
+	master->prepare_message = sifive_spi_prepare_message;
+	master->set_cs = sifive_spi_set_cs;
+	master->transfer_one = sifive_spi_transfer_one;
+
+	/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
+	 * Probably it should rely on the SPI core auto mapping instead.
+	 */
+	pdev->dev.dma_mask = NULL;
+
+	/* Configure the SPI master hardware */
+	sifive_spi_init(spi);
+
+	/* Register for SPI Interrupt */
+	ret = devm_request_irq(&pdev->dev, irq, sifive_spi_irq, 0,
+				dev_name(&pdev->dev), spi);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to bind to interrupt\n");
+		goto put_master;
+	}
+
+	dev_info(&pdev->dev, "mapped; irq=%d, cs=%d\n",
+		irq, master->num_chipselect);
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "spi_register_master failed\n");
+		goto put_master;
+	}
+
+	return 0;
+
+put_master:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int sifive_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct sifive_spi *spi = spi_master_get_devdata(master);
+
+	/* Disable all the interrupts just in case */
+	sifive_spi_write(spi, REG_IE, 0);
+	spi_master_put(master);
+
+	return 0;
+}
+
+static const struct of_device_id sifive_spi_of_match[] = {
+	{ .compatible = "sifive,spi0", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sifive_spi_of_match);
+
+static struct platform_driver sifive_spi_driver = {
+	.probe = sifive_spi_probe,
+	.remove = sifive_spi_remove,
+	.driver = {
+		.name = SIFIVE_SPI_NAME,
+		.of_match_table = sifive_spi_of_match,
+	},
+};
+module_platform_driver(sifive_spi_driver);
+
+MODULE_AUTHOR("SiFive, Inc. <sifive@sifive.com>");
+MODULE_DESCRIPTION("SiFive SPI driver");
+MODULE_LICENSE("GPL");
-- 
2.19.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-12 14:27 [RFC PATCH] spi: add driver for the SiFive SPI controller kernel
  2018-11-12 14:27 ` Emil Renner Berthing
@ 2018-11-13 18:35 ` broonie
  2018-11-13 18:35   ` Mark Brown
  2018-11-13 19:48   ` kernel
  2019-02-13 10:03 ` Yash Shah
  2 siblings, 2 replies; 10+ messages in thread
From: broonie @ 2018-11-13 18:35 UTC (permalink / raw)
  To: linux-riscv

On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:

> I know the discussions about the sifive devicetree compatible
> strings haven't come to a conclusion, so I'm sending this as
> an RFC to get some feedback on the rest of the code.

I've not seen any of these discussions or earlier versions of this
driver so I've no idea what's going on here :(

> +Optional properties:
> +- sifive,fifo-depth		: Depth of hardware queues; defaults to 8
> +- sifive,max-bits-per-word	: Maximum bits per word; defaults to 8
> +

If the hardware isn't fixed yet making these enumerable from the
hardware would be good...

> @@ -0,0 +1,442 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SiFive SPI controller driver (master mode only)
> + *

Please make the entire comment a C++ one to make this look more
intentinal.

> +/* for consistency we need this symbol */
> +#ifdef REG_FMT
> +#undef REG_FMT
> +#endif

We do?  For consistency with what?

> +static void sifive_spi_init(struct sifive_spi *spi)
> +{

> +	/* Set CS/SCK Delays and Inactive Time to defaults */
> +
> +	/* Exit specialized memory-mapped SPI flash mode */

...or not?

> +	/* Set frame format */
> +	cr = FMT_LEN(t->bits_per_word);
> +	switch (mode) {
> +	case SPI_NBITS_QUAD:
> +		cr |= FMT_PROTO_QUAD;
> +		break;

Some namespacing on the driver #defines would be a bit safer against the
possibility of collision with future changes in headers.

> +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> +{
> +	if (poll) {
> +		u32 cr;
> +		do cr = sifive_spi_read(spi, REG_IP);
> +		while (!(cr & bit));

Please add some braces, indentation or something to make it more clear
that the read is part of a do/while loop - right now it's not
immediately obvious that this is correct.

> +static int sifive_spi_transfer_one(struct spi_master *master,
> +		struct spi_device *device, struct spi_transfer *t)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(master);
> +	int poll = sifive_spi_prep_transfer(spi, device, t);
> +
> +	sifive_spi_execute(spi, t, poll);
> +

Why not just inline the execute function here?  It's the only caller
AFAICT.

> +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(device->master);
> +
> +	/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> +	if (device->mode & SPI_CS_HIGH)
> +		is_high = !is_high;

spi_set_cs() will handle CS_HIGH for you.

> +	master->bits_per_word_mask = SPI_BPW_MASK(8);

I thought the device supported other bits per word values?

> +	/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> +	 * Probably it should rely on the SPI core auto mapping instead.
> +	 */
> +	pdev->dev.dma_mask = NULL;

If this is a problem please fix it in the MMC core, don't bodge it like
this.

> +static const struct of_device_id sifive_spi_of_match[] = {
> +	{ .compatible = "sifive,spi0", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);

spi0 is a *weird* compatible name.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20181113/cc9de2da/attachment.sig>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-13 18:35 ` broonie
@ 2018-11-13 18:35   ` Mark Brown
  2018-11-13 19:48   ` kernel
  1 sibling, 0 replies; 10+ messages in thread
From: Mark Brown @ 2018-11-13 18:35 UTC (permalink / raw)
  To: Emil Renner Berthing
  Cc: Mark Rutland, devicetree, Palmer Dabbelt, linux-kernel,
	linux-spi, Rob Herring, linux-riscv

[-- Attachment #1.1: Type: text/plain, Size: 3044 bytes --]

On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:

> I know the discussions about the sifive devicetree compatible
> strings haven't come to a conclusion, so I'm sending this as
> an RFC to get some feedback on the rest of the code.

I've not seen any of these discussions or earlier versions of this
driver so I've no idea what's going on here :(

> +Optional properties:
> +- sifive,fifo-depth		: Depth of hardware queues; defaults to 8
> +- sifive,max-bits-per-word	: Maximum bits per word; defaults to 8
> +

If the hardware isn't fixed yet making these enumerable from the
hardware would be good...

> @@ -0,0 +1,442 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SiFive SPI controller driver (master mode only)
> + *

Please make the entire comment a C++ one to make this look more
intentinal.

> +/* for consistency we need this symbol */
> +#ifdef REG_FMT
> +#undef REG_FMT
> +#endif

We do?  For consistency with what?

> +static void sifive_spi_init(struct sifive_spi *spi)
> +{

> +	/* Set CS/SCK Delays and Inactive Time to defaults */
> +
> +	/* Exit specialized memory-mapped SPI flash mode */

...or not?

> +	/* Set frame format */
> +	cr = FMT_LEN(t->bits_per_word);
> +	switch (mode) {
> +	case SPI_NBITS_QUAD:
> +		cr |= FMT_PROTO_QUAD;
> +		break;

Some namespacing on the driver #defines would be a bit safer against the
possibility of collision with future changes in headers.

> +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> +{
> +	if (poll) {
> +		u32 cr;
> +		do cr = sifive_spi_read(spi, REG_IP);
> +		while (!(cr & bit));

Please add some braces, indentation or something to make it more clear
that the read is part of a do/while loop - right now it's not
immediately obvious that this is correct.

> +static int sifive_spi_transfer_one(struct spi_master *master,
> +		struct spi_device *device, struct spi_transfer *t)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(master);
> +	int poll = sifive_spi_prep_transfer(spi, device, t);
> +
> +	sifive_spi_execute(spi, t, poll);
> +

Why not just inline the execute function here?  It's the only caller
AFAICT.

> +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(device->master);
> +
> +	/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> +	if (device->mode & SPI_CS_HIGH)
> +		is_high = !is_high;

spi_set_cs() will handle CS_HIGH for you.

> +	master->bits_per_word_mask = SPI_BPW_MASK(8);

I thought the device supported other bits per word values?

> +	/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> +	 * Probably it should rely on the SPI core auto mapping instead.
> +	 */
> +	pdev->dev.dma_mask = NULL;

If this is a problem please fix it in the MMC core, don't bodge it like
this.

> +static const struct of_device_id sifive_spi_of_match[] = {
> +	{ .compatible = "sifive,spi0", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);

spi0 is a *weird* compatible name.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-13 18:35 ` broonie
  2018-11-13 18:35   ` Mark Brown
@ 2018-11-13 19:48   ` kernel
  2018-11-13 19:48     ` Emil Renner Berthing
  2018-11-13 22:38     ` broonie
  1 sibling, 2 replies; 10+ messages in thread
From: kernel @ 2018-11-13 19:48 UTC (permalink / raw)
  To: linux-riscv

Hi Mark,
On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:
>
> > I know the discussions about the sifive devicetree compatible
> > strings haven't come to a conclusion, so I'm sending this as
> > an RFC to get some feedback on the rest of the code.
>
> I've not seen any of these discussions or earlier versions of this
> driver so I've no idea what's going on here :(

No, sorry. This has been discussed on linux-riscv for other drivers
like the uart. See my last answer.

> > +Optional properties:
> > +- sifive,fifo-depth          : Depth of hardware queues; defaults to 8
> > +- sifive,max-bits-per-word   : Maximum bits per word; defaults to 8
> > +
>
> If the hardware isn't fixed yet making these enumerable from the
> hardware would be good...

Agreed, but unfortunately this is already in the FU540-C000 chip on
the HiFive Unleashed board sold by SiFive.

> > @@ -0,0 +1,442 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SiFive SPI controller driver (master mode only)
> > + *
>
> Please make the entire comment a C++ one to make this look more
> intentinal.

Will do.

> > +/* for consistency we need this symbol */
> > +#ifdef REG_FMT
> > +#undef REG_FMT
> > +#endif
>
> We do?  For consistency with what?

Below all the register offsets are defined as
REG_<register name>. This is is a pattern I
copied from other drivers, but here we have a
register called "fmt" - hence REG_FMT.
If you have a better pattern that doesn't clash
with REG_FMT please let me know.

> > +static void sifive_spi_init(struct sifive_spi *spi)
> > +{
>
> > +     /* Set CS/SCK Delays and Inactive Time to defaults */
> > +
> > +     /* Exit specialized memory-mapped SPI flash mode */
>
> ...or not?

Right. Will add that or just delete the comment.

> > +     /* Set frame format */
> > +     cr = FMT_LEN(t->bits_per_word);
> > +     switch (mode) {
> > +     case SPI_NBITS_QUAD:
> > +             cr |= FMT_PROTO_QUAD;
> > +             break;
>
> Some namespacing on the driver #defines would be a bit safer against the
> possibility of collision with future changes in headers.

Right.

> > +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> > +{
> > +     if (poll) {
> > +             u32 cr;
> > +             do cr = sifive_spi_read(spi, REG_IP);
> > +             while (!(cr & bit));
>
> Please add some braces, indentation or something to make it more clear
> that the read is part of a do/while loop - right now it's not
> immediately obvious that this is correct.

Good point. Will do.

> > +static int sifive_spi_transfer_one(struct spi_master *master,
> > +             struct spi_device *device, struct spi_transfer *t)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(master);
> > +     int poll = sifive_spi_prep_transfer(spi, device, t);
> > +
> > +     sifive_spi_execute(spi, t, poll);
> > +
>
> Why not just inline the execute function here?  It's the only caller
> AFAICT.

Yeah, it is. Will do.

> > +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(device->master);
> > +
> > +     /* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> > +     if (device->mode & SPI_CS_HIGH)
> > +             is_high = !is_high;
>
> spi_set_cs() will handle CS_HIGH for you.
>
> > +     master->bits_per_word_mask = SPI_BPW_MASK(8);
>
> I thought the device supported other bits per word values?

It does, but the driver doesn't yet. When bits per word is < 8
we need to shift the bits in each byte to be "left-aligned"
(unless SPI_LSB_FIRST is set).

> > +     /* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> > +      * Probably it should rely on the SPI core auto mapping instead.
> > +      */
> > +     pdev->dev.dma_mask = NULL;
>
> If this is a problem please fix it in the MMC core, don't bodge it like
> this.

Gotcha. Will remove this.

> > +static const struct of_device_id sifive_spi_of_match[] = {
> > +     { .compatible = "sifive,spi0", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);
>
> spi0 is a *weird* compatible name.

Exactly. Hence the discussion about the compatible strings.
Once this discussion comes to a conclusion I'll update this.

Thank you for the review!
/Emil

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-13 19:48   ` kernel
@ 2018-11-13 19:48     ` Emil Renner Berthing
  2018-11-13 22:38     ` broonie
  1 sibling, 0 replies; 10+ messages in thread
From: Emil Renner Berthing @ 2018-11-13 19:48 UTC (permalink / raw)
  To: Mark Brown
  Cc: Mark Rutland, devicetree, Palmer Dabbelt,
	Linux Kernel Mailing List, linux-spi, Rob Herring, linux-riscv

Hi Mark,
On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:
>
> > I know the discussions about the sifive devicetree compatible
> > strings haven't come to a conclusion, so I'm sending this as
> > an RFC to get some feedback on the rest of the code.
>
> I've not seen any of these discussions or earlier versions of this
> driver so I've no idea what's going on here :(

No, sorry. This has been discussed on linux-riscv for other drivers
like the uart. See my last answer.

> > +Optional properties:
> > +- sifive,fifo-depth          : Depth of hardware queues; defaults to 8
> > +- sifive,max-bits-per-word   : Maximum bits per word; defaults to 8
> > +
>
> If the hardware isn't fixed yet making these enumerable from the
> hardware would be good...

Agreed, but unfortunately this is already in the FU540-C000 chip on
the HiFive Unleashed board sold by SiFive.

> > @@ -0,0 +1,442 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SiFive SPI controller driver (master mode only)
> > + *
>
> Please make the entire comment a C++ one to make this look more
> intentinal.

Will do.

> > +/* for consistency we need this symbol */
> > +#ifdef REG_FMT
> > +#undef REG_FMT
> > +#endif
>
> We do?  For consistency with what?

Below all the register offsets are defined as
REG_<register name>. This is is a pattern I
copied from other drivers, but here we have a
register called "fmt" - hence REG_FMT.
If you have a better pattern that doesn't clash
with REG_FMT please let me know.

> > +static void sifive_spi_init(struct sifive_spi *spi)
> > +{
>
> > +     /* Set CS/SCK Delays and Inactive Time to defaults */
> > +
> > +     /* Exit specialized memory-mapped SPI flash mode */
>
> ...or not?

Right. Will add that or just delete the comment.

> > +     /* Set frame format */
> > +     cr = FMT_LEN(t->bits_per_word);
> > +     switch (mode) {
> > +     case SPI_NBITS_QUAD:
> > +             cr |= FMT_PROTO_QUAD;
> > +             break;
>
> Some namespacing on the driver #defines would be a bit safer against the
> possibility of collision with future changes in headers.

Right.

> > +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> > +{
> > +     if (poll) {
> > +             u32 cr;
> > +             do cr = sifive_spi_read(spi, REG_IP);
> > +             while (!(cr & bit));
>
> Please add some braces, indentation or something to make it more clear
> that the read is part of a do/while loop - right now it's not
> immediately obvious that this is correct.

Good point. Will do.

> > +static int sifive_spi_transfer_one(struct spi_master *master,
> > +             struct spi_device *device, struct spi_transfer *t)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(master);
> > +     int poll = sifive_spi_prep_transfer(spi, device, t);
> > +
> > +     sifive_spi_execute(spi, t, poll);
> > +
>
> Why not just inline the execute function here?  It's the only caller
> AFAICT.

Yeah, it is. Will do.

> > +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(device->master);
> > +
> > +     /* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> > +     if (device->mode & SPI_CS_HIGH)
> > +             is_high = !is_high;
>
> spi_set_cs() will handle CS_HIGH for you.
>
> > +     master->bits_per_word_mask = SPI_BPW_MASK(8);
>
> I thought the device supported other bits per word values?

It does, but the driver doesn't yet. When bits per word is < 8
we need to shift the bits in each byte to be "left-aligned"
(unless SPI_LSB_FIRST is set).

> > +     /* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> > +      * Probably it should rely on the SPI core auto mapping instead.
> > +      */
> > +     pdev->dev.dma_mask = NULL;
>
> If this is a problem please fix it in the MMC core, don't bodge it like
> this.

Gotcha. Will remove this.

> > +static const struct of_device_id sifive_spi_of_match[] = {
> > +     { .compatible = "sifive,spi0", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);
>
> spi0 is a *weird* compatible name.

Exactly. Hence the discussion about the compatible strings.
Once this discussion comes to a conclusion I'll update this.

Thank you for the review!
/Emil

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-13 19:48   ` kernel
  2018-11-13 19:48     ` Emil Renner Berthing
@ 2018-11-13 22:38     ` broonie
  2018-11-13 22:38       ` Mark Brown
  1 sibling, 1 reply; 10+ messages in thread
From: broonie @ 2018-11-13 22:38 UTC (permalink / raw)
  To: linux-riscv

On Tue, Nov 13, 2018 at 08:48:43PM +0100, Emil Renner Berthing wrote:
> On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:

> > > I know the discussions about the sifive devicetree compatible
> > > strings haven't come to a conclusion, so I'm sending this as
> > > an RFC to get some feedback on the rest of the code.

> > I've not seen any of these discussions or earlier versions of this
> > driver so I've no idea what's going on here :(

> No, sorry. This has been discussed on linux-riscv for other drivers
> like the uart. See my last answer.

> > > +Optional properties:
> > > +- sifive,fifo-depth          : Depth of hardware queues; defaults to 8
> > > +- sifive,max-bits-per-word   : Maximum bits per word; defaults to 8

> > If the hardware isn't fixed yet making these enumerable from the
> > hardware would be good...

> Agreed, but unfortunately this is already in the FU540-C000 chip on
> the HiFive Unleashed board sold by SiFive.

Pick an unused register you can read safely and define that value to the
default :)

> > > +/* for consistency we need this symbol */
> > > +#ifdef REG_FMT
> > > +#undef REG_FMT
> > > +#endif

> > We do?  For consistency with what?

> Below all the register offsets are defined as
> REG_<register name>. This is is a pattern I
> copied from other drivers, but here we have a
> register called "fmt" - hence REG_FMT.
> If you have a better pattern that doesn't clash
> with REG_FMT please let me know.

You shouldn't be using such generic names for your internal identifiers,
add a prefix to everything.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20181113/0987ee5a/attachment.sig>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-13 22:38     ` broonie
@ 2018-11-13 22:38       ` Mark Brown
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2018-11-13 22:38 UTC (permalink / raw)
  To: Emil Renner Berthing
  Cc: Mark Rutland, devicetree, Palmer Dabbelt,
	Linux Kernel Mailing List, linux-spi, Rob Herring, linux-riscv

[-- Attachment #1.1: Type: text/plain, Size: 1643 bytes --]

On Tue, Nov 13, 2018 at 08:48:43PM +0100, Emil Renner Berthing wrote:
> On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:

> > > I know the discussions about the sifive devicetree compatible
> > > strings haven't come to a conclusion, so I'm sending this as
> > > an RFC to get some feedback on the rest of the code.

> > I've not seen any of these discussions or earlier versions of this
> > driver so I've no idea what's going on here :(

> No, sorry. This has been discussed on linux-riscv for other drivers
> like the uart. See my last answer.

> > > +Optional properties:
> > > +- sifive,fifo-depth          : Depth of hardware queues; defaults to 8
> > > +- sifive,max-bits-per-word   : Maximum bits per word; defaults to 8

> > If the hardware isn't fixed yet making these enumerable from the
> > hardware would be good...

> Agreed, but unfortunately this is already in the FU540-C000 chip on
> the HiFive Unleashed board sold by SiFive.

Pick an unused register you can read safely and define that value to the
default :)

> > > +/* for consistency we need this symbol */
> > > +#ifdef REG_FMT
> > > +#undef REG_FMT
> > > +#endif

> > We do?  For consistency with what?

> Below all the register offsets are defined as
> REG_<register name>. This is is a pattern I
> copied from other drivers, but here we have a
> register called "fmt" - hence REG_FMT.
> If you have a better pattern that doesn't clash
> with REG_FMT please let me know.

You shouldn't be using such generic names for your internal identifiers,
add a prefix to everything.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] spi: add driver for the SiFive SPI controller
  2018-11-12 14:27 [RFC PATCH] spi: add driver for the SiFive SPI controller kernel
  2018-11-12 14:27 ` Emil Renner Berthing
  2018-11-13 18:35 ` broonie
@ 2019-02-13 10:03 ` Yash Shah
  2019-02-13 13:10   ` Emil Renner Berthing
  2 siblings, 1 reply; 10+ messages in thread
From: Yash Shah @ 2019-02-13 10:03 UTC (permalink / raw)
  To: kernel; +Cc: sachin.ghadi, linux-riscv, palmer, Yash Shah, paul.walmsley

Hi Emil,

I am working full time on Linux device drivers for SiFive.
I have some bandwidth to work on this SPI driver.
Do you mind if I take over this SPI driver from here on?

Thanks,
Yash Shah

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] spi: add driver for the SiFive SPI controller
  2019-02-13 10:03 ` Yash Shah
@ 2019-02-13 13:10   ` Emil Renner Berthing
  0 siblings, 0 replies; 10+ messages in thread
From: Emil Renner Berthing @ 2019-02-13 13:10 UTC (permalink / raw)
  To: Yash Shah; +Cc: sachin.ghadi, linux-riscv, Palmer Dabbelt, paul.walmsley

Hi Yash,

On Wed, 13 Feb 2019 at 11:04, Yash Shah <yash.shah@sifive.com> wrote:
> I am working full time on Linux device drivers for SiFive.
> I have some bandwidth to work on this SPI driver.
> Do you mind if I take over this SPI driver from here on?

No, that's fine. You can pick it up from here:
https://github.com/esmil/linux/commit/870b04e85662a02ee1f6333e1d037c774ed4350e

That's with all of Mark's comments fixed, but the bindings doc
probably needs to be updated and split into a separate patch.

/Emil

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-12 14:27 [RFC PATCH] spi: add driver for the SiFive SPI controller kernel
2018-11-12 14:27 ` Emil Renner Berthing
2018-11-13 18:35 ` broonie
2018-11-13 18:35   ` Mark Brown
2018-11-13 19:48   ` kernel
2018-11-13 19:48     ` Emil Renner Berthing
2018-11-13 22:38     ` broonie
2018-11-13 22:38       ` Mark Brown
2019-02-13 10:03 ` Yash Shah
2019-02-13 13:10   ` Emil Renner Berthing

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org infradead-linux-riscv@archiver.kernel.org
	public-inbox-index linux-riscv


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox