diff mbox series

[v1,2/5] mtd: nand: raw: rockchip-nand-controller: add skipbbt option

Message ID 4d9473ce-c167-47d7-e37a-7b39915a724c@gmail.com
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series Fixes for Rockchip NAND controller driver | expand

Commit Message

Johan Jonker June 8, 2023, 4:30 p.m. UTC 
On Rockchip SoCs the first boot stages are written on NAND
with help of manufacturer software that uses a different format
then the MTD framework. Skip the automatic BBT scan with the
NAND_SKIP_BBTSCAN option so we can run it manually.
The NAND_NO_BBM_QUIRK option allows us to erase bad blocks with
the nand_erase_nand() function and the flash_erase command.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 drivers/mtd/nand/raw/rockchip-nand-controller.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--
2.30.2

Comments

Miquel Raynal June 9, 2023, 8:44 a.m. UTC | #1 
Hi Johan,

jbx6244@gmail.com wrote on Thu, 8 Jun 2023 18:30:27 +0200:

> On Rockchip SoCs the first boot stages are written on NAND
> with help of manufacturer software that uses a different format
> then the MTD framework. Skip the automatic BBT scan with the
> NAND_SKIP_BBTSCAN option so we can run it manually.

How do you run it manually?

> The NAND_NO_BBM_QUIRK option allows us to erase bad blocks with
> the nand_erase_nand() function and the flash_erase command.

For erasure you now have access to a debugfs entry for
experts/forensics which allows you to bypass all bad block checks.

> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  drivers/mtd/nand/raw/rockchip-nand-controller.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> index cafccc324..f56430f6c 100644
> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -188,6 +188,10 @@ struct rk_nfc {
>  	unsigned long assigned_cs;
>  };
> 
> +static int skipbbt;
> +module_param(skipbbt, int, 0644);
> +MODULE_PARM_DESC(skipbbt, "Skip BBT scan if the NAND chip contains data not in MTD format.");

I highly dislike this.

It's not a module parameter that you need, it is related to a partition.

> +
>  static inline struct rk_nfc_nand_chip *rk_nfc_to_rknand(struct nand_chip *chip)
>  {
>  	return container_of(chip, struct rk_nfc_nand_chip, chip);
> @@ -1156,6 +1160,10 @@ static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> 
>  	nand_set_controller_data(chip, nfc);
> 
> +	/* Skip the automatic BBT scan so we can run it manually. */
> +	if (skipbbt)
> +		chip->options |= NAND_SKIP_BBTSCAN | NAND_NO_BBM_QUIRK;
> +
>  	chip->options |= NAND_USES_DMA | NAND_NO_SUBPAGE_WRITE;
>  	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> 
> --
> 2.30.2
> 


Thanks,
Miquèl
Johan Jonker June 10, 2023, 10:13 p.m. UTC | #2 
On 6/9/23 10:44, Miquel Raynal wrote:
> Hi Johan,
> 
> jbx6244@gmail.com wrote on Thu, 8 Jun 2023 18:30:27 +0200:
> 
>> On Rockchip SoCs the first boot stages are written on NAND
>> with help of manufacturer software that uses a different format
>> then the MTD framework. Skip the automatic BBT scan with the
>> NAND_SKIP_BBTSCAN option so we can run it manually.
> 

> How do you run it manually?
> 
>> The NAND_NO_BBM_QUIRK option allows us to erase bad blocks with
>> the nand_erase_nand() function and the flash_erase command.
> 
> For erasure you now have access to a debugfs entry for

Hi Miquel,

Thank for your comments.
It's not about erasure alone. 

> experts/forensics which allows you to bypass all bad block checks.


It would be nice if all manufacturers would have taken the MTD framework as center for there NAND activity.
In the real world Rockchip has done it there own way.
For the Rockchip rk3066 SoC we still depend on a closed source loader/usbplug to either (1) load U-Boot/Linux from MMC or (2) write Linux to NAND with help of a closed source FTL.

We currently end up in these scenarios:

(1)
Full NAND support in U-Boot that works with all existing tools is still WIP.
I've done some work there, but still incomplete:

Fixes for Rockchip NFC driver part 1
https://lore.kernel.org/u-boot/be3c5f12-9df4-0a52-4858-c44d848e9147@gmail.com/

Proof of concept:
[PATCH v2 00/11] Add Rockchip IDB device
https://lore.kernel.org/u-boot/a1458a7b-2043-6397-3107-2d1fdf08c8e1@gmail.com/

So if users already manage to use U-Boot they probably skip formatting due to the lack of support or not at all if they don't manage to solder the missing serial debug port in the box.
Best chance is most users see all blocks marked as bad due to the different format when this module is built in.
On top of that the MTD framework also starts a block search loop and a mess with the original content in particular at the BBT location in the last blocks which is "not done" if not MTD formatted yet!
If a user only wants to read/write/erase boot blocks he/she must do so without effect on other content.
A fixed module parameter is then the better option either as built-in or as separate module then your debugfs suggestion that is not going to work for the average user.

(2)
When Linux is loaded from NAND with help of a FTL "what most users still do". It should not start to overwrite it's own blocks of that just happens to be MTD BBT blocks.

===

Both cases require a more neutral probe beside the optimal normal Linux boot flow with already MTD formatted.

Johan


===

Example skipbbt function in existing driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/nand/raw/cafe_nand.c#n80


> 
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  drivers/mtd/nand/raw/rockchip-nand-controller.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> index cafccc324..f56430f6c 100644
>> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> @@ -188,6 +188,10 @@ struct rk_nfc {
>>  	unsigned long assigned_cs;
>>  };
>>
>> +static int skipbbt;
>> +module_param(skipbbt, int, 0644);
>> +MODULE_PARM_DESC(skipbbt, "Skip BBT scan if the NAND chip contains data not in MTD format.");
> 

> I highly dislike this.

When build in how to change probe behavior in a more neutral way?
No scanning, erasing allowed everywhere and no changes to original content!

> 
> It's not a module parameter that you need, it is related to a partition.

Please advise what text could be better in this case.

> 
>> +
>>  static inline struct rk_nfc_nand_chip *rk_nfc_to_rknand(struct nand_chip *chip)
>>  {
>>  	return container_of(chip, struct rk_nfc_nand_chip, chip);
>> @@ -1156,6 +1160,10 @@ static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
>>
>>  	nand_set_controller_data(chip, nfc);
>>
>> +	/* Skip the automatic BBT scan so we can run it manually. */
>> +	if (skipbbt)
>> +		chip->options |= NAND_SKIP_BBTSCAN | NAND_NO_BBM_QUIRK;
>> +
>>  	chip->options |= NAND_USES_DMA | NAND_NO_SUBPAGE_WRITE;
>>  	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>>
>> --
>> 2.30.2
>>
> 
> 
> Thanks,
> Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index cafccc324..f56430f6c 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -188,6 +188,10 @@  struct rk_nfc {
 	unsigned long assigned_cs;
 };

+static int skipbbt;
+module_param(skipbbt, int, 0644);
+MODULE_PARM_DESC(skipbbt, "Skip BBT scan if the NAND chip contains data not in MTD format.");
+
 static inline struct rk_nfc_nand_chip *rk_nfc_to_rknand(struct nand_chip *chip)
 {
 	return container_of(chip, struct rk_nfc_nand_chip, chip);
@@ -1156,6 +1160,10 @@  static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,

 	nand_set_controller_data(chip, nfc);

+	/* Skip the automatic BBT scan so we can run it manually. */
+	if (skipbbt)
+		chip->options |= NAND_SKIP_BBTSCAN | NAND_NO_BBM_QUIRK;
+
 	chip->options |= NAND_USES_DMA | NAND_NO_SUBPAGE_WRITE;
 	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;