Fwd: [PATCH v4] mtd: sunxi-nand: add mdma support for allwinner h3

Fwd: [PATCH v4] mtd: sunxi-nand: add mdma support for allwinner h3

[Manuel Dipolt]

hi Boris,

here are the answers from your review,
will send new patch V5.

With V5 mdma is now set as default dma method for a23

yours,
Manuel

> 
> ----- Original Message -----
> From: "Boris Brezillon" <boris.brezillon@collabora.com>
> To: "Manuel Dipolt" <mdipolt@robart.cc>
> Cc: "linux-mtd" <linux-mtd-bounces@lists.infradead.org>, "maxime" <maxime@cerno.tech>, "miquel raynal" <miquel.raynal@bootlin.com>
> Sent: Wednesday, October 7, 2020 5:53:21 PM
> Subject: 
> 
> On Wed, 7 Oct 2020 16:27:06 +0200 (CEST)
> Manuel Dipolt <mdipolt@robart.cc> wrote:
> 
> > The Allwinner H3 soc supports mdma mode, this patch enable support for it 
> > see old sunxi drivers https://github.com/allwinner-zh/linux-3.4-sunxi/tree/master/modules/nand
> 
> No need to point to the out of tree driver, stating that you add
> support for MDMA should be enough.


Will remove it, wanted to point out where i got the information for the mdma,
cause the information you can get from the allwinner datasheets is not enough.


> > 
> > 
> > Signed-off-by: Manuel Dipolt <manuel.dipolt@robart.cc>
> > ---
> >  drivers/mtd/nand/raw/sunxi_nand.c | 196 ++++++++++++++++++++++--------
> >  1 file changed, 147 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> > index 2a7ca3072f35..28ee2f44f179 100644
> > --- a/drivers/mtd/nand/raw/sunxi_nand.c
> > +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> > @@ -51,6 +51,7 @@
> >  #define NFC_REG_USER_DATA(x)	(0x0050 + ((x) * 4))
> >  #define NFC_REG_SPARE_AREA	0x00A0
> >  #define NFC_REG_PAT_ID		0x00A4
> > +#define NFC_REG_MDMA_ADDR	0x00C0
> >  #define NFC_REG_MDMA_CNT	0x00C4
> >  #define NFC_RAM0_BASE		0x0400
> >  #define NFC_RAM1_BASE		0x0800
> > @@ -60,7 +61,7 @@
> >  #define NFC_RESET		BIT(1)
> >  #define NFC_BUS_WIDTH_MSK	BIT(2)
> >  #define NFC_BUS_WIDTH_8		(0 << 2)
> > -#define NFC_BUS_WIDTH_16	(1 << 2)
> > +#define NFC_BUS_WIDTH_16	BIT(2)
> 
> No need to change that line, even if checkpatch complains. BTW, you
> should only run checkpatch on your patch, not the source file. I'm fine
> with this change if you:
> 
> * move it to a separate patch
> * drop the NFC_BUS_WIDTH_8 and NFC_BUS_WIDTH_MSK definitions
> 


Will revert all changes that aren't related to my mdma patch.


> >  #define NFC_RB_SEL_MSK		BIT(3)
> >  #define NFC_RB_SEL(x)		((x) << 3)
> >  #define NFC_CE_SEL_MSK		GENMASK(26, 24)
> > @@ -118,7 +119,7 @@
> >  #define NFC_SEND_CMD4		BIT(29)
> >  #define NFC_CMD_TYPE_MSK	GENMASK(31, 30)
> >  #define NFC_NORMAL_OP		(0 << 30)
> > -#define NFC_ECC_OP		(1 << 30)
> > +#define NFC_ECC_OP		BIT(30)
> 
> No, we should use BIT() in that case, that's an enum. I know checkpatch
> issues a warning for that, but that's fine.
> 


Ok will revert this changes


> >  #define NFC_PAGE_OP		(2U << 30)
> >  
> >  /* define bit use in NFC_RCMD_SET */
> > @@ -207,12 +208,14 @@ static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
> >   * NAND Controller capabilities structure: stores NAND controller capabilities
> >   * for distinction between compatible strings.
> >   *
> > + * @has_mdma:			use mbus dma mode, otherwise general dma
> 
> Try to align things properly, and start the description with an
> uppercase.
> 


Aligned and upper case now.


> 
> >   * @extra_mbus_conf:	Contrary to A10, A10s and A13, accessing internal RAM
> >   *			through MBUS on A23/A33 needs extra configuration.
> >   * @reg_io_data:	I/O data register
> >   * @dma_maxburst:	DMA maxburst
> >   */
> >  struct sunxi_nfc_caps {
> > +	bool has_mdma;
> >  	bool extra_mbus_conf;
> >  	unsigned int reg_io_data;
> >  	unsigned int dma_maxburst;
> > @@ -289,7 +292,7 @@ static int sunxi_nfc_wait_events(struct sunxi_nfc *nfc, u32 events,
> >  		writel(events, nfc->regs + NFC_REG_INT);
> >  
> >  		ret = wait_for_completion_timeout(&nfc->complete,
> > -						msecs_to_jiffies(timeout_ms));
> > +						  msecs_to_jiffies(timeout_ms));
> 
> Yet another unrelated coding-style change. Please move that to a
> separate patch if you really care about it.


reverted this change in my patch.


> 
> >  		if (!ret)
> >  			ret = -ETIMEDOUT;
> >  		else
> > @@ -393,6 +396,35 @@ static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf,
> >  	return ret;
> >  }
> >  
> > +static int sunxi_nfc_mdma_op_prepare(struct sunxi_nfc *nfc, const void *buf,
> > +				     int chunksize, int nchunks,
> > +				     enum dma_data_direction ddir,
> > +				     __u32 *mem_addr)
> 
> You mean "dma_addr_t *mem_addr". Anyway, I think you should use the same
> prototype for both sunxi_nfc_dma_op_cleanup() and
> sunxi_nfc_mdma_op_cleanup():


changed to dma_addr_t, moved cleanup functions together

> 
> static int sunxi_nfc_mdma_op_prepare(struct sunxi_nfc *nfc,
>                                      const void *buf,
>                                      int chunksize, int nchunks,
>                                      enum dma_data_direction ddir,
>                                      struct scatterlist *sg)
> 
> > +{
> > +	enum dma_transfer_direction tdir;
> > +	int ret;
> > +
> > +	if (ddir == DMA_FROM_DEVICE)
> > +		tdir = DMA_DEV_TO_MEM;
> > +	else
> > +		tdir = DMA_MEM_TO_DEV;
> > +
> > +	*mem_addr = (__u32)dma_map_single(nfc->dev, (void *)buf, nchunks * chunksize, tdir);
> 
> 	sg_init_one(sg, buf, nchunks * chunksize);
> 	ret = dma_map_sg(nfc->dev, sg, 1, ddir);
> 	if (!ret)
> 		return -ENOMEM;
> 
> > +
> > +	writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RAM_METHOD, nfc->regs + NFC_REG_CTL);
> > +	writel(nchunks, nfc->regs + NFC_REG_SECTOR_NUM);
> > +	writel(chunksize * nchunks, nfc->regs + NFC_REG_MDMA_CNT);
> > +	writel(*mem_addr, nfc->regs + NFC_REG_MDMA_ADDR);
> 
> 	writel(sg_dma_address(sg), nfc->regs + NFC_REG_MDMA_ADDR);
> 
> > +	writel(chunksize, nfc->regs + NFC_REG_CNT);
> > +
> > +	return 0;
> > +}
> 
> But now that I look at it, I feel like the mdma logic could be merged
> in the existing dma_op_prepare() function.


merged both prepare functions together


> 
> static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc,
>                                     const void *buf,
>                                     int chunksize, int nchunks,
>                                     enum dma_data_direction ddir,
>                                     struct scatterlist *sg)
> {
> 	struct dma_async_tx_descriptor *dmad;
> 	enum dma_transfer_direction tdir;
> 	dma_cookie_t dmat;
> 	int ret;
> 
> 	if (ddir == DMA_FROM_DEVICE)
> 		tdir = DMA_DEV_TO_MEM;
> 	else
> 		tdir = DMA_MEM_TO_DEV;
> 
> 	sg_init_one(sg, buf, nchunks * chunksize);
> 	ret = dma_map_sg(nfc->dev, sg, 1, ddir);
> 	if (!ret)
> 		return -ENOMEM;
> 
> 	if (!nfc->caps->has_mdma) {
> 		dmad = dmaengine_prep_slave_sg(nfc->dmac, sg, 1, tdir, DMA_CTRL_ACK);
> 		if (!dmad) {
> 			ret = -EINVAL;
> 			goto err_unmap_buf;
> 		}
> 	}
> 
> 	writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RAM_METHOD, nfc->regs + NFC_REG_CTL);
> 	writel(nchunks, nfc->regs + NFC_REG_SECTOR_NUM);
> 	writel(chunksize, nfc->regs + NFC_REG_CNT);
> 
> 	if (nfc->caps->has_mdma) {
> 		writel(chunksize * nchunks, nfc->regs + NFC_REG_MDMA_CNT);
> 		writel(sg_dma_address(sg), nfc->regs + NFC_REG_MDMA_ADDR);
> 	} else {
> 		dmat = dmaengine_submit(dmad);
> 
> 		ret = dma_submit_error(dmat);
> 		if (ret)
> 			goto err_clr_dma_flag;
> 	}
> 
> 	return 0;
> 
> err_clr_dma_flag:
> 	writel(readl(nfc->regs + NFC_REG_CTL) & ~NFC_RAM_METHOD, nfc->regs + NFC_REG_CTL);
> 
> err_unmap_buf:
> 	dma_unmap_sg(nfc->dev, sg, 1, ddir);
> 	return ret;
> }
> 
> 
> > +
> >  static void sunxi_nfc_dma_op_cleanup(struct sunxi_nfc *nfc,
> >  				     enum dma_data_direction ddir,
> >  				     struct scatterlist *sg)
> > @@ -402,6 +434,21 @@ static void sunxi_nfc_dma_op_cleanup(struct sunxi_nfc *nfc,
> >  	       nfc->regs + NFC_REG_CTL);
> >  }
> >  
> > +static void sunxi_nfc_mdma_op_cleanup(struct sunxi_nfc *nfc,
> > +				      enum dma_data_direction ddir,
> > +				      __u32 *mem_addr, size_t size)
> > +{
> > +	enum dma_transfer_direction tdir;
> > +
> > +	if (ddir == DMA_FROM_DEVICE)
> > +		tdir = DMA_DEV_TO_MEM;
> > +	else
> > +		tdir = DMA_MEM_TO_DEV;
> > +
> > +	dma_unmap_single(nfc->dev, *mem_addr, size, tdir);
> > +	writel(readl(nfc->regs + NFC_REG_CTL) & ~NFC_RAM_METHOD,  nfc->regs + NFC_REG_CTL);
> > +}
> 
> If you go for the option I suggested above you don't even need this
> sunxi_nfc_mdma_op_cleanup() helper.


merged both dma cleanup functions together


> 
> 
> > @@ -1199,7 +1260,7 @@ static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand,
> >  }
> >  
> >  static int sunxi_nfc_hw_ecc_write_page(struct nand_chip *nand,
> > -				       const uint8_t *buf, int oob_required,
> > +				       const u8 *buf, int oob_required,
> 
> Unrelated coding-style update.


reverted this change


> 
> >  				       int page)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(nand);
> > @@ -1277,6 +1338,8 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> >  	struct nand_ecc_ctrl *ecc = &nand->ecc;
> >  	struct scatterlist sg;
> >  	int ret, i;
> > +	u32 wait_event_flags;
> 
> Or just "wait".
> 


removed variable cause waiting again only for nfc interrupt


> > +	__u32 mem_addr;
> >  
> >  	sunxi_nfc_select_chip(nand, nand->cur_cs);
> >  
> > @@ -1284,10 +1347,14 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps,
> > -				       DMA_TO_DEVICE, &sg);
> > +	if (nfc->caps->has_mdma)
> > +		ret = sunxi_nfc_mdma_op_prepare(nfc, buf, ecc->size, ecc->steps,
> > +						DMA_TO_DEVICE, &mem_addr);
> > +	else
> > +		ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps, DMA_TO_DEVICE, &sg);
> > +
> >  	if (ret)
> > -		goto pio_fallback;
> > +		return ret;
> >  
> >  	for (i = 0; i < ecc->steps; i++) {
> >  		const u8 *oob = nand->oob_poi + (i * (ecc->bytes + 4));
> > @@ -1304,20 +1371,28 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> >  	writel((NAND_CMD_RNDIN << 8) | NAND_CMD_PAGEPROG,
> >  	       nfc->regs + NFC_REG_WCMD_SET);
> >  
> > -	dma_async_issue_pending(nfc->dmac);
> > +	wait_event_flags = NFC_CMD_INT_FLAG;
> > +
> > +	if (nfc->caps->has_mdma)
> > +		wait_event_flags = NFC_DMA_INT_FLAG;
> 
> Are you sure you don't need the NFC_CMD_INT_FLAG flag in that case? I'm
> pretty sure the DMA transfer is done before the PAGEPROG command is
> issued, meaning that you might be queuing new operations before the
> operation is actually finished.


can never be sure without any proper documentation about this nand controller ):
was guessing cause i saw dma int triggered,
tested again always both flags were set, so changed it to NFC_CMD_INT_FLAG to be on safe side

> 
> > +	else
> > +		dma_async_issue_pending(nfc->dmac);
> >  
> >  	writel(NFC_PAGE_OP | NFC_DATA_SWAP_METHOD |
> > -	       NFC_DATA_TRANS | NFC_ACCESS_DIR,
> > -	       nfc->regs + NFC_REG_CMD);
> > +		NFC_DATA_TRANS | NFC_ACCESS_DIR,
> > +		nfc->regs + NFC_REG_CMD);
> 
> Why? AFAICS those were already properly aligned.


assume the check script complained cause of the space at line begining, changed it back.


> 
> >  
> > -	ret = sunxi_nfc_wait_events(nfc, NFC_CMD_INT_FLAG, false, 0);
> > -	if (ret)
> > +	ret = sunxi_nfc_wait_events(nfc, wait_event_flags, false, 0);
> > +	if (ret && !nfc->caps->has_mdma)
> >  		dmaengine_terminate_all(nfc->dmac);
> >  
> >  	sunxi_nfc_randomizer_disable(nand);
> >  	sunxi_nfc_hw_ecc_disable(nand);
> >  
> > -	sunxi_nfc_dma_op_cleanup(nfc, DMA_TO_DEVICE, &sg);
> > +	if (nfc->caps->has_mdma)
> > +		sunxi_nfc_mdma_op_cleanup(nfc, DMA_TO_DEVICE, &mem_addr, ecc->size * ecc->steps);
> > +	else
> > +		sunxi_nfc_dma_op_cleanup(nfc, DMA_TO_DEVICE, &sg);
> >  
> >  	if (ret)
> >  		return ret;
> > @@ -1359,7 +1434,7 @@ static const s32 tWB_lut[] = {6, 12, 16, 20};
> >  static const s32 tRHW_lut[] = {4, 8, 12, 20};
> >  
> >  static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration,
> > -		u32 clk_period)
> > +				     u32 clk_period)
> >  {
> >  	u32 clk_cycles = DIV_ROUND_UP(duration, clk_period);
> >  	int i;
> > @@ -1695,7 +1770,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> >  	mtd_set_ooblayout(mtd, &sunxi_nand_ooblayout_ops);
> >  	ecc->priv = data;
> >  
> > -	if (nfc->dmac) {
> > +	if (nfc->dmac || nfc->caps->has_mdma) {
> >  		ecc->read_page = sunxi_nfc_hw_ecc_read_page_dma;
> >  		ecc->read_subpage = sunxi_nfc_hw_ecc_read_subpage_dma;
> >  		ecc->write_page = sunxi_nfc_hw_ecc_write_page_dma;
> > @@ -1949,10 +2024,8 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
> >  
> >  	sunxi_nand = devm_kzalloc(dev, struct_size(sunxi_nand, sels, nsels),
> >  				  GFP_KERNEL);
> > -	if (!sunxi_nand) {
> > -		dev_err(dev, "could not allocate chip\n");
> > +	if (!sunxi_nand)
> >  		return -ENOMEM;
> > -	}
> >  
> >  	sunxi_nand->nsels = nsels;
> >  
> > @@ -2058,6 +2131,41 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
> >  	}
> >  }
> >  
> > +static int sunxi_nfc_dma_init(struct sunxi_nfc *nfc, struct resource *r)
> > +{
> > +	int ret;
> > +
> > +	if (nfc->caps->has_mdma) {
> > +		writel(readl(nfc->regs + NFC_REG_CTL) & ~NFC_DMA_TYPE_NORMAL,
> > +		       nfc->regs + NFC_REG_CTL);
> 
> Don't you have to program the CTL reg any time you issue a command anyway?
> I'm pretty sure this write is useless, and if it's not, it probably papers
> over a real bug.


makes sense, will move it to prepare function


> 
> > +	} else {
> 
> You can drop one level of indentation if you do:


changed


> 
> 	if (nfc->caps->has_mdma)
> 		return;
> 
> > +		nfc->dmac = dma_request_chan(nfc->dev, "rxtx");
> > +		if (IS_ERR(nfc->dmac)) {
> > +			ret = PTR_ERR(nfc->dmac);
> > +			if (ret)
> > +				return ret;
> > +
> > +			/* Ignore errors to fall back to PIO mode */
> > +			dev_warn(nfc->dev, "failed to request rxtx DMA channel: %d\n", ret);
> > +		} else {
> > +			struct dma_slave_config dmac_cfg = { };
> > +
> > +			dmac_cfg.src_addr = r->start + nfc->caps->reg_io_data;
> > +			dmac_cfg.dst_addr = dmac_cfg.src_addr;
> > +			dmac_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +			dmac_cfg.dst_addr_width = dmac_cfg.src_addr_width;
> > +			dmac_cfg.src_maxburst = nfc->caps->dma_maxburst;
> > +			dmac_cfg.dst_maxburst = nfc->caps->dma_maxburst;
> > +			dmaengine_slave_config(nfc->dmac, &dmac_cfg);
> > +
> > +			if (nfc->caps->extra_mbus_conf)
> > +				writel(readl(nfc->regs + NFC_REG_CTL) |
> > +					NFC_DMA_TYPE_NORMAL, nfc->regs + NFC_REG_CTL);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int sunxi_nfc_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -2132,30 +2240,10 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto out_ahb_reset_reassert;
> >  
> > -	nfc->dmac = dma_request_chan(dev, "rxtx");
> > -	if (IS_ERR(nfc->dmac)) {
> > -		ret = PTR_ERR(nfc->dmac);
> > -		if (ret == -EPROBE_DEFER)
> > -			goto out_ahb_reset_reassert;
> > +	ret = sunxi_nfc_dma_init(nfc, r);
> >  
> > -		/* Ignore errors to fall back to PIO mode */
> > -		dev_warn(dev, "failed to request rxtx DMA channel: %d\n", ret);
> > -		nfc->dmac = NULL;
> > -	} else {
> > -		struct dma_slave_config dmac_cfg = { };
> > -
> > -		dmac_cfg.src_addr = r->start + nfc->caps->reg_io_data;
> > -		dmac_cfg.dst_addr = dmac_cfg.src_addr;
> > -		dmac_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > -		dmac_cfg.dst_addr_width = dmac_cfg.src_addr_width;
> > -		dmac_cfg.src_maxburst = nfc->caps->dma_maxburst;
> > -		dmac_cfg.dst_maxburst = nfc->caps->dma_maxburst;
> > -		dmaengine_slave_config(nfc->dmac, &dmac_cfg);
> > -
> > -		if (nfc->caps->extra_mbus_conf)
> > -			writel(readl(nfc->regs + NFC_REG_CTL) |
> > -			       NFC_DMA_TYPE_NORMAL, nfc->regs + NFC_REG_CTL);
> > -	}
> > +	if (ret)
> > +		goto out_ahb_reset_reassert;
> >  
> >  	platform_set_drvdata(pdev, nfc);
> >  
> > @@ -2197,16 +2285,22 @@ static int sunxi_nfc_remove(struct platform_device *pdev)
> >  }
> >  
> >  static const struct sunxi_nfc_caps sunxi_nfc_a10_caps = {
> > +	.has_mdma = false,
> 
> It's already false by default, you can drop that line.

dropped

> 
> >  	.reg_io_data = NFC_REG_A10_IO_DATA,
> >  	.dma_maxburst = 4,
> >  };
> >  
> >  static const struct sunxi_nfc_caps sunxi_nfc_a23_caps = {
> > +	.has_mdma = false,
> 
> I'd set that one to true (I'm pretty sure the A23 has an MDMA).


ok, removed h3 caps and compatible, changed the a23 caps to mdma


> 
> >  	.extra_mbus_conf = true,
> 
> If you set has_mdma to true, you can probably drop the extra_mbus_conf
> field...


dropped extra_mbus_conf option 


> 
> >  	.reg_io_data = NFC_REG_A23_IO_DATA,
> >  	.dma_maxburst = 8,
> 
> ... and leave that one to 0.


removed it, set only has_mdma to true now


> 
> >  };
> >  
> > +static const struct sunxi_nfc_caps sunxi_nfc_h3_caps = {
> > +	.has_mdma = true,
> > +};
> > +
> >  static const struct of_device_id sunxi_nfc_ids[] = {
> >  	{
> >  		.compatible = "allwinner,sun4i-a10-nand",
> > @@ -2216,6 +2310,10 @@ static const struct of_device_id sunxi_nfc_ids[] = {
> >  		.compatible = "allwinner,sun8i-a23-nand-controller",
> >  		.data = &sunxi_nfc_a23_caps,
> >  	},
> > +	{
> > +		.compatible = "allwinner,sun8i-h3-nand-controller",
> > +		.data = &sunxi_nfc_h3_caps,
> > +	},
> 
> I'm pretty sure you can re-use the a23 compat string for h3, no need to
> add a new one if it's the same HW block.


Ok, will remove the h3 compat string


> 
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);
> 
>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v4] mtd: sunxi-nand: add mdma support for allwinner h3

[Boris Brezillon]

On Thu, 8 Oct 2020 16:51:50 +0200 (CEST)
Manuel Dipolt <mdipolt@robart.cc> wrote:

> > >  				       int page)
> > >  {
> > >  	struct mtd_info *mtd = nand_to_mtd(nand);
> > > @@ -1277,6 +1338,8 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> > >  	struct nand_ecc_ctrl *ecc = &nand->ecc;
> > >  	struct scatterlist sg;
> > >  	int ret, i;
> > > +	u32 wait_event_flags;  
> > 
> > Or just "wait".
> >   
> 
> 
> removed variable cause waiting again only for nfc interrupt

I think you should wait for both.

> 
> 
> > > +	__u32 mem_addr;
> > >  
> > >  	sunxi_nfc_select_chip(nand, nand->cur_cs);
> > >  
> > > @@ -1284,10 +1347,14 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps,
> > > -				       DMA_TO_DEVICE, &sg);
> > > +	if (nfc->caps->has_mdma)
> > > +		ret = sunxi_nfc_mdma_op_prepare(nfc, buf, ecc->size, ecc->steps,
> > > +						DMA_TO_DEVICE, &mem_addr);
> > > +	else
> > > +		ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps, DMA_TO_DEVICE, &sg);
> > > +
> > >  	if (ret)
> > > -		goto pio_fallback;
> > > +		return ret;
> > >  
> > >  	for (i = 0; i < ecc->steps; i++) {
> > >  		const u8 *oob = nand->oob_poi + (i * (ecc->bytes + 4));
> > > @@ -1304,20 +1371,28 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
> > >  	writel((NAND_CMD_RNDIN << 8) | NAND_CMD_PAGEPROG,
> > >  	       nfc->regs + NFC_REG_WCMD_SET);
> > >  
> > > -	dma_async_issue_pending(nfc->dmac);
> > > +	wait_event_flags = NFC_CMD_INT_FLAG;
> > > +
> > > +	if (nfc->caps->has_mdma)
> > > +		wait_event_flags = NFC_DMA_INT_FLAG;  
> > 
> > Are you sure you don't need the NFC_CMD_INT_FLAG flag in that case? I'm
> > pretty sure the DMA transfer is done before the PAGEPROG command is
> > issued, meaning that you might be queuing new operations before the
> > operation is actually finished.  
> 
> 
> can never be sure without any proper documentation about this nand controller ):
> was guessing cause i saw dma int triggered,
> tested again always both flags were set, so changed it to NFC_CMD_INT_FLAG to be on safe side

Actually, if you want to be on safe side, you should probably set both
(and that applies to the read path as well, unless no commands are
issued in that case).


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v4] mtd: sunxi-nand: add mdma support for allwinner h3

[Manuel Dipolt]

The Allwinner H3 soc supports mdma mode, this patch enable support for it 
see old sunxi drivers https://github.com/allwinner-zh/linux-3.4-sunxi/tree/master/modules/nand


Signed-off-by: Manuel Dipolt <manuel.dipolt@robart.cc> 
--- 
drivers/mtd/nand/raw/sunxi_nand.c | 196 ++++++++++++++++++++++-------- 
1 file changed, 147 insertions(+), 49 deletions(-) 

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c 
index 2a7ca3072f35..28ee2f44f179 100644 
--- a/drivers/mtd/nand/raw/sunxi_nand.c 
+++ b/drivers/mtd/nand/raw/sunxi_nand.c 
@@ -51,6 +51,7 @@ 
#define NFC_REG_USER_DATA(x) (0x0050 + ((x) * 4)) 
#define NFC_REG_SPARE_AREA 0x00A0 
#define NFC_REG_PAT_ID 0x00A4 
+#define NFC_REG_MDMA_ADDR 0x00C0 
#define NFC_REG_MDMA_CNT 0x00C4 
#define NFC_RAM0_BASE 0x0400 
#define NFC_RAM1_BASE 0x0800 
@@ -60,7 +61,7 @@ 
#define NFC_RESET BIT(1) 
#define NFC_BUS_WIDTH_MSK BIT(2) 
#define NFC_BUS_WIDTH_8 (0 << 2) 
-#define NFC_BUS_WIDTH_16 (1 << 2) 
+#define NFC_BUS_WIDTH_16 BIT(2) 
#define NFC_RB_SEL_MSK BIT(3) 
#define NFC_RB_SEL(x) ((x) << 3) 
#define NFC_CE_SEL_MSK GENMASK(26, 24) 
@@ -118,7 +119,7 @@ 
#define NFC_SEND_CMD4 BIT(29) 
#define NFC_CMD_TYPE_MSK GENMASK(31, 30) 
#define NFC_NORMAL_OP (0 << 30) 
-#define NFC_ECC_OP (1 << 30) 
+#define NFC_ECC_OP BIT(30) 
#define NFC_PAGE_OP (2U << 30) 

/* define bit use in NFC_RCMD_SET */ 
@@ -207,12 +208,14 @@ static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand) 
* NAND Controller capabilities structure: stores NAND controller capabilities 
* for distinction between compatible strings. 
* 
+ * @has_mdma: use mbus dma mode, otherwise general dma 
* @extra_mbus_conf: Contrary to A10, A10s and A13, accessing internal RAM 
* through MBUS on A23/A33 needs extra configuration. 
* @reg_io_data: I/O data register 
* @dma_maxburst: DMA maxburst 
*/ 
struct sunxi_nfc_caps { 
+ bool has_mdma; 
bool extra_mbus_conf; 
unsigned int reg_io_data; 
unsigned int dma_maxburst; 
@@ -289,7 +292,7 @@ static int sunxi_nfc_wait_events(struct sunxi_nfc *nfc, u32 events, 
writel(events, nfc->regs + NFC_REG_INT); 

ret = wait_for_completion_timeout(&nfc->complete, 
- msecs_to_jiffies(timeout_ms)); 
+ msecs_to_jiffies(timeout_ms)); 
if (!ret) 
ret = -ETIMEDOUT; 
else 
@@ -393,6 +396,35 @@ static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf, 
return ret; 
} 

+static int sunxi_nfc_mdma_op_prepare(struct sunxi_nfc *nfc, const void *buf, 
+ int chunksize, int nchunks, 
+ enum dma_data_direction ddir, 
+ __u32 *mem_addr) 
+{ 
+ enum dma_transfer_direction tdir; 
+ int ret; 
+ 
+ if (ddir == DMA_FROM_DEVICE) 
+ tdir = DMA_DEV_TO_MEM; 
+ else 
+ tdir = DMA_MEM_TO_DEV; 
+ 
+ *mem_addr = (__u32)dma_map_single(nfc->dev, (void *)buf, nchunks * chunksize, tdir); 
+ ret = dma_mapping_error(nfc->dev, *mem_addr); 
+ if (ret) { 
+ dev_err(nfc->dev, "DMA mapping error\n"); 
+ return ret; 
+ } 
+ 
+ writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RAM_METHOD, nfc->regs + NFC_REG_CTL); 
+ writel(nchunks, nfc->regs + NFC_REG_SECTOR_NUM); 
+ writel(chunksize * nchunks, nfc->regs + NFC_REG_MDMA_CNT); 
+ writel(*mem_addr, nfc->regs + NFC_REG_MDMA_ADDR); 
+ writel(chunksize, nfc->regs + NFC_REG_CNT); 
+ 
+ return 0; 
+} 
+ 
static void sunxi_nfc_dma_op_cleanup(struct sunxi_nfc *nfc, 
enum dma_data_direction ddir, 
struct scatterlist *sg) 
@@ -402,6 +434,21 @@ static void sunxi_nfc_dma_op_cleanup(struct sunxi_nfc *nfc, 
nfc->regs + NFC_REG_CTL); 
} 

+static void sunxi_nfc_mdma_op_cleanup(struct sunxi_nfc *nfc, 
+ enum dma_data_direction ddir, 
+ __u32 *mem_addr, size_t size) 
+{ 
+ enum dma_transfer_direction tdir; 
+ 
+ if (ddir == DMA_FROM_DEVICE) 
+ tdir = DMA_DEV_TO_MEM; 
+ else 
+ tdir = DMA_MEM_TO_DEV; 
+ 
+ dma_unmap_single(nfc->dev, *mem_addr, size, tdir); 
+ writel(readl(nfc->regs + NFC_REG_CTL) & ~NFC_RAM_METHOD, nfc->regs + NFC_REG_CTL); 
+} 
+ 
static void sunxi_nfc_select_chip(struct nand_chip *nand, unsigned int cs) 
{ 
struct mtd_info *mtd = nand_to_mtd(nand); 
@@ -656,7 +703,7 @@ static void sunxi_nfc_randomize_bbm(struct nand_chip *nand, int page, u8 *bbm) 
} 

static void sunxi_nfc_randomizer_write_buf(struct nand_chip *nand, 
- const uint8_t *buf, int len, 
+ const u8 *buf, int len, 
bool ecc, int page) 
{ 
sunxi_nfc_randomizer_config(nand, page, ecc); 
@@ -911,14 +958,20 @@ static int sunxi_nfc_hw_ecc_read_chunks_dma(struct nand_chip *nand, uint8_t *buf 
unsigned int max_bitflips = 0; 
int ret, i, raw_mode = 0; 
struct scatterlist sg; 
- u32 status; 
+ u32 status, wait_event_flags; 
+ __u32 mem_addr; 

ret = sunxi_nfc_wait_cmd_fifo_empty(nfc); 
if (ret) 
return ret; 

- ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, nchunks, 
- DMA_FROM_DEVICE, &sg); 
+ if (nfc->caps->has_mdma) 
+ ret = sunxi_nfc_mdma_op_prepare(nfc, buf, ecc->size, nchunks, 
+ DMA_FROM_DEVICE, &mem_addr); 
+ else 
+ ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, nchunks, 
+ DMA_FROM_DEVICE, &sg); 
+ 
if (ret) 
return ret; 

@@ -929,19 +982,27 @@ static int sunxi_nfc_hw_ecc_read_chunks_dma(struct nand_chip *nand, uint8_t *buf 
writel((NAND_CMD_RNDOUTSTART << 16) | (NAND_CMD_RNDOUT << 8) | 
NAND_CMD_READSTART, nfc->regs + NFC_REG_RCMD_SET); 

- dma_async_issue_pending(nfc->dmac); 
+ wait_event_flags = NFC_CMD_INT_FLAG; 
+ 
+ if (nfc->caps->has_mdma) 
+ wait_event_flags = NFC_DMA_INT_FLAG; 
+ else 
+ dma_async_issue_pending(nfc->dmac); 

writel(NFC_PAGE_OP | NFC_DATA_SWAP_METHOD | NFC_DATA_TRANS, 
nfc->regs + NFC_REG_CMD); 

- ret = sunxi_nfc_wait_events(nfc, NFC_CMD_INT_FLAG, false, 0); 
- if (ret) 
+ ret = sunxi_nfc_wait_events(nfc, wait_event_flags, false, 0); 
+ if (ret && !nfc->caps->has_mdma) 
dmaengine_terminate_all(nfc->dmac); 

sunxi_nfc_randomizer_disable(nand); 
sunxi_nfc_hw_ecc_disable(nand); 

- sunxi_nfc_dma_op_cleanup(nfc, DMA_FROM_DEVICE, &sg); 
+ if (nfc->caps->has_mdma) 
+ sunxi_nfc_mdma_op_cleanup(nfc, DMA_FROM_DEVICE, &mem_addr, nchunks * ecc->size); 
+ else 
+ sunxi_nfc_dma_op_cleanup(nfc, DMA_FROM_DEVICE, &sg); 

if (ret) 
return ret; 
@@ -1199,7 +1260,7 @@ static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand, 
} 

static int sunxi_nfc_hw_ecc_write_page(struct nand_chip *nand, 
- const uint8_t *buf, int oob_required, 
+ const u8 *buf, int oob_required, 
int page) 
{ 
struct mtd_info *mtd = nand_to_mtd(nand); 
@@ -1277,6 +1338,8 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, 
struct nand_ecc_ctrl *ecc = &nand->ecc; 
struct scatterlist sg; 
int ret, i; 
+ u32 wait_event_flags; 
+ __u32 mem_addr; 

sunxi_nfc_select_chip(nand, nand->cur_cs); 

@@ -1284,10 +1347,14 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, 
if (ret) 
return ret; 

- ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps, 
- DMA_TO_DEVICE, &sg); 
+ if (nfc->caps->has_mdma) 
+ ret = sunxi_nfc_mdma_op_prepare(nfc, buf, ecc->size, ecc->steps, 
+ DMA_TO_DEVICE, &mem_addr); 
+ else 
+ ret = sunxi_nfc_dma_op_prepare(nfc, buf, ecc->size, ecc->steps, DMA_TO_DEVICE, &sg); 
+ 
if (ret) 
- goto pio_fallback; 
+ return ret; 

for (i = 0; i < ecc->steps; i++) { 
const u8 *oob = nand->oob_poi + (i * (ecc->bytes + 4)); 
@@ -1304,20 +1371,28 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, 
writel((NAND_CMD_RNDIN << 8) | NAND_CMD_PAGEPROG, 
nfc->regs + NFC_REG_WCMD_SET); 

- dma_async_issue_pending(nfc->dmac); 
+ wait_event_flags = NFC_CMD_INT_FLAG; 
+ 
+ if (nfc->caps->has_mdma) 
+ wait_event_flags = NFC_DMA_INT_FLAG; 
+ else 
+ dma_async_issue_pending(nfc->dmac); 

writel(NFC_PAGE_OP | NFC_DATA_SWAP_METHOD | 
- NFC_DATA_TRANS | NFC_ACCESS_DIR, 
- nfc->regs + NFC_REG_CMD); 
+ NFC_DATA_TRANS | NFC_ACCESS_DIR, 
+ nfc->regs + NFC_REG_CMD); 

- ret = sunxi_nfc_wait_events(nfc, NFC_CMD_INT_FLAG, false, 0); 
- if (ret) 
+ ret = sunxi_nfc_wait_events(nfc, wait_event_flags, false, 0); 
+ if (ret && !nfc->caps->has_mdma) 
dmaengine_terminate_all(nfc->dmac); 

sunxi_nfc_randomizer_disable(nand); 
sunxi_nfc_hw_ecc_disable(nand); 

- sunxi_nfc_dma_op_cleanup(nfc, DMA_TO_DEVICE, &sg); 
+ if (nfc->caps->has_mdma) 
+ sunxi_nfc_mdma_op_cleanup(nfc, DMA_TO_DEVICE, &mem_addr, ecc->size * ecc->steps); 
+ else 
+ sunxi_nfc_dma_op_cleanup(nfc, DMA_TO_DEVICE, &sg); 

if (ret) 
return ret; 
@@ -1359,7 +1434,7 @@ static const s32 tWB_lut[] = {6, 12, 16, 20}; 
static const s32 tRHW_lut[] = {4, 8, 12, 20}; 

static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration, 
- u32 clk_period) 
+ u32 clk_period) 
{ 
u32 clk_cycles = DIV_ROUND_UP(duration, clk_period); 
int i; 
@@ -1695,7 +1770,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand, 
mtd_set_ooblayout(mtd, &sunxi_nand_ooblayout_ops); 
ecc->priv = data; 

- if (nfc->dmac) { 
+ if (nfc->dmac || nfc->caps->has_mdma) { 
ecc->read_page = sunxi_nfc_hw_ecc_read_page_dma; 
ecc->read_subpage = sunxi_nfc_hw_ecc_read_subpage_dma; 
ecc->write_page = sunxi_nfc_hw_ecc_write_page_dma; 
@@ -1949,10 +2024,8 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc, 

sunxi_nand = devm_kzalloc(dev, struct_size(sunxi_nand, sels, nsels), 
GFP_KERNEL); 
- if (!sunxi_nand) { 
- dev_err(dev, "could not allocate chip\n"); 
+ if (!sunxi_nand) 
return -ENOMEM; 
- } 

sunxi_nand->nsels = nsels; 

@@ -2058,6 +2131,41 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc) 
} 
} 

+static int sunxi_nfc_dma_init(struct sunxi_nfc *nfc, struct resource *r) 
+{ 
+ int ret; 
+ 
+ if (nfc->caps->has_mdma) { 
+ writel(readl(nfc->regs + NFC_REG_CTL) & ~NFC_DMA_TYPE_NORMAL, 
+ nfc->regs + NFC_REG_CTL); 
+ } else { 
+ nfc->dmac = dma_request_chan(nfc->dev, "rxtx"); 
+ if (IS_ERR(nfc->dmac)) { 
+ ret = PTR_ERR(nfc->dmac); 
+ if (ret) 
+ return ret; 
+ 
+ /* Ignore errors to fall back to PIO mode */ 
+ dev_warn(nfc->dev, "failed to request rxtx DMA channel: %d\n", ret); 
+ } else { 
+ struct dma_slave_config dmac_cfg = { }; 
+ 
+ dmac_cfg.src_addr = r->start + nfc->caps->reg_io_data; 
+ dmac_cfg.dst_addr = dmac_cfg.src_addr; 
+ dmac_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; 
+ dmac_cfg.dst_addr_width = dmac_cfg.src_addr_width; 
+ dmac_cfg.src_maxburst = nfc->caps->dma_maxburst; 
+ dmac_cfg.dst_maxburst = nfc->caps->dma_maxburst; 
+ dmaengine_slave_config(nfc->dmac, &dmac_cfg); 
+ 
+ if (nfc->caps->extra_mbus_conf) 
+ writel(readl(nfc->regs + NFC_REG_CTL) | 
+ NFC_DMA_TYPE_NORMAL, nfc->regs + NFC_REG_CTL); 
+ } 
+ } 
+ return 0; 
+} 
+ 
static int sunxi_nfc_probe(struct platform_device *pdev) 
{ 
struct device *dev = &pdev->dev; 
@@ -2132,30 +2240,10 @@ static int sunxi_nfc_probe(struct platform_device *pdev) 
if (ret) 
goto out_ahb_reset_reassert; 

- nfc->dmac = dma_request_chan(dev, "rxtx"); 
- if (IS_ERR(nfc->dmac)) { 
- ret = PTR_ERR(nfc->dmac); 
- if (ret == -EPROBE_DEFER) 
- goto out_ahb_reset_reassert; 
+ ret = sunxi_nfc_dma_init(nfc, r); 

- /* Ignore errors to fall back to PIO mode */ 
- dev_warn(dev, "failed to request rxtx DMA channel: %d\n", ret); 
- nfc->dmac = NULL; 
- } else { 
- struct dma_slave_config dmac_cfg = { }; 
- 
- dmac_cfg.src_addr = r->start + nfc->caps->reg_io_data; 
- dmac_cfg.dst_addr = dmac_cfg.src_addr; 
- dmac_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; 
- dmac_cfg.dst_addr_width = dmac_cfg.src_addr_width; 
- dmac_cfg.src_maxburst = nfc->caps->dma_maxburst; 
- dmac_cfg.dst_maxburst = nfc->caps->dma_maxburst; 
- dmaengine_slave_config(nfc->dmac, &dmac_cfg); 
- 
- if (nfc->caps->extra_mbus_conf) 
- writel(readl(nfc->regs + NFC_REG_CTL) | 
- NFC_DMA_TYPE_NORMAL, nfc->regs + NFC_REG_CTL); 
- } 
+ if (ret) 
+ goto out_ahb_reset_reassert; 

platform_set_drvdata(pdev, nfc); 

@@ -2197,16 +2285,22 @@ static int sunxi_nfc_remove(struct platform_device *pdev) 
} 

static const struct sunxi_nfc_caps sunxi_nfc_a10_caps = { 
+ .has_mdma = false, 
.reg_io_data = NFC_REG_A10_IO_DATA, 
.dma_maxburst = 4, 
}; 

static const struct sunxi_nfc_caps sunxi_nfc_a23_caps = { 
+ .has_mdma = false, 
.extra_mbus_conf = true, 
.reg_io_data = NFC_REG_A23_IO_DATA, 
.dma_maxburst = 8, 
}; 

+static const struct sunxi_nfc_caps sunxi_nfc_h3_caps = { 
+ .has_mdma = true, 
+}; 
+ 
static const struct of_device_id sunxi_nfc_ids[] = { 
{ 
.compatible = "allwinner,sun4i-a10-nand", 
@@ -2216,6 +2310,10 @@ static const struct of_device_id sunxi_nfc_ids[] = { 
.compatible = "allwinner,sun8i-a23-nand-controller", 
.data = &sunxi_nfc_a23_caps, 
}, 
+ { 
+ .compatible = "allwinner,sun8i-h3-nand-controller", 
+ .data = &sunxi_nfc_h3_caps, 
+ }, 
{ /* sentinel */ } 
}; 
MODULE_DEVICE_TABLE(of, sunxi_nfc_ids); 
-- 
2.20.1 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/