[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Boris Brezillon]

On Wed, 24 Jan 2018 09:06:26 +0100
Maxime Ripard <maxime.ripard at free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:51AM +0100, Miquel Raynal wrote:
> > Because using DMA implementation is not generic and was not developped
> > to work on SoCs like A33, migrate the SPL to use PIO.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal at free-electrons.com>  
> 
> I guess the issue is slightly different than what you're saying, or at
> least should be expressed differently.
> 
> The issue is that the SPL support has be done to support only the
> earlier generations of Allwinner SoCs, and only really enabled on the
> A13 / GR8. However, those old SoCs had a DMA engine that has been
> replaced since the A31 by another DMA controller that is no longer
> compatible.
> 
> Since the code directly uses that DMA controller, it cannot operate
> properly on the later SoCs, while the NAND controller hasn't changed.
> 
> There's two pathes forward, the first one would be to add support for
> that DMA controller too, or you can just remove the DMA usage entirely
> and rely on PIO that will make the driver simpler.
> 
> Explaining why you chose PIO would be great too.
> 
> > ---
> >  board/sunxi/board.c               |   4 +-
> >  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
> >  2 files changed, 79 insertions(+), 92 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 70e01437c4..512e2c17a6 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
> >  	struct sunxi_ccm_reg *const ccm =
> >  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >  
> > -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));  
> 
> That has nothing to do with PIO vs DMA. It should be in another patch.
> 
> >  #ifdef CONFIG_MACH_SUN9I
> >  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
> >  #else
> >  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));  
> 
> So you're de-asserting the reset line for the DMA controller, while
> the commit is about removing the need for that DMA controller ? :)
> 
> That whole block can actually be removed (in a separate patch) now
> that you're not using DMA anymore.
> 
> >  #endif
> >  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
> >  }
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 2488d5cb51..5de6825994 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <config.h>
> >  #include <nand.h>
> > +#include <linux/ctype.h>
> >  
> >  /* registers */
> >  #define NFC_CTL                    0x00000000
> > @@ -45,32 +46,22 @@
> >  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
> >  #define NFC_CTL_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_ADDR_NUM_OFFSET        16
> > -#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)
> >  #define NFC_SEND_CMD2              (1 << 24)
> > -#define NFC_SEQ                    (1 << 25)
> > -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> > -#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_PAGE_CMD               (2 << 30)
> > +#define NFC_ECC_CMD                (1 << 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)

Oh, I didn't notice you were removing some definitions. I
agree what Maxime says below, it's definitely not a good move.

> >  
> >  #define NFC_READ_CMD_OFFSET         0
> >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > @@ -80,22 +71,6 @@
> >  #define NFC_CMD_RNDOUT             0x05
> >  #define NFC_CMD_READSTART          0x30
> >  
> > -#define SUNXI_DMA_CFG_REG0              0x300
> > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > -
> > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > -
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > -  
> 
> We should probably keep those values. They're not used anymore, but
> the NAND controller has been under-documented on most of the datasheet
> we've had. It's good to keep it at least for reference.
> 

Hm, your statement is valid for the NFC_ defs, but these are purely DMA
engine related definitions, so I think we can get rid of them.