LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
@ 2019-07-16  5:30 Naga Sureshkumar Relli
  2019-07-16  7:31 ` Boris Brezillon
  0 siblings, 1 reply; 9+ messages in thread
From: Naga Sureshkumar Relli @ 2019-07-16  5:30 UTC (permalink / raw)
  To: miquel.raynal, bbrezillon
  Cc: richard, dwmw2, computersforpeace, marek.vasut, vigneshr,
	yamada.masahiro, linux-mtd, linux-kernel, michal.simek, svemula,
	nagasuresh12, Naga Sureshkumar Relli

Add check before assigning chip->ecc.read_page() and chip->ecc.write_page()

Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
---
Changes in v18
 - None
---
 drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index cbd4f09ac178..565f2696c747 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
 		chip->ecc.size = 512;
 		chip->ecc.strength = chip->base.eccreq.strength;
 		chip->ecc.algo = NAND_ECC_BCH;
-		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
-		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
+		if (!chip->ecc.read_page)
+			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
+
+		if (!chip->ecc.write_page)
+			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
 
 		if (ondie == MICRON_ON_DIE_MANDATORY) {
 			chip->ecc.read_page_raw = nand_read_page_raw_notsupp;
-- 
2.17.1


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

* Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-16  5:30 [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page() Naga Sureshkumar Relli
@ 2019-07-16  7:31 ` Boris Brezillon
  2019-07-16  7:44   ` Boris Brezillon
  0 siblings, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2019-07-16  7:31 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	michal.simek, svemula, nagasuresh12

On Mon, 15 Jul 2019 23:30:51 -0600
Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:

> Add check before assigning chip->ecc.read_page() and chip->ecc.write_page()
> 
> Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> ---
> Changes in v18
>  - None
> ---
>  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index cbd4f09ac178..565f2696c747 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
>  		chip->ecc.size = 512;
>  		chip->ecc.strength = chip->base.eccreq.strength;
>  		chip->ecc.algo = NAND_ECC_BCH;
> -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> +		if (!chip->ecc.read_page)
> +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> +
> +		if (!chip->ecc.write_page)
> +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
>  

Seriously?! I told you this was inappropriate and you keep sending this
patch. So let's make it clear:

Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>

Fix your controller driver instead of adding hacks to the Micron logic!

>  		if (ondie == MICRON_ON_DIE_MANDATORY) {
>  			chip->ecc.read_page_raw = nand_read_page_raw_notsupp;


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

* Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-16  7:31 ` Boris Brezillon
@ 2019-07-16  7:44   ` Boris Brezillon
  2019-07-17  5:33     ` Naga Sureshkumar Relli
  0 siblings, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2019-07-16  7:44 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	michal.simek, svemula, nagasuresh12

On Tue, 16 Jul 2019 09:31:37 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Mon, 15 Jul 2019 23:30:51 -0600
> Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:
> 
> > Add check before assigning chip->ecc.read_page() and chip->ecc.write_page()
> > 
> > Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> > ---
> > Changes in v18
> >  - None
> > ---
> >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> > index cbd4f09ac178..565f2696c747 100644
> > --- a/drivers/mtd/nand/raw/nand_micron.c
> > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> >  		chip->ecc.size = 512;
> >  		chip->ecc.strength = chip->base.eccreq.strength;
> >  		chip->ecc.algo = NAND_ECC_BCH;
> > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > +		if (!chip->ecc.read_page)
> > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > +
> > +		if (!chip->ecc.write_page)
> > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> >    
> 
> Seriously?! I told you this was inappropriate and you keep sending this
> patch. So let's make it clear:
> 
> Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Fix your controller driver instead of adding hacks to the Micron logic!

Not even going to review the other patch: if you have to do that, that
means the driver is broken. On a side note, this patch series is still
not threaded as it should be and it's a v18 for a damn NAND controller
driver! Sorry but you reached the limit of my patience. Please find
someone to help you with that task.

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

* RE: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-16  7:44   ` Boris Brezillon
@ 2019-07-17  5:33     ` Naga Sureshkumar Relli
  2019-07-17  7:55       ` Boris Brezillon
  0 siblings, 1 reply; 9+ messages in thread
From: Naga Sureshkumar Relli @ 2019-07-17  5:33 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Tuesday, July 16, 2019 1:15 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; nagasuresh12@gmail.com
> Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> driver's read_page()/write_page()
> 
> On Tue, 16 Jul 2019 09:31:37 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Mon, 15 Jul 2019 23:30:51 -0600
> > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:
> >
> > > Add check before assigning chip->ecc.read_page() and
> > > chip->ecc.write_page()
> > >
> > > Signed-off-by: Naga Sureshkumar Relli
> > > <naga.sureshkumar.relli@xilinx.com>
> > > ---
> > > Changes in v18
> > >  - None
> > > ---
> > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > b/drivers/mtd/nand/raw/nand_micron.c
> > > index cbd4f09ac178..565f2696c747 100644
> > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > >  		chip->ecc.size = 512;
> > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > +		if (!chip->ecc.read_page)
> > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > +
> > > +		if (!chip->ecc.write_page)
> > > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > >
> >
> > Seriously?! I told you this was inappropriate and you keep sending
> > this patch. So let's make it clear:
> >
> > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > Fix your controller driver instead of adding hacks to the Micron logic!
> 
> Not even going to review the other patch: if you have to do that, that means the driver is
> broken. On a side note, this patch series is still not threaded as it should be and it's a v18 for a
> damn NAND controller driver! Sorry but you reached the limit of my patience. Please find
> someone to help you with that task.
My intention is not to resend this 1/2 again. Sorry for that.
We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
And there we didn't conclude that raw_read()/writes(). 
So I thought that, will send updated driver along with this patch, then will get more information about
The issue on the latest driver review.
There is nothing like keep on sending this patch, As you people are experts in the driver review, 
if this patch is a hack, then we will definitely fix that in controller driver. I will find a way to do that.

But in this flow of patch sending, if the work I did hurts you, then I am really sorry for that.
Will fix this issue in the controller driver and will send the updated one.
Could you please let me know if this is OK.

I will send the series as threaded one from next time onwards.

Thanks,
pcieNaga Sureshkumar Relli

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

* Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-17  5:33     ` Naga Sureshkumar Relli
@ 2019-07-17  7:55       ` Boris Brezillon
  2019-07-17  8:21         ` Boris Brezillon
  2019-07-17  9:07         ` Naga Sureshkumar Relli
  0 siblings, 2 replies; 9+ messages in thread
From: Boris Brezillon @ 2019-07-17  7:55 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12

On Wed, 17 Jul 2019 05:33:35 +0000
Naga Sureshkumar Relli <nagasure@xilinx.com> wrote:

> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Sent: Tuesday, July 16, 2019 1:15 PM
> > To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> > dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> > vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> > kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> > <svemula@xilinx.com>; nagasuresh12@gmail.com
> > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> > driver's read_page()/write_page()
> > 
> > On Tue, 16 Jul 2019 09:31:37 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Mon, 15 Jul 2019 23:30:51 -0600
> > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:
> > >  
> > > > Add check before assigning chip->ecc.read_page() and
> > > > chip->ecc.write_page()
> > > >
> > > > Signed-off-by: Naga Sureshkumar Relli
> > > > <naga.sureshkumar.relli@xilinx.com>
> > > > ---
> > > > Changes in v18
> > > >  - None
> > > > ---
> > > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > index cbd4f09ac178..565f2696c747 100644
> > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > >  		chip->ecc.size = 512;
> > > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > +		if (!chip->ecc.read_page)
> > > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > +
> > > > +		if (!chip->ecc.write_page)
> > > > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > >  
> > >
> > > Seriously?! I told you this was inappropriate and you keep sending
> > > this patch. So let's make it clear:
> > >
> > > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > >
> > > Fix your controller driver instead of adding hacks to the Micron logic!  
> > 
> > Not even going to review the other patch: if you have to do that, that means the driver is
> > broken. On a side note, this patch series is still not threaded as it should be and it's a v18 for a
> > damn NAND controller driver! Sorry but you reached the limit of my patience. Please find
> > someone to help you with that task.  
> My intention is not to resend this 1/2 again. Sorry for that.
> We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
> And there we didn't conclude that raw_read()/writes().

Yes, looks like I never replied to that one, but I think my previous
explanation were clear enough to not argue on that aspect any longer/

> So I thought that, will send updated driver along with this patch, then will get more information about
> The issue on the latest driver review.

More on that topic. I don't think you ever tested on-die ECC on a
Micron NAND, otherwise you would have noticed that your solution
completely bypasses the on-die ECC logic (and this will clearly break
existing on-die ECC users). See, that's what I'm complaining about,
Looks like you don't really understand what you're doing.

> There is nothing like keep on sending this patch, As you people are experts in the driver review, 
> if this patch is a hack, then we will definitely fix that in controller driver. I will find a way to do that.
> 
> But in this flow of patch sending, if the work I did hurts you, then I am really sorry for that.

I'm not offended, just tired going through the same driver over and
over again, reporting things that are wrong/inappropriate to then
realize you only addressed of a tiny portion of it in the following
version. My last reviews were rather incomplete because of that, and
now I'm giving up.

> Will fix this issue in the controller driver and will send the updated one.

How? You say you'll fix the issue but I'm not even sure you understand
what the issue is? Clearly, the patch you've posted doesn't fix
anything, it's just papering over the fact that your controller driver
is not supporting raw accesses (or at least, not supporting it
properly).

Have you even looked at the datasheet you pointed to in patch 2 [1]?
Just went through it, and found a field that's supposed to control the
ECC engine activation: ecc_memcfg.ecc_mode. I don't see anything
changing that field in your code, so I guess raw accesses are actually
not really happening with the ECC engine disabled... 

> Could you please let me know if this is OK.

You can send a new version, I'm just saying I won't spend time
reviewing it.

> 
> I will send the series as threaded one from next time onwards.
> 
> Thanks,
> pcieNaga Sureshkumar Relli

[1]http://infocenter.arm.com/help/topic/com.arm.doc.ddi0380g/DDI0380G_smc_pl350_series_r2p1_trm.pdf

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

* Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-17  7:55       ` Boris Brezillon
@ 2019-07-17  8:21         ` Boris Brezillon
  2019-07-17  9:04           ` Boris Brezillon
  2019-07-17  9:07         ` Naga Sureshkumar Relli
  1 sibling, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2019-07-17  8:21 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12

On Wed, 17 Jul 2019 09:55:25 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Wed, 17 Jul 2019 05:33:35 +0000
> Naga Sureshkumar Relli <nagasure@xilinx.com> wrote:
> 
> > Hi Boris,
> >   
> > > -----Original Message-----
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > > Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> > > dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> > > vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> > > kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> > > <svemula@xilinx.com>; nagasuresh12@gmail.com
> > > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> > > driver's read_page()/write_page()
> > > 
> > > On Tue, 16 Jul 2019 09:31:37 +0200
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > >     
> > > > On Mon, 15 Jul 2019 23:30:51 -0600
> > > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:
> > > >    
> > > > > Add check before assigning chip->ecc.read_page() and
> > > > > chip->ecc.write_page()
> > > > >
> > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > <naga.sureshkumar.relli@xilinx.com>
> > > > > ---
> > > > > Changes in v18
> > > > >  - None
> > > > > ---
> > > > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > > index cbd4f09ac178..565f2696c747 100644
> > > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > > >  		chip->ecc.size = 512;
> > > > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > > > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > +		if (!chip->ecc.read_page)
> > > > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > +
> > > > > +		if (!chip->ecc.write_page)
> > > > > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > >    
> > > >
> > > > Seriously?! I told you this was inappropriate and you keep sending
> > > > this patch. So let's make it clear:
> > > >
> > > > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > >
> > > > Fix your controller driver instead of adding hacks to the Micron logic!    
> > > 
> > > Not even going to review the other patch: if you have to do that, that means the driver is
> > > broken. On a side note, this patch series is still not threaded as it should be and it's a v18 for a
> > > damn NAND controller driver! Sorry but you reached the limit of my patience. Please find
> > > someone to help you with that task.    
> > My intention is not to resend this 1/2 again. Sorry for that.
> > We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
> > And there we didn't conclude that raw_read()/writes().  
> 
> Yes, looks like I never replied to that one, but I think my previous
> explanation were clear enough to not argue on that aspect any longer/
> 
> > So I thought that, will send updated driver along with this patch, then will get more information about
> > The issue on the latest driver review.  
> 
> More on that topic. I don't think you ever tested on-die ECC on a
> Micron NAND, otherwise you would have noticed that your solution
> completely bypasses the on-die ECC logic (and this will clearly break
> existing on-die ECC users). See, that's what I'm complaining about,
> Looks like you don't really understand what you're doing.
> 
> > There is nothing like keep on sending this patch, As you people are experts in the driver review, 
> > if this patch is a hack, then we will definitely fix that in controller driver. I will find a way to do that.
> > 
> > But in this flow of patch sending, if the work I did hurts you, then I am really sorry for that.  
> 
> I'm not offended, just tired going through the same driver over and
> over again, reporting things that are wrong/inappropriate to then
> realize you only addressed of a tiny portion of it in the following
> version. My last reviews were rather incomplete because of that, and
> now I'm giving up.
> 
> > Will fix this issue in the controller driver and will send the updated one.  
> 
> How? You say you'll fix the issue but I'm not even sure you understand
> what the issue is? Clearly, the patch you've posted doesn't fix
> anything, it's just papering over the fact that your controller driver
> is not supporting raw accesses (or at least, not supporting it
> properly).
> 
> Have you even looked at the datasheet you pointed to in patch 2 [1]?
> Just went through it, and found a field that's supposed to control the
> ECC engine activation: ecc_memcfg.ecc_mode. I don't see anything
> changing that field in your code, so I guess raw accesses are actually
> not really happening with the ECC engine disabled... 

Looks like I was wrong about that part, you seem to call
pl353_smc_set_ecc_mode(), just not in the right place (this should be
done in the read/write_page_raw()).

Also noticed the ecc_memcfg.ecc_extra_block field. Have you tried
setting it to 0 for your raw accesses to get rid of the extra
PL353_NAND_LAST_TRANSFER_LENGTH transfer?

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

* Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-17  8:21         ` Boris Brezillon
@ 2019-07-17  9:04           ` Boris Brezillon
  2019-07-18 12:32             ` Naga Sureshkumar Relli
  0 siblings, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2019-07-17  9:04 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12

On Wed, 17 Jul 2019 10:21:56 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Wed, 17 Jul 2019 09:55:25 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Wed, 17 Jul 2019 05:33:35 +0000
> > Naga Sureshkumar Relli <nagasure@xilinx.com> wrote:
> >   
> > > Hi Boris,
> > >     
> > > > -----Original Message-----
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > > To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > > > Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> > > > dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> > > > vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> > > > kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> > > > <svemula@xilinx.com>; nagasuresh12@gmail.com
> > > > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> > > > driver's read_page()/write_page()
> > > > 
> > > > On Tue, 16 Jul 2019 09:31:37 +0200
> > > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > >       
> > > > > On Mon, 15 Jul 2019 23:30:51 -0600
> > > > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote:
> > > > >      
> > > > > > Add check before assigning chip->ecc.read_page() and
> > > > > > chip->ecc.write_page()
> > > > > >
> > > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > > <naga.sureshkumar.relli@xilinx.com>
> > > > > > ---
> > > > > > Changes in v18
> > > > > >  - None
> > > > > > ---
> > > > > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > index cbd4f09ac178..565f2696c747 100644
> > > > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > > > >  		chip->ecc.size = 512;
> > > > > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > > > > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > > > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > > +		if (!chip->ecc.read_page)
> > > > > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > +
> > > > > > +		if (!chip->ecc.write_page)
> > > > > > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > >      
> > > > >
> > > > > Seriously?! I told you this was inappropriate and you keep sending
> > > > > this patch. So let's make it clear:
> > > > >
> > > > > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > >
> > > > > Fix your controller driver instead of adding hacks to the Micron logic!      
> > > > 
> > > > Not even going to review the other patch: if you have to do that, that means the driver is
> > > > broken. On a side note, this patch series is still not threaded as it should be and it's a v18 for a
> > > > damn NAND controller driver! Sorry but you reached the limit of my patience. Please find
> > > > someone to help you with that task.      
> > > My intention is not to resend this 1/2 again. Sorry for that.
> > > We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
> > > And there we didn't conclude that raw_read()/writes().    
> > 
> > Yes, looks like I never replied to that one, but I think my previous
> > explanation were clear enough to not argue on that aspect any longer/
> >   
> > > So I thought that, will send updated driver along with this patch, then will get more information about
> > > The issue on the latest driver review.    
> > 
> > More on that topic. I don't think you ever tested on-die ECC on a
> > Micron NAND, otherwise you would have noticed that your solution
> > completely bypasses the on-die ECC logic (and this will clearly break
> > existing on-die ECC users). See, that's what I'm complaining about,
> > Looks like you don't really understand what you're doing.
> >   
> > > There is nothing like keep on sending this patch, As you people are experts in the driver review, 
> > > if this patch is a hack, then we will definitely fix that in controller driver. I will find a way to do that.
> > > 
> > > But in this flow of patch sending, if the work I did hurts you, then I am really sorry for that.    
> > 
> > I'm not offended, just tired going through the same driver over and
> > over again, reporting things that are wrong/inappropriate to then
> > realize you only addressed of a tiny portion of it in the following
> > version. My last reviews were rather incomplete because of that, and
> > now I'm giving up.
> >   
> > > Will fix this issue in the controller driver and will send the updated one.    
> > 
> > How? You say you'll fix the issue but I'm not even sure you understand
> > what the issue is? Clearly, the patch you've posted doesn't fix
> > anything, it's just papering over the fact that your controller driver
> > is not supporting raw accesses (or at least, not supporting it
> > properly).
> > 
> > Have you even looked at the datasheet you pointed to in patch 2 [1]?
> > Just went through it, and found a field that's supposed to control the
> > ECC engine activation: ecc_memcfg.ecc_mode. I don't see anything
> > changing that field in your code, so I guess raw accesses are actually
> > not really happening with the ECC engine disabled...   
> 
> Looks like I was wrong about that part, you seem to call
> pl353_smc_set_ecc_mode(), just not in the right place (this should be
> done in the read/write_page_raw()).

I'm wrong again. ECC should be in BYPASS mode by default and you should
enable it when entering pl353_nand_{read,write}_page() and disable it
(put it back to BYPASS mode) when leaving those functions. This way,
you don't even have to implement your own raw accessors and can use
nand_{read,write}_page_raw() instead.

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

* RE: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-17  7:55       ` Boris Brezillon
  2019-07-17  8:21         ` Boris Brezillon
@ 2019-07-17  9:07         ` Naga Sureshkumar Relli
  1 sibling, 0 replies; 9+ messages in thread
From: Naga Sureshkumar Relli @ 2019-07-17  9:07 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12



> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Wednesday, July 17, 2019 1:25 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; nagasuresh12@gmail.com
> Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> driver's read_page()/write_page()
> 
> On Wed, 17 Jul 2019 05:33:35 +0000
> Naga Sureshkumar Relli <nagasure@xilinx.com> wrote:
> 
> > Hi Boris,
> >
> > > -----Original Message-----
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > > Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org;
> > > richard@nod.at; dwmw2@infradead.org; computersforpeace@gmail.com;
> > > marek.vasut@gmail.com; vigneshr@ti.com;
> > > yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> > > kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth
> > > Vemula <svemula@xilinx.com>; nagasuresh12@gmail.com
> > > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not
> > > over write driver's read_page()/write_page()
> > >
> > > On Tue, 16 Jul 2019 09:31:37 +0200
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > >
> > > > On Mon, 15 Jul 2019 23:30:51 -0600 Naga Sureshkumar Relli
> > > > <naga.sureshkumar.relli@xilinx.com> wrote:
> > > >
> > > > > Add check before assigning chip->ecc.read_page() and
> > > > > chip->ecc.write_page()
> > > > >
> > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > <naga.sureshkumar.relli@xilinx.com>
> > > > > ---
> > > > > Changes in v18
> > > > >  - None
> > > > > ---
> > > > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > > index cbd4f09ac178..565f2696c747 100644
> > > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > > >  		chip->ecc.size = 512;
> > > > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > > > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > +		if (!chip->ecc.read_page)
> > > > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > +
> > > > > +		if (!chip->ecc.write_page)
> > > > > +			chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > >
> > > >
> > > > Seriously?! I told you this was inappropriate and you keep sending
> > > > this patch. So let's make it clear:
> > > >
> > > > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > >
> > > > Fix your controller driver instead of adding hacks to the Micron logic!
> > >
> > > Not even going to review the other patch: if you have to do that,
> > > that means the driver is broken. On a side note, this patch series
> > > is still not threaded as it should be and it's a v18 for a damn NAND
> > > controller driver! Sorry but you reached the limit of my patience. Please find someone to
> help you with that task.
> > My intention is not to resend this 1/2 again. Sorry for that.
> > We already had some discussion on [v17 1/2],
> > https://lkml.org/lkml/2019/6/26/430
> > And there we didn't conclude that raw_read()/writes().
> 
> Yes, looks like I never replied to that one, but I think my previous explanation were clear
> enough to not argue on that aspect any longer/
You replied on that. But I thought that you have some more inputs to give.
Ok understood thanks.
> 
> > So I thought that, will send updated driver along with this patch,
> > then will get more information about The issue on the latest driver review.
> 
> More on that topic. I don't think you ever tested on-die ECC on a Micron NAND, otherwise
> you would have noticed that your solution completely bypasses the on-die ECC logic (and this
> will clearly break existing on-die ECC users). See, that's what I'm complaining about, Looks
> like you don't really understand what you're doing.
We tested it with pl353_read/write_page_raw(). Hence no information of bit flips.
As I said, will fix the driver to use micron _read_page_ondie().
> 
> > There is nothing like keep on sending this patch, As you people are
> > experts in the driver review, if this patch is a hack, then we will definitely fix that in
> controller driver. I will find a way to do that.
> >
> > But in this flow of patch sending, if the work I did hurts you, then I am really sorry for that.
> 
> I'm not offended, just tired going through the same driver over and over again, reporting
> things that are wrong/inappropriate to then realize you only addressed of a tiny portion of it in
> the following version. My last reviews were rather incomplete because of that, and now I'm
> giving up.
> 
> > Will fix this issue in the controller driver and will send the updated one.
> 
> How? You say you'll fix the issue but I'm not even sure you understand what the issue is?
> Clearly, the patch you've posted doesn't fix anything, it's just papering over the fact that your
> controller driver is not supporting raw accesses (or at least, not supporting it properly).
> 
> Have you even looked at the datasheet you pointed to in patch 2 [1]?
> Just went through it, and found a field that's supposed to control the ECC engine activation:
> ecc_memcfg.ecc_mode. I don't see anything changing that field in your code, so I guess raw
> accesses are actually not really happening with the ECC engine disabled...
It is happening, in the drivers ecc_init(), if the ecc mode is ON_DIE, then 
We are calling pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_BYPASS).
This will internally disables the HW-ECC.
> 
> > Could you please let me know if this is OK.
> 
> You can send a new version, I'm just saying I won't spend time reviewing it.
> 
> >
> > I will send the series as threaded one from next time onwards.
> >
> > Thanks,
> > pcieNaga Sureshkumar Relli
> 
> [1]http://infocenter.arm.com/help/topic/com.arm.doc.ddi0380g/DDI0380G_smc_pl350_seri
> es_r2p1_trm.pdf

Regards,
Naga Sureshkumar Relli

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

* RE: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()
  2019-07-17  9:04           ` Boris Brezillon
@ 2019-07-18 12:32             ` Naga Sureshkumar Relli
  0 siblings, 0 replies; 9+ messages in thread
From: Naga Sureshkumar Relli @ 2019-07-18 12:32 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: miquel.raynal, bbrezillon, richard, dwmw2, computersforpeace,
	marek.vasut, vigneshr, yamada.masahiro, linux-mtd, linux-kernel,
	Michal Simek, Srikanth Vemula, nagasuresh12

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Wednesday, July 17, 2019 2:34 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com;
> vigneshr@ti.com; yamada.masahiro@socionext.com; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; nagasuresh12@gmail.com
> Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> driver's read_page()/write_page()
> 
> On Wed, 17 Jul 2019 10:21:56 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Wed, 17 Jul 2019 09:55:25 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >
> > > On Wed, 17 Jul 2019 05:33:35 +0000
> > > Naga Sureshkumar Relli <nagasure@xilinx.com> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > > -----Original Message-----
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > > > To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > > > > Cc: miquel.raynal@bootlin.com; bbrezillon@kernel.org;
> > > > > richard@nod.at; dwmw2@infradead.org;
> > > > > computersforpeace@gmail.com; marek.vasut@gmail.com;
> > > > > vigneshr@ti.com; yamada.masahiro@socionext.com;
> > > > > linux-mtd@lists.infradead.org; linux- kernel@vger.kernel.org;
> > > > > Michal Simek <michals@xilinx.com>; Srikanth Vemula
> > > > > <svemula@xilinx.com>; nagasuresh12@gmail.com
> > > > > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do
> > > > > not over write driver's read_page()/write_page()
> > > > >
> > > > > On Tue, 16 Jul 2019 09:31:37 +0200 Boris Brezillon
> > > > > <boris.brezillon@collabora.com> wrote:
> > > > >
> > > > > > On Mon, 15 Jul 2019 23:30:51 -0600 Naga Sureshkumar Relli
> > > > > > <naga.sureshkumar.relli@xilinx.com> wrote:
> > > > > >
> > > > > > > Add check before assigning chip->ecc.read_page() and
> > > > > > > chip->ecc.write_page()
> > > > > > >
> > > > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > > > <naga.sureshkumar.relli@xilinx.com>
> > > > > > > ---
> > > > > > > Changes in v18
> > > > > > >  - None
> > > > > > > ---
> > > > > > >  drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > index cbd4f09ac178..565f2696c747 100644
> > > > > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > > > > >  		chip->ecc.size = 512;
> > > > > > >  		chip->ecc.strength = chip->base.eccreq.strength;
> > > > > > >  		chip->ecc.algo = NAND_ECC_BCH;
> > > > > > > -		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > > -		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > > > +		if (!chip->ecc.read_page)
> > > > > > > +			chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > > +
> > > > > > > +		if (!chip->ecc.write_page)
> > > > > > > +			chip->ecc.write_page =
> > > > > > > +micron_nand_write_page_on_die_ecc;
> > > > > > >
> > > > > >
> > > > > > Seriously?! I told you this was inappropriate and you keep
> > > > > > sending this patch. So let's make it clear:
> > > > > >
> > > > > > Nacked-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > >
> > > > > > Fix your controller driver instead of adding hacks to the Micron logic!
> > > > >
> > > > > Not even going to review the other patch: if you have to do
> > > > > that, that means the driver is broken. On a side note, this
> > > > > patch series is still not threaded as it should be and it's a v18 for a damn NAND
> controller driver! Sorry but you reached the limit of my patience. Please find
> > > > > someone to help you with that task.
> > > > My intention is not to resend this 1/2 again. Sorry for that.
> > > > We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
> > > > And there we didn't conclude that raw_read()/writes().
> > >
> > > Yes, looks like I never replied to that one, but I think my previous
> > > explanation were clear enough to not argue on that aspect any
> > > longer/
> > >
> > > > So I thought that, will send updated driver along with this patch, then will get more
> information about
> > > > The issue on the latest driver review.
> > >
> > > More on that topic. I don't think you ever tested on-die ECC on a
> > > Micron NAND, otherwise you would have noticed that your solution
> > > completely bypasses the on-die ECC logic (and this will clearly
> > > break existing on-die ECC users). See, that's what I'm complaining
> > > about, Looks like you don't really understand what you're doing.
> > >
> > > > There is nothing like keep on sending this patch, As you people
> > > > are experts in the driver review, if this patch is a hack, then we will definitely fix that in
> controller driver. I will find a way to do that.
> > > >
> > > > But in this flow of patch sending, if the work I did hurts you, then I am really sorry for
> that.
> > >
> > > I'm not offended, just tired going through the same driver over and
> > > over again, reporting things that are wrong/inappropriate to then
> > > realize you only addressed of a tiny portion of it in the following
> > > version. My last reviews were rather incomplete because of that, and
> > > now I'm giving up.
> > >
> > > > Will fix this issue in the controller driver and will send the updated one.
> > >
> > > How? You say you'll fix the issue but I'm not even sure you
> > > understand what the issue is? Clearly, the patch you've posted
> > > doesn't fix anything, it's just papering over the fact that your
> > > controller driver is not supporting raw accesses (or at least, not
> > > supporting it properly).
> > >
> > > Have you even looked at the datasheet you pointed to in patch 2 [1]?
> > > Just went through it, and found a field that's supposed to control
> > > the ECC engine activation: ecc_memcfg.ecc_mode. I don't see anything
> > > changing that field in your code, so I guess raw accesses are actually
> > > not really happening with the ECC engine disabled...
> >
> > Looks like I was wrong about that part, you seem to call
> > pl353_smc_set_ecc_mode(), just not in the right place (this should be
> > done in the read/write_page_raw()).
> 
> I'm wrong again. ECC should be in BYPASS mode by default and you should enable it when
> entering pl353_nand_{read,write}_page() and disable it (put it back to BYPASS mode) when
> leaving those functions. This way, you don't even have to implement your own raw accessors
> and can use
> nand_{read,write}_page_raw() instead.
Yes, ECC should be in BYPASS mode, and we are already doing that in pl353_ecc_init(),
I will move that to page read/writes.

The PL353 driver is not configuring properly the third row address cycle, i.e. when chip->options is set
With NAND_ROW_ADDR_3, which says third row address cycle is needed.
In the drivers ->exec_op() method, it is just setting three row address cycles but not writing
The row address properly during NAND_OP_ADDR_INSTR.
I fixed that and now as you said, without pl353_nand_read_page_raw() and pl353_nand_write_page_raw()
The on-die page read/writes are working.

I have tested ubifs and I see no issues with that.

Thanks,
Naga Sureshkumar Relli



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

end of thread, other threads:[~2019-07-18 12:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16  5:30 [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page() Naga Sureshkumar Relli
2019-07-16  7:31 ` Boris Brezillon
2019-07-16  7:44   ` Boris Brezillon
2019-07-17  5:33     ` Naga Sureshkumar Relli
2019-07-17  7:55       ` Boris Brezillon
2019-07-17  8:21         ` Boris Brezillon
2019-07-17  9:04           ` Boris Brezillon
2019-07-18 12:32             ` Naga Sureshkumar Relli
2019-07-17  9:07         ` Naga Sureshkumar Relli

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