Skip to contentSkip to conversation

Add add SPI-NAND Flash controller driver for EN7581

PATCH Superseded Lorenzo Bianconi 3 participants 12 messages Apr 23, 2024 – Apr 24, 2024

Subsystems (4)
Device tree bindings SPI Arm/airoha SoC support Airoha SPI snfi driver

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Lorenzo Bianconi

author maintainer
#0 lore raw

v3 cover Add add SPI-NAND Flash controller driver for EN7581

Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface found on Airoha ARM EN7581 SoCs.

Changes since v2:
- Fix compilation warnings
- Remove interrupt entry in dts since it is not connected so far Changes since v1:
- Introduce spi clock dependency

Lorenzo Bianconi (3):
dt-bindings: spi: airoha: Add YAML schema for SNFI controller
arm64: dts: airoha: add EN7581 spi-nand node
spi: airoha: add SPI-NAND Flash controller driver

 .../bindings/spi/airoha,en7581-snand.yaml     |   65 +
 MAINTAINERS                                   |    9 +
 arch/arm64/boot/dts/airoha/en7581.dtsi        |   19 +
 drivers/spi/Kconfig                           |   10 +
 drivers/spi/Makefile                          |    1 +
 drivers/spi/spi-airoha-snfi.c                 | 1156 +++++++++++++++++
 6 files changed, 1260 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml
 create mode 100644 drivers/spi/spi-airoha-snfi.c
Signature · 1 line
2.44.0
author maintainer
#1 lore raw

PATCH v3 1/3 dt-bindings: spi: airoha: Add YAML schema for SNFI controller

Introduce Airoha EN7581 SPI NAND controller binding

Reviewed-by Conor Dooley <[email protected]>
Tested-by Rajeev Kumar <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 .../bindings/spi/airoha,en7581-snand.yaml     | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml
Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml new file +65 -0
new file mode 100644
index 000000000000..b820c5613dcc
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/airoha,en7581-snand.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SPI-NAND flash controller for Airoha ARM SoCs
+
+maintainers:
+  - Lorenzo Bianconi <[email protected]>
+
+allOf:
+  - $ref: spi-controller.yaml#
+
+properties:
+  compatible:
+    const: airoha,en7581-snand
+
+  reg:
+    items:
+      - description: spi base address
+      - description: nfi2spi base address
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: spi
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/en7523-clk.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      spi@1fa10000 {
+        compatible = "airoha,en7581-snand";
+        reg = <0x0 0x1fa10000 0x0 0x140>,
+              <0x0 0x1fa11000 0x0 0x160>;
+
+        clocks = <&scuclk EN7523_CLK_SPI>;
+        clock-names = "spi";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        flash@0 {
+          compatible = "spi-nand";
+          reg = <0>;
+          spi-tx-bus-width = <1>;
+          spi-rx-bus-width = <2>;
+        };
+      };
+    };
Signature · 1 line
2.44.0
author maintainer
#3 lore raw

PATCH v3 2/3 arm64: dts: airoha: add EN7581 spi-nand node

In reply to Lorenzo Bianconi

Introduce the Airoha EN7581 spi-nand node in Airoha EN7581 dtsi

Tested-by Rajeev Kumar <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 arch/arm64/boot/dts/airoha/en7581.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/arm64/boot/dts/airoha/en7581.dtsi +19 -0
index 5c4bfe3e1e5a..98c2a86086e1 100644
@@ -159,5 +159,24 @@ scuclk: clock-controller@1fa20000 {
 			      <0x0 0x1fbe3400 0x0 0xfc>;
 			#clock-cells = <1>;
 		};
+
+		snfi: spi@1fa10000 {
+			compatible = "airoha,en7581-snand";
+			reg = <0x0 0x1fa10000 0x0 0x140>,
+			      <0x0 0x1fa11000 0x0 0x160>;
+
+			clocks = <&scuclk EN7523_CLK_SPI>;
+			clock-names = "spi";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+			spi_nand: nand@0 {
+				compatible = "spi-nand";
+				reg = <0>;
+				spi-max-frequency = <50000000>;
+				spi-tx-bus-width = <1>;
+				spi-rx-bus-width = <2>;
+			};
+		};
 	};
 };
Signature · 1 line
2.44.0
author maintainer
#5 lore raw

PATCH v3 3/3 spi: airoha: add SPI-NAND Flash controller driver

In reply to Lorenzo Bianconi

Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface found on Airoha ARM SoCs.

Tested-by Rajeev Kumar <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 MAINTAINERS                   |    9 +
 drivers/spi/Kconfig           |   10 +
 drivers/spi/Makefile          |    1 +
 drivers/spi/spi-airoha-snfi.c | 1156 +++++++++++++++++++++++++++++++++
 4 files changed, 1176 insertions(+)
 create mode 100644 drivers/spi/spi-airoha-snfi.c
MAINTAINERS +9 -0
index aa3b947fb080..ce9fac46f741 100644
@@ -653,6 +653,15 @@ S:	Supported
 F:	fs/aio.c
 F:	include/linux/*aio*.h
 
+AIROHA SPI SNFI DRIVER
+M:	Lorenzo Bianconi <[email protected]>
+M:	Ray Liu <[email protected]>
+L:	[email protected] (moderated for non-subscribers)
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml
+F:	drivers/spi/spi-airoha.c
+
 AIRSPY MEDIA DRIVER
 L:	[email protected]
 S:	Orphan
drivers/spi/Kconfig +10 -0
index bc7021da2fe9..6fa91775f334 100644
@@ -57,6 +57,16 @@ config SPI_MEM
 
 comment "SPI Master Controller Drivers"
 
+config SPI_AIROHA_SNFI
+	tristate "Airoha SPI NAND Flash Interface"
+	depends on ARCH_AIROHA || COMPILE_TEST
+	depends on SPI_MASTER
+	select REGMAP_MMIO
+	help
+	  This enables support for SPI-NAND mode on the Airoha NAND
+	  Flash Interface found on Airoha ARM SoCs. This controller
+	  is implemented as a SPI-MEM controller.
+
 config SPI_ALTERA
 	tristate "Altera SPI Controller platform driver"
 	select SPI_ALTERA_CORE
drivers/spi/Makefile +1 -0
index 4ff8d725ba5e..e694254dec04 100644
@@ -14,6 +14,7 @@ obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
 obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 
 # SPI master controller drivers (bus)
+obj-$(CONFIG_SPI_AIROHA_SNFI)		+= spi-airoha-snfi.o
 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
drivers/spi/spi-airoha-snfi.c new file +1156 -0

1,156 lines not on this page · raw message

Signature · 1 line
2.44.0
maintainer
#6 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

Il 23/04/24 12:16, Lorenzo Bianconi ha scritto:

Quoted 79 lines

Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface found on Airoha ARM SoCs.

Tested-by Rajeev Kumar <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
  MAINTAINERS                   |    9 +
  drivers/spi/Kconfig           |   10 +
  drivers/spi/Makefile          |    1 +
  drivers/spi/spi-airoha-snfi.c | 1156 +++++++++++++++++++++++++++++++++
  4 files changed, 1176 insertions(+)

create mode 100644 drivers/spi/spi-airoha-snfi.c

MAINTAINERS +9 -0
index aa3b947fb080..ce9fac46f741 100644
@@ -653,6 +653,15 @@ S:	Supported
  F:	fs/aio.c
  F:	include/linux/*aio*.h
 
+AIROHA SPI SNFI DRIVER
+M:	Lorenzo Bianconi <[email protected]>
+M:	Ray Liu <[email protected]>
+L:	[email protected] (moderated for non-subscribers)
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml
+F:	drivers/spi/spi-airoha.c
+
  AIRSPY MEDIA DRIVER
  L:	[email protected]
  S:	Orphan
drivers/spi/Kconfig +10 -0
index bc7021da2fe9..6fa91775f334 100644
@@ -57,6 +57,16 @@ config SPI_MEM
 
  comment "SPI Master Controller Drivers"
 
+config SPI_AIROHA_SNFI
+	tristate "Airoha SPI NAND Flash Interface"
+	depends on ARCH_AIROHA || COMPILE_TEST
+	depends on SPI_MASTER
+	select REGMAP_MMIO
+	help
+	  This enables support for SPI-NAND mode on the Airoha NAND
+	  Flash Interface found on Airoha ARM SoCs. This controller
+	  is implemented as a SPI-MEM controller.
+
  config SPI_ALTERA
  	tristate "Altera SPI Controller platform driver"
  	select SPI_ALTERA_CORE
drivers/spi/Makefile +1 -0
index 4ff8d725ba5e..e694254dec04 100644
@@ -14,6 +14,7 @@ obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
  obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 
  # SPI master controller drivers (bus)
+obj-$(CONFIG_SPI_AIROHA_SNFI)		+= spi-airoha-snfi.o
  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
drivers/spi/spi-airoha-snfi.c new file +7 -0
new file mode 100644
index 000000000000..eacb32d6e28e
@@ -0,0 +1,1156 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 AIROHA Inc
+ * Author: Lorenzo Bianconi <[email protected]>
+ * Author: Ray Liu <[email protected]>
+ */
+

..snip...

Quoted 29 lines
drivers/spi/spi-airoha-snfi.c new file +29 -0
+static ssize_t airoha_snand_dirmap_read(struct spi_mem_dirmap_desc *desc,
+					u64 offs, size_t len, void *buf)
+{
+	struct spi_device *spi = desc->mem->spi;
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct spi_mem_op *op = &desc->info.op_tmpl;
+	struct airoha_snand_ctrl *as_ctrl;
+	u32 val, rd_mode;
+	int err;
+
+	if (!as_dev->data_need_update)
+		return len;
+
+	as_dev->data_need_update = false;
+
+	switch (op->cmd.opcode) {
+	case SPI_NAND_OP_READ_FROM_CACHE_DUAL:
+		rd_mode = 1;
+		break;
+	case SPI_NAND_OP_READ_FROM_CACHE_QUAD:
+		rd_mode = 2;
+		break;
+	default:
+		rd_mode = 0;
+		break;
+	}
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_DMA);

drivers/spi/spi-airoha-snfi.c line 685

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	if (err < 0)
+		return err;
+
+	err = airoha_snand_nfi_config(as_ctrl);
+	if (err)
+		return err;
+
+	dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
+				   as_dev->buf_len, DMA_BIDIRECTIONAL);
+	mb();

Are you sure you need this memory barrier here?

P.S.: Just in case... any other write or read will anyway add a barrier ;-)

Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +7 -0
+
+	/* set dma addr */
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
+			   as_dev->dma_addr);
+	if (err)
+		return err;
+

..snip..

Quoted 24 lines
drivers/spi/spi-airoha-snfi.c new file +24 -0
+
+static ssize_t airoha_snand_dirmap_write(struct spi_mem_dirmap_desc *desc,
+					 u64 offs, size_t len, const void *buf)
+{
+	struct spi_device *spi = desc->mem->spi;
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct spi_mem_op *op = &desc->info.op_tmpl;
+	struct airoha_snand_ctrl *as_ctrl;
+	u32 wr_mode, val;
+	int err;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_MANUAL);
+	if (err < 0)
+		return err;
+
+	dma_sync_single_for_cpu(as_ctrl->dev, as_dev->dma_addr,
+				as_dev->buf_len, DMA_BIDIRECTIONAL);
+	memcpy(as_dev->txrx_buf + offs, buf, len);
+	dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
+				   as_dev->buf_len, DMA_BIDIRECTIONAL);
+	mb();
+
+	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_DMA);

drivers/spi/spi-airoha-snfi.c line 810

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	if (err < 0)
+		return err;
+
+	err = airoha_snand_nfi_config(as_ctrl);
+	if (err)
+		return err;
+
+	if (op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_QUAD ||
+	    op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_RAMDON_QUAD)
+		wr_mode = 2;

I'm mostly sure that this '2' is supposed to be BIT(1) instead

drivers/spi/spi-airoha-snfi.c line 820

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	else
+		wr_mode = 0;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
+			   as_dev->dma_addr);
+	if (err)
+		return err;
+
+	val = as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num;
+	val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM, val);

val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM,

                 as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num);

Being this 100 cols eventually, you wouldn't even need two lines - but if you don't want to go 100, it's still more readable (imo) like this, even if in two lines.

Quoted 17 lines
drivers/spi/spi-airoha-snfi.c new file +17 -0
+	err = regmap_update_bits(as_ctrl->regmap_nfi,
+				 REG_SPI_NFI_SNF_MISC_CTL2,
+				 SPI_NFI_PROG_LOAD_BYTE_NUM, val);
+	if (err)
+		return err;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_PG_CTL1,
+			   FIELD_PREP(SPI_NFI_PG_LOAD_CMD,
+				      op->cmd.opcode));
+	if (err)
+		return err;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_SNF_MISC_CTL,
+			   FIELD_PREP(SPI_NFI_DATA_READ_WR_MODE, wr_mode));
+	if (err)
+		return err;
+

..snip..

drivers/spi/spi-airoha-snfi.c line 977

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+
+#define SPI_NAND_CACHE_SIZE	(4096 + 256)

#include <linux/sizes.h> <--- at the beginning

#define SPI_NAND_CACHE_SIZE (SZ_4K + SZ_256)

...there are macros for that, might as well use them, right? :-)

Quoted 88 lines
drivers/spi/spi-airoha-snfi.c new file +88 -0
+
+static int airoha_snand_setup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
+	if (!as_dev)
+		return -ENOMEM;
+
+	spi_set_ctldata(spi, as_dev);
+	as_dev->data_need_update = true;
+
+	/* prepare device buffer */
+	as_dev->buf_len = SPI_NAND_CACHE_SIZE;
+	as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
+	if (!as_dev->txrx_buf)
+		goto error_dev_free;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,
+					  as_dev->buf_len, DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
+		goto error_buf_free;
+
+	return 0;
+
+error_buf_free:
+	kfree(as_dev->txrx_buf);
+error_dev_free:
+	kfree(as_dev);
+
+	return -EINVAL;
+}
+
+static void airoha_snand_cleanup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	dma_unmap_single(as_ctrl->dev, as_dev->dma_addr,
+			 as_dev->buf_len, DMA_BIDIRECTIONAL);
+	kfree(as_dev->txrx_buf);
+
+	spi_set_ctldata(spi, NULL);
+}
+
+static int airoha_snand_nfi_setup(struct airoha_snand_ctrl *as_ctrl)
+{
+	u32 val, sec_size, sec_num;
+	int err;
+
+	err = regmap_read(as_ctrl->regmap_nfi, REG_SPI_NFI_CON, &val);
+	if (err)
+		return err;
+
+	sec_num = FIELD_GET(SPI_NFI_SEC_NUM, val);
+
+	err = regmap_read(as_ctrl->regmap_nfi,
+			  REG_SPI_NFI_SECCUS_SIZE, &val);
+	if (err)
+		return err;
+
+	sec_size = FIELD_GET(SPI_NFI_CUS_SEC_SIZE, val);
+
+	/* init default value */
+	as_ctrl->nfi_cfg.sec_size = sec_size;
+	as_ctrl->nfi_cfg.sec_num = sec_num;
+	as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);
+	as_ctrl->nfi_cfg.spare_size = 16;
+
+	err = airoha_snand_nfi_init(as_ctrl);
+	if (err)
+		return err;
+
+	return airoha_snand_nfi_config(as_ctrl);
+}
+
+static const struct regmap_config spi_ctrl_regmap_config = {
+	.name		= "ctrl",
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= REG_SPI_CTRL_NFI2SPI_EN,
+};
+
+static const struct regmap_config spi_nfi_regmap_config = {

drivers/spi/spi-airoha-snfi.c line 1075

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	.name		= "nfi",
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= REG_SPI_NFI_SNF_NFI_CNFG,
+};
+
+static const struct of_device_id airoha_snand_ids[] = {
+	{ .compatible	= "airoha,en7581-snand" },
+	{ }

{ /* sentinel */ }

...please!

drivers/spi/spi-airoha-snfi.c line 1078

Quoted 3 lines
drivers/spi/spi-airoha-snfi.c new file +3 -0
+};
+MODULE_DEVICE_TABLE(of, airoha_snand_ids);
+

Cheers,
Angelo

author maintainer
#7 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

Quoted 1 line

Il 23/04/24 12:16, Lorenzo Bianconi ha scritto:

[...]

Quoted 11 lines

drivers/spi/spi-airoha-snfi.c line 685

Quoted 8 lines
drivers/spi/spi-airoha-snfi.c new file +8 -0
+
+	err = airoha_snand_nfi_config(as_ctrl);
+	if (err)
+		return err;
+
+	dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
+				   as_dev->buf_len, DMA_BIDIRECTIONAL);
+	mb();

Are you sure you need this memory barrier here?

Quoted 2 lines

P.S.: Just in case... any other write or read will anyway add a barrier ;-)

ack, I will fix it in v4.

Quoted 38 lines
Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +7 -0
+
+	/* set dma addr */
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
+			   as_dev->dma_addr);
+	if (err)
+		return err;
+

..snip..

Quoted 28 lines
drivers/spi/spi-airoha-snfi.c new file +28 -0
+
+static ssize_t airoha_snand_dirmap_write(struct spi_mem_dirmap_desc *desc,
+					 u64 offs, size_t len, const void *buf)
+{
+	struct spi_device *spi = desc->mem->spi;
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct spi_mem_op *op = &desc->info.op_tmpl;
+	struct airoha_snand_ctrl *as_ctrl;
+	u32 wr_mode, val;
+	int err;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_MANUAL);
+	if (err < 0)
+		return err;
+
+	dma_sync_single_for_cpu(as_ctrl->dev, as_dev->dma_addr,
+				as_dev->buf_len, DMA_BIDIRECTIONAL);
+	memcpy(as_dev->txrx_buf + offs, buf, len);
+	dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
+				   as_dev->buf_len, DMA_BIDIRECTIONAL);
+	mb();
+
+	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_DMA);
+	if (err < 0)
+		return err;
+
+	err = airoha_snand_nfi_config(as_ctrl);
Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 810

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+	if (err)
+		return err;
+
+	if (op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_QUAD ||
+	    op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_RAMDON_QUAD)
+		wr_mode = 2;

I'm mostly sure that this '2' is supposed to be BIT(1) instead

ack, I will fix it in v4.

Quoted 13 lines

drivers/spi/spi-airoha-snfi.c line 820

Quoted 11 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	else
+		wr_mode = 0;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
+			   as_dev->dma_addr);
+	if (err)
+		return err;
+
+	val = as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num;
+	val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM, val);

val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM,

Quoted 5 lines
                 as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num);

Being this 100 cols eventually, you wouldn't even need two lines - but if you don't want to go 100, it's still more readable (imo) like this, even if in two lines.

I prefer to be below 79 columns, so I will go for the 2 lines :) I will fix it in v4.

Quoted 21 lines
Quoted 18 lines
drivers/spi/spi-airoha-snfi.c new file +17 -0
+	err = regmap_update_bits(as_ctrl->regmap_nfi,
+				 REG_SPI_NFI_SNF_MISC_CTL2,
+				 SPI_NFI_PROG_LOAD_BYTE_NUM, val);
+	if (err)
+		return err;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_PG_CTL1,
+			   FIELD_PREP(SPI_NFI_PG_LOAD_CMD,
+				      op->cmd.opcode));
+	if (err)
+		return err;
+
+	err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_SNF_MISC_CTL,
+			   FIELD_PREP(SPI_NFI_DATA_READ_WR_MODE, wr_mode));
+	if (err)
+		return err;
+

..snip..

Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 977

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+
+#define SPI_NAND_CACHE_SIZE	(4096 + 256)

#include <linux/sizes.h> <--- at the beginning

#define SPI_NAND_CACHE_SIZE (SZ_4K + SZ_256)

...there are macros for that, might as well use them, right? :-)

ack, I will fix it in v4.

Quoted 102 lines
Quoted 89 lines
drivers/spi/spi-airoha-snfi.c new file +88 -0
+
+static int airoha_snand_setup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
+	if (!as_dev)
+		return -ENOMEM;
+
+	spi_set_ctldata(spi, as_dev);
+	as_dev->data_need_update = true;
+
+	/* prepare device buffer */
+	as_dev->buf_len = SPI_NAND_CACHE_SIZE;
+	as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
+	if (!as_dev->txrx_buf)
+		goto error_dev_free;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,
+					  as_dev->buf_len, DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
+		goto error_buf_free;
+
+	return 0;
+
+error_buf_free:
+	kfree(as_dev->txrx_buf);
+error_dev_free:
+	kfree(as_dev);
+
+	return -EINVAL;
+}
+
+static void airoha_snand_cleanup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	dma_unmap_single(as_ctrl->dev, as_dev->dma_addr,
+			 as_dev->buf_len, DMA_BIDIRECTIONAL);
+	kfree(as_dev->txrx_buf);
+
+	spi_set_ctldata(spi, NULL);
+}
+
+static int airoha_snand_nfi_setup(struct airoha_snand_ctrl *as_ctrl)
+{
+	u32 val, sec_size, sec_num;
+	int err;
+
+	err = regmap_read(as_ctrl->regmap_nfi, REG_SPI_NFI_CON, &val);
+	if (err)
+		return err;
+
+	sec_num = FIELD_GET(SPI_NFI_SEC_NUM, val);
+
+	err = regmap_read(as_ctrl->regmap_nfi,
+			  REG_SPI_NFI_SECCUS_SIZE, &val);
+	if (err)
+		return err;
+
+	sec_size = FIELD_GET(SPI_NFI_CUS_SEC_SIZE, val);
+
+	/* init default value */
+	as_ctrl->nfi_cfg.sec_size = sec_size;
+	as_ctrl->nfi_cfg.sec_num = sec_num;
+	as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);
+	as_ctrl->nfi_cfg.spare_size = 16;
+
+	err = airoha_snand_nfi_init(as_ctrl);
+	if (err)
+		return err;
+
+	return airoha_snand_nfi_config(as_ctrl);
+}
+
+static const struct regmap_config spi_ctrl_regmap_config = {
+	.name		= "ctrl",
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= REG_SPI_CTRL_NFI2SPI_EN,
+};
+
+static const struct regmap_config spi_nfi_regmap_config = {

drivers/spi/spi-airoha-snfi.c line 1075

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+	.name		= "nfi",
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= REG_SPI_NFI_SNF_NFI_CNFG,
+};
+
+static const struct of_device_id airoha_snand_ids[] = {
+	{ .compatible	= "airoha,en7581-snand" },
+	{ }

{ /* sentinel */ }

Quoted 1 line

...please!

ack, I will fix it in v4.

Regards,
Lorenzo

Quoted 9 lines

drivers/spi/spi-airoha-snfi.c line 1078

Quoted 4 lines
drivers/spi/spi-airoha-snfi.c new file +3 -0
+};
+MODULE_DEVICE_TABLE(of, airoha_snand_ids);
+

Cheers,
Angelo

#8 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

In reply to Lorenzo Bianconi

Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:

Quoted 2 lines

Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface found on Airoha ARM SoCs.

...

drivers/spi/spi-airoha-snfi.c line 14

Quoted 8 lines
drivers/spi/spi-airoha-snfi.c new file +7 -0
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
 
+#include <linux/kernel.h>

Make sure you are using exact headers you need, this one seems "proxy" and not really in use here.

(Quite likely you wanted minmax.h, types.h, and possible others.)

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>

...

drivers/spi/spi-airoha-snfi.c line 118

Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +7 -0
+#define SPI_NFI_ALL_IRQ_EN			(SPI_NFI_RD_DONE_EN | \
+						 SPI_NFI_WR_DONE_EN | \
+						 SPI_NFI_RST_DONE_EN | \
+						 SPI_NFI_ERASE_DONE_EN | \
+						 SPI_NFI_BUSY_RETURN_EN | \
+						 SPI_NFI_ACCESS_LOCK_EN | \
+						 SPI_NFI_AHB_DONE_EN)

What about writing this as

#define SPI_NFI_ALL_IRQ_EN					\
	(SPI_NFI_RD_DONE_EN | SPI_NFI_WR_DONE_EN |		\
	 SPI_NFI_RST_DONE_EN | SPI_NFI_ERASE_DONE_EN |		\
	 SPI_NFI_BUSY_RETURN_EN | SPI_NFI_ACCESS_LOCK_EN |	\
	 SPI_NFI_AHB_DONE_EN)

?

...

drivers/spi/spi-airoha-snfi.c line 195

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+enum airoha_snand_mode {
+	SPI_MODE_AUTO,
+	SPI_MODE_MANUAL,
+	SPI_MODE_DMA,
+	SPI_MODE_NO

Is _NO a termination entry? Meaning there always be only 3 modes no matter what. If not, leave the trailing comma as it helps to reduce a burden in case this list will be expanded.

Quoted 1 line

+};

...

drivers/spi/spi-airoha-snfi.c line 211

Quoted 9 lines
drivers/spi/spi-airoha-snfi.c new file +9 -0
+struct airoha_snand_dev {
+	size_t buf_len;
+
+	u8 *txrx_buf;
+	dma_addr_t dma_addr;
+
+	bool data_need_update;
+	u64 cur_page_num;
+};

Most likely pahole shows better layout to save a few bytes in some cases.

...

drivers/spi/spi-airoha-snfi.c line 223

Quoted 12 lines
drivers/spi/spi-airoha-snfi.c new file +11 -0
+struct airoha_snand_ctrl {
+	struct device *dev;
+	struct regmap *regmap_ctrl;
+	struct regmap *regmap_nfi;
+	struct clk *spi_clk;
+
+	struct {
+		size_t page_size;
+		size_t sec_size;
 
+		unsigned char sec_num;
+		unsigned char spare_size;

Hmm... Why not u8 for both of these?

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	} nfi_cfg;
+};

...

drivers/spi/spi-airoha-snfi.c line 400

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+static int airoha_snand_write_data(struct airoha_snand_ctrl *as_ctrl, u8 cmd,
+				   const u8 *data, int len)
+{
+	int i = 0;
+
+	while (i < len) {

Seems nothing prevents you from using for-loop here as well.

Quoted 17 lines
drivers/spi/spi-airoha-snfi.c new file +17 -0
+		int data_len = min(len, MAX_TRANSFER_SIZE);
+		int err;
+
+		err = airoha_snand_set_fifo_op(as_ctrl, cmd, data_len);
+		if (err)
+			return err;
+
+		err = airoha_snand_write_data_to_fifo(as_ctrl, &data[i],
+						      data_len);
+		if (err < 0)
+			return err;
+
+		i += data_len;
+	}
+
+	return 0;
+}

...

drivers/spi/spi-airoha-snfi.c line 420

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+static int airoha_snand_read_data(struct airoha_snand_ctrl *as_ctrl, u8 *data,
+				  int len)

As per above.

...

drivers/spi/spi-airoha-snfi.c line 936

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+	/* addr part */
+	for (i = 0; i < op->addr.nbytes; i++) {
+		u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
+
+		data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);

Seems like you wanted to have always the same endianess and hence can be done outside the loop via cpu_to_xxx()?

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+		err = airoha_snand_write_data(as_ctrl, cmd, &data,
+					      sizeof(data));
+		if (err)
+			return err;
+	}

...

Quoted 20 lines
drivers/spi/spi-airoha-snfi.c new file +20 -0
+static int airoha_snand_setup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
+	if (!as_dev)
+		return -ENOMEM;
+
+	spi_set_ctldata(spi, as_dev);
+	as_dev->data_need_update = true;
+
+	/* prepare device buffer */
+	as_dev->buf_len = SPI_NAND_CACHE_SIZE;
+	as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
+	if (!as_dev->txrx_buf)
+		goto error_dev_free;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,

drivers/spi/spi-airoha-snfi.c line 1008

Quoted 10 lines
drivers/spi/spi-airoha-snfi.c new file +10 -0
+					  as_dev->buf_len, DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
+		goto error_buf_free;
+
+	return 0;
+
+error_buf_free:
+	kfree(as_dev->txrx_buf);
+error_dev_free:
+	kfree(as_dev);

Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)

Quoted 2 lines
(quoted diff) +2 -0
+	return -EINVAL;
+}

...

drivers/spi/spi-airoha-snfi.c line 1038

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	err = regmap_read(as_ctrl->regmap_nfi,
+			  REG_SPI_NFI_SECCUS_SIZE, &val);

One line?

Quoted 2 lines
(quoted diff) +2 -0
+	if (err)
+		return err;

...

drivers/spi/spi-airoha-snfi.c line 1047

Quoted 1 line
drivers/spi/spi-airoha-snfi.c new file +1 -0
+	as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);

round_down() is optimised for power-of-2.
You would need to include math.h IIRC.

...

drivers/spi/spi-airoha-snfi.c line 1099

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	as_ctrl->regmap_ctrl = devm_regmap_init_mmio(&pdev->dev, base,
+						     &spi_ctrl_regmap_config);

With help of

	struct device *dev = &pdev->dev;

at the top of the function the entire code will become neater.

drivers/spi/spi-airoha-snfi.c line 1103

Quoted 4 lines
drivers/spi/spi-airoha-snfi.c new file +4 -0
+	if (IS_ERR(as_ctrl->regmap_ctrl)) {
+		dev_err(&pdev->dev, "failed to init spi ctrl regmap: %ld\n",
+			PTR_ERR(as_ctrl->regmap_ctrl));
+		return PTR_ERR(as_ctrl->regmap_ctrl);

		return dev_err_probe(...);
Quoted 1 line
(quoted diff) +1 -0
+	}

...

drivers/spi/spi-airoha-snfi.c line 1115

Quoted 3 lines
drivers/spi/spi-airoha-snfi.c new file +3 -0
+		dev_err(&pdev->dev, "failed to init spi nfi regmap: %ld\n",
+			PTR_ERR(as_ctrl->regmap_nfi));
+		return PTR_ERR(as_ctrl->regmap_nfi);

		return dev_err_probe(...);

...

drivers/spi/spi-airoha-snfi.c line 1121

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+		dev_err(&pdev->dev, "unable to get spi clk");
+		return PTR_ERR(as_ctrl->spi_clk);

Ditto.

...

Quoted 1 line

+

Unneeded blank line.

Quoted 1 line

+module_platform_driver(airoha_snand_driver);

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#9 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

Quoted 13 lines

Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:

Quoted 2 lines

Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface found on Airoha ARM SoCs.

...

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
Quoted 6 lines
Quoted 1 line

+#include <linux/kernel.h>

Make sure you are using exact headers you need, this one seems "proxy" and not really in use here.

(Quite likely you wanted minmax.h, types.h, and possible others.)

ack, I will fix it.

Quoted 20 lines
Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>

...

drivers/spi/spi-airoha-snfi.c line 118

Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +7 -0
+#define SPI_NFI_ALL_IRQ_EN			(SPI_NFI_RD_DONE_EN | \
+						 SPI_NFI_WR_DONE_EN | \
+						 SPI_NFI_RST_DONE_EN | \
+						 SPI_NFI_ERASE_DONE_EN | \
+						 SPI_NFI_BUSY_RETURN_EN | \
+						 SPI_NFI_ACCESS_LOCK_EN | \
+						 SPI_NFI_AHB_DONE_EN)

What about writing this as

Quoted 7 lines
#define SPI_NFI_ALL_IRQ_EN					\
	(SPI_NFI_RD_DONE_EN | SPI_NFI_WR_DONE_EN |		\
	 SPI_NFI_RST_DONE_EN | SPI_NFI_ERASE_DONE_EN |		\
	 SPI_NFI_BUSY_RETURN_EN | SPI_NFI_ACCESS_LOCK_EN |	\
	 SPI_NFI_AHB_DONE_EN)

?

no strong opinion about it, I will fix it.

Quoted 12 lines

...

drivers/spi/spi-airoha-snfi.c line 195

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+enum airoha_snand_mode {
+	SPI_MODE_AUTO,
+	SPI_MODE_MANUAL,
+	SPI_MODE_DMA,
+	SPI_MODE_NO

Is _NO a termination entry? Meaning there always be only 3 modes no matter what. If not, leave the trailing comma as it helps to reduce a burden in case this list will be expanded.

I think we can get rid of it

Quoted 5 lines
Quoted 2 lines

+};

...

Quoted 11 lines

drivers/spi/spi-airoha-snfi.c line 211

Quoted 9 lines
drivers/spi/spi-airoha-snfi.c new file +9 -0
+struct airoha_snand_dev {
+	size_t buf_len;
+
+	u8 *txrx_buf;
+	dma_addr_t dma_addr;
+
+	bool data_need_update;
+	u64 cur_page_num;
+};

Most likely pahole shows better layout to save a few bytes in some cases.

ack, I think we can swap data_need_update and cur_page_num.

Quoted 13 lines

...

Quoted 9 lines
drivers/spi/spi-airoha-snfi.c new file +9 -0
+struct airoha_snand_ctrl {
+	struct device *dev;
+	struct regmap *regmap_ctrl;
+	struct regmap *regmap_nfi;
+	struct clk *spi_clk;
+
+	struct {
+		size_t page_size;
+		size_t sec_size;
Quoted 4 lines

drivers/spi/spi-airoha-snfi.c line 223

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+		unsigned char sec_num;
+		unsigned char spare_size;

Hmm... Why not u8 for both of these?

ack, I will fix it.

Quoted 6 lines
Quoted 3 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	} nfi_cfg;
+};

...

Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 400

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+static int airoha_snand_write_data(struct airoha_snand_ctrl *as_ctrl, u8 cmd,
+				   const u8 *data, int len)
+{
+	int i = 0;
+
+	while (i < len) {

Seems nothing prevents you from using for-loop here as well.

ack, I will fix it.

Quoted 21 lines
Quoted 18 lines
drivers/spi/spi-airoha-snfi.c new file +17 -0
+		int data_len = min(len, MAX_TRANSFER_SIZE);
+		int err;
+
+		err = airoha_snand_set_fifo_op(as_ctrl, cmd, data_len);
+		if (err)
+			return err;
+
+		err = airoha_snand_write_data_to_fifo(as_ctrl, &data[i],
+						      data_len);
+		if (err < 0)
+			return err;
+
+		i += data_len;
+	}
+
+	return 0;
+}

...

Quoted 4 lines

drivers/spi/spi-airoha-snfi.c line 420

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+static int airoha_snand_read_data(struct airoha_snand_ctrl *as_ctrl, u8 *data,
+				  int len)

As per above.

ack, I will fix it.

Quoted 11 lines

...

drivers/spi/spi-airoha-snfi.c line 936

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+	/* addr part */
+	for (i = 0; i < op->addr.nbytes; i++) {
+		u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
+
+		data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);

Seems like you wanted to have always the same endianess and hence can be done outside the loop via cpu_to_xxx()?

sorry, I did not get what you mean here, data value relies on the loop iteration.

Quoted 33 lines
Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+		err = airoha_snand_write_data(as_ctrl, cmd, &data,
+					      sizeof(data));
+		if (err)
+			return err;
+	}

...

Quoted 24 lines
drivers/spi/spi-airoha-snfi.c new file +24 -0
+static int airoha_snand_setup(struct spi_device *spi)
+{
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
+	struct airoha_snand_ctrl *as_ctrl;
+
+	as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
+	if (!as_dev)
+		return -ENOMEM;
+
+	spi_set_ctldata(spi, as_dev);
+	as_dev->data_need_update = true;
+
+	/* prepare device buffer */
+	as_dev->buf_len = SPI_NAND_CACHE_SIZE;
+	as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
+	if (!as_dev->txrx_buf)
+		goto error_dev_free;
+
+	as_ctrl = spi_controller_get_devdata(spi->controller);
+	as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,
+					  as_dev->buf_len, DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
+		goto error_buf_free;
+
Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 1008

Quoted 6 lines
drivers/spi/spi-airoha-snfi.c new file +6 -0
+	return 0;
+
+error_buf_free:
+	kfree(as_dev->txrx_buf);
+error_dev_free:
+	kfree(as_dev);

Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)

I guess we can just allocate as_dev and as_dev->txrx_buf with devm_kzalloc() here, agree?

Quoted 10 lines
Quoted 3 lines
(quoted diff) +2 -0
+	return -EINVAL;
+}

...

drivers/spi/spi-airoha-snfi.c line 1038

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	err = regmap_read(as_ctrl->regmap_nfi,
+			  REG_SPI_NFI_SECCUS_SIZE, &val);

One line?

ack, I will fix it.

Quoted 10 lines
Quoted 3 lines
(quoted diff) +2 -0
+	if (err)
+		return err;

...

drivers/spi/spi-airoha-snfi.c line 1047

Quoted 1 line
drivers/spi/spi-airoha-snfi.c new file +1 -0
+	as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);

round_down() is optimised for power-of-2.
You would need to include math.h IIRC.

ack, I will fix it.

Quoted 11 lines

...

drivers/spi/spi-airoha-snfi.c line 1099

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+	as_ctrl->regmap_ctrl = devm_regmap_init_mmio(&pdev->dev, base,
+						     &spi_ctrl_regmap_config);

With help of

	struct device *dev = &pdev->dev;

at the top of the function the entire code will become neater.

ack, I will fix it.

Quoted 25 lines

drivers/spi/spi-airoha-snfi.c line 1103

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +4 -0
+	if (IS_ERR(as_ctrl->regmap_ctrl)) {
+		dev_err(&pdev->dev, "failed to init spi ctrl regmap: %ld\n",
+			PTR_ERR(as_ctrl->regmap_ctrl));
+		return PTR_ERR(as_ctrl->regmap_ctrl);

		return dev_err_probe(...);
Quoted 1 line
(quoted diff) +1 -0
+	}

...

drivers/spi/spi-airoha-snfi.c line 1115

Quoted 3 lines
drivers/spi/spi-airoha-snfi.c new file +3 -0
+		dev_err(&pdev->dev, "failed to init spi nfi regmap: %ld\n",
+			PTR_ERR(as_ctrl->regmap_nfi));
+		return PTR_ERR(as_ctrl->regmap_nfi);

		return dev_err_probe(...);

...

drivers/spi/spi-airoha-snfi.c line 1121

Quoted 2 lines
drivers/spi/spi-airoha-snfi.c new file +2 -0
+		dev_err(&pdev->dev, "unable to get spi clk");
+		return PTR_ERR(as_ctrl->spi_clk);

Ditto.

Quoted 5 lines

...

Quoted 1 line

+

Unneeded blank line.

ack, I will fix it.

Regards,
Lorenzo

Quoted 9 lines
Quoted 2 lines

+module_platform_driver(airoha_snand_driver);

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

Quoted 1 line · Lorenzo Bianconi
Quoted 1 line

Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:

...

Quoted 11 lines
Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 936

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+   /* addr part */
+   for (i = 0; i < op->addr.nbytes; i++) {
+           u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
+
+           data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);

Seems like you wanted to have always the same endianess and hence can be done outside the loop via cpu_to_xxx()?

sorry, I did not get what you mean here, data value relies on the loop iteration.

  u8 byte_stream[8];
  u8 cmd;

  cmd = ...;

  // find what suits your case(s)
  put_unaligned_be64(op->addr.val, byte_stream);
  for-loop {
     err = _write_data();
     ...
  }
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+           err = airoha_snand_write_data(as_ctrl, cmd, &data,
+                                         sizeof(data));
+           if (err)
+                   return err;
+   }

...

Quoted 4 lines
Quoted 1 line

Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)

I guess we can just allocate as_dev and as_dev->txrx_buf with devm_kzalloc() here, agree?

If this is a solely part of ->probe() stage, yes.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#11 lore raw

Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

Quoted 17 lines · Lorenzo Bianconi
Quoted 1 line
Quoted 1 line

Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:

...

Quoted 11 lines
Quoted 8 lines

drivers/spi/spi-airoha-snfi.c line 936

Quoted 5 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+   /* addr part */
+   for (i = 0; i < op->addr.nbytes; i++) {
+           u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
+
+           data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);

Seems like you wanted to have always the same endianess and hence can be done outside the loop via cpu_to_xxx()?

sorry, I did not get what you mean here, data value relies on the loop iteration.

Quoted 11 lines
  u8 byte_stream[8];
  u8 cmd;

  cmd = ...;

  // find what suits your case(s)
  put_unaligned_be64(op->addr.val, byte_stream);
  for-loop {
     err = _write_data();
     ...
  }

ack, I will fix it in v4.

Rregards,
Lorenzo

Quoted 20 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/spi/spi-airoha-snfi.c new file +5 -0
+           err = airoha_snand_write_data(as_ctrl, cmd, &data,
+                                         sizeof(data));
+           if (err)
+                   return err;
+   }

...

Quoted 4 lines
Quoted 1 line

Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)

I guess we can just allocate as_dev and as_dev->txrx_buf with devm_kzalloc() here, agree?

If this is a solely part of ->probe() stage, yes.

Signature · 2 lines
With Best Regards,
Andy Shevchenko