Hi, On Wed, Jan 24, 2018 at 01:44:50AM +0100, Miquel Raynal wrote: > Do some cleaning in sunxi NAND SPL driver like adding helpers and > clearing flags at the right spot > > Signed-off-by: Miquel Raynal <miquel.ray...@free-electrons.com>
I'm not really fond of these kind of wildcard patches, especially
since..
> ---
> drivers/mtd/nand/sunxi_nand_spl.c | 64
> +++++++++++++++++++++++++--------------
> 1 file changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c
> b/drivers/mtd/nand/sunxi_nand_spl.c
> index 06695fc15f..2488d5cb51 100644
> --- a/drivers/mtd/nand/sunxi_nand_spl.c
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -55,8 +55,8 @@
>
>
> #define NFC_ADDR_NUM_OFFSET 16
> -#define NFC_SEND_ADR (1 << 19)
> #define NFC_ACCESS_DIR (1 << 20)
> +#define NFC_SEND_ADDR (1 << 19)
> #define NFC_DATA_TRANS (1 << 21)
> #define NFC_SEND_CMD1 (1 << 22)
> #define NFC_WAIT_FLAG (1 << 23)
... here you reorder a single value, which means that the set of these
values is not sorted anymore ...
> @@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int
> unexpected_bits,
> return check_value_inner(offset, unexpected_bits, timeout_us, 1);
> }
>
> +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)) {
> + printf("nand: timeout waiting for empty cmd FIFO\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +static int nand_wait_int(void)
> +{
> + if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> + DEFAULT_TIMEOUT_US)) {
> + printf("nand: timeout waiting for interruption\n");
> + return -ETIMEDOUT;
> + }
> +
> + udelay(1);
> +
> + return 0;
> +}
> +
... here you add the helpers that you talked about in your commit log,
good. but ...
> void nand_init(void)
> {
> uint32_t val;
> @@ -172,22 +196,21 @@ void nand_init(void)
> }
>
> /* reset NAND */
> + nand_wait_cmd_fifo_empty();
> +
... here you call one of these helpers where you didn't have that code
before, and that's not mentionned or explained in your commit log.
> writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
> SUNXI_NFC_BASE + NFC_CMD);
>
> - if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> - DEFAULT_TIMEOUT_US)) {
> - printf("Error timeout waiting for nand reset\n");
> - return;
> - }
> - writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> + nand_wait_int();
> }
>
> static void nand_apply_config(const struct nfc_config *conf)
> {
> u32 val;
>
> + nand_wait_cmd_fifo_empty();
> +
Ditto.
> val = readl(SUNXI_NFC_BASE + NFC_CTL);
> val &= ~NFC_CTL_PAGE_SIZE_MASK;
> writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
> @@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf,
> u32 offs)
> {
> int page = offs / conf->page_size;
>
> + nand_wait_cmd_fifo_empty();
> +
Ditto.
> writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
> @@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config
> *conf, u32 offs)
> writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
> writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
> - ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
> + ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,
I guess it was a typo fix then?
> SUNXI_NFC_BASE + NFC_CMD);
>
> - if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> - DEFAULT_TIMEOUT_US)) {
> - printf("Error while initializing dma interrupt\n");
> - return -EIO;
> - }
> -
> - return 0;
> + return nand_wait_int();
> }
>
> static int nand_reset_column(void)
> {
> + nand_wait_cmd_fifo_empty();
> +
Not mentionned or explained in your commit log.
> writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> SUNXI_NFC_BASE + NFC_RCMD_SET);
> + writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
Ditto
> writel(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> - (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
> + (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> SUNXI_NFC_BASE + NFC_CMD);
>
> - if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> - DEFAULT_TIMEOUT_US)) {
> - printf("Error while initializing dma interrupt\n");
> - return -1;
> - }
> + return nand_wait_int();
>
> - return 0;
> }
>
> +static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> +
> static int nand_read_page(const struct nfc_config *conf, u32 offs,
> void *dest, int len)
> {
> @@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf,
> u32 offs,
>
> static int nand_max_ecc_strength(struct nfc_config *conf)
> {
> - static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116
> };
Ditto.
Overall, I think it would be great to split that patch into 4:
- the first one to fix the ADR vs ADDR typo (without changing the order)
- the second one to introduce the nand_wait_int helper
- the third one to introduce the nand_wait_cmd_fifo_empty, with a
proper commit log explaining why this needs to be done and which
issues is it fixing
- And maybe if justified moving the ecc_bytes array to global scope
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
