Re: [PATCH] Revert "mtd: davinci-nand: disable subpage write for keystone-nand"

[Boris Brezillon]

On Thu, 30 Mar 2017 15:38:54 +0530
Sekhar Nori <nsekhar@xxxxxx> wrote:

> On Thursday 30 March 2017 03:30 PM, Boris Brezillon wrote:
> > On Thu, 30 Mar 2017 13:13:58 +0530
> > Sekhar Nori <nsekhar@xxxxxx> wrote:
> >   
> >> Hi Boris,
> >>
> >> On Wednesday 29 March 2017 08:33 PM, Boris Brezillon wrote:  
> >>> On Mon, 27 Mar 2017 15:56:55 +0530
> >>> Sekhar Nori <nsekhar@xxxxxx> wrote:
> >>>     
> >>>> This partly reverts commit 28c015a9daabe4ed3aeb0ccf669a3f1c2b8b81d5.
> >>>>
> >>>> Since f6d7c1b5598b ("mtd: nand: davinci: Reinitialize the HW ECC
> >>>> engine in 4bit hwctl") using subpage and 4bit HWECC works.
> >>>>
> >>>> Revert the original workaround made for this. Tested with
> >>>> mtd_subpagetest and UBIFS on K2E, K2HK, K2L EVMs.
> >>>>
> >>>> The addition of keystone specific compatible to davinci_nand_of_match[]
> >>>> contained in original commit is retained.    
> >>>
> >>> Hm, it sounds like a bad idea to re-enable sub-page writes afterwards.
> >>> You might break existing systems using UBI, because UBI detects the
> >>> sub-page write feature and decides to place the VID header at a
> >>> different position depending on that.
> >>> To sum-up, you're likely to break users that have already flashed a UBI
> >>> image with the old driver.    
> >>
> >> Okay, I didn't realize that.
> >>  
> >>> If you want to re-enable sub-page writes, just switch back to the
> >>> "ti,davinci-nand" compatible.    
> >>
> >> How about a comment on top of the disable subpage write code explaining 
> >> that its not really needed anymore ? Something like this patch:
> >>
> >> ---8<---
> >> From: Sekhar Nori <nsekhar@xxxxxx>
> >> Date: Thu, 30 Mar 2017 13:08:30 +0530
> >> Subject: [PATCH] mtd: nand: davinci: add comment on NAND subpage write status
> >>  on keystone
> >>
> >> Add a comment clarifying that NAND subpage write on keystone works,
> >> but is not being enabled in the interest of backward compatibility.
> >>
> >> Signed-off-by: Sekhar Nori <nsekhar@xxxxxx>
> >> ---
> >>  drivers/mtd/nand/davinci_nand.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> >> index 27fa8b87cd5f..5bf8be1cfc8c 100644
> >> --- a/drivers/mtd/nand/davinci_nand.c
> >> +++ b/drivers/mtd/nand/davinci_nand.c
> >> @@ -581,6 +581,17 @@ static struct davinci_nand_pdata
> >>  			"ti,davinci-nand-use-bbt"))
> >>  			pdata->bbt_options = NAND_BBT_USE_FLASH;
> >>  
> >> +		/*
> >> +		 * Since kernel v4.8, this driver has been fixed to enable
> >> +		 * use of 4-bit hardware ECC with subpages and verified on
> >> +		 * TI's keystone EVMs (K2L, K2HK and K2E).
> >> +		 * However, in the interest of not breaking systems using
> >> +		 * existing UBI partitions, sub-page writes are not being
> >> +		 * (re)enabled. If you want to use subpage writes on Keystone
> >> +		 * platforms (i.e. do not have any existing UBI partitions),
> >> +		 * then remove the if() block below or pass "ti,davinci-nand"
> >> +		 * as the compatible in your device-tree file.  
> > 
> > Hm, I'm not sure we should suggest to remove these lines from the code.
> > Just suggest to use "ti,davinci-nand" compatible.  

Can you send a separate patch so that it appears on its own in
patchwork?