Re: [PATCH 03/24] mtd: rawnand: sunxi_spl: cosmetic: harmonize register defines with non spl file

Fri, 17 Oct 2025 10:10:17 -0700 

Hi,
Le 17/10/2025 à 09:57, Andre Przywara a écrit :

Hi,
please avoid the use of the word "cosmectic" in the subject if the change is not really cosmetic, as in just fixing some formatting or a typo. This is refactoring that should not affect functionality, but this change is beyond what I would characterise as "cosmetic".
Otherwise those are good changes, many thanks for cleaning this up and unifying the two drivers! 

Ok, I wasn't sure about that.
Thanks for clarifying.



On 16/10/2025 15:27, Richard Genoud wrote:

Harmonize registers definition in sunxi_nand{,_spl}.c files

This is a first step to then include the same file from both
sunxi_nand{,_spl}.c files


Please also mention here that you remove unused definitions.

Indeed, I forgot that.




Signed-off-by: Richard Genoud <[email protected]>
---
  drivers/mtd/nand/raw/sunxi_nand_spl.c | 148 ++++++++++++--------------
  1 file changed, 66 insertions(+), 82 deletions(-)
diff --git a/drivers/mtd/nand/raw/sunxi_nand_spl.c b/drivers/mtd/nand/ raw/sunxi_nand_spl.c 
index 19091ece632b..3ccf716b99ac 100644
--- a/drivers/mtd/nand/raw/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/raw/sunxi_nand_spl.c
@@ -14,48 +14,32 @@
  #include <linux/mtd/rawnand.h>
  /* registers */
-#define NFC_CTL                    0x00000000
-#define NFC_ST                     0x00000004
-#define NFC_INT                    0x00000008
-#define NFC_TIMING_CTL             0x0000000C
-#define NFC_TIMING_CFG             0x00000010
-#define NFC_ADDR_LOW               0x00000014
-#define NFC_ADDR_HIGH              0x00000018
-#define NFC_SECTOR_NUM             0x0000001C
-#define NFC_CNT                    0x00000020
-#define NFC_CMD                    0x00000024
-#define NFC_RCMD_SET               0x00000028
-#define NFC_WCMD_SET               0x0000002C
-#define NFC_IO_DATA                0x00000030
-#define NFC_ECC_CTL                0x00000034
-#define NFC_ECC_ST                 0x00000038
-#define NFC_DEBUG                  0x0000003C
-#define NFC_ECC_CNT0               0x00000040
-#define NFC_ECC_CNT1               0x00000044
-#define NFC_ECC_CNT2               0x00000048
-#define NFC_ECC_CNT3               0x0000004C
-#define NFC_USER_DATA_BASE         0x00000050
-#define NFC_EFNAND_STATUS          0x00000090
-#define NFC_SPARE_AREA             0x000000A0
-#define NFC_PATTERN_ID             0x000000A4
+#define NFC_REG_CTL                0x00000000
+#define NFC_REG_ST                 0x00000004
+#define NFC_REG_ADDR_LOW           0x00000014
+#define NFC_REG_ADDR_HIGH          0x00000018
+#define NFC_REG_CNT                0x00000020
+#define NFC_REG_CMD                0x00000024
+#define NFC_REG_RCMD_SET           0x00000028
+#define NFC_REG_ECC_CTL            0x00000034
+#define NFC_REG_ECC_ST             0x00000038
+#define NFC_REG_SPARE_AREA         0x000000A0
  #define NFC_RAM0_BASE              0x00000400
  #define NFC_RAM1_BASE              0x00000800
-#define NFC_CTL_EN                 (1 << 0)
-#define NFC_CTL_RESET              (1 << 1)
-#define NFC_CTL_RAM_METHOD         (1 << 14)
-#define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
-#define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
+#define NFC_EN                     (1 << 0)
Is there a strong reason to lose the _CTL_ infix here? I think it's better to keep the register name in the bit field definition, otherwise it becomes too generic and is prone to conflicts. 
Hum, it's like that in the Linux module and in sunxi_nand.c
But otherwise, no strong reason.




+#define NFC_RESET                  (1 << 1)
+#define NFC_PAGE_SHIFT_MSK         (0xf << 8)
+#define NFC_PAGE_SIZE(a)           ((fls(a) - 11) << 8)
  #define NFC_ECC_EN                 (1 << 0)
-#define NFC_ECC_PIPELINE           (1 << 3)
  #define NFC_ECC_EXCEPTION          (1 << 4)
-#define NFC_ECC_BLOCK_SIZE         (1 << 5)
-#define NFC_ECC_RANDOM_EN          (1 << 9)
-#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
+#define NFC_ECC_BLOCK_512          (1 << 5)
+#define NFC_RANDOM_EN              (1 << 9)
+#define NFC_RANDOM_DIRECTION       (1 << 10)
-#define NFC_ADDR_NUM_OFFSET        16
-#define NFC_SEND_ADDR              (1 << 19)
+#define NFC_ADR_NUM_OFFSET         16
+#define NFC_SEND_ADR               (1 << 19)


But ADDR with two D's looks more correct to me?
Defilitely, but same remark, it's NFC_SEND_ADR in the Linux module and sunxi_nand.c
In general, can't we keep the better version of each name, and adjust both the SPL and proper code?
I was a bit more on keeping consistency between the Linux defines and U-Boot defines, even if I agree with you, some term are so-so. 

But if you think I should change them, I'm open to it.

Thanks!



Cheers,
Andre


  #define NFC_ACCESS_DIR             (1 << 20)
  #define NFC_DATA_TRANS             (1 << 21)
  #define NFC_SEND_CMD1              (1 << 22)
@@ -66,17 +50,17 @@
  #define NFC_ROW_AUTO_INC           (1 << 27)
  #define NFC_SEND_CMD3              (1 << 28)
  #define NFC_SEND_CMD4              (1 << 29)
-#define NFC_RAW_CMD                (0 << 30)
-#define NFC_ECC_CMD                (1 << 30)
-#define NFC_PAGE_CMD               (2 << 30)
+#define NFC_NORMAL_OP              (0 << 30)
+#define NFC_ECC_OP                 (1 << 30)
+#define NFC_PAGE_OP                (2 << 30)
-#define NFC_ST_CMD_INT_FLAG        (1 << 1)
-#define NFC_ST_DMA_INT_FLAG        (1 << 2)
-#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
+#define NFC_CMD_INT_FLAG           (1 << 1)
+#define NFC_DMA_INT_FLAG           (1 << 2)
+#define NFC_CMD_FIFO_STATUS        (1 << 3)
  #define NFC_READ_CMD_OFFSET         0
-#define NFC_RANDOM_READ_CMD0_OFFSET 8
-#define NFC_RANDOM_READ_CMD1_OFFSET 16
+#define NFC_RND_READ_CMD0_OFFSET    8
+#define NFC_RND_READ_CMD1_OFFSET    16
  #define NFC_CMD_RNDOUTSTART        0xE0
  #define NFC_CMD_RNDOUT             0x05
@@ -143,8 +127,8 @@ static inline int check_value_negated(int offset, int unexpected_bits, 
  static int nand_wait_cmd_fifo_empty(void)
  {
-    if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT, 
-                 DEFAULT_TIMEOUT_US)) {
+    if (!check_value_negated(SUNXI_NFC_BASE + NFC_REG_ST,
+                 NFC_CMD_FIFO_STATUS, DEFAULT_TIMEOUT_US)) {
          printf("nand: timeout waiting for empty cmd FIFO\n");
          return -ETIMEDOUT;
      }
@@ -154,7 +138,7 @@ static int nand_wait_cmd_fifo_empty(void)
  static int nand_wait_int(void)
  {
-    if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
+    if (!check_value(SUNXI_NFC_BASE + NFC_REG_ST, NFC_CMD_INT_FLAG,
               DEFAULT_TIMEOUT_US)) {
          printf("nand: timeout waiting for interruption\n");
          return -ETIMEDOUT;
@@ -171,8 +155,8 @@ static int nand_exec_cmd(u32 cmd)
      if (ret)
          return ret;
-    writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
-    writel(cmd, SUNXI_NFC_BASE + NFC_CMD);
+    writel(NFC_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_REG_ST);
+    writel(cmd, SUNXI_NFC_BASE + NFC_REG_CMD);
      return nand_wait_int();
  }
@@ -183,13 +167,13 @@ void nand_init(void)
      board_nand_init();
-    val = readl(SUNXI_NFC_BASE + NFC_CTL);
+    val = readl(SUNXI_NFC_BASE + NFC_REG_CTL);
      /* enable and reset CTL */
-    writel(val | NFC_CTL_EN | NFC_CTL_RESET,
-           SUNXI_NFC_BASE + NFC_CTL);
+    writel(val | NFC_EN | NFC_RESET,
+           SUNXI_NFC_BASE + NFC_REG_CTL);
-    if (!check_value_negated(SUNXI_NFC_BASE + NFC_CTL,
-                 NFC_CTL_RESET, DEFAULT_TIMEOUT_US)) {
+    if (!check_value_negated(SUNXI_NFC_BASE + NFC_REG_CTL,
+                 NFC_RESET, DEFAULT_TIMEOUT_US)) {
          printf("Couldn't initialize nand\n");
      }
@@ -203,42 +187,42 @@ static void nand_apply_config(const struct nfc_config *conf) 
      nand_wait_cmd_fifo_empty();
-    val = readl(SUNXI_NFC_BASE + NFC_CTL);
-    val &= ~NFC_CTL_PAGE_SIZE_MASK;
-    writel(val | NFC_CTL_PAGE_SIZE(conf->page_size),
-           SUNXI_NFC_BASE + NFC_CTL);
-    writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
-    writel(conf->page_size, SUNXI_NFC_BASE + NFC_SPARE_AREA);
+    val = readl(SUNXI_NFC_BASE + NFC_REG_CTL);
+    val &= ~NFC_PAGE_SHIFT_MSK;
+    writel(val | NFC_PAGE_SIZE(conf->page_size),
+           SUNXI_NFC_BASE + NFC_REG_CTL);
+    writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_REG_CNT);
+    writel(conf->page_size, SUNXI_NFC_BASE + NFC_REG_SPARE_AREA);
  }
  static int nand_load_page(const struct nfc_config *conf, u32 offs)
  {
      int page = offs / conf->page_size;
-    writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
-           (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
+    writel((NFC_CMD_RNDOUTSTART << NFC_RND_READ_CMD1_OFFSET) |
+           (NFC_CMD_RNDOUT << NFC_RND_READ_CMD0_OFFSET) |
             (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
-           SUNXI_NFC_BASE + NFC_RCMD_SET);
-    writel(((page & 0xFFFF) << 16), SUNXI_NFC_BASE + NFC_ADDR_LOW);
-    writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
+           SUNXI_NFC_BASE + NFC_REG_RCMD_SET);
+    writel(((page & 0xFFFF) << 16), SUNXI_NFC_BASE + NFC_REG_ADDR_LOW);
+    writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_REG_ADDR_HIGH);
-    return nand_exec_cmd(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
-                 NFC_SEND_ADDR | NFC_WAIT_FLAG |
-                 ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET));
+    return nand_exec_cmd(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_NORMAL_OP |
+                 NFC_SEND_ADR | NFC_WAIT_FLAG |
+                 ((conf->addr_cycles - 1) << NFC_ADR_NUM_OFFSET));
  }
  static int nand_change_column(u16 column)
  {
      int ret;
-    writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
-           (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
+    writel((NFC_CMD_RNDOUTSTART << NFC_RND_READ_CMD1_OFFSET) |
+           (NFC_CMD_RNDOUT << NFC_RND_READ_CMD0_OFFSET) |
             (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
-           SUNXI_NFC_BASE + NFC_RCMD_SET);
-    writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
+           SUNXI_NFC_BASE + NFC_REG_RCMD_SET);
+    writel(column, SUNXI_NFC_BASE + NFC_REG_ADDR_LOW);
-    ret = nand_exec_cmd(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
-                (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR |
+    ret = nand_exec_cmd(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_NORMAL_OP |
+                (1 << NFC_ADR_NUM_OFFSET) | NFC_SEND_ADR |
                  NFC_CMD_RNDOUT);
      if (ret)
          return ret;
@@ -276,16 +260,16 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs, 
          u8 *data = dest + data_off;
          /* Clear ECC status and restart ECC engine */
-        writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
+        writel(0, SUNXI_NFC_BASE + NFC_REG_ECC_ST);
          writel((rand_seed << 16) | (conf->ecc_strength << 12) |
-               (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
-               (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
+               (conf->randomize ? NFC_RANDOM_EN : 0) |
+               (conf->ecc_size == 512 ? NFC_ECC_BLOCK_512 : 0) |
                 NFC_ECC_EN | NFC_ECC_EXCEPTION,
-               SUNXI_NFC_BASE + NFC_ECC_CTL);
+               SUNXI_NFC_BASE + NFC_REG_ECC_CTL);
          /* Move the data in SRAM */
          nand_change_column(data_off);
-        writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
+        writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_REG_CNT);
          nand_exec_cmd(NFC_DATA_TRANS);
          /*
@@ -293,10 +277,10 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs, 
           * the data.
           */
          nand_change_column(oob_off);
-        nand_exec_cmd(NFC_DATA_TRANS | NFC_ECC_CMD);
+        nand_exec_cmd(NFC_DATA_TRANS | NFC_ECC_OP);
          /* Get the ECC status */
-        ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
+        ecc_st = readl(SUNXI_NFC_BASE + NFC_REG_ECC_ST);
          /* ECC error detected. */
          if (ecc_st & 0xffff)
@@ -315,8 +299,8 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs, 
                    conf->ecc_size);
          /* Stop the ECC engine */
-        writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
-               SUNXI_NFC_BASE + NFC_ECC_CTL);
+        writel(readl(SUNXI_NFC_BASE + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
+               SUNXI_NFC_BASE + NFC_REG_ECC_CTL);
          if (data_off + conf->ecc_size >= len)
              break;



--
Richard Genoud, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Reply via email to