[PATCH v4 0/3] support for amlogic the new SPI IP

[PATCH v4 0/3] support for amlogic the new SPI IP

[Xianwei Zhao via B4 Relay]

Introduced support for the new SPI IP (SPISG). The SPISG is
a communication-oriented SPI controller from Amlogic,supporting
three operation modes: PIO, block DMA, and scatter-gather DMA.

Add the drivers and device tree bindings corresponding to the SPISG.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Changes in v4:
- Add resets prop and modify some formats for bindings.
- Remove irrelevant headers files and fix some issues.
- Link to v3: https://lore.kernel.org/r/20250623-spisg-v3-0-c731f57e289c@amlogic.com

Changes in v3:
- Rename of bit definition and fix some issues.
- Enable runtime_suspend function.
- Link to v2: https://lore.kernel.org/r/20250617-spisg-v2-0-51a605a84bd5@amlogic.com

Changes in v2:
- Use regmap to operation register and drop bitfied define.
- Use "SPISG" prefix intead of "SPICC", and declare clock div table in the spisg_device. 
- Delete other power operation functions except for runtime_supspend and runtime_resume.
- Fix some format corrections.
- Link to v1: https://lore.kernel.org/r/20250604-spisg-v1-0-5893dbe9d953@amlogic.com

---
Sunny Luo (2):
      spi: dt-bindings: Add binding document of Amlogic SPISG controller
      spi: Add Amlogic SPISG driver

Xianwei Zhao (1):
      MAINTAINERS: Add an entry for Amlogic spi driver

 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  |  59 ++
 MAINTAINERS                                        |   9 +
 drivers/spi/Kconfig                                |   9 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-amlogic-spisg.c                    | 871 +++++++++++++++++++++
 5 files changed, 949 insertions(+)
---
base-commit: bd30b995df8fd053e13d10f78dbc7b2fa5ed1aae
change-id: 20250603-spisg-78f21682ebac

Best regards,
-- 
Xianwei Zhao <xianwei.zhao@amlogic.com>



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

[PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller

[Xianwei Zhao via B4 Relay]

From: Sunny Luo <sunny.luo@amlogic.com>

The SPISG is a new communication oriented SPI controller of Amlogic, which
supports PIO, block DMA and scatter-gather DMA three operation modes.

Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
new file mode 100644
index 000000000000..9bfb8089f7ea
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2025 Amlogic, Inc. All rights reserved
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/amlogic,a4-spisg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic SPI Scatter-Gather Controller
+
+maintainers:
+  - Xianwei Zhao <xianwei.zhao@amlogic.com>
+  - Sunny Luo <sunny.luo@amlogic.com>
+
+allOf:
+  - $ref: spi-controller.yaml#
+
+properties:
+  compatible:
+    const: amlogic,a4-spisg
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: core
+      - const: pclk
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    spi@50000 {
+        compatible = "amlogic,a4-spisg";
+        reg = <0x50000 0x38>;
+        interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clkc 37>,
+                 <&clkc 93>;
+        clock-names = "core", "pclk";
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };

-- 
2.37.1



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

[PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao via B4 Relay]

From: Sunny Luo <sunny.luo@amlogic.com>

Introduced support for the new SPI IP (SPISG) driver. The SPISG is
a communication-oriented SPI controller from Amlogic,supporting
three operation modes: PIO, block DMA, and scatter-gather DMA.

Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 drivers/spi/Kconfig             |   9 +
 drivers/spi/Makefile            |   1 +
 drivers/spi/spi-amlogic-spisg.c | 871 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 881 insertions(+)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index c51da3fc3604..e11341df2ecf 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -99,6 +99,15 @@ config SPI_AMLOGIC_SPIFC_A1
 	  This enables master mode support for the SPIFC (SPI flash
 	  controller) available in Amlogic A1 (A113L SoC).
 
+config SPI_AMLOGIC_SPISG
+	tristate "Amlogic SPISG controller"
+	depends on COMMON_CLK
+	depends on ARCH_MESON || COMPILE_TEST
+	help
+	  This enables master mode support for the SPISG (SPI scatter-gather
+	  communication controller), which is available on platforms such as
+	  Amlogic A4 SoCs.
+
 config SPI_APPLE
 	tristate "Apple SoC SPI Controller platform driver"
 	depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 4ea89f6fc531..b74e3104d71f 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SPI_ALTERA)		+= spi-altera-platform.o
 obj-$(CONFIG_SPI_ALTERA_CORE)		+= spi-altera-core.o
 obj-$(CONFIG_SPI_ALTERA_DFL)		+= spi-altera-dfl.o
 obj-$(CONFIG_SPI_AMLOGIC_SPIFC_A1)	+= spi-amlogic-spifc-a1.o
+obj-$(CONFIG_SPI_AMLOGIC_SPISG)		+= spi-amlogic-spisg.o
 obj-$(CONFIG_SPI_APPLE)			+= spi-apple.o
 obj-$(CONFIG_SPI_AR934X)		+= spi-ar934x.o
 obj-$(CONFIG_SPI_ARMADA_3700)		+= spi-armada-3700.o
diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
new file mode 100644
index 000000000000..068fea095039
--- /dev/null
+++ b/drivers/spi/spi-amlogic-spisg.c
@@ -0,0 +1,871 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Amlogic SPI communication Scatter-Gather Controller
+ *
+ * Copyright (C) 2025 Amlogic, Inc. All rights reserved
+ *
+ * Author: Sunny Luo <sunny.luo@amlogic.com>
+ * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+
+/* Register Map */
+#define SPISG_REG_CFG_READY		0x00
+
+#define SPISG_REG_CFG_SPI		0x04
+#define CFG_BUS64_EN			BIT(0)
+#define CFG_SLAVE_EN			BIT(1)
+#define CFG_SLAVE_SELECT		GENMASK(3, 2)
+#define CFG_SFLASH_WP			BIT(4)
+#define CFG_SFLASH_HD			BIT(5)
+/* start on vsync rising */
+#define CFG_HW_POS			BIT(6)
+/* start on vsync falling */
+#define CFG_HW_NEG			BIT(7)
+
+#define SPISG_REG_CFG_START		0x08
+#define CFG_BLOCK_NUM			GENMASK(19, 0)
+#define CFG_BLOCK_SIZE			GENMASK(22, 20)
+#define CFG_DATA_COMMAND		BIT(23)
+#define CFG_OP_MODE			GENMASK(25, 24)
+#define CFG_RXD_MODE			GENMASK(27, 26)
+#define CFG_TXD_MODE			GENMASK(29, 28)
+#define CFG_EOC				BIT(30)
+#define CFG_PEND			BIT(31)
+
+#define SPISG_REG_CFG_BUS		0x0C
+#define CFG_CLK_DIV			GENMASK(7, 0)
+#define CLK_DIV_WIDTH			8
+#define CFG_RX_TUNING			GENMASK(11, 8)
+#define CFG_TX_TUNING			GENMASK(15, 12)
+#define CFG_CS_SETUP			GENMASK(19, 16)
+#define CFG_LANE			GENMASK(21, 20)
+#define CFG_HALF_DUPLEX			BIT(22)
+#define CFG_B_L_ENDIAN			BIT(23)
+#define CFG_DC_MODE			BIT(24)
+#define CFG_NULL_CTL			BIT(25)
+#define CFG_DUMMY_CTL			BIT(26)
+#define CFG_READ_TURN			GENMASK(28, 27)
+#define CFG_KEEP_SS			BIT(29)
+#define CFG_CPHA			BIT(30)
+#define CFG_CPOL			BIT(31)
+
+#define SPISG_REG_PIO_TX_DATA_L		0x10
+#define SPISG_REG_PIO_TX_DATA_H		0x14
+#define SPISG_REG_PIO_RX_DATA_L		0x18
+#define SPISG_REG_PIO_RX_DATA_H		0x1C
+#define SPISG_REG_MEM_TX_ADDR_L		0x10
+#define SPISG_REG_MEM_TX_ADDR_H		0x14
+#define SPISG_REG_MEM_RX_ADDR_L		0x18
+#define SPISG_REG_MEM_RX_ADDR_H		0x1C
+#define SPISG_REG_DESC_LIST_L		0x20
+#define SPISG_REG_DESC_LIST_H		0x24
+#define LIST_DESC_PENDING		BIT(31)
+#define SPISG_REG_DESC_CURRENT_L	0x28
+#define SPISG_REG_DESC_CURRENT_H	0x2c
+#define SPISG_REG_IRQ_STS		0x30
+#define SPISG_REG_IRQ_ENABLE		0x34
+#define IRQ_RCH_DESC_EOC		BIT(0)
+#define IRQ_RCH_DESC_INVALID		BIT(1)
+#define IRQ_RCH_DESC_RESP		BIT(2)
+#define IRQ_RCH_DATA_RESP		BIT(3)
+#define IRQ_WCH_DESC_EOC		BIT(4)
+#define IRQ_WCH_DESC_INVALID		BIT(5)
+#define IRQ_WCH_DESC_RESP		BIT(6)
+#define IRQ_WCH_DATA_RESP		BIT(7)
+#define IRQ_DESC_ERR			BIT(8)
+#define IRQ_SPI_READY			BIT(9)
+#define IRQ_DESC_DONE			BIT(10)
+#define IRQ_DESC_CHAIN_DONE		BIT(11)
+
+#define SPISG_MAX_REG			0x40
+
+#define SPISG_BLOCK_MAX			0x100000
+
+#define SPISG_OP_MODE_WRITE_CMD		0
+#define SPISG_OP_MODE_READ_STS		1
+#define SPISG_OP_MODE_WRITE		2
+#define SPISG_OP_MODE_READ		3
+
+#define SPISG_DATA_MODE_NONE		0
+#define SPISG_DATA_MODE_PIO		1
+#define SPISG_DATA_MODE_MEM		2
+#define SPISG_DATA_MODE_SG		3
+
+#define SPISG_CLK_DIV_MAX		256
+/* recommended by specification */
+#define SPISG_CLK_DIV_MIN		4
+#define DIV_NUM (SPISG_CLK_DIV_MAX - SPISG_CLK_DIV_MIN + 1)
+
+#define SPISG_PCLK_RATE_MIN		24000000
+
+#define SPISG_SINGLE_SPI		0
+#define SPISG_DUAL_SPI			1
+#define SPISG_QUAD_SPI			2
+
+struct spisg_sg_link {
+#define LINK_ADDR_VALID		BIT(0)
+#define LINK_ADDR_EOC		BIT(1)
+#define LINK_ADDR_IRQ		BIT(2)
+#define LINK_ADDR_ACT		GENMASK(5, 3)
+#define LINK_ADDR_RING		BIT(6)
+#define LINK_ADDR_LEN		GENMASK(31, 8)
+	u32			addr;
+	u32			addr1;
+};
+
+struct spisg_descriptor {
+	u32				cfg_start;
+	u32				cfg_bus;
+	u64				tx_paddr;
+	u64				rx_paddr;
+};
+
+struct spisg_descriptor_extra {
+	struct spisg_sg_link		*tx_ccsg;
+	struct spisg_sg_link		*rx_ccsg;
+	int				tx_ccsg_len;
+	int				rx_ccsg_len;
+};
+
+struct spisg_device {
+	struct spi_controller		*controller;
+	struct platform_device		*pdev;
+	struct regmap			*map;
+	struct clk			*core;
+	struct clk			*pclk;
+	struct clk			*sclk;
+	struct clk_div_table		*tbl;
+	struct completion		completion;
+	u32				status;
+	u32				speed_hz;
+	u32				effective_speed_hz;
+	u32				bytes_per_word;
+	u32				cfg_spi;
+	u32				cfg_start;
+	u32				cfg_bus;
+};
+
+static int spi_delay_to_sclk(u32 slck_speed_hz, struct spi_delay *delay)
+{
+	u32 ns;
+
+	if (!delay)
+		return 0;
+
+	if (delay->unit == SPI_DELAY_UNIT_SCK)
+		return delay->value;
+
+	ns = spi_delay_to_ns(delay, NULL);
+	if (ns < 0)
+		return 0;
+
+	return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
+}
+
+static inline u32 aml_spisg_sem_down_read(struct spisg_device *spisg)
+{
+	u32 ret;
+
+	regmap_read(spisg->map, SPISG_REG_CFG_READY, &ret);
+	if (ret)
+		regmap_write(spisg->map, SPISG_REG_CFG_READY, 0);
+
+	return ret;
+}
+
+static inline void aml_spisg_sem_up_write(struct spisg_device *spisg)
+{
+	regmap_write(spisg->map, SPISG_REG_CFG_READY, 1);
+}
+
+static int aml_spisg_set_speed(struct spisg_device *spisg, uint speed_hz)
+{
+	u32 cfg_bus;
+
+	if (!speed_hz || speed_hz == spisg->speed_hz)
+		return 0;
+
+	spisg->speed_hz = speed_hz;
+	clk_set_rate(spisg->sclk, speed_hz);
+	/* Store the div for the descriptor mode */
+	regmap_read(spisg->map, SPISG_REG_CFG_BUS, &cfg_bus);
+	spisg->cfg_bus &= ~CFG_CLK_DIV;
+	spisg->cfg_bus |= cfg_bus & CFG_CLK_DIV;
+	spisg->effective_speed_hz = clk_get_rate(spisg->sclk);
+	dev_dbg(&spisg->pdev->dev,
+		"desired speed %dHz, effective speed %dHz\n",
+		speed_hz, spisg->effective_speed_hz);
+
+	return 0;
+}
+
+static bool aml_spisg_can_dma(struct spi_controller *ctlr,
+			      struct spi_device *spi,
+			      struct spi_transfer *xfer)
+{
+	return true;
+}
+
+static void aml_spisg_sg_xlate(struct sg_table *sgt, struct spisg_sg_link *ccsg)
+{
+	struct scatterlist *sg;
+	int i;
+
+	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+		ccsg->addr = FIELD_PREP(LINK_ADDR_VALID, 1) |
+			     FIELD_PREP(LINK_ADDR_RING, 0) |
+			     FIELD_PREP(LINK_ADDR_EOC, sg_is_last(sg)) |
+			     FIELD_PREP(LINK_ADDR_LEN, sg_dma_len(sg));
+		ccsg->addr1 = (u32)sg_dma_address(sg);
+		ccsg++;
+	}
+}
+
+static int nbits_to_lane[] = {
+	SPISG_SINGLE_SPI,
+	SPISG_SINGLE_SPI,
+	SPISG_DUAL_SPI,
+	-EINVAL,
+	SPISG_QUAD_SPI
+};
+
+static int aml_spisg_setup_transfer(struct spisg_device *spisg,
+				    struct spi_transfer *xfer,
+				    struct spisg_descriptor *desc,
+				    struct spisg_descriptor_extra *exdesc)
+{
+	int block_size, blocks;
+	struct device *dev = &spisg->pdev->dev;
+	struct spisg_sg_link *ccsg;
+	int ccsg_len;
+	dma_addr_t paddr;
+	int ret;
+
+	memset(desc, 0, sizeof(*desc));
+	if (exdesc)
+		memset(exdesc, 0, sizeof(*exdesc));
+	aml_spisg_set_speed(spisg, xfer->speed_hz);
+	xfer->effective_speed_hz = spisg->effective_speed_hz;
+
+	desc->cfg_start = spisg->cfg_start;
+	desc->cfg_bus = spisg->cfg_bus;
+
+	block_size = xfer->bits_per_word >> 3;
+	blocks = xfer->len / block_size;
+
+	desc->cfg_start |= FIELD_PREP(CFG_EOC, 0);
+	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, !xfer->cs_change);
+	desc->cfg_bus |= FIELD_PREP(CFG_NULL_CTL, 0);
+
+	if (xfer->tx_buf || xfer->tx_dma) {
+		desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->tx_nbits]);
+		desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_WRITE);
+	}
+	if (xfer->rx_buf || xfer->rx_dma) {
+		desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->rx_nbits]);
+		desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_READ);
+	}
+
+	if (FIELD_GET(CFG_OP_MODE, desc->cfg_start) == SPISG_OP_MODE_READ_STS) {
+		desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, blocks) |
+				   FIELD_PREP(CFG_BLOCK_NUM, 1);
+	} else {
+		blocks = min_t(int, blocks, SPISG_BLOCK_MAX);
+		desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) |
+				   FIELD_PREP(CFG_BLOCK_NUM, blocks);
+	}
+
+	if (xfer->tx_sg.nents && xfer->tx_sg.sgl) {
+		ccsg_len = xfer->tx_sg.nents * sizeof(struct spisg_sg_link);
+		ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA);
+		if (!ccsg) {
+			dev_err(dev, "alloc tx_ccsg failed\n");
+			return -ENOMEM;
+		}
+
+		aml_spisg_sg_xlate(&xfer->tx_sg, ccsg);
+		paddr = dma_map_single(dev, (void *)ccsg,
+				       ccsg_len, DMA_TO_DEVICE);
+		ret = dma_mapping_error(dev, paddr);
+		if (ret) {
+			kfree(ccsg);
+			dev_err(dev, "tx ccsg map failed\n");
+			return ret;
+		}
+
+		desc->tx_paddr = paddr;
+		desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_SG);
+		exdesc->tx_ccsg = ccsg;
+		exdesc->tx_ccsg_len = ccsg_len;
+		dma_sync_sgtable_for_device(spisg->controller->cur_tx_dma_dev,
+					    &xfer->tx_sg, DMA_TO_DEVICE);
+	} else if (xfer->tx_buf || xfer->tx_dma) {
+		paddr = xfer->tx_dma;
+		if (!paddr) {
+			paddr = dma_map_single(dev, (void *)xfer->tx_buf,
+					       xfer->len, DMA_TO_DEVICE);
+			ret = dma_mapping_error(dev, paddr);
+			if (ret) {
+				dev_err(dev, "tx buf map failed\n");
+				return ret;
+			}
+		}
+		desc->tx_paddr = paddr;
+		desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_MEM);
+	}
+
+	if (xfer->rx_sg.nents && xfer->rx_sg.sgl) {
+		ccsg_len = xfer->rx_sg.nents * sizeof(struct spisg_sg_link);
+		ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA);
+		if (!ccsg) {
+			dev_err(dev, "alloc rx_ccsg failed\n");
+			return -ENOMEM;
+		}
+
+		aml_spisg_sg_xlate(&xfer->rx_sg, ccsg);
+		paddr = dma_map_single(dev, (void *)ccsg,
+				       ccsg_len, DMA_TO_DEVICE);
+		ret = dma_mapping_error(dev, paddr);
+		if (ret) {
+			kfree(ccsg);
+			dev_err(dev, "rx ccsg map failed\n");
+			return ret;
+		}
+
+		desc->rx_paddr = paddr;
+		desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_SG);
+		exdesc->rx_ccsg = ccsg;
+		exdesc->rx_ccsg_len = ccsg_len;
+		dma_sync_sgtable_for_device(spisg->controller->cur_rx_dma_dev,
+					    &xfer->rx_sg, DMA_FROM_DEVICE);
+	} else if (xfer->rx_buf || xfer->rx_dma) {
+		paddr = xfer->rx_dma;
+		if (!paddr) {
+			paddr = dma_map_single(dev, xfer->rx_buf,
+					       xfer->len, DMA_FROM_DEVICE);
+			ret = dma_mapping_error(dev, paddr);
+			if (ret) {
+				dev_err(dev, "rx buf map failed\n");
+				return ret;
+			}
+		}
+
+		desc->rx_paddr = paddr;
+		desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_MEM);
+	}
+
+	return 0;
+}
+
+static void aml_spisg_cleanup_transfer(struct spisg_device *spisg,
+				       struct spi_transfer *xfer,
+				       struct spisg_descriptor *desc,
+				       struct spisg_descriptor_extra *exdesc)
+{
+	struct device *dev = &spisg->pdev->dev;
+
+	if (desc->tx_paddr) {
+		if (FIELD_GET(CFG_TXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) {
+			dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr,
+					 exdesc->tx_ccsg_len, DMA_TO_DEVICE);
+			kfree(exdesc->tx_ccsg);
+			dma_sync_sgtable_for_cpu(spisg->controller->cur_tx_dma_dev,
+						 &xfer->tx_sg, DMA_TO_DEVICE);
+		} else if (!xfer->tx_dma) {
+			dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr,
+					 xfer->len, DMA_TO_DEVICE);
+		}
+	}
+
+	if (desc->rx_paddr) {
+		if (FIELD_GET(CFG_RXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) {
+			dma_unmap_single(dev, (dma_addr_t)desc->rx_paddr,
+					 exdesc->rx_ccsg_len, DMA_TO_DEVICE);
+			kfree(exdesc->rx_ccsg);
+			dma_sync_sgtable_for_cpu(spisg->controller->cur_rx_dma_dev,
+						 &xfer->rx_sg, DMA_FROM_DEVICE);
+		} else if (!xfer->rx_dma) {
+			dma_unmap_single(dev, (dma_addr_t)desc->rx_paddr,
+					 xfer->len, DMA_FROM_DEVICE);
+		}
+	}
+}
+
+static void aml_spisg_setup_null_desc(struct spisg_device *spisg,
+				      struct spisg_descriptor *desc,
+				      u32 n_sclk)
+{
+	/* unit is the last xfer sclk */
+	desc->cfg_start = spisg->cfg_start;
+	desc->cfg_bus = spisg->cfg_bus;
+
+	desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_WRITE) |
+			   FIELD_PREP(CFG_BLOCK_SIZE, 1) |
+			   FIELD_PREP(CFG_BLOCK_NUM, DIV_ROUND_UP(n_sclk, 8));
+
+	desc->cfg_bus |= FIELD_PREP(CFG_NULL_CTL, 1);
+}
+
+static void aml_spisg_pending(struct spisg_device *spisg,
+			      dma_addr_t desc_paddr,
+			      bool trig,
+			      bool irq_en)
+{
+	u32 desc_l, desc_h, cfg_spi;
+
+#ifdef	CONFIG_ARCH_DMA_ADDR_T_64BIT
+	desc_l = (u64)desc_paddr & 0xffffffff;
+	desc_h = (u64)desc_paddr >> 32;
+#else
+	desc_l = desc_paddr & 0xffffffff;
+	desc_h = 0;
+#endif
+
+	cfg_spi = spisg->cfg_spi;
+	if (trig)
+		cfg_spi |= CFG_HW_POS;
+	else
+		desc_h |= LIST_DESC_PENDING;
+
+	regmap_write(spisg->map, SPISG_REG_IRQ_ENABLE, irq_en ? IRQ_DESC_CHAIN_DONE : 0);
+	regmap_write(spisg->map, SPISG_REG_CFG_SPI, cfg_spi);
+	regmap_write(spisg->map, SPISG_REG_DESC_LIST_L, desc_l);
+	regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, desc_h);
+}
+
+static irqreturn_t aml_spisg_irq(int irq, void *data)
+{
+	struct spisg_device *spisg = (void *)data;
+	u32 sts;
+
+	spisg->status = 0;
+	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
+	regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts);
+	if (sts & (IRQ_RCH_DESC_INVALID |
+		   IRQ_RCH_DESC_RESP |
+		   IRQ_RCH_DATA_RESP |
+		   IRQ_WCH_DESC_INVALID |
+		   IRQ_WCH_DESC_RESP |
+		   IRQ_WCH_DATA_RESP |
+		   IRQ_DESC_ERR))
+		spisg->status = sts;
+
+	complete(&spisg->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
+					  struct spi_message *msg)
+{
+	struct spisg_device *spisg = spi_controller_get_devdata(ctlr);
+	struct device *dev = &spisg->pdev->dev;
+	unsigned long long ms = 0;
+	struct spi_transfer *xfer;
+	struct spisg_descriptor *descs, *desc;
+	struct spisg_descriptor_extra *exdescs, *exdesc;
+	dma_addr_t descs_paddr;
+	int desc_num = 1, descs_len;
+	u32 cs_hold_in_sclk = 0;
+	int ret = -EIO;
+
+	if (!aml_spisg_sem_down_read(spisg)) {
+		spi_finalize_current_message(ctlr);
+		dev_err(dev, "controller busy\n");
+		return -EBUSY;
+	}
+
+	/* calculate the desc num for all xfer */
+	list_for_each_entry(xfer, &msg->transfers, transfer_list)
+		desc_num++;
+
+	/* alloc descriptor/extra-descriptor table */
+	descs = kcalloc(desc_num, sizeof(*desc) + sizeof(*exdesc),
+			GFP_KERNEL | GFP_DMA);
+	if (!descs) {
+		spi_finalize_current_message(ctlr);
+		aml_spisg_sem_up_write(spisg);
+		return -ENOMEM;
+	}
+	descs_len = sizeof(*desc) * desc_num;
+	exdescs = (struct spisg_descriptor_extra *)(descs + desc_num);
+
+	/* config descriptor for each xfer */
+	desc = descs;
+	exdesc = exdescs;
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		ret = aml_spisg_setup_transfer(spisg, xfer, desc, exdesc);
+		if (ret) {
+			dev_err(dev, "config descriptor failed\n");
+			goto end;
+		}
+
+		/* calculate cs-setup delay with the first xfer speed */
+		if (list_is_first(&xfer->transfer_list, &msg->transfers))
+			desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP,
+				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup));
+
+		/* calculate cs-hold delay with the last xfer speed */
+		if (list_is_last(&xfer->transfer_list, &msg->transfers))
+			cs_hold_in_sclk =
+				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_hold);
+
+		desc++;
+		exdesc++;
+		ms += DIV_ROUND_UP_ULL(8LL * MSEC_PER_SEC * xfer->len,
+				       xfer->effective_speed_hz);
+	}
+
+	if (cs_hold_in_sclk)
+		/* additional null-descriptor to achieve the cs-hold delay */
+		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
+	else
+		desc--;
+
+	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0);
+	desc->cfg_start |= FIELD_PREP(CFG_EOC, 1);
+
+	/* some tolerances */
+	ms += ms + 20;
+	if (ms > UINT_MAX)
+		ms = UINT_MAX;
+
+	descs_paddr = dma_map_single(dev, (void *)descs,
+				     descs_len, DMA_TO_DEVICE);
+	ret = dma_mapping_error(dev, descs_paddr);
+	if (ret) {
+		dev_err(dev, "desc table map failed\n");
+		goto end;
+	}
+
+	reinit_completion(&spisg->completion);
+	aml_spisg_pending(spisg, descs_paddr, false, true);
+	if (wait_for_completion_timeout(&spisg->completion,
+					spi_controller_is_target(spisg->controller) ?
+					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms)))
+		ret = spisg->status ? -EIO : 0;
+	else
+		ret = -ETIMEDOUT;
+
+	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
+end:
+	desc = descs;
+	exdesc = exdescs;
+	list_for_each_entry(xfer, &msg->transfers, transfer_list)
+		aml_spisg_cleanup_transfer(spisg, xfer, desc++, exdesc++);
+	kfree(descs);
+
+	if (!ret)
+		msg->actual_length = msg->frame_length;
+	msg->status = ret;
+	spi_finalize_current_message(ctlr);
+	aml_spisg_sem_up_write(spisg);
+
+	return ret;
+}
+
+static int aml_spisg_prepare_message(struct spi_controller *ctlr,
+				     struct spi_message *message)
+{
+	struct spisg_device *spisg = spi_controller_get_devdata(ctlr);
+	struct spi_device *spi = message->spi;
+
+	if (!spi->bits_per_word || spi->bits_per_word % 8) {
+		dev_err(&spisg->pdev->dev, "invalid wordlen %d\n", spi->bits_per_word);
+		return -EINVAL;
+	}
+
+	spisg->bytes_per_word = spi->bits_per_word >> 3;
+
+	spisg->cfg_spi &= ~CFG_SLAVE_SELECT;
+	spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_SELECT, spi_get_chipselect(spi, 0));
+
+	spisg->cfg_bus &= ~(CFG_CPOL | CFG_CPHA | CFG_B_L_ENDIAN | CFG_HALF_DUPLEX);
+	spisg->cfg_bus |= FIELD_PREP(CFG_CPOL, !!(spi->mode & SPI_CPOL)) |
+			  FIELD_PREP(CFG_CPHA, !!(spi->mode & SPI_CPHA)) |
+			  FIELD_PREP(CFG_B_L_ENDIAN, !!(spi->mode & SPI_LSB_FIRST)) |
+			  FIELD_PREP(CFG_HALF_DUPLEX, !!(spi->mode & SPI_3WIRE));
+
+	return 0;
+}
+
+static int aml_spisg_setup(struct spi_device *spi)
+{
+	if (!spi->controller_state)
+		spi->controller_state = spi_controller_get_devdata(spi->controller);
+
+	return 0;
+}
+
+static void aml_spisg_cleanup(struct spi_device *spi)
+{
+	spi->controller_state = NULL;
+}
+
+static int aml_spisg_target_abort(struct spi_controller *ctlr)
+{
+	struct spisg_device *spisg = spi_controller_get_devdata(ctlr);
+
+	spisg->status = 0;
+	regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
+	complete(&spisg->completion);
+
+	return 0;
+}
+
+static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
+{
+	struct device *dev = &spisg->pdev->dev;
+	struct clk_init_data init;
+	struct clk_divider *div;
+	struct clk_div_table *tbl;
+	const char *parent_names[1];
+	char name[32];
+	int i;
+
+	spisg->core = devm_clk_get_enabled(dev, "core");
+	if (IS_ERR_OR_NULL(spisg->core)) {
+		dev_err(dev, "core clock request failed\n");
+		return PTR_ERR(spisg->core);
+	}
+
+	spisg->pclk = devm_clk_get_enabled(dev, "pclk");
+	if (IS_ERR_OR_NULL(spisg->pclk)) {
+		dev_err(dev, "pclk clock request failed\n");
+		return PTR_ERR(spisg->pclk);
+	}
+
+	clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);
+
+	clk_disable_unprepare(spisg->pclk);
+
+	tbl = devm_kzalloc(dev, sizeof(struct clk_div_table) * (DIV_NUM + 1), GFP_KERNEL);
+	if (!tbl)
+		return -ENOMEM;
+
+	for (i = 0; i < DIV_NUM; i++) {
+		tbl[i].val = i + SPISG_CLK_DIV_MIN - 1;
+		tbl[i].div = i + SPISG_CLK_DIV_MIN;
+	}
+	spisg->tbl = tbl;
+
+	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return -ENOMEM;
+
+	div->flags = CLK_DIVIDER_ROUND_CLOSEST;
+	div->reg = base + SPISG_REG_CFG_BUS;
+	div->shift = __bf_shf(CFG_CLK_DIV);
+	div->width = CLK_DIV_WIDTH;
+	div->table = tbl;
+
+	/* Register value should not be outside of the table */
+	regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
+			   FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
+
+	/* Register clk-divider */
+	parent_names[0] = __clk_get_name(spisg->pclk);
+	snprintf(name, sizeof(name), "%s_div", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_divider_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+	div->hw.init = &init;
+
+	spisg->sclk = devm_clk_register(dev, &div->hw);
+	if (IS_ERR_OR_NULL(spisg->sclk)) {
+		dev_err(dev, "clock registration failed\n");
+		return PTR_ERR(spisg->sclk);
+	}
+
+	clk_prepare_enable(spisg->sclk);
+
+	return 0;
+}
+
+static int aml_spisg_probe(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr;
+	struct spisg_device *spisg;
+	struct device *dev = &pdev->dev;
+	void __iomem *base;
+	int ret, irq;
+
+	const struct regmap_config aml_regmap_config = {
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+		.max_register = SPISG_MAX_REG,
+	};
+
+	if (of_property_read_bool(dev->of_node, "spi-slave"))
+		ctlr = spi_alloc_target(dev, sizeof(*spisg));
+	else
+		ctlr = spi_alloc_host(dev, sizeof(*spisg));
+	if (!ctlr)
+		return dev_err_probe(dev, -ENOMEM, "controller allocation failed\n");
+
+	spisg = spi_controller_get_devdata(ctlr);
+	spisg->controller = ctlr;
+
+	spisg->pdev = pdev;
+	platform_set_drvdata(pdev, spisg);
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base), "resource ioremap failed\n");
+
+	spisg->map = devm_regmap_init_mmio(dev, base, &aml_regmap_config);
+	if (IS_ERR(spisg->map))
+		return dev_err_probe(dev, PTR_ERR(spisg->map), "regmap init failed\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		goto out_controller;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
+	if (ret) {
+		dev_err(&pdev->dev, "irq request failed\n");
+		goto out_controller;
+	}
+
+	ret = aml_spisg_clk_init(spisg, base);
+	if (ret)
+		goto out_controller;
+
+	spisg->cfg_spi = 0;
+	spisg->cfg_start = 0;
+	spisg->cfg_bus = 0;
+
+	spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) |
+			 FIELD_PREP(CFG_SFLASH_HD, 1);
+	if (spi_controller_is_target(ctlr)) {
+		spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1);
+		spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf);
+	}
+	/* default pending */
+	spisg->cfg_start = FIELD_PREP(CFG_PEND, 1);
+
+	pm_runtime_set_active(&spisg->pdev->dev);
+	pm_runtime_enable(&spisg->pdev->dev);
+	pm_runtime_resume_and_get(&spisg->pdev->dev);
+
+	device_reset_optional(dev);
+	ctlr->num_chipselect = 4;
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST |
+			  SPI_3WIRE | SPI_TX_QUAD | SPI_RX_QUAD;
+	ctlr->max_speed_hz = 1000 * 1000 * 100;
+	ctlr->min_speed_hz = 1000 * 10;
+	ctlr->setup = aml_spisg_setup;
+	ctlr->cleanup = aml_spisg_cleanup;
+	ctlr->prepare_message = aml_spisg_prepare_message;
+	ctlr->transfer_one_message = aml_spisg_transfer_one_message;
+	ctlr->target_abort = aml_spisg_target_abort;
+	ctlr->can_dma = aml_spisg_can_dma;
+	ctlr->max_dma_len = SPISG_BLOCK_MAX;
+	ctlr->auto_runtime_pm = true;
+
+	dma_set_max_seg_size(&pdev->dev, SPISG_BLOCK_MAX);
+	ret = devm_spi_register_controller(dev, ctlr);
+	if (ret) {
+		dev_err(&pdev->dev, "spi controller registration failed\n");
+		goto out_clk;
+	}
+
+	init_completion(&spisg->completion);
+
+	pm_runtime_put(&spisg->pdev->dev);
+
+	return 0;
+out_clk:
+	if (spisg->core)
+		clk_disable_unprepare(spisg->core);
+	clk_disable_unprepare(spisg->pclk);
+out_controller:
+	spi_controller_put(ctlr);
+
+	return ret;
+}
+
+static void aml_spisg_remove(struct platform_device *pdev)
+{
+	struct spisg_device *spisg = platform_get_drvdata(pdev);
+
+	if (!pm_runtime_suspended(&pdev->dev)) {
+		pinctrl_pm_select_sleep_state(&spisg->pdev->dev);
+		clk_disable_unprepare(spisg->core);
+		clk_disable_unprepare(spisg->pclk);
+	}
+}
+
+static int spisg_suspend_runtime(struct device *dev)
+{
+	struct spisg_device *spisg = dev_get_drvdata(dev);
+
+	pinctrl_pm_select_sleep_state(&spisg->pdev->dev);
+	clk_disable_unprepare(spisg->sclk);
+	clk_disable_unprepare(spisg->core);
+
+	return 0;
+}
+
+static int spisg_resume_runtime(struct device *dev)
+{
+	struct spisg_device *spisg = dev_get_drvdata(dev);
+
+	clk_prepare_enable(spisg->core);
+	clk_prepare_enable(spisg->sclk);
+	pinctrl_pm_select_default_state(&spisg->pdev->dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops amlogic_spisg_pm_ops = {
+	.runtime_suspend	= spisg_suspend_runtime,
+	.runtime_resume		= spisg_resume_runtime,
+};
+
+static const struct of_device_id amlogic_spisg_of_match[] = {
+	{
+		.compatible = "amlogic,a4-spisg",
+	},
+
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, amlogic_spisg_of_match);
+
+static struct platform_driver amlogic_spisg_driver = {
+	.probe = aml_spisg_probe,
+	.remove = aml_spisg_remove,
+	.driver  = {
+		.name = "amlogic-spisg",
+		.of_match_table = amlogic_spisg_of_match,
+		.pm = &amlogic_spisg_pm_ops,
+	},
+};
+
+module_platform_driver(amlogic_spisg_driver);
+
+MODULE_DESCRIPTION("Amlogic SPI Scatter-Gather Controller driver");
+MODULE_AUTHOR("Sunny Luo <sunny.luo@amlogic.com>");
+MODULE_LICENSE("GPL");

-- 
2.37.1



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

[PATCH v4 3/3] MAINTAINERS: Add an entry for Amlogic spi driver

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Add Amlogic spi entry to MAINTAINERS to clarify the maintainers.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a92290fffa16..8225df5ede74 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1307,6 +1307,15 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/rtc/amlogic,a4-rtc.yaml
 F:	drivers/rtc/rtc-amlogic-a4.c
 
+AMLOGIC SPISG DRIVER
+M:	Sunny Luo <sunny.luo@amlogic.com>
+M:	Xianwei Zhao <xianwei.zhao@amlogic.com>
+L:	linux-amlogic@lists.infradead.org
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
+F:	drivers/spi/spi-amlogic-spisg.c
+
 AMPHENOL CHIPCAP 2 DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 L:	linux-hwmon@vger.kernel.org

-- 
2.37.1



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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Mark Brown]

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

On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote:

> Introduced support for the new SPI IP (SPISG) driver. The SPISG is
> a communication-oriented SPI controller from Amlogic,supporting
> three operation modes: PIO, block DMA, and scatter-gather DMA.

This looks good, a few small things below but nothing major.

> +static bool aml_spisg_can_dma(struct spi_controller *ctlr,
> +			      struct spi_device *spi,
> +			      struct spi_transfer *xfer)
> +{
> +	return true;
> +}

Is it worth having a copybreak such that smaller transfers are done
using PIO?  With a lot of controllers that increases performance due to
the extra overhead of setting up DMA, talking to the DMA and interrupt
controllers can be as expensive as directly accessing the FIFOs.

> +static irqreturn_t aml_spisg_irq(int irq, void *data)
> +{
> +	struct spisg_device *spisg = (void *)data;
> +	u32 sts;
> +
> +	spisg->status = 0;
> +	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
> +	regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts);
> +	if (sts & (IRQ_RCH_DESC_INVALID |
> +		   IRQ_RCH_DESC_RESP |
> +		   IRQ_RCH_DATA_RESP |
> +		   IRQ_WCH_DESC_INVALID |
> +		   IRQ_WCH_DESC_RESP |
> +		   IRQ_WCH_DATA_RESP |
> +		   IRQ_DESC_ERR))
> +		spisg->status = sts;
> +
> +	complete(&spisg->completion);
> +
> +	return IRQ_HANDLED;

It'd be better to check if there's an interrupt actually flagged and
return IRQ_NONE if not, as well as supporting sharing that means that
the interrupt core can handle any errors that cause the interrupt to
latch on.

> +	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
> +	if (ret) {
> +		dev_err(&pdev->dev, "irq request failed\n");
> +		goto out_controller;
> +	}
> +
> +	ret = aml_spisg_clk_init(spisg, base);
> +	if (ret)
> +		goto out_controller;

Do we need the clocks for register access - if so what happens if the
interrupt fires as soon as it is registered?  I'd have expected
requesting the interrupt to be one of the last things done.

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

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

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao]

Hi Mark,
    Thanks for your advice.

On 2025/7/7 21:05, Mark Brown wrote:
> Subject:
> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
> From:
> Mark Brown <broonie@kernel.org>
> Date:
> 2025/7/7 21:05
> 
> To:
> xianwei.zhao@amlogic.com
> CC:
> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, 
> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley 
> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, 
> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, 
> linux-kernel@vger.kernel.org
> 
> 
> 
> On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote:
> 
>> Introduced support for the new SPI IP (SPISG) driver. The SPISG is
>> a communication-oriented SPI controller from Amlogic,supporting
>> three operation modes: PIO, block DMA, and scatter-gather DMA.
> This looks good, a few small things below but nothing major.
> 
>> +static bool aml_spisg_can_dma(struct spi_controller *ctlr,
>> +			      struct spi_device *spi,
>> +			      struct spi_transfer *xfer)
>> +{
>> +	return true;
>> +}
> Is it worth having a copybreak such that smaller transfers are done
> using PIO?  With a lot of controllers that increases performance due to
> the extra overhead of setting up DMA, talking to the DMA and interrupt
> controllers can be as expensive as directly accessing the FIFOs.
> 

If the data volume of a single transfer (xfer) is small, PIO mode does 
offer some advantages. However, since PIO requires the CPU to wait in a 
busy loop for the transfer to complete, it continuously occupies CPU 
resources. As a result, its advantages are not particularly significant.

If PIO is to be implemented, it can only handle one transfer at a time 
(via transfer_one), and not entire messages (which consist of multiple 
transfers). In contrast, when processing messages, the SPI controller 
can handle the entire sequence in one go, which also provides certain 
benefits.

Taking all factors into account, it may be better not to add PIO support.

>> +static irqreturn_t aml_spisg_irq(int irq, void *data)
>> +{
>> +	struct spisg_device *spisg = (void *)data;
>> +	u32 sts;
>> +
>> +	spisg->status = 0;
>> +	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
>> +	regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts);
>> +	if (sts & (IRQ_RCH_DESC_INVALID |
>> +		   IRQ_RCH_DESC_RESP |
>> +		   IRQ_RCH_DATA_RESP |
>> +		   IRQ_WCH_DESC_INVALID |
>> +		   IRQ_WCH_DESC_RESP |
>> +		   IRQ_WCH_DATA_RESP |
>> +		   IRQ_DESC_ERR))
>> +		spisg->status = sts;
>> +
>> +	complete(&spisg->completion);
>> +
>> +	return IRQ_HANDLED;
> It'd be better to check if there's an interrupt actually flagged and
> return IRQ_NONE if not, as well as supporting sharing that means that
> the interrupt core can handle any errors that cause the interrupt to
> latch on.
>

Will do,unreasonable values will be returned IRQ_NONE.

>> +	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "irq request failed\n");
>> +		goto out_controller;
>> +	}
>> +
>> +	ret = aml_spisg_clk_init(spisg, base);
>> +	if (ret)
>> +		goto out_controller;
> Do we need the clocks for register access - if so what happens if the
> interrupt fires as soon as it is registered?  I'd have expected
> requesting the interrupt to be one of the last things done.

Will move irq request of the processing to a bit further back.

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Mark Brown]

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

On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote:
> On 2025/7/7 21:05, Mark Brown wrote:

> > Is it worth having a copybreak such that smaller transfers are done
> > using PIO?  With a lot of controllers that increases performance due to
> > the extra overhead of setting up DMA, talking to the DMA and interrupt
> > controllers can be as expensive as directly accessing the FIFOs.

> If the data volume of a single transfer (xfer) is small, PIO mode does offer
> some advantages. However, since PIO requires the CPU to wait in a busy loop
> for the transfer to complete, it continuously occupies CPU resources. As a
> result, its advantages are not particularly significant.

The CPU overhead tends to be higher (you can avoid some of it with a
dead reckoning sleep), but the latency vastly improved which for many
applications is a worthwhile advantage.  It tends to be things like
accesses to one or two registers on a device with registers where this
wins, 16 bytes or lower would be a common number off the top of my head.

> If PIO is to be implemented, it can only handle one transfer at a time (via
> transfer_one), and not entire messages (which consist of multiple
> transfers). In contrast, when processing messages, the SPI controller can
> handle the entire sequence in one go, which also provides certain benefits.

It's probably worth adding something to the framework to be able to take
a decision at the message level, for writes this tends to all fall out
naturally since the write will tend to be a single transfer anyway.

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

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

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Martin Blumenstingl]

Hi,

On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay
<devnull+xianwei.zhao.amlogic.com@kernel.org> wrote:
[...]
> +       div->table = tbl;
> +
> +       /* Register value should not be outside of the table */
> +       regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
> +                          FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
Are you doing this to prevent errors for value zero?
If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been
discussed for the t7 clock controller recently: [0])?

> +       /* Register clk-divider */
> +       parent_names[0] = __clk_get_name(spisg->pclk);
Instead of using __clk_get_name my suggestion is to use struct
clk_parent_data with .fw_name set.
If you want to simplify the code further you can use helper macros
like CLK_HW_INIT_FW_NAME

> +       snprintf(name, sizeof(name), "%s_div", dev_name(dev));
> +       init.name = name;
> +       init.ops = &clk_divider_ops;
> +       init.flags = CLK_SET_RATE_PARENT;
> +       init.parent_names = parent_names;
> +       init.num_parents = 1;
> +       div->hw.init = &init;
> +
> +       spisg->sclk = devm_clk_register(dev, &div->hw);
My understanding is that devm_clk_register() is not recommended for new drivers.
The replacement is to use devm_clk_hw_register() first, then
devm_clk_hw_get_clk(). drivers/pwm/pwm-meson.c implements this in case
you're looking for an example


[...]
> +static int aml_spisg_probe(struct platform_device *pdev)
> +{
> +       struct spi_controller *ctlr;
> +       struct spisg_device *spisg;
> +       struct device *dev = &pdev->dev;
> +       void __iomem *base;
> +       int ret, irq;
> +
> +       const struct regmap_config aml_regmap_config = {
> +               .reg_bits = 32,
> +               .val_bits = 32,
> +               .reg_stride = 4,
> +               .max_register = SPISG_MAX_REG,
> +       };
Most regmap_configs in Amlogic drivers are static const.
If you make it a static const then I suggest renaming the variable to
aml_spisg_regmap_config for consistency.

[...]
> +       device_reset_optional(dev);
I haven't checked the reset code but I think the return value still
needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so).
Even though the reset is optional there's conditions where we must act
for example on -EPROBE_DEFER (which must be propagated).


Best regards,
Martin


[0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao]

Hi Martin,
    Thanks for your reply.

On 2025/7/9 00:01, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay
> <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote:
> [...]
>> +       div->table = tbl;
>> +
>> +       /* Register value should not be outside of the table */
>> +       regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
>> +                          FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
> Are you doing this to prevent errors for value zero?
> If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been
> discussed for the t7 clock controller recently: [0])?
> 

No, if add add flag CLK_DIVIDER_MAX_AT_ZERO, reg value will equals 
divider, see
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/clk-divider.c?h=next-20250708
As follow in function _get_div.
         "if (flags & CLK_DIVIDER_MAX_AT_ZERO)
                 return val ? val : clk_div_mask(width) + 1;"

But here reg value adding one equals divider.

>> +       /* Register clk-divider */
>> +       parent_names[0] = __clk_get_name(spisg->pclk);
> Instead of using __clk_get_name my suggestion is to use struct
> clk_parent_data with .fw_name set.
> If you want to simplify the code further you can use helper macros
> like CLK_HW_INIT_FW_NAME

here I don't know parent fw_name, The system does not have a 
corresponding interface to obtain parent fw_name, only name (clk-core->name)

> 
>> +       snprintf(name, sizeof(name), "%s_div", dev_name(dev));
>> +       init.name = name;
>> +       init.ops = &clk_divider_ops;
>> +       init.flags = CLK_SET_RATE_PARENT;
>> +       init.parent_names = parent_names;
>> +       init.num_parents = 1;
>> +       div->hw.init = &init;
>> +
>> +       spisg->sclk = devm_clk_register(dev, &div->hw);
> My understanding is that devm_clk_register() is not recommended for new drivers.
> The replacement is to use devm_clk_hw_register() first, then
> devm_clk_hw_get_clk(). drivers/pwm/pwm-meson.c implements this in case
> you're looking for an example
> 

Will do.

> 
> [...]
>> +static int aml_spisg_probe(struct platform_device *pdev)
>> +{
>> +       struct spi_controller *ctlr;
>> +       struct spisg_device *spisg;
>> +       struct device *dev = &pdev->dev;
>> +       void __iomem *base;
>> +       int ret, irq;
>> +
>> +       const struct regmap_config aml_regmap_config = {
>> +               .reg_bits = 32,
>> +               .val_bits = 32,
>> +               .reg_stride = 4,
>> +               .max_register = SPISG_MAX_REG,
>> +       };
> Most regmap_configs in Amlogic drivers are static const.
> If you make it a static const then I suggest renaming the variable to
> aml_spisg_regmap_config for consistency.
> 

regmap_config as a local variable, it can save space.

> [...]
>> +       device_reset_optional(dev);
> I haven't checked the reset code but I think the return value still
> needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so).
> Even though the reset is optional there's conditions where we must act
> for example on -EPROBE_DEFER (which must be propagated).
> 

The reset might not exist for this node, see relevant yaml file.

> 
> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao]

Hi Mark,
    Thanks for your advice.

On 2025/7/8 21:50, Mark Brown wrote:
> Subject:
> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
> From:
> Mark Brown <broonie@kernel.org>
> Date:
> 2025/7/8 21:50
> 
> To:
> Xianwei Zhao <xianwei.zhao@amlogic.com>
> CC:
> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, 
> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley 
> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, 
> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, 
> linux-kernel@vger.kernel.org
> 
> 
> 
> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote:
>> On 2025/7/7 21:05, Mark Brown wrote:
>>> Is it worth having a copybreak such that smaller transfers are done
>>> using PIO?  With a lot of controllers that increases performance due to
>>> the extra overhead of setting up DMA, talking to the DMA and interrupt
>>> controllers can be as expensive as directly accessing the FIFOs.
>> If the data volume of a single transfer (xfer) is small, PIO mode does offer
>> some advantages. However, since PIO requires the CPU to wait in a busy loop
>> for the transfer to complete, it continuously occupies CPU resources. As a
>> result, its advantages are not particularly significant.
> The CPU overhead tends to be higher (you can avoid some of it with a
> dead reckoning sleep), but the latency vastly improved which for many
> applications is a worthwhile advantage.  It tends to be things like
> accesses to one or two registers on a device with registers where this
> wins, 16 bytes or lower would be a common number off the top of my head.
>
>> If PIO is to be implemented, it can only handle one transfer at a time (via
>> transfer_one), and not entire messages (which consist of multiple
>> transfers). In contrast, when processing messages, the SPI controller can
>> handle the entire sequence in one go, which also provides certain benefits.
> It's probably worth adding something to the framework to be able to take
> a decision at the message level, for writes this tends to all fall out
> naturally since the write will tend to be a single transfer anyway.

I will try to add new API message_can_dma for framework, and implement 
PIO for message.


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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Martin Blumenstingl]

Hi,

On Wed, Jul 9, 2025 at 8:29 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote:
>
> Hi Martin,
>     Thanks for your reply.
>
> On 2025/7/9 00:01, Martin Blumenstingl wrote:
> > [ EXTERNAL EMAIL ]
> >
> > Hi,
> >
> > On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay
> > <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote:
> > [...]
> >> +       div->table = tbl;
> >> +
> >> +       /* Register value should not be outside of the table */
> >> +       regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
> >> +                          FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
> > Are you doing this to prevent errors for value zero?
> > If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been
> > discussed for the t7 clock controller recently: [0])?
> >
>
> No, if add add flag CLK_DIVIDER_MAX_AT_ZERO, reg value will equals
> divider, see
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/clk-divider.c?h=next-20250708
> As follow in function _get_div.
>          "if (flags & CLK_DIVIDER_MAX_AT_ZERO)
>                  return val ? val : clk_div_mask(width) + 1;"
>
> But here reg value adding one equals divider.
I see, thanks.

> >> +       /* Register clk-divider */
> >> +       parent_names[0] = __clk_get_name(spisg->pclk);
> > Instead of using __clk_get_name my suggestion is to use struct
> > clk_parent_data with .fw_name set.
> > If you want to simplify the code further you can use helper macros
> > like CLK_HW_INIT_FW_NAME
>
> here I don't know parent fw_name, The system does not have a
> corresponding interface to obtain parent fw_name, only name (clk-core->name)
The .fw_name is simply "pclk" in this case.
That clock is then internally looked up (by the common clock
framework) using the "clock-names" and "clocks" properties from your
device-tree.

[...]
> >> +static int aml_spisg_probe(struct platform_device *pdev)
> >> +{
> >> +       struct spi_controller *ctlr;
> >> +       struct spisg_device *spisg;
> >> +       struct device *dev = &pdev->dev;
> >> +       void __iomem *base;
> >> +       int ret, irq;
> >> +
> >> +       const struct regmap_config aml_regmap_config = {
> >> +               .reg_bits = 32,
> >> +               .val_bits = 32,
> >> +               .reg_stride = 4,
> >> +               .max_register = SPISG_MAX_REG,
> >> +       };
> > Most regmap_configs in Amlogic drivers are static const.
> > If you make it a static const then I suggest renaming the variable to
> > aml_spisg_regmap_config for consistency.
> >
>
> regmap_config as a local variable, it can save space.
Thanks, I was not aware of that.
For documentation purposes, with the original approach:
$ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko
  text    data     bss     dec     hex filename
  7504    1377       0    8881    22b1 drivers/spi/spi-amlogic-spisg.ko

and making the regmap_config static const:
$ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko
  text    data     bss     dec     hex filename
  7716    1377       0    9093    2385 drivers/spi/spi-amlogic-spisg.ko

[...]
> >> +       device_reset_optional(dev);
> > I haven't checked the reset code but I think the return value still
> > needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so).
> > Even though the reset is optional there's conditions where we must act
> > for example on -EPROBE_DEFER (which must be propagated).
> >
>
> The reset might not exist for this node, see relevant yaml file.
This part I understand. Optional however doesn't mean that we can
simply ignore all errors.
Let's consider the following three scenarios:
1) reset not provided in .dtb -> we don't expect any error here
2) reset is provided in .dtb but the reset-controller has not been
registered -> spisg driver must propagate -EPROBE_DEFER
3) reset is provided in .dtb but reset_control_reset (which is used
internally in device_reset_optional) returns an error -> spisg must
propagate this error

Your code works for the first case but it doesn't consider the other two.
That said, I'm not sure if the third case is realistic given the T7
reset controller uses MMIO access. This may change in future though
(if SPISG IP is the same but the reset controller changes).
It still leaves the second case where the spisg driver should be
probed only after the reset controller is available.


Best regards,
Martin

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao]

Hi Mark,

On 2025/7/9 15:02, Xianwei Zhao wrote:
> Hi Mark,
>     Thanks for your advice.
> 
> On 2025/7/8 21:50, Mark Brown wrote:
>> Subject:
>> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
>> From:
>> Mark Brown <broonie@kernel.org>
>> Date:
>> 2025/7/8 21:50
>>
>> To:
>> Xianwei Zhao <xianwei.zhao@amlogic.com>
>> CC:
>> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, 
>> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley 
>> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, 
>> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, 
>> linux-kernel@vger.kernel.org
>>
>>
>>
>> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote:
>>> On 2025/7/7 21:05, Mark Brown wrote:
>>>> Is it worth having a copybreak such that smaller transfers are done
>>>> using PIO?  With a lot of controllers that increases performance due to
>>>> the extra overhead of setting up DMA, talking to the DMA and interrupt
>>>> controllers can be as expensive as directly accessing the FIFOs.
>>> If the data volume of a single transfer (xfer) is small, PIO mode 
>>> does offer
>>> some advantages. However, since PIO requires the CPU to wait in a 
>>> busy loop
>>> for the transfer to complete, it continuously occupies CPU resources. 
>>> As a
>>> result, its advantages are not particularly significant.
>> The CPU overhead tends to be higher (you can avoid some of it with a
>> dead reckoning sleep), but the latency vastly improved which for many
>> applications is a worthwhile advantage.  It tends to be things like
>> accesses to one or two registers on a device with registers where this
>> wins, 16 bytes or lower would be a common number off the top of my head.
>>
>>> If PIO is to be implemented, it can only handle one transfer at a 
>>> time (via
>>> transfer_one), and not entire messages (which consist of multiple
>>> transfers). In contrast, when processing messages, the SPI controller 
>>> can
>>> handle the entire sequence in one go, which also provides certain 
>>> benefits.
>> It's probably worth adding something to the framework to be able to take
>> a decision at the message level, for writes this tends to all fall out
>> naturally since the write will tend to be a single transfer anyway.
> 
> I will try to add new API message_can_dma for framework, and implement 
> PIO for message.
> 

I tried to implement PIO mode in the driver, but it turned out to be too 
slow. Due to the lack of an internal FIFO, data could only be 
transmitted one word at a time, and each transmission required 
reconfiguring the corresponding registers. As a result, the efficiency 
was quite low.

The only simplifications provided by PIO mode were in two areas:

1. The allocation and release of the transfer descriptor
2. The DMA mapping and unmapping process

Therefore, I suggest not implementing PIO mode in this driver. I will 
document clearly in the code that PIO mode is not supported and explain 
the reasons behind this decision.

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Da Xue]

On Wed, Jul 16, 2025 at 5:30 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote:
>
> Hi Mark,
>
> On 2025/7/9 15:02, Xianwei Zhao wrote:
> > Hi Mark,
> >     Thanks for your advice.
> >
> > On 2025/7/8 21:50, Mark Brown wrote:
> >> Subject:
> >> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
> >> From:
> >> Mark Brown <broonie@kernel.org>
> >> Date:
> >> 2025/7/8 21:50
> >>
> >> To:
> >> Xianwei Zhao <xianwei.zhao@amlogic.com>
> >> CC:
> >> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>,
> >> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
> >> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org,
> >> linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
> >> linux-kernel@vger.kernel.org
> >>
> >>
> >>
> >> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote:
> >>> On 2025/7/7 21:05, Mark Brown wrote:
> >>>> Is it worth having a copybreak such that smaller transfers are done
> >>>> using PIO?  With a lot of controllers that increases performance due to
> >>>> the extra overhead of setting up DMA, talking to the DMA and interrupt
> >>>> controllers can be as expensive as directly accessing the FIFOs.
> >>> If the data volume of a single transfer (xfer) is small, PIO mode
> >>> does offer
> >>> some advantages. However, since PIO requires the CPU to wait in a
> >>> busy loop
> >>> for the transfer to complete, it continuously occupies CPU resources.
> >>> As a
> >>> result, its advantages are not particularly significant.
> >> The CPU overhead tends to be higher (you can avoid some of it with a
> >> dead reckoning sleep), but the latency vastly improved which for many
> >> applications is a worthwhile advantage.  It tends to be things like
> >> accesses to one or two registers on a device with registers where this
> >> wins, 16 bytes or lower would be a common number off the top of my head.
> >>
> >>> If PIO is to be implemented, it can only handle one transfer at a
> >>> time (via
> >>> transfer_one), and not entire messages (which consist of multiple
> >>> transfers). In contrast, when processing messages, the SPI controller
> >>> can
> >>> handle the entire sequence in one go, which also provides certain
> >>> benefits.
> >> It's probably worth adding something to the framework to be able to take
> >> a decision at the message level, for writes this tends to all fall out
> >> naturally since the write will tend to be a single transfer anyway.
> >
> > I will try to add new API message_can_dma for framework, and implement
> > PIO for message.
> >
>
> I tried to implement PIO mode in the driver, but it turned out to be too
> slow. Due to the lack of an internal FIFO, data could only be
> transmitted one word at a time, and each transmission required
> reconfiguring the corresponding registers. As a result, the efficiency
> was quite low.

One of the use cases is for SPI-based displays and it transfers one
word via PIO and then a lot of words via DMA. I see PIO as beneficial
for this common use case for SPI.
The efficiency does not need to be high for this one word but reducing
the latency for DMA setup is a significant gain.

>
> The only simplifications provided by PIO mode were in two areas:
>
> 1. The allocation and release of the transfer descriptor
> 2. The DMA mapping and unmapping process
>
> Therefore, I suggest not implementing PIO mode in this driver. I will
> document clearly in the code that PIO mode is not supported and explain
> the reasons behind this decision.
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao]

On 2025/7/17 00:25, Da Xue wrote:
> [ EXTERNAL EMAIL ]
> 
> On Wed, Jul 16, 2025 at 5:30 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote:
>>
>> Hi Mark,
>>
>> On 2025/7/9 15:02, Xianwei Zhao wrote:
>>> Hi Mark,
>>>      Thanks for your advice.
>>>
>>> On 2025/7/8 21:50, Mark Brown wrote:
>>>> Subject:
>>>> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
>>>> From:
>>>> Mark Brown <broonie@kernel.org>
>>>> Date:
>>>> 2025/7/8 21:50
>>>>
>>>> To:
>>>> Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>> CC:
>>>> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>,
>>>> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
>>>> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org,
>>>> linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
>>>> linux-kernel@vger.kernel.org
>>>>
>>>>
>>>>
>>>> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote:
>>>>> On 2025/7/7 21:05, Mark Brown wrote:
>>>>>> Is it worth having a copybreak such that smaller transfers are done
>>>>>> using PIO?  With a lot of controllers that increases performance due to
>>>>>> the extra overhead of setting up DMA, talking to the DMA and interrupt
>>>>>> controllers can be as expensive as directly accessing the FIFOs.
>>>>> If the data volume of a single transfer (xfer) is small, PIO mode
>>>>> does offer
>>>>> some advantages. However, since PIO requires the CPU to wait in a
>>>>> busy loop
>>>>> for the transfer to complete, it continuously occupies CPU resources.
>>>>> As a
>>>>> result, its advantages are not particularly significant.
>>>> The CPU overhead tends to be higher (you can avoid some of it with a
>>>> dead reckoning sleep), but the latency vastly improved which for many
>>>> applications is a worthwhile advantage.  It tends to be things like
>>>> accesses to one or two registers on a device with registers where this
>>>> wins, 16 bytes or lower would be a common number off the top of my head.
>>>>
>>>>> If PIO is to be implemented, it can only handle one transfer at a
>>>>> time (via
>>>>> transfer_one), and not entire messages (which consist of multiple
>>>>> transfers). In contrast, when processing messages, the SPI controller
>>>>> can
>>>>> handle the entire sequence in one go, which also provides certain
>>>>> benefits.
>>>> It's probably worth adding something to the framework to be able to take
>>>> a decision at the message level, for writes this tends to all fall out
>>>> naturally since the write will tend to be a single transfer anyway.
>>>
>>> I will try to add new API message_can_dma for framework, and implement
>>> PIO for message.
>>>
>>
>> I tried to implement PIO mode in the driver, but it turned out to be too
>> slow. Due to the lack of an internal FIFO, data could only be
>> transmitted one word at a time, and each transmission required
>> reconfiguring the corresponding registers. As a result, the efficiency
>> was quite low.
> 
> One of the use cases is for SPI-based displays and it transfers one
> word via PIO and then a lot of words via DMA. I see PIO as beneficial
> for this common use case for SPI.
> The efficiency does not need to be high for this one word but reducing
> the latency for DMA setup is a significant gain.
> 

For the new SPISG driver, regardless of the mode(PIO, DMA(MEM or sg), 
the configuration registers are the same; the only difference value  in 
the mode selection. see reg cfg_start.

>>
>> The only simplifications provided by PIO mode were in two areas:
>>
>> 1. The allocation and release of the transfer descriptor
>> 2. The DMA mapping and unmapping process
>>
>> Therefore, I suggest not implementing PIO mode in this driver. I will
>> document clearly in the code that PIO mode is not supported and explain
>> the reasons behind this decision.
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic

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