[PATCH v5 0/3] Add support for unprotected spare data page

[PATCH v5 0/3] Add support for unprotected spare data page

[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.

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.

v5:
- Rename boot-pages to boot-partitions
- Add additional check to parsing function
- Rename unprotect_spare_data to codeword_fixup
- Add additional info from Manivannan
- Add patch to remove holes in qcom_nand_host struct
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 (3):
  mtd: nand: raw: qcom_nandc: add support for unprotected spare data
    pages
  dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding
  mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

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

-- 
2.36.1

[PATCH v5 1/3] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

[Ansuel Smith]

IPQ8064 nand have special pages where a different layout scheme is used.
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.

The layout scheme used by these special page consist in using 512 bytes
as the codeword size (even for the last codeword) while writing to CFG0
register. This forces the NAND controller to unprotect the 4 bytes of
spare data.

Since the kernel is unaware of this different layout for these special
page, it does try to protect the spare data too during read/write and
warn about CRC errors.

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 layout is used.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 174 +++++++++++++++++++++++++++++-
 1 file changed, 169 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 1a77542c6d67..06ee9a836a3b 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -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 partitions
+ *
+ * @page_offset:		offset of the partition where spare data is not protected
+ *				by ECC (value in pages)
+ * @page_offset:		size of the partition where spare data is not protected
+ *				by ECC (value in pages)
+ */
+struct qcom_nand_boot_partition {
+	u32 page_offset;
+	u32 page_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
+ *
+ * @codeword_fixup:		keep track of the current layout used by
+ *				the driver for read/write operation.
+ * @nr_boot_partitions:		count of the boot partitions where spare data is not
+ *				protected by ECC
+ * @boot_pages:			array of boot partitions where offset and size of the
+ *				boot partitions 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 codeword_fixup;
+	int nr_boot_partitions;
+	struct qcom_nand_boot_partition *boot_partitions;
 };
 
 /*
@@ -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
+ * @use_codeword_fixup - whether NAND has different layout for boot partitions
  * @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 use_codeword_fixup;
 	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->codeword_fixup) {
 		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->codeword_fixup) {
 			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->codeword_fixup) {
 			data_size = ecc->size - ((ecc->steps - 1) << 2);
 			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
 				   host->spare_bytes;
@@ -2037,6 +2066,55 @@ 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_partition *boot_partition;
+	u32 start, end;
+	int i;
+
+	for (i = 0; i < host->nr_boot_partitions; i++) {
+		boot_partition = &host->boot_partitions[i];
+		start = boot_partition->page_offset;
+		end = start + boot_partition->page_size;
+
+		/* Boot pages are normally at the start of
+		 * the nand in various partition.
+		 * Check the page from the boot page end first
+		 * to save one extra check and optimize this
+		 * in case real no-boot partition are used.
+		 */
+		if (page < end && page >= start)
+			return true;
+	}
+
+	return false;
+}
+
+static void
+qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)
+{
+	bool codeword_fixup = qcom_nandc_is_boot_page(host, page);
+
+	/* Skip conf write if we are already in the correct mode */
+	if (codeword_fixup == host->codeword_fixup)
+		return;
+
+	host->codeword_fixup = codeword_fixup;
+
+	host->cw_data = codeword_fixup ? 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 = (codeword_fixup ? 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 +2123,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->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(host, page);
+
 	nand_read_page_op(chip, page, 0, NULL, 0);
 	data_buf = buf;
 	oob_buf = oob_required ? chip->oob_poi : NULL;
@@ -2064,6 +2145,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->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(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 +2168,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->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(host, page);
+
 	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);
 
@@ -2104,6 +2191,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->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(host, page);
+
 	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
 
 	clear_read_regs(nandc);
@@ -2119,7 +2209,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->codeword_fixup) {
 			data_size = ecc->size - ((ecc->steps - 1) << 2);
 			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
 				   host->spare_bytes;
@@ -2176,6 +2266,9 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
 	u8 *data_buf, *oob_buf;
 	int i, ret;
 
+	if (host->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(host, page);
+
 	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
 	clear_read_regs(nandc);
 	clear_bam_transaction(nandc);
@@ -2194,7 +2287,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->codeword_fixup) {
 			data_size2 = ecc->size - data_size1 -
 				     ((ecc->steps - 1) << 2);
 			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
@@ -2254,6 +2347,9 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
 	int data_size, oob_size;
 	int ret;
 
+	if (host->nr_boot_partitions)
+		qcom_nandc_codeword_fixup(host, page);
+
 	host->use_ecc = true;
 	clear_bam_transaction(nandc);
 
@@ -2902,6 +2998,71 @@ 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_partitions(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_partition *boot_partition;
+	struct device *dev = nandc->dev;
+	int partitions_count, i, j, ret;
+
+	if (!nandc->props->use_codeword_fixup)
+		return 0;
+
+	if (!of_find_property(dn, "qcom,boot-partitions", NULL))
+		return 0;
+
+	partitions_count = of_property_count_u32_elems(dn, "qcom,boot-partitions");
+	if (partitions_count < 0) {
+		dev_err(dev, "Error parsing boot partition.");
+		return ret;
+	}
+
+	host->nr_boot_partitions = partitions_count / 2;
+	host->boot_partitions = devm_kcalloc(dev, host->nr_boot_partitions,
+					     sizeof(*host->boot_partitions), GFP_KERNEL);
+	if (!host->boot_partitions)
+		return -ENOMEM;
+
+	for (i = 0, j = 0; i < host->nr_boot_partitions; i++, j += 2) {
+		boot_partition = &host->boot_partitions[i];
+
+		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j,
+						 &boot_partition->page_offset);
+		if (ret) {
+			dev_err(dev, "Error parsing boot partition offset at index %d", i);
+			return ret;
+		}
+
+		if (boot_partition->page_offset % mtd->writesize) {
+			dev_err(dev, "Boot partition offset not multiple of writesize at index %i",
+				i);
+			return -EINVAL;
+		}
+		/* Convert offset to nand pages */
+		boot_partition->page_offset /= mtd->writesize;
+
+		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j + 1,
+						 &boot_partition->page_size);
+		if (ret) {
+			dev_err(dev, "Error parsing boot partition size at index %d", i);
+			return ret;
+		}
+
+		if (boot_partition->page_size % mtd->writesize) {
+			dev_err(dev, "Boot partition size not multiple of writesize at index %i",
+				i);
+			return -EINVAL;
+		}
+		/* Convert size to nand pages */
+		boot_partition->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 +3131,8 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
 	if (ret)
 		nand_cleanup(chip);
 
+	qcom_nand_host_parse_boot_partitions(nandc, host, dn);
+
 	return ret;
 }
 
@@ -3135,6 +3298,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,
+	.use_codeword_fixup = true,
 	.dev_cmd_reg_start = 0x0,
 };
 
-- 
2.36.1

[PATCH v5 2/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

[Ansuel Smith]

Document new qcom,boot-partition binding used to apply special
read/write layout to boot partitions.

QCOM apply a special layout 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 <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
index 84ad7ff30121..a0914ccb95b0 100644
--- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
+++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
@@ -102,6 +102,30 @@ allOf:
             - const: rx
             - const: cmd
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-nand
+
+    then:
+      properties:
+        qcom,boot-partitions:
+          $ref: /schemas/types.yaml#/definitions/uint32-matrix
+          items:
+            items:
+              - description: offset
+              - description: size
+          description:
+            Boot partition use a different layout where the 4 bytes of spare
+            data are not protected by ECC. Use this to declare these special
+            partitions 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-partitions = <0x0 0x58a0000>;
+
         partitions {
           compatible = "fixed-partitions";
           #address-cells = <1>;
-- 
2.36.1

[PATCH v5 3/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

[Ansuel Smith]

Reorder qcom_nand_host to save holes in the struct.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 06ee9a836a3b..110f839c9e51 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -475,11 +475,13 @@ struct qcom_nand_host {
 	int cs;
 	int cw_size;
 	int cw_data;
-	bool use_ecc;
-	bool bch_enabled;
 	int ecc_bytes_hw;
 	int spare_bytes;
 	int bbm_size;
+
+	bool codeword_fixup;
+	bool use_ecc;
+	bool bch_enabled;
 	u8 status;
 	int last_command;
 
@@ -490,7 +492,6 @@ struct qcom_nand_host {
 	u32 clrflashstatus;
 	u32 clrreadstatus;
 
-	bool codeword_fixup;
 	int nr_boot_partitions;
 	struct qcom_nand_boot_partition *boot_partitions;
 };
-- 
2.36.1

Re: [PATCH v5 2/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

[Manivannan Sadhasivam]

On Wed, Jun 08, 2022 at 02:10:29AM +0200, Ansuel Smith wrote:
> Document new qcom,boot-partition binding used to apply special
> read/write layout to boot partitions.
> 
> QCOM apply a special layout 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 <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> index 84ad7ff30121..a0914ccb95b0 100644
> --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> @@ -102,6 +102,30 @@ allOf:
>              - const: rx
>              - const: cmd
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq806x-nand
> +
> +    then:
> +      properties:
> +        qcom,boot-partitions:
> +          $ref: /schemas/types.yaml#/definitions/uint32-matrix

Wondering if u32 is enough for covering all ranges? Other than this,

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> +          items:
> +            items:
> +              - description: offset
> +              - description: size
> +          description:
> +            Boot partition use a different layout where the 4 bytes of spare
> +            data are not protected by ECC. Use this to declare these special
> +            partitions 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-partitions = <0x0 0x58a0000>;
> +
>          partitions {
>            compatible = "fixed-partitions";
>            #address-cells = <1>;
> -- 
> 2.36.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

[Manivannan Sadhasivam]

On Wed, Jun 08, 2022 at 02:10:30AM +0200, Ansuel Smith wrote:
> Reorder qcom_nand_host to save holes in the struct.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

If this patch gets moved to 2/3, you could save few changes. Also, do the same
for other structs as well.

Thanks,
Mani

> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 06ee9a836a3b..110f839c9e51 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -475,11 +475,13 @@ struct qcom_nand_host {
>  	int cs;
>  	int cw_size;
>  	int cw_data;
> -	bool use_ecc;
> -	bool bch_enabled;
>  	int ecc_bytes_hw;
>  	int spare_bytes;
>  	int bbm_size;
> +
> +	bool codeword_fixup;
> +	bool use_ecc;
> +	bool bch_enabled;
>  	u8 status;
>  	int last_command;
>  
> @@ -490,7 +492,6 @@ struct qcom_nand_host {
>  	u32 clrflashstatus;
>  	u32 clrreadstatus;
>  
> -	bool codeword_fixup;
>  	int nr_boot_partitions;
>  	struct qcom_nand_boot_partition *boot_partitions;
>  };
> -- 
> 2.36.1
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Jun 08, 2022 at 02:10:28AM +0200, Ansuel Smith wrote:
> IPQ8064 nand have special pages where a different layout scheme is used.
> 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.
> 
> The layout scheme used by these special page consist in using 512 bytes
> as the codeword size (even for the last codeword) while writing to CFG0
> register. This forces the NAND controller to unprotect the 4 bytes of
> spare data.
> 
> Since the kernel is unaware of this different layout for these special
> page, it does try to protect the spare data too during read/write and
> warn about CRC errors.
> 
> 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 layout is used.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 174 +++++++++++++++++++++++++++++-
>  1 file changed, 169 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 1a77542c6d67..06ee9a836a3b 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -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 partitions
> + *
> + * @page_offset:		offset of the partition where spare data is not protected
> + *				by ECC (value in pages)

s/page_offset/offset

> + * @page_offset:		size of the partition where spare data is not protected
> + *				by ECC (value in pages)

s/page_offset/size

> + */
> +struct qcom_nand_boot_partition {
> +	u32 page_offset;
> +	u32 page_size;

same here

> +};
> +
>  /*
>   * 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
> + *
> + * @codeword_fixup:		keep track of the current layout used by
> + *				the driver for read/write operation.
> + * @nr_boot_partitions:		count of the boot partitions where spare data is not
> + *				protected by ECC

Align the Kdoc comments w.r.t other members.

> + * @boot_pages:			array of boot partitions where offset and size of the
> + *				boot partitions are stored

s/boot_pages/boot_partitions

>   */
>  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 codeword_fixup;
> +	int nr_boot_partitions;
> +	struct qcom_nand_boot_partition *boot_partitions;
>  };
>  
>  /*
> @@ -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
> + * @use_codeword_fixup - whether NAND has different layout for boot partitions
>   * @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 use_codeword_fixup;
>  	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->codeword_fixup) {
>  		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->codeword_fixup) {
>  			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->codeword_fixup) {
>  			data_size = ecc->size - ((ecc->steps - 1) << 2);
>  			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
>  				   host->spare_bytes;
> @@ -2037,6 +2066,55 @@ 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)

Move function name to previous line. If it exceeds 100 lines then wrap
arguments.

s/qcom_nandc_is_boot_page/qcom_nandc_is_boot_partition

> +{
> +	struct qcom_nand_boot_partition *boot_partition;
> +	u32 start, end;
> +	int i;
> +
> +	for (i = 0; i < host->nr_boot_partitions; i++) {
> +		boot_partition = &host->boot_partitions[i];
> +		start = boot_partition->page_offset;
> +		end = start + boot_partition->page_size;
> +
> +		/* Boot pages are normally at the start of

Block comments should start with:

	/*
	 * ...

Also, are you sure that only few pages in the partitions have different layout
and not all pages? If not, then this comment needs to be reworded.

> +		 * the nand in various partition.
> +		 * Check the page from the boot page end first
> +		 * to save one extra check and optimize this
> +		 * in case real no-boot partition are used.
> +		 */
> +		if (page < end && page >= start)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void
> +qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)
> +{
> +	bool codeword_fixup = qcom_nandc_is_boot_page(host, page);
> +
> +	/* Skip conf write if we are already in the correct mode */
> +	if (codeword_fixup == host->codeword_fixup)
> +		return;
> +
> +	host->codeword_fixup = codeword_fixup;
> +
> +	host->cw_data = codeword_fixup ? 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 = (codeword_fixup ? 0x1ff : 0x203) << NUM_STEPS;

s/1ff/(512 - 1)
s/203/(516 - 1)

> +}
> +
>  /* implements ecc->read_page() */
>  static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
>  				int oob_required, int page)
> @@ -2045,6 +2123,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->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(host, page);
> +
>  	nand_read_page_op(chip, page, 0, NULL, 0);
>  	data_buf = buf;
>  	oob_buf = oob_required ? chip->oob_poi : NULL;
> @@ -2064,6 +2145,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->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(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 +2168,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->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(host, page);
> +
>  	clear_read_regs(nandc);
>  	clear_bam_transaction(nandc);
>  
> @@ -2104,6 +2191,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->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(host, page);
> +
>  	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>  
>  	clear_read_regs(nandc);
> @@ -2119,7 +2209,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->codeword_fixup) {
>  			data_size = ecc->size - ((ecc->steps - 1) << 2);
>  			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
>  				   host->spare_bytes;
> @@ -2176,6 +2266,9 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
>  	u8 *data_buf, *oob_buf;
>  	int i, ret;
>  
> +	if (host->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(host, page);
> +
>  	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>  	clear_read_regs(nandc);
>  	clear_bam_transaction(nandc);
> @@ -2194,7 +2287,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->codeword_fixup) {
>  			data_size2 = ecc->size - data_size1 -
>  				     ((ecc->steps - 1) << 2);
>  			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
> @@ -2254,6 +2347,9 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
>  	int data_size, oob_size;
>  	int ret;
>  
> +	if (host->nr_boot_partitions)
> +		qcom_nandc_codeword_fixup(host, page);
> +
>  	host->use_ecc = true;
>  	clear_bam_transaction(nandc);
>  
> @@ -2902,6 +2998,71 @@ 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_partitions(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_partition *boot_partition;
> +	struct device *dev = nandc->dev;
> +	int partitions_count, i, j, ret;
> +
> +	if (!nandc->props->use_codeword_fixup)
> +		return 0;

Move this check to caller as I suggested previously.

> +
> +	if (!of_find_property(dn, "qcom,boot-partitions", NULL))
> +		return 0;
> +
> +	partitions_count = of_property_count_u32_elems(dn, "qcom,boot-partitions");
> +	if (partitions_count < 0) {

partitions_count <= 0

> +		dev_err(dev, "Error parsing boot partition.");

Add newline at the end of error message

> +		return ret;
> +	}
> +
> +	host->nr_boot_partitions = partitions_count / 2;
> +	host->boot_partitions = devm_kcalloc(dev, host->nr_boot_partitions,
> +					     sizeof(*host->boot_partitions), GFP_KERNEL);
> +	if (!host->boot_partitions)

host->nr_boot_partitions = 0;

> +		return -ENOMEM;
> +
> +	for (i = 0, j = 0; i < host->nr_boot_partitions; i++, j += 2) {
> +		boot_partition = &host->boot_partitions[i];
> +
> +		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j,
> +						 &boot_partition->page_offset);
> +		if (ret) {
> +			dev_err(dev, "Error parsing boot partition offset at index %d", i);

Add newline at the end of error message. Do the same for all error prints.

> +			return ret;
> +		}
> +
> +		if (boot_partition->page_offset % mtd->writesize) {
> +			dev_err(dev, "Boot partition offset not multiple of writesize at index %i",
> +				i);
> +			return -EINVAL;
> +		}
> +		/* Convert offset to nand pages */

s/pages/partitions

> +		boot_partition->page_offset /= mtd->writesize;

s/page_offset/offset

> +
> +		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j + 1,
> +						 &boot_partition->page_size);
> +		if (ret) {
> +			dev_err(dev, "Error parsing boot partition size at index %d", i);
> +			return ret;
> +		}
> +
> +		if (boot_partition->page_size % mtd->writesize) {

s/page_size/size here and below

> +			dev_err(dev, "Boot partition size not multiple of writesize at index %i",
> +				i);
> +			return -EINVAL;
> +		}
> +		/* Convert size to nand pages */

s/pages/partitions

Thanks,
Mani

> +		boot_partition->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 +3131,8 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
>  	if (ret)
>  		nand_cleanup(chip);
>  
> +	qcom_nand_host_parse_boot_partitions(nandc, host, dn);
> +
>  	return ret;
>  }
>  
> @@ -3135,6 +3298,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,
> +	.use_codeword_fixup = true,
>  	.dev_cmd_reg_start = 0x0,
>  };
>  
> -- 
> 2.36.1
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Ansuel Smith]

On Thu, Jun 09, 2022 at 01:22:54PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 08, 2022 at 02:10:28AM +0200, Ansuel Smith wrote:
> > IPQ8064 nand have special pages where a different layout scheme is used.
> > 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.
> > 
> > The layout scheme used by these special page consist in using 512 bytes
> > as the codeword size (even for the last codeword) while writing to CFG0
> > register. This forces the NAND controller to unprotect the 4 bytes of
> > spare data.
> > 
> > Since the kernel is unaware of this different layout for these special
> > page, it does try to protect the spare data too during read/write and
> > warn about CRC errors.
> > 
> > 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 layout is used.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/mtd/nand/raw/qcom_nandc.c | 174 +++++++++++++++++++++++++++++-
> >  1 file changed, 169 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 1a77542c6d67..06ee9a836a3b 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -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 partitions
> > + *
> > + * @page_offset:		offset of the partition where spare data is not protected
> > + *				by ECC (value in pages)
> 
> s/page_offset/offset
>

Mhhh i changed this to page_offset since the struct is called
boot_partition and it can be confusing since internally the values is in
pages. Someone can think that the real partition offset/size is used
instead of the value converted in pages.

> > + * @page_offset:		size of the partition where spare data is not protected
> > + *				by ECC (value in pages)
> 
> s/page_offset/size
> 
> > + */
> > +struct qcom_nand_boot_partition {
> > +	u32 page_offset;
> > +	u32 page_size;
> 
> same here
> 
> > +};
> > +
> >  /*
> >   * 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
> > + *
> > + * @codeword_fixup:		keep track of the current layout used by
> > + *				the driver for read/write operation.
> > + * @nr_boot_partitions:		count of the boot partitions where spare data is not
> > + *				protected by ECC
> 
> Align the Kdoc comments w.r.t other members.
> 
> > + * @boot_pages:			array of boot partitions where offset and size of the
> > + *				boot partitions are stored
> 
> s/boot_pages/boot_partitions
> 

Sorry.

> >   */
> >  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 codeword_fixup;
> > +	int nr_boot_partitions;
> > +	struct qcom_nand_boot_partition *boot_partitions;
> >  };
> >  
> >  /*
> > @@ -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
> > + * @use_codeword_fixup - whether NAND has different layout for boot partitions
> >   * @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 use_codeword_fixup;
> >  	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->codeword_fixup) {
> >  		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->codeword_fixup) {
> >  			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->codeword_fixup) {
> >  			data_size = ecc->size - ((ecc->steps - 1) << 2);
> >  			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
> >  				   host->spare_bytes;
> > @@ -2037,6 +2066,55 @@ 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)
> 
> Move function name to previous line. If it exceeds 100 lines then wrap
> arguments.
> 
> s/qcom_nandc_is_boot_page/qcom_nandc_is_boot_partition
> 

Again considering the check is done on the page and not on the
partition, wonder what is correct.

> > +{
> > +	struct qcom_nand_boot_partition *boot_partition;
> > +	u32 start, end;
> > +	int i;
> > +
> > +	for (i = 0; i < host->nr_boot_partitions; i++) {
> > +		boot_partition = &host->boot_partitions[i];
> > +		start = boot_partition->page_offset;
> > +		end = start + boot_partition->page_size;
> > +
> > +		/* Boot pages are normally at the start of
> 
> Block comments should start with:
> 
> 	/*
> 	 * ...
> 
> Also, are you sure that only few pages in the partitions have different layout
> and not all pages? If not, then this comment needs to be reworded.
> 

I probably worderd this bad...
Each page of the boot partitions require the codeword fix.
But other may have the normal codeword
For example this is from a Netgear router:
mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
mtd3: 00140000 00020000 "art"       (doesn't)
mtd4: 00140000 00020000 "artbak"    (doesn't)
mtd5: 00400000 00020000 "kernel"    (doesn't)
mtd6: 06080000 00020000 "ubi"       (doesn't)
mtd7: 00700000 00020000 "reserve"   (doesn't)

This is part of the case tho as there are some partition table with
something like

mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
mtd3: 00140000 00020000 "APPSBL2"  (require codeword fix)
[...]

In all case 99% of the time in a normal system what you commonly write
are the rootfs partition that are AFTER the boot partition so checking
if the page is after such partition should speed up the check.
Now that I think about it, a good idea is also start checking the
boot_partition table from the end to improve the checking even more.

So tell me if the comments is wronlgy worded.

> > +		 * the nand in various partition.
> > +		 * Check the page from the boot page end first
> > +		 * to save one extra check and optimize this
> > +		 * in case real no-boot partition are used.
> > +		 */
> > +		if (page < end && page >= start)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static void
> > +qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)
> > +{
> > +	bool codeword_fixup = qcom_nandc_is_boot_page(host, page);
> > +
> > +	/* Skip conf write if we are already in the correct mode */
> > +	if (codeword_fixup == host->codeword_fixup)
> > +		return;
> > +
> > +	host->codeword_fixup = codeword_fixup;
> > +
> > +	host->cw_data = codeword_fixup ? 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 = (codeword_fixup ? 0x1ff : 0x203) << NUM_STEPS;
> 
> s/1ff/(512 - 1)
> s/203/(516 - 1)
> 

ok. I wonder if this can just be reduced to (host->cw_data - 1) and
skipping the check entirely.

> > +}
> > +
> >  /* implements ecc->read_page() */
> >  static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
> >  				int oob_required, int page)
> > @@ -2045,6 +2123,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->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(host, page);
> > +
> >  	nand_read_page_op(chip, page, 0, NULL, 0);
> >  	data_buf = buf;
> >  	oob_buf = oob_required ? chip->oob_poi : NULL;
> > @@ -2064,6 +2145,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->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(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 +2168,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->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(host, page);
> > +
> >  	clear_read_regs(nandc);
> >  	clear_bam_transaction(nandc);
> >  
> > @@ -2104,6 +2191,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->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(host, page);
> > +
> >  	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> >  
> >  	clear_read_regs(nandc);
> > @@ -2119,7 +2209,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->codeword_fixup) {
> >  			data_size = ecc->size - ((ecc->steps - 1) << 2);
> >  			oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
> >  				   host->spare_bytes;
> > @@ -2176,6 +2266,9 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
> >  	u8 *data_buf, *oob_buf;
> >  	int i, ret;
> >  
> > +	if (host->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(host, page);
> > +
> >  	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> >  	clear_read_regs(nandc);
> >  	clear_bam_transaction(nandc);
> > @@ -2194,7 +2287,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->codeword_fixup) {
> >  			data_size2 = ecc->size - data_size1 -
> >  				     ((ecc->steps - 1) << 2);
> >  			oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
> > @@ -2254,6 +2347,9 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
> >  	int data_size, oob_size;
> >  	int ret;
> >  
> > +	if (host->nr_boot_partitions)
> > +		qcom_nandc_codeword_fixup(host, page);
> > +
> >  	host->use_ecc = true;
> >  	clear_bam_transaction(nandc);
> >  
> > @@ -2902,6 +2998,71 @@ 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_partitions(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_partition *boot_partition;
> > +	struct device *dev = nandc->dev;
> > +	int partitions_count, i, j, ret;
> > +
> > +	if (!nandc->props->use_codeword_fixup)
> > +		return 0;
> 
> Move this check to caller as I suggested previously.
> 

Sorry I missed that.

> > +
> > +	if (!of_find_property(dn, "qcom,boot-partitions", NULL))
> > +		return 0;
> > +
> > +	partitions_count = of_property_count_u32_elems(dn, "qcom,boot-partitions");
> > +	if (partitions_count < 0) {
> 
> partitions_count <= 0
> 
> > +		dev_err(dev, "Error parsing boot partition.");
> 
> Add newline at the end of error message
> 
> > +		return ret;
> > +	}
> > +
> > +	host->nr_boot_partitions = partitions_count / 2;
> > +	host->boot_partitions = devm_kcalloc(dev, host->nr_boot_partitions,
> > +					     sizeof(*host->boot_partitions), GFP_KERNEL);
> > +	if (!host->boot_partitions)
> 
> host->nr_boot_partitions = 0;
> 
> > +		return -ENOMEM;
> > +
> > +	for (i = 0, j = 0; i < host->nr_boot_partitions; i++, j += 2) {
> > +		boot_partition = &host->boot_partitions[i];
> > +
> > +		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j,
> > +						 &boot_partition->page_offset);
> > +		if (ret) {
> > +			dev_err(dev, "Error parsing boot partition offset at index %d", i);
> 
> Add newline at the end of error message. Do the same for all error prints.
> 
> > +			return ret;
> > +		}
> > +
> > +		if (boot_partition->page_offset % mtd->writesize) {
> > +			dev_err(dev, "Boot partition offset not multiple of writesize at index %i",
> > +				i);
> > +			return -EINVAL;
> > +		}
> > +		/* Convert offset to nand pages */
> 
> s/pages/partitions
> 
> > +		boot_partition->page_offset /= mtd->writesize;
> 
> s/page_offset/offset
> 
> > +
> > +		ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j + 1,
> > +						 &boot_partition->page_size);
> > +		if (ret) {
> > +			dev_err(dev, "Error parsing boot partition size at index %d", i);
> > +			return ret;
> > +		}
> > +
> > +		if (boot_partition->page_size % mtd->writesize) {
> 
> s/page_size/size here and below
> 
> > +			dev_err(dev, "Boot partition size not multiple of writesize at index %i",
> > +				i);
> > +			return -EINVAL;
> > +		}
> > +		/* Convert size to nand pages */
> 
> s/pages/partitions
> 
> Thanks,
> Mani
> 
> > +		boot_partition->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 +3131,8 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
> >  	if (ret)
> >  		nand_cleanup(chip);
> >  
> > +	qcom_nand_host_parse_boot_partitions(nandc, host, dn);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -3135,6 +3298,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,
> > +	.use_codeword_fixup = true,
> >  	.dev_cmd_reg_start = 0x0,
> >  };
> >  
> > -- 
> > 2.36.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
	Ansuel

Re: [PATCH v5 2/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

[Ansuel Smith]

On Thu, Jun 09, 2022 at 12:50:29PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 08, 2022 at 02:10:29AM +0200, Ansuel Smith wrote:
> > Document new qcom,boot-partition binding used to apply special
> > read/write layout to boot partitions.
> > 
> > QCOM apply a special layout 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 <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > index 84ad7ff30121..a0914ccb95b0 100644
> > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > @@ -102,6 +102,30 @@ allOf:
> >              - const: rx
> >              - const: cmd
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,ipq806x-nand
> > +
> > +    then:
> > +      properties:
> > +        qcom,boot-partitions:
> > +          $ref: /schemas/types.yaml#/definitions/uint32-matrix
> 
> Wondering if u32 is enough for covering all ranges? Other than this,
> 
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> Thanks,
> Mani
>

I mean they are offset and sizes... Considering it's an old SoC and max
nand mounted is 1g we should be safe with u32.

> > +          items:
> > +            items:
> > +              - description: offset
> > +              - description: size
> > +          description:
> > +            Boot partition use a different layout where the 4 bytes of spare
> > +            data are not protected by ECC. Use this to declare these special
> > +            partitions 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-partitions = <0x0 0x58a0000>;
> > +
> >          partitions {
> >            compatible = "fixed-partitions";
> >            #address-cells = <1>;
> > -- 
> > 2.36.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
	Ansuel

Re: [PATCH v5 3/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

[Ansuel Smith]

On Thu, Jun 09, 2022 at 12:52:40PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 08, 2022 at 02:10:30AM +0200, Ansuel Smith wrote:
> > Reorder qcom_nand_host to save holes in the struct.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> If this patch gets moved to 2/3, you could save few changes. Also, do the same
> for other structs as well.
> 
> Thanks,
> Mani
>

Since 2/3 already had lots of changes didn't want to put a struct
reorder in it since it does touch also other values. Tell me if I should
squash the 2 commit.

> > ---
> >  drivers/mtd/nand/raw/qcom_nandc.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 06ee9a836a3b..110f839c9e51 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -475,11 +475,13 @@ struct qcom_nand_host {
> >  	int cs;
> >  	int cw_size;
> >  	int cw_data;
> > -	bool use_ecc;
> > -	bool bch_enabled;
> >  	int ecc_bytes_hw;
> >  	int spare_bytes;
> >  	int bbm_size;
> > +
> > +	bool codeword_fixup;
> > +	bool use_ecc;
> > +	bool bch_enabled;
> >  	u8 status;
> >  	int last_command;
> >  
> > @@ -490,7 +492,6 @@ struct qcom_nand_host {
> >  	u32 clrflashstatus;
> >  	u32 clrreadstatus;
> >  
> > -	bool codeword_fixup;
> >  	int nr_boot_partitions;
> >  	struct qcom_nand_boot_partition *boot_partitions;
> >  };
> > -- 
> > 2.36.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
	Ansuel

Re: [PATCH v5 2/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

[Manivannan Sadhasivam]

On Thu, Jun 09, 2022 at 12:25:27PM +0200, Ansuel Smith wrote:
> On Thu, Jun 09, 2022 at 12:50:29PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 08, 2022 at 02:10:29AM +0200, Ansuel Smith wrote:
> > > Document new qcom,boot-partition binding used to apply special
> > > read/write layout to boot partitions.
> > > 
> > > QCOM apply a special layout 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 <ansuelsmth@gmail.com>
> > > ---
> > >  .../devicetree/bindings/mtd/qcom,nandc.yaml   | 26 +++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > index 84ad7ff30121..a0914ccb95b0 100644
> > > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > @@ -102,6 +102,30 @@ allOf:
> > >              - const: rx
> > >              - const: cmd
> > >  
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - qcom,ipq806x-nand
> > > +
> > > +    then:
> > > +      properties:
> > > +        qcom,boot-partitions:
> > > +          $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > 
> > Wondering if u32 is enough for covering all ranges? Other than this,
> > 
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > Thanks,
> > Mani
> >
> 
> I mean they are offset and sizes... Considering it's an old SoC and max
> nand mounted is 1g we should be safe with u32.
> 

I thought so but wanted to confirm...

Thanks,
Mani

> > > +          items:
> > > +            items:
> > > +              - description: offset
> > > +              - description: size
> > > +          description:
> > > +            Boot partition use a different layout where the 4 bytes of spare
> > > +            data are not protected by ECC. Use this to declare these special
> > > +            partitions 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-partitions = <0x0 0x58a0000>;
> > > +
> > >          partitions {
> > >            compatible = "fixed-partitions";
> > >            #address-cells = <1>;
> > > -- 
> > > 2.36.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
> 
> -- 
> 	Ansuel

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

[Manivannan Sadhasivam]

On Thu, Jun 09, 2022 at 12:26:53PM +0200, Ansuel Smith wrote:
> On Thu, Jun 09, 2022 at 12:52:40PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 08, 2022 at 02:10:30AM +0200, Ansuel Smith wrote:
> > > Reorder qcom_nand_host to save holes in the struct.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > 
> > If this patch gets moved to 2/3, you could save few changes. Also, do the same
> > for other structs as well.
> > 
> > Thanks,
> > Mani
> >
> 
> Since 2/3 already had lots of changes didn't want to put a struct
> reorder in it since it does touch also other values. Tell me if I should
> squash the 2 commit.
> 

Not squashing but I mean to move this patch before the driver change (1/3), so
that "codeword_fixup" can be added in the correct place.

Also move the respective Kdoc comments.

Thanks,
Mani

> > > ---
> > >  drivers/mtd/nand/raw/qcom_nandc.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > > index 06ee9a836a3b..110f839c9e51 100644
> > > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > > @@ -475,11 +475,13 @@ struct qcom_nand_host {
> > >  	int cs;
> > >  	int cw_size;
> > >  	int cw_data;
> > > -	bool use_ecc;
> > > -	bool bch_enabled;
> > >  	int ecc_bytes_hw;
> > >  	int spare_bytes;
> > >  	int bbm_size;
> > > +
> > > +	bool codeword_fixup;
> > > +	bool use_ecc;
> > > +	bool bch_enabled;
> > >  	u8 status;
> > >  	int last_command;
> > >  
> > > @@ -490,7 +492,6 @@ struct qcom_nand_host {
> > >  	u32 clrflashstatus;
> > >  	u32 clrreadstatus;
> > >  
> > > -	bool codeword_fixup;
> > >  	int nr_boot_partitions;
> > >  	struct qcom_nand_boot_partition *boot_partitions;
> > >  };
> > > -- 
> > > 2.36.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
> 
> -- 
> 	Ansuel

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Thu, Jun 09, 2022 at 12:24:22PM +0200, Ansuel Smith wrote:
> On Thu, Jun 09, 2022 at 01:22:54PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 08, 2022 at 02:10:28AM +0200, Ansuel Smith wrote:
> > > IPQ8064 nand have special pages where a different layout scheme is used.
> > > 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.
> > > 
> > > The layout scheme used by these special page consist in using 512 bytes
> > > as the codeword size (even for the last codeword) while writing to CFG0
> > > register. This forces the NAND controller to unprotect the 4 bytes of
> > > spare data.
> > > 
> > > Since the kernel is unaware of this different layout for these special
> > > page, it does try to protect the spare data too during read/write and
> > > warn about CRC errors.
> > > 
> > > 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 layout is used.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/mtd/nand/raw/qcom_nandc.c | 174 +++++++++++++++++++++++++++++-
> > >  1 file changed, 169 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > > index 1a77542c6d67..06ee9a836a3b 100644
> > > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > > @@ -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 partitions
> > > + *
> > > + * @page_offset:		offset of the partition where spare data is not protected
> > > + *				by ECC (value in pages)
> > 
> > s/page_offset/offset
> >
> 
> Mhhh i changed this to page_offset since the struct is called
> boot_partition and it can be confusing since internally the values is in
> pages. Someone can think that the real partition offset/size is used
> instead of the value converted in pages.

Hmm, okay.

> 
> > > + * @page_offset:		size of the partition where spare data is not protected
> > > + *				by ECC (value in pages)
> > 

[...]

> > > +static bool
> > > +qcom_nandc_is_boot_page(struct qcom_nand_host *host, int page)
> > 
> > Move function name to previous line. If it exceeds 100 lines then wrap
> > arguments.
> > 
> > s/qcom_nandc_is_boot_page/qcom_nandc_is_boot_partition
> > 
> 
> Again considering the check is done on the page and not on the
> partition, wonder what is correct.
> 

But the end goal is to detect if the page belongs to boot partitionis, right?

Also, mixing up boot_pages and boot_partitions is confusing. Let's stick to the
fact that this patch checks for pages belonging to the boot partition.

> > > +{
> > > +	struct qcom_nand_boot_partition *boot_partition;
> > > +	u32 start, end;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < host->nr_boot_partitions; i++) {
> > > +		boot_partition = &host->boot_partitions[i];
> > > +		start = boot_partition->page_offset;
> > > +		end = start + boot_partition->page_size;
> > > +
> > > +		/* Boot pages are normally at the start of
> > 
> > Block comments should start with:
> > 
> > 	/*
> > 	 * ...
> > 
> > Also, are you sure that only few pages in the partitions have different layout
> > and not all pages? If not, then this comment needs to be reworded.
> > 
> 
> I probably worderd this bad...
> Each page of the boot partitions require the codeword fix.
> But other may have the normal codeword
> For example this is from a Netgear router:
> mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
> mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
> mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
> mtd3: 00140000 00020000 "art"       (doesn't)
> mtd4: 00140000 00020000 "artbak"    (doesn't)
> mtd5: 00400000 00020000 "kernel"    (doesn't)
> mtd6: 06080000 00020000 "ubi"       (doesn't)
> mtd7: 00700000 00020000 "reserve"   (doesn't)
> 
> This is part of the case tho as there are some partition table with
> something like
> 
> mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
> mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
> mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
> mtd3: 00140000 00020000 "APPSBL2"  (require codeword fix)
> [...]
> 
> In all case 99% of the time in a normal system what you commonly write
> are the rootfs partition that are AFTER the boot partition so checking
> if the page is after such partition should speed up the check.

This should be included in the comment.

> Now that I think about it, a good idea is also start checking the
> boot_partition table from the end to improve the checking even more.
> 

Yes and if the page lies after last boot partition then we can bail out in the
first iteration itself. But the binding should make it clear that the offset
should be specified in ascending order.

> So tell me if the comments is wronlgy worded.
> 

Based on the above algorithm:

/*
 * Since the frequent access will be to the non-boot partitions like rootfs,
 * optimize the page check by:

 * 1. Checking if the page lies after the last boot partition.
 * 2. Checking from the boot partition end.
 */

> > > +		 * the nand in various partition.
> > > +		 * Check the page from the boot page end first
> > > +		 * to save one extra check and optimize this
> > > +		 * in case real no-boot partition are used.
> > > +		 */
> > > +		if (page < end && page >= start)
> > > +			return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static void
> > > +qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)
> > > +{
> > > +	bool codeword_fixup = qcom_nandc_is_boot_page(host, page);
> > > +
> > > +	/* Skip conf write if we are already in the correct mode */
> > > +	if (codeword_fixup == host->codeword_fixup)
> > > +		return;
> > > +
> > > +	host->codeword_fixup = codeword_fixup;
> > > +
> > > +	host->cw_data = codeword_fixup ? 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 = (codeword_fixup ? 0x1ff : 0x203) << NUM_STEPS;
> > 
> > s/1ff/(512 - 1)
> > s/203/(516 - 1)
> > 
> 
> ok. I wonder if this can just be reduced to (host->cw_data - 1) and
> skipping the check entirely.
> 

Yeah, that sounds even good.

Thanks,
Mani

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

[kernel test robot]

Hi Ansuel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mtd/nand/next]
[also build test WARNING on mtd/mtd/next mtd/mtd/fixes robh/for-next v5.19-rc2 next-20220610]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Ansuel-Smith/Add-support-for-unprotected-spare-data-page/20220608-104834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
config: hexagon-randconfig-r041-20220613 (https://download.01.org/0day-ci/archive/20220613/202206132205.G3tGFPx7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d378268ead93c85803c270277f0243737b536ae7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5f9263b88e99a6cae44be5e737cb0928ee420e87
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ansuel-Smith/Add-support-for-unprotected-spare-data-page/20220608-104834
        git checkout 5f9263b88e99a6cae44be5e737cb0928ee420e87
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/mtd/nand/raw/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mtd/nand/raw/qcom_nandc.c:3020:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   return ret;
                          ^~~
   drivers/mtd/nand/raw/qcom_nandc.c:3009:33: note: initialize the variable 'ret' to silence this warning
           int partitions_count, i, j, ret;
                                          ^
                                           = 0
   1 warning generated.


vim +/ret +3020 drivers/mtd/nand/raw/qcom_nandc.c

  3000	
  3001	static int qcom_nand_host_parse_boot_partitions(struct qcom_nand_controller *nandc,
  3002							struct qcom_nand_host *host,
  3003							struct device_node *dn)
  3004	{
  3005		struct nand_chip *chip = &host->chip;
  3006		struct mtd_info *mtd = nand_to_mtd(chip);
  3007		struct qcom_nand_boot_partition *boot_partition;
  3008		struct device *dev = nandc->dev;
  3009		int partitions_count, i, j, ret;
  3010	
  3011		if (!nandc->props->use_codeword_fixup)
  3012			return 0;
  3013	
  3014		if (!of_find_property(dn, "qcom,boot-partitions", NULL))
  3015			return 0;
  3016	
  3017		partitions_count = of_property_count_u32_elems(dn, "qcom,boot-partitions");
  3018		if (partitions_count < 0) {
  3019			dev_err(dev, "Error parsing boot partition.");
> 3020			return ret;
  3021		}
  3022	
  3023		host->nr_boot_partitions = partitions_count / 2;
  3024		host->boot_partitions = devm_kcalloc(dev, host->nr_boot_partitions,
  3025						     sizeof(*host->boot_partitions), GFP_KERNEL);
  3026		if (!host->boot_partitions)
  3027			return -ENOMEM;
  3028	
  3029		for (i = 0, j = 0; i < host->nr_boot_partitions; i++, j += 2) {
  3030			boot_partition = &host->boot_partitions[i];
  3031	
  3032			ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j,
  3033							 &boot_partition->page_offset);
  3034			if (ret) {
  3035				dev_err(dev, "Error parsing boot partition offset at index %d", i);
  3036				return ret;
  3037			}
  3038	
  3039			if (boot_partition->page_offset % mtd->writesize) {
  3040				dev_err(dev, "Boot partition offset not multiple of writesize at index %i",
  3041					i);
  3042				return -EINVAL;
  3043			}
  3044			/* Convert offset to nand pages */
  3045			boot_partition->page_offset /= mtd->writesize;
  3046	
  3047			ret = of_property_read_u32_index(dn, "qcom,boot-partitions", j + 1,
  3048							 &boot_partition->page_size);
  3049			if (ret) {
  3050				dev_err(dev, "Error parsing boot partition size at index %d", i);
  3051				return ret;
  3052			}
  3053	
  3054			if (boot_partition->page_size % mtd->writesize) {
  3055				dev_err(dev, "Boot partition size not multiple of writesize at index %i",
  3056					i);
  3057				return -EINVAL;
  3058			}
  3059			/* Convert size to nand pages */
  3060			boot_partition->page_size /= mtd->writesize;
  3061		}
  3062	
  3063		return 0;
  3064	}
  3065	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp