[RFC PATCH v5 0/6] refactoring, fixes and updates for Meson NAND

[RFC PATCH v5 0/6] refactoring, fixes and updates for Meson NAND

[Arseniy Krasnov]

Hello,

this patchset does several things:

1) Fixes value of ready/busy command. This new value was suggested by
   Liang Yang <liang.yang@amlogic.com>.

2) Adds waiting for command completion by calling 'nand_soft_waitrdy()'
   instead of using ready/busy pin and command from 1). This is really
   needed because I don't have device with such pin implemented.

3) It moves OOB free bytes to non-protected by ECC area. Here are some
   details:

   Current OOB free bytes are 4 bytes (2 x 2 user bytes) under ECC engine.
   Here is how it looks like in the current implementation:

   [ 2B user bytes ][     14B ECC codes    ]
   [ 2B user bytes ][     14B ECC codes    ]
   [ 16B unused area, not protected by ECC ]
   [ 16B unused area, not protected by ECC ]

   All 4 user bytes are protected by ECC. This patch changes OOB free
   bytes in this way:

   [ 2B unused area ][     14B ECC codes     ]
   [ 2B unused area ][     14B ECC codes     ]
   [  16B user bytes, not protected by ECC   ]
   [  16B user bytes, not protected by ECC   ]

   Now OOB user bytes are 32 bytes instead of 4 bytes and not protected
   by ECC.

   Motivation of this layout comes from problem with JFFS2. It uses OOB
   free bytes for cleanmarkers. Each cleanmarker is 4 bytes and written
   by JFFS2 driver (small remark - cleanmarkers are always written in
   case of NAND storage for JFFS2).
   We have two ways to write this data to OOB (e.g. user bytes):

   1) ECC mode. In this case it will be ECC covered user bytes, e.g.
      writing this bytes will update ECC codes. Problem fires, when
      JFFS2 tries to write this page later - this write makes controller
      to update ECC codes again, but it is impossible to do it correctly,
      because we can't update bits from 0 to 1 (only from 1 to 0).

   2) Raw mode. In this case ECC codes won't be updated. But later, it
      will be impossible to read this page in ECC mode, because we have
      some user bytes, but ECC codes are missed.

   So let's move OOB free bytes out of ECC area. In this case we can
   read/write OOB separately in raw mode and at the same time work with
   data in ECC mode. JFFS2 is happy now. User bytes are untouched - all
   of them are ignored during non-OOB access.

   I've tested this with mount/unmount/read/write cases for JFFS2 and
   nanddump/nandwrite utlities on AXG family (A113X SoC).

   Here is link to discussion:
   https://lore.kernel.org/linux-mtd/a9f8307a-77d7-a69f-ce11-2629909172d2@sberdevices.ru/T/#m3087bd06386a7f430cd5e343e22b25d724d3e2d7

4) Replaces calculation of OOB related thing with macros. This is just
   cosmetic change.

5) Checks buffer length on accesses to NAND controller.

6) Removes useless bitwise OR with zeroes.

Link to v1:
https://lore.kernel.org/linux-mtd/20230412061700.1492474-1-AVKrasnov@sberdevices.ru/
Link to v2:
https://lore.kernel.org/linux-mtd/20230426073632.3905682-1-AVKrasnov@sberdevices.ru/
Link to v3:
https://lore.kernel.org/linux-mtd/20230510110835.26115-1-AVKrasnov@sberdevices.ru/
Link to v4:
https://lore.kernel.org/linux-mtd/20230515094440.3552094-1-AVKrasnov@sberdevices.ru/

Changelog:

v1 -> v2:
 * Add patch which renames dts value for chip select.
 * Add patch which moves OOB to non-protected ECC area.
v2 -> v3:
 * Change patch which fixes read/write access according discussion link
   in 1) above.
v3 -> v4:
 * Remove patch which renames dts value for chip select.
   Here is link to discussion:
   https://lore.kernel.org/linux-mtd/20230510110835.26115-7-AVKrasnov@sberdevices.ru/
 * Pass 1 to 'meson_nfc_queue_rb()' in case of NAND_OP_WAITRDY_INSTR.
   This fixes ONFI page processing during NAND driver initialization.
v4 -> v5:
 * Move update of 'NFC_CMD_RB_INT' to the separate patch.
 * Replace code which uses extra status and READ0 commands for waiting
   command with 'nand_soft_waitrdy()'. In fact this patch adds second
   mode for command waiting by using 'nand_soft_waitrdy()'.
 * For OOB layout patch see changelog in a patch file.
 * For check length patch see changelog in a patch file.

Arseniy Krasnov (6):
  mtd: rawnand: meson: fix ready/busy command
  mtd: rawnand: meson: wait for command in polling mode
  mtd: rawnand: meson: only expose unprotected user OOB bytes
  mtd: rawnand: meson: use macro for OOB area
  mtd: rawnand: meson: check buffer length
  mtd: rawnand: meson: remove unneeded bitwise OR with zeroes

 drivers/mtd/nand/raw/meson_nand.c | 234 +++++++++++++++++++++++-------
 1 file changed, 182 insertions(+), 52 deletions(-)

-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 1/6] mtd: rawnand: meson: fix ready/busy command

[Arseniy Krasnov]

This fixes ready/busy command value.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 074e14225c06..9dd4a676497b 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -37,7 +37,7 @@
 #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
 #define NFC_CMD_SCRAMBLER_DISABLE	0
 #define NFC_CMD_SHORTMODE_DISABLE	0
-#define NFC_CMD_RB_INT		BIT(14)
+#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
 
 #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
 
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 1/6] mtd: rawnand: meson: fix ready/busy command

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:44 +0300:

> This fixes ready/busy command value.

nit: "Fix the ready/busy command value."
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 074e14225c06..9dd4a676497b 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -37,7 +37,7 @@
>  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
>  #define NFC_CMD_SCRAMBLER_DISABLE	0
>  #define NFC_CMD_SHORTMODE_DISABLE	0
> -#define NFC_CMD_RB_INT		BIT(14)
> +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
>  
>  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
>  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 1/6] mtd: rawnand: meson: fix ready/busy command

[Arseniy Krasnov]

Hello Miquel!

May be I can exclude this patch from this patchset and send it as a single patch
as it is fix and not related with other patches?

Thanks, Arseniy

On 01.06.2023 10:51, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:44 +0300:
> 
>> This fixes ready/busy command value.
> 
> nit: "Fix the ready/busy command value."
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  drivers/mtd/nand/raw/meson_nand.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index 074e14225c06..9dd4a676497b 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -37,7 +37,7 @@
>>  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
>>  #define NFC_CMD_SCRAMBLER_DISABLE	0
>>  #define NFC_CMD_SHORTMODE_DISABLE	0
>> -#define NFC_CMD_RB_INT		BIT(14)
>> +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
>>  
>>  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
>>  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 1/6] mtd: rawnand: meson: fix ready/busy command

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Fri, 2 Jun 2023 01:44:01 +0300:

> Hello Miquel!
> 
> May be I can exclude this patch from this patchset and send it as a single patch
> as it is fix and not related with other patches?

Yes absolutely.

> 
> Thanks, Arseniy
> 
> On 01.06.2023 10:51, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:44 +0300:
> >   
> >> This fixes ready/busy command value.  
> > 
> > nit: "Fix the ready/busy command value."  
> >>
> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >> ---
> >>  drivers/mtd/nand/raw/meson_nand.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >> index 074e14225c06..9dd4a676497b 100644
> >> --- a/drivers/mtd/nand/raw/meson_nand.c
> >> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >> @@ -37,7 +37,7 @@
> >>  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
> >>  #define NFC_CMD_SCRAMBLER_DISABLE	0
> >>  #define NFC_CMD_SHORTMODE_DISABLE	0
> >> -#define NFC_CMD_RB_INT		BIT(14)
> >> +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
> >>  
> >>  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
> >>    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

This adds support of waiting for command completion in sofyware polling
mode. It is needed when ready/busy pin is not implemented in hardware.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 9dd4a676497b..82a629025adc 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -179,6 +179,7 @@ struct meson_nfc {
 	u32 info_bytes;
 
 	unsigned long assigned_cs;
+	bool use_polling;
 };
 
 enum {
@@ -392,32 +393,38 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
 	}
 }
 
-static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
+static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
 {
-	u32 cmd, cfg;
-	int ret = 0;
+	struct meson_nfc *nfc = nand_get_controller_data(nand);
 
-	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
-	meson_nfc_drain_cmd(nfc);
-	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
+	if (nfc->use_polling) {
+		return nand_soft_waitrdy(nand, timeout_ms);
+	} else {
+		u32 cmd, cfg;
+		int ret = 0;
 
-	cfg = readl(nfc->reg_base + NFC_REG_CFG);
-	cfg |= NFC_RB_IRQ_EN;
-	writel(cfg, nfc->reg_base + NFC_REG_CFG);
+		meson_nfc_cmd_idle(nfc, nfc->timing.twb);
+		meson_nfc_drain_cmd(nfc);
+		meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
 
-	reinit_completion(&nfc->completion);
+		cfg = readl(nfc->reg_base + NFC_REG_CFG);
+		cfg |= NFC_RB_IRQ_EN;
+		writel(cfg, nfc->reg_base + NFC_REG_CFG);
 
-	/* use the max erase time as the maximum clock for waiting R/B */
-	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
-		| nfc->param.chip_select | nfc->timing.tbers_max;
-	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+		reinit_completion(&nfc->completion);
 
-	ret = wait_for_completion_timeout(&nfc->completion,
-					  msecs_to_jiffies(timeout_ms));
-	if (ret == 0)
-		ret = -1;
+		/* use the max erase time as the maximum clock for waiting R/B */
+		cmd = NFC_CMD_RB | NFC_CMD_RB_INT
+			| nfc->param.chip_select | nfc->timing.tbers_max;
+		writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
-	return ret;
+		ret = wait_for_completion_timeout(&nfc->completion,
+						  msecs_to_jiffies(timeout_ms));
+		if (ret == 0)
+			return -ETIMEDOUT;
+
+		return 0;
+	}
 }
 
 static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
@@ -623,7 +630,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
 	if (in) {
 		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
 		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
-		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
+		meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tR_max));
 	} else {
 		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
 	}
@@ -669,7 +676,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
 
 	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
-	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
+	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
 
 	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
 
@@ -952,7 +959,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
 			break;
 
 		case NAND_OP_WAITRDY_INSTR:
-			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
+			meson_nfc_queue_rb(nand, instr->ctx.waitrdy.timeout_ms);
 			if (instr->delay_ns)
 				meson_nfc_cmd_idle(nfc, delay_idle);
 			break;
@@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
+
 	writel(0, nfc->reg_base + NFC_REG_CFG);
 	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
 	if (ret) {
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 01.06.2023 09:18, Arseniy Krasnov wrote:
> This adds support of waiting for command completion in sofyware polling

                                                         ^^^ will fix it

> mode. It is needed when ready/busy pin is not implemented in hardware.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 9dd4a676497b..82a629025adc 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -179,6 +179,7 @@ struct meson_nfc {
>  	u32 info_bytes;
>  
>  	unsigned long assigned_cs;
> +	bool use_polling;
>  };
>  
>  enum {
> @@ -392,32 +393,38 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  	}
>  }
>  
> -static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
> +static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
>  {
> -	u32 cmd, cfg;
> -	int ret = 0;
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>  
> -	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> -	meson_nfc_drain_cmd(nfc);
> -	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> +	if (nfc->use_polling) {
> +		return nand_soft_waitrdy(nand, timeout_ms);
> +	} else {
> +		u32 cmd, cfg;
> +		int ret = 0;
>  
> -	cfg = readl(nfc->reg_base + NFC_REG_CFG);
> -	cfg |= NFC_RB_IRQ_EN;
> -	writel(cfg, nfc->reg_base + NFC_REG_CFG);
> +		meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +		meson_nfc_drain_cmd(nfc);
> +		meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>  
> -	reinit_completion(&nfc->completion);
> +		cfg = readl(nfc->reg_base + NFC_REG_CFG);
> +		cfg |= NFC_RB_IRQ_EN;
> +		writel(cfg, nfc->reg_base + NFC_REG_CFG);
>  
> -	/* use the max erase time as the maximum clock for waiting R/B */
> -	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> -		| nfc->param.chip_select | nfc->timing.tbers_max;
> -	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +		reinit_completion(&nfc->completion);
>  
> -	ret = wait_for_completion_timeout(&nfc->completion,
> -					  msecs_to_jiffies(timeout_ms));
> -	if (ret == 0)
> -		ret = -1;
> +		/* use the max erase time as the maximum clock for waiting R/B */
> +		cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> +			| nfc->param.chip_select | nfc->timing.tbers_max;
> +		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  
> -	return ret;
> +		ret = wait_for_completion_timeout(&nfc->completion,
> +						  msecs_to_jiffies(timeout_ms));
> +		if (ret == 0)
> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
>  }
>  
>  static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> @@ -623,7 +630,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>  	if (in) {
>  		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
>  		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
> -		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
> +		meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tR_max));
>  	} else {
>  		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>  	}
> @@ -669,7 +676,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>  
>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> -	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
> +	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>  
>  	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
>  
> @@ -952,7 +959,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
>  			break;
>  
>  		case NAND_OP_WAITRDY_INSTR:
> -			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
> +			meson_nfc_queue_rb(nand, instr->ctx.waitrdy.timeout_ms);
>  			if (instr->delay_ns)
>  				meson_nfc_cmd_idle(nfc, delay_idle);
>  			break;
> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
> +
>  	writel(0, nfc->reg_base + NFC_REG_CFG);
>  	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>  	if (ret) {

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:45 +0300:

> This adds support of waiting for command completion in sofyware polling

							software

> mode. It is needed when ready/busy pin is not implemented in hardware.

Please also use (here and in all your commits) the affirmative tense:

"Add support for "

instead of

"This adds support"

or

"This commit adds"

Also, this is not a fix but a feature, so it should be introduced after
all the fixes. This way I can take all the fixes first without
dependency.

> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 9dd4a676497b..82a629025adc 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -179,6 +179,7 @@ struct meson_nfc {
>  	u32 info_bytes;
>  
>  	unsigned long assigned_cs;
> +	bool use_polling;

Very ambiguous wording. Polling is usually what you do to get the data.
Here you want a control signal so I would rename this flag with
something like "no_rb_pin".

Do you have a driver structure to represent the nand chip? Because
there is one RB per chip (even per die), not per controller.

>  };
>  
>  enum {
> @@ -392,32 +393,38 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  	}
>  }
>  
> -static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
> +static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)

I would rather prefer keeping the controller pointer here. It's your
main structure here.

>  {
> -	u32 cmd, cfg;
> -	int ret = 0;
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>  
> -	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> -	meson_nfc_drain_cmd(nfc);
> -	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> +	if (nfc->use_polling) {
> +		return nand_soft_waitrdy(nand, timeout_ms);

You could simplify the diff by a lot by avoiding this extra tab
you added in the second part of the function, using:

	if (no_rb_pin)
		return nand_soft_waitrdy();

	...

> +	} else {
> +		u32 cmd, cfg;
> +		int ret = 0;
>  
> -	cfg = readl(nfc->reg_base + NFC_REG_CFG);
> -	cfg |= NFC_RB_IRQ_EN;
> -	writel(cfg, nfc->reg_base + NFC_REG_CFG);
> +		meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +		meson_nfc_drain_cmd(nfc);
> +		meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>  
> -	reinit_completion(&nfc->completion);
> +		cfg = readl(nfc->reg_base + NFC_REG_CFG);
> +		cfg |= NFC_RB_IRQ_EN;
> +		writel(cfg, nfc->reg_base + NFC_REG_CFG);
>  
> -	/* use the max erase time as the maximum clock for waiting R/B */
> -	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> -		| nfc->param.chip_select | nfc->timing.tbers_max;
> -	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +		reinit_completion(&nfc->completion);
>  
> -	ret = wait_for_completion_timeout(&nfc->completion,
> -					  msecs_to_jiffies(timeout_ms));
> -	if (ret == 0)
> -		ret = -1;
> +		/* use the max erase time as the maximum clock for waiting R/B */
> +		cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> +			| nfc->param.chip_select | nfc->timing.tbers_max;
> +		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  
> -	return ret;
> +		ret = wait_for_completion_timeout(&nfc->completion,
> +						  msecs_to_jiffies(timeout_ms));
> +		if (ret == 0)
> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
>  }
>  
>  static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> @@ -623,7 +630,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>  	if (in) {
>  		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
>  		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
> -		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
> +		meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tR_max));

Let's avoid that.

>  	} else {
>  		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>  	}
> @@ -669,7 +676,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>  
>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> -	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
> +	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>  
>  	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
>  
> @@ -952,7 +959,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
>  			break;
>  
>  		case NAND_OP_WAITRDY_INSTR:
> -			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
> +			meson_nfc_queue_rb(nand, instr->ctx.waitrdy.timeout_ms);
>  			if (instr->delay_ns)
>  				meson_nfc_cmd_idle(nfc, delay_idle);
>  			break;
> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");

This is a problem. You cannot add a polling property like that.

There is already a nand-rb property which is supposed to carry how are
wired the RB lines. I don't see any in-tree users of the compatibles, I
don't know how acceptable it is to consider using soft fallback when
this property is missing, otherwise take the values of the rb lines
provided in the DT and user hardware control, but I would definitely
prefer that.

In any case you'll need a dt-binding update which must be acked by
dt-binding maintainers.

> +
>  	writel(0, nfc->reg_base + NFC_REG_CFG);
>  	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>  	if (ret) {


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 01.06.2023 11:07, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:45 +0300:
> 
>> This adds support of waiting for command completion in sofyware polling
> 
> 							software
> 
>> mode. It is needed when ready/busy pin is not implemented in hardware.
> 
> Please also use (here and in all your commits) the affirmative tense:
> 
> "Add support for "
> 
> instead of
> 
> "This adds support"
> 
> or
> 
> "This commit adds"
> 
> Also, this is not a fix but a feature, so it should be introduced after
> all the fixes. This way I can take all the fixes first without
> dependency.

Ack

> 
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
>>  1 file changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index 9dd4a676497b..82a629025adc 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -179,6 +179,7 @@ struct meson_nfc {
>>  	u32 info_bytes;
>>  
>>  	unsigned long assigned_cs;
>> +	bool use_polling;
> 
> Very ambiguous wording. Polling is usually what you do to get the data.
> Here you want a control signal so I would rename this flag with
> something like "no_rb_pin".

Ack

> 
> Do you have a driver structure to represent the nand chip? Because
> there is one RB per chip (even per die), not per controller.
> 
>>  };
>>  
>>  enum {
>> @@ -392,32 +393,38 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>  	}
>>  }
>>  
>> -static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
>> +static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
> 
> I would rather prefer keeping the controller pointer here. It's your
> main structure here.

Ack

> 
>>  {
>> -	u32 cmd, cfg;
>> -	int ret = 0;
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>  
>> -	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
>> -	meson_nfc_drain_cmd(nfc);
>> -	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>> +	if (nfc->use_polling) {
>> +		return nand_soft_waitrdy(nand, timeout_ms);
> 
> You could simplify the diff by a lot by avoiding this extra tab
> you added in the second part of the function, using:
> 
> 	if (no_rb_pin)
> 		return nand_soft_waitrdy();
> 
> 	...
> 
>> +	} else {
>> +		u32 cmd, cfg;
>> +		int ret = 0;
>>  
>> -	cfg = readl(nfc->reg_base + NFC_REG_CFG);
>> -	cfg |= NFC_RB_IRQ_EN;
>> -	writel(cfg, nfc->reg_base + NFC_REG_CFG);
>> +		meson_nfc_cmd_idle(nfc, nfc->timing.twb);
>> +		meson_nfc_drain_cmd(nfc);
>> +		meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>>  
>> -	reinit_completion(&nfc->completion);
>> +		cfg = readl(nfc->reg_base + NFC_REG_CFG);
>> +		cfg |= NFC_RB_IRQ_EN;
>> +		writel(cfg, nfc->reg_base + NFC_REG_CFG);
>>  
>> -	/* use the max erase time as the maximum clock for waiting R/B */
>> -	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
>> -		| nfc->param.chip_select | nfc->timing.tbers_max;
>> -	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +		reinit_completion(&nfc->completion);
>>  
>> -	ret = wait_for_completion_timeout(&nfc->completion,
>> -					  msecs_to_jiffies(timeout_ms));
>> -	if (ret == 0)
>> -		ret = -1;
>> +		/* use the max erase time as the maximum clock for waiting R/B */
>> +		cmd = NFC_CMD_RB | NFC_CMD_RB_INT
>> +			| nfc->param.chip_select | nfc->timing.tbers_max;
>> +		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>  
>> -	return ret;
>> +		ret = wait_for_completion_timeout(&nfc->completion,
>> +						  msecs_to_jiffies(timeout_ms));
>> +		if (ret == 0)
>> +			return -ETIMEDOUT;
>> +
>> +		return 0;
>> +	}
>>  }
>>  
>>  static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>> @@ -623,7 +630,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>  	if (in) {
>>  		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
>>  		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
>> -		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
>> +		meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tR_max));
> 
> Let's avoid that.
> 
>>  	} else {
>>  		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>>  	}
>> @@ -669,7 +676,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>  
>>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> -	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
>> +	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>>  
>>  	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
>>  
>> @@ -952,7 +959,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
>>  			break;
>>  
>>  		case NAND_OP_WAITRDY_INSTR:
>> -			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
>> +			meson_nfc_queue_rb(nand, instr->ctx.waitrdy.timeout_ms);
>>  			if (instr->delay_ns)
>>  				meson_nfc_cmd_idle(nfc, delay_idle);
>>  			break;
>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> +	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
> 
> This is a problem. You cannot add a polling property like that.
> 
> There is already a nand-rb property which is supposed to carry how are
> wired the RB lines. I don't see any in-tree users of the compatibles, I
> don't know how acceptable it is to consider using soft fallback when
> this property is missing, otherwise take the values of the rb lines
> provided in the DT and user hardware control, but I would definitely
> prefer that.

I see. So i need to implement processing of this property here? And if it
is missed -> use software waiting. I think interesting thing will be that:

1) Even with support of this property here, I really don't know how to pass
   RB values to this controller - I just have define for RB command and that's
   it. I found that this property is an array of u32 - IIUC each element is
   RB pin per chip. May be i need to dive into the old vendor's driver to find
   how to use RB values (although this driver uses software waiting so I'm not
   sure that I'll find something in it).
2) I can't test RB mode - I don't have such device :( 

Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
in controller specific register for waiting (I guess Meson controller has something
like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
'nand-rb' property, but never use it.

> 
> In any case you'll need a dt-binding update which must be acked by
> dt-binding maintainers.

You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?

Thanks, Arseniy

> 
>> +
>>  	writel(0, nfc->reg_base + NFC_REG_CFG);
>>  	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>>  	if (ret) {
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

> >> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
> >>  		return ret;
> >>  	}
> >>  
> >> +	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");  
> > 
> > This is a problem. You cannot add a polling property like that.
> > 
> > There is already a nand-rb property which is supposed to carry how are
> > wired the RB lines. I don't see any in-tree users of the compatibles, I
> > don't know how acceptable it is to consider using soft fallback when
> > this property is missing, otherwise take the values of the rb lines
> > provided in the DT and user hardware control, but I would definitely
> > prefer that.  
> 
> I see. So i need to implement processing of this property here? And if it
> is missed -> use software waiting. I think interesting thing will be that:
> 
> 1) Even with support of this property here, I really don't know how to pass
>    RB values to this controller - I just have define for RB command and that's
>    it. I found that this property is an array of u32 - IIUC each element is
>    RB pin per chip. May be i need to dive into the old vendor's driver to find
>    how to use RB values (although this driver uses software waiting so I'm not
>    sure that I'll find something in it).

Liang, can you please give use the relevant information here? How do we
target RB0 and RB1? It seems like you use the CS as only information
like if the RB lines where hardwired internally to a CS. Can we invert
the lines with a specific configuration?

Arseniy, if the answer to my above question is no, then you should
expect the nand-rb and reg arrays to be identical. If they are not,
then you can return -EINVAL.

If the nand-rb property is missing, then fallback to software wait.

> 2) I can't test RB mode - I don't have such device :( 
> 
> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> in controller specific register for waiting (I guess Meson controller has something
> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> 'nand-rb' property, but never use it.

Yes, the logic around the second RB line (taking care of CS1/CS3) is
slightly broken or at least badly documented, and thus should not be
used.

> > In any case you'll need a dt-binding update which must be acked by
> > dt-binding maintainers.  
> 
> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?

Yes. In a dedicated patch. Something along the lines:

	nand-rb: true

inside the nand chip object should be fine. And flag the change as a
fix because we should have used and parsed this property since the
beginning.

Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Liang Yang]

Hi Miquel and Arseniy,


On 2023/6/5 17:05, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Arseniy,
> 
>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>>>             return ret;
>>>>     }
>>>>
>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
>>>
>>> This is a problem. You cannot add a polling property like that.
>>>
>>> There is already a nand-rb property which is supposed to carry how are
>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>> don't know how acceptable it is to consider using soft fallback when
>>> this property is missing, otherwise take the values of the rb lines
>>> provided in the DT and user hardware control, but I would definitely
>>> prefer that.
>>
>> I see. So i need to implement processing of this property here? And if it
>> is missed -> use software waiting. I think interesting thing will be that:
>>
>> 1) Even with support of this property here, I really don't know how to pass
>>     RB values to this controller - I just have define for RB command and that's
>>     it. I found that this property is an array of u32 - IIUC each element is
>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>     how to use RB values (although this driver uses software waiting so I'm not
>>     sure that I'll find something in it).
> 
> Liang, can you please give use the relevant information here? How do we
> target RB0 and RB1? It seems like you use the CS as only information
> like if the RB lines where hardwired internally to a CS. Can we invert
> the lines with a specific configuration?

Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
of different CEs need to be bound into one wire and connect with
NAND_RB0 if want to use controller polling rb. the current operating
CE of NAND is decided to "chip_select", of course controller internally 
has different nfc commands to regconize which Ce's RB signal is polling.

<&nand_pins> in dts/yaml should include the NAND_RB0 if hardware 
connects, or use software polling here.

@Arseniy, sorry, i don't travel all the informations yet. but why don't 
you use the new RB_INT command with irq that i provided in another 
thread. the new RB_INT command doesn't depend on the physical RB wires, 
it also send the READ status command(0x70) and wait for the irq wake up 
completion.

> Arseniy, if the answer to my above question is no, then you should
> expect the nand-rb and reg arrays to be identical. If they are not,
> then you can return -EINVAL.
> 
> If the nand-rb property is missing, then fallback to software wait.
> 
>> 2) I can't test RB mode - I don't have such device :(
>>
>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>> in controller specific register for waiting (I guess Meson controller has something
>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>> 'nand-rb' property, but never use it.
> 
> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> slightly broken or at least badly documented, and thus should not be
> used.
> 
>>> In any case you'll need a dt-binding update which must be acked by
>>> dt-binding maintainers.
>>
>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
> 
> Yes. In a dedicated patch. Something along the lines:
> 
>          nand-rb: true
> 
> inside the nand chip object should be fine. And flag the change as a
> fix because we should have used and parsed this property since the
> beginning.
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Liang Yang]

On 2023/6/5 21:19, Liang Yang wrote:
> Hi Miquel and Arseniy,
> 
> 
> On 2023/6/5 17:05, Miquel Raynal wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi Arseniy,
>>
>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct 
>>>>> platform_device *pdev)
>>>>>             return ret;
>>>>>     }
>>>>>
>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
>>>>
>>>> This is a problem. You cannot add a polling property like that.
>>>>
>>>> There is already a nand-rb property which is supposed to carry how are
>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>>> don't know how acceptable it is to consider using soft fallback when
>>>> this property is missing, otherwise take the values of the rb lines
>>>> provided in the DT and user hardware control, but I would definitely
>>>> prefer that.
>>>
>>> I see. So i need to implement processing of this property here? And 
>>> if it
>>> is missed -> use software waiting. I think interesting thing will be 
>>> that:
>>>
>>> 1) Even with support of this property here, I really don't know how 
>>> to pass
>>>     RB values to this controller - I just have define for RB command 
>>> and that's
>>>     it. I found that this property is an array of u32 - IIUC each 
>>> element is
>>>     RB pin per chip. May be i need to dive into the old vendor's 
>>> driver to find
>>>     how to use RB values (although this driver uses software waiting 
>>> so I'm not
>>>     sure that I'll find something in it).
>>
>> Liang, can you please give use the relevant information here? How do we
>> target RB0 and RB1? It seems like you use the CS as only information
>> like if the RB lines where hardwired internally to a CS. Can we invert
>> the lines with a specific configuration?
> 
> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> of different CEs need to be bound into one wire and connect with
> NAND_RB0 if want to use controller polling rb. the current operating
> CE of NAND is decided to "chip_select", of course controller internally 
> has different nfc commands to regconize which Ce's RB signal is polling.
> 
> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware 
> connects, or use software polling here.
> 
> @Arseniy, sorry, i don't travel all the informations yet. but why don't 
> you use the new RB_INT command with irq that i provided in another 
> thread. the new RB_INT command doesn't depend on the physical RB wires, 
> it also send the READ status command(0x70) and wait for the irq wake up 
> completion.

Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) 
or new RB_INT(no physical RB wires). the new RB_INT command decides the 
RB0 or RB1 by the previous command with ce args.

> 
>> Arseniy, if the answer to my above question is no, then you should
>> expect the nand-rb and reg arrays to be identical. If they are not,
>> then you can return -EINVAL.
>>
>> If the nand-rb property is missing, then fallback to software wait.
>>
>>> 2) I can't test RB mode - I don't have such device :(
>>>
>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values 
>>> are used
>>> in controller specific register for waiting (I guess Meson controller 
>>> has something
>>> like that, but I don't have doc). While in marvell_nand.c it looks 
>>> like that they parse
>>> 'nand-rb' property, but never use it.
>>
>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>> slightly broken or at least badly documented, and thus should not be
>> used.
>>
>>>> In any case you'll need a dt-binding update which must be acked by
>>>> dt-binding maintainers.
>>>
>>> You mean to add this property desc to 
>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>
>> Yes. In a dedicated patch. Something along the lines:
>>
>>          nand-rb: true
>>
>> inside the nand chip object should be fine. And flag the change as a
>> fix because we should have used and parsed this property since the
>> beginning.
>>
>> Thanks,
>> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 05.06.2023 16:30, Liang Yang wrote:
> 
> 
> On 2023/6/5 21:19, Liang Yang wrote:
>> Hi Miquel and Arseniy,
>>
>>
>> On 2023/6/5 17:05, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi Arseniy,
>>>
>>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>>>>>             return ret;
>>>>>>     }
>>>>>>
>>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
>>>>>
>>>>> This is a problem. You cannot add a polling property like that.
>>>>>
>>>>> There is already a nand-rb property which is supposed to carry how are
>>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>>>> don't know how acceptable it is to consider using soft fallback when
>>>>> this property is missing, otherwise take the values of the rb lines
>>>>> provided in the DT and user hardware control, but I would definitely
>>>>> prefer that.
>>>>
>>>> I see. So i need to implement processing of this property here? And if it
>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>
>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>     RB values to this controller - I just have define for RB command and that's
>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>     sure that I'll find something in it).
>>>
>>> Liang, can you please give use the relevant information here? How do we
>>> target RB0 and RB1? It seems like you use the CS as only information
>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>> the lines with a specific configuration?
>>
>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>> of different CEs need to be bound into one wire and connect with
>> NAND_RB0 if want to use controller polling rb. the current operating
>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>
>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>
>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.

Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
implemented RB_INT as interrupt driven way. What do You think Miquel ?

> 
> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> 

So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?

Thanks, Arseniy

>>
>>> Arseniy, if the answer to my above question is no, then you should
>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>> then you can return -EINVAL.
>>>
>>> If the nand-rb property is missing, then fallback to software wait.
>>>
>>>> 2) I can't test RB mode - I don't have such device :(
>>>>
>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>> in controller specific register for waiting (I guess Meson controller has something
>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>> 'nand-rb' property, but never use it.
>>>
>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>> slightly broken or at least badly documented, and thus should not be
>>> used.
>>>
>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>> dt-binding maintainers.
>>>>
>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>>
>>> Yes. In a dedicated patch. Something along the lines:
>>>
>>>          nand-rb: true
>>>
>>> inside the nand chip object should be fine. And flag the change as a
>>> fix because we should have used and parsed this property since the
>>> beginning.
>>>
>>> Thanks,
>>> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:

> On 05.06.2023 16:30, Liang Yang wrote:
> > 
> > 
> > On 2023/6/5 21:19, Liang Yang wrote:  
> >> Hi Miquel and Arseniy,
> >>
> >>
> >> On 2023/6/5 17:05, Miquel Raynal wrote:  
> >>> [ EXTERNAL EMAIL ]
> >>>
> >>> Hi Arseniy,
> >>>  
> >>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
> >>>>>>             return ret;
> >>>>>>     }
> >>>>>>
> >>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");  
> >>>>>
> >>>>> This is a problem. You cannot add a polling property like that.
> >>>>>
> >>>>> There is already a nand-rb property which is supposed to carry how are
> >>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
> >>>>> don't know how acceptable it is to consider using soft fallback when
> >>>>> this property is missing, otherwise take the values of the rb lines
> >>>>> provided in the DT and user hardware control, but I would definitely
> >>>>> prefer that.  
> >>>>
> >>>> I see. So i need to implement processing of this property here? And if it
> >>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>
> >>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>     RB values to this controller - I just have define for RB command and that's
> >>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>     sure that I'll find something in it).  
> >>>
> >>> Liang, can you please give use the relevant information here? How do we
> >>> target RB0 and RB1? It seems like you use the CS as only information
> >>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>> the lines with a specific configuration?  
> >>
> >> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >> of different CEs need to be bound into one wire and connect with
> >> NAND_RB0 if want to use controller polling rb. the current operating
> >> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>
> >> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>
> >> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
> 
> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
> implemented RB_INT as interrupt driven way. What do You think Miquel ?
> 
> > 
> > Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >   
> 
> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?

As long as it works and does not contain any extremely strange READ0 or
READ_STATUS in the middle of nothing, I'm fine, take the simplest
approach which will work for all.

> 
> Thanks, Arseniy
> 
> >>  
> >>> Arseniy, if the answer to my above question is no, then you should
> >>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>> then you can return -EINVAL.
> >>>
> >>> If the nand-rb property is missing, then fallback to software wait.
> >>>  
> >>>> 2) I can't test RB mode - I don't have such device :(
> >>>>
> >>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>> in controller specific register for waiting (I guess Meson controller has something
> >>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>> 'nand-rb' property, but never use it.  
> >>>
> >>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>> slightly broken or at least badly documented, and thus should not be
> >>> used.
> >>>  
> >>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>> dt-binding maintainers.  
> >>>>
> >>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
> >>>
> >>> Yes. In a dedicated patch. Something along the lines:
> >>>
> >>>          nand-rb: true
> >>>
> >>> inside the nand chip object should be fine. And flag the change as a
> >>> fix because we should have used and parsed this property since the
> >>> beginning.
> >>>
> >>> Thanks,
> >>> Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 06.06.2023 10:03, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:
> 
>> On 05.06.2023 16:30, Liang Yang wrote:
>>>
>>>
>>> On 2023/6/5 21:19, Liang Yang wrote:  
>>>> Hi Miquel and Arseniy,
>>>>
>>>>
>>>> On 2023/6/5 17:05, Miquel Raynal wrote:  
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Arseniy,
>>>>>  
>>>>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>>>>>>>             return ret;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");  
>>>>>>>
>>>>>>> This is a problem. You cannot add a polling property like that.
>>>>>>>
>>>>>>> There is already a nand-rb property which is supposed to carry how are
>>>>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>>>>>> don't know how acceptable it is to consider using soft fallback when
>>>>>>> this property is missing, otherwise take the values of the rb lines
>>>>>>> provided in the DT and user hardware control, but I would definitely
>>>>>>> prefer that.  
>>>>>>
>>>>>> I see. So i need to implement processing of this property here? And if it
>>>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>>>
>>>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>>>     RB values to this controller - I just have define for RB command and that's
>>>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>>>     sure that I'll find something in it).  
>>>>>
>>>>> Liang, can you please give use the relevant information here? How do we
>>>>> target RB0 and RB1? It seems like you use the CS as only information
>>>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>>>> the lines with a specific configuration?  
>>>>
>>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>>>> of different CEs need to be bound into one wire and connect with
>>>> NAND_RB0 if want to use controller polling rb. the current operating
>>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>>>
>>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>>>
>>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
>>
>> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
>> implemented RB_INT as interrupt driven way. What do You think Miquel ?
>>
>>>
>>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
>>>   
>>
>> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?
> 
> As long as it works and does not contain any extremely strange READ0 or
> READ_STATUS in the middle of nothing, I'm fine, take the simplest
> approach which will work for all.

"extremetely strange READ0" is method which uses STATUS, interrupt, READ0? This method was
described by Liang.

And You mean to use the following logic:
if ("nand-rb" == true)
    use RB_INT which requires wire
else
    use 'nand_soft_waitrdy()'

?

Thanks, Arseniy

> 
>>
>> Thanks, Arseniy
>>
>>>>  
>>>>> Arseniy, if the answer to my above question is no, then you should
>>>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>>>> then you can return -EINVAL.
>>>>>
>>>>> If the nand-rb property is missing, then fallback to software wait.
>>>>>  
>>>>>> 2) I can't test RB mode - I don't have such device :(
>>>>>>
>>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>>>> in controller specific register for waiting (I guess Meson controller has something
>>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>>>> 'nand-rb' property, but never use it.  
>>>>>
>>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>>>> slightly broken or at least badly documented, and thus should not be
>>>>> used.
>>>>>  
>>>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>>>> dt-binding maintainers.  
>>>>>>
>>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
>>>>>
>>>>> Yes. In a dedicated patch. Something along the lines:
>>>>>
>>>>>          nand-rb: true
>>>>>
>>>>> inside the nand chip object should be fine. And flag the change as a
>>>>> fix because we should have used and parsed this property since the
>>>>> beginning.
>>>>>
>>>>> Thanks,
>>>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 10:40:21 +0300:

> On 06.06.2023 10:03, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:
> >   
> >> On 05.06.2023 16:30, Liang Yang wrote:  
> >>>
> >>>
> >>> On 2023/6/5 21:19, Liang Yang wrote:    
> >>>> Hi Miquel and Arseniy,
> >>>>
> >>>>
> >>>> On 2023/6/5 17:05, Miquel Raynal wrote:    
> >>>>> [ EXTERNAL EMAIL ]
> >>>>>
> >>>>> Hi Arseniy,
> >>>>>    
> >>>>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
> >>>>>>>>             return ret;
> >>>>>>>>     }
> >>>>>>>>
> >>>>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");    
> >>>>>>>
> >>>>>>> This is a problem. You cannot add a polling property like that.
> >>>>>>>
> >>>>>>> There is already a nand-rb property which is supposed to carry how are
> >>>>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
> >>>>>>> don't know how acceptable it is to consider using soft fallback when
> >>>>>>> this property is missing, otherwise take the values of the rb lines
> >>>>>>> provided in the DT and user hardware control, but I would definitely
> >>>>>>> prefer that.    
> >>>>>>
> >>>>>> I see. So i need to implement processing of this property here? And if it
> >>>>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>>>
> >>>>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>>>     RB values to this controller - I just have define for RB command and that's
> >>>>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>>>     sure that I'll find something in it).    
> >>>>>
> >>>>> Liang, can you please give use the relevant information here? How do we
> >>>>> target RB0 and RB1? It seems like you use the CS as only information
> >>>>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>>>> the lines with a specific configuration?    
> >>>>
> >>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >>>> of different CEs need to be bound into one wire and connect with
> >>>> NAND_RB0 if want to use controller polling rb. the current operating
> >>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>>>
> >>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>>>
> >>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.    
> >>
> >> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
> >> implemented RB_INT as interrupt driven way. What do You think Miquel ?
> >>  
> >>>
> >>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >>>     
> >>
> >> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?  
> > 
> > As long as it works and does not contain any extremely strange READ0 or
> > READ_STATUS in the middle of nothing, I'm fine, take the simplest
> > approach which will work for all.  
> 
> "extremetely strange READ0" is method which uses STATUS, interrupt, READ0? This method was
> described by Liang.

It needs to be very well contained in dedicated helpers and documented.
You choose what is easier for you (Liang's method or
nand_soft_waitrdy()), but I don't want to see spurious READ0 or
READ_STATUS calls inside read/write_page helpers like before.

> And You mean to use the following logic:
> if ("nand-rb" == true)
>     use RB_INT which requires wire
> else
>     use 'nand_soft_waitrdy()'
> 
> ?
> 
> Thanks, Arseniy
> 
> >   
> >>
> >> Thanks, Arseniy
> >>  
> >>>>    
> >>>>> Arseniy, if the answer to my above question is no, then you should
> >>>>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>>>> then you can return -EINVAL.
> >>>>>
> >>>>> If the nand-rb property is missing, then fallback to software wait.
> >>>>>    
> >>>>>> 2) I can't test RB mode - I don't have such device :(
> >>>>>>
> >>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>>>> in controller specific register for waiting (I guess Meson controller has something
> >>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>>>> 'nand-rb' property, but never use it.    
> >>>>>
> >>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>>>> slightly broken or at least badly documented, and thus should not be
> >>>>> used.
> >>>>>    
> >>>>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>>>> dt-binding maintainers.    
> >>>>>>
> >>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?    
> >>>>>
> >>>>> Yes. In a dedicated patch. Something along the lines:
> >>>>>
> >>>>>          nand-rb: true
> >>>>>
> >>>>> inside the nand chip object should be fine. And flag the change as a
> >>>>> fix because we should have used and parsed this property since the
> >>>>> beginning.
> >>>>>
> >>>>> Thanks,
> >>>>> Miquèl    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 05.06.2023 16:30, Liang Yang wrote:
> 
> 
> On 2023/6/5 21:19, Liang Yang wrote:
>> Hi Miquel and Arseniy,
>>
>>
>> On 2023/6/5 17:05, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi Arseniy,
>>>
>>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>>>>>             return ret;
>>>>>>     }
>>>>>>
>>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");
>>>>>
>>>>> This is a problem. You cannot add a polling property like that.
>>>>>
>>>>> There is already a nand-rb property which is supposed to carry how are
>>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>>>> don't know how acceptable it is to consider using soft fallback when
>>>>> this property is missing, otherwise take the values of the rb lines
>>>>> provided in the DT and user hardware control, but I would definitely
>>>>> prefer that.
>>>>
>>>> I see. So i need to implement processing of this property here? And if it
>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>
>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>     RB values to this controller - I just have define for RB command and that's
>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>     sure that I'll find something in it).
>>>
>>> Liang, can you please give use the relevant information here? How do we
>>> target RB0 and RB1? It seems like you use the CS as only information
>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>> the lines with a specific configuration?
>>
>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>> of different CEs need to be bound into one wire and connect with
>> NAND_RB0 if want to use controller polling rb. the current operating
>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>
>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>
>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.
> 
> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> 
>>
>>> Arseniy, if the answer to my above question is no, then you should
>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>> then you can return -EINVAL.
>>>
>>> If the nand-rb property is missing, then fallback to software wait.
>>>
>>>> 2) I can't test RB mode - I don't have such device :(
>>>>
>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>> in controller specific register for waiting (I guess Meson controller has something
>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>> 'nand-rb' property, but never use it.
>>>
>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>> slightly broken or at least badly documented, and thus should not be
>>> used.
>>>
>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>> dt-binding maintainers.
>>>>
>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>>
>>> Yes. In a dedicated patch. Something along the lines:
>>>
>>>          nand-rb: true
>>>
>>> inside the nand chip object should be fine. And flag the change as a
>>> fix because we should have used and parsed this property since the
>>> beginning.

Miquel,

Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
meson controller structure?

Thanks, Arseniy

>>>
>>> Thanks,
>>> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 14:49:19 +0300:

> On 05.06.2023 16:30, Liang Yang wrote:
> > 
> > 
> > On 2023/6/5 21:19, Liang Yang wrote:  
> >> Hi Miquel and Arseniy,
> >>
> >>
> >> On 2023/6/5 17:05, Miquel Raynal wrote:  
> >>> [ EXTERNAL EMAIL ]
> >>>
> >>> Hi Arseniy,
> >>>  
> >>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
> >>>>>>             return ret;
> >>>>>>     }
> >>>>>>
> >>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");  
> >>>>>
> >>>>> This is a problem. You cannot add a polling property like that.
> >>>>>
> >>>>> There is already a nand-rb property which is supposed to carry how are
> >>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
> >>>>> don't know how acceptable it is to consider using soft fallback when
> >>>>> this property is missing, otherwise take the values of the rb lines
> >>>>> provided in the DT and user hardware control, but I would definitely
> >>>>> prefer that.  
> >>>>
> >>>> I see. So i need to implement processing of this property here? And if it
> >>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>
> >>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>     RB values to this controller - I just have define for RB command and that's
> >>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>     sure that I'll find something in it).  
> >>>
> >>> Liang, can you please give use the relevant information here? How do we
> >>> target RB0 and RB1? It seems like you use the CS as only information
> >>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>> the lines with a specific configuration?  
> >>
> >> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >> of different CEs need to be bound into one wire and connect with
> >> NAND_RB0 if want to use controller polling rb. the current operating
> >> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>
> >> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>
> >> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
> > 
> > Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >   
> >>  
> >>> Arseniy, if the answer to my above question is no, then you should
> >>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>> then you can return -EINVAL.
> >>>
> >>> If the nand-rb property is missing, then fallback to software wait.
> >>>  
> >>>> 2) I can't test RB mode - I don't have such device :(
> >>>>
> >>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>> in controller specific register for waiting (I guess Meson controller has something
> >>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>> 'nand-rb' property, but never use it.  
> >>>
> >>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>> slightly broken or at least badly documented, and thus should not be
> >>> used.
> >>>  
> >>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>> dt-binding maintainers.  
> >>>>
> >>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
> >>>
> >>> Yes. In a dedicated patch. Something along the lines:
> >>>
> >>>          nand-rb: true
> >>>
> >>> inside the nand chip object should be fine. And flag the change as a
> >>> fix because we should have used and parsed this property since the
> >>> beginning.  
> 
> Miquel,
> 
> Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
> that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
> meson controller structure?

You only need a boolean in the controller structure, I guess.

> 
> Thanks, Arseniy
> 
> >>>
> >>> Thanks,
> >>> Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Arseniy Krasnov]

On 06.06.2023 15:11, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 14:49:19 +0300:
> 
>> On 05.06.2023 16:30, Liang Yang wrote:
>>>
>>>
>>> On 2023/6/5 21:19, Liang Yang wrote:  
>>>> Hi Miquel and Arseniy,
>>>>
>>>>
>>>> On 2023/6/5 17:05, Miquel Raynal wrote:  
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Arseniy,
>>>>>  
>>>>>>>> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>>>>>>>>             return ret;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +  nfc->use_polling = of_property_read_bool(dev->of_node, "polling");  
>>>>>>>
>>>>>>> This is a problem. You cannot add a polling property like that.
>>>>>>>
>>>>>>> There is already a nand-rb property which is supposed to carry how are
>>>>>>> wired the RB lines. I don't see any in-tree users of the compatibles, I
>>>>>>> don't know how acceptable it is to consider using soft fallback when
>>>>>>> this property is missing, otherwise take the values of the rb lines
>>>>>>> provided in the DT and user hardware control, but I would definitely
>>>>>>> prefer that.  
>>>>>>
>>>>>> I see. So i need to implement processing of this property here? And if it
>>>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>>>
>>>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>>>     RB values to this controller - I just have define for RB command and that's
>>>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>>>     sure that I'll find something in it).  
>>>>>
>>>>> Liang, can you please give use the relevant information here? How do we
>>>>> target RB0 and RB1? It seems like you use the CS as only information
>>>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>>>> the lines with a specific configuration?  
>>>>
>>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>>>> of different CEs need to be bound into one wire and connect with
>>>> NAND_RB0 if want to use controller polling rb. the current operating
>>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>>>
>>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>>>
>>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
>>>
>>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
>>>   
>>>>  
>>>>> Arseniy, if the answer to my above question is no, then you should
>>>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>>>> then you can return -EINVAL.
>>>>>
>>>>> If the nand-rb property is missing, then fallback to software wait.
>>>>>  
>>>>>> 2) I can't test RB mode - I don't have such device :(
>>>>>>
>>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>>>> in controller specific register for waiting (I guess Meson controller has something
>>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>>>> 'nand-rb' property, but never use it.  
>>>>>
>>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>>>> slightly broken or at least badly documented, and thus should not be
>>>>> used.
>>>>>  
>>>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>>>> dt-binding maintainers.  
>>>>>>
>>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
>>>>>
>>>>> Yes. In a dedicated patch. Something along the lines:
>>>>>
>>>>>          nand-rb: true
>>>>>
>>>>> inside the nand chip object should be fine. And flag the change as a
>>>>> fix because we should have used and parsed this property since the
>>>>> beginning.  
>>
>> Miquel,
>>
>> Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
>> that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
>> meson controller structure?
> 
> You only need a boolean in the controller structure, I guess.

Got it!

Thanks, Arseniy

> 
>>
>> Thanks, Arseniy
>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Arseniy Krasnov]

This moves free bytes of OOB to non-protected ECC area. It is needed to
make JFFS2 works correctly with this NAND controller. Problem fires when
JFFS2 driver writes cleanmarker to some page and later it tries to write
to this page - write will be done successfully, but after that such page
becomes unreadable due to invalid ECC codes. This happens because second
write needs to update ECC codes, but it is impossible to do it correctly
without block erase. So idea of this patch is to use the unprotected OOB
area to store the cleanmarkers, so that they can be written by the
filesystem without caring much about the page being empty or not: the
ECC codes will not be written anyway.

JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
are usually true SLC flashes, with the capability of writing a page with
empty (0xFF) data, and still be able to write actual data to it later in
a second write.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 Changelog v4->v5:
 * Drop cosmetic changes from this patch.
 * Do not ignore ECC protected user bytes provided by hw. Even these
   bytes are out of user area of OOB, its values are still read from
   the provided OOB buffer and written by hardware. Same behaviour is
   preserved for read access - such bytes are read from DMA buffer and
   placed to OOB buffer.
 * OOB read and write become more lightweight because I removed heavy
   READ0 and PAGEPROG command from it (both commands are still sent
   when OOB access is performed using OOB callbacks). In case of page
   read/write OOB data is handled in the internal SRAM of the controller.
 * Commit message updated.
 * Temporary buffer for OOB read/write is removed. Seems everything
   works correctly without it.

 drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
 1 file changed, 117 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 82a629025adc..e42c28be02f3 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
 static void meson_nfc_get_data_oob(struct nand_chip *nand,
 				   u8 *buf, u8 *oobbuf)
 {
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
 	int i, oob_len = 0;
 	u8 *dsrc, *osrc;
+	u8 *oobtail;
 
 	oob_len = nand->ecc.bytes + 2;
 	for (i = 0; i < nand->ecc.steps; i++) {
@@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
 			memcpy(buf, dsrc, nand->ecc.size);
 			buf += nand->ecc.size;
 		}
+
 		osrc = meson_nfc_oob_ptr(nand, i);
 		memcpy(oobbuf, osrc, oob_len);
 		oobbuf += oob_len;
 	}
+
+	oobtail = meson_chip->data_buf + nand->ecc.steps *
+		  (nand->ecc.size + oob_len);
+
+	/* 'oobbuf' points to the start of user area. */
+	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
 }
 
 static void meson_nfc_set_data_oob(struct nand_chip *nand,
 				   const u8 *buf, u8 *oobbuf)
 {
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct mtd_info *mtd = nand_to_mtd(nand);
 	int i, oob_len = 0;
 	u8 *dsrc, *osrc;
+	u8 *oobtail;
 
 	oob_len = nand->ecc.bytes + 2;
 	for (i = 0; i < nand->ecc.steps; i++) {
@@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
 		memcpy(osrc, oobbuf, oob_len);
 		oobbuf += oob_len;
 	}
+
+	oobtail = meson_chip->data_buf + nand->ecc.steps *
+		  (nand->ecc.size + oob_len);
+
+	/* 'oobbuf' points to the start of user area. */
+	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);
 }
 
 static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
@@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
 	__le64 *info;
 	int i, count;
 
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
+	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
 		info = &meson_chip->info_buf[i];
 		*info |= oob_buf[count];
 		*info |= oob_buf[count + 1] << 8;
@@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
 	__le64 *info;
 	int i, count;
 
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
+	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
 		info = &meson_chip->info_buf[i];
 		oob_buf[count] = *info;
 		oob_buf[count + 1] = *info >> 8;
@@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
 	return 0;
 }
 
+static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+
+	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);
+}
+
+static int meson_nfc_write_oob(struct nand_chip *nand, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	u32 page_size = mtd->writesize + mtd->oobsize;
+	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
+	u8 *oob_buf;
+	int ret;
+
+	if (!oob_bytes)
+		return 0;
+
+	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
+	if (page != -1) {
+		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
+		if (ret)
+			return ret;
+	}
+
+	oob_buf = nand->oob_poi;
+
+	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
+					  oob_buf + (mtd->oobsize - oob_bytes),
+					  oob_bytes, false);
+	if (ret)
+		return ret;
+
+	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
+}
+
+static int meson_nfc_read_oob(struct nand_chip *nand, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	u8 *oob_buf = nand->oob_poi;
+	u32 oob_bytes;
+	u32 page_size;
+	int ret;
+	int i;
+
+	/* Called as OOB read helper, send NAND_CMD_READ0. */
+	if (page != -1) {
+		ret = nand_read_page_op(nand, page, 0, NULL, 0);
+		if (ret)
+			return ret;
+	}
+
+	/* Read ECC codes and user bytes. */
+	for (i = 0; i < nand->ecc.steps; i++) {
+		u32 ecc_offs = nand->ecc.size * (i + 1) +
+			       (nand->ecc.bytes + 2) * i;
+
+		ret = nand_change_read_column_op(nand, ecc_offs,
+						 oob_buf + i * (nand->ecc.bytes + 2),
+						 (nand->ecc.bytes + 2), false);
+		if (ret)
+			return ret;
+	}
+
+	oob_bytes = meson_nfc_oob_free_bytes(nand);
+
+	if (!oob_bytes)
+		return 0;
+
+	page_size = mtd->writesize + mtd->oobsize;
+
+	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
+					 oob_buf + (mtd->oobsize - oob_bytes),
+					 oob_bytes, false);
+
+	return ret;
+}
+
 static int meson_nfc_write_page_sub(struct nand_chip *nand,
 				    int page, int raw)
 {
@@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
 				     NFC_CMD_SCRAMBLER_DISABLE);
 	}
 
+	if (!raw) {
+		ret = meson_nfc_write_oob(nand, -1);
+		if (ret)
+			return ret;
+	}
+
 	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
@@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
 		memcpy(buf, meson_chip->data_buf, mtd->writesize);
 	}
 
-	return bitflips;
-}
-
-static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
-{
-	return meson_nfc_read_page_raw(nand, NULL, 1, page);
-}
+	if (oob_required && ret)
+		meson_nfc_read_oob(nand, -1);
 
-static int meson_nfc_read_oob(struct nand_chip *nand, int page)
-{
-	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
+	return bitflips;
 }
 
 static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
@@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
 				struct mtd_oob_region *oobregion)
 {
 	struct nand_chip *nand = mtd_to_nand(mtd);
+	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
 
 	if (section >= nand->ecc.steps)
 		return -ERANGE;
 
-	oobregion->offset = section * (2 + nand->ecc.bytes);
-	oobregion->length = 2;
+	/* Split rest of OOB area (not covered by ECC engine) per each
+	 * ECC section. This will be OOB data available to user.
+	 */
+	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);
+	oobregion->length = oob_bytes / nand->ecc.steps;
 
 	return 0;
 }
@@ -1220,12 +1320,12 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 	nand->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
 	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
 	nand->ecc.write_page = meson_nfc_write_page_hwecc;
-	nand->ecc.write_oob_raw = nand_write_oob_std;
-	nand->ecc.write_oob = nand_write_oob_std;
 
+	nand->ecc.write_oob_raw = meson_nfc_write_oob;
+	nand->ecc.write_oob = meson_nfc_write_oob;
 	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
 	nand->ecc.read_page = meson_nfc_read_page_hwecc;
-	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
+	nand->ecc.read_oob_raw = meson_nfc_read_oob;
 	nand->ecc.read_oob = meson_nfc_read_oob;
 
 	if (nand->options & NAND_BUSWIDTH_16) {
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:

> This moves free bytes of OOB to non-protected ECC area. It is needed to

As we discussed, I expect this commit to just change the OOB layout to
expose unprotected OOB bytes to the user, that is the only change this
commit should carry. If that does not work, you should add a
preparation patch.

> make JFFS2 works correctly with this NAND controller. Problem fires when
> JFFS2 driver writes cleanmarker to some page and later it tries to write
> to this page - write will be done successfully, but after that such page
> becomes unreadable due to invalid ECC codes. This happens because second
> write needs to update ECC codes, but it is impossible to do it correctly
> without block erase. So idea of this patch is to use the unprotected OOB
> area to store the cleanmarkers, so that they can be written by the
> filesystem without caring much about the page being empty or not: the
> ECC codes will not be written anyway.
> 
> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
> are usually true SLC flashes, with the capability of writing a page with
> empty (0xFF) data, and still be able to write actual data to it later in
> a second write.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  Changelog v4->v5:
>  * Drop cosmetic changes from this patch.
>  * Do not ignore ECC protected user bytes provided by hw. Even these
>    bytes are out of user area of OOB, its values are still read from
>    the provided OOB buffer and written by hardware. Same behaviour is
>    preserved for read access - such bytes are read from DMA buffer and
>    placed to OOB buffer.
>  * OOB read and write become more lightweight because I removed heavy
>    READ0 and PAGEPROG command from it (both commands are still sent
>    when OOB access is performed using OOB callbacks). In case of page
>    read/write OOB data is handled in the internal SRAM of the controller.
>  * Commit message updated.
>  * Temporary buffer for OOB read/write is removed. Seems everything
>    works correctly without it.
> 
>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
>  1 file changed, 117 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 82a629025adc..e42c28be02f3 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
>  				   u8 *buf, u8 *oobbuf)
>  {
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct mtd_info *mtd = nand_to_mtd(nand);
>  	int i, oob_len = 0;
>  	u8 *dsrc, *osrc;
> +	u8 *oobtail;
>  
>  	oob_len = nand->ecc.bytes + 2;
>  	for (i = 0; i < nand->ecc.steps; i++) {
> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>  			memcpy(buf, dsrc, nand->ecc.size);
>  			buf += nand->ecc.size;
>  		}
> +
>  		osrc = meson_nfc_oob_ptr(nand, i);
>  		memcpy(oobbuf, osrc, oob_len);
>  		oobbuf += oob_len;
>  	}
> +
> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> +		  (nand->ecc.size + oob_len);
> +
> +	/* 'oobbuf' points to the start of user area. */
> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
>  }
>  
>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  				   const u8 *buf, u8 *oobbuf)
>  {
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct mtd_info *mtd = nand_to_mtd(nand);
>  	int i, oob_len = 0;
>  	u8 *dsrc, *osrc;
> +	u8 *oobtail;
>  
>  	oob_len = nand->ecc.bytes + 2;
>  	for (i = 0; i < nand->ecc.steps; i++) {
> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  		memcpy(osrc, oobbuf, oob_len);
>  		oobbuf += oob_len;
>  	}
> +
> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> +		  (nand->ecc.size + oob_len);

This is always targeting the same area, so it looks strange to me.

> +
> +	/* 'oobbuf' points to the start of user area. */
> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);

TBH I don't get what you do here.

>  }
>  
>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>  	__le64 *info;
>  	int i, count;
>  
> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>  		info = &meson_chip->info_buf[i];
>  		*info |= oob_buf[count];
>  		*info |= oob_buf[count + 1] << 8;
> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>  	__le64 *info;
>  	int i, count;
>  
> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>  		info = &meson_chip->info_buf[i];
>  		oob_buf[count] = *info;
>  		oob_buf[count + 1] = *info >> 8;
> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>  	return 0;
>  }
>  
> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +
> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);

This looks like a static value, just save it somewhere instead of
recomputing it?

> +}
> +
> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	u32 page_size = mtd->writesize + mtd->oobsize;
> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> +	u8 *oob_buf;
> +	int ret;
> +
> +	if (!oob_bytes)
> +		return 0;

Can this happen?

> +
> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
> +	if (page != -1) {
> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	oob_buf = nand->oob_poi;
> +
> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
> +					  oob_buf + (mtd->oobsize - oob_bytes),
> +					  oob_bytes, false);
> +	if (ret)
> +		return ret;
> +
> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
> +}
> +
> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	u8 *oob_buf = nand->oob_poi;
> +	u32 oob_bytes;
> +	u32 page_size;
> +	int ret;
> +	int i;
> +
> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
> +	if (page != -1) {

I don't like this logic with the "-1", it really hides what the
controller needs to do, if you need a helper to send a command, then
create that helper and give it a decent name.

> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Read ECC codes and user bytes. */
> +	for (i = 0; i < nand->ecc.steps; i++) {
> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
> +			       (nand->ecc.bytes + 2) * i;
> +
> +		ret = nand_change_read_column_op(nand, ecc_offs,
> +						 oob_buf + i * (nand->ecc.bytes + 2),
> +						 (nand->ecc.bytes + 2), false);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
> +
> +	if (!oob_bytes)
> +		return 0;
> +
> +	page_size = mtd->writesize + mtd->oobsize;
> +
> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
> +					 oob_buf + (mtd->oobsize - oob_bytes),
> +					 oob_bytes, false);
> +
> +	return ret;
> +}
> +
>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
>  				    int page, int raw)
>  {
> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>  				     NFC_CMD_SCRAMBLER_DISABLE);
>  	}
>  
> +	if (!raw) {

Why this check?

You should instead propagate the oob_required field and check that
value I believe.

> +		ret = meson_nfc_write_oob(nand, -1);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
>  	}
>  
> -	return bitflips;
> -}
> -
> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
> -{
> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
> -}
> +	if (oob_required && ret)

Unclear why you check ret here?

> +		meson_nfc_read_oob(nand, -1);
>  
> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> -{
> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
> +	return bitflips;
>  }
>  
>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>  				struct mtd_oob_region *oobregion)
>  {
>  	struct nand_chip *nand = mtd_to_nand(mtd);
> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>  
>  	if (section >= nand->ecc.steps)
>  		return -ERANGE;
>  
> -	oobregion->offset = section * (2 + nand->ecc.bytes);

The first two bytes of OOB are reserved for the bad block markers. This
is not related to your controller.

> -	oobregion->length = 2;
> +	/* Split rest of OOB area (not covered by ECC engine) per each
> +	 * ECC section. This will be OOB data available to user.
> +	 */
> +	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);

This is not possible, see above.

> +	oobregion->length = oob_bytes / nand->ecc.steps;
>  
>  	return 0;
>  }
> @@ -1220,12 +1320,12 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>  	nand->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
>  	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>  	nand->ecc.write_page = meson_nfc_write_page_hwecc;
> -	nand->ecc.write_oob_raw = nand_write_oob_std;
> -	nand->ecc.write_oob = nand_write_oob_std;
>  
> +	nand->ecc.write_oob_raw = meson_nfc_write_oob;
> +	nand->ecc.write_oob = meson_nfc_write_oob;
>  	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>  	nand->ecc.read_page = meson_nfc_read_page_hwecc;
> -	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
> +	nand->ecc.read_oob_raw = meson_nfc_read_oob;
>  	nand->ecc.read_oob = meson_nfc_read_oob;
>  
>  	if (nand->options & NAND_BUSWIDTH_16) {


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Arseniy Krasnov]

Hello Miquel, thanks for review!

On 01.06.2023 11:31, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
> 
>> This moves free bytes of OOB to non-protected ECC area. It is needed to
> 
> As we discussed, I expect this commit to just change the OOB layout to
> expose unprotected OOB bytes to the user, that is the only change this
> commit should carry. If that does not work, you should add a
> preparation patch.

Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
mtd_ooblayout_ops, I also need to implement code which performs read/write
according new layout (it must be done in a single patch)?

Main thing is:

I guess that general confuse with this patch is that You consider
that we change only OOB layout by moving user bytes out of ECC area, but at the same
time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
to 32 bytes (e.g. tail of page after data and ECC codes), so if this
assumption is correct, in the next version I won't change size of user area in
OOB, thus this patch will be reduced as some comments from this review.

> 
>> make JFFS2 works correctly with this NAND controller. Problem fires when
>> JFFS2 driver writes cleanmarker to some page and later it tries to write
>> to this page - write will be done successfully, but after that such page
>> becomes unreadable due to invalid ECC codes. This happens because second
>> write needs to update ECC codes, but it is impossible to do it correctly
>> without block erase. So idea of this patch is to use the unprotected OOB
>> area to store the cleanmarkers, so that they can be written by the
>> filesystem without caring much about the page being empty or not: the
>> ECC codes will not be written anyway.
>>
>> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
>> are usually true SLC flashes, with the capability of writing a page with
>> empty (0xFF) data, and still be able to write actual data to it later in
>> a second write.
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  Changelog v4->v5:
>>  * Drop cosmetic changes from this patch.
>>  * Do not ignore ECC protected user bytes provided by hw. Even these
>>    bytes are out of user area of OOB, its values are still read from
>>    the provided OOB buffer and written by hardware. Same behaviour is
>>    preserved for read access - such bytes are read from DMA buffer and
>>    placed to OOB buffer.
>>  * OOB read and write become more lightweight because I removed heavy
>>    READ0 and PAGEPROG command from it (both commands are still sent
>>    when OOB access is performed using OOB callbacks). In case of page
>>    read/write OOB data is handled in the internal SRAM of the controller.
>>  * Commit message updated.
>>  * Temporary buffer for OOB read/write is removed. Seems everything
>>    works correctly without it.
>>
>>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
>>  1 file changed, 117 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index 82a629025adc..e42c28be02f3 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>  				   u8 *buf, u8 *oobbuf)
>>  {
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>  	int i, oob_len = 0;
>>  	u8 *dsrc, *osrc;
>> +	u8 *oobtail;
>>  
>>  	oob_len = nand->ecc.bytes + 2;
>>  	for (i = 0; i < nand->ecc.steps; i++) {
>> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>  			memcpy(buf, dsrc, nand->ecc.size);
>>  			buf += nand->ecc.size;
>>  		}
>> +
>>  		osrc = meson_nfc_oob_ptr(nand, i);
>>  		memcpy(oobbuf, osrc, oob_len);
>>  		oobbuf += oob_len;
>>  	}
>> +
>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>> +		  (nand->ecc.size + oob_len);
>> +
>> +	/* 'oobbuf' points to the start of user area. */
>> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
>>  }
>>  
>>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>  				   const u8 *buf, u8 *oobbuf)
>>  {
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>  	int i, oob_len = 0;
>>  	u8 *dsrc, *osrc;
>> +	u8 *oobtail;
>>  
>>  	oob_len = nand->ecc.bytes + 2;
>>  	for (i = 0; i < nand->ecc.steps; i++) {
>> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>  		memcpy(osrc, oobbuf, oob_len);
>>  		oobbuf += oob_len;
>>  	}
>> +
>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>> +		  (nand->ecc.size + oob_len);
> 
> This is always targeting the same area, so it looks strange to me.
> 
>> +
>> +	/* 'oobbuf' points to the start of user area. */
>> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);
> 
> TBH I don't get what you do here.

This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
This is because number of user bytes is increased in this patch.

> 
>>  }
>>  
>>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
>> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>  	__le64 *info;
>>  	int i, count;
>>  
>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>  		info = &meson_chip->info_buf[i];
>>  		*info |= oob_buf[count];
>>  		*info |= oob_buf[count + 1] << 8;
>> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>  	__le64 *info;
>>  	int i, count;
>>  
>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>  		info = &meson_chip->info_buf[i];
>>  		oob_buf[count] = *info;
>>  		oob_buf[count + 1] = *info >> 8;
>> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>  	return 0;
>>  }
>>  
>> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +
>> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);
> 
> This looks like a static value, just save it somewhere instead of
> recomputing it?

Ack

> 
>> +}
>> +
>> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +	u32 page_size = mtd->writesize + mtd->oobsize;
>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>> +	u8 *oob_buf;
>> +	int ret;
>> +
>> +	if (!oob_bytes)
>> +		return 0;
> 
> Can this happen?

Ack, seems forget to remove it

> 
>> +
>> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
>> +	if (page != -1) {
>> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	oob_buf = nand->oob_poi;
>> +
>> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
>> +					  oob_buf + (mtd->oobsize - oob_bytes),
>> +					  oob_bytes, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
>> +}
>> +
>> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>> +	u8 *oob_buf = nand->oob_poi;
>> +	u32 oob_bytes;
>> +	u32 page_size;
>> +	int ret;
>> +	int i;
>> +
>> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
>> +	if (page != -1) {
> 
> I don't like this logic with the "-1", it really hides what the
> controller needs to do, if you need a helper to send a command, then
> create that helper and give it a decent name.

I see, so I think I need to implement this in the following way:
1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')

> 
>> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/* Read ECC codes and user bytes. */
>> +	for (i = 0; i < nand->ecc.steps; i++) {
>> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
>> +			       (nand->ecc.bytes + 2) * i;
>> +
>> +		ret = nand_change_read_column_op(nand, ecc_offs,
>> +						 oob_buf + i * (nand->ecc.bytes + 2),
>> +						 (nand->ecc.bytes + 2), false);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
>> +
>> +	if (!oob_bytes)
>> +		return 0;
>> +
>> +	page_size = mtd->writesize + mtd->oobsize;
>> +
>> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
>> +					 oob_buf + (mtd->oobsize - oob_bytes),
>> +					 oob_bytes, false);
>> +
>> +	return ret;
>> +}
>> +
>>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>  				    int page, int raw)
>>  {
>> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>  				     NFC_CMD_SCRAMBLER_DISABLE);
>>  	}
>>  
>> +	if (!raw) {
> 
> Why this check?
> 
> You should instead propagate the oob_required field and check that
> value I believe.


This check is for ECC mode, because in this mode we write user bytes of OOB.
ECC bytes of OOB are written by hardware. I think I made a mistake, because
I need new callback to write OOB in raw mode - it will write both ECC and user
parts, in current version I write only user part in raw mode.

> 
>> +		ret = meson_nfc_write_oob(nand, -1);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
>>  	}
>>  
>> -	return bitflips;
>> -}
>> -
>> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
>> -{
>> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
>> -}
>> +	if (oob_required && ret)
> 
> Unclear why you check ret here?
> 

If read was successful, we read OOB. If not - there is no sense in it.

>> +		meson_nfc_read_oob(nand, -1);
>>  
>> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>> -{
>> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
>> +	return bitflips;
>>  }
>>  
>>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
>> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>>  				struct mtd_oob_region *oobregion)
>>  {
>>  	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>  
>>  	if (section >= nand->ecc.steps)
>>  		return -ERANGE;
>>  
>> -	oobregion->offset = section * (2 + nand->ecc.bytes);
> 
> The first two bytes of OOB are reserved for the bad block markers. This
> is not related to your controller.

I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
is considered by hardware as user bytes covered by ECC.

Thanks, Arseniy

> 
>> -	oobregion->length = 2;
>> +	/* Split rest of OOB area (not covered by ECC engine) per each
>> +	 * ECC section. This will be OOB data available to user.
>> +	 */
>> +	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);
> 
> This is not possible, see above.
> 
>> +	oobregion->length = oob_bytes / nand->ecc.steps;
>>  
>>  	return 0;
>>  }
>> @@ -1220,12 +1320,12 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>>  	nand->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
>>  	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>>  	nand->ecc.write_page = meson_nfc_write_page_hwecc;
>> -	nand->ecc.write_oob_raw = nand_write_oob_std;
>> -	nand->ecc.write_oob = nand_write_oob_std;
>>  
>> +	nand->ecc.write_oob_raw = meson_nfc_write_oob;
>> +	nand->ecc.write_oob = meson_nfc_write_oob;
>>  	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>>  	nand->ecc.read_page = meson_nfc_read_page_hwecc;
>> -	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
>> +	nand->ecc.read_oob_raw = meson_nfc_read_oob;
>>  	nand->ecc.read_oob = meson_nfc_read_oob;
>>  
>>  	if (nand->options & NAND_BUSWIDTH_16) {
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:

> Hello Miquel, thanks for review!
> 
> On 01.06.2023 11:31, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
> >   
> >> This moves free bytes of OOB to non-protected ECC area. It is needed to  
> > 
> > As we discussed, I expect this commit to just change the OOB layout to
> > expose unprotected OOB bytes to the user, that is the only change this
> > commit should carry. If that does not work, you should add a
> > preparation patch.  
> 
> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
> mtd_ooblayout_ops, I also need to implement code which performs read/write
> according new layout (it must be done in a single patch)?

No, this is orthogonal.

The driver must read the the whole OOB area (and perhaps reorder the
data), but you should not make any decision regarding what bytes you
want or not want to expose.

Then, the user (no matter what "user" is here) will decide how to deal
with the data.

> Main thing is:
> 
> I guess that general confuse with this patch is that You consider
> that we change only OOB layout by moving user bytes out of ECC area, but at the same
> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
> assumption is correct, in the next version I won't change size of user area in
> OOB, thus this patch will be reduced as some comments from this review.

Exposing only 4 bytes was a mistake in the first place, please fix this
in a dedicated patch.

> >> make JFFS2 works correctly with this NAND controller. Problem fires when
> >> JFFS2 driver writes cleanmarker to some page and later it tries to write
> >> to this page - write will be done successfully, but after that such page
> >> becomes unreadable due to invalid ECC codes. This happens because second
> >> write needs to update ECC codes, but it is impossible to do it correctly
> >> without block erase. So idea of this patch is to use the unprotected OOB
> >> area to store the cleanmarkers, so that they can be written by the
> >> filesystem without caring much about the page being empty or not: the
> >> ECC codes will not be written anyway.
> >>
> >> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
> >> are usually true SLC flashes, with the capability of writing a page with
> >> empty (0xFF) data, and still be able to write actual data to it later in
> >> a second write.
> >>
> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >> ---
> >>  Changelog v4->v5:
> >>  * Drop cosmetic changes from this patch.
> >>  * Do not ignore ECC protected user bytes provided by hw. Even these
> >>    bytes are out of user area of OOB, its values are still read from
> >>    the provided OOB buffer and written by hardware. Same behaviour is
> >>    preserved for read access - such bytes are read from DMA buffer and
> >>    placed to OOB buffer.
> >>  * OOB read and write become more lightweight because I removed heavy
> >>    READ0 and PAGEPROG command from it (both commands are still sent
> >>    when OOB access is performed using OOB callbacks). In case of page
> >>    read/write OOB data is handled in the internal SRAM of the controller.
> >>  * Commit message updated.
> >>  * Temporary buffer for OOB read/write is removed. Seems everything
> >>    works correctly without it.
> >>
> >>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
> >>  1 file changed, 117 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >> index 82a629025adc..e42c28be02f3 100644
> >> --- a/drivers/mtd/nand/raw/meson_nand.c
> >> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
> >>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>  				   u8 *buf, u8 *oobbuf)
> >>  {
> >> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>  	int i, oob_len = 0;
> >>  	u8 *dsrc, *osrc;
> >> +	u8 *oobtail;
> >>  
> >>  	oob_len = nand->ecc.bytes + 2;
> >>  	for (i = 0; i < nand->ecc.steps; i++) {
> >> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>  			memcpy(buf, dsrc, nand->ecc.size);
> >>  			buf += nand->ecc.size;
> >>  		}
> >> +
> >>  		osrc = meson_nfc_oob_ptr(nand, i);
> >>  		memcpy(oobbuf, osrc, oob_len);
> >>  		oobbuf += oob_len;
> >>  	}
> >> +
> >> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >> +		  (nand->ecc.size + oob_len);
> >> +
> >> +	/* 'oobbuf' points to the start of user area. */
> >> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
> >>  }
> >>  
> >>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>  				   const u8 *buf, u8 *oobbuf)
> >>  {
> >> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>  	int i, oob_len = 0;
> >>  	u8 *dsrc, *osrc;
> >> +	u8 *oobtail;
> >>  
> >>  	oob_len = nand->ecc.bytes + 2;
> >>  	for (i = 0; i < nand->ecc.steps; i++) {
> >> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>  		memcpy(osrc, oobbuf, oob_len);
> >>  		oobbuf += oob_len;
> >>  	}
> >> +
> >> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >> +		  (nand->ecc.size + oob_len);  
> > 
> > This is always targeting the same area, so it looks strange to me.
> >   
> >> +
> >> +	/* 'oobbuf' points to the start of user area. */
> >> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);  
> > 
> > TBH I don't get what you do here.  
> 
> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
> This is because number of user bytes is increased in this patch.
> 
> >   
> >>  }
> >>  
> >>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
> >> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>  	__le64 *info;
> >>  	int i, count;
> >>  
> >> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>  		info = &meson_chip->info_buf[i];
> >>  		*info |= oob_buf[count];
> >>  		*info |= oob_buf[count + 1] << 8;
> >> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>  	__le64 *info;
> >>  	int i, count;
> >>  
> >> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>  		info = &meson_chip->info_buf[i];
> >>  		oob_buf[count] = *info;
> >>  		oob_buf[count + 1] = *info >> 8;
> >> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >>  	return 0;
> >>  }
> >>  
> >> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +
> >> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);  
> > 
> > This looks like a static value, just save it somewhere instead of
> > recomputing it?  
> 
> Ack
> 
> >   
> >> +}
> >> +
> >> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +	u32 page_size = mtd->writesize + mtd->oobsize;
> >> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >> +	u8 *oob_buf;
> >> +	int ret;
> >> +
> >> +	if (!oob_bytes)
> >> +		return 0;  
> > 
> > Can this happen?  
> 
> Ack, seems forget to remove it
> 
> >   
> >> +
> >> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
> >> +	if (page != -1) {
> >> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	oob_buf = nand->oob_poi;
> >> +
> >> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
> >> +					  oob_buf + (mtd->oobsize - oob_bytes),
> >> +					  oob_bytes, false);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
> >> +}
> >> +
> >> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +	u8 *oob_buf = nand->oob_poi;
> >> +	u32 oob_bytes;
> >> +	u32 page_size;
> >> +	int ret;
> >> +	int i;
> >> +
> >> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
> >> +	if (page != -1) {  
> > 
> > I don't like this logic with the "-1", it really hides what the
> > controller needs to do, if you need a helper to send a command, then
> > create that helper and give it a decent name.  
> 
> I see, so I think I need to implement this in the following way:
> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
> 
> >   
> >> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	/* Read ECC codes and user bytes. */
> >> +	for (i = 0; i < nand->ecc.steps; i++) {
> >> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
> >> +			       (nand->ecc.bytes + 2) * i;
> >> +
> >> +		ret = nand_change_read_column_op(nand, ecc_offs,
> >> +						 oob_buf + i * (nand->ecc.bytes + 2),
> >> +						 (nand->ecc.bytes + 2), false);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
> >> +
> >> +	if (!oob_bytes)
> >> +		return 0;
> >> +
> >> +	page_size = mtd->writesize + mtd->oobsize;
> >> +
> >> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
> >> +					 oob_buf + (mtd->oobsize - oob_bytes),
> >> +					 oob_bytes, false);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>  				    int page, int raw)
> >>  {
> >> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>  				     NFC_CMD_SCRAMBLER_DISABLE);
> >>  	}
> >>  
> >> +	if (!raw) {  
> > 
> > Why this check?
> > 
> > You should instead propagate the oob_required field and check that
> > value I believe.  
> 
> 
> This check is for ECC mode, because in this mode we write user bytes of OOB.
> ECC bytes of OOB are written by hardware.

Just provide the buffer. The ECC engine will smash data if there was
any there. Otherwise it will fill the holes. It's expected. Don't try
to be smarter than you should :)

> I think I made a mistake, because
> I need new callback to write OOB in raw mode - it will write both ECC and user
> parts,

There is no such thing as user and ECC part at the driver level. You
get a buffer, you need to write it to the flash.

The user expects:

| data | OOB |

The controller expects something like:

| data1 | OOB1 | data2 | OOB2 |

So just perform the reordering between data and OOB in the DMA buffer,
that is _all_.

> in current version I write only user part in raw mode.
> 
> >   
> >> +		ret = meson_nfc_write_oob(nand, -1);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
> >>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
> >> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
> >>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
> >>  	}
> >>  
> >> -	return bitflips;
> >> -}
> >> -
> >> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
> >> -{
> >> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
> >> -}
> >> +	if (oob_required && ret)  
> > 
> > Unclear why you check ret here?

In general, if (ret) means there is an error.

Please consider using:

if (ret)
	goto error path;

do something else;

> >   
> 
> If read was successful, we read OOB. If not - there is no sense in it.
> 
> >> +		meson_nfc_read_oob(nand, -1);
> >>  
> >> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >> -{
> >> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
> >> +	return bitflips;
> >>  }
> >>  
> >>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
> >> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
> >>  				struct mtd_oob_region *oobregion)
> >>  {
> >>  	struct nand_chip *nand = mtd_to_nand(mtd);
> >> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >>  
> >>  	if (section >= nand->ecc.steps)
> >>  		return -ERANGE;
> >>  
> >> -	oobregion->offset = section * (2 + nand->ecc.bytes);  
> > 
> > The first two bytes of OOB are reserved for the bad block markers. This
> > is not related to your controller.  
> 
> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
> is considered by hardware as user bytes covered by ECC.

The two first bytes should not be available. They are not "ECC" bytes,
they are not "free" bytes. None of these two callbacks should give
access to these two bytes reserved for bad block markers.

Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
"these are the protected bytes". They mean "these are the bytes in OOB
the hardware ECC engine will use to place its own data to make the
recovery process work".

Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Arseniy Krasnov]

On 05.06.2023 12:48, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:
> 
>> Hello Miquel, thanks for review!
>>
>> On 01.06.2023 11:31, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
>>>   
>>>> This moves free bytes of OOB to non-protected ECC area. It is needed to  
>>>
>>> As we discussed, I expect this commit to just change the OOB layout to
>>> expose unprotected OOB bytes to the user, that is the only change this
>>> commit should carry. If that does not work, you should add a
>>> preparation patch.  
>>
>> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
>> mtd_ooblayout_ops, I also need to implement code which performs read/write
>> according new layout (it must be done in a single patch)?
> 
> No, this is orthogonal.
> 
> The driver must read the the whole OOB area (and perhaps reorder the
> data), but you should not make any decision regarding what bytes you
> want or not want to expose.
> 
> Then, the user (no matter what "user" is here) will decide how to deal
> with the data.

Hello Miquel!

Ok, so in case of:
1) read I just need to read OOB data using 'nand_change_read_column_op()' and place it to 'oob_buf'.
2) write I need to write OOB data using 'nand_change_write_column_op()' to controller internal RAM and then call PAGE_PROG.
   Even in ECC mode, data which occupies places of ECC codes will be removed by hw ( as You mentinoed below).

That's all?:)

> 
>> Main thing is:
>>
>> I guess that general confuse with this patch is that You consider
>> that we change only OOB layout by moving user bytes out of ECC area, but at the same
>> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
>> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
>> assumption is correct, in the next version I won't change size of user area in
>> OOB, thus this patch will be reduced as some comments from this review.
> 
> Exposing only 4 bytes was a mistake in the first place, please fix this
> in a dedicated patch.

So current (not merged) version exposes bytes 0,1,16,17 of OOB, You mean this is wrong?
Correct way is to expose 32,33,48,49 (e.g. shifted by 32)?

Thanks, Arseniy

> 
>>>> make JFFS2 works correctly with this NAND controller. Problem fires when
>>>> JFFS2 driver writes cleanmarker to some page and later it tries to write
>>>> to this page - write will be done successfully, but after that such page
>>>> becomes unreadable due to invalid ECC codes. This happens because second
>>>> write needs to update ECC codes, but it is impossible to do it correctly
>>>> without block erase. So idea of this patch is to use the unprotected OOB
>>>> area to store the cleanmarkers, so that they can be written by the
>>>> filesystem without caring much about the page being empty or not: the
>>>> ECC codes will not be written anyway.
>>>>
>>>> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
>>>> are usually true SLC flashes, with the capability of writing a page with
>>>> empty (0xFF) data, and still be able to write actual data to it later in
>>>> a second write.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>>  Changelog v4->v5:
>>>>  * Drop cosmetic changes from this patch.
>>>>  * Do not ignore ECC protected user bytes provided by hw. Even these
>>>>    bytes are out of user area of OOB, its values are still read from
>>>>    the provided OOB buffer and written by hardware. Same behaviour is
>>>>    preserved for read access - such bytes are read from DMA buffer and
>>>>    placed to OOB buffer.
>>>>  * OOB read and write become more lightweight because I removed heavy
>>>>    READ0 and PAGEPROG command from it (both commands are still sent
>>>>    when OOB access is performed using OOB callbacks). In case of page
>>>>    read/write OOB data is handled in the internal SRAM of the controller.
>>>>  * Commit message updated.
>>>>  * Temporary buffer for OOB read/write is removed. Seems everything
>>>>    works correctly without it.
>>>>
>>>>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
>>>>  1 file changed, 117 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>> index 82a629025adc..e42c28be02f3 100644
>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>>>>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>>  				   u8 *buf, u8 *oobbuf)
>>>>  {
>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>  	int i, oob_len = 0;
>>>>  	u8 *dsrc, *osrc;
>>>> +	u8 *oobtail;
>>>>  
>>>>  	oob_len = nand->ecc.bytes + 2;
>>>>  	for (i = 0; i < nand->ecc.steps; i++) {
>>>> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>>  			memcpy(buf, dsrc, nand->ecc.size);
>>>>  			buf += nand->ecc.size;
>>>>  		}
>>>> +
>>>>  		osrc = meson_nfc_oob_ptr(nand, i);
>>>>  		memcpy(oobbuf, osrc, oob_len);
>>>>  		oobbuf += oob_len;
>>>>  	}
>>>> +
>>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>> +		  (nand->ecc.size + oob_len);
>>>> +
>>>> +	/* 'oobbuf' points to the start of user area. */
>>>> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
>>>>  }
>>>>  
>>>>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>>  				   const u8 *buf, u8 *oobbuf)
>>>>  {
>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>  	int i, oob_len = 0;
>>>>  	u8 *dsrc, *osrc;
>>>> +	u8 *oobtail;
>>>>  
>>>>  	oob_len = nand->ecc.bytes + 2;
>>>>  	for (i = 0; i < nand->ecc.steps; i++) {
>>>> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>>  		memcpy(osrc, oobbuf, oob_len);
>>>>  		oobbuf += oob_len;
>>>>  	}
>>>> +
>>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>> +		  (nand->ecc.size + oob_len);  
>>>
>>> This is always targeting the same area, so it looks strange to me.
>>>   
>>>> +
>>>> +	/* 'oobbuf' points to the start of user area. */
>>>> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);  
>>>
>>> TBH I don't get what you do here.  
>>
>> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
>> This is because number of user bytes is increased in this patch.
>>
>>>   
>>>>  }
>>>>  
>>>>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
>>>> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>  	__le64 *info;
>>>>  	int i, count;
>>>>  
>>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>>  		info = &meson_chip->info_buf[i];
>>>>  		*info |= oob_buf[count];
>>>>  		*info |= oob_buf[count + 1] << 8;
>>>> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>  	__le64 *info;
>>>>  	int i, count;
>>>>  
>>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>>  		info = &meson_chip->info_buf[i];
>>>>  		oob_buf[count] = *info;
>>>>  		oob_buf[count + 1] = *info >> 8;
>>>> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>> +
>>>> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);  
>>>
>>> This looks like a static value, just save it somewhere instead of
>>> recomputing it?  
>>
>> Ack
>>
>>>   
>>>> +}
>>>> +
>>>> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>> +	u32 page_size = mtd->writesize + mtd->oobsize;
>>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>> +	u8 *oob_buf;
>>>> +	int ret;
>>>> +
>>>> +	if (!oob_bytes)
>>>> +		return 0;  
>>>
>>> Can this happen?  
>>
>> Ack, seems forget to remove it
>>
>>>   
>>>> +
>>>> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
>>>> +	if (page != -1) {
>>>> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	oob_buf = nand->oob_poi;
>>>> +
>>>> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
>>>> +					  oob_buf + (mtd->oobsize - oob_bytes),
>>>> +					  oob_bytes, false);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
>>>> +}
>>>> +
>>>> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>> +	u8 *oob_buf = nand->oob_poi;
>>>> +	u32 oob_bytes;
>>>> +	u32 page_size;
>>>> +	int ret;
>>>> +	int i;
>>>> +
>>>> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
>>>> +	if (page != -1) {  
>>>
>>> I don't like this logic with the "-1", it really hides what the
>>> controller needs to do, if you need a helper to send a command, then
>>> create that helper and give it a decent name.  
>>
>> I see, so I think I need to implement this in the following way:
>> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
>> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
>>
>>>   
>>>> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	/* Read ECC codes and user bytes. */
>>>> +	for (i = 0; i < nand->ecc.steps; i++) {
>>>> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
>>>> +			       (nand->ecc.bytes + 2) * i;
>>>> +
>>>> +		ret = nand_change_read_column_op(nand, ecc_offs,
>>>> +						 oob_buf + i * (nand->ecc.bytes + 2),
>>>> +						 (nand->ecc.bytes + 2), false);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>> +
>>>> +	if (!oob_bytes)
>>>> +		return 0;
>>>> +
>>>> +	page_size = mtd->writesize + mtd->oobsize;
>>>> +
>>>> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
>>>> +					 oob_buf + (mtd->oobsize - oob_bytes),
>>>> +					 oob_bytes, false);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>>  				    int page, int raw)
>>>>  {
>>>> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>>  				     NFC_CMD_SCRAMBLER_DISABLE);
>>>>  	}
>>>>  
>>>> +	if (!raw) {  
>>>
>>> Why this check?
>>>
>>> You should instead propagate the oob_required field and check that
>>> value I believe.  
>>
>>
>> This check is for ECC mode, because in this mode we write user bytes of OOB.
>> ECC bytes of OOB are written by hardware.
> 
> Just provide the buffer. The ECC engine will smash data if there was
> any there. Otherwise it will fill the holes. It's expected. Don't try
> to be smarter than you should :)
> 
>> I think I made a mistake, because
>> I need new callback to write OOB in raw mode - it will write both ECC and user
>> parts,
> 
> There is no such thing as user and ECC part at the driver level. You
> get a buffer, you need to write it to the flash.
> 
> The user expects:
> 
> | data | OOB |
> 
> The controller expects something like:
> 
> | data1 | OOB1 | data2 | OOB2 |
> 
> So just perform the reordering between data and OOB in the DMA buffer,
> that is _all_.
> 
>> in current version I write only user part in raw mode.
>>
>>>   
>>>> +		ret = meson_nfc_write_oob(nand, -1);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>>>> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>>>>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
>>>>  	}
>>>>  
>>>> -	return bitflips;
>>>> -}
>>>> -
>>>> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
>>>> -{
>>>> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
>>>> -}
>>>> +	if (oob_required && ret)  
>>>
>>> Unclear why you check ret here?
> 
> In general, if (ret) means there is an error.
> 
> Please consider using:
> 
> if (ret)
> 	goto error path;
> 
> do something else;
> 
>>>   
>>
>> If read was successful, we read OOB. If not - there is no sense in it.
>>
>>>> +		meson_nfc_read_oob(nand, -1);
>>>>  
>>>> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>> -{
>>>> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
>>>> +	return bitflips;
>>>>  }
>>>>  
>>>>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
>>>> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>>>>  				struct mtd_oob_region *oobregion)
>>>>  {
>>>>  	struct nand_chip *nand = mtd_to_nand(mtd);
>>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>>  
>>>>  	if (section >= nand->ecc.steps)
>>>>  		return -ERANGE;
>>>>  
>>>> -	oobregion->offset = section * (2 + nand->ecc.bytes);  
>>>
>>> The first two bytes of OOB are reserved for the bad block markers. This
>>> is not related to your controller.  
>>
>> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
>> is considered by hardware as user bytes covered by ECC.
> 
> The two first bytes should not be available. They are not "ECC" bytes,
> they are not "free" bytes. None of these two callbacks should give
> access to these two bytes reserved for bad block markers.
> 
> Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
> "these are the protected bytes". They mean "these are the bytes in OOB
> the hardware ECC engine will use to place its own data to make the
> recovery process work".
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 07:42:58 +0300:

> On 05.06.2023 12:48, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@sberdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:
> >   
> >> Hello Miquel, thanks for review!
> >>
> >> On 01.06.2023 11:31, Miquel Raynal wrote:  
> >>> Hi Arseniy,
> >>>
> >>> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
> >>>     
> >>>> This moves free bytes of OOB to non-protected ECC area. It is needed to    
> >>>
> >>> As we discussed, I expect this commit to just change the OOB layout to
> >>> expose unprotected OOB bytes to the user, that is the only change this
> >>> commit should carry. If that does not work, you should add a
> >>> preparation patch.    
> >>
> >> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
> >> mtd_ooblayout_ops, I also need to implement code which performs read/write
> >> according new layout (it must be done in a single patch)?  
> > 
> > No, this is orthogonal.
> > 
> > The driver must read the the whole OOB area (and perhaps reorder the
> > data), but you should not make any decision regarding what bytes you
> > want or not want to expose.
> > 
> > Then, the user (no matter what "user" is here) will decide how to deal
> > with the data.  
> 
> Hello Miquel!
> 
> Ok, so in case of:
> 1) read I just need to read OOB data using 'nand_change_read_column_op()' and place it to 'oob_buf'.
> 2) write I need to write OOB data using 'nand_change_write_column_op()' to controller internal RAM and then call PAGE_PROG.
>    Even in ECC mode, data which occupies places of ECC codes will be removed by hw ( as You mentinoed below).
> 
> That's all?:)

Sounds right :)

If you find too costly to make many nand_change_read_column_op() and
want to leverage DMA instead, you can as well read everything
(data+oob) in one go in a bounce buffer and then perform memcpy's into
your final buffer in order to reorganize the data. That's entirely up to
you (and same of course in the write path, you could first
memcpy/reorder the data into a DMA bounce buffer locally, and then send
all the data in a single DMA transfer and call PAGE_PROG).

> 
> >   
> >> Main thing is:
> >>
> >> I guess that general confuse with this patch is that You consider
> >> that we change only OOB layout by moving user bytes out of ECC area, but at the same
> >> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
> >> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
> >> assumption is correct, in the next version I won't change size of user area in
> >> OOB, thus this patch will be reduced as some comments from this review.  
> > 
> > Exposing only 4 bytes was a mistake in the first place, please fix this
> > in a dedicated patch.  
> 
> So current (not merged) version exposes bytes 0,1,16,17 of OOB, You mean this is wrong?
> Correct way is to expose 32,33,48,49 (e.g. shifted by 32)?
> 
> Thanks, Arseniy
> 
> >   
> >>>> make JFFS2 works correctly with this NAND controller. Problem fires when
> >>>> JFFS2 driver writes cleanmarker to some page and later it tries to write
> >>>> to this page - write will be done successfully, but after that such page
> >>>> becomes unreadable due to invalid ECC codes. This happens because second
> >>>> write needs to update ECC codes, but it is impossible to do it correctly
> >>>> without block erase. So idea of this patch is to use the unprotected OOB
> >>>> area to store the cleanmarkers, so that they can be written by the
> >>>> filesystem without caring much about the page being empty or not: the
> >>>> ECC codes will not be written anyway.
> >>>>
> >>>> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
> >>>> are usually true SLC flashes, with the capability of writing a page with
> >>>> empty (0xFF) data, and still be able to write actual data to it later in
> >>>> a second write.
> >>>>
> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >>>> ---
> >>>>  Changelog v4->v5:
> >>>>  * Drop cosmetic changes from this patch.
> >>>>  * Do not ignore ECC protected user bytes provided by hw. Even these
> >>>>    bytes are out of user area of OOB, its values are still read from
> >>>>    the provided OOB buffer and written by hardware. Same behaviour is
> >>>>    preserved for read access - such bytes are read from DMA buffer and
> >>>>    placed to OOB buffer.
> >>>>  * OOB read and write become more lightweight because I removed heavy
> >>>>    READ0 and PAGEPROG command from it (both commands are still sent
> >>>>    when OOB access is performed using OOB callbacks). In case of page
> >>>>    read/write OOB data is handled in the internal SRAM of the controller.
> >>>>  * Commit message updated.
> >>>>  * Temporary buffer for OOB read/write is removed. Seems everything
> >>>>    works correctly without it.
> >>>>
> >>>>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
> >>>>  1 file changed, 117 insertions(+), 17 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >>>> index 82a629025adc..e42c28be02f3 100644
> >>>> --- a/drivers/mtd/nand/raw/meson_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >>>> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
> >>>>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>>>  				   u8 *buf, u8 *oobbuf)
> >>>>  {
> >>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>>  	int i, oob_len = 0;
> >>>>  	u8 *dsrc, *osrc;
> >>>> +	u8 *oobtail;
> >>>>  
> >>>>  	oob_len = nand->ecc.bytes + 2;
> >>>>  	for (i = 0; i < nand->ecc.steps; i++) {
> >>>> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>>>  			memcpy(buf, dsrc, nand->ecc.size);
> >>>>  			buf += nand->ecc.size;
> >>>>  		}
> >>>> +
> >>>>  		osrc = meson_nfc_oob_ptr(nand, i);
> >>>>  		memcpy(oobbuf, osrc, oob_len);
> >>>>  		oobbuf += oob_len;
> >>>>  	}
> >>>> +
> >>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >>>> +		  (nand->ecc.size + oob_len);
> >>>> +
> >>>> +	/* 'oobbuf' points to the start of user area. */
> >>>> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
> >>>>  }
> >>>>  
> >>>>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>>>  				   const u8 *buf, u8 *oobbuf)
> >>>>  {
> >>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>>  	int i, oob_len = 0;
> >>>>  	u8 *dsrc, *osrc;
> >>>> +	u8 *oobtail;
> >>>>  
> >>>>  	oob_len = nand->ecc.bytes + 2;
> >>>>  	for (i = 0; i < nand->ecc.steps; i++) {
> >>>> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>>>  		memcpy(osrc, oobbuf, oob_len);
> >>>>  		oobbuf += oob_len;
> >>>>  	}
> >>>> +
> >>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >>>> +		  (nand->ecc.size + oob_len);    
> >>>
> >>> This is always targeting the same area, so it looks strange to me.
> >>>     
> >>>> +
> >>>> +	/* 'oobbuf' points to the start of user area. */
> >>>> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);    
> >>>
> >>> TBH I don't get what you do here.    
> >>
> >> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
> >> This is because number of user bytes is increased in this patch.
> >>  
> >>>     
> >>>>  }
> >>>>  
> >>>>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
> >>>> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>>>  	__le64 *info;
> >>>>  	int i, count;
> >>>>  
> >>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>>>  		info = &meson_chip->info_buf[i];
> >>>>  		*info |= oob_buf[count];
> >>>>  		*info |= oob_buf[count + 1] << 8;
> >>>> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>>>  	__le64 *info;
> >>>>  	int i, count;
> >>>>  
> >>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>>>  		info = &meson_chip->info_buf[i];
> >>>>  		oob_buf[count] = *info;
> >>>>  		oob_buf[count + 1] = *info >> 8;
> >>>> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
> >>>> +{
> >>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>> +
> >>>> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);    
> >>>
> >>> This looks like a static value, just save it somewhere instead of
> >>> recomputing it?    
> >>
> >> Ack
> >>  
> >>>     
> >>>> +}
> >>>> +
> >>>> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
> >>>> +{
> >>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>> +	u32 page_size = mtd->writesize + mtd->oobsize;
> >>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >>>> +	u8 *oob_buf;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!oob_bytes)
> >>>> +		return 0;    
> >>>
> >>> Can this happen?    
> >>
> >> Ack, seems forget to remove it
> >>  
> >>>     
> >>>> +
> >>>> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
> >>>> +	if (page != -1) {
> >>>> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>> +	oob_buf = nand->oob_poi;
> >>>> +
> >>>> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
> >>>> +					  oob_buf + (mtd->oobsize - oob_bytes),
> >>>> +					  oob_bytes, false);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
> >>>> +}
> >>>> +
> >>>> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >>>> +{
> >>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>> +	u8 *oob_buf = nand->oob_poi;
> >>>> +	u32 oob_bytes;
> >>>> +	u32 page_size;
> >>>> +	int ret;
> >>>> +	int i;
> >>>> +
> >>>> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
> >>>> +	if (page != -1) {    
> >>>
> >>> I don't like this logic with the "-1", it really hides what the
> >>> controller needs to do, if you need a helper to send a command, then
> >>> create that helper and give it a decent name.    
> >>
> >> I see, so I think I need to implement this in the following way:
> >> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
> >> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
> >>  
> >>>     
> >>>> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>> +	/* Read ECC codes and user bytes. */
> >>>> +	for (i = 0; i < nand->ecc.steps; i++) {
> >>>> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
> >>>> +			       (nand->ecc.bytes + 2) * i;
> >>>> +
> >>>> +		ret = nand_change_read_column_op(nand, ecc_offs,
> >>>> +						 oob_buf + i * (nand->ecc.bytes + 2),
> >>>> +						 (nand->ecc.bytes + 2), false);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
> >>>> +
> >>>> +	if (!oob_bytes)
> >>>> +		return 0;
> >>>> +
> >>>> +	page_size = mtd->writesize + mtd->oobsize;
> >>>> +
> >>>> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
> >>>> +					 oob_buf + (mtd->oobsize - oob_bytes),
> >>>> +					 oob_bytes, false);
> >>>> +
> >>>> +	return ret;
> >>>> +}
> >>>> +
> >>>>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>>>  				    int page, int raw)
> >>>>  {
> >>>> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>>>  				     NFC_CMD_SCRAMBLER_DISABLE);
> >>>>  	}
> >>>>  
> >>>> +	if (!raw) {    
> >>>
> >>> Why this check?
> >>>
> >>> You should instead propagate the oob_required field and check that
> >>> value I believe.    
> >>
> >>
> >> This check is for ECC mode, because in this mode we write user bytes of OOB.
> >> ECC bytes of OOB are written by hardware.  
> > 
> > Just provide the buffer. The ECC engine will smash data if there was
> > any there. Otherwise it will fill the holes. It's expected. Don't try
> > to be smarter than you should :)
> >   
> >> I think I made a mistake, because
> >> I need new callback to write OOB in raw mode - it will write both ECC and user
> >> parts,  
> > 
> > There is no such thing as user and ECC part at the driver level. You
> > get a buffer, you need to write it to the flash.
> > 
> > The user expects:
> > 
> > | data | OOB |
> > 
> > The controller expects something like:
> > 
> > | data1 | OOB1 | data2 | OOB2 |
> > 
> > So just perform the reordering between data and OOB in the DMA buffer,
> > that is _all_.
> >   
> >> in current version I write only user part in raw mode.
> >>  
> >>>     
> >>>> +		ret = meson_nfc_write_oob(nand, -1);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
> >>>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>>>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
> >>>> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
> >>>>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
> >>>>  	}
> >>>>  
> >>>> -	return bitflips;
> >>>> -}
> >>>> -
> >>>> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
> >>>> -{
> >>>> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
> >>>> -}
> >>>> +	if (oob_required && ret)    
> >>>
> >>> Unclear why you check ret here?  
> > 
> > In general, if (ret) means there is an error.
> > 
> > Please consider using:
> > 
> > if (ret)
> > 	goto error path;
> > 
> > do something else;
> >   
> >>>     
> >>
> >> If read was successful, we read OOB. If not - there is no sense in it.
> >>  
> >>>> +		meson_nfc_read_oob(nand, -1);
> >>>>  
> >>>> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >>>> -{
> >>>> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
> >>>> +	return bitflips;
> >>>>  }
> >>>>  
> >>>>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
> >>>> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
> >>>>  				struct mtd_oob_region *oobregion)
> >>>>  {
> >>>>  	struct nand_chip *nand = mtd_to_nand(mtd);
> >>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >>>>  
> >>>>  	if (section >= nand->ecc.steps)
> >>>>  		return -ERANGE;
> >>>>  
> >>>> -	oobregion->offset = section * (2 + nand->ecc.bytes);    
> >>>
> >>> The first two bytes of OOB are reserved for the bad block markers. This
> >>> is not related to your controller.    
> >>
> >> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
> >> is considered by hardware as user bytes covered by ECC.  
> > 
> > The two first bytes should not be available. They are not "ECC" bytes,
> > they are not "free" bytes. None of these two callbacks should give
> > access to these two bytes reserved for bad block markers.
> > 
> > Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
> > "these are the protected bytes". They mean "these are the bytes in OOB
> > the hardware ECC engine will use to place its own data to make the
> > recovery process work".
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes

[Arseniy Krasnov]

On 06.06.2023 10:11, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 07:42:58 +0300:
> 
>> On 05.06.2023 12:48, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> avkrasnov@sberdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:
>>>   
>>>> Hello Miquel, thanks for review!
>>>>
>>>> On 01.06.2023 11:31, Miquel Raynal wrote:  
>>>>> Hi Arseniy,
>>>>>
>>>>> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
>>>>>     
>>>>>> This moves free bytes of OOB to non-protected ECC area. It is needed to    
>>>>>
>>>>> As we discussed, I expect this commit to just change the OOB layout to
>>>>> expose unprotected OOB bytes to the user, that is the only change this
>>>>> commit should carry. If that does not work, you should add a
>>>>> preparation patch.    
>>>>
>>>> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
>>>> mtd_ooblayout_ops, I also need to implement code which performs read/write
>>>> according new layout (it must be done in a single patch)?  
>>>
>>> No, this is orthogonal.
>>>
>>> The driver must read the the whole OOB area (and perhaps reorder the
>>> data), but you should not make any decision regarding what bytes you
>>> want or not want to expose.
>>>
>>> Then, the user (no matter what "user" is here) will decide how to deal
>>> with the data.  
>>
>> Hello Miquel!
>>
>> Ok, so in case of:
>> 1) read I just need to read OOB data using 'nand_change_read_column_op()' and place it to 'oob_buf'.
>> 2) write I need to write OOB data using 'nand_change_write_column_op()' to controller internal RAM and then call PAGE_PROG.
>>    Even in ECC mode, data which occupies places of ECC codes will be removed by hw ( as You mentinoed below).
>>
>> That's all?:)
> 
> Sounds right :)
> 
> If you find too costly to make many nand_change_read_column_op() and
> want to leverage DMA instead, you can as well read everything
> (data+oob) in one go in a bounce buffer and then perform memcpy's into
> your final buffer in order to reorganize the data. That's entirely up to
> you (and same of course in the write path, you could first
> memcpy/reorder the data into a DMA bounce buffer locally, and then send
> all the data in a single DMA transfer and call PAGE_PROG).

Got it!

Thanks, Arseniy

> 
>>
>>>   
>>>> Main thing is:
>>>>
>>>> I guess that general confuse with this patch is that You consider
>>>> that we change only OOB layout by moving user bytes out of ECC area, but at the same
>>>> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
>>>> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
>>>> assumption is correct, in the next version I won't change size of user area in
>>>> OOB, thus this patch will be reduced as some comments from this review.  
>>>
>>> Exposing only 4 bytes was a mistake in the first place, please fix this
>>> in a dedicated patch.  
>>
>> So current (not merged) version exposes bytes 0,1,16,17 of OOB, You mean this is wrong?
>> Correct way is to expose 32,33,48,49 (e.g. shifted by 32)?
>>
>> Thanks, Arseniy
>>
>>>   
>>>>>> make JFFS2 works correctly with this NAND controller. Problem fires when
>>>>>> JFFS2 driver writes cleanmarker to some page and later it tries to write
>>>>>> to this page - write will be done successfully, but after that such page
>>>>>> becomes unreadable due to invalid ECC codes. This happens because second
>>>>>> write needs to update ECC codes, but it is impossible to do it correctly
>>>>>> without block erase. So idea of this patch is to use the unprotected OOB
>>>>>> area to store the cleanmarkers, so that they can be written by the
>>>>>> filesystem without caring much about the page being empty or not: the
>>>>>> ECC codes will not be written anyway.
>>>>>>
>>>>>> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
>>>>>> are usually true SLC flashes, with the capability of writing a page with
>>>>>> empty (0xFF) data, and still be able to write actual data to it later in
>>>>>> a second write.
>>>>>>
>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>> ---
>>>>>>  Changelog v4->v5:
>>>>>>  * Drop cosmetic changes from this patch.
>>>>>>  * Do not ignore ECC protected user bytes provided by hw. Even these
>>>>>>    bytes are out of user area of OOB, its values are still read from
>>>>>>    the provided OOB buffer and written by hardware. Same behaviour is
>>>>>>    preserved for read access - such bytes are read from DMA buffer and
>>>>>>    placed to OOB buffer.
>>>>>>  * OOB read and write become more lightweight because I removed heavy
>>>>>>    READ0 and PAGEPROG command from it (both commands are still sent
>>>>>>    when OOB access is performed using OOB callbacks). In case of page
>>>>>>    read/write OOB data is handled in the internal SRAM of the controller.
>>>>>>  * Commit message updated.
>>>>>>  * Temporary buffer for OOB read/write is removed. Seems everything
>>>>>>    works correctly without it.
>>>>>>
>>>>>>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
>>>>>>  1 file changed, 117 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>>>> index 82a629025adc..e42c28be02f3 100644
>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>>>> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>>>>>>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>>>>  				   u8 *buf, u8 *oobbuf)
>>>>>>  {
>>>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>>  	int i, oob_len = 0;
>>>>>>  	u8 *dsrc, *osrc;
>>>>>> +	u8 *oobtail;
>>>>>>  
>>>>>>  	oob_len = nand->ecc.bytes + 2;
>>>>>>  	for (i = 0; i < nand->ecc.steps; i++) {
>>>>>> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>>>>  			memcpy(buf, dsrc, nand->ecc.size);
>>>>>>  			buf += nand->ecc.size;
>>>>>>  		}
>>>>>> +
>>>>>>  		osrc = meson_nfc_oob_ptr(nand, i);
>>>>>>  		memcpy(oobbuf, osrc, oob_len);
>>>>>>  		oobbuf += oob_len;
>>>>>>  	}
>>>>>> +
>>>>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>>>> +		  (nand->ecc.size + oob_len);
>>>>>> +
>>>>>> +	/* 'oobbuf' points to the start of user area. */
>>>>>> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
>>>>>>  }
>>>>>>  
>>>>>>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>>>>  				   const u8 *buf, u8 *oobbuf)
>>>>>>  {
>>>>>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>>  	int i, oob_len = 0;
>>>>>>  	u8 *dsrc, *osrc;
>>>>>> +	u8 *oobtail;
>>>>>>  
>>>>>>  	oob_len = nand->ecc.bytes + 2;
>>>>>>  	for (i = 0; i < nand->ecc.steps; i++) {
>>>>>> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>>>>  		memcpy(osrc, oobbuf, oob_len);
>>>>>>  		oobbuf += oob_len;
>>>>>>  	}
>>>>>> +
>>>>>> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>>>> +		  (nand->ecc.size + oob_len);    
>>>>>
>>>>> This is always targeting the same area, so it looks strange to me.
>>>>>     
>>>>>> +
>>>>>> +	/* 'oobbuf' points to the start of user area. */
>>>>>> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);    
>>>>>
>>>>> TBH I don't get what you do here.    
>>>>
>>>> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
>>>> This is because number of user bytes is increased in this patch.
>>>>  
>>>>>     
>>>>>>  }
>>>>>>  
>>>>>>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
>>>>>> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>>>  	__le64 *info;
>>>>>>  	int i, count;
>>>>>>  
>>>>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>>>>  		info = &meson_chip->info_buf[i];
>>>>>>  		*info |= oob_buf[count];
>>>>>>  		*info |= oob_buf[count + 1] << 8;
>>>>>> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>>>  	__le64 *info;
>>>>>>  	int i, count;
>>>>>>  
>>>>>> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>>>> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>>>>  		info = &meson_chip->info_buf[i];
>>>>>>  		oob_buf[count] = *info;
>>>>>>  		oob_buf[count + 1] = *info >> 8;
>>>>>> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
>>>>>> +{
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>> +
>>>>>> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);    
>>>>>
>>>>> This looks like a static value, just save it somewhere instead of
>>>>> recomputing it?    
>>>>
>>>> Ack
>>>>  
>>>>>     
>>>>>> +}
>>>>>> +
>>>>>> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
>>>>>> +{
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>> +	u32 page_size = mtd->writesize + mtd->oobsize;
>>>>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>>>> +	u8 *oob_buf;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (!oob_bytes)
>>>>>> +		return 0;    
>>>>>
>>>>> Can this happen?    
>>>>
>>>> Ack, seems forget to remove it
>>>>  
>>>>>     
>>>>>> +
>>>>>> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
>>>>>> +	if (page != -1) {
>>>>>> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	oob_buf = nand->oob_poi;
>>>>>> +
>>>>>> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
>>>>>> +					  oob_buf + (mtd->oobsize - oob_bytes),
>>>>>> +					  oob_bytes, false);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>>>> +{
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>> +	u8 *oob_buf = nand->oob_poi;
>>>>>> +	u32 oob_bytes;
>>>>>> +	u32 page_size;
>>>>>> +	int ret;
>>>>>> +	int i;
>>>>>> +
>>>>>> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
>>>>>> +	if (page != -1) {    
>>>>>
>>>>> I don't like this logic with the "-1", it really hides what the
>>>>> controller needs to do, if you need a helper to send a command, then
>>>>> create that helper and give it a decent name.    
>>>>
>>>> I see, so I think I need to implement this in the following way:
>>>> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
>>>> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
>>>>  
>>>>>     
>>>>>> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	/* Read ECC codes and user bytes. */
>>>>>> +	for (i = 0; i < nand->ecc.steps; i++) {
>>>>>> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
>>>>>> +			       (nand->ecc.bytes + 2) * i;
>>>>>> +
>>>>>> +		ret = nand_change_read_column_op(nand, ecc_offs,
>>>>>> +						 oob_buf + i * (nand->ecc.bytes + 2),
>>>>>> +						 (nand->ecc.bytes + 2), false);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>>>> +
>>>>>> +	if (!oob_bytes)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	page_size = mtd->writesize + mtd->oobsize;
>>>>>> +
>>>>>> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
>>>>>> +					 oob_buf + (mtd->oobsize - oob_bytes),
>>>>>> +					 oob_bytes, false);
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>>>>  				    int page, int raw)
>>>>>>  {
>>>>>> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>>>>  				     NFC_CMD_SCRAMBLER_DISABLE);
>>>>>>  	}
>>>>>>  
>>>>>> +	if (!raw) {    
>>>>>
>>>>> Why this check?
>>>>>
>>>>> You should instead propagate the oob_required field and check that
>>>>> value I believe.    
>>>>
>>>>
>>>> This check is for ECC mode, because in this mode we write user bytes of OOB.
>>>> ECC bytes of OOB are written by hardware.  
>>>
>>> Just provide the buffer. The ECC engine will smash data if there was
>>> any there. Otherwise it will fill the holes. It's expected. Don't try
>>> to be smarter than you should :)
>>>   
>>>> I think I made a mistake, because
>>>> I need new callback to write OOB in raw mode - it will write both ECC and user
>>>> parts,  
>>>
>>> There is no such thing as user and ECC part at the driver level. You
>>> get a buffer, you need to write it to the flash.
>>>
>>> The user expects:
>>>
>>> | data | OOB |
>>>
>>> The controller expects something like:
>>>
>>> | data1 | OOB1 | data2 | OOB2 |
>>>
>>> So just perform the reordering between data and OOB in the DMA buffer,
>>> that is _all_.
>>>   
>>>> in current version I write only user part in raw mode.
>>>>  
>>>>>     
>>>>>> +		ret = meson_nfc_write_oob(nand, -1);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>>>>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>>>>>> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>>>>>>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
>>>>>>  	}
>>>>>>  
>>>>>> -	return bitflips;
>>>>>> -}
>>>>>> -
>>>>>> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
>>>>>> -{
>>>>>> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
>>>>>> -}
>>>>>> +	if (oob_required && ret)    
>>>>>
>>>>> Unclear why you check ret here?  
>>>
>>> In general, if (ret) means there is an error.
>>>
>>> Please consider using:
>>>
>>> if (ret)
>>> 	goto error path;
>>>
>>> do something else;
>>>   
>>>>>     
>>>>
>>>> If read was successful, we read OOB. If not - there is no sense in it.
>>>>  
>>>>>> +		meson_nfc_read_oob(nand, -1);
>>>>>>  
>>>>>> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>>>> -{
>>>>>> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
>>>>>> +	return bitflips;
>>>>>>  }
>>>>>>  
>>>>>>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
>>>>>> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>>>>>>  				struct mtd_oob_region *oobregion)
>>>>>>  {
>>>>>>  	struct nand_chip *nand = mtd_to_nand(mtd);
>>>>>> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>>>>  
>>>>>>  	if (section >= nand->ecc.steps)
>>>>>>  		return -ERANGE;
>>>>>>  
>>>>>> -	oobregion->offset = section * (2 + nand->ecc.bytes);    
>>>>>
>>>>> The first two bytes of OOB are reserved for the bad block markers. This
>>>>> is not related to your controller.    
>>>>
>>>> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
>>>> is considered by hardware as user bytes covered by ECC.  
>>>
>>> The two first bytes should not be available. They are not "ECC" bytes,
>>> they are not "free" bytes. None of these two callbacks should give
>>> access to these two bytes reserved for bad block markers.
>>>
>>> Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
>>> "these are the protected bytes". They mean "these are the bytes in OOB
>>> the hardware ECC engine will use to place its own data to make the
>>> recovery process work".
>>>
>>> Thanks,
>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 4/6] mtd: rawnand: meson: use macro for OOB area

[Arseniy Krasnov]

This replaces constants and same patterns for OOB handling with special
macroses.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 33 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index e42c28be02f3..23a73268421b 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -108,6 +108,9 @@
 
 #define PER_INFO_BYTE		8
 
+#define NFC_USER_BYTES		2
+#define NFC_OOB_PER_ECC(nand)	((nand)->ecc.bytes + NFC_USER_BYTES)
+
 struct meson_nfc_nand_chip {
 	struct list_head node;
 	struct nand_chip nand;
@@ -339,7 +342,7 @@ static u8 *meson_nfc_oob_ptr(struct nand_chip *nand, int i)
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	int len;
 
-	len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i;
+	len = nand->ecc.size * (i + 1) + NFC_OOB_PER_ECC(nand) * i;
 
 	return meson_chip->data_buf + len;
 }
@@ -350,7 +353,7 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
 	int len, temp;
 
 	temp = nand->ecc.size + nand->ecc.bytes;
-	len = (temp + 2) * i;
+	len = (temp + NFC_USER_BYTES) * i;
 
 	return meson_chip->data_buf + len;
 }
@@ -364,7 +367,7 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
 	u8 *dsrc, *osrc;
 	u8 *oobtail;
 
-	oob_len = nand->ecc.bytes + 2;
+	oob_len = NFC_OOB_PER_ECC(nand);
 	for (i = 0; i < nand->ecc.steps; i++) {
 		if (buf) {
 			dsrc = meson_nfc_data_ptr(nand, i);
@@ -393,7 +396,7 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
 	u8 *dsrc, *osrc;
 	u8 *oobtail;
 
-	oob_len = nand->ecc.bytes + 2;
+	oob_len = NFC_OOB_PER_ECC(nand);
 	for (i = 0; i < nand->ecc.steps; i++) {
 		if (buf) {
 			dsrc = meson_nfc_data_ptr(nand, i);
@@ -452,7 +455,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
 	__le64 *info;
 	int i, count;
 
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
+	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += NFC_OOB_PER_ECC(nand)) {
 		info = &meson_chip->info_buf[i];
 		*info |= oob_buf[count];
 		*info |= oob_buf[count + 1] << 8;
@@ -465,7 +468,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
 	__le64 *info;
 	int i, count;
 
-	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
+	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += NFC_OOB_PER_ECC(nand)) {
 		info = &meson_chip->info_buf[i];
 		oob_buf[count] = *info;
 		oob_buf[count + 1] = *info >> 8;
@@ -661,7 +664,7 @@ static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
 {
 	struct mtd_info *mtd = nand_to_mtd(nand);
 
-	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);
+	return mtd->oobsize - nand->ecc.steps * NFC_OOB_PER_ECC(nand);
 }
 
 static int meson_nfc_write_oob(struct nand_chip *nand, int page)
@@ -712,11 +715,11 @@ static int meson_nfc_read_oob(struct nand_chip *nand, int page)
 	/* Read ECC codes and user bytes. */
 	for (i = 0; i < nand->ecc.steps; i++) {
 		u32 ecc_offs = nand->ecc.size * (i + 1) +
-			       (nand->ecc.bytes + 2) * i;
+			       NFC_OOB_PER_ECC(nand) * i;
 
 		ret = nand_change_read_column_op(nand, ecc_offs,
-						 oob_buf + i * (nand->ecc.bytes + 2),
-						 (nand->ecc.bytes + 2), false);
+						 oob_buf + i * NFC_OOB_PER_ECC(nand),
+						 NFC_OOB_PER_ECC(nand), false);
 		if (ret)
 			return ret;
 	}
@@ -918,12 +921,14 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
 
 		for (i = 0; i < nand->ecc.steps ; i++) {
 			u8 *data = buf + i * ecc->size;
-			u8 *oob = nand->oob_poi + i * (ecc->bytes + 2);
+			u8 *oob = nand->oob_poi + i * NFC_OOB_PER_ECC(nand);
 
 			if (correct_bitmap & BIT_ULL(i))
 				continue;
+
 			ret = nand_check_erased_ecc_chunk(data,	ecc->size,
-							  oob, ecc->bytes + 2,
+							  oob,
+							  NFC_OOB_PER_ECC(nand),
 							  NULL, 0,
 							  ecc->strength);
 			if (ret < 0) {
@@ -1073,7 +1078,7 @@ static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
 	if (section >= nand->ecc.steps)
 		return -ERANGE;
 
-	oobregion->offset =  2 + (section * (2 + nand->ecc.bytes));
+	oobregion->offset = NFC_USER_BYTES + (section * NFC_OOB_PER_ECC(nand));
 	oobregion->length = nand->ecc.bytes;
 
 	return 0;
@@ -1091,7 +1096,7 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
 	/* Split rest of OOB area (not covered by ECC engine) per each
 	 * ECC section. This will be OOB data available to user.
 	 */
-	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);
+	oobregion->offset = (section + nand->ecc.steps) * NFC_OOB_PER_ECC(nand);
 	oobregion->length = oob_bytes / nand->ecc.steps;
 
 	return 0;
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 4/6] mtd: rawnand: meson: use macro for OOB area

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:47 +0300:

> This replaces constants and same patterns for OOB handling with special
> macroses.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 33 ++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index e42c28be02f3..23a73268421b 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -108,6 +108,9 @@
>  
>  #define PER_INFO_BYTE		8
>  
> +#define NFC_USER_BYTES		2
> +#define NFC_OOB_PER_ECC(nand)	((nand)->ecc.bytes + NFC_USER_BYTES)

OOB per ECC for me does not make sense.
OOB is the whole area after the data.
What about NFC_OOB_SZ_PER_ECC_STEP ?

> +
>  struct meson_nfc_nand_chip {
>  	struct list_head node;
>  	struct nand_chip nand;
> @@ -339,7 +342,7 @@ static u8 *meson_nfc_oob_ptr(struct nand_chip *nand, int i)
>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>  	int len;
>  
> -	len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i;
> +	len = nand->ecc.size * (i + 1) + NFC_OOB_PER_ECC(nand) * i;
>  
>  	return meson_chip->data_buf + len;
>  }
> @@ -350,7 +353,7 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>  	int len, temp;
>  
>  	temp = nand->ecc.size + nand->ecc.bytes;
> -	len = (temp + 2) * i;
> +	len = (temp + NFC_USER_BYTES) * i;
>  
>  	return meson_chip->data_buf + len;
>  }
> @@ -364,7 +367,7 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>  	u8 *dsrc, *osrc;
>  	u8 *oobtail;
>  
> -	oob_len = nand->ecc.bytes + 2;
> +	oob_len = NFC_OOB_PER_ECC(nand);
>  	for (i = 0; i < nand->ecc.steps; i++) {
>  		if (buf) {
>  			dsrc = meson_nfc_data_ptr(nand, i);
> @@ -393,7 +396,7 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  	u8 *dsrc, *osrc;
>  	u8 *oobtail;
>  
> -	oob_len = nand->ecc.bytes + 2;
> +	oob_len = NFC_OOB_PER_ECC(nand);
>  	for (i = 0; i < nand->ecc.steps; i++) {
>  		if (buf) {
>  			dsrc = meson_nfc_data_ptr(nand, i);
> @@ -452,7 +455,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>  	__le64 *info;
>  	int i, count;
>  
> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += NFC_OOB_PER_ECC(nand)) {
>  		info = &meson_chip->info_buf[i];
>  		*info |= oob_buf[count];
>  		*info |= oob_buf[count + 1] << 8;
> @@ -465,7 +468,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>  	__le64 *info;
>  	int i, count;
>  
> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += NFC_OOB_PER_ECC(nand)) {
>  		info = &meson_chip->info_buf[i];
>  		oob_buf[count] = *info;
>  		oob_buf[count + 1] = *info >> 8;
> @@ -661,7 +664,7 @@ static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(nand);
>  
> -	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);
> +	return mtd->oobsize - nand->ecc.steps * NFC_OOB_PER_ECC(nand);
>  }
>  
>  static int meson_nfc_write_oob(struct nand_chip *nand, int page)
> @@ -712,11 +715,11 @@ static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>  	/* Read ECC codes and user bytes. */
>  	for (i = 0; i < nand->ecc.steps; i++) {
>  		u32 ecc_offs = nand->ecc.size * (i + 1) +
> -			       (nand->ecc.bytes + 2) * i;
> +			       NFC_OOB_PER_ECC(nand) * i;
>  
>  		ret = nand_change_read_column_op(nand, ecc_offs,
> -						 oob_buf + i * (nand->ecc.bytes + 2),
> -						 (nand->ecc.bytes + 2), false);
> +						 oob_buf + i * NFC_OOB_PER_ECC(nand),
> +						 NFC_OOB_PER_ECC(nand), false);
>  		if (ret)
>  			return ret;
>  	}
> @@ -918,12 +921,14 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>  
>  		for (i = 0; i < nand->ecc.steps ; i++) {
>  			u8 *data = buf + i * ecc->size;
> -			u8 *oob = nand->oob_poi + i * (ecc->bytes + 2);
> +			u8 *oob = nand->oob_poi + i * NFC_OOB_PER_ECC(nand);
>  
>  			if (correct_bitmap & BIT_ULL(i))
>  				continue;
> +
>  			ret = nand_check_erased_ecc_chunk(data,	ecc->size,
> -							  oob, ecc->bytes + 2,
> +							  oob,
> +							  NFC_OOB_PER_ECC(nand),
>  							  NULL, 0,
>  							  ecc->strength);
>  			if (ret < 0) {
> @@ -1073,7 +1078,7 @@ static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
>  	if (section >= nand->ecc.steps)
>  		return -ERANGE;
>  
> -	oobregion->offset =  2 + (section * (2 + nand->ecc.bytes));
> +	oobregion->offset = NFC_USER_BYTES + (section * NFC_OOB_PER_ECC(nand));

No, the first "2" here is for bad block markers, it is not related to
your ECC engine layout I believe.

>  	oobregion->length = nand->ecc.bytes;
>  
>  	return 0;
> @@ -1091,7 +1096,7 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>  	/* Split rest of OOB area (not covered by ECC engine) per each
>  	 * ECC section. This will be OOB data available to user.
>  	 */
> -	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);
> +	oobregion->offset = (section + nand->ecc.steps) * NFC_OOB_PER_ECC(nand);
>  	oobregion->length = oob_bytes / nand->ecc.steps;
>  
>  	return 0;


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 5/6] mtd: rawnand: meson: check buffer length

[Arseniy Krasnov]

This NAND controller has limited buffer length, so check it before
command execution to avoid length trim. Also check MTD write size on
chip attach.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 Changelog v4->v5:
 * Move length checks from functions 'meson_nfc_read/write_buf()' to
   'meson_nfc_exec_op()'.

 drivers/mtd/nand/raw/meson_nand.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 23a73268421b..2a91916566f4 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -111,6 +111,8 @@
 #define NFC_USER_BYTES		2
 #define NFC_OOB_PER_ECC(nand)	((nand)->ecc.bytes + NFC_USER_BYTES)
 
+#define NFC_CMD_RAW_LEN		GENMASK(13, 0)
+
 struct meson_nfc_nand_chip {
 	struct list_head node;
 	struct nand_chip nand;
@@ -284,7 +286,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
 
 	if (raw) {
 		len = mtd->writesize + mtd->oobsize;
-		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);
+		cmd = len | scrambler | DMA_DIR(dir);
 		writel(cmd, nfc->reg_base + NFC_REG_CMD);
 		return;
 	}
@@ -573,7 +575,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
 	if (ret)
 		goto out;
 
-	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
+	cmd = NFC_CMD_N2M | len;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
@@ -597,7 +599,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
 	if (ret)
 		return ret;
 
-	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
+	cmd = NFC_CMD_M2N | len;
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
@@ -1044,6 +1046,9 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
 			break;
 
 		case NAND_OP_DATA_IN_INSTR:
+			if (instr->ctx.data.len > NFC_CMD_RAW_LEN)
+				return -EINVAL;
+
 			buf = meson_nand_op_get_dma_safe_input_buf(instr);
 			if (!buf)
 				return -ENOMEM;
@@ -1052,6 +1057,9 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
 			break;
 
 		case NAND_OP_DATA_OUT_INSTR:
+			if (instr->ctx.data.len > NFC_CMD_RAW_LEN)
+				return -EINVAL;
+
 			buf = meson_nand_op_get_dma_safe_output_buf(instr);
 			if (!buf)
 				return -ENOMEM;
@@ -1293,6 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	struct mtd_info *mtd = nand_to_mtd(nand);
 	int nsectors = mtd->writesize / 1024;
+	int raw_writesize;
 	int ret;
 
 	if (!mtd->name) {
@@ -1304,6 +1313,13 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 			return -ENOMEM;
 	}
 
+	raw_writesize = mtd->writesize + mtd->oobsize;
+	if (raw_writesize > NFC_CMD_RAW_LEN) {
+		dev_err(nfc->dev, "too big write size in raw mode: %d > %ld\n",
+			raw_writesize, NFC_CMD_RAW_LEN);
+		return -EINVAL;
+	}
+
 	if (nand->bbt_options & NAND_BBT_USE_FLASH)
 		nand->bbt_options |= NAND_BBT_NO_OOB;
 
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH v5 6/6] mtd: rawnand: meson: remove unneeded bitwise OR with zeroes

[Arseniy Krasnov]

Both operations have no effect.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 2a91916566f4..e78912bd3b4d 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -626,12 +626,12 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
 	cmd0 = in ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
 	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
 
-	addrs[0] = cs | NFC_CMD_ALE | 0;
+	addrs[0] = cs | NFC_CMD_ALE;
 	if (mtd->writesize <= 512) {
 		cmd_num--;
 		row_start = 1;
 	} else {
-		addrs[1] = cs | NFC_CMD_ALE | 0;
+		addrs[1] = cs | NFC_CMD_ALE;
 		row_start = 2;
 	}
 
-- 
2.35.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 0/6] refactoring, fixes and updates for Meson NAND

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:43 +0300:

> Hello,
> 
> this patchset does several things:

All the fixes should contain:

Fixes: <hash> ("log")
Cc: stable@vger.kernel.org

> 
> 1) Fixes value of ready/busy command. This new value was suggested by
>    Liang Yang <liang.yang@amlogic.com>.
> 
> 2) Adds waiting for command completion by calling 'nand_soft_waitrdy()'
>    instead of using ready/busy pin and command from 1). This is really
>    needed because I don't have device with such pin implemented.
> 
> 3) It moves OOB free bytes to non-protected by ECC area. Here are some
>    details:
> 
>    Current OOB free bytes are 4 bytes (2 x 2 user bytes) under ECC engine.
>    Here is how it looks like in the current implementation:
> 
>    [ 2B user bytes ][     14B ECC codes    ]
>    [ 2B user bytes ][     14B ECC codes    ]
>    [ 16B unused area, not protected by ECC ]
>    [ 16B unused area, not protected by ECC ]
> 
>    All 4 user bytes are protected by ECC. This patch changes OOB free
>    bytes in this way:
> 
>    [ 2B unused area ][     14B ECC codes     ]
>    [ 2B unused area ][     14B ECC codes     ]
>    [  16B user bytes, not protected by ECC   ]
>    [  16B user bytes, not protected by ECC   ]
> 
>    Now OOB user bytes are 32 bytes instead of 4 bytes and not protected
>    by ECC.
> 
>    Motivation of this layout comes from problem with JFFS2. It uses OOB
>    free bytes for cleanmarkers. Each cleanmarker is 4 bytes and written
>    by JFFS2 driver (small remark - cleanmarkers are always written in
>    case of NAND storage for JFFS2).
>    We have two ways to write this data to OOB (e.g. user bytes):
> 
>    1) ECC mode. In this case it will be ECC covered user bytes, e.g.
>       writing this bytes will update ECC codes. Problem fires, when
>       JFFS2 tries to write this page later - this write makes controller
>       to update ECC codes again, but it is impossible to do it correctly,
>       because we can't update bits from 0 to 1 (only from 1 to 0).
> 
>    2) Raw mode. In this case ECC codes won't be updated. But later, it
>       will be impossible to read this page in ECC mode, because we have
>       some user bytes, but ECC codes are missed.
> 
>    So let's move OOB free bytes out of ECC area. In this case we can
>    read/write OOB separately in raw mode and at the same time work with
>    data in ECC mode. JFFS2 is happy now. User bytes are untouched - all
>    of them are ignored during non-OOB access.
> 
>    I've tested this with mount/unmount/read/write cases for JFFS2 and
>    nanddump/nandwrite utlities on AXG family (A113X SoC).
> 
>    Here is link to discussion:
>    https://lore.kernel.org/linux-mtd/a9f8307a-77d7-a69f-ce11-2629909172d2@sberdevices.ru/T/#m3087bd06386a7f430cd5e343e22b25d724d3e2d7
> 
> 4) Replaces calculation of OOB related thing with macros. This is just
>    cosmetic change.
> 
> 5) Checks buffer length on accesses to NAND controller.
> 
> 6) Removes useless bitwise OR with zeroes.
> 
> Link to v1:
> https://lore.kernel.org/linux-mtd/20230412061700.1492474-1-AVKrasnov@sberdevices.ru/
> Link to v2:
> https://lore.kernel.org/linux-mtd/20230426073632.3905682-1-AVKrasnov@sberdevices.ru/
> Link to v3:
> https://lore.kernel.org/linux-mtd/20230510110835.26115-1-AVKrasnov@sberdevices.ru/
> Link to v4:
> https://lore.kernel.org/linux-mtd/20230515094440.3552094-1-AVKrasnov@sberdevices.ru/
> 
> Changelog:
> 
> v1 -> v2:
>  * Add patch which renames dts value for chip select.
>  * Add patch which moves OOB to non-protected ECC area.
> v2 -> v3:
>  * Change patch which fixes read/write access according discussion link
>    in 1) above.
> v3 -> v4:
>  * Remove patch which renames dts value for chip select.
>    Here is link to discussion:
>    https://lore.kernel.org/linux-mtd/20230510110835.26115-7-AVKrasnov@sberdevices.ru/
>  * Pass 1 to 'meson_nfc_queue_rb()' in case of NAND_OP_WAITRDY_INSTR.
>    This fixes ONFI page processing during NAND driver initialization.
> v4 -> v5:
>  * Move update of 'NFC_CMD_RB_INT' to the separate patch.
>  * Replace code which uses extra status and READ0 commands for waiting
>    command with 'nand_soft_waitrdy()'. In fact this patch adds second
>    mode for command waiting by using 'nand_soft_waitrdy()'.
>  * For OOB layout patch see changelog in a patch file.
>  * For check length patch see changelog in a patch file.
> 
> Arseniy Krasnov (6):
>   mtd: rawnand: meson: fix ready/busy command
>   mtd: rawnand: meson: wait for command in polling mode
>   mtd: rawnand: meson: only expose unprotected user OOB bytes
>   mtd: rawnand: meson: use macro for OOB area
>   mtd: rawnand: meson: check buffer length
>   mtd: rawnand: meson: remove unneeded bitwise OR with zeroes
> 
>  drivers/mtd/nand/raw/meson_nand.c | 234 +++++++++++++++++++++++-------
>  1 file changed, 182 insertions(+), 52 deletions(-)
> 


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH v5 0/6] refactoring, fixes and updates for Meson NAND

[Arseniy Krasnov]

On 01.06.2023 10:50, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:43 +0300:
> 
>> Hello,
>>
>> this patchset does several things:
> 
> All the fixes should contain:
> 
> Fixes: <hash> ("log")
> Cc: stable@vger.kernel.org

Hello Miquel,

Sorry. my fault, I'll add this in the next version for 0001 (anyway i think current is not final).

Thanks, Arseniy

> 
>>
>> 1) Fixes value of ready/busy command. This new value was suggested by
>>    Liang Yang <liang.yang@amlogic.com>.
>>
>> 2) Adds waiting for command completion by calling 'nand_soft_waitrdy()'
>>    instead of using ready/busy pin and command from 1). This is really
>>    needed because I don't have device with such pin implemented.
>>
>> 3) It moves OOB free bytes to non-protected by ECC area. Here are some
>>    details:
>>
>>    Current OOB free bytes are 4 bytes (2 x 2 user bytes) under ECC engine.
>>    Here is how it looks like in the current implementation:
>>
>>    [ 2B user bytes ][     14B ECC codes    ]
>>    [ 2B user bytes ][     14B ECC codes    ]
>>    [ 16B unused area, not protected by ECC ]
>>    [ 16B unused area, not protected by ECC ]
>>
>>    All 4 user bytes are protected by ECC. This patch changes OOB free
>>    bytes in this way:
>>
>>    [ 2B unused area ][     14B ECC codes     ]
>>    [ 2B unused area ][     14B ECC codes     ]
>>    [  16B user bytes, not protected by ECC   ]
>>    [  16B user bytes, not protected by ECC   ]
>>
>>    Now OOB user bytes are 32 bytes instead of 4 bytes and not protected
>>    by ECC.
>>
>>    Motivation of this layout comes from problem with JFFS2. It uses OOB
>>    free bytes for cleanmarkers. Each cleanmarker is 4 bytes and written
>>    by JFFS2 driver (small remark - cleanmarkers are always written in
>>    case of NAND storage for JFFS2).
>>    We have two ways to write this data to OOB (e.g. user bytes):
>>
>>    1) ECC mode. In this case it will be ECC covered user bytes, e.g.
>>       writing this bytes will update ECC codes. Problem fires, when
>>       JFFS2 tries to write this page later - this write makes controller
>>       to update ECC codes again, but it is impossible to do it correctly,
>>       because we can't update bits from 0 to 1 (only from 1 to 0).
>>
>>    2) Raw mode. In this case ECC codes won't be updated. But later, it
>>       will be impossible to read this page in ECC mode, because we have
>>       some user bytes, but ECC codes are missed.
>>
>>    So let's move OOB free bytes out of ECC area. In this case we can
>>    read/write OOB separately in raw mode and at the same time work with
>>    data in ECC mode. JFFS2 is happy now. User bytes are untouched - all
>>    of them are ignored during non-OOB access.
>>
>>    I've tested this with mount/unmount/read/write cases for JFFS2 and
>>    nanddump/nandwrite utlities on AXG family (A113X SoC).
>>
>>    Here is link to discussion:
>>    https://lore.kernel.org/linux-mtd/a9f8307a-77d7-a69f-ce11-2629909172d2@sberdevices.ru/T/#m3087bd06386a7f430cd5e343e22b25d724d3e2d7
>>
>> 4) Replaces calculation of OOB related thing with macros. This is just
>>    cosmetic change.
>>
>> 5) Checks buffer length on accesses to NAND controller.
>>
>> 6) Removes useless bitwise OR with zeroes.
>>
>> Link to v1:
>> https://lore.kernel.org/linux-mtd/20230412061700.1492474-1-AVKrasnov@sberdevices.ru/
>> Link to v2:
>> https://lore.kernel.org/linux-mtd/20230426073632.3905682-1-AVKrasnov@sberdevices.ru/
>> Link to v3:
>> https://lore.kernel.org/linux-mtd/20230510110835.26115-1-AVKrasnov@sberdevices.ru/
>> Link to v4:
>> https://lore.kernel.org/linux-mtd/20230515094440.3552094-1-AVKrasnov@sberdevices.ru/
>>
>> Changelog:
>>
>> v1 -> v2:
>>  * Add patch which renames dts value for chip select.
>>  * Add patch which moves OOB to non-protected ECC area.
>> v2 -> v3:
>>  * Change patch which fixes read/write access according discussion link
>>    in 1) above.
>> v3 -> v4:
>>  * Remove patch which renames dts value for chip select.
>>    Here is link to discussion:
>>    https://lore.kernel.org/linux-mtd/20230510110835.26115-7-AVKrasnov@sberdevices.ru/
>>  * Pass 1 to 'meson_nfc_queue_rb()' in case of NAND_OP_WAITRDY_INSTR.
>>    This fixes ONFI page processing during NAND driver initialization.
>> v4 -> v5:
>>  * Move update of 'NFC_CMD_RB_INT' to the separate patch.
>>  * Replace code which uses extra status and READ0 commands for waiting
>>    command with 'nand_soft_waitrdy()'. In fact this patch adds second
>>    mode for command waiting by using 'nand_soft_waitrdy()'.
>>  * For OOB layout patch see changelog in a patch file.
>>  * For check length patch see changelog in a patch file.
>>
>> Arseniy Krasnov (6):
>>   mtd: rawnand: meson: fix ready/busy command
>>   mtd: rawnand: meson: wait for command in polling mode
>>   mtd: rawnand: meson: only expose unprotected user OOB bytes
>>   mtd: rawnand: meson: use macro for OOB area
>>   mtd: rawnand: meson: check buffer length
>>   mtd: rawnand: meson: remove unneeded bitwise OR with zeroes
>>
>>  drivers/mtd/nand/raw/meson_nand.c | 234 +++++++++++++++++++++++-------
>>  1 file changed, 182 insertions(+), 52 deletions(-)
>>
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic