linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
@ 2020-10-16  7:49 Miquel Raynal
  2020-10-16 11:32 ` Fabio Estevam
  2020-10-16 13:53 ` Sascha Hauer
  0 siblings, 2 replies; 12+ messages in thread
From: Miquel Raynal @ 2020-10-16  7:49 UTC (permalink / raw)
  To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, linux-mtd
  Cc: Miquel Raynal, Sascha Hauer, Han Xu, Fabio Estevam, Lucas Stach

No ECC initialization should happen during the host controller probe.

Indeed, we need the probe to call nand_scan() in order to:
- identify the device, its capabilities and constraints (nand_scan_ident())
- configure the ECC engine accordingly (->attach_chip())
- scan its content and prepare the core (nand_scan_tail())

Moving these lines to mxcnd_attach_chip() fixes a regression caused by
a previous commit supposed to clarify these steps.

Fixes: TODO
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Hi Han,

Could you please give this patch a shot? It is supposed to fix
the LS1043A issue we have seen in robots reports the last weeks.

I kept the Fixes: tag empty because I need the original patch to be
merged in Linus' tree first but if it fixes the issue I will merge it
at -rc1 or -rc2.

Thanks,
Miquèl

 drivers/mtd/nand/raw/mxc_nand.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index d4200eb2ad32..6ba96f343a3d 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1681,6 +1681,18 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
 	struct device *dev = mtd->dev.parent;
 
+	chip->ecc.bytes = host->devtype_data->eccbytes;
+	host->eccsize = host->devtype_data->eccsize;
+	chip->ecc.size = 512;
+	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
+
+	if (host->pdata.hw_ecc) {
+		chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
+	} else {
+		chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
+		chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
+	}
+
 	switch (chip->ecc.engine_type) {
 	case NAND_ECC_ENGINE_TYPE_ON_HOST:
 		chip->ecc.read_page = mxc_nand_read_page;
@@ -1836,19 +1848,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 	if (host->devtype_data->axi_offset)
 		host->regs_axi = host->base + host->devtype_data->axi_offset;
 
-	this->ecc.bytes = host->devtype_data->eccbytes;
-	host->eccsize = host->devtype_data->eccsize;
-
 	this->legacy.select_chip = host->devtype_data->select_chip;
-	this->ecc.size = 512;
-	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
-
-	if (host->pdata.hw_ecc) {
-		this->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
-	} else {
-		this->ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
-		this->ecc.algo = NAND_ECC_ALGO_HAMMING;
-	}
 
 	/* NAND bus width determines access functions used by upper layer */
 	if (host->pdata.width == 2)
-- 
2.20.1


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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16  7:49 [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place Miquel Raynal
@ 2020-10-16 11:32 ` Fabio Estevam
  2020-10-16 11:45   ` Fabio Estevam
  2020-10-16 13:53 ` Sascha Hauer
  1 sibling, 1 reply; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 11:32 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 4:49 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> No ECC initialization should happen during the host controller probe.
>
> Indeed, we need the probe to call nand_scan() in order to:
> - identify the device, its capabilities and constraints (nand_scan_ident())
> - configure the ECC engine accordingly (->attach_chip())
> - scan its content and prepare the core (nand_scan_tail())
>
> Moving these lines to mxcnd_attach_chip() fixes a regression caused by
> a previous commit supposed to clarify these steps.
>
> Fixes: TODO
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>
> Hi Han,
>
> Could you please give this patch a shot? It is supposed to fix
> the LS1043A issue we have seen in robots reports the last weeks.

Thanks for the mxc_nand fix!

The LS1043A uses a different NAND controller and its driver is
drivers/mtd/nand/raw/fsl_ifc_nand.c

Thanks

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 11:32 ` Fabio Estevam
@ 2020-10-16 11:45   ` Fabio Estevam
  2020-10-16 12:05     ` Miquel Raynal
  0 siblings, 1 reply; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 11:45 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel and Han,

On Fri, Oct 16, 2020 at 8:32 AM Fabio Estevam <festevam@gmail.com> wrote:

> The LS1043A uses a different NAND controller and its driver is
> drivers/mtd/nand/raw/fsl_ifc_nand.c

Should we follow the same idea here and move the ECC initialization to
fsl_ifc_attach_chip()?

Does this patch help?
https://pastebin.com/raw/xwHKXFmu

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 11:45   ` Fabio Estevam
@ 2020-10-16 12:05     ` Miquel Raynal
  2020-10-16 12:11       ` Fabio Estevam
  0 siblings, 1 reply; 12+ messages in thread
From: Miquel Raynal @ 2020-10-16 12:05 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Fabio,

Fabio Estevam <festevam@gmail.com> wrote on Fri, 16 Oct 2020 08:45:08
-0300:

> Hi Miquel and Han,
> 
> On Fri, Oct 16, 2020 at 8:32 AM Fabio Estevam <festevam@gmail.com> wrote:
> 
> > The LS1043A uses a different NAND controller and its driver is
> > drivers/mtd/nand/raw/fsl_ifc_nand.c  
> 
> Should we follow the same idea here and move the ECC initialization to
> fsl_ifc_attach_chip()?
> 
> Does this patch help?
> https://pastebin.com/raw/xwHKXFmu

Definitely, yes!

I guess I will have to fix all the drivers doing part of the ECC
initialization in their probe function with the same logic. Hopefully
there are not so many...

Thanks,
Miquèl

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 12:05     ` Miquel Raynal
@ 2020-10-16 12:11       ` Fabio Estevam
  0 siblings, 0 replies; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 12:11 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 9:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> > Does this patch help?
> > https://pastebin.com/raw/xwHKXFmu
>
> Definitely, yes!
>
> I guess I will have to fix all the drivers doing part of the ECC
> initialization in their probe function with the same logic. Hopefully
> there are not so many...

Ok, let me send a formal patch for the ifc driver then.

I will also audit the other drivers.

Thanks,

Fabio Estevam

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16  7:49 [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place Miquel Raynal
  2020-10-16 11:32 ` Fabio Estevam
@ 2020-10-16 13:53 ` Sascha Hauer
  2020-10-16 17:01   ` Miquel Raynal
  1 sibling, 1 reply; 12+ messages in thread
From: Sascha Hauer @ 2020-10-16 13:53 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	linux-mtd, Fabio Estevam, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 09:49:42AM +0200, Miquel Raynal wrote:
> No ECC initialization should happen during the host controller probe.
> 
> Indeed, we need the probe to call nand_scan() in order to:
> - identify the device, its capabilities and constraints (nand_scan_ident())
> - configure the ECC engine accordingly (->attach_chip())
> - scan its content and prepare the core (nand_scan_tail())
> 
> Moving these lines to mxcnd_attach_chip() fixes a regression caused by
> a previous commit supposed to clarify these steps.
> 
> Fixes: TODO
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> 
> Hi Han,
> 
> Could you please give this patch a shot? It is supposed to fix
> the LS1043A issue we have seen in robots reports the last weeks.

With this there's no longer a division by zero in the kernel, but NAND
now fails with the following. Also I can confirm that "mtd: rawnand:
Use the ECC framework user input parsing bits" in next breaks it, without
this patch the driver runs well.

Sascha

nand: device found, Manufacturer ID: 0x20, Chip ID: 0xa1
nand: ST Micro NAND01GR3B2CZA6
nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/mxc_nand.c:1390 mxc_nand_command+0xc4/0x26c
Unimplemented command (cmd=0)
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.9.0-rc2-00020-g2e00450a5852 #20
Hardware name: Freescale i.MX27 (Device Tree Support)
[<c00106a4>] (unwind_backtrace) from [<c000dd08>] (show_stack+0x10/0x18)
[<c000dd08>] (show_stack) from [<c037950c>] (dump_stack+0x20/0x2c)
[<c037950c>] (dump_stack) from [<c001c68c>] (__warn+0xb8/0xec)
[<c001c68c>] (__warn) from [<c001ca54>] (warn_slowpath_fmt+0x90/0xbc)
[<c001ca54>] (warn_slowpath_fmt) from [<c0494cb8>] (mxc_nand_command+0xc4/0x26c)
[<c0494cb8>] (mxc_nand_command) from [<c0489f50>] (nand_read_page_op+0x28c/0x300)
[<c0489f50>] (nand_read_page_op) from [<c048a0d4>] (nand_read_page_raw+0x2c/0x6c)
[<c048a0d4>] (nand_read_page_raw) from [<c0486b68>] (nand_read_page_swecc+0x38/0x11c)
[<c0486b68>] (nand_read_page_swecc) from [<c0487f5c>] (nand_read_oob+0x238/0x730)
[<c0487f5c>] (nand_read_oob) from [<c047650c>] (mtd_read_oob+0x84/0x14c)
[<c047650c>] (mtd_read_oob) from [<c048eb7c>] (scan_read+0xd0/0x138)
[<c048eb7c>] (scan_read) from [<c048fe18>] (search_bbt+0x254/0x2cc)
[<c048fe18>] (search_bbt) from [<c0490068>] (nand_create_bbt+0x1d8/0x6ec)
[<c0490068>] (nand_create_bbt) from [<c048d20c>] (nand_scan_with_ids+0x10fc/0x164c)
[<c048d20c>] (nand_scan_with_ids) from [<c0495dd8>] (mxcnd_probe+0x2bc/0x3b4)
[<c0495dd8>] (mxcnd_probe) from [<c041c2b4>] (platform_drv_probe+0x4c/0xa0)
[<c041c2b4>] (platform_drv_probe) from [<c0419e18>] (really_probe+0x1e8/0x3d0)
[<c0419e18>] (really_probe) from [<c041a170>] (driver_probe_device+0x54/0xb0)
[<c041a170>] (driver_probe_device) from [<c041a384>] (device_driver_attach+0x5c/0x64)
[<c041a384>] (device_driver_attach) from [<c041a3e8>] (__driver_attach+0x5c/0xcc)
[<c041a3e8>] (__driver_attach) from [<c0417f30>] (bus_for_each_dev+0x78/0xc4)
[<c0417f30>] (bus_for_each_dev) from [<c0419734>] (driver_attach+0x18/0x24)
[<c0419734>] (driver_attach) from [<c04191b4>] (bus_add_driver+0x178/0x1d8)
[<c04191b4>] (bus_add_driver) from [<c041af74>] (driver_register+0x74/0x114)
[<c041af74>] (driver_register) from [<c041c20c>] (__platform_driver_register+0x30/0x48)
[<c041c20c>] (__platform_driver_register) from [<c090ecb8>] (mxcnd_driver_init+0x10/0x1c)
[<c090ecb8>] (mxcnd_driver_init) from [<c000a1dc>] (do_one_initcall+0x50/0x278)
[<c000a1dc>] (do_one_initcall) from [<c08ebffc>] (kernel_init_freeable+0x13c/0x1c0)
[<c08ebffc>] (kernel_init_freeable) from [<c070e150>] (kernel_init+0x8/0xf8)
[<c070e150>] (kernel_init) from [<c0008560>] (ret_from_fork+0x14/0x34)
Exception stack(0xc783dfb0 to 0xc783dff8)
dfa0:                                     00000000 00000000 00000000 00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 0f9701f8ecf348de ]---
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
Bad block table not found for chip 0
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error
__nand_correct_data: uncorrectable ECC error

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 13:53 ` Sascha Hauer
@ 2020-10-16 17:01   ` Miquel Raynal
  2020-10-16 17:37     ` Fabio Estevam
  2020-10-16 19:18     ` Fabio Estevam
  0 siblings, 2 replies; 12+ messages in thread
From: Miquel Raynal @ 2020-10-16 17:01 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	linux-mtd, Fabio Estevam, Lucas Stach

Hi Sascha,

Sascha Hauer <s.hauer@pengutronix.de> wrote on Fri, 16 Oct 2020
15:53:51 +0200:

> Hi Miquel,
> 
> On Fri, Oct 16, 2020 at 09:49:42AM +0200, Miquel Raynal wrote:
> > No ECC initialization should happen during the host controller probe.
> > 
> > Indeed, we need the probe to call nand_scan() in order to:
> > - identify the device, its capabilities and constraints (nand_scan_ident())
> > - configure the ECC engine accordingly (->attach_chip())
> > - scan its content and prepare the core (nand_scan_tail())
> > 
> > Moving these lines to mxcnd_attach_chip() fixes a regression caused by
> > a previous commit supposed to clarify these steps.
> > 
> > Fixes: TODO
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > 
> > Hi Han,
> > 
> > Could you please give this patch a shot? It is supposed to fix
> > the LS1043A issue we have seen in robots reports the last weeks.  
> 
> With this there's no longer a division by zero in the kernel, but NAND
> now fails with the following. Also I can confirm that "mtd: rawnand:
> Use the ECC framework user input parsing bits" in next breaks it, without
> this patch the driver runs well.
> 
> Sascha
> 
> nand: device found, Manufacturer ID: 0x20, Chip ID: 0xa1
> nand: ST Micro NAND01GR3B2CZA6
> nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/mxc_nand.c:1390 mxc_nand_command+0xc4/0x26c
> Unimplemented command (cmd=0)
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.9.0-rc2-00020-g2e00450a5852 #20
> Hardware name: Freescale i.MX27 (Device Tree Support)
> [<c00106a4>] (unwind_backtrace) from [<c000dd08>] (show_stack+0x10/0x18)
> [<c000dd08>] (show_stack) from [<c037950c>] (dump_stack+0x20/0x2c)
> [<c037950c>] (dump_stack) from [<c001c68c>] (__warn+0xb8/0xec)
> [<c001c68c>] (__warn) from [<c001ca54>] (warn_slowpath_fmt+0x90/0xbc)
> [<c001ca54>] (warn_slowpath_fmt) from [<c0494cb8>] (mxc_nand_command+0xc4/0x26c)
> [<c0494cb8>] (mxc_nand_command) from [<c0489f50>] (nand_read_page_op+0x28c/0x300)
> [<c0489f50>] (nand_read_page_op) from [<c048a0d4>] (nand_read_page_raw+0x2c/0x6c)
> [<c048a0d4>] (nand_read_page_raw) from [<c0486b68>] (nand_read_page_swecc+0x38/0x11c)
> [<c0486b68>] (nand_read_page_swecc) from [<c0487f5c>] (nand_read_oob+0x238/0x730)

Software ECC is used, is it expected?

Can you trace rawnand_dt_init() to see what happen in the engine_type
choice?

Thanks,
Miquèl

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 17:01   ` Miquel Raynal
@ 2020-10-16 17:37     ` Fabio Estevam
  2020-10-16 19:18     ` Fabio Estevam
  1 sibling, 0 replies; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 17:37 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 2:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Software ECC is used, is it expected?
>
> Can you trace rawnand_dt_init() to see what happen in the engine_type
> choice?

I managed to resurrect an old imx27-pdk board here and reproduced the
same behavior as Sascha reported.

engine_type looks good. ecc.size is still 0.

I am using this debug patch:

--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5036,6 +5036,12 @@ static int rawnand_dt_init(struct nand_chip *chip)
        chip->ecc.strength = nand->ecc.user_conf.strength;
        chip->ecc.size = nand->ecc.user_conf.step_size;

+
+       pr_info("********** chip->ecc.engine_type is %d\n",
chip->ecc.engine_type);
+       pr_info("********** chip->ecc.strength is %d\n", chip->ecc.strength);
+       pr_info("********** chip->ecc.size is %d\n", chip->ecc.size);
+       pr_info("********** chip->ecc.algo is %d\n", chip->ecc.algo);
+
        return 0;
 }

and I get:

nand: ********** chip->ecc.engine_type is 3
nand: ********** chip->ecc.strength is 0
nand: ********** chip->ecc.size is 0
nand: ********** chip->ecc.algo is 0
nand: device found, Manufacturer ID: 0xec, Chip ID: 0xaa
nand: Samsung NAND 256MiB 1,8V 8-bit
nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/mxc_nand.c:1391
mxc_nand_command+0x22c/0x280
Unimplemented command (cmd=0)
Modules linked in:

If I checkout commit  d7157ff49a ("mtd: rawnand: Use the ECC framework
user input parsing bits") in today's linux-next and revert it, then
the driver probes fine and prints:

nand: ******** ecc.engine_type = 3
nand: ******** chip->ecc.strength = 0
nand: ******** chip->ecc.size = 512

Thanks

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 17:01   ` Miquel Raynal
  2020-10-16 17:37     ` Fabio Estevam
@ 2020-10-16 19:18     ` Fabio Estevam
  2020-10-16 21:05       ` Miquel Raynal
  1 sibling, 1 reply; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 19:18 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 2:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Software ECC is used, is it expected?

I noticed the issue with your patch: host->pdata.hw_ecc cannot be
called within attach() because pdata is not initialized at this point.

Instead of using pdata, we can retrieve the "nand-ecc-mode" string
from the device tree.

I will submit the correct fix.

Thanks

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 19:18     ` Fabio Estevam
@ 2020-10-16 21:05       ` Miquel Raynal
  2020-10-16 21:32         ` Fabio Estevam
  0 siblings, 1 reply; 12+ messages in thread
From: Miquel Raynal @ 2020-10-16 21:05 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Fabio,

Fabio Estevam <festevam@gmail.com> wrote on Fri, 16 Oct 2020 16:18:15
-0300:

> Hi Miquel,
> 
> On Fri, Oct 16, 2020 at 2:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Software ECC is used, is it expected?  
> 
> I noticed the issue with your patch: host->pdata.hw_ecc cannot be
> called within attach() because pdata is not initialized at this point.t->

Nice catch! But I don't get why host->pdata.hw_ecc would not be
accessible from the attach hook. host->pdata is populated in the probe
function, way before nand_scan(), where ->attach() is called. So for me
host->pdata.hw_ecc should be accessible from ->attach().

> Instead of using pdata, we can retrieve the "nand-ecc-mode" string
> from the device tree.

Please don't do that! The DT parsing should be centralized in the core.

However, if you don't need this pdata entry you can get rid of it
entirely. In theory, if the user set the nand-ecc-mode property, then
chip->ecc.engine_type should already be set to the appropriate value
when entering ->attach(). Can you please check its value? It should
have been updated by rawnand_dt_init().

Thanks,
Miquèl

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 21:05       ` Miquel Raynal
@ 2020-10-16 21:32         ` Fabio Estevam
  2020-10-17 18:17           ` Miquel Raynal
  0 siblings, 1 reply; 12+ messages in thread
From: Fabio Estevam @ 2020-10-16 21:32 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Miquel,

On Fri, Oct 16, 2020 at 6:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Nice catch! But I don't get why host->pdata.hw_ecc would not be
> accessible from the attach hook. host->pdata is populated in the probe
> function, way before nand_scan(), where ->attach() is called. So for me
> host->pdata.hw_ecc should be accessible from ->attach().

Yes, now I understand it. pdata is only populated for non-dt platforms.

On 5.10-rc1 the non-dt imx users are gone, so we can get rid of pdata
on a separate patch.

> > Instead of using pdata, we can retrieve the "nand-ecc-mode" string
> > from the device tree.
>
> Please don't do that! The DT parsing should be centralized in the core.
>
> However, if you don't need this pdata entry you can get rid of it
> entirely. In theory, if the user set the nand-ecc-mode property, then
> chip->ecc.engine_type should already be set to the appropriate value
> when entering ->attach(). Can you please check its value? It should
> have been updated by rawnand_dt_init().

You are right. I have sent a v2, which lets the core determine the engine type.

Thanks,

Fabio Estevam

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place
  2020-10-16 21:32         ` Fabio Estevam
@ 2020-10-17 18:17           ` Miquel Raynal
  0 siblings, 0 replies; 12+ messages in thread
From: Miquel Raynal @ 2020-10-17 18:17 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Han Xu, Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
	Sascha Hauer, linux-mtd, Lucas Stach

Hi Fabio,

Fabio Estevam <festevam@gmail.com> wrote on Fri, 16 Oct 2020 18:32:33
-0300:

> Hi Miquel,
> 
> On Fri, Oct 16, 2020 at 6:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Nice catch! But I don't get why host->pdata.hw_ecc would not be
> > accessible from the attach hook. host->pdata is populated in the probe
> > function, way before nand_scan(), where ->attach() is called. So for me
> > host->pdata.hw_ecc should be accessible from ->attach().  
> 
> Yes, now I understand it. pdata is only populated for non-dt platforms.
> 
> On 5.10-rc1 the non-dt imx users are gone, so we can get rid of pdata
> on a separate patch.

Nice!

> 
> > > Instead of using pdata, we can retrieve the "nand-ecc-mode" string
> > > from the device tree.  
> >
> > Please don't do that! The DT parsing should be centralized in the core.
> >
> > However, if you don't need this pdata entry you can get rid of it
> > entirely. In theory, if the user set the nand-ecc-mode property, then
> > chip->ecc.engine_type should already be set to the appropriate value
> > when entering ->attach(). Can you please check its value? It should
> > have been updated by rawnand_dt_init().  
> 
> You are right. I have sent a v2, which lets the core determine the engine type.

The patch looks good to me!

Thanks,
Miquèl

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-10-17 18:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16  7:49 [PATCH] mtd: rawnand: mxc: Move the ECC engine initialization to the right place Miquel Raynal
2020-10-16 11:32 ` Fabio Estevam
2020-10-16 11:45   ` Fabio Estevam
2020-10-16 12:05     ` Miquel Raynal
2020-10-16 12:11       ` Fabio Estevam
2020-10-16 13:53 ` Sascha Hauer
2020-10-16 17:01   ` Miquel Raynal
2020-10-16 17:37     ` Fabio Estevam
2020-10-16 19:18     ` Fabio Estevam
2020-10-16 21:05       ` Miquel Raynal
2020-10-16 21:32         ` Fabio Estevam
2020-10-17 18:17           ` Miquel Raynal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).