[4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

  This is already accounted for in the subtraction for OOB, since the BBM
overlaps the first OOB dword. With this change, the driver picks the
same ECC strength as the vendor driver.

Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

Hi Samuel,

samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:

> This is already accounted for in the subtraction for OOB, since the BBM
> overlaps the first OOB dword. With this change, the driver picks the
> same ECC strength as the vendor driver.
> 
> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 1bddeb1be66f..1ecf2cee343b 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  		ecc->size = 1024;
>  		nsectors = mtd->writesize / ecc->size;
>  
> -		/* Reserve 2 bytes for the BBM */
> -		bytes = (mtd->oobsize - 2) / nsectors;
> +		bytes = mtd->oobsize / nsectors;

I'm sorry but I don't think we can make this work. This change would
break all existing users...

>  
>  		/* 4 non-ECC bytes are added before each ECC bytes section */
>  		bytes -= 4;


Thanks,
Miquèl
  

Hi Miquèl,

On 1/2/23 03:11, Miquel Raynal wrote:
> Hi Samuel,
> 
> samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:
> 
>> This is already accounted for in the subtraction for OOB, since the BBM
>> overlaps the first OOB dword. With this change, the driver picks the
>> same ECC strength as the vendor driver.
>>
>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>> index 1bddeb1be66f..1ecf2cee343b 100644
>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  		ecc->size = 1024;
>>  		nsectors = mtd->writesize / ecc->size;
>>  
>> -		/* Reserve 2 bytes for the BBM */
>> -		bytes = (mtd->oobsize - 2) / nsectors;
>> +		bytes = mtd->oobsize / nsectors;
> 
> I'm sorry but I don't think we can make this work. This change would
> break all existing users...

OK, it is not too much of an issue because I can manually specify the
ECC parameters in the devicetree. Do you think it makes sense to fix
this when adding new hardware variants/compatible strings?

Regards,
Samuel
  

Hi Samuel,

samuel@sholland.org wrote on Mon, 2 Jan 2023 09:59:29 -0600:

> Hi Miquèl,
> 
> On 1/2/23 03:11, Miquel Raynal wrote:
> > Hi Samuel,
> > 
> > samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:
> >   
> >> This is already accounted for in the subtraction for OOB, since the BBM
> >> overlaps the first OOB dword. With this change, the driver picks the
> >> same ECC strength as the vendor driver.
> >>
> >> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >> index 1bddeb1be66f..1ecf2cee343b 100644
> >> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> >>  		ecc->size = 1024;
> >>  		nsectors = mtd->writesize / ecc->size;
> >>  
> >> -		/* Reserve 2 bytes for the BBM */
> >> -		bytes = (mtd->oobsize - 2) / nsectors;
> >> +		bytes = mtd->oobsize / nsectors;  
> > 
> > I'm sorry but I don't think we can make this work. This change would
> > break all existing users...  
> 
> OK, it is not too much of an issue because I can manually specify the
> ECC parameters in the devicetree. Do you think it makes sense to fix
> this when adding new hardware variants/compatible strings?

Actually, looking at the code again, I don't get how the above diff
could be valid. The "maximize strength" logic (in which this diff is)
looks for the biggest region to store ECC bytes. These bytes cannot
be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
cannot get rid of this.

Thanks,
Miquèl
  

Hi Miquèl,

On 1/2/23 10:45, Miquel Raynal wrote:
>>>> This is already accounted for in the subtraction for OOB, since the BBM
>>>> overlaps the first OOB dword. With this change, the driver picks the
>>>> same ECC strength as the vendor driver.
>>>>
>>>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>
>>>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>>>> index 1bddeb1be66f..1ecf2cee343b 100644
>>>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>>>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>>>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>>>  		ecc->size = 1024;
>>>>  		nsectors = mtd->writesize / ecc->size;
>>>>  
>>>> -		/* Reserve 2 bytes for the BBM */
>>>> -		bytes = (mtd->oobsize - 2) / nsectors;
>>>> +		bytes = mtd->oobsize / nsectors;  
>>>
>>> I'm sorry but I don't think we can make this work. This change would
>>> break all existing users...  
>>
>> OK, it is not too much of an issue because I can manually specify the
>> ECC parameters in the devicetree. Do you think it makes sense to fix
>> this when adding new hardware variants/compatible strings?
> 
> Actually, looking at the code again, I don't get how the above diff
> could be valid. The "maximize strength" logic (in which this diff is)
> looks for the biggest region to store ECC bytes. These bytes cannot
> be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
> cannot get rid of this.

Right, we cannot overlap the BBM, but the BBM is accounted for in the
line below:

  /* 4 non-ECC bytes are added before each ECC bytes section */
  bytes -= 4;

Normally those 4 bytes are all free OOB, but for the first ECC step,
those are split into 2 free bytes and 2 BBM bytes:

  /*
   * The first 2 bytes are used for BB markers, hence we
   * only have 2 bytes available in the first user data
   * section.
   */
  if (!section && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
          oobregion->offset = 2;
          oobregion->length = 2;

          return 0;
  }

So if we subtract 4 bytes for the each free OOB area, including the
first one, and also subtract 2 bytes for the BBM, we are double-counting
the BBM. I should have made my commit message clearer. But I am going to
drop this patch anyway.

Regards,
Samuel
  

Hi Samuel,

samuel@sholland.org wrote on Mon, 2 Jan 2023 11:06:20 -0600:

> Hi Miquèl,
> 
> On 1/2/23 10:45, Miquel Raynal wrote:
> >>>> This is already accounted for in the subtraction for OOB, since the BBM
> >>>> overlaps the first OOB dword. With this change, the driver picks the
> >>>> same ECC strength as the vendor driver.
> >>>>
> >>>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> >>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >>>> ---
> >>>>
> >>>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
> >>>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> index 1bddeb1be66f..1ecf2cee343b 100644
> >>>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> >>>>  		ecc->size = 1024;
> >>>>  		nsectors = mtd->writesize / ecc->size;
> >>>>  
> >>>> -		/* Reserve 2 bytes for the BBM */
> >>>> -		bytes = (mtd->oobsize - 2) / nsectors;
> >>>> +		bytes = mtd->oobsize / nsectors;    
> >>>
> >>> I'm sorry but I don't think we can make this work. This change would
> >>> break all existing users...    
> >>
> >> OK, it is not too much of an issue because I can manually specify the
> >> ECC parameters in the devicetree. Do you think it makes sense to fix
> >> this when adding new hardware variants/compatible strings?  
> > 
> > Actually, looking at the code again, I don't get how the above diff
> > could be valid. The "maximize strength" logic (in which this diff is)
> > looks for the biggest region to store ECC bytes. These bytes cannot
> > be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
> > cannot get rid of this.  
> 
> Right, we cannot overlap the BBM, but the BBM is accounted for in the
> line below:
> 
>   /* 4 non-ECC bytes are added before each ECC bytes section */
>   bytes -= 4;
> 
> Normally those 4 bytes are all free OOB, but for the first ECC step,
> those are split into 2 free bytes and 2 BBM bytes:
> 
>   /*
>    * The first 2 bytes are used for BB markers, hence we
>    * only have 2 bytes available in the first user data
>    * section.
>    */
>   if (!section && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
>           oobregion->offset = 2;
>           oobregion->length = 2;
> 
>           return 0;
>   }
> 
> So if we subtract 4 bytes for the each free OOB area, including the
> first one, and also subtract 2 bytes for the BBM, we are double-counting
> the BBM. I should have made my commit message clearer. But I am going to
> drop this patch anyway.

Ah, yes, you are absolutely right, then.

Thanks,
Miquèl