Skip to contentSkip to conversation

Add support for unprotected spare data page

PATCH Superseded Ansuel Smith 4 participants 14 messages May 19, 2022 – Jun 7, 2022

Subsystems (5)
Device tree bindings Arm Qualcomm mailing list Memory technology devices (MTD) NAND flash +1 more subsystems

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Ansuel Smith

author
#0 lore raw

v4 cover Add support for unprotected spare data page

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v4 1/2 mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

IPQ8064 nand have special pages where the spare data is not protected by ECC. These special page are used by boot partition and on reading them lots of warning are reported about wrong ECC data and if written to results in broken data and not bootable device.

Under the hood these special page are just normal page with the spare data not protected by ECC.

Add support for this by permitting the user to declare these special pages in dts by declaring offset and size of the partition. The driver internally will convert these value to nand pages.

On user read/write the page is checked and if it's a boot page the correct configuration is applied.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 148 +++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +143 -5
index 1a77542c6d67..289aef4f191d 100644
@@ -80,8 +80,10 @@
 #define	DISABLE_STATUS_AFTER_WRITE	4
 #define	CW_PER_PAGE			6
 #define	UD_SIZE_BYTES			9
+#define	UD_SIZE_BYTES_MASK		GENMASK(18, 9)
 #define	ECC_PARITY_SIZE_BYTES_RS	19
 #define	SPARE_SIZE_BYTES		23
+#define	SPARE_SIZE_BYTES_MASK		GENMASK(26, 23)
 #define	NUM_ADDR_CYCLES			27
 #define	STATUS_BFR_READ			30
 #define	SET_RD_MODE_AFTER_STATUS	31

@@ -102,6 +104,7 @@
 #define	ECC_MODE			4
 #define	ECC_PARITY_SIZE_BYTES_BCH	8
 #define	ECC_NUM_DATA_BYTES		16
+#define	ECC_NUM_DATA_BYTES_MASK		GENMASK(25, 16)
 #define	ECC_FORCE_CLK_OPEN		30
 
 /* NAND_DEV_CMD1 bits */

@@ -418,6 +421,19 @@ struct qcom_nand_controller {
 	const struct qcom_nandc_props *props;
 };
 
+/*
+ * NAND special boot pages
+ *
+ * @offset:			offset of the page where spare data is not protected
+ *				by ECC
+ * @size:			size of the page where spare data is not protected
+ *				by ECC
+ */
+struct qcom_nand_boot_page {
+	u32 offset;
+	u32 size;
+};
+
 /*
  * NAND chip structure
  *

@@ -444,6 +460,13 @@ struct qcom_nand_controller {
  * @cfg0, cfg1, cfg0_raw..:	NANDc register configurations needed for
  *				ecc/non-ecc mode for the current nand flash
  *				device
+ *
+ * @unprotect_spare_data:	keep track of the current ecc configuration used by
+ *				the driver for read/write operation.
+ * @boot_pages_count:		count of the boot pages where spare data is not
+ *				protected by ECC
+ * @boot_pages:			array of boot pages where offset and size of the
+ *				boot pages are stored
  */
 struct qcom_nand_host {
 	struct nand_chip chip;

@@ -466,6 +489,10 @@ struct qcom_nand_host {
 	u32 ecc_bch_cfg;
 	u32 clrflashstatus;
 	u32 clrreadstatus;
+
+	bool unprotect_spare_data;
+	int boot_pages_count;
+	struct qcom_nand_boot_page *boot_pages;
 };
 
 /*

@@ -475,6 +502,7 @@ struct qcom_nand_host {
  * @is_bam - whether NAND controller is using BAM
  * @is_qpic - whether NAND CTRL is part of qpic IP
  * @qpic_v2 - flag to indicate QPIC IP version 2
+ * @has_boot_pages - whether NAND has different ecc settings for boot pages
  * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
  */
 struct qcom_nandc_props {

@@ -482,6 +510,7 @@ struct qcom_nandc_props {
 	bool is_bam;
 	bool is_qpic;
 	bool qpic_v2;
+	bool has_boot_pages;
 	u32 dev_cmd_reg_start;
 };
 

@@ -1701,7 +1730,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
 	data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
 	oob_size1 = host->bbm_size;
 
-	if (qcom_nandc_is_last_cw(ecc, cw)) {
+	if (qcom_nandc_is_last_cw(ecc, cw) && !host->unprotect_spare_data) {
 		data_size2 = ecc->size - data_size1 -
 			     ((ecc->steps - 1) * 4);
 		oob_size2 = (ecc->steps * 4) + host->ecc_bytes_hw +

@@ -1782,7 +1811,7 @@ check_for_erased_page(struct qcom_nand_host *host, u8 *data_buf,
 	}
 
 	for_each_set_bit(cw, &uncorrectable_cws, ecc->steps) {
-		if (qcom_nandc_is_last_cw(ecc, cw)) {
+		if (qcom_nandc_is_last_cw(ecc, cw) && !host->unprotect_spare_data) {
 			data_size = ecc->size - ((ecc->steps - 1) * 4);
 			oob_size = (ecc->steps * 4) + host->ecc_bytes_hw;
 		} else {

@@ -1940,7 +1969,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	for (i = 0; i < ecc->steps; i++) {
 		int data_size, oob_size;
 
-		if (qcom_nandc_is_last_cw(ecc, i)) {
+		if (qcom_nandc_is_last_cw(ecc, i) && !host->unprotect_spare_data) {
 			data_size = ecc->size - ((ecc->steps - 1) << 2);
 			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
 				   host->spare_bytes;

@@ -2037,6 +2066,52 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
 	return ret;
 }
 
+static bool
+qcom_nandc_is_boot_page(struct qcom_nand_host *host, int page)
+{
+	struct qcom_nand_boot_page *boot_page;
+	u32 start, end;
+	int i;
+
+	for (i = 0; i < host->boot_pages_count; i++) {
+		boot_page = &host->boot_pages[i];
+		start = boot_page->offset;
+		end = start + boot_page->size;
+		/* Boot page are at the start of the nand.
+		 * Check the page from the boot page end first
+		 * to save one extra check.
+		 */
+		if (page < end && page >= start)
+			return 1;
+	}
+
+	return 0;
+}
+
+static void
+qcom_nandc_check_boot_pages(struct qcom_nand_host *host, int page)
+{
+	bool unprotect_spare_data = qcom_nandc_is_boot_page(host, page);
+
+	/* Skip conf write if we are already in the correct mode */
+	if (unprotect_spare_data == host->unprotect_spare_data)
+		return;
+
+	host->unprotect_spare_data = unprotect_spare_data;
+
+	host->cw_data = unprotect_spare_data ? 512 : 516;
+	host->spare_bytes = host->cw_size - host->ecc_bytes_hw -
+			    host->bbm_size - host->cw_data;
+
+	host->cfg0 &= ~(SPARE_SIZE_BYTES_MASK | UD_SIZE_BYTES_MASK);
+	host->cfg0 |= host->spare_bytes << SPARE_SIZE_BYTES |
+		      host->cw_data << UD_SIZE_BYTES;
+
+	host->ecc_bch_cfg &= ~ECC_NUM_DATA_BYTES_MASK;
+	host->ecc_bch_cfg |= host->cw_data << ECC_NUM_DATA_BYTES;
+	host->ecc_buf_cfg = (unprotect_spare_data ? 0x1ff : 0x203) << NUM_STEPS;
+}
+
 /* implements ecc->read_page() */
 static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
 				int oob_required, int page)

@@ -2045,6 +2120,9 @@ static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	u8 *data_buf, *oob_buf = NULL;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	nand_read_page_op(chip, page, 0, NULL, 0);
 	data_buf = buf;
 	oob_buf = oob_required ? chip->oob_poi : NULL;

@@ -2064,6 +2142,9 @@ static int qcom_nandc_read_page_raw(struct nand_chip *chip, uint8_t *buf,
 	int cw, ret;
 	u8 *data_buf = buf, *oob_buf = chip->oob_poi;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	for (cw = 0; cw < ecc->steps; cw++) {
 		ret = qcom_nandc_read_cw_raw(mtd, chip, data_buf, oob_buf,
 					     page, cw);

@@ -2084,6 +2165,9 @@ static int qcom_nandc_read_oob(struct nand_chip *chip, int page)
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);
 

@@ -2104,6 +2188,9 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const uint8_t *buf,
 	u8 *data_buf, *oob_buf;
 	int i, ret;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
 
 	clear_read_regs(nandc);

@@ -2119,7 +2206,7 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const uint8_t *buf,
 	for (i = 0; i < ecc->steps; i++) {
 		int data_size, oob_size;
 
-		if (qcom_nandc_is_last_cw(ecc, i)) {
+		if (qcom_nandc_is_last_cw(ecc, i) && !host->unprotect_spare_data) {
 			data_size = ecc->size - ((ecc->steps - 1) << 2);
 			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
 				   host->spare_bytes;

@@ -2176,6 +2263,9 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
 	u8 *data_buf, *oob_buf;
 	int i, ret;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
 	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);

@@ -2194,7 +2284,7 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
 		data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
 		oob_size1 = host->bbm_size;
 
-		if (qcom_nandc_is_last_cw(ecc, i)) {
+		if (qcom_nandc_is_last_cw(ecc, i) && !host->unprotect_spare_data) {
 			data_size2 = ecc->size - data_size1 -
 				     ((ecc->steps - 1) << 2);
 			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +

@@ -2254,6 +2344,9 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
 	int data_size, oob_size;
 	int ret;
 
+	if (host->boot_pages_count)
+		qcom_nandc_check_boot_pages(host, page);
+
 	host->use_ecc = true;
 	clear_bam_transaction(nandc);
 

@@ -2902,6 +2995,48 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
 
 static const char * const probes[] = { "cmdlinepart", "ofpart", "qcomsmem", NULL };
 
+static int qcom_nand_host_parse_boot_pages(struct qcom_nand_controller *nandc,
+					   struct qcom_nand_host *host,
+					   struct device_node *dn)
+{
+	struct nand_chip *chip = &host->chip;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct qcom_nand_boot_page *boot_page;
+	struct device *dev = nandc->dev;
+	int pages_count, i, ret;
+
+	if (!nandc->props->has_boot_pages)
+		return 0;
+
+	pages_count = of_property_count_u32_elems(dn, "qcom,boot-pages");
+	if (pages_count < 0) {
+		dev_err(dev, "Error parsing boot_pages. Ignoring.");
+		return 0;
+	}
+
+	host->boot_pages_count = pages_count / 2;
+	host->boot_pages = devm_kcalloc(dev, host->boot_pages_count,
+					sizeof(*host->boot_pages), GFP_KERNEL);
+	if (!host->boot_pages)
+		return 0;
+
+	ret = of_property_read_u32_array(dn, "qcom,boot-pages", (u32 *)host->boot_pages,
+					 pages_count);
+	if (ret) {
+		dev_err(dev, "Error reading boot_pages. Ignoring.");
+		return 0;
+	}
+
+	/* Convert offset to nand pages */
+	for (i = 0 ; i < host->boot_pages_count; i++) {
+		boot_page = &host->boot_pages[i];
+		boot_page->offset /= mtd->writesize;
+		boot_page->size /= mtd->writesize;
+	}
+
+	return 0;
+}
+
 static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
 					    struct qcom_nand_host *host,
 					    struct device_node *dn)

@@ -2970,6 +3105,8 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
 	if (ret)
 		nand_cleanup(chip);
 
+	qcom_nand_host_parse_boot_pages(nandc, host, dn);
+
 	return ret;
 }
 

@@ -3135,6 +3272,7 @@ static int qcom_nandc_remove(struct platform_device *pdev)
 static const struct qcom_nandc_props ipq806x_nandc_props = {
 	.ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT),
 	.is_bam = false,
+	.has_boot_pages = true,
 	.dev_cmd_reg_start = 0x0,
 };
Signature · 1 line
2.34.1
maintainer
#2 lore raw

Re: [PATCH v4 1/2] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

+ Sricharan, Alam

Quoted 5 lines · Ansuel Smith

IPQ8064 nand have special pages where the spare data is not protected by ECC. These special page are used by boot partition and on reading them lots of warning are reported about wrong ECC data and if written to results in broken data and not bootable device.

After checking internally I gathered more information on this issue. The problem is that the boot partitions are using a different layout scheme compared to other partitions like rootfs. This is because, the boot partitions are accessed by bootloader like u-boot that uses the modified layout.

And the modified layout uses 512 bytes as the codeword size (even for the last codeword) while writing to the CFG0 register. This forces the NAND controller to unprotect the 4 bytes of spare data. So if kernel is unaware of this layout, it will try to protect the spare data too during read/write to these partitions and that will result in CRC errors.

So please update the commit message with above info.

Quoted 39 lines

Under the hood these special page are just normal page with the spare data not protected by ECC.

Add support for this by permitting the user to declare these special pages in dts by declaring offset and size of the partition. The driver internally will convert these value to nand pages.

On user read/write the page is checked and if it's a boot page the correct configuration is applied.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 148 +++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +3 -0
index 1a77542c6d67..289aef4f191d 100644
@@ -80,8 +80,10 @@
 #define	DISABLE_STATUS_AFTER_WRITE	4
 #define	CW_PER_PAGE			6
 #define	UD_SIZE_BYTES			9
+#define	UD_SIZE_BYTES_MASK		GENMASK(18, 9)
 #define	ECC_PARITY_SIZE_BYTES_RS	19
 #define	SPARE_SIZE_BYTES		23
+#define	SPARE_SIZE_BYTES_MASK		GENMASK(26, 23)
 #define	NUM_ADDR_CYCLES			27
 #define	STATUS_BFR_READ			30
 #define	SET_RD_MODE_AFTER_STATUS	31

@@ -102,6 +104,7 @@
 #define	ECC_MODE			4
 #define	ECC_PARITY_SIZE_BYTES_BCH	8
 #define	ECC_NUM_DATA_BYTES		16
+#define	ECC_NUM_DATA_BYTES_MASK		GENMASK(25, 16)
 #define	ECC_FORCE_CLK_OPEN		30
 
 /* NAND_DEV_CMD1 bits */

drivers/mtd/nand/raw/qcom_nandc.c struct qcom_nand_controller { · line 425

Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +2 -0
@@ -418,6 +421,19 @@ struct qcom_nand_controller {
 	const struct qcom_nandc_props *props;
 };
 
+/*
+ * NAND special boot pages

s/page/partitions everywhere

Quoted 15 lines
drivers/mtd/nand/raw/qcom_nandc.c +11 -0
+ *
+ * @offset:			offset of the page where spare data is not protected
+ *				by ECC
+ * @size:			size of the page where spare data is not protected
+ *				by ECC
+ */
+struct qcom_nand_boot_page {
+	u32 offset;
+	u32 size;
+};
+
 /*
  * NAND chip structure
  *

drivers/mtd/nand/raw/qcom_nandc.c struct qcom_nand_controller { · line 465

Quoted 6 lines
drivers/mtd/nand/raw/qcom_nandc.c +3 -0
@@ -444,6 +460,13 @@ struct qcom_nand_controller {
  * @cfg0, cfg1, cfg0_raw..:	NANDc register configurations needed for
  *				ecc/non-ecc mode for the current nand flash
  *				device
+ *
+ * @unprotect_spare_data:	keep track of the current ecc configuration used by
+ *				the driver for read/write operation.

Unprotected spare data is the effect of using a different codeword size. So use something like, @codeword_fixup.

Quoted 8 lines
drivers/mtd/nand/raw/qcom_nandc.c +4 -0
+ * @boot_pages_count:		count of the boot pages where spare data is not
+ *				protected by ECC
+ * @boot_pages:			array of boot pages where offset and size of the
+ *				boot pages are stored
  */
 struct qcom_nand_host {
 	struct nand_chip chip;

drivers/mtd/nand/raw/qcom_nandc.c struct qcom_nand_host { · line 495

Quoted 7 lines
drivers/mtd/nand/raw/qcom_nandc.c +4 -0
@@ -466,6 +489,10 @@ struct qcom_nand_host {
 	u32 ecc_bch_cfg;
 	u32 clrflashstatus;
 	u32 clrreadstatus;
+
+	bool unprotect_spare_data;
+	int boot_pages_count;
+	struct qcom_nand_boot_page *boot_pages;

Reorganize the members to avoid holes.

drivers/mtd/nand/raw/qcom_nandc.c struct qcom_nand_host { · line 505

Quoted 8 lines
drivers/mtd/nand/raw/qcom_nandc.c +1 -0
 };
 
 /*

@@ -475,6 +502,7 @@ struct qcom_nand_host {
  * @is_bam - whether NAND controller is using BAM
  * @is_qpic - whether NAND CTRL is part of qpic IP
  * @qpic_v2 - flag to indicate QPIC IP version 2
+ * @has_boot_pages - whether NAND has different ecc settings for boot pages

@use_codeword_fixup?

Quoted 46 lines
drivers/mtd/nand/raw/qcom_nandc.c +8 -3
  * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
  */
 struct qcom_nandc_props {

@@ -482,6 +510,7 @@ struct qcom_nandc_props {
 	bool is_bam;
 	bool is_qpic;
 	bool qpic_v2;
+	bool has_boot_pages;
 	u32 dev_cmd_reg_start;
 };
 

@@ -1701,7 +1730,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
 	data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
 	oob_size1 = host->bbm_size;
 
-	if (qcom_nandc_is_last_cw(ecc, cw)) {
+	if (qcom_nandc_is_last_cw(ecc, cw) && !host->unprotect_spare_data) {
 		data_size2 = ecc->size - data_size1 -
 			     ((ecc->steps - 1) * 4);
 		oob_size2 = (ecc->steps * 4) + host->ecc_bytes_hw +

@@ -1782,7 +1811,7 @@ check_for_erased_page(struct qcom_nand_host *host, u8 *data_buf,
 	}
 
 	for_each_set_bit(cw, &uncorrectable_cws, ecc->steps) {
-		if (qcom_nandc_is_last_cw(ecc, cw)) {
+		if (qcom_nandc_is_last_cw(ecc, cw) && !host->unprotect_spare_data) {
 			data_size = ecc->size - ((ecc->steps - 1) * 4);
 			oob_size = (ecc->steps * 4) + host->ecc_bytes_hw;
 		} else {

@@ -1940,7 +1969,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	for (i = 0; i < ecc->steps; i++) {
 		int data_size, oob_size;
 
-		if (qcom_nandc_is_last_cw(ecc, i)) {
+		if (qcom_nandc_is_last_cw(ecc, i) && !host->unprotect_spare_data) {
 			data_size = ecc->size - ((ecc->steps - 1) << 2);
 			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
 				   host->spare_bytes;

@@ -2037,6 +2066,52 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
 	return ret;
 }
 
+static bool
+qcom_nandc_is_boot_page(struct qcom_nand_host *host, int page)
+{
+	struct qcom_nand_boot_page *boot_page;

drivers/mtd/nand/raw/qcom_nandc.c static int copy_last_cw(struct qcom_nand_host *host, int page) · line 2082

Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +10 -0
+	u32 start, end;
+	int i;
+
+	for (i = 0; i < host->boot_pages_count; i++) {
+		boot_page = &host->boot_pages[i];
+		start = boot_page->offset;
+		end = start + boot_page->size;
+		/* Boot page are at the start of the nand.
+		 * Check the page from the boot page end first
+		 * to save one extra check.

Is the comment valid still?

drivers/mtd/nand/raw/qcom_nandc.c static int copy_last_cw(struct qcom_nand_host *host, int page) · line 2085

Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c +3 -0
+		 */
+		if (page < end && page >= start)
+			return 1;

true?

drivers/mtd/nand/raw/qcom_nandc.c static int copy_last_cw(struct qcom_nand_host *host, int page) · line 2088

Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c +3 -0
+	}
+
+	return 0;

false?

drivers/mtd/nand/raw/qcom_nandc.c static int copy_last_cw(struct qcom_nand_host *host, int page) · line 2092

Quoted 4 lines
drivers/mtd/nand/raw/qcom_nandc.c +4 -0
+}
+
+static void
+qcom_nandc_check_boot_pages(struct qcom_nand_host *host, int page)

qcom_nandc_codeword_fixup()?

Quoted 22 lines
drivers/mtd/nand/raw/qcom_nandc.c +22 -0
+{
+	bool unprotect_spare_data = qcom_nandc_is_boot_page(host, page);
+
+	/* Skip conf write if we are already in the correct mode */
+	if (unprotect_spare_data == host->unprotect_spare_data)
+		return;
+
+	host->unprotect_spare_data = unprotect_spare_data;
+
+	host->cw_data = unprotect_spare_data ? 512 : 516;
+	host->spare_bytes = host->cw_size - host->ecc_bytes_hw -
+			    host->bbm_size - host->cw_data;
+
+	host->cfg0 &= ~(SPARE_SIZE_BYTES_MASK | UD_SIZE_BYTES_MASK);
+	host->cfg0 |= host->spare_bytes << SPARE_SIZE_BYTES |
+		      host->cw_data << UD_SIZE_BYTES;
+
+	host->ecc_bch_cfg &= ~ECC_NUM_DATA_BYTES_MASK;
+	host->ecc_bch_cfg |= host->cw_data << ECC_NUM_DATA_BYTES;
+	host->ecc_buf_cfg = (unprotect_spare_data ? 0x1ff : 0x203) << NUM_STEPS;
+}
+

[...]

Quoted 6 lines
drivers/mtd/nand/raw/qcom_nandc.c +6 -0
+static int qcom_nand_host_parse_boot_pages(struct qcom_nand_controller *nandc,
+					   struct qcom_nand_host *host,
+					   struct device_node *dn)
+{
+	struct nand_chip *chip = &host->chip;
+	struct mtd_info *mtd = nand_to_mtd(chip);

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_setup(struct qcom_nand_controller *nandc) · line 3013

Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +10 -0
+	struct qcom_nand_boot_page *boot_page;
+	struct device *dev = nandc->dev;
+	int pages_count, i, ret;
+
+	if (!nandc->props->has_boot_pages)
+		return 0;
+
+	pages_count = of_property_count_u32_elems(dn, "qcom,boot-pages");
+	if (pages_count < 0) {
+		dev_err(dev, "Error parsing boot_pages. Ignoring.");

I'd first check for the existence of the property first and bail out if it is not present. If it is present, then this error is a hard error.

This way, we will preserve DT backward compatibility and abort if the DT is broken.

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_setup(struct qcom_nand_controller *nandc) · line 3021

Quoted 8 lines
drivers/mtd/nand/raw/qcom_nandc.c +8 -0
+		return 0;
+	}
+
+	host->boot_pages_count = pages_count / 2;
+	host->boot_pages = devm_kcalloc(dev, host->boot_pages_count,
+					sizeof(*host->boot_pages), GFP_KERNEL);
+	if (!host->boot_pages)
+		return 0;

This should be a hard error since the property is present in DT and it is broken.

drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_setup(struct qcom_nand_controller *nandc) · line 3028

Quoted 7 lines
drivers/mtd/nand/raw/qcom_nandc.c +7 -0
+
+	ret = of_property_read_u32_array(dn, "qcom,boot-pages", (u32 *)host->boot_pages,
+					 pages_count);
+	if (ret) {
+		dev_err(dev, "Error reading boot_pages. Ignoring.");
+		return 0;
+	}

How about,

        for (i = 0, j = 0; i < host->nr_boot_partitions; i++, j += 2) {
                of_property_read_u32_index(dn, "qcom,boot-partitions", j,
                                           &host->boot_pages[i].offset);
                of_property_read_u32_index(dn, "qcom,boot-partitions", j + 1,
                                           &host->boot_pages[i].size);
        }

Thanks,
Mani

Signature · 5 lines
மணிவண்ணன் சதாசிவம்

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
author
#3 lore raw

PATCH v4 2/2 dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

In reply to Ansuel Smith

Document new qcom,boot-pages binding used to apply special read/write configuration to boot pages.

QCOM apply a special configuration where spare data is not protected by ECC for some special pages (used for boot partition). Add Documentation on how to declare these special pages.

Signed-off-by Ansuel Smith <[email protected]>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +26 -0
index 84ad7ff30121..a59ae9525f4e 100644
@@ -102,6 +102,30 @@ allOf:
             - const: rx
             - const: cmd
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-nand
+
+    then:
+      properties:
+        qcom,boot-pages:
+          $ref: /schemas/types.yaml#/definitions/uint32-matrix
+          items:
+            items:
+              - description: offset
+              - description: size
+          description:
+            Some special page used by boot partition have spare data
+            not protected by ECC. Use this to declare these special page
+            by defining first the offset and then the size.
+
+            It's in the form of <offset1 size1 offset2 size2 offset3 ...>
+
+            Refer to the ipq8064 example on how to use this special binding.
+
 required:
   - compatible
   - reg

@@ -135,6 +159,8 @@ examples:
         nand-ecc-strength = <4>;
         nand-bus-width = <8>;
 
+        qcom,boot-pages = <0x0 0x58a0000>;
+
         partitions {
           compatible = "fixed-partitions";
           #address-cells = <1>;
Signature · 1 line
2.34.1
maintainer Reviewed-by on v4 2/2
#4 lore raw

Re: [PATCH v4 2/2] dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

Quoted 12 lines · Ansuel Smith

Document new qcom,boot-pages binding used to apply special read/write configuration to boot pages.

QCOM apply a special configuration where spare data is not protected by ECC for some special pages (used for boot partition). Add Documentation on how to declare these special pages.

Signed-off-by Ansuel Smith <[email protected]>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Reviewed-by Rob Herring <[email protected]>
maintainer
#5 lore raw

Re: [PATCH v4 2/2] dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

In reply to Ansuel Smith
Quoted 20 lines · Ansuel Smith

Document new qcom,boot-pages binding used to apply special read/write configuration to boot pages.

QCOM apply a special configuration where spare data is not protected by ECC for some special pages (used for boot partition). Add Documentation on how to declare these special pages.

Signed-off-by Ansuel Smith <[email protected]>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +0 -0
index 84ad7ff30121..a59ae9525f4e 100644
@@ -102,6 +102,30 @@ allOf:
             - const: rx
             - const: cmd

Documentation/devicetree/bindings/mtd/qcom,nandc.yaml allOf: · line 114

Quoted 10 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +10 -0
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-nand
+
+    then:
+      properties:
+        qcom,boot-pages:

Eventhough the page layout is what making the difference, here the boot partition offset and size are getting specified. So how about, changing it to "qcom,boot-partitions"?

Thanks,
Mani

Quoted 29 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +16 -0
+          $ref: /schemas/types.yaml#/definitions/uint32-matrix
+          items:
+            items:
+              - description: offset
+              - description: size
+          description:
+            Some special page used by boot partition have spare data
+            not protected by ECC. Use this to declare these special page
+            by defining first the offset and then the size.
+
+            It's in the form of <offset1 size1 offset2 size2 offset3 ...>
+
+            Refer to the ipq8064 example on how to use this special binding.
+
 required:
   - compatible
   - reg

@@ -135,6 +159,8 @@ examples:
         nand-ecc-strength = <4>;
         nand-bus-width = <8>;
 
+        qcom,boot-pages = <0x0 0x58a0000>;
+
         partitions {
           compatible = "fixed-partitions";
           #address-cells = <1>;
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
author
#6 lore raw

Re: [PATCH v4 2/2] dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

Quoted 36 lines · Manivannan Sadhasivam
Quoted 20 lines · Ansuel Smith

Document new qcom,boot-pages binding used to apply special read/write configuration to boot pages.

QCOM apply a special configuration where spare data is not protected by ECC for some special pages (used for boot partition). Add Documentation on how to declare these special pages.

Signed-off-by Ansuel Smith <[email protected]>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +0 -0
index 84ad7ff30121..a59ae9525f4e 100644
@@ -102,6 +102,30 @@ allOf:
             - const: rx
             - const: cmd

Documentation/devicetree/bindings/mtd/qcom,nandc.yaml allOf: · line 114

Quoted 10 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +10 -0
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-nand
+
+    then:
+      properties:
+        qcom,boot-pages:

Eventhough the page layout is what making the difference, here the boot partition offset and size are getting specified. So how about, changing it to "qcom,boot-partitions"?

Quoted 3 lines

Thanks,
Mani

Yep, you are correct and the naming is confusing. Will do the change. Did you check the code if you notice something to improve / an idea of a better implementation or better naming?
Just to skip sending multiple revision with small changes.

Quoted 32 lines
Quoted 29 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +16 -0
+          $ref: /schemas/types.yaml#/definitions/uint32-matrix
+          items:
+            items:
+              - description: offset
+              - description: size
+          description:
+            Some special page used by boot partition have spare data
+            not protected by ECC. Use this to declare these special page
+            by defining first the offset and then the size.
+
+            It's in the form of <offset1 size1 offset2 size2 offset3 ...>
+
+            Refer to the ipq8064 example on how to use this special binding.
+
 required:
   - compatible
   - reg

@@ -135,6 +159,8 @@ examples:
         nand-ecc-strength = <4>;
         nand-bus-width = <8>;
 
+        qcom,boot-pages = <0x0 0x58a0000>;
+
         partitions {
           compatible = "fixed-partitions";
           #address-cells = <1>;
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Signature · 5 lines
	Ansuel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer
#7 lore raw

Re: [PATCH v4 2/2] dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

Quoted 37 lines · Ansuel Smith
Quoted 36 lines · Manivannan Sadhasivam
Quoted 20 lines · Ansuel Smith

Document new qcom,boot-pages binding used to apply special read/write configuration to boot pages.

QCOM apply a special configuration where spare data is not protected by ECC for some special pages (used for boot partition). Add Documentation on how to declare these special pages.

Signed-off-by Ansuel Smith <[email protected]>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +0 -0
index 84ad7ff30121..a59ae9525f4e 100644
@@ -102,6 +102,30 @@ allOf:
             - const: rx
             - const: cmd

Documentation/devicetree/bindings/mtd/qcom,nandc.yaml allOf: · line 114

Quoted 10 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +10 -0
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-nand
+
+    then:
+      properties:
+        qcom,boot-pages:

Eventhough the page layout is what making the difference, here the boot partition offset and size are getting specified. So how about, changing it to "qcom,boot-partitions"?

Quoted 9 lines
Quoted 3 lines

Thanks,
Mani

Yep, you are correct and the naming is confusing. Will do the change. Did you check the code if you notice something to improve / an idea of a better implementation or better naming?
Just to skip sending multiple revision with small changes.

Yep, I do have some comments. Will share them.

Thanks,
Mani

Quoted 35 lines
Quoted 32 lines
Quoted 29 lines
Documentation/devicetree/bindings/mtd/qcom,nandc.yaml +16 -0
+          $ref: /schemas/types.yaml#/definitions/uint32-matrix
+          items:
+            items:
+              - description: offset
+              - description: size
+          description:
+            Some special page used by boot partition have spare data
+            not protected by ECC. Use this to declare these special page
+            by defining first the offset and then the size.
+
+            It's in the form of <offset1 size1 offset2 size2 offset3 ...>
+
+            Refer to the ipq8064 example on how to use this special binding.
+
 required:
   - compatible
   - reg

@@ -135,6 +159,8 @@ examples:
         nand-ecc-strength = <4>;
         nand-bus-width = <8>;
 
+        qcom,boot-pages = <0x0 0x58a0000>;
+
         partitions {
           compatible = "fixed-partitions";
           #address-cells = <1>;
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Signature · 1 line
	Ansuel
Signature · 1 line
மணிவண்ணன் சதாசிவம்
maintainer
#8 lore raw
In reply to Ansuel Smith

Hi,

[email protected] wrote on Thu, 19 May 2022 21:01:10 +0200:

Quoted 35 lines

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

Quoted 8 lines

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

Manivannan, any feedback on this?

Quoted 9 lines

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Quoted 9 lines

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)

Thanks,
Miquèl

maintainer
#9 lore raw
Quoted 43 lines · Miquel Raynal

Hi,

[email protected] wrote on Thu, 19 May 2022 21:01:10 +0200:

Quoted 39 lines

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

Quoted 7 lines
Quoted 4 lines

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

Manivannan, any feedback on this?

Sorry for the delay. I will check internally on some of the unknown implementations mentioned and provide my comments.

Thanks,
Mani

Quoted 21 lines
Quoted 9 lines

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Quoted 9 lines

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)

Thanks,
Miquèl

Signature · 1 line
மணிவண்ணன் சதாசிவம்
maintainer
#10 lore raw
In reply to Ansuel Smith
Quoted 9 lines · Ansuel Smith

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Quoted 6 lines

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

Can you please point me to the downstream/vendor driver that has this implementation?

Thanks,
Mani

Quoted 49 lines

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
author
#11 lore raw
Quoted 17 lines · Manivannan Sadhasivam
Quoted 9 lines · Ansuel Smith

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Quoted 6 lines

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

Quoted 6 lines

Can you please point me to the downstream/vendor driver that has this implementation?

Thanks,
Mani

Sure, is it good if I give you a gist link with the source of driver?

Quoted 52 lines
Quoted 49 lines

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Signature · 1 line
	Ansuel
author
#12 lore raw
In reply to Manivannan Sadhasivam
Quoted 17 lines · Manivannan Sadhasivam
Quoted 9 lines · Ansuel Smith

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Quoted 6 lines

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

Quoted 6 lines

Can you please point me to the downstream/vendor driver that has this implementation?

Thanks,
Mani

Actually found the repo...This is the link [1].

My implementation is a variant of this since originally they used a sysfs entry to swap the ecc configuration.

[1] https://github.com/marxfang/ipq807x-spf100-cs/blob/master/qsdk/qca/src/linux-4.4/drivers/mtd/nand/qcom_nandc.c

Quoted 52 lines
Quoted 49 lines

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Signature · 1 line
	Ansuel
maintainer
#13 lore raw
Quoted 25 lines · Ansuel Smith
Quoted 17 lines · Manivannan Sadhasivam
Quoted 9 lines · Ansuel Smith

Some background about this.
On original qsdk ipq8064 based firmware there was a big separation from boot partition and user partition. With boot partition we refer to partition used to init the router (bootloader, spm firmware and other internal stuff) With user partition we refer to linux partition and data partition not used to init the router.
When someone had to write to these boot partition a special mode was needed, to switch the nand driver to this special configuration.

Quoted 6 lines

Upstream version of the nandc driver totally dropped this and the result is that if someone try to read data from these partition a CRC warning is printed and if someone try to write that (if for example someone wants to replace the bootloader) result is a broken system as the data is badly written.

Quoted 6 lines

Can you please point me to the downstream/vendor driver that has this implementation?

Thanks,
Mani

Quoted 7 lines

Actually found the repo...This is the link [1].

My implementation is a variant of this since originally they used a sysfs entry to swap the ecc configuration.

[1] https://github.com/marxfang/ipq807x-spf100-cs/blob/master/qsdk/qca/src/linux-4.4/drivers/mtd/nand/qcom_nandc.c

Thanks for the link! After talking internally to Qcom folks, I confirmed that this quirk is only needed on IPQ8064 based platforms.

More in the driver patch...

Thanks,
Mani

Quoted 55 lines
Quoted 52 lines
Quoted 49 lines

This series comes to fix this.

A user can declare offset and size of these special partition using the qcom,boot-pages binding.

An initial implementation of this assumed that the boot-pages started from the start of the nand but we discover that some device have backup of these special partition and we can have situation where we have this partition scheme
- APPSBL (require special mode)
- APPSBLENV (doesn't require special mode)
- ART
- APPSBLBK (back of APPSBL require special mode)
- APPSBLENVBK (back of APPSBLENV doesn't require special mode) With this configuration we need to declare sparse boot page and we can't assume boot-pages always starts from the start of the nand.

A user can use this form to declare sparse boot pages qcom,boot-pages = <0x0 0x0c80000 0x0c80000 0x0500000>;

The driver internally will parse this array, convert it to nand pages and check internally on every read/write if this special configuration should used for that page or the normal one.

The reason for all of this is that qcom FOR SOME REASON, disable ECC for spare data only for these boot partition and we need to reflect this special configuration to mute these warning and to permit actually writing to these pages.

v4:
- Fix wrong compatible set for boot-pages (ipq8074 instead of ipq806x) v3:
- Fix typo in Docmunetation commit desription
- Add items description for uint32-matrix
v2:
- Add fixes from Krzysztof in Documentation

Ansuel Smith (2):
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-pages binding

 .../devicetree/bindings/mtd/qcom,nandc.yaml   |  26 +++
 drivers/mtd/nand/raw/qcom_nandc.c             | 148 +++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)
Signature · 1 line
2.34.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Signature · 1 line
	Ansuel
Signature · 1 line
மணிவண்ணன் சதாசிவம்