diff mbox series

[v3,5/5] mtd: rawnand: brcmnand: Fix mtd oobsize

Message ID 20230627193738.19596-6-william.zhang@broadcom.com
State New
Headers
Series mtd: rawnand: brcmnand: driver and doc updates |

Commit Message

William Zhang June 27, 2023, 7:37 p.m. UTC 
  brcmnand controller can only access the flash spare area up to certain
bytes based on the ECC level. It can be less than the actual flash spare
area size. For example, for many NAND chip supporting ECC BCH-8, it has
226 bytes spare area. But controller can only uses 218 bytes. So brcmand
driver overrides the mtd oobsize with the controller's accessible spare
area size. When the nand base driver utilizes the nand_device object, it
resets the oobsize back to the actual flash spare aprea size from
nand_memory_organization structure and controller may not able to access
all the oob area as mtd advises.

This change fixes the issue by overriding the oobsize in the
nand_memory_organization structure to the controller's accessible spare
area size.

Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
Signed-off-by: William Zhang <william.zhang@broadcom.com>

---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Miquel Raynal July 4, 2023, 3:30 p.m. UTC | #1 
Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:

> brcmnand controller can only access the flash spare area up to certain
> bytes based on the ECC level. It can be less than the actual flash spare
> area size. For example, for many NAND chip supporting ECC BCH-8, it has
> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
> driver overrides the mtd oobsize with the controller's accessible spare
> area size. When the nand base driver utilizes the nand_device object, it
> resets the oobsize back to the actual flash spare aprea size from
> nand_memory_organization structure and controller may not able to access
> all the oob area as mtd advises.
> 
> This change fixes the issue by overriding the oobsize in the
> nand_memory_organization structure to the controller's accessible spare
> area size.

I am clearly not a big fan of this solution. memorg should be and
remain a read only object. Can you please find another solution?

> 
> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> 
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 407bf79cbaf4..39c7f547db1f 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	struct nand_chip *chip = &host->chip;
>  	const struct nand_ecc_props *requirements =
>  		nanddev_get_ecc_requirements(&chip->base);
> +	struct nand_memory_organization *memorg =
> +		nanddev_get_memorg(&chip->base);
>  	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct brcmnand_cfg *cfg = &host->hwcfg;
>  	char msg[128];
> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	if (cfg->spare_area_size > ctrl->max_oob)
>  		cfg->spare_area_size = ctrl->max_oob;
>  	/*
> -	 * Set oobsize to be consistent with controller's spare_area_size, as
> -	 * the rest is inaccessible.
> +	 * Set mtd and memorg oobsize to be consistent with controller's
> +	 * spare_area_size, as the rest is inaccessible.
>  	 */
>  	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
> +	memorg->oobsize = mtd->oobsize;
>  
>  	cfg->device_size = mtd->size;
>  	cfg->block_size = mtd->erasesize;


Thanks,
Miquèl
William Zhang July 5, 2023, 12:50 a.m. UTC | #2 
Hi Miquel,

On 07/04/2023 08:30 AM, Miquel Raynal wrote:
> Hi William,
> 
> william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:
> 
>> brcmnand controller can only access the flash spare area up to certain
>> bytes based on the ECC level. It can be less than the actual flash spare
>> area size. For example, for many NAND chip supporting ECC BCH-8, it has
>> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
>> driver overrides the mtd oobsize with the controller's accessible spare
>> area size. When the nand base driver utilizes the nand_device object, it
>> resets the oobsize back to the actual flash spare aprea size from
>> nand_memory_organization structure and controller may not able to access
>> all the oob area as mtd advises.
>>
>> This change fixes the issue by overriding the oobsize in the
>> nand_memory_organization structure to the controller's accessible spare
>> area size.
> 
> I am clearly not a big fan of this solution. memorg should be and
> remain a read only object. Can you please find another solution?
> 
I was debating on this too but I don't see option because there is no 
other hooks after nanddev_init to set the mtd->oobsize as far as I can 
see and I see there were similar fixes for other controller drivers 
after the nand device object init was committed, for example:
Fixes: 629a442cad5f ("mtd: rawnand: Fill memorg during detection")

I will think through this again but I am open to any suggestion.

>>
>> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index 407bf79cbaf4..39c7f547db1f 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>   	struct nand_chip *chip = &host->chip;
>>   	const struct nand_ecc_props *requirements =
>>   		nanddev_get_ecc_requirements(&chip->base);
>> +	struct nand_memory_organization *memorg =
>> +		nanddev_get_memorg(&chip->base);
>>   	struct brcmnand_controller *ctrl = host->ctrl;
>>   	struct brcmnand_cfg *cfg = &host->hwcfg;
>>   	char msg[128];
>> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>   	if (cfg->spare_area_size > ctrl->max_oob)
>>   		cfg->spare_area_size = ctrl->max_oob;
>>   	/*
>> -	 * Set oobsize to be consistent with controller's spare_area_size, as
>> -	 * the rest is inaccessible.
>> +	 * Set mtd and memorg oobsize to be consistent with controller's
>> +	 * spare_area_size, as the rest is inaccessible.
>>   	 */
>>   	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
>> +	memorg->oobsize = mtd->oobsize;
>>   
>>   	cfg->device_size = mtd->size;
>>   	cfg->block_size = mtd->erasesize;
> 
> 
> Thanks,
> Miquèl
>
Miquel Raynal July 5, 2023, 7:11 a.m. UTC | #3 
Hi William,

william.zhang@broadcom.com wrote on Tue, 4 Jul 2023 17:50:28 -0700:

> Hi Miquel,
> 
> On 07/04/2023 08:30 AM, Miquel Raynal wrote:
> > Hi William,
> > 
> > william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:
> >   
> >> brcmnand controller can only access the flash spare area up to certain
> >> bytes based on the ECC level. It can be less than the actual flash spare
> >> area size. For example, for many NAND chip supporting ECC BCH-8, it has
> >> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
> >> driver overrides the mtd oobsize with the controller's accessible spare
> >> area size. When the nand base driver utilizes the nand_device object, it
> >> resets the oobsize back to the actual flash spare aprea size from
> >> nand_memory_organization structure and controller may not able to access
> >> all the oob area as mtd advises.
> >>
> >> This change fixes the issue by overriding the oobsize in the
> >> nand_memory_organization structure to the controller's accessible spare
> >> area size.  
> > 
> > I am clearly not a big fan of this solution. memorg should be and
> > remain a read only object. Can you please find another solution?
> >   
> I was debating on this too but I don't see option because there is no other hooks after nanddev_init to set the mtd->oobsize as far as I can see and I see there were similar fixes for other controller drivers after the nand device object init was committed, for example:
> Fixes: 629a442cad5f ("mtd: rawnand: Fill memorg during detection")

You are right, that was indeed the first approach, I'll remain on this
path then. Thanks for the pointer.

> 
> I will think through this again but I am open to any suggestion.
> 
> >>
> >> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
> >> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >>
> >> ---
> >>
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index 407bf79cbaf4..39c7f547db1f 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>   	struct nand_chip *chip = &host->chip;
> >>   	const struct nand_ecc_props *requirements =
> >>   		nanddev_get_ecc_requirements(&chip->base);
> >> +	struct nand_memory_organization *memorg =
> >> +		nanddev_get_memorg(&chip->base);
> >>   	struct brcmnand_controller *ctrl = host->ctrl;
> >>   	struct brcmnand_cfg *cfg = &host->hwcfg;
> >>   	char msg[128];
> >> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>   	if (cfg->spare_area_size > ctrl->max_oob)
> >>   		cfg->spare_area_size = ctrl->max_oob;
> >>   	/*
> >> -	 * Set oobsize to be consistent with controller's spare_area_size, as
> >> -	 * the rest is inaccessible.
> >> +	 * Set mtd and memorg oobsize to be consistent with controller's
> >> +	 * spare_area_size, as the rest is inaccessible.
> >>   	 */
> >>   	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
> >> +	memorg->oobsize = mtd->oobsize;  
> >>   >>   	cfg->device_size = mtd->size;  
> >>   	cfg->block_size = mtd->erasesize;  
> > 
> > 
> > Thanks,
> > Miquèl
> >   


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 407bf79cbaf4..39c7f547db1f 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2647,6 +2647,8 @@  static int brcmnand_setup_dev(struct brcmnand_host *host)
 	struct nand_chip *chip = &host->chip;
 	const struct nand_ecc_props *requirements =
 		nanddev_get_ecc_requirements(&chip->base);
+	struct nand_memory_organization *memorg =
+		nanddev_get_memorg(&chip->base);
 	struct brcmnand_controller *ctrl = host->ctrl;
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	char msg[128];
@@ -2668,10 +2670,11 @@  static int brcmnand_setup_dev(struct brcmnand_host *host)
 	if (cfg->spare_area_size > ctrl->max_oob)
 		cfg->spare_area_size = ctrl->max_oob;
 	/*
-	 * Set oobsize to be consistent with controller's spare_area_size, as
-	 * the rest is inaccessible.
+	 * Set mtd and memorg oobsize to be consistent with controller's
+	 * spare_area_size, as the rest is inaccessible.
 	 */
 	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
+	memorg->oobsize = mtd->oobsize;
 
 	cfg->device_size = mtd->size;
 	cfg->block_size = mtd->erasesize;