[PATCH v2 0/3] spi: support the SpacemiT K1 SPI controller

[PATCH v2 0/3] spi: support the SpacemiT K1 SPI controller

[Alex Elder]

This series adds support for the SPI controller found in the SpacemiT
K1 SoC.  The driver currently supports only master mode.  The controller
has two 32-entry FIFOs and supports PIO and DMA for transfers.

Version 2 incorporates changes suggested during review of v1.

					-Alex

This series is available here:
  https://github.com/riscstar/linux/tree/outgoing/spi-v2

Between version 1 and version 2:
  - Use enum rather than const for the binding compatible string
  - Omit the label and status property in the binding example
  - The spi-spacemit-k1.o make target is now added in sorted order
  - The SPI_SPACEMIT_K1 config option is added in sorted order
  - The SPI_SPACEMIT_K1 config does *not* depend on MMP_PDMA,
    however MMP_PDMA is checked at runtime, and if not enabled,
    DMA will not be used
  - Read/modify/writes of registers no longer use an additional
    "virt" variable to hold the address accessed
  - The k1_spi_driver_data->ioaddr field has been renamed base
  - The DMA address for the base address is maintained, rather than
    saving the DMA address of the data register
  - The spi-max-frequency property value is now bounds checked
  - A local variable is now initialized to 0 in k1_spi_write_word()
  - The driver name is now "k1-spi"
  - DT aliases are used rather than spacemit,k1-ssp-id for bus number
  - The order of two pin control properties was changed as requested
  - Clock names and DMA names are now on one line in the "k1.dtsi"
  - The interrupts property is used rather than interrupts-extended
  - The order of two pin control properties was changed as requested
  - Clock names and DMA names are now on one line in the "k1.dtsi"
  - The interrupts property is used rather than interrupts-extended

Here is version 1 of this series:
  https://lore.kernel.org/lkml/20250917220724.288127-1-elder@riscstar.com/

Alex Elder (3):
  dt-bindings: spi: add SpacemiT K1 SPI support
  spi: spacemit: introduce SpacemiT K1 SPI controller driver
  riscv: dts: spacemit: define a SPI controller node

 .../bindings/spi/spacemit,k1-spi.yaml         |  87 ++
 .../boot/dts/spacemit/k1-bananapi-f3.dts      |   7 +
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  |  20 +
 arch/riscv/boot/dts/spacemit/k1.dtsi          |  16 +
 drivers/spi/Kconfig                           |   8 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-spacemit-k1.c                 | 968 ++++++++++++++++++
 7 files changed, 1107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml
 create mode 100644 drivers/spi/spi-spacemit-k1.c


base-commit: 846bd2225ec3cfa8be046655e02b9457ed41973e
-- 
2.48.1

[PATCH v2 1/3] dt-bindings: spi: add SpacemiT K1 SPI support

[Alex Elder]

Add support for the SPI controller implemented by the SpacemiT K1 SoC.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Use enum rather than const for the binding compatible string
    - Omit the label and status property in the binding example
    - No more spacemit,k1-ssp-id property
    - Interrupts no longer specified with interrupts-extended

 .../bindings/spi/spacemit,k1-spi.yaml         | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml

diff --git a/Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml b/Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml
new file mode 100644
index 0000000000000..a6978212ff570
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/spacemit,k1-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 SoC Serial Peripheral Interface (SPI)
+
+maintainers:
+  - Alex Elder <elder@kernel.org>
+
+description:
+  The SpacemiT K1 SoC implements a SPI controller that has two 32-entry
+  FIFOs, for transmit and receive.  Details are currently available in
+  section 18.2.1 of the K1 User Manual, found in the SpacemiT Keystone
+  K1 Documentation[1].  The controller transfers words using PIO.  DMA
+  transfers are supported as well, if both TX and RX DMA channels are
+  specified,
+
+  [1] https://developer.spacemit.com/documentation
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    const: spacemit,k1-spi
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Core clock
+      - description: Bus clock
+
+  clock-names:
+    items:
+      - const: core
+      - const: bus
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  dmas:
+    items:
+      - description: RX DMA channel
+      - description: TX DMA channel
+
+  dma-names:
+    items:
+      - const: rx
+      - const: tx
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/clock/spacemit,k1-syscon.h>
+    spi@d401c000 {
+        compatible = "spacemit,k1-spi";
+        reg = <0xd401c000 0x30>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        clocks = <&syscon_apbc CLK_SSP3>,
+                 <&syscon_apbc CLK_SSP3_BUS>;
+        clock-names = "core",
+                      "bus";
+        resets = <&syscon_apbc RESET_SSP3>;
+        interrupts = <55>;
+        dmas = <&pdma 20>,
+               <&pdma 19>;
+        dma-names = "rx",
+                    "tx";
+    };
-- 
2.48.1

Re: [PATCH v2 1/3] dt-bindings: spi: add SpacemiT K1 SPI support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH v2 1/3] dt-bindings: spi: add SpacemiT K1 SPI support

[Rob Herring (Arm)]

On Fri, 19 Sep 2025 10:59:11 -0500, Alex Elder wrote:
> Add support for the SPI controller implemented by the SpacemiT K1 SoC.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Use enum rather than const for the binding compatible string
>     - Omit the label and status property in the binding example
>     - No more spacemit,k1-ssp-id property
>     - Interrupts no longer specified with interrupts-extended
> 
>  .../bindings/spi/spacemit,k1-spi.yaml         | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spacemit,k1-spi.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Alex Elder]

This patch introduces the driver for the SPI controller found in the
SpacemiT K1 SoC.  Currently the driver supports master mode only.
The SPI hardware implements RX and TX FIFOs, 32 entries each, and
supports both PIO and DMA mode transfers.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - The SPI_SPACEMIT_K1 config option is added in sorted order
    - The spi-spacemit-k1.o make target is now added in sorted order
    - Read/modify/writes of registers no longer use an additional
      "virt" variable to hold the address accessed
    - The k1_spi_driver_data->ioaddr field has been renamed base
    - The DMA address for the base address is maintained, rather than
      saving the DMA address of the data register
    - CONFIG_MMP_PDMA is checked at runtime, and if it is not enabled,
      DMA will not be used
    - The spi-max-frequency property value is now bounds checked
    - A local variable is now initialized to 0 in k1_spi_write_word()
    - The driver name is now "k1-spi"

 drivers/spi/Kconfig           |   8 +
 drivers/spi/Makefile          |   1 +
 drivers/spi/spi-spacemit-k1.c | 968 ++++++++++++++++++++++++++++++++++
 3 files changed, 977 insertions(+)
 create mode 100644 drivers/spi/spi-spacemit-k1.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 82fa5eb3b8684..4f6c446c6bc16 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
 	  also supporting 3Byte address devices and 4Byte address
 	  devices.
 
+config SPI_SPACEMIT_K1
+	tristate "K1 SPI Controller"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on OF
+	default ARCH_SPACEMIT
+	help
+	  Enable support for the SpacemiT K1 SPI controller.
+
 config SPI_SPRD
 	tristate "Spreadtrum SPI controller"
 	depends on ARCH_SPRD || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index eefaeca097456..637d750ead996 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -140,6 +140,7 @@ obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
 obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
 obj-$(CONFIG_SPI_SN_F_OSPI)		+= spi-sn-f-ospi.o
 obj-$(CONFIG_SPI_SG2044_NOR)	+= spi-sg2044-nor.o
+obj-$(CONFIG_SPI_SPACEMIT_K1)		+= spi-spacemit-k1.o
 obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
 obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
 obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
new file mode 100644
index 0000000000000..8d564fe6c4303
--- /dev/null
+++ b/drivers/spi/spi-spacemit-k1.c
@@ -0,0 +1,968 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for SpacemiT K1 SPI controller
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ * Copyright (c) 2023, spacemit Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/scatterlist.h>
+#include <linux/sizes.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#include "internals.h"
+
+/* This is the range of transfer rates supported by the K1 SoC */
+#define K1_SPI_MIN_SPEED_HZ		6250
+#define K1_SPI_MAX_SPEED_HZ		51200000
+
+/* Default speed used if spi-max-frequency is invalid or not present */
+#define K1_SPI_DEFAULT_MAX_SPEED_HZ	25600000
+
+/* DMA constraints */
+#define K1_SPI_DMA_ALIGNMENT	64
+#define K1_SPI_MAX_DMA_LEN	SZ_512K
+
+/* SpacemiT K1 SPI Registers */
+
+/* SSP Top Control Register */
+#define SSP_TOP_CTRL		0x00
+#define TOP_SSE				BIT(0)		/* Enable port */
+#define TOP_FRF_MASK			GENMASK(2, 1)	/* Frame format */
+#define TOP_FRF_MOTOROLA			0	/* Motorola SPI */
+#define TOP_DSS_MASK			GENMASK(9, 5)	/* Data size (1-32) */
+#define TOP_SPO				BIT(10)		/* Polarity: 0=low */
+#define TOP_SPH				BIT(11)		/* Half-cycle phase */
+#define TOP_LBM				BIT(12)		/* Loopback mode */
+#define TOP_TRAIL			BIT(13)		/* Trailing bytes */
+#define TOP_HOLD_FRAME_LOW		BIT(14)		/* Master mode */
+
+/* SSP FIFO Control Register */
+#define SSP_FIFO_CTRL		0x04
+#define FIFO_TFT_MASK			GENMASK(4, 0)	/* TX FIFO threshold */
+#define FIFO_RFT_MASK			GENMASK(9, 5)	/* RX FIFO threshold */
+#define FIFO_TSRE			BIT(10)		/* TX service request */
+#define FIFO_RSRE			BIT(11)		/* RX service request */
+
+/* SSP Interrupt Enable Register */
+#define SSP_INT_EN		0x08
+#define SSP_INT_EN_TINTE		BIT(1)		/* RX timeout */
+#define SSP_INT_EN_RIE			BIT(2)		/* RX FIFO */
+#define SSP_INT_EN_TIE			BIT(3)		/* TX FIFO */
+#define SSP_INT_EN_RIM			BIT(4)		/* RX FIFO overrun */
+#define SSP_INT_EN_TIM			BIT(5)		/* TX FIFO underrun */
+
+/* SSP Time Out Register */
+#define SSP_TIMEOUT		0x0c
+#define SSP_TIMEOUT_MASK		GENMASK(23, 0)
+
+/* SSP Data Register */
+#define SSP_DATAR		0x10
+
+/* SSP Status Register */
+#define SSP_STATUS		0x14
+#define SSP_STATUS_BSY			BIT(0)		/* SPI/I2S busy */
+#define SSP_STATUS_TNF			BIT(6)		/* TX FIFO not full */
+#define SSP_STATUS_TFL			GENMASK(11, 7)	/* TX FIFO level */
+#define SSP_STATUS_TUR			BIT(12)		/* TX FIFO underrun */
+#define SSP_STATUS_RNE			BIT(14)		/* RX FIFO not empty */
+#define SSP_STATUS_RFL			GENMASK(19, 15)	/* RX FIFO level */
+#define SSP_STATUS_ROR			BIT(20)		/* RX FIFO overrun */
+
+/* The FIFO sizes and thresholds are the same for RX and TX */
+#define K1_SPI_FIFO_SIZE	32
+#define K1_SPI_THRESH		(K1_SPI_FIFO_SIZE / 2)
+
+struct k1_spi_io {
+	enum dma_data_direction dir;
+	struct dma_chan *chan;
+	void *buf;
+	unsigned int resid;
+	u32 nents;
+	struct sg_table sgt;
+};
+
+struct k1_spi_driver_data {
+	struct spi_controller *controller;
+	struct device *dev;
+	void __iomem *base;
+	unsigned long bus_rate;
+	struct clk *clk;
+	unsigned long rate;
+	u32 rx_timeout;
+	int irq;
+
+	struct k1_spi_io rx;
+	struct k1_spi_io tx;
+
+	void *dummy;			/* DMA disabled if NULL */
+	u32 base_addr;			/* DMA address corresponding to base */
+
+	struct spi_message *message;	/* Current message */
+
+	/* Current transfer information; not valid if message is null */
+	unsigned int len;
+	u32 bytes;			/* Bytes used for bits_per_word */
+	bool dma_mapped;
+	struct completion completion;	/* Transfer completion */
+};
+
+static bool k1_spi_dma_enabled(struct k1_spi_driver_data *drv_data)
+{
+	return !!drv_data->dummy;
+}
+
+static bool k1_spi_map_dma_buffer(struct k1_spi_io *io, size_t len, void *dummy)
+{
+	struct device *dmadev = io->chan->device->dev;
+	unsigned int nents = DIV_ROUND_UP(len, SZ_2K);
+	struct sg_table *sgt = &io->sgt;
+	void *bufp = io->buf ? : dummy;
+	struct scatterlist *sg;
+	unsigned int i;
+
+	if (nents != sgt->nents) {
+		sg_free_table(sgt);
+		if (sg_alloc_table(sgt, nents, GFP_KERNEL))
+			return false;
+	}
+
+	for_each_sg(sgt->sgl, sg, nents, i) {
+		size_t bytes = min_t(size_t, len, SZ_2K);
+
+		sg_set_buf(sg, bufp, bytes);
+		if (bufp != dummy)
+			bufp += bytes;
+		len -= bytes;
+	}
+	io->nents = dma_map_sg(dmadev, sgt->sgl, nents, io->dir);
+
+	return !!io->nents;
+}
+
+static void k1_spi_unmap_dma_buffer(struct k1_spi_io *io)
+{
+	struct sg_table *sgt = &io->sgt;
+
+	dma_unmap_sg(io->chan->device->dev, sgt->sgl, io->nents, io->dir);
+	io->nents = 0;
+}
+
+static bool k1_spi_map_dma_buffers(struct k1_spi_driver_data *drv_data)
+{
+	u32 dma_burst_size;
+	void *dummy;
+
+	if (!k1_spi_dma_enabled(drv_data))
+		return false;
+
+	dma_burst_size = K1_SPI_THRESH * drv_data->bytes;
+
+	/* Don't bother with DMA if we can't do even a single burst */
+	if (drv_data->len < dma_burst_size)
+		return false;
+
+	/* We won't use DMA if the transfer is too big, either */
+	if (drv_data->len > K1_SPI_MAX_DMA_LEN)
+		return false;
+
+	/* Map both directions for DMA; if either fails, we'll use PIO */
+	dummy = drv_data->dummy;
+	if (!k1_spi_map_dma_buffer(&drv_data->rx, drv_data->len, dummy))
+		return false;
+
+	if (k1_spi_map_dma_buffer(&drv_data->tx, drv_data->len, dummy))
+		return true;		/* Success! */
+
+	/* Failed to map the RX buffer; undo the TX mapping */
+	k1_spi_unmap_dma_buffer(&drv_data->rx);
+
+	return false;
+}
+
+static struct dma_async_tx_descriptor *
+k1_spi_prepare_dma_io(struct k1_spi_driver_data *drv_data, struct k1_spi_io *io)
+{
+	u32 addr = drv_data->base_addr + SSP_DATAR;
+	struct dma_slave_config cfg = { };
+	enum dma_transfer_direction dir;
+	enum dma_slave_buswidth width;
+	u32 dma_burst_size;
+	int ret;
+
+	dir = io->dir == DMA_TO_DEVICE ? DMA_MEM_TO_DEV
+				       : DMA_DEV_TO_MEM;
+
+	width = drv_data->bytes == 1 ? DMA_SLAVE_BUSWIDTH_1_BYTE :
+		drv_data->bytes == 2 ? DMA_SLAVE_BUSWIDTH_2_BYTES
+				     : DMA_SLAVE_BUSWIDTH_4_BYTES;
+
+	dma_burst_size = K1_SPI_THRESH * drv_data->bytes;
+
+	cfg.direction = dir;
+	if (dir == DMA_MEM_TO_DEV) {
+		cfg.dst_addr = addr;
+		cfg.dst_addr_width = width;
+		cfg.dst_maxburst = dma_burst_size;
+	} else {
+		cfg.src_addr = addr;
+		cfg.src_addr_width = width;
+		cfg.src_maxburst = dma_burst_size;
+	}
+
+	ret = dmaengine_slave_config(io->chan, &cfg);
+	if (ret)
+		return NULL;
+
+	return dmaengine_prep_slave_sg(io->chan, io->sgt.sgl, io->nents, dir,
+				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+}
+
+/* DMA completion callback */
+static void k1_spi_callback(void *data)
+{
+	complete(data);
+}
+
+static bool k1_spi_transfer_start_dma(struct k1_spi_driver_data *drv_data)
+{
+	struct dma_async_tx_descriptor *rx_desc;
+	struct dma_async_tx_descriptor *tx_desc;
+	struct device *dev = drv_data->dev;
+	u32 val;
+
+	rx_desc = k1_spi_prepare_dma_io(drv_data, &drv_data->rx);
+	if (!rx_desc) {
+		dev_err(dev, "failed to get DMA RX descriptor\n");
+		return false;
+	}
+
+	tx_desc = k1_spi_prepare_dma_io(drv_data, &drv_data->tx);
+	if (!tx_desc) {
+		dev_err(dev, "failed to get DMA TX descriptor\n");
+		return false;
+	}
+
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val |= TOP_TRAIL;	/* Trailing bytes handled by DMA */
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	val = readl(drv_data->base + SSP_FIFO_CTRL);
+	val |= FIFO_TSRE | FIFO_RSRE;
+	writel(val, drv_data->base + SSP_FIFO_CTRL);
+
+	/* When RX is complete we also know TX has completed */
+	rx_desc->callback = k1_spi_callback;
+	rx_desc->callback_param = &drv_data->completion;
+
+	dmaengine_submit(rx_desc);
+	dmaengine_submit(tx_desc);
+
+	dma_async_issue_pending(drv_data->rx.chan);
+	dma_async_issue_pending(drv_data->tx.chan);
+
+	return true;
+}
+
+static void k1_spi_transfer_end_dma(struct k1_spi_driver_data *drv_data)
+{
+	u32 val;
+
+	val = readl(drv_data->base + SSP_FIFO_CTRL);
+	val &= ~(FIFO_TSRE | FIFO_RSRE);
+	writel(val, drv_data->base + SSP_FIFO_CTRL);
+
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val &= ~TOP_TRAIL;		/* Trailing bytes handled by the CPU */
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	/* Signal an error if an RX overflow or TX underflow occurred */
+	val = readl(drv_data->base + SSP_STATUS);
+	if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR))
+		drv_data->message->status = -EIO;
+}
+
+/* Discard any data in the RX FIFO */
+static void k1_spi_flush(struct k1_spi_driver_data *drv_data)
+{
+	u32 val = readl(drv_data->base + SSP_STATUS);
+	u32 count;
+
+	/* If there's nothing in the FIFO, we're done */
+	if (!(val & SSP_STATUS_RNE))
+		return;
+
+	/* Read and discard what's there (one more than what the field says) */
+	count = FIELD_GET(SSP_STATUS_RFL, val) + 1;
+	do
+		(void)readl(drv_data->base + SSP_DATAR);
+	while (--count);
+}
+
+/* Set the transfer speed; the SPI core code ensures it is supported */
+static bool k1_spi_set_speed(struct k1_spi_driver_data *drv_data, u32 rate)
+{
+	struct clk *clk = drv_data->clk;
+	u64 nsec_per_word;
+	u64 bus_ticks;
+
+	if (clk_set_rate(clk, rate))
+		return false;
+
+	drv_data->rate = clk_get_rate(clk);
+
+	/*
+	 * Compute the RX FIFO inactivity timeout value that should be used.
+	 * The inactivity timer restarts with each word that lands in the
+	 * FIFO.  If two or more "word transfer times" pass without any new
+	 * data in the RX FIFO, we might as well read what's there.
+	 *
+	 * The rate at which words land in the FIFO is determined by the
+	 * word size and the transfer rate.  One bit is transferred per
+	 * clock tick, and 8 (or 16 or 32) bits are transferred per word.
+	 *
+	 * So we can get word transfer time (in nanoseconds) from:
+	 *   nsec_per_tick = NANOHZ_PER_HZ / drv_data->rate;
+	 *   ticks_per_word = BITS_PER_BYTE * drv_data->bytes;
+	 * We do the divide last for better accuracy.
+	 */
+	nsec_per_word = NANOHZ_PER_HZ * BITS_PER_BYTE * drv_data->bytes;
+	nsec_per_word = DIV_ROUND_UP(nsec_per_word, drv_data->rate);
+
+	/*
+	 * The timeout (which we'll set to three word transfer times) is
+	 * expressed as a number of APB clock ticks.
+	 *   bus_ticks = 3 * nsec * (drv_data->bus_rate / NANOHZ_PER_HZ)
+	 */
+	bus_ticks = 3 * nsec_per_word * drv_data->bus_rate;
+	drv_data->rx_timeout = DIV_ROUND_UP(bus_ticks, NANOHZ_PER_HZ);
+
+	return true;
+}
+
+static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
+{
+	struct k1_spi_io *rx = &drv_data->rx;
+	u32 bytes = drv_data->bytes;
+	u32 val;
+
+	val = readl(drv_data->base + SSP_DATAR);
+	rx->resid -= bytes;
+
+	if (!rx->buf)
+		return;	/* Null reader: discard the data */
+
+	if (bytes == 1)
+		*(u8 *)rx->buf = val;
+	else if (bytes == 1)
+		*(u16 *)rx->buf = val;
+	else
+		*(u32 *)rx->buf = val;
+
+	rx->buf += bytes;
+}
+
+static bool k1_spi_read(struct k1_spi_driver_data *drv_data)
+{
+	struct k1_spi_io *rx = &drv_data->rx;
+	unsigned int count;
+	u32 val;
+
+	if (!rx->resid)
+		return true;	/* Nothing more to receive */
+
+	/* We'll read as many slots in the FIFO as there are available */
+	val = readl(drv_data->base + SSP_STATUS);
+	/* The number of open slots is one more than what's in the field */
+	count = FIELD_GET(SSP_STATUS_RFL, val) + 1;
+
+	/* A full FIFO count means the FIFO is either full or empty */
+
+	if (count == K1_SPI_FIFO_SIZE)
+		if (!(val & SSP_STATUS_RNE))
+			return false;	/* Nothing available to read */
+
+	count = min(count, rx->resid);
+	while (count--)
+		k1_spi_read_word(drv_data);
+
+	return !rx->resid;
+}
+
+static void k1_spi_write_word(struct k1_spi_driver_data *drv_data)
+{
+	struct k1_spi_io *tx = &drv_data->tx;
+	u32 val = 0;
+	u32 bytes;
+
+	bytes = drv_data->bytes;
+	if (tx->buf) {
+		if (bytes == 1)
+			val = *(u8 *)tx->buf;
+		else if (bytes == 2)
+			val = *(u16 *)tx->buf;
+		else if (bytes == 4)
+			val = *(u32 *)tx->buf;
+		tx->buf += bytes;
+	} /* Otherwise null writer; write 1, 2, or 4 zero bytes */
+
+	tx->resid -= bytes;
+	writel(val, drv_data->base + SSP_DATAR);
+}
+
+static bool k1_spi_write(struct k1_spi_driver_data *drv_data)
+{
+	struct k1_spi_io *tx = &drv_data->tx;
+	unsigned int count;
+	u32 val;
+
+	if (!tx->resid)
+		return true;	/* Nothing more to send */
+
+	/* See how many slots in the TX FIFO are available */
+	val = readl(drv_data->base + SSP_STATUS);
+	count = FIELD_GET(SSP_STATUS_TFL, val);
+
+	/* A zero count means the FIFO is either full or empty */
+	if (!count) {
+		if (val & SSP_STATUS_TNF)
+			count = K1_SPI_FIFO_SIZE;
+		else
+			return false;	/* No room in the FIFO */
+	}
+
+	/*
+	 * Limit how much we try to send at a time, to reduce the
+	 * chance the other side can overrun our RX FIFO.
+	 */
+	count = min3(count, K1_SPI_THRESH, tx->resid);
+	while (count--)
+		k1_spi_write_word(drv_data);
+
+	return !tx->resid;
+}
+
+static bool k1_spi_transfer_start(struct k1_spi_driver_data *drv_data,
+				  struct spi_transfer *transfer)
+{
+	u32 val;
+
+	/* Bits per word can change on a per-transfer basis */
+	drv_data->bytes = spi_bpw_to_bytes(transfer->bits_per_word);
+
+	/* Each transfer can also specify a different rate */
+	if (!k1_spi_set_speed(drv_data, transfer->speed_hz)) {
+		dev_err(drv_data->dev, "failed to set transfer speed\n");
+		return false;
+	}
+
+	k1_spi_flush(drv_data);
+
+	/* Record the current transfer information */
+	drv_data->rx.buf = transfer->rx_buf;
+	drv_data->rx.resid = transfer->len;
+	drv_data->tx.buf = (void *)transfer->tx_buf;
+	drv_data->tx.resid = transfer->len;
+	drv_data->len = transfer->len;
+
+	drv_data->dma_mapped = k1_spi_map_dma_buffers(drv_data);
+
+	/* Set the RX timeout period (required for both DMA and PIO) */
+	val = FIELD_PREP(SSP_TIMEOUT_MASK, drv_data->rx_timeout);
+	writel(val, drv_data->base + SSP_TIMEOUT);
+
+	/* Clear any existing interrupt conditions */
+	val = readl(drv_data->base + SSP_STATUS);
+	writel(val, drv_data->base + SSP_STATUS);
+
+	/* Set the data size and enable the hardware */
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val |= FIELD_PREP(TOP_DSS_MASK, transfer->bits_per_word - 1);
+	val |= TOP_SSE;
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	/* DMA transfers are programmmed, then initiated */
+	if (drv_data->dma_mapped)
+		return k1_spi_transfer_start_dma(drv_data);
+
+	/*
+	 * For PIO transfers, interrupts will cause words to get
+	 * transferred.  The interrupts will get disabled as the
+	 * transfer completes.  We'll write what we can to get
+	 * things started.
+	 */
+	(void)k1_spi_write(drv_data);
+
+	val = SSP_INT_EN_RIM | SSP_INT_EN_TIM;
+	val |= SSP_INT_EN_TINTE | SSP_INT_EN_RIE | SSP_INT_EN_TIE;
+	writel(val, drv_data->base + SSP_INT_EN);
+
+	return true;
+}
+
+static void k1_spi_transfer_end(struct k1_spi_driver_data *drv_data,
+				struct spi_transfer *transfer)
+{
+	struct spi_message *message = drv_data->message;
+	u32 val;
+
+	if (drv_data->dma_mapped)
+		k1_spi_transfer_end_dma(drv_data);
+
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val &= ~TOP_SSE;
+	val &= ~TOP_DSS_MASK;
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	writel(0, drv_data->base + SSP_TIMEOUT);
+
+	if (drv_data->dma_mapped) {
+		k1_spi_unmap_dma_buffer(&drv_data->tx);
+		k1_spi_unmap_dma_buffer(&drv_data->rx);
+	}
+
+	spi_transfer_delay_exec(transfer);
+
+	if (!message->status)
+		message->actual_length += drv_data->len;
+}
+
+static void k1_spi_transfer_wait(struct k1_spi_driver_data *drv_data)
+{
+	struct completion *completion = &drv_data->completion;
+	struct spi_message *message = drv_data->message;
+	unsigned long timeout;
+	int ret;
+
+	/* Length in bits to be transferred */
+	timeout = BITS_PER_BYTE * drv_data->bytes * drv_data->len;
+	/* Time (usec) to transfer that many bits at the current bit rate */
+	timeout = DIV_ROUND_UP(timeout * MICROHZ_PER_HZ, drv_data->rate);
+	/* Convert that (+ 25%) to jiffies for the wait call */
+	timeout = usecs_to_jiffies(5 * timeout / 4);
+
+	ret = wait_for_completion_interruptible_timeout(completion, timeout);
+	if (ret > 0)
+		return;
+
+	message->status = -EIO;
+	if (ret && drv_data->dma_mapped) {
+		dmaengine_terminate_sync(drv_data->tx.chan);
+		dmaengine_terminate_sync(drv_data->rx.chan);
+	}
+}
+
+static int k1_spi_transfer_one_message(struct spi_controller *host,
+					   struct spi_message *message)
+{
+	struct k1_spi_driver_data *drv_data = spi_controller_get_devdata(host);
+	struct completion *completion = &drv_data->completion;
+	struct spi_transfer *transfer;
+	u32 val;
+
+	drv_data->message = message;
+
+	/* Message status starts out successful; set to -EIO on error */
+	message->status = 0;
+
+	/* Hold frame low to avoid losing transferred data */
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val |= TOP_HOLD_FRAME_LOW;
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	list_for_each_entry(transfer, &message->transfers, transfer_list) {
+		reinit_completion(completion);
+
+		/* Issue the next transfer */
+		if (!k1_spi_transfer_start(drv_data, transfer)) {
+			message->status = -EIO;
+			break;
+		}
+
+		k1_spi_transfer_wait(drv_data);
+
+		k1_spi_transfer_end(drv_data, transfer);
+
+		/* If an error has occurred, we're done */
+		if (message->status)
+			break;
+	}
+
+	drv_data->message = NULL;
+
+	spi_finalize_current_message(drv_data->controller);
+
+	val = readl(drv_data->base + SSP_TOP_CTRL);
+	val &= ~TOP_HOLD_FRAME_LOW;
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	return 0;
+}
+
+/*
+ * The client can call the setup function multiple times, and each call
+ * can specify a different SPI mode (and transfer speed).  Each transfer
+ * can specify its own speed though, and the core code ensures each
+ * transfer's speed is set to something nonzero and supported by both
+ * the controller and the device).  We just set the speed for each
+ * transfer.
+ */
+static int k1_spi_setup(struct spi_device *spi)
+{
+	struct k1_spi_driver_data *drv_data;
+	u32 val;
+
+	drv_data = spi_controller_get_devdata(spi->controller);
+
+	/*
+	 * Configure the message format for this device.  We only
+	 * support Motorola SPI format in master mode.
+	 */
+	val = FIELD_PREP(TOP_FRF_MASK, TOP_FRF_MOTOROLA);
+	val |= TOP_HOLD_FRAME_LOW;	/* Master mode */
+
+	/* Translate the mode into the value used to program the hardware. */
+	if (spi->mode & SPI_CPHA)
+		val |= TOP_SPH;		/* 1/2 cycle */
+	if (spi->mode & SPI_CPOL)
+		val |= TOP_SPO;		/* active low */
+	if (spi->mode & SPI_LOOP)
+		val |= TOP_LBM;		/* enable loopback */
+	writel(val, drv_data->base + SSP_TOP_CTRL);
+
+	return 0;
+}
+
+static void k1_spi_cleanup(struct spi_device *spi)
+{
+	struct k1_spi_driver_data *drv_data;
+
+	drv_data = spi_controller_get_devdata(spi->controller);
+
+	writel(0, drv_data->base + SSP_TOP_CTRL);
+}
+
+static int k1_spi_dma_setup_io(struct k1_spi_driver_data *drv_data, bool rx)
+{
+	const char *name = rx ? "rx" : "tx";
+	struct device *dev = drv_data->dev;
+	struct dma_chan *chan;
+	struct k1_spi_io *io;
+
+	chan = devm_dma_request_chan(dev, name);
+	if (IS_ERR(chan))
+		return PTR_ERR(chan);
+
+	io = rx ? &drv_data->rx : &drv_data->tx;
+	io->dir = rx ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	io->chan = chan;
+
+	return 0;
+}
+
+static void k1_spi_dma_cleanup_io(struct k1_spi_driver_data *drv_data, bool rx)
+{
+	struct k1_spi_io *io = rx ? &drv_data->rx : &drv_data->tx;
+
+	dmaengine_terminate_sync(io->chan);
+	sg_free_table(&io->sgt);
+}
+
+static int k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
+{
+	struct device *dev = drv_data->dev;
+	int rx_ret;
+	int tx_ret;
+
+	/* We must get both DMA channels, or neither of them */
+	rx_ret = k1_spi_dma_setup_io(drv_data, true);
+	if (rx_ret == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	tx_ret = k1_spi_dma_setup_io(drv_data, false);
+
+	/* If neither is specified, we don't use DMA */
+	if (rx_ret == -ENODEV && tx_ret == -ENODEV)
+		return 0;
+
+	if (rx_ret || tx_ret)
+		goto err_cleanup;
+
+	drv_data->dummy = devm_kzalloc(dev, SZ_2K, GFP_KERNEL);
+	if (drv_data->dummy)
+		return 0;		/* Success! */
+
+	dev_warn(dev, "error allocating DMA dummy buffer; DMA disabled\n");
+err_cleanup:
+	if (tx_ret) {
+		if (tx_ret != -EPROBE_DEFER)
+			dev_err(dev, "error requesting DMA TX DMA channel\n");
+	} else {
+		k1_spi_dma_cleanup_io(drv_data, false);
+	}
+
+	if (rx_ret)
+		dev_err(dev, "error requesting DMA RX DMA channel\n");
+	else
+		k1_spi_dma_cleanup_io(drv_data, true);
+
+	if (tx_ret == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	/* Return success if we don't get the dummy buffer; PIO will be used */
+
+	return rx_ret ? : tx_ret ? : 0;
+}
+
+static void k1_spi_dma_cleanup(struct device *dev, void *res)
+{
+	struct k1_spi_driver_data *drv_data = res;
+
+	if (k1_spi_dma_enabled(drv_data)) {
+		k1_spi_dma_cleanup_io(drv_data, false);
+		k1_spi_dma_cleanup_io(drv_data, true);
+	}
+}
+
+static int devm_k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
+{
+	struct k1_spi_driver_data **ptr;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_MMP_PDMA)) {
+		dev_warn(drv_data->dev, "DMA not available; using PIO\n");
+		return 0;
+	}
+
+	ptr = devres_alloc(k1_spi_dma_cleanup, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = k1_spi_dma_setup(drv_data);
+	if (ret) {
+		devres_free(ptr);
+		return ret;
+	}
+
+	*ptr = drv_data;
+	devres_add(drv_data->dev, ptr);
+
+	return 0;
+}
+
+static const struct of_device_id k1_spi_dt_ids[] = {
+	{ .compatible = "spacemit,k1-spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, k1_spi_dt_ids);
+
+static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
+{
+	struct device_node *np = dev_of_node(drv_data->dev);
+	struct spi_controller *host = drv_data->controller;
+	struct device *dev = drv_data->dev;
+	u32 max_speed_hz;
+	int ret;
+
+	host->dev.of_node = np;
+	host->dev.parent = drv_data->dev;
+	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
+	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+	host->num_chipselect = 1;
+
+	if (k1_spi_dma_enabled(drv_data))
+		host->dma_alignment = K1_SPI_DMA_ALIGNMENT;
+	host->cleanup = k1_spi_cleanup;
+	host->setup = k1_spi_setup;
+	host->transfer_one_message = k1_spi_transfer_one_message;
+
+	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
+	if (!ret) {
+		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
+					   K1_SPI_MAX_SPEED_HZ);
+		if (host->max_speed_hz != max_speed_hz)
+			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
+				max_speed_hz, host->max_speed_hz);
+	} else {
+		if (ret != -EINVAL)
+			dev_warn(dev, "bad spi-max-frequency, using %u\n",
+				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
+		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
+	}
+}
+
+/* Set our registers to a known initial state */
+static void
+k1_spi_register_reset(struct k1_spi_driver_data *drv_data, bool initial)
+{
+	u32 val = 0;
+
+	writel(0, drv_data->base + SSP_TOP_CTRL);
+
+	if (initial) {
+		/*
+		 * The TX and RX FIFO thresholds are the same no matter
+		 * what the speed or bits per word, so we can just set
+		 * them once.  The thresholds are one more than the values
+		 * in the register.
+		 */
+		val = FIELD_PREP(FIFO_RFT_MASK, K1_SPI_THRESH - 1);
+		val |= FIELD_PREP(FIFO_TFT_MASK, K1_SPI_THRESH - 1);
+	}
+	writel(val, drv_data->base + SSP_FIFO_CTRL);
+
+	writel(0, drv_data->base + SSP_INT_EN);
+	writel(0, drv_data->base + SSP_TIMEOUT);
+
+	/* Clear any pending interrupt conditions */
+	val = readl(drv_data->base + SSP_STATUS);
+	writel(val, drv_data->base + SSP_STATUS);
+}
+
+static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
+{
+	struct k1_spi_driver_data *drv_data = dev_id;
+	bool rx_done;
+	bool tx_done;
+	u32 val;
+
+	/* Get status and clear pending interrupts */
+	val = readl(drv_data->base + SSP_STATUS);
+	writel(val, drv_data->base + SSP_STATUS);
+
+	if (!drv_data->message)
+		return IRQ_NONE;
+
+	/* Check for a TX underrun or RX underrun first */
+	if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR)) {
+		/* Disable all interrupts on error */
+		writel(0, drv_data->base + SSP_INT_EN);
+
+		drv_data->message->status = -EIO;
+		complete(&drv_data->completion);
+
+		return IRQ_HANDLED;
+	}
+
+	/* Drain the RX FIFO first, then transmit what we can */
+	rx_done = k1_spi_read(drv_data);
+	tx_done = k1_spi_write(drv_data);
+
+	/* Disable interrupts if we're done transferring either direction */
+	if (rx_done || tx_done) {
+		/* If both are done, disable all interrupts */
+		if (rx_done && tx_done) {
+			val = 0;
+		} else {
+			val = readl(drv_data->base + SSP_INT_EN);
+			if (rx_done)
+				val &= ~(SSP_INT_EN_TINTE | SSP_INT_EN_RIE);
+			if (tx_done)
+				val &= ~SSP_INT_EN_TIE;
+		}
+		writel(val, drv_data->base + SSP_INT_EN);
+	}
+
+	if (rx_done && tx_done)
+		complete(&drv_data->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int k1_spi_probe(struct platform_device *pdev)
+{
+	struct k1_spi_driver_data *drv_data;
+	struct device *dev = &pdev->dev;
+	struct reset_control *reset;
+	struct spi_controller *host;
+	struct resource *iores;
+	struct clk *clk_bus;
+	int ret;
+
+	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
+	if (!host)
+		return -ENOMEM;
+	drv_data = spi_controller_get_devdata(host);
+	drv_data->controller = host;
+	platform_set_drvdata(pdev, drv_data);
+	drv_data->dev = dev;
+	init_completion(&drv_data->completion);
+
+	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
+								&iores);
+	if (IS_ERR(drv_data->base))
+		return dev_err_probe(dev, PTR_ERR(drv_data->base),
+				     "error mapping memory\n");
+	drv_data->base_addr = iores->start;
+
+	ret = devm_k1_spi_dma_setup(drv_data);
+	if (ret)
+		return dev_err_probe(dev, ret, "error setting up DMA\n");
+
+	k1_spi_host_init(drv_data);
+
+	clk_bus = devm_clk_get_enabled(dev, "bus");
+	if (IS_ERR(clk_bus))
+		return dev_err_probe(dev, PTR_ERR(clk_bus),
+				     "error getting/enabling bus clock\n");
+	drv_data->bus_rate = clk_get_rate(clk_bus);
+
+	drv_data->clk = devm_clk_get_enabled(dev, "core");
+	if (IS_ERR(drv_data->clk))
+		return dev_err_probe(dev, PTR_ERR(drv_data->clk),
+				     "error getting/enabling core clock\n");
+
+	reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset),
+				     "error getting/deasserting reset\n");
+
+	k1_spi_register_reset(drv_data, true);
+
+	drv_data->irq = platform_get_irq(pdev, 0);
+	if (drv_data->irq < 0)
+		return dev_err_probe(dev, drv_data->irq, "error getting IRQ\n");
+
+	ret = devm_request_irq(dev, drv_data->irq, k1_spi_ssp_isr,
+			       IRQF_SHARED, dev_name(dev), drv_data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "error requesting IRQ\n");
+
+	ret = devm_spi_register_controller(dev, host);
+	if (ret)
+		dev_err(dev, "error registering controller\n");
+
+	return ret;
+}
+
+static void k1_spi_remove(struct platform_device *pdev)
+{
+	struct k1_spi_driver_data *drv_data = platform_get_drvdata(pdev);
+
+	k1_spi_register_reset(drv_data, false);
+}
+
+static struct platform_driver k1_spi_driver = {
+	.driver = {
+		.name		= "k1-spi",
+		.of_match_table	= k1_spi_dt_ids,
+	},
+	.probe			= k1_spi_probe,
+	.remove			= k1_spi_remove,
+};
+
+module_platform_driver(k1_spi_driver);
+
+MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
+MODULE_LICENSE("GPL");
-- 
2.48.1

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Vivian Wang]

Hi Alex,

On 9/19/25 23:59, Alex Elder wrote:
> [...]
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 82fa5eb3b8684..4f6c446c6bc16 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>  	  also supporting 3Byte address devices and 4Byte address
>  	  devices.
>  
> +config SPI_SPACEMIT_K1
> +	tristate "K1 SPI Controller"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on OF
> +	default ARCH_SPACEMIT
> +	help
> +	  Enable support for the SpacemiT K1 SPI controller.
> +

We could still add:

	imply MMP_PDMA if ARCH_SPACEMIT

To add a "recommended" dependency. This way, enabling SPI_SPACEMIT_K1
automatically enables MMP_PDMA, but if the user is willing to fiddle
around, they can explicitly disable it. What do you think?

>  config SPI_SPRD
>  	tristate "Spreadtrum SPI controller"
>  	depends on ARCH_SPRD || COMPILE_TEST
>
> [...]
>
> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
> new file mode 100644
> index 0000000000000..8d564fe6c4303
> --- /dev/null
> +++ b/drivers/spi/spi-spacemit-k1.c
> @@ -0,0 +1,968 @@
>
> [...]
>
> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
> +{
> +	struct k1_spi_io *rx = &drv_data->rx;
> +	u32 bytes = drv_data->bytes;
> +	u32 val;
> +
> +	val = readl(drv_data->base + SSP_DATAR);
> +	rx->resid -= bytes;
> +
> +	if (!rx->buf)
> +		return;	/* Null reader: discard the data */
> +
> +	if (bytes == 1)
> +		*(u8 *)rx->buf = val;
> +	else if (bytes == 1)

Typo? else if (bytes == 2)

> +		*(u16 *)rx->buf = val;
> +	else
> +		*(u32 *)rx->buf = val;

Maybe

	else if (bytes == 4)
		*(u32 *)rx->buf = val;
	else
		WARN_ON_ONCE(1);

Just to make the pattern consistent? Same for k1_spi_write_word.

> +
> +	rx->buf += bytes;
> +}
>
> [...]
>
> +static int k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
> +{
> +	struct device *dev = drv_data->dev;
> +	int rx_ret;
> +	int tx_ret;
> +
> +	/* We must get both DMA channels, or neither of them */
> +	rx_ret = k1_spi_dma_setup_io(drv_data, true);
> +	if (rx_ret == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	tx_ret = k1_spi_dma_setup_io(drv_data, false);
> +
> +	/* If neither is specified, we don't use DMA */
> +	if (rx_ret == -ENODEV && tx_ret == -ENODEV)
> +		return 0;
> +
> +	if (rx_ret || tx_ret)
> +		goto err_cleanup;

This seems a bit convoluted. I'm wondering if all this explicit handling
really is necessary - if we get an error at probe time, can we just
return that error and let devres handle the rest? With the special case
that if we get both -ENODEV then disable DMA.

k1_spi_dma_cleanup_io seems to handle unmapping and termination of DMA,
which we... don't need, right?

> +
> +	drv_data->dummy = devm_kzalloc(dev, SZ_2K, GFP_KERNEL);
> +	if (drv_data->dummy)
> +		return 0;		/* Success! */
> +
> +	dev_warn(dev, "error allocating DMA dummy buffer; DMA disabled\n");

Just return -ENOMEM. If we can't even allocate 2K of buffer, we're
doomed anyway.

> +err_cleanup:
> +	if (tx_ret) {
> +		if (tx_ret != -EPROBE_DEFER)
> +			dev_err(dev, "error requesting DMA TX DMA channel\n");
> +	} else {
> +		k1_spi_dma_cleanup_io(drv_data, false);
> +	}
> +
> +	if (rx_ret)
> +		dev_err(dev, "error requesting DMA RX DMA channel\n");
> +	else
> +		k1_spi_dma_cleanup_io(drv_data, true);
> +
> +	if (tx_ret == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	/* Return success if we don't get the dummy buffer; PIO will be used */
> +
> +	return rx_ret ? : tx_ret ? : 0;
> +}
>
> [...]
>
> +static int devm_k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
> +{
> +	struct k1_spi_driver_data **ptr;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_MMP_PDMA)) {
> +		dev_warn(drv_data->dev, "DMA not available; using PIO\n");
> +		return 0;
> +	}
> +

Shouldn't be necessary if we do the "imply" thing in Kconfig.

> [...]
>
> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
> +{
>
> [...]
>
> +
> +	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
> +	if (!ret) {
> +		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
> +					   K1_SPI_MAX_SPEED_HZ);
> +		if (host->max_speed_hz != max_speed_hz)
> +			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
> +				max_speed_hz, host->max_speed_hz);
> +	} else {
> +		if (ret != -EINVAL)
> +			dev_warn(dev, "bad spi-max-frequency, using %u\n",
> +				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
> +		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
> +	}

I think it makes sense to have spi-max-frequency default to
K1_SPI_DEFAULT_MAX_SPEED_HZ, but if the value is out of range just print
a message and return an error, to get whoever wrote the bad value to fix it.

This range seems to be fixed by hardware, so, it should also be
specified in the bindings.

> +}
> +
>
> [...]
>
> +
> +static int k1_spi_probe(struct platform_device *pdev)
> +{
> +	struct k1_spi_driver_data *drv_data;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *reset;
> +	struct spi_controller *host;
> +	struct resource *iores;
> +	struct clk *clk_bus;
> +	int ret;
> +
> +	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
> +	if (!host)
> +		return -ENOMEM;
> +	drv_data = spi_controller_get_devdata(host);
> +	drv_data->controller = host;
> +	platform_set_drvdata(pdev, drv_data);
> +	drv_data->dev = dev;
> +	init_completion(&drv_data->completion);
> +
> +	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
> +								&iores);

Maybe

    devm_platform_ioremap_resource(pdev, 0);

> [...]
>
> +
> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
> +MODULE_LICENSE("GPL");

Maybe MODULE_AUTHOR()?

Vivian "dramforever" Wang

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Alex Elder]

On 9/19/25 10:52 PM, Vivian Wang wrote:
> Hi Alex,
> 
> On 9/19/25 23:59, Alex Elder wrote:
>> [...]
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 82fa5eb3b8684..4f6c446c6bc16 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>>   	  also supporting 3Byte address devices and 4Byte address
>>   	  devices.
>>   
>> +config SPI_SPACEMIT_K1
>> +	tristate "K1 SPI Controller"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on OF
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enable support for the SpacemiT K1 SPI controller.
>> +
> 
> We could still add:
> 
> 	imply MMP_PDMA if ARCH_SPACEMIT

I have never used "imply", I guess it's new (as of 2016)...

This sounds like a good suggestion.  This would mean MMP_PDMA
would by default take the same value as SPI_SPACEMIT_K1 (if
the former's dependencies were met), while allowing it to be
deselected for the configuration.
  > To add a "recommended" dependency. This way, enabling SPI_SPACEMIT_K1
> automatically enables MMP_PDMA, but if the user is willing to fiddle
> around, they can explicitly disable it. What do you think?

I like it.

>>   config SPI_SPRD
>>   	tristate "Spreadtrum SPI controller"
>>   	depends on ARCH_SPRD || COMPILE_TEST
>>
>> [...]
>>
>> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
>> new file mode 100644
>> index 0000000000000..8d564fe6c4303
>> --- /dev/null
>> +++ b/drivers/spi/spi-spacemit-k1.c
>> @@ -0,0 +1,968 @@
>>
>> [...]
>>
>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_io *rx = &drv_data->rx;
>> +	u32 bytes = drv_data->bytes;
>> +	u32 val;
>> +
>> +	val = readl(drv_data->base + SSP_DATAR);
>> +	rx->resid -= bytes;
>> +
>> +	if (!rx->buf)
>> +		return;	/* Null reader: discard the data */
>> +
>> +	if (bytes == 1)
>> +		*(u8 *)rx->buf = val;
>> +	else if (bytes == 1)
> 
> Typo? else if (bytes == 2)

Wow.  Yes that is an error that I'll correct.

>> +		*(u16 *)rx->buf = val;
>> +	else
>> +		*(u32 *)rx->buf = val;
> 
> Maybe
> 
> 	else if (bytes == 4)
> 		*(u32 *)rx->buf = val;
> 	else
> 		WARN_ON_ONCE(1);

The value of bytes will be 1, 2, or 4, which we can tell
by inspection.  At one time I had a switch statement with
a default, but I decided to leave out the default, which
won't happen.

> Just to make the pattern consistent? Same for k1_spi_write_word.

Consistent with what?

>> +	rx->buf += bytes;
>> +}
>>
>> [...]
>>
>> +static int k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct device *dev = drv_data->dev;
>> +	int rx_ret;
>> +	int tx_ret;
>> +
>> +	/* We must get both DMA channels, or neither of them */
>> +	rx_ret = k1_spi_dma_setup_io(drv_data, true);
>> +	if (rx_ret == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	tx_ret = k1_spi_dma_setup_io(drv_data, false);
>> +
>> +	/* If neither is specified, we don't use DMA */
>> +	if (rx_ret == -ENODEV && tx_ret == -ENODEV)
>> +		return 0;
>> +
>> +	if (rx_ret || tx_ret)
>> +		goto err_cleanup;
> 
> This seems a bit convoluted. I'm wondering if all this explicit handling
> really is necessary - if we get an error at probe time, can we just
> return that error and let devres handle the rest? With the special case
> that if we get both -ENODEV then disable DMA.

I agree it seems it should be less complex.

I'm trying to ensure that both channels are set up, or
that neither channel is set up, or that we try again if
we get -EPROBE_DEFER.  And if there's something wrong
with the configuration (only one of TX and RX is set up
successfully), an error occurs.

RX		TX		Result
--		--		------
0		0		0	(DMA)
-ENODEV		-ENODEV		0	(PIO)
-EPROBE_DEFER	(anything)	-EPROBE_DEFER (try again)
(anything)	-EPROBE_DEFER	-EPROBE_DEFER (try again)
0		-ENODEV		-ENODEV	(error, abort probe)
-ENODEV		0		-ENODEV	(error, abort probe)
error		(anything)	error	(error, abort probe)
(anything)	error		error	(error, abort probe)

Finally, if the buffer allocation fails:
0		0		0	(PIO; clean up TX and RX)

Let me think about this.  I'll see if I can find a simpler way
to achieve the above result, relying on devres to clean things
up.  I'd have to change k1_spi_dma_cleanup(), but you might be
right that it could be done more simply.

> k1_spi_dma_cleanup_io seems to handle unmapping and termination of DMA,
> which we... don't need, right?
> 
>> +
>> +	drv_data->dummy = devm_kzalloc(dev, SZ_2K, GFP_KERNEL);
>> +	if (drv_data->dummy)
>> +		return 0;		/* Success! */
>> +
>> +	dev_warn(dev, "error allocating DMA dummy buffer; DMA disabled\n");
> 
> Just return -ENOMEM. If we can't even allocate 2K of buffer, we're
> doomed anyway.

You're generally right, but I don't want my code to assume that.

>> +err_cleanup:
>> +	if (tx_ret) {
>> +		if (tx_ret != -EPROBE_DEFER)
>> +			dev_err(dev, "error requesting DMA TX DMA channel\n");
>> +	} else {
>> +		k1_spi_dma_cleanup_io(drv_data, false);
>> +	}
>> +
>> +	if (rx_ret)
>> +		dev_err(dev, "error requesting DMA RX DMA channel\n");
>> +	else
>> +		k1_spi_dma_cleanup_io(drv_data, true);
>> +
>> +	if (tx_ret == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	/* Return success if we don't get the dummy buffer; PIO will be used */
>> +
>> +	return rx_ret ? : tx_ret ? : 0;
>> +}
>>
>> [...]
>>
>> +static int devm_k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_driver_data **ptr;
>> +	int ret;
>> +
>> +	if (!IS_ENABLED(CONFIG_MMP_PDMA)) {
>> +		dev_warn(drv_data->dev, "DMA not available; using PIO\n");
>> +		return 0;
>> +	}
>> +
> 
> Shouldn't be necessary if we do the "imply" thing in Kconfig.

The messages I provide are based on an assumption that using
DMA is desirable and it will normally be used by this driver.
So if it won't be used, I'd like to provide that information.

On the other hand, I don't issue a warning if neither of
the channels is configured in the DTB.

I'm not going to commit either way on keeping/removing this.
If someone else weighs in I'm open to changing it.

>> [...]
>>
>> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
>> +{
>>
>> [...]
>>
>> +
>> +	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
>> +	if (!ret) {
>> +		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
>> +					   K1_SPI_MAX_SPEED_HZ);
>> +		if (host->max_speed_hz != max_speed_hz)
>> +			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
>> +				max_speed_hz, host->max_speed_hz);
>> +	} else {
>> +		if (ret != -EINVAL)
>> +			dev_warn(dev, "bad spi-max-frequency, using %u\n",
>> +				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
>> +		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
>> +	}
> 
> I think it makes sense to have spi-max-frequency default to
> K1_SPI_DEFAULT_MAX_SPEED_HZ, but if the value is out of range just print
> a message and return an error, to get whoever wrote the bad value to fix it.

These errors just shouldn't happen.  But the way I handle this,
it allows the SPI controller to still be used, even if the
administrator can't really update the DTB.

> This range seems to be fixed by hardware, so, it should also be
> specified in the bindings.

I define the hardware limits here, and enforce
them, in case the bindings specify an out-of-range
value.  Again, this is an error that just shouldn't
occur in practice, but the code is defensive.

Most of your comments really made me think about how
errors are handled.  I appreciate it.

					-Alex

> 
>> +}
>> +
>>
>> [...]
>>
>> +
>> +static int k1_spi_probe(struct platform_device *pdev)
>> +{
>> +	struct k1_spi_driver_data *drv_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *reset;
>> +	struct spi_controller *host;
>> +	struct resource *iores;
>> +	struct clk *clk_bus;
>> +	int ret;
>> +
>> +	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
>> +	if (!host)
>> +		return -ENOMEM;
>> +	drv_data = spi_controller_get_devdata(host);
>> +	drv_data->controller = host;
>> +	platform_set_drvdata(pdev, drv_data);
>> +	drv_data->dev = dev;
>> +	init_completion(&drv_data->completion);
>> +
>> +	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
>> +								&iores);
> 
> Maybe
> 
>      devm_platform_ioremap_resource(pdev, 0);
> 
>> [...]
>>
>> +
>> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
>> +MODULE_LICENSE("GPL");
> 
> Maybe MODULE_AUTHOR()?
> 
> Vivian "dramforever" Wang
> 

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Vivian Wang]

On 9/20/25 23:59, Alex Elder wrote:
> On 9/19/25 10:52 PM, Vivian Wang wrote:
>>
>> [...]
>>
>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>> +{
>> +    struct k1_spi_io *rx = &drv_data->rx;
>> +    u32 bytes = drv_data->bytes;
>> +    u32 val;
>> +
>> +    val = readl(drv_data->base + SSP_DATAR);
>> +    rx->resid -= bytes;
>> +
>> +    if (!rx->buf)
>> +        return;    /* Null reader: discard the data */
>> +
>> +    if (bytes == 1)
>> +        *(u8 *)rx->buf = val;
>> +    else if (bytes == 1)
>>
>> Typo? else if (bytes == 2)
>
> Wow.  Yes that is an error that I'll correct.
>
>>> +        *(u16 *)rx->buf = val;
>>> +    else
>>> +        *(u32 *)rx->buf = val;
>>
>> Maybe
>>
>>     else if (bytes == 4)
>>         *(u32 *)rx->buf = val;
>>     else
>>         WARN_ON_ONCE(1);
>
> The value of bytes will be 1, 2, or 4, which we can tell
> by inspection.  At one time I had a switch statement with
> a default, but I decided to leave out the default, which
> won't happen.
>
>> Just to make the pattern consistent? Same for k1_spi_write_word.
>
> Consistent with what? 
>
I was just thinking it would be clearer if the code states clearly:

    1 -> u8
    2 -> u16
    4 -> u32
    anything else -> shouldn't happen

As is, it wasn't obvious to me that we're just handling 4 as u32. Maybe
we're just capping it at u32, and 8 is also handled.

Well, maybe I'm just not familiar with SPI stuff, and word size above 4
doesn't make sense anyway.

It could also be a comment

    else /* 4 */

Just a suggestion, no strong preference from me.

Vivian "dramforever" Wang

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Alex Elder]

On 9/21/25 1:51 PM, Vivian Wang wrote:
> 
> On 9/20/25 23:59, Alex Elder wrote:
>> On 9/19/25 10:52 PM, Vivian Wang wrote:
>>>
>>> [...]
>>>
>>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>>> +{
>>> +    struct k1_spi_io *rx = &drv_data->rx;
>>> +    u32 bytes = drv_data->bytes;
>>> +    u32 val;
>>> +
>>> +    val = readl(drv_data->base + SSP_DATAR);
>>> +    rx->resid -= bytes;
>>> +
>>> +    if (!rx->buf)
>>> +        return;    /* Null reader: discard the data */
>>> +
>>> +    if (bytes == 1)
>>> +        *(u8 *)rx->buf = val;
>>> +    else if (bytes == 1)
>>>
>>> Typo? else if (bytes == 2)
>>
>> Wow.  Yes that is an error that I'll correct.
>>
>>>> +        *(u16 *)rx->buf = val;
>>>> +    else
>>>> +        *(u32 *)rx->buf = val;
>>>
>>> Maybe
>>>
>>>      else if (bytes == 4)
>>>          *(u32 *)rx->buf = val;
>>>      else
>>>          WARN_ON_ONCE(1);
>>
>> The value of bytes will be 1, 2, or 4, which we can tell
>> by inspection.  At one time I had a switch statement with
>> a default, but I decided to leave out the default, which
>> won't happen.
>>
>>> Just to make the pattern consistent? Same for k1_spi_write_word.
>>
>> Consistent with what?
>>
> I was just thinking it would be clearer if the code states clearly:
> 
>      1 -> u8
>      2 -> u16
>      4 -> u32
>      anything else -> shouldn't happen
> 
> As is, it wasn't obvious to me that we're just handling 4 as u32. Maybe
> we're just capping it at u32, and 8 is also handled.
> 
> Well, maybe I'm just not familiar with SPI stuff, and word size above 4
> doesn't make sense anyway.

Understood.  I only know it because I checked.  And I do want my
code to me understandable, so I'll add a comment as you suggest
below.

Thank you.

					-Alex

> 
> It could also be a comment
> 
>      else /* 4 */
> 
> Just a suggestion, no strong preference from me.
> 
> Vivian "dramforever" Wang
> 

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Alex Elder]

On 9/20/25 10:59 AM, Alex Elder wrote:
> On 9/19/25 10:52 PM, Vivian Wang wrote:
>> Hi Alex,
>>
>> On 9/19/25 23:59, Alex Elder wrote:
>>> [...]
>>>
>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>> index 82fa5eb3b8684..4f6c446c6bc16 100644
>>> --- a/drivers/spi/Kconfig
>>> +++ b/drivers/spi/Kconfig
>>> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>>>         also supporting 3Byte address devices and 4Byte address
>>>         devices.
>>> +config SPI_SPACEMIT_K1
>>> +    tristate "K1 SPI Controller"
>>> +    depends on ARCH_SPACEMIT || COMPILE_TEST
>>> +    depends on OF
>>> +    default ARCH_SPACEMIT
>>> +    help
>>> +      Enable support for the SpacemiT K1 SPI controller.
>>> +
>>
>> We could still add:
>>
>>     imply MMP_PDMA if ARCH_SPACEMIT
> 
> I have never used "imply", I guess it's new (as of 2016)...
> 
> This sounds like a good suggestion.  This would mean MMP_PDMA
> would by default take the same value as SPI_SPACEMIT_K1 (if
> the former's dependencies were met), while allowing it to be
> deselected for the configuration.
>   > To add a "recommended" dependency. This way, enabling SPI_SPACEMIT_K1
>> automatically enables MMP_PDMA, but if the user is willing to fiddle
>> around, they can explicitly disable it. What do you think?
> 
> I like it.
> 
>>>   config SPI_SPRD
>>>       tristate "Spreadtrum SPI controller"
>>>       depends on ARCH_SPRD || COMPILE_TEST
>>>
>>> [...]
>>>
>>> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi- 
>>> spacemit-k1.c
>>> new file mode 100644
>>> index 0000000000000..8d564fe6c4303
>>> --- /dev/null
>>> +++ b/drivers/spi/spi-spacemit-k1.c
>>> @@ -0,0 +1,968 @@
>>>
>>> [...]
>>>
>>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>>> +{
>>> +    struct k1_spi_io *rx = &drv_data->rx;
>>> +    u32 bytes = drv_data->bytes;
>>> +    u32 val;
>>> +
>>> +    val = readl(drv_data->base + SSP_DATAR);
>>> +    rx->resid -= bytes;
>>> +
>>> +    if (!rx->buf)
>>> +        return;    /* Null reader: discard the data */
>>> +
>>> +    if (bytes == 1)
>>> +        *(u8 *)rx->buf = val;
>>> +    else if (bytes == 1)
>>
>> Typo? else if (bytes == 2)
> 
> Wow.  Yes that is an error that I'll correct.
> 
>>> +        *(u16 *)rx->buf = val;
>>> +    else
>>> +        *(u32 *)rx->buf = val;
>>
>> Maybe
>>
>>     else if (bytes == 4)
>>         *(u32 *)rx->buf = val;
>>     else
>>         WARN_ON_ONCE(1);
> 
> The value of bytes will be 1, 2, or 4, which we can tell
> by inspection.  At one time I had a switch statement with
> a default, but I decided to leave out the default, which
> won't happen.
> 
>> Just to make the pattern consistent? Same for k1_spi_write_word.
> 
> Consistent with what?

FYI in updating my code I now realize that k1_spi_write()
and k1_spi_read() were done differently, and that's what
you were talking about.  (I assumed they both used the same
pattern, not checking in the case bytes == 4.)

That was the basis of my "consistent with what?" question
and I'm sorry I didn't realize I was mistaken about how
this was written.

They're both going to look roughly like this now:

         if (bytes == 1)
                 *(u8 *)rx->buf = val;
         else if (bytes == 2)
                 *(u16 *)rx->buf = val;
         else    /* bytes == 4 */
                 *(u32 *)rx->buf = val;

>>> +    rx->buf += bytes;
>>> +}
>>>
>>> [...]
>>>
>>> +static int k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>>> +{
>>> +    struct device *dev = drv_data->dev;
>>> +    int rx_ret;
>>> +    int tx_ret;
>>> +
>>> +    /* We must get both DMA channels, or neither of them */
>>> +    rx_ret = k1_spi_dma_setup_io(drv_data, true);
>>> +    if (rx_ret == -EPROBE_DEFER)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    tx_ret = k1_spi_dma_setup_io(drv_data, false);
>>> +
>>> +    /* If neither is specified, we don't use DMA */
>>> +    if (rx_ret == -ENODEV && tx_ret == -ENODEV)
>>> +        return 0;
>>> +
>>> +    if (rx_ret || tx_ret)
>>> +        goto err_cleanup;
>>
>> This seems a bit convoluted. I'm wondering if all this explicit handling
>> really is necessary - if we get an error at probe time, can we just
>> return that error and let devres handle the rest? With the special case
>> that if we get both -ENODEV then disable DMA.
> 
> I agree it seems it should be less complex.

Your suggestion is to simplify this code by falling
back on the devres mechanism to take care of cleaning
things up when an error is returned.

But that won't work, because this function is called
by the devm_k1_spi_dma_setup() devres wrapper function.

If this function returns an error, that function assumes
failure, so it never sets up the resource, and never
calls the cleanup function.  This function must restore
everything to the previous state if it returns an error.

> I'm trying to ensure that both channels are set up, or
> that neither channel is set up, or that we try again if
> we get -EPROBE_DEFER.  And if there's something wrong
> with the configuration (only one of TX and RX is set up
> successfully), an error occurs.
> 
> RX        TX        Result
> --        --        ------
> 0        0        0    (DMA)
> -ENODEV        -ENODEV        0    (PIO)
> -EPROBE_DEFER    (anything)    -EPROBE_DEFER (try again)
> (anything)    -EPROBE_DEFER    -EPROBE_DEFER (try again)
> 0        -ENODEV        -ENODEV    (error, abort probe)
> -ENODEV        0        -ENODEV    (error, abort probe)
> error        (anything)    error    (error, abort probe)
> (anything)    error        error    (error, abort probe)
> 
> Finally, if the buffer allocation fails:
> 0        0        0    (PIO; clean up TX and RX)
> 
> Let me think about this.  I'll see if I can find a simpler way
> to achieve the above result, relying on devres to clean things
> up.  I'd have to change k1_spi_dma_cleanup(), but you might be
> right that it could be done more simply.

Version 3 of this series will keep this code similar to v2.

> 
>> k1_spi_dma_cleanup_io seems to handle unmapping and termination of DMA,
>> which we... don't need, right?

You're right.  Those cleanup activities are not necessary,
because they're meant to clean up stuff that happens *after*
setup completes--once we perform at least one DMA.

HOWEVER, because devm_k1_spi_dma_setup() assumes this
function returns everything to its previous state, the
channels need to be freed explicitly on error (which
did not occur in the v2 code).

And because devm_k1_spi_dma_setup() is already arranging
for resource-managed cleanup, I'm just going to request
the channel "directly" in k1_spi_dma_setup_io() rather
than layering another devm_*() call there.  Then I'll
explicitly release the channel in k1_spi_dma_cleanup_io().

So k1_spi_dma_setup() will basically stay the same.

The same logic applies to allocating the dummy buffer;
it will be done with kzalloc(), and will be freed in
k1_spi_dma_cleanup().

This ended up needing to be fixed, even though it wasn't
exactly what you were looking for.  So either way, I'm
glad you asked about this.

Thanks.

					-Alex

>>> +    drv_data->dummy = devm_kzalloc(dev, SZ_2K, GFP_KERNEL);
>>> +    if (drv_data->dummy)
>>> +        return 0;        /* Success! */
>>> +
>>> +    dev_warn(dev, "error allocating DMA dummy buffer; DMA disabled\n");
>>
>> Just return -ENOMEM. If we can't even allocate 2K of buffer, we're
>> doomed anyway.
> 
> You're generally right, but I don't want my code to assume that.
> 
>>> +err_cleanup:
>>> +    if (tx_ret) {
>>> +        if (tx_ret != -EPROBE_DEFER)
>>> +            dev_err(dev, "error requesting DMA TX DMA channel\n");
>>> +    } else {
>>> +        k1_spi_dma_cleanup_io(drv_data, false);
>>> +    }
>>> +
>>> +    if (rx_ret)
>>> +        dev_err(dev, "error requesting DMA RX DMA channel\n");
>>> +    else
>>> +        k1_spi_dma_cleanup_io(drv_data, true);
>>> +
>>> +    if (tx_ret == -EPROBE_DEFER)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    /* Return success if we don't get the dummy buffer; PIO will be 
>>> used */
>>> +
>>> +    return rx_ret ? : tx_ret ? : 0;
>>> +}
>>>
>>> [...]
>>>
>>> +static int devm_k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>>> +{
>>> +    struct k1_spi_driver_data **ptr;
>>> +    int ret;
>>> +
>>> +    if (!IS_ENABLED(CONFIG_MMP_PDMA)) {
>>> +        dev_warn(drv_data->dev, "DMA not available; using PIO\n");
>>> +        return 0;
>>> +    }
>>> +
>>
>> Shouldn't be necessary if we do the "imply" thing in Kconfig.
> 
> The messages I provide are based on an assumption that using
> DMA is desirable and it will normally be used by this driver.
> So if it won't be used, I'd like to provide that information.
> 
> On the other hand, I don't issue a warning if neither of
> the channels is configured in the DTB.
> 
> I'm not going to commit either way on keeping/removing this.
> If someone else weighs in I'm open to changing it.
> 
>>> [...]
>>>
>>> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
>>> +{
>>>
>>> [...]
>>>
>>> +
>>> +    ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
>>> +    if (!ret) {
>>> +        host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
>>> +                       K1_SPI_MAX_SPEED_HZ);
>>> +        if (host->max_speed_hz != max_speed_hz)
>>> +            dev_warn(dev, "spi-max-frequency %u out of range, using 
>>> %u\n",
>>> +                max_speed_hz, host->max_speed_hz);
>>> +    } else {
>>> +        if (ret != -EINVAL)
>>> +            dev_warn(dev, "bad spi-max-frequency, using %u\n",
>>> +                 K1_SPI_DEFAULT_MAX_SPEED_HZ);
>>> +        host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
>>> +    }
>>
>> I think it makes sense to have spi-max-frequency default to
>> K1_SPI_DEFAULT_MAX_SPEED_HZ, but if the value is out of range just print
>> a message and return an error, to get whoever wrote the bad value to 
>> fix it.
> 
> These errors just shouldn't happen.  But the way I handle this,
> it allows the SPI controller to still be used, even if the
> administrator can't really update the DTB.
> 
>> This range seems to be fixed by hardware, so, it should also be
>> specified in the bindings.
> 
> I define the hardware limits here, and enforce
> them, in case the bindings specify an out-of-range
> value.  Again, this is an error that just shouldn't
> occur in practice, but the code is defensive.
> 
> Most of your comments really made me think about how
> errors are handled.  I appreciate it.
> 
>                      -Alex
> 
>>
>>> +}
>>> +
>>>
>>> [...]
>>>
>>> +
>>> +static int k1_spi_probe(struct platform_device *pdev)
>>> +{
>>> +    struct k1_spi_driver_data *drv_data;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct reset_control *reset;
>>> +    struct spi_controller *host;
>>> +    struct resource *iores;
>>> +    struct clk *clk_bus;
>>> +    int ret;
>>> +
>>> +    host = devm_spi_alloc_host(dev, sizeof(*drv_data));
>>> +    if (!host)
>>> +        return -ENOMEM;
>>> +    drv_data = spi_controller_get_devdata(host);
>>> +    drv_data->controller = host;
>>> +    platform_set_drvdata(pdev, drv_data);
>>> +    drv_data->dev = dev;
>>> +    init_completion(&drv_data->completion);
>>> +
>>> +    drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
>>> +                                &iores);
>>
>> Maybe
>>
>>      devm_platform_ioremap_resource(pdev, 0);
>>
>>> [...]
>>>
>>> +
>>> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
>>> +MODULE_LICENSE("GPL");
>>
>> Maybe MODULE_AUTHOR()?
>>
>> Vivian "dramforever" Wang
>>
> 

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Troy Mitchell]

On Fri, Sep 19, 2025 at 10:59:12AM -0500, Alex Elder wrote:
> This patch introduces the driver for the SPI controller found in the
> SpacemiT K1 SoC.  Currently the driver supports master mode only.
> The SPI hardware implements RX and TX FIFOs, 32 entries each, and
> supports both PIO and DMA mode transfers.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - The SPI_SPACEMIT_K1 config option is added in sorted order
>     - The spi-spacemit-k1.o make target is now added in sorted order
>     - Read/modify/writes of registers no longer use an additional
>       "virt" variable to hold the address accessed
>     - The k1_spi_driver_data->ioaddr field has been renamed base
>     - The DMA address for the base address is maintained, rather than
>       saving the DMA address of the data register
>     - CONFIG_MMP_PDMA is checked at runtime, and if it is not enabled,
>       DMA will not be used
>     - The spi-max-frequency property value is now bounds checked
>     - A local variable is now initialized to 0 in k1_spi_write_word()
>     - The driver name is now "k1-spi"
> 
>  drivers/spi/Kconfig           |   8 +
>  drivers/spi/Makefile          |   1 +
>  drivers/spi/spi-spacemit-k1.c | 968 ++++++++++++++++++++++++++++++++++
>  3 files changed, 977 insertions(+)
>  create mode 100644 drivers/spi/spi-spacemit-k1.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 82fa5eb3b8684..4f6c446c6bc16 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>  	  also supporting 3Byte address devices and 4Byte address
>  	  devices.
>  
> +config SPI_SPACEMIT_K1
> +	tristate "K1 SPI Controller"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on OF
> +	default ARCH_SPACEMIT
> +	help
> +	  Enable support for the SpacemiT K1 SPI controller.
> +
>  config SPI_SPRD
>  	tristate "Spreadtrum SPI controller"
>  	depends on ARCH_SPRD || COMPILE_TEST
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index eefaeca097456..637d750ead996 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -140,6 +140,7 @@ obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
>  obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
>  obj-$(CONFIG_SPI_SN_F_OSPI)		+= spi-sn-f-ospi.o
>  obj-$(CONFIG_SPI_SG2044_NOR)	+= spi-sg2044-nor.o
> +obj-$(CONFIG_SPI_SPACEMIT_K1)		+= spi-spacemit-k1.o
>  obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
>  obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
>  obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
> new file mode 100644
> index 0000000000000..8d564fe6c4303
> --- /dev/null
> +++ b/drivers/spi/spi-spacemit-k1.c
> @@ -0,0 +1,968 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for SpacemiT K1 SPI controller
> + *
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + * Copyright (c) 2023, spacemit Corporation.
SpacemiT?

> + */
> +
[...]
> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
> +{
> +	struct k1_spi_io *rx = &drv_data->rx;
> +	u32 bytes = drv_data->bytes;
> +	u32 val;
> +
> +	val = readl(drv_data->base + SSP_DATAR);
> +	rx->resid -= bytes;
> +
> +	if (!rx->buf)
> +		return;	/* Null reader: discard the data */
> +
> +	if (bytes == 1)
> +		*(u8 *)rx->buf = val;
> +	else if (bytes == 1)
typo?

> +		*(u16 *)rx->buf = val;
> +	else
> +		*(u32 *)rx->buf = val;
> +
> +	rx->buf += bytes;
> +}
> +
[...]
> +static bool k1_spi_write(struct k1_spi_driver_data *drv_data)
> +{
> +	struct k1_spi_io *tx = &drv_data->tx;
> +	unsigned int count;
> +	u32 val;
> +
> +	if (!tx->resid)
> +		return true;	/* Nothing more to send */
> +
> +	/* See how many slots in the TX FIFO are available */
> +	val = readl(drv_data->base + SSP_STATUS);
> +	count = FIELD_GET(SSP_STATUS_TFL, val);
> +
> +	/* A zero count means the FIFO is either full or empty */
> +	if (!count) {
> +		if (val & SSP_STATUS_TNF)
> +			count = K1_SPI_FIFO_SIZE;
> +		else
> +			return false;	/* No room in the FIFO */
please early return:

if (!(val & SSP_STATUS_TNF))
  return false;
count = K1_SPI_FIFO_SIZE;
> +	}
> +
> +	/*
> +	 * Limit how much we try to send at a time, to reduce the
> +	 * chance the other side can overrun our RX FIFO.
> +	 */
> +	count = min3(count, K1_SPI_THRESH, tx->resid);
> +	while (count--)
> +		k1_spi_write_word(drv_data);
> +
> +	return !tx->resid;
> +}
> +
[...]
> +
> +static int k1_spi_transfer_one_message(struct spi_controller *host,
> +					   struct spi_message *message)
> +{
> +	struct k1_spi_driver_data *drv_data = spi_controller_get_devdata(host);
> +	struct completion *completion = &drv_data->completion;
> +	struct spi_transfer *transfer;
> +	u32 val;
> +
> +	drv_data->message = message;
> +
> +	/* Message status starts out successful; set to -EIO on error */
> +	message->status = 0;
> +
> +	/* Hold frame low to avoid losing transferred data */
> +	val = readl(drv_data->base + SSP_TOP_CTRL);
> +	val |= TOP_HOLD_FRAME_LOW;
> +	writel(val, drv_data->base + SSP_TOP_CTRL);
> +
> +	list_for_each_entry(transfer, &message->transfers, transfer_list) {
> +		reinit_completion(completion);
> +
> +		/* Issue the next transfer */
> +		if (!k1_spi_transfer_start(drv_data, transfer)) {
> +			message->status = -EIO;
> +			break;
we don't need return a error code?

> +		}
> +
> +		k1_spi_transfer_wait(drv_data);
> +
> +		k1_spi_transfer_end(drv_data, transfer);
> +
> +		/* If an error has occurred, we're done */
> +		if (message->status)
> +			break;
ditto.

[...]
> +}
> +
> +static void k1_spi_dma_cleanup(struct device *dev, void *res)
> +{
> +	struct k1_spi_driver_data *drv_data = res;
> +
> +	if (k1_spi_dma_enabled(drv_data)) {
if (!k1_spi_dma_enabled(drv_data))
  return;

                  - Troy

> +		k1_spi_dma_cleanup_io(drv_data, false);
> +		k1_spi_dma_cleanup_io(drv_data, true);
> +	}
> +}
> +
> +
> +static const struct of_device_id k1_spi_dt_ids[] = {
> +	{ .compatible = "spacemit,k1-spi", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, k1_spi_dt_ids);
> +
> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
> +{
> +	struct device_node *np = dev_of_node(drv_data->dev);
> +	struct spi_controller *host = drv_data->controller;
> +	struct device *dev = drv_data->dev;
> +	u32 max_speed_hz;
> +	int ret;
> +
> +	host->dev.of_node = np;
> +	host->dev.parent = drv_data->dev;
> +	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
> +	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +	host->num_chipselect = 1;
> +
> +	if (k1_spi_dma_enabled(drv_data))
> +		host->dma_alignment = K1_SPI_DMA_ALIGNMENT;
> +	host->cleanup = k1_spi_cleanup;
> +	host->setup = k1_spi_setup;
> +	host->transfer_one_message = k1_spi_transfer_one_message;
> +
> +	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
> +	if (!ret) {
> +		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
> +					   K1_SPI_MAX_SPEED_HZ);
> +		if (host->max_speed_hz != max_speed_hz)
> +			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
> +				max_speed_hz, host->max_speed_hz);
> +	} else {
> +		if (ret != -EINVAL)
> +			dev_warn(dev, "bad spi-max-frequency, using %u\n",
> +				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
> +		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
> +	}
> +}
> +
> +/* Set our registers to a known initial state */
> +static void
> +k1_spi_register_reset(struct k1_spi_driver_data *drv_data, bool initial)
> +{
> +	u32 val = 0;
> +
> +	writel(0, drv_data->base + SSP_TOP_CTRL);
> +
> +	if (initial) {
> +		/*
> +		 * The TX and RX FIFO thresholds are the same no matter
> +		 * what the speed or bits per word, so we can just set
> +		 * them once.  The thresholds are one more than the values
> +		 * in the register.
> +		 */
> +		val = FIELD_PREP(FIFO_RFT_MASK, K1_SPI_THRESH - 1);
> +		val |= FIELD_PREP(FIFO_TFT_MASK, K1_SPI_THRESH - 1);
> +	}
> +	writel(val, drv_data->base + SSP_FIFO_CTRL);
> +
> +	writel(0, drv_data->base + SSP_INT_EN);
> +	writel(0, drv_data->base + SSP_TIMEOUT);
> +
> +	/* Clear any pending interrupt conditions */
> +	val = readl(drv_data->base + SSP_STATUS);
> +	writel(val, drv_data->base + SSP_STATUS);
> +}
> +
> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
> +{
> +	struct k1_spi_driver_data *drv_data = dev_id;
> +	bool rx_done;
> +	bool tx_done;
> +	u32 val;
> +
> +	/* Get status and clear pending interrupts */
> +	val = readl(drv_data->base + SSP_STATUS);
> +	writel(val, drv_data->base + SSP_STATUS);
> +
> +	if (!drv_data->message)
> +		return IRQ_NONE;
> +
> +	/* Check for a TX underrun or RX underrun first */
> +	if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR)) {
> +		/* Disable all interrupts on error */
> +		writel(0, drv_data->base + SSP_INT_EN);
> +
> +		drv_data->message->status = -EIO;
> +		complete(&drv_data->completion);
> +
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Drain the RX FIFO first, then transmit what we can */
> +	rx_done = k1_spi_read(drv_data);
> +	tx_done = k1_spi_write(drv_data);
> +
> +	/* Disable interrupts if we're done transferring either direction */
> +	if (rx_done || tx_done) {
> +		/* If both are done, disable all interrupts */
> +		if (rx_done && tx_done) {
> +			val = 0;
> +		} else {
> +			val = readl(drv_data->base + SSP_INT_EN);
> +			if (rx_done)
> +				val &= ~(SSP_INT_EN_TINTE | SSP_INT_EN_RIE);
> +			if (tx_done)
> +				val &= ~SSP_INT_EN_TIE;
> +		}
> +		writel(val, drv_data->base + SSP_INT_EN);
> +	}
> +
> +	if (rx_done && tx_done)
> +		complete(&drv_data->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int k1_spi_probe(struct platform_device *pdev)
> +{
> +	struct k1_spi_driver_data *drv_data;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *reset;
> +	struct spi_controller *host;
> +	struct resource *iores;
> +	struct clk *clk_bus;
> +	int ret;
> +
> +	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
> +	if (!host)
> +		return -ENOMEM;
> +	drv_data = spi_controller_get_devdata(host);
> +	drv_data->controller = host;
> +	platform_set_drvdata(pdev, drv_data);
> +	drv_data->dev = dev;
> +	init_completion(&drv_data->completion);
> +
> +	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
> +								&iores);
> +	if (IS_ERR(drv_data->base))
> +		return dev_err_probe(dev, PTR_ERR(drv_data->base),
> +				     "error mapping memory\n");
> +	drv_data->base_addr = iores->start;
> +
> +	ret = devm_k1_spi_dma_setup(drv_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error setting up DMA\n");
> +
> +	k1_spi_host_init(drv_data);
> +
> +	clk_bus = devm_clk_get_enabled(dev, "bus");
> +	if (IS_ERR(clk_bus))
> +		return dev_err_probe(dev, PTR_ERR(clk_bus),
> +				     "error getting/enabling bus clock\n");
> +	drv_data->bus_rate = clk_get_rate(clk_bus);
> +
> +	drv_data->clk = devm_clk_get_enabled(dev, "core");
> +	if (IS_ERR(drv_data->clk))
> +		return dev_err_probe(dev, PTR_ERR(drv_data->clk),
> +				     "error getting/enabling core clock\n");
> +
> +	reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				     "error getting/deasserting reset\n");
> +
> +	k1_spi_register_reset(drv_data, true);
> +
> +	drv_data->irq = platform_get_irq(pdev, 0);
> +	if (drv_data->irq < 0)
> +		return dev_err_probe(dev, drv_data->irq, "error getting IRQ\n");
> +
> +	ret = devm_request_irq(dev, drv_data->irq, k1_spi_ssp_isr,
> +			       IRQF_SHARED, dev_name(dev), drv_data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "error requesting IRQ\n");
> +
> +	ret = devm_spi_register_controller(dev, host);
> +	if (ret)
> +		dev_err(dev, "error registering controller\n");
> +
> +	return ret;
> +}
> +
> +static void k1_spi_remove(struct platform_device *pdev)
> +{
> +	struct k1_spi_driver_data *drv_data = platform_get_drvdata(pdev);
> +
> +	k1_spi_register_reset(drv_data, false);
> +}
> +
> +static struct platform_driver k1_spi_driver = {
> +	.driver = {
> +		.name		= "k1-spi",
> +		.of_match_table	= k1_spi_dt_ids,
> +	},
> +	.probe			= k1_spi_probe,
> +	.remove			= k1_spi_remove,
> +};
> +
> +module_platform_driver(k1_spi_driver);
> +
> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.48.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Alex Elder]

On 9/20/25 10:15 PM, Troy Mitchell wrote:
> On Fri, Sep 19, 2025 at 10:59:12AM -0500, Alex Elder wrote:
>> This patch introduces the driver for the SPI controller found in the
>> SpacemiT K1 SoC.  Currently the driver supports master mode only.
>> The SPI hardware implements RX and TX FIFOs, 32 entries each, and
>> supports both PIO and DMA mode transfers.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - The SPI_SPACEMIT_K1 config option is added in sorted order
>>      - The spi-spacemit-k1.o make target is now added in sorted order
>>      - Read/modify/writes of registers no longer use an additional
>>        "virt" variable to hold the address accessed
>>      - The k1_spi_driver_data->ioaddr field has been renamed base
>>      - The DMA address for the base address is maintained, rather than
>>        saving the DMA address of the data register
>>      - CONFIG_MMP_PDMA is checked at runtime, and if it is not enabled,
>>        DMA will not be used
>>      - The spi-max-frequency property value is now bounds checked
>>      - A local variable is now initialized to 0 in k1_spi_write_word()
>>      - The driver name is now "k1-spi"
>>
>>   drivers/spi/Kconfig           |   8 +
>>   drivers/spi/Makefile          |   1 +
>>   drivers/spi/spi-spacemit-k1.c | 968 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 977 insertions(+)
>>   create mode 100644 drivers/spi/spi-spacemit-k1.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 82fa5eb3b8684..4f6c446c6bc16 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>>   	  also supporting 3Byte address devices and 4Byte address
>>   	  devices.
>>   
>> +config SPI_SPACEMIT_K1
>> +	tristate "K1 SPI Controller"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on OF
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enable support for the SpacemiT K1 SPI controller.
>> +
>>   config SPI_SPRD
>>   	tristate "Spreadtrum SPI controller"
>>   	depends on ARCH_SPRD || COMPILE_TEST
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index eefaeca097456..637d750ead996 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -140,6 +140,7 @@ obj-$(CONFIG_SPI_SIFIVE)		+= spi-sifive.o
>>   obj-$(CONFIG_SPI_SLAVE_MT27XX)          += spi-slave-mt27xx.o
>>   obj-$(CONFIG_SPI_SN_F_OSPI)		+= spi-sn-f-ospi.o
>>   obj-$(CONFIG_SPI_SG2044_NOR)	+= spi-sg2044-nor.o
>> +obj-$(CONFIG_SPI_SPACEMIT_K1)		+= spi-spacemit-k1.o
>>   obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
>>   obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
>>   obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
>> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
>> new file mode 100644
>> index 0000000000000..8d564fe6c4303
>> --- /dev/null
>> +++ b/drivers/spi/spi-spacemit-k1.c
>> @@ -0,0 +1,968 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for SpacemiT K1 SPI controller
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + * Copyright (c) 2023, spacemit Corporation.
> SpacemiT?

Already addressed this suggestion, which Yixun also made.
  
https://lore.kernel.org/lkml/034cecd3-c168-4c8d-9ad5-10cc1853894b@riscstar.com/


> 
>> + */
>> +
> [...]
>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_io *rx = &drv_data->rx;
>> +	u32 bytes = drv_data->bytes;
>> +	u32 val;
>> +
>> +	val = readl(drv_data->base + SSP_DATAR);
>> +	rx->resid -= bytes;
>> +
>> +	if (!rx->buf)
>> +		return;	/* Null reader: discard the data */
>> +
>> +	if (bytes == 1)
>> +		*(u8 *)rx->buf = val;
>> +	else if (bytes == 1)
> typo?

Yes, Vivian noticed this too.  It's a bug, and it will be
fixed in the next version.
  
https://lore.kernel.org/lkml/a7070f3f-8857-4834-9e9e-02068637075c@iscas.ac.cn/

> 
>> +		*(u16 *)rx->buf = val;
>> +	else
>> +		*(u32 *)rx->buf = val;
>> +
>> +	rx->buf += bytes;
>> +}
>> +
> [...]
>> +static bool k1_spi_write(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_io *tx = &drv_data->tx;
>> +	unsigned int count;
>> +	u32 val;
>> +
>> +	if (!tx->resid)
>> +		return true;	/* Nothing more to send */
>> +
>> +	/* See how many slots in the TX FIFO are available */
>> +	val = readl(drv_data->base + SSP_STATUS);
>> +	count = FIELD_GET(SSP_STATUS_TFL, val);
>> +
>> +	/* A zero count means the FIFO is either full or empty */
>> +	if (!count) {
>> +		if (val & SSP_STATUS_TNF)
>> +			count = K1_SPI_FIFO_SIZE;
>> +		else
>> +			return false;	/* No room in the FIFO */
> please early return:

OK.

> 
> if (!(val & SSP_STATUS_TNF))
>    return false;
> count = K1_SPI_FIFO_SIZE;
>> +	}
>> +
>> +	/*
>> +	 * Limit how much we try to send at a time, to reduce the
>> +	 * chance the other side can overrun our RX FIFO.
>> +	 */
>> +	count = min3(count, K1_SPI_THRESH, tx->resid);
>> +	while (count--)
>> +		k1_spi_write_word(drv_data);
>> +
>> +	return !tx->resid;
>> +}
>> +
> [...]
>> +
>> +static int k1_spi_transfer_one_message(struct spi_controller *host,
>> +					   struct spi_message *message)
>> +{
>> +	struct k1_spi_driver_data *drv_data = spi_controller_get_devdata(host);
>> +	struct completion *completion = &drv_data->completion;
>> +	struct spi_transfer *transfer;
>> +	u32 val;
>> +
>> +	drv_data->message = message;
>> +
>> +	/* Message status starts out successful; set to -EIO on error */
>> +	message->status = 0;
>> +
>> +	/* Hold frame low to avoid losing transferred data */
>> +	val = readl(drv_data->base + SSP_TOP_CTRL);
>> +	val |= TOP_HOLD_FRAME_LOW;
>> +	writel(val, drv_data->base + SSP_TOP_CTRL);
>> +
>> +	list_for_each_entry(transfer, &message->transfers, transfer_list) {
>> +		reinit_completion(completion);
>> +
>> +		/* Issue the next transfer */
>> +		if (!k1_spi_transfer_start(drv_data, transfer)) {
>> +			message->status = -EIO;
>> +			break;
> we don't need return a error code?

You're right.  This always returns 0.  I think I'll
just return message->status.

Thank you for catching this.  I'll fix it in the next version.

					-Alex


> 
>> +		}
>> +
>> +		k1_spi_transfer_wait(drv_data);
>> +
>> +		k1_spi_transfer_end(drv_data, transfer);
>> +
>> +		/* If an error has occurred, we're done */
>> +		if (message->status)
>> +			break;
> ditto.
> 
> [...]
>> +}
>> +
>> +static void k1_spi_dma_cleanup(struct device *dev, void *res)
>> +{
>> +	struct k1_spi_driver_data *drv_data = res;
>> +
>> +	if (k1_spi_dma_enabled(drv_data)) {
> if (!k1_spi_dma_enabled(drv_data))
>    return;
> 
>                    - Troy
> 
>> +		k1_spi_dma_cleanup_io(drv_data, false);
>> +		k1_spi_dma_cleanup_io(drv_data, true);
>> +	}
>> +}
>> +
>> +
>> +static const struct of_device_id k1_spi_dt_ids[] = {
>> +	{ .compatible = "spacemit,k1-spi", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, k1_spi_dt_ids);
>> +
>> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct device_node *np = dev_of_node(drv_data->dev);
>> +	struct spi_controller *host = drv_data->controller;
>> +	struct device *dev = drv_data->dev;
>> +	u32 max_speed_hz;
>> +	int ret;
>> +
>> +	host->dev.of_node = np;
>> +	host->dev.parent = drv_data->dev;
>> +	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
>> +	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
>> +	host->num_chipselect = 1;
>> +
>> +	if (k1_spi_dma_enabled(drv_data))
>> +		host->dma_alignment = K1_SPI_DMA_ALIGNMENT;
>> +	host->cleanup = k1_spi_cleanup;
>> +	host->setup = k1_spi_setup;
>> +	host->transfer_one_message = k1_spi_transfer_one_message;
>> +
>> +	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
>> +	if (!ret) {
>> +		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
>> +					   K1_SPI_MAX_SPEED_HZ);
>> +		if (host->max_speed_hz != max_speed_hz)
>> +			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
>> +				max_speed_hz, host->max_speed_hz);
>> +	} else {
>> +		if (ret != -EINVAL)
>> +			dev_warn(dev, "bad spi-max-frequency, using %u\n",
>> +				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
>> +		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
>> +	}
>> +}
>> +
>> +/* Set our registers to a known initial state */
>> +static void
>> +k1_spi_register_reset(struct k1_spi_driver_data *drv_data, bool initial)
>> +{
>> +	u32 val = 0;
>> +
>> +	writel(0, drv_data->base + SSP_TOP_CTRL);
>> +
>> +	if (initial) {
>> +		/*
>> +		 * The TX and RX FIFO thresholds are the same no matter
>> +		 * what the speed or bits per word, so we can just set
>> +		 * them once.  The thresholds are one more than the values
>> +		 * in the register.
>> +		 */
>> +		val = FIELD_PREP(FIFO_RFT_MASK, K1_SPI_THRESH - 1);
>> +		val |= FIELD_PREP(FIFO_TFT_MASK, K1_SPI_THRESH - 1);
>> +	}
>> +	writel(val, drv_data->base + SSP_FIFO_CTRL);
>> +
>> +	writel(0, drv_data->base + SSP_INT_EN);
>> +	writel(0, drv_data->base + SSP_TIMEOUT);
>> +
>> +	/* Clear any pending interrupt conditions */
>> +	val = readl(drv_data->base + SSP_STATUS);
>> +	writel(val, drv_data->base + SSP_STATUS);
>> +}
>> +
>> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
>> +{
>> +	struct k1_spi_driver_data *drv_data = dev_id;
>> +	bool rx_done;
>> +	bool tx_done;
>> +	u32 val;
>> +
>> +	/* Get status and clear pending interrupts */
>> +	val = readl(drv_data->base + SSP_STATUS);
>> +	writel(val, drv_data->base + SSP_STATUS);
>> +
>> +	if (!drv_data->message)
>> +		return IRQ_NONE;
>> +
>> +	/* Check for a TX underrun or RX underrun first */
>> +	if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR)) {
>> +		/* Disable all interrupts on error */
>> +		writel(0, drv_data->base + SSP_INT_EN);
>> +
>> +		drv_data->message->status = -EIO;
>> +		complete(&drv_data->completion);
>> +
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	/* Drain the RX FIFO first, then transmit what we can */
>> +	rx_done = k1_spi_read(drv_data);
>> +	tx_done = k1_spi_write(drv_data);
>> +
>> +	/* Disable interrupts if we're done transferring either direction */
>> +	if (rx_done || tx_done) {
>> +		/* If both are done, disable all interrupts */
>> +		if (rx_done && tx_done) {
>> +			val = 0;
>> +		} else {
>> +			val = readl(drv_data->base + SSP_INT_EN);
>> +			if (rx_done)
>> +				val &= ~(SSP_INT_EN_TINTE | SSP_INT_EN_RIE);
>> +			if (tx_done)
>> +				val &= ~SSP_INT_EN_TIE;
>> +		}
>> +		writel(val, drv_data->base + SSP_INT_EN);
>> +	}
>> +
>> +	if (rx_done && tx_done)
>> +		complete(&drv_data->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int k1_spi_probe(struct platform_device *pdev)
>> +{
>> +	struct k1_spi_driver_data *drv_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *reset;
>> +	struct spi_controller *host;
>> +	struct resource *iores;
>> +	struct clk *clk_bus;
>> +	int ret;
>> +
>> +	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
>> +	if (!host)
>> +		return -ENOMEM;
>> +	drv_data = spi_controller_get_devdata(host);
>> +	drv_data->controller = host;
>> +	platform_set_drvdata(pdev, drv_data);
>> +	drv_data->dev = dev;
>> +	init_completion(&drv_data->completion);
>> +
>> +	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
>> +								&iores);
>> +	if (IS_ERR(drv_data->base))
>> +		return dev_err_probe(dev, PTR_ERR(drv_data->base),
>> +				     "error mapping memory\n");
>> +	drv_data->base_addr = iores->start;
>> +
>> +	ret = devm_k1_spi_dma_setup(drv_data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "error setting up DMA\n");
>> +
>> +	k1_spi_host_init(drv_data);
>> +
>> +	clk_bus = devm_clk_get_enabled(dev, "bus");
>> +	if (IS_ERR(clk_bus))
>> +		return dev_err_probe(dev, PTR_ERR(clk_bus),
>> +				     "error getting/enabling bus clock\n");
>> +	drv_data->bus_rate = clk_get_rate(clk_bus);
>> +
>> +	drv_data->clk = devm_clk_get_enabled(dev, "core");
>> +	if (IS_ERR(drv_data->clk))
>> +		return dev_err_probe(dev, PTR_ERR(drv_data->clk),
>> +				     "error getting/enabling core clock\n");
>> +
>> +	reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
>> +	if (IS_ERR(reset))
>> +		return dev_err_probe(dev, PTR_ERR(reset),
>> +				     "error getting/deasserting reset\n");
>> +
>> +	k1_spi_register_reset(drv_data, true);
>> +
>> +	drv_data->irq = platform_get_irq(pdev, 0);
>> +	if (drv_data->irq < 0)
>> +		return dev_err_probe(dev, drv_data->irq, "error getting IRQ\n");
>> +
>> +	ret = devm_request_irq(dev, drv_data->irq, k1_spi_ssp_isr,
>> +			       IRQF_SHARED, dev_name(dev), drv_data);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "error requesting IRQ\n");
>> +
>> +	ret = devm_spi_register_controller(dev, host);
>> +	if (ret)
>> +		dev_err(dev, "error registering controller\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static void k1_spi_remove(struct platform_device *pdev)
>> +{
>> +	struct k1_spi_driver_data *drv_data = platform_get_drvdata(pdev);
>> +
>> +	k1_spi_register_reset(drv_data, false);
>> +}
>> +
>> +static struct platform_driver k1_spi_driver = {
>> +	.driver = {
>> +		.name		= "k1-spi",
>> +		.of_match_table	= k1_spi_dt_ids,
>> +	},
>> +	.probe			= k1_spi_probe,
>> +	.remove			= k1_spi_remove,
>> +};
>> +
>> +module_platform_driver(k1_spi_driver);
>> +
>> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.48.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 3/3] riscv: dts: spacemit: define a SPI controller node

[Alex Elder]

Define a node for the fourth SoC SPI controller (number 3) on
the SpacemiT K1 SoC.

Enable it on the Banana Pi BPI-F3 board, which exposes this feature
via its GPIO block:
  GPIO PIN 19:  MOSI
  GPIO PIN 21:  MISO
  GPIO PIN 23:  SCLK
  GPIO PIN 24:  SS (inverted)

Define pincontrol configurations for the pins as used on that board.

(This was tested using a GigaDevice GD25Q64E SPI NOR chip.)

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - DT aliases are used rather than spacemit,k1-ssp-id for bus number
    - The order of two pin control properties was changed as requested
    - Clock names and DMA names are now on one line in the "k1.dtsi"
    - The interrupts property is used rather than interrupts-extended

 .../boot/dts/spacemit/k1-bananapi-f3.dts      |  7 +++++++
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  | 20 +++++++++++++++++++
 arch/riscv/boot/dts/spacemit/k1.dtsi          | 16 +++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 2aaaff77831e1..d9d865fbe320e 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -14,6 +14,7 @@ aliases {
 		ethernet0 = &eth0;
 		ethernet1 = &eth1;
 		serial0 = &uart0;
+		spi3 = &spi3;
 	};
 
 	chosen {
@@ -92,6 +93,12 @@ &pdma {
 	status = "okay";
 };
 
+&spi3 {
+	pinctrl-0 = <&ssp3_0_cfg>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_2_cfg>;
diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
index aff19c86d5ff3..205c201a3005c 100644
--- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
@@ -76,4 +76,24 @@ pwm14-1-pins {
 			drive-strength = <32>;
 		};
 	};
+
+	ssp3_0_cfg: ssp3-0-cfg {
+		ssp3-0-no-pull-pins {
+			pinmux = <K1_PADCONF(75, 2)>,	/* SCLK */
+				 <K1_PADCONF(77, 2)>,	/* MOSI  */
+				 <K1_PADCONF(78, 2)>;	/* MISO */
+
+			bias-disable;
+			drive-strength = <19>;
+			power-source = <3300>;
+		};
+
+		ssp3-0-frm-pins {
+			pinmux = <K1_PADCONF(76, 2)>;	/* FRM (frame) */
+
+			bias-pull-up = <0>;
+			drive-strength = <19>;
+			power-source = <3300>;
+		};
+	};
 };
diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 6cdcd80a7c83b..f8c37d16968e4 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -856,6 +856,22 @@ storage-bus {
 			#size-cells = <2>;
 			dma-ranges = <0x0 0x00000000 0x0 0x00000000 0x0 0x80000000>;
 
+			spi3: spi@d401c000 {
+				compatible = "spacemit,k1-spi";
+				reg = <0x0 0xd401c000 0x0 0x30>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				clocks = <&syscon_apbc CLK_SSP3>,
+					 <&syscon_apbc CLK_SSP3_BUS>;
+				clock-names = "core", "bus";
+				resets = <&syscon_apbc RESET_SSP3>;
+				interrupts = <55>;
+				dmas = <&pdma 20>,
+				       <&pdma 19>;
+				dma-names = "rx", "tx";
+				status = "disabled";
+			};
+
 			emmc: mmc@d4281000 {
 				compatible = "spacemit,k1-sdhci";
 				reg = <0x0 0xd4281000 0x0 0x200>;
-- 
2.48.1

Re: [PATCH v2 3/3] riscv: dts: spacemit: define a SPI controller node

[Vivian Wang]

On 9/19/25 23:59, Alex Elder wrote:

> [...]
>
> diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> index 6cdcd80a7c83b..f8c37d16968e4 100644
> --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> @@ -856,6 +856,22 @@ storage-bus {
>  			#size-cells = <2>;
>  			dma-ranges = <0x0 0x00000000 0x0 0x00000000 0x0 0x80000000>;
>  
> +			spi3: spi@d401c000 {
> +				compatible = "spacemit,k1-spi";
> +				reg = <0x0 0xd401c000 0x0 0x30>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&syscon_apbc CLK_SSP3>,
> +					 <&syscon_apbc CLK_SSP3_BUS>;
> +				clock-names = "core", "bus";
> +				resets = <&syscon_apbc RESET_SSP3>;
> +				interrupts = <55>;
> +				dmas = <&pdma 20>,
> +				       <&pdma 19>;
> +				dma-names = "rx", "tx";
> +				status = "disabled";
> +			};
> +

Is storage-bus the right place for SPI? I'd have thought that SPI
wouldn't need its own dma-ranges if it does DMA though &pdma.

I know "dram_range4" is where SpacemiT put it but I'm not sure if that
makes sense now.

Vivian "dramforever" Wang

Re: [PATCH v2 3/3] riscv: dts: spacemit: define a SPI controller node

[Alex Elder]

On 9/19/25 9:57 PM, Vivian Wang wrote:
> On 9/19/25 23:59, Alex Elder wrote:
> 
>> [...]
>>
>> diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
>> index 6cdcd80a7c83b..f8c37d16968e4 100644
>> --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
>> +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
>> @@ -856,6 +856,22 @@ storage-bus {
>>   			#size-cells = <2>;
>>   			dma-ranges = <0x0 0x00000000 0x0 0x00000000 0x0 0x80000000>;
>>   
>> +			spi3: spi@d401c000 {
>> +				compatible = "spacemit,k1-spi";
>> +				reg = <0x0 0xd401c000 0x0 0x30>;
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +				clocks = <&syscon_apbc CLK_SSP3>,
>> +					 <&syscon_apbc CLK_SSP3_BUS>;
>> +				clock-names = "core", "bus";
>> +				resets = <&syscon_apbc RESET_SSP3>;
>> +				interrupts = <55>;
>> +				dmas = <&pdma 20>,
>> +				       <&pdma 19>;
>> +				dma-names = "rx", "tx";
>> +				status = "disabled";
>> +			};
>> +
> 
> Is storage-bus the right place for SPI? I'd have thought that SPI
> wouldn't need its own dma-ranges if it does DMA though &pdma.
> 
> I know "dram_range4" is where SpacemiT put it but I'm not sure if that
> makes sense now.

You're right.  It belongs in the dma-bus region.  I will fix that in v3.

					-Alex

> 
> Vivian "dramforever" Wang
>