On Wed, 24 Jan 2018 09:06:26 +0100 Maxime Ripard <maxime.rip...@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.ray...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot