Skip to contentSkip to conversation

mtd: sunxi-nand: add mdma support for allwinner h3

PATCH Superseded Manuel Dipolt 2 participants 2 messages Oct 8, 2020 4 messages not in the archive Memory technology devices (MTD)

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Manuel Dipolt

author
#4 lore raw

PATCH v4 1/1 mtd: sunxi-nand: add mdma support for allwinner h3

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

Quoted 8 lines

----- Original Message -----
From: "Boris Brezillon" <[email protected]>
To: "Manuel Dipolt" <[email protected]>
Cc: "linux-mtd" <[email protected]>, "maxime" <[email protected]>, "miquel raynal" <[email protected]>
Sent: Wednesday, October 7, 2020 5:53:21 PM
Subject:

Quoted 8 lines
Quoted 2 lines · On Wed, 7 Oct 2020 16:27:06 +0200 (CEST) Manuel Dipolt <[email protected]>…

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.

Quoted 30 lines
Quoted 20 lines
Signed-off-by Manuel Dipolt <[email protected]>
---
 drivers/mtd/nand/raw/sunxi_nand.c | 196 ++++++++++++++++++++++--------
 1 file changed, 147 insertions(+), 49 deletions(-)
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
index 2a7ca3072f35..28ee2f44f179 100644
@@ -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

drivers/mtd/nand/raw/sunxi_nand.c line 64

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -1
@@ -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:

Quoted 3 lines

* 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.

Quoted 4 lines
Quoted 4 lines
drivers/mtd/nand/raw/sunxi_nand.c +0 -0
 #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 @@
Quoted 9 lines

drivers/mtd/nand/raw/sunxi_nand.c line 122

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -1
 #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

Quoted 12 lines

drivers/mtd/nand/raw/sunxi_nand.c static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand) · line 211

Quoted 8 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
 #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.

Quoted 14 lines
Quoted 14 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
  * @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);
Quoted 6 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_wait_events(struct sunxi_nfc *nfc, u32 events, · line 295

Quoted 3 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -1
 		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.

Quoted 8 lines
Quoted 8 lines
drivers/mtd/nand/raw/sunxi_nand.c +0 -0
 		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;
 }
Quoted 8 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf, · line 402

Quoted 4 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -0
+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

Quoted 31 lines

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)

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf, · line 412

Quoted 10 lines
drivers/mtd/nand/raw/sunxi_nand.c +10 -0
+{
+	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;

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf, · line 417

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +5 -0
+
+	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);
Quoted 7 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_dma_op_prepare(struct sunxi_nfc *nfc, const void *buf, · line 421

Quoted 4 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -0
+	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

Quoted 73 lines

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;
}
Quoted 17 lines
drivers/mtd/nand/raw/sunxi_nand.c +10 -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
Quoted 8 lines

drivers/mtd/nand/raw/sunxi_nand.c static void sunxi_nfc_dma_op_cleanup(struct sunxi_nfc *nfc, · line 450

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +5 -0
+		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

Quoted 10 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand, · line 1263

Quoted 8 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -1
@@ -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

Quoted 12 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, · line 1341

Quoted 9 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
 				       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

Quoted 26 lines
Quoted 26 lines
drivers/mtd/nand/raw/sunxi_nand.c +8 -3
+	__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);
Quoted 10 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, · line 1377

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -1
-	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

Quoted 11 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, · line 1383

Quoted 9 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -2
+	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.

Quoted 53 lines
Quoted 53 lines
drivers/mtd/nand/raw/sunxi_nand.c +9 -8
-	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)
 	}
 }
Quoted 11 lines

drivers/mtd/nand/raw/sunxi_nand.c static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc) · line 2140

Quoted 7 lines
drivers/mtd/nand/raw/sunxi_nand.c +7 -0
+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

Quoted 4 lines

drivers/mtd/nand/raw/sunxi_nand.c static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc) · line 2141

Quoted 2 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
+	} else {

You can drop one level of indentation if you do:

changed

Quoted 68 lines
	if (nfc->caps->has_mdma)
		return;
Quoted 64 lines
drivers/mtd/nand/raw/sunxi_nand.c +30 -23
+		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);
Quoted 7 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_remove(struct platform_device *pdev) · line 2288

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
@@ -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

Quoted 9 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_remove(struct platform_device *pdev) · line 2294

Quoted 7 lines
drivers/mtd/nand/raw/sunxi_nand.c +1 -0
 	.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

Quoted 5 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_remove(struct platform_device *pdev) · line 2295

Quoted 2 lines
drivers/mtd/nand/raw/sunxi_nand.c +0 -0
 	.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

Quoted 5 lines

drivers/mtd/nand/raw/sunxi_nand.c static int sunxi_nfc_remove(struct platform_device *pdev) · line 2297

Quoted 3 lines
drivers/mtd/nand/raw/sunxi_nand.c +0 -0
 	.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

Quoted 13 lines
Quoted 13 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -0
 };
 
+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,
Quoted 8 lines

drivers/mtd/nand/raw/sunxi_nand.c static const struct of_device_id sunxi_nfc_ids[] = { · line 2316

Quoted 5 lines
drivers/mtd/nand/raw/sunxi_nand.c +4 -0
 	},
+	{
+		.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

Quoted 6 lines

drivers/mtd/nand/raw/sunxi_nand.c static const struct of_device_id sunxi_nfc_ids[] = { · line 2319

Quoted 4 lines
drivers/mtd/nand/raw/sunxi_nand.c +0 -0
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);

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

#5 lore raw
Quoted 13 lines · On Thu, 8 Oct 2020 16:51:50 +0200 (CEST) Manuel Dipolt <[email protected]>…
Quoted 11 lines

Review static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, · line 1341

Quoted 8 lines
(quoted diff) +1 -0
 				       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".

Quoted 1 line

removed variable cause waiting again only for nfc interrupt

I think you should wait for both.

Quoted 40 lines
Quoted 28 lines
Quoted 28 lines
(quoted diff) +8 -3
+	__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);
Quoted 10 lines

Review static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand, · line 1377

Quoted 5 lines
(quoted diff) +4 -1
-	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.

Quoted 3 lines

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/