[PATCH 03/24] mtd: rawnand: sunxi_spl: cosmetic: harmonize register defines with non spl file
Andre Przywara
andre.przywara at arm.com
Fri Oct 17 16:27:58 CEST 2025
Hi Richard,
On 17/10/2025 15:22, Richard GENOUD wrote:
> 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 <richard.genoud at bootlin.com>
>>> ---
>>> 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.
Ah, I see. If it's like this in Linux, then please stick to this, even
if the naming raises some eyebrowses.
Thanks,
Andre
P.S. I will continue with the review in the next days, so please wait
before sending another version. I am tempted to take at least the
cleanups in the current merge window still, that should decrease your
patch queue significantly.
>
>>
>>> +#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;
>>
More information about the U-Boot
mailing list