Revert "mtd: davinci-nand: disable subpage write for keystone-nand"

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@ti.com> 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@ti.com>
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@ti.com>
---
 drivers/mtd/nand/davinci_nand.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

On Thu, 30 Mar 2017 13:13:58 +0530
Sekhar Nori <nsekhar@ti.com> 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@ti.com> 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@ti.com>
> 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@ti.com>
> ---
>  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.
If you're okay with that, no need to resend, I can rework the comment
accordingly before applying the patch.

Another long term solution: if UBI is the only MTD user to be
incompatible when sub-page write is activated/deactivated, maybe we can
relax the checks in UBI to allow it to attach a partition where VID
header offset is at mtd->writesize instead of
mtd->writesize >> mtd->subpage_sft.

I'll let Richard comment on that solution though.

> +		 */
>  		if (of_device_is_compatible(pdev->dev.of_node,
>  					    "ti,keystone-nand")) {
>  			pdata->options |= NAND_NO_SUBPAGE_WRITE;

On Thursday 30 March 2017 03:30 PM, Boris Brezillon wrote:
> On Thu, 30 Mar 2017 13:13:58 +0530
> Sekhar Nori <nsekhar@ti.com> 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@ti.com> 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@ti.com>
>> 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@ti.com>
>> ---
>>  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.

Thats fine with me. Today, this if() block is the only place where
ti,keystone-nand is used. If in future it gets used for something else
(yet unknown), then the comment might start to get misleading. Thats why
just removing the code seemed like a safer option to me.

But the main intention was to document that subpage is not really broken
on K2 platform. So, what you suggest will do as well.

Thanks,
Sekhar

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

> On Thursday 30 March 2017 03:30 PM, Boris Brezillon wrote:
> > On Thu, 30 Mar 2017 13:13:58 +0530
> > Sekhar Nori <nsekhar@ti.com> 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@ti.com> 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@ti.com>
> >> 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@ti.com>
> >> ---
> >>  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?