Re: [PATCH 2/3] spi: sophgo: add Sophgo SPI NOR controller driver

On Mon, Feb 24, 2025 at 01:21:15PM +0000, Yixun Lan wrote:

Hi Longbin:

On 18:12 Mon 24 Feb     , Longbin Li wrote:

quoted

Add support for Sophgo SPI NOR controller in Sophgo SoC.

Signed-off-by: Longbin Li <redacted>
---
 drivers/spi/Kconfig          |   9 +
 drivers/spi/Makefile         |   1 +
 drivers/spi/spi-sophgo-nor.c | 501 +++++++++++++++++++++++++++++++++++
 3 files changed, 511 insertions(+)
 create mode 100644 drivers/spi/spi-sophgo-nor.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ea8a31032927..6b6d7b348485 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig

@@ -1021,6 +1021,15 @@ config SPI_SN_F_OSPI
 	  for connecting an SPI Flash memory over up to 8-bit wide bus.
 	  It supports indirect access mode only.

+config SPI_SOPHGO_NOR
+	tristate "Sophgo SPI NOR Controller"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  This enables support for the Sophgo SPI NOR controller,
+	  which supports Dual/Qual read and write operations while
+	  also supporting 3Byte address devices and 4Byte address
+	  devices.
+
 config SPI_SPRD
 	tristate "Spreadtrum SPI controller"
 	depends on ARCH_SPRD || COMPILE_TEST

diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 9db7554c1864..9ded1de4b2fd 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile

@@ -134,6 +134,7 @@ obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
 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_SOPHGO_NOR)	+= spi-sophgo-nor.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-sophgo-nor.c b/drivers/spi/spi-sophgo-nor.c
new file mode 100644
index 000000000000..1139deeac327
--- /dev/null
+++ b/drivers/spi/spi-sophgo-nor.c

@@ -0,0 +1,501 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Sophgo SPI NOR controller driver
+ *
+ * Copyright (c) 2025 Longbin Li <looong.bin@gmail.com>
+ */
+

[...]
+struct sophgo_spifmc {
+	struct spi_controller *ctrl;
+	void __iomem *io_base;
+	struct device *dev;
+	struct mutex lock;

it will be great to document the lock

Thanks, but I don't think the function of this lock is complicated,
adding comments may be useless.

quoted

+	struct clk *clk;
+};
+
+static int sophgo_spifmc_wait_int(struct sophgo_spifmc *spifmc, u8 int_type)
+{
+	u32 stat;
+
+	return readl_poll_timeout(spifmc->io_base + SPIFMC_INT_STS, stat,
+				  (stat & int_type), 0, 1000000);
+}
+
+
[...]
+static ssize_t sophgo_spifmc_trans_reg(struct sophgo_spifmc *spifmc,
+				       const struct spi_mem_op *op)
+{
+	const u8 *dout = NULL;
+	u8 *din = NULL;
+	size_t len = op->data.nbytes;
+	u32 reg;
+	int ret;
+	int i;

squash them which save one line:
	int i, ret;

Thanks, I will modify it.

quoted

+
+	if (op->data.dir == SPI_MEM_DATA_IN)
+		din = op->data.buf.in;
+	else
+		dout = op->data.buf.out;
+
+	reg = sophgo_spifmc_init_reg(spifmc);
+	reg |= SPIFMC_TRAN_CSR_FIFO_TRG_LVL_1_BYTE;
+	reg |= SPIFMC_TRAN_CSR_WITH_CMD;
+
+	if (din) {
+		reg |= SPIFMC_TRAN_CSR_BUS_WIDTH_1_BIT;
+		reg |= SPIFMC_TRAN_CSR_TRAN_MODE_RX;
+		reg |= SPIFMC_TRAN_CSR_TRAN_MODE_TX;
+
+		writel(SPIFMC_OPT_DISABLE_FIFO_FLUSH, spifmc->io_base + SPIFMC_OPT);
+	} else {
+		/*
+		 * If write values to the Status Register,
+		 * configure TRAN_CSR register as the same as
+		 * sophgo_spifmc_read_reg.
+		 */
+		if (op->cmd.opcode == 0x01) {
+			reg |= SPIFMC_TRAN_CSR_TRAN_MODE_RX;
+			reg |= SPIFMC_TRAN_CSR_TRAN_MODE_TX;
+			writel(len, spifmc->io_base + SPIFMC_TRAN_NUM);
+		}
+	}
+
+
[...]
+static const struct spi_controller_mem_ops sophgo_spifmc_mem_ops = {
+	.exec_op = sophgo_spifmc_exec_op,
+};
+
+static void sophgo_spifmc_init(struct sophgo_spifmc *spifmc)
+{
+	u32 tran_csr;
+	u32 reg;
+
+	writel(0, spifmc->io_base + SPIFMC_DMMR);
+
+	reg = readl(spifmc->io_base + SPIFMC_CTRL);
+	reg |= SPIFMC_CTRL_SRST;

..

quoted

+	reg &= ~((1 << 11) - 1);

so this is a mask? use macro to define, instead of using magic number

Yes, this is a mask to init SPI Clock Divider, I will add a macro, thanks.

quoted

+	reg |= 1;
+	writel(reg, spifmc->io_base + SPIFMC_CTRL);
+
+	writel(0, spifmc->io_base + SPIFMC_CE_CTRL);
+
+	tran_csr = readl(spifmc->io_base + SPIFMC_TRAN_CSR);
+	tran_csr |= (0 << SPIFMC_TRAN_CSR_ADDR_BYTES_SHIFT);
+	tran_csr |= SPIFMC_TRAN_CSR_FIFO_TRG_LVL_4_BYTE;
+	tran_csr |= SPIFMC_TRAN_CSR_WITH_CMD;
+	writel(tran_csr, spifmc->io_base + SPIFMC_TRAN_CSR);
+}
+
+static int sophgo_spifmc_probe(struct platform_device *pdev)
+{
+	struct spi_controller *ctrl;
+	struct sophgo_spifmc *spifmc;
+	void __iomem *base;
+	int ret;
+
+	ctrl = devm_spi_alloc_host(&pdev->dev, sizeof(*spifmc));
+	if (!ctrl)
+		return -ENOMEM;
+
+	spifmc = spi_controller_get_devdata(ctrl);
+	dev_set_drvdata(&pdev->dev, ctrl);
+

..

quoted

+	spifmc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(spifmc->clk)) {
+		dev_err(&pdev->dev, "AHB clock not found.\n");
+		return PTR_ERR(spifmc->clk);
+	}
+
+	ret = clk_prepare_enable(spifmc->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable AHB clock.\n");
+		return ret;
+	}

you can combine above with devm_clk_get_enabled(), and simplify 
return routine by using "return dev_err_probe(..)"

Thanks, I will modify it.

quoted

+
+	spifmc->dev = &pdev->dev;
+	spifmc->ctrl = ctrl;
+
+	spifmc->io_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	ctrl->num_chipselect = 1;
+	ctrl->dev.of_node = pdev->dev.of_node;
+	ctrl->bits_per_word_mask = SPI_BPW_MASK(8);
+	ctrl->auto_runtime_pm = false;
+	ctrl->mem_ops = &sophgo_spifmc_mem_ops;
+	ctrl->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL | SPI_RX_QUAD | SPI_TX_QUAD;
+
+	mutex_init(&spifmc->lock);

strictly, you still need to do error handler, e.g, destroy mutex if probe fail

Thanks, I will add it.

quoted

+
+	sophgo_spifmc_init(spifmc);
+	sophgo_spifmc_init_reg(spifmc);
+
+	return devm_spi_register_controller(&pdev->dev, ctrl);
+}
+
[...]

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55