2022-06-16 00:20:37

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v8 0/3] 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-partitions = <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.

v8:
- Removed all review tag (sob changed)
- Change sob from Ansuel Smith to Christian Marangi
- Address small fixup suggested by Manivannan
v7:
- Add missing nand_cleanup
- Fix wrong error return
- Actually implement suggested boot_partition check
- Remove unnecessary comments previously added
- Add missing new line in dev_err
- Reorder structs with suggested order
v6:
- Add additional comments on boot partition check
- First reorder struct then make change
- Add additional changes request from Manivannan
- Add review tag for dt commit
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

Christian Marangi (3):
mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct
mtd: nand: raw: qcom_nandc: add support for unprotected spare data
pages
dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

.../devicetree/bindings/mtd/qcom,nandc.yaml | 27 ++
drivers/mtd/nand/raw/qcom_nandc.c | 306 +++++++++++++++---
2 files changed, 283 insertions(+), 50 deletions(-)

--
2.36.1


2022-06-16 00:20:39

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v8 1/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

Reorder structs in nandc driver to save holes.

Signed-off-by: Christian Marangi <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 107 +++++++++++++++++-------------
1 file changed, 62 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 1a77542c6d67..f2990d721733 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -238,6 +238,9 @@ nandc_set_reg(chip, reg, \
* @bam_ce - the array of BAM command elements
* @cmd_sgl - sgl for NAND BAM command pipe
* @data_sgl - sgl for NAND BAM consumer/producer pipe
+ * @last_data_desc - last DMA desc in data channel (tx/rx).
+ * @last_cmd_desc - last DMA desc in command channel.
+ * @txn_done - completion for NAND transfer.
* @bam_ce_pos - the index in bam_ce which is available for next sgl
* @bam_ce_start - the index in bam_ce which marks the start position ce
* for current sgl. It will be used for size calculation
@@ -250,14 +253,14 @@ nandc_set_reg(chip, reg, \
* @rx_sgl_start - start index in data sgl for rx.
* @wait_second_completion - wait for second DMA desc completion before making
* the NAND transfer completion.
- * @txn_done - completion for NAND transfer.
- * @last_data_desc - last DMA desc in data channel (tx/rx).
- * @last_cmd_desc - last DMA desc in command channel.
*/
struct bam_transaction {
struct bam_cmd_element *bam_ce;
struct scatterlist *cmd_sgl;
struct scatterlist *data_sgl;
+ struct dma_async_tx_descriptor *last_data_desc;
+ struct dma_async_tx_descriptor *last_cmd_desc;
+ struct completion txn_done;
u32 bam_ce_pos;
u32 bam_ce_start;
u32 cmd_sgl_pos;
@@ -267,25 +270,23 @@ struct bam_transaction {
u32 rx_sgl_pos;
u32 rx_sgl_start;
bool wait_second_completion;
- struct completion txn_done;
- struct dma_async_tx_descriptor *last_data_desc;
- struct dma_async_tx_descriptor *last_cmd_desc;
};

/*
* This data type corresponds to the nand dma descriptor
+ * @dma_desc - low level DMA engine descriptor
* @list - list for desc_info
- * @dir - DMA transfer direction
+ *
* @adm_sgl - sgl which will be used for single sgl dma descriptor. Only used by
* ADM
* @bam_sgl - sgl which will be used for dma descriptor. Only used by BAM
* @sgl_cnt - number of SGL in bam_sgl. Only used by BAM
- * @dma_desc - low level DMA engine descriptor
+ * @dir - DMA transfer direction
*/
struct desc_info {
+ struct dma_async_tx_descriptor *dma_desc;
struct list_head node;

- enum dma_data_direction dir;
union {
struct scatterlist adm_sgl;
struct {
@@ -293,7 +294,7 @@ struct desc_info {
int sgl_cnt;
};
};
- struct dma_async_tx_descriptor *dma_desc;
+ enum dma_data_direction dir;
};

/*
@@ -337,52 +338,64 @@ struct nandc_regs {
/*
* NAND controller data struct
*
- * @controller: base controller structure
- * @host_list: list containing all the chips attached to the
- * controller
* @dev: parent device
+ *
* @base: MMIO base
- * @base_phys: physical base address of controller registers
- * @base_dma: dma base address of controller registers
+ *
* @core_clk: controller clock
* @aon_clk: another controller clock
*
+ * @regs: a contiguous chunk of memory for DMA register
+ * writes. contains the register values to be
+ * written to controller
+ *
+ * @props: properties of current NAND controller,
+ * initialized via DT match data
+ *
+ * @controller: base controller structure
+ * @host_list: list containing all the chips attached to the
+ * controller
+ *
* @chan: dma channel
* @cmd_crci: ADM DMA CRCI for command flow control
* @data_crci: ADM DMA CRCI for data flow control
+ *
* @desc_list: DMA descriptor list (list of desc_infos)
*
* @data_buffer: our local DMA buffer for page read/writes,
* used when we can't use the buffer provided
* by upper layers directly
- * @buf_size/count/start: markers for chip->legacy.read_buf/write_buf
- * functions
* @reg_read_buf: local buffer for reading back registers via DMA
+ *
+ * @base_phys: physical base address of controller registers
+ * @base_dma: dma base address of controller registers
* @reg_read_dma: contains dma address for register read buffer
- * @reg_read_pos: marker for data read in reg_read_buf
*
- * @regs: a contiguous chunk of memory for DMA register
- * writes. contains the register values to be
- * written to controller
- * @cmd1/vld: some fixed controller register values
- * @props: properties of current NAND controller,
- * initialized via DT match data
+ * @buf_size/count/start: markers for chip->legacy.read_buf/write_buf
+ * functions
* @max_cwperpage: maximum QPIC codewords required. calculated
* from all connected NAND devices pagesize
+ *
+ * @reg_read_pos: marker for data read in reg_read_buf
+ *
+ * @cmd1/vld: some fixed controller register values
*/
struct qcom_nand_controller {
- struct nand_controller controller;
- struct list_head host_list;
-
struct device *dev;

void __iomem *base;
- phys_addr_t base_phys;
- dma_addr_t base_dma;

struct clk *core_clk;
struct clk *aon_clk;

+ struct nandc_regs *regs;
+ struct bam_transaction *bam_txn;
+
+ const struct qcom_nandc_props *props;
+
+ struct nand_controller controller;
+ struct list_head host_list;
+
union {
/* will be used only by QPIC for BAM DMA */
struct {
@@ -400,22 +413,22 @@ struct qcom_nand_controller {
};

struct list_head desc_list;
- struct bam_transaction *bam_txn;

u8 *data_buffer;
+ __le32 *reg_read_buf;
+
+ phys_addr_t base_phys;
+ dma_addr_t base_dma;
+ dma_addr_t reg_read_dma;
+
int buf_size;
int buf_count;
int buf_start;
unsigned int max_cwperpage;

- __le32 *reg_read_buf;
- dma_addr_t reg_read_dma;
int reg_read_pos;

- struct nandc_regs *regs;
-
u32 cmd1, vld;
- const struct qcom_nandc_props *props;
};

/*
@@ -431,19 +444,21 @@ struct qcom_nand_controller {
* and reserved bytes
* @cw_data: the number of bytes within a codeword protected
* by ECC
- * @use_ecc: request the controller to use ECC for the
- * upcoming read/write
- * @bch_enabled: flag to tell whether BCH ECC mode is used
* @ecc_bytes_hw: ECC bytes used by controller hardware for this
* chip
- * @status: value to be returned if NAND_CMD_STATUS command
- * is executed
+ *
* @last_command: keeps track of last command on this chip. used
* for reading correct status
*
* @cfg0, cfg1, cfg0_raw..: NANDc register configurations needed for
* ecc/non-ecc mode for the current nand flash
* device
+ *
+ * @status: value to be returned if NAND_CMD_STATUS command
+ * is executed
+ * @use_ecc: request the controller to use ECC for the
+ * upcoming read/write
+ * @bch_enabled: flag to tell whether BCH ECC mode is used
*/
struct qcom_nand_host {
struct nand_chip chip;
@@ -452,12 +467,10 @@ 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;
- u8 status;
+
int last_command;

u32 cfg0, cfg1;
@@ -466,23 +479,27 @@ struct qcom_nand_host {
u32 ecc_bch_cfg;
u32 clrflashstatus;
u32 clrreadstatus;
+
+ u8 status;
+ bool use_ecc;
+ bool bch_enabled;
};

/*
* This data type corresponds to the NAND controller properties which varies
* among different NAND controllers.
* @ecc_modes - ecc mode for NAND
+ * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
* @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
- * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
*/
struct qcom_nandc_props {
u32 ecc_modes;
+ u32 dev_cmd_reg_start;
bool is_bam;
bool is_qpic;
bool qpic_v2;
- u32 dev_cmd_reg_start;
};

/* Frees the BAM transaction memory */
--
2.36.1

2022-06-16 00:21:46

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v8 3/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

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: Christian Marangi <[email protected]>
---
.../devicetree/bindings/mtd/qcom,nandc.yaml | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
index 84ad7ff30121..482a2c068740 100644
--- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
+++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
@@ -102,6 +102,31 @@ 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 ...>
+ and should be declared in ascending order.
+
+ Refer to the ipq8064 example on how to use this special binding.
+
required:
- compatible
- reg
@@ -135,6 +160,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

2022-06-16 00:50:15

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v8 2/3] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

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: Christian Marangi <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 199 +++++++++++++++++++++++++++++-
1 file changed, 194 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index f2990d721733..ce1cb048bafc 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 */
@@ -431,13 +434,32 @@ struct qcom_nand_controller {
u32 cmd1, vld;
};

+/*
+ * 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
*
+ * @boot_partitions: array of boot partitions where offset and size of the
+ * boot partitions are stored
+ *
* @chip: base NAND chip structure
* @node: list node to add itself to host_list in
* qcom_nand_controller
*
+ * @nr_boot_partitions: count of the boot partitions where spare data is not
+ * protected by ECC
+ *
* @cs: chip select value for this chip
* @cw_size: the number of bytes in a single step/codeword
* of a page, consisting of all data, ecc, spare
@@ -456,14 +478,20 @@ struct qcom_nand_controller {
*
* @status: value to be returned if NAND_CMD_STATUS command
* is executed
+ * @codeword_fixup: keep track of the current layout used by
+ * the driver for read/write operation.
* @use_ecc: request the controller to use ECC for the
* upcoming read/write
* @bch_enabled: flag to tell whether BCH ECC mode is used
*/
struct qcom_nand_host {
+ struct qcom_nand_boot_partition *boot_partitions;
+
struct nand_chip chip;
struct list_head node;

+ int nr_boot_partitions;
+
int cs;
int cw_size;
int cw_data;
@@ -481,6 +509,7 @@ struct qcom_nand_host {
u32 clrreadstatus;

u8 status;
+ bool codeword_fixup;
bool use_ecc;
bool bch_enabled;
};
@@ -493,6 +522,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
*/
struct qcom_nandc_props {
u32 ecc_modes;
@@ -500,6 +530,7 @@ struct qcom_nandc_props {
bool is_bam;
bool is_qpic;
bool qpic_v2;
+ bool use_codeword_fixup;
};

/* Frees the BAM transaction memory */
@@ -1718,7 +1749,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 +
@@ -1799,7 +1830,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 {
@@ -1957,7 +1988,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;
@@ -2054,6 +2085,69 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
return ret;
}

+static bool qcom_nandc_is_boot_partition(struct qcom_nand_host *host, int page)
+{
+ struct qcom_nand_boot_partition *boot_partition;
+ u32 start, end;
+ int i;
+
+ /*
+ * 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.
+ */
+
+ /* First check the last boot partition */
+ boot_partition = &host->boot_partitions[host->nr_boot_partitions - 1];
+ start = boot_partition->page_offset;
+ end = start + boot_partition->page_size;
+
+ /* Page is after the last boot partition end. This is NOT a boot partition */
+ if (page > end)
+ return false;
+
+ /* Actually check if it's a boot partition */
+ if (page < end && page >= start)
+ return true;
+
+ /* Check the other boot partitions starting from the second-last partition */
+ for (i = host->nr_boot_partitions - 2; i >= 0; i--) {
+ boot_partition = &host->boot_partitions[i];
+ start = boot_partition->page_offset;
+ end = start + boot_partition->page_size;
+
+ 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_partition(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 = (host->cw_data - 1) << NUM_STEPS;
+}
+
/* implements ecc->read_page() */
static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
int oob_required, int page)
@@ -2062,6 +2156,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;
@@ -2081,6 +2178,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);
@@ -2101,6 +2201,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);

@@ -2121,6 +2224,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);
@@ -2136,7 +2242,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;
@@ -2193,6 +2299,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);
@@ -2211,7 +2320,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 +
@@ -2271,6 +2380,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);

@@ -2919,6 +3031,74 @@ 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 (!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\n");
+ return partitions_count ? partitions_count : -EINVAL;
+ }
+
+ 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\n", i);
+ host->nr_boot_partitions = 0;
+ return ret;
+ }
+
+ if (boot_partition->page_offset % mtd->writesize) {
+ dev_err(dev, "Boot partition offset not multiple of writesize at index %i\n",
+ i);
+ host->nr_boot_partitions = 0;
+ 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\n", i);
+ host->nr_boot_partitions = 0;
+ return ret;
+ }
+
+ if (boot_partition->page_size % mtd->writesize) {
+ dev_err(dev, "Boot partition size not multiple of writesize at index %i\n",
+ i);
+ host->nr_boot_partitions = 0;
+ 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)
@@ -2987,6 +3167,14 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
if (ret)
nand_cleanup(chip);

+ if (nandc->props->use_codeword_fixup) {
+ ret = qcom_nand_host_parse_boot_partitions(nandc, host, dn);
+ if (ret) {
+ nand_cleanup(chip);
+ return ret;
+ }
+ }
+
return ret;
}

@@ -3152,6 +3340,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

2022-06-16 16:01:25

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v8 3/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

On Thu, 16 Jun 2022 02:18:35 +0200, Christian Marangi 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: Christian Marangi <[email protected]>
> ---
> .../devicetree/bindings/mtd/qcom,nandc.yaml | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>

2022-06-16 20:32:59

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v8 2/3] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

On Thu, Jun 16, 2022 at 02:18:34AM +0200, Christian Marangi 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: Christian Marangi <[email protected]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 199 +++++++++++++++++++++++++++++-
> 1 file changed, 194 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index f2990d721733..ce1cb048bafc 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 */
> @@ -431,13 +434,32 @@ struct qcom_nand_controller {
> u32 cmd1, vld;
> };
>
> +/*
> + * 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
> *
> + * @boot_partitions: array of boot partitions where offset and size of the
> + * boot partitions are stored
> + *
> * @chip: base NAND chip structure
> * @node: list node to add itself to host_list in
> * qcom_nand_controller
> *
> + * @nr_boot_partitions: count of the boot partitions where spare data is not
> + * protected by ECC
> + *
> * @cs: chip select value for this chip
> * @cw_size: the number of bytes in a single step/codeword
> * of a page, consisting of all data, ecc, spare
> @@ -456,14 +478,20 @@ struct qcom_nand_controller {
> *
> * @status: value to be returned if NAND_CMD_STATUS command
> * is executed
> + * @codeword_fixup: keep track of the current layout used by
> + * the driver for read/write operation.
> * @use_ecc: request the controller to use ECC for the
> * upcoming read/write
> * @bch_enabled: flag to tell whether BCH ECC mode is used
> */
> struct qcom_nand_host {
> + struct qcom_nand_boot_partition *boot_partitions;
> +
> struct nand_chip chip;
> struct list_head node;
>
> + int nr_boot_partitions;
> +
> int cs;
> int cw_size;
> int cw_data;
> @@ -481,6 +509,7 @@ struct qcom_nand_host {
> u32 clrreadstatus;
>
> u8 status;
> + bool codeword_fixup;
> bool use_ecc;
> bool bch_enabled;
> };
> @@ -493,6 +522,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
> */
> struct qcom_nandc_props {
> u32 ecc_modes;
> @@ -500,6 +530,7 @@ struct qcom_nandc_props {
> bool is_bam;
> bool is_qpic;
> bool qpic_v2;
> + bool use_codeword_fixup;
> };
>
> /* Frees the BAM transaction memory */
> @@ -1718,7 +1749,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 +
> @@ -1799,7 +1830,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 {
> @@ -1957,7 +1988,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;
> @@ -2054,6 +2085,69 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
> return ret;
> }
>
> +static bool qcom_nandc_is_boot_partition(struct qcom_nand_host *host, int page)
> +{
> + struct qcom_nand_boot_partition *boot_partition;
> + u32 start, end;
> + int i;
> +
> + /*
> + * 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.
> + */
> +
> + /* First check the last boot partition */
> + boot_partition = &host->boot_partitions[host->nr_boot_partitions - 1];
> + start = boot_partition->page_offset;
> + end = start + boot_partition->page_size;
> +
> + /* Page is after the last boot partition end. This is NOT a boot partition */
> + if (page > end)
> + return false;
> +
> + /* Actually check if it's a boot partition */
> + if (page < end && page >= start)
> + return true;
> +
> + /* Check the other boot partitions starting from the second-last partition */
> + for (i = host->nr_boot_partitions - 2; i >= 0; i--) {
> + boot_partition = &host->boot_partitions[i];
> + start = boot_partition->page_offset;
> + end = start + boot_partition->page_size;
> +
> + 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_partition(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 = (host->cw_data - 1) << NUM_STEPS;
> +}
> +
> /* implements ecc->read_page() */
> static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
> int oob_required, int page)
> @@ -2062,6 +2156,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;
> @@ -2081,6 +2178,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);
> @@ -2101,6 +2201,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);
>
> @@ -2121,6 +2224,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);
> @@ -2136,7 +2242,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;
> @@ -2193,6 +2299,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);
> @@ -2211,7 +2320,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 +
> @@ -2271,6 +2380,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);
>
> @@ -2919,6 +3031,74 @@ 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 (!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\n");
> + return partitions_count ? partitions_count : -EINVAL;
> + }
> +
> + 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\n", i);
> + host->nr_boot_partitions = 0;
> + return ret;
> + }
> +
> + if (boot_partition->page_offset % mtd->writesize) {
> + dev_err(dev, "Boot partition offset not multiple of writesize at index %i\n",
> + i);
> + host->nr_boot_partitions = 0;
> + 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\n", i);
> + host->nr_boot_partitions = 0;
> + return ret;
> + }
> +
> + if (boot_partition->page_size % mtd->writesize) {
> + dev_err(dev, "Boot partition size not multiple of writesize at index %i\n",
> + i);
> + host->nr_boot_partitions = 0;
> + 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)
> @@ -2987,6 +3167,14 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
> if (ret)
> nand_cleanup(chip);
>
> + if (nandc->props->use_codeword_fixup) {
> + ret = qcom_nand_host_parse_boot_partitions(nandc, host, dn);
> + if (ret) {
> + nand_cleanup(chip);
> + return ret;
> + }
> + }
> +
> return ret;
> }
>
> @@ -3152,6 +3340,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
>

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

2022-06-16 20:40:37

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v8 1/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

On Thu, Jun 16, 2022 at 02:18:33AM +0200, Christian Marangi wrote:
> Reorder structs in nandc driver to save holes.
>
> Signed-off-by: Christian Marangi <[email protected]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 107 +++++++++++++++++-------------
> 1 file changed, 62 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 1a77542c6d67..f2990d721733 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -238,6 +238,9 @@ nandc_set_reg(chip, reg, \
> * @bam_ce - the array of BAM command elements
> * @cmd_sgl - sgl for NAND BAM command pipe
> * @data_sgl - sgl for NAND BAM consumer/producer pipe
> + * @last_data_desc - last DMA desc in data channel (tx/rx).
> + * @last_cmd_desc - last DMA desc in command channel.
> + * @txn_done - completion for NAND transfer.
> * @bam_ce_pos - the index in bam_ce which is available for next sgl
> * @bam_ce_start - the index in bam_ce which marks the start position ce
> * for current sgl. It will be used for size calculation
> @@ -250,14 +253,14 @@ nandc_set_reg(chip, reg, \
> * @rx_sgl_start - start index in data sgl for rx.
> * @wait_second_completion - wait for second DMA desc completion before making
> * the NAND transfer completion.
> - * @txn_done - completion for NAND transfer.
> - * @last_data_desc - last DMA desc in data channel (tx/rx).
> - * @last_cmd_desc - last DMA desc in command channel.
> */
> struct bam_transaction {
> struct bam_cmd_element *bam_ce;
> struct scatterlist *cmd_sgl;
> struct scatterlist *data_sgl;
> + struct dma_async_tx_descriptor *last_data_desc;
> + struct dma_async_tx_descriptor *last_cmd_desc;
> + struct completion txn_done;
> u32 bam_ce_pos;
> u32 bam_ce_start;
> u32 cmd_sgl_pos;
> @@ -267,25 +270,23 @@ struct bam_transaction {
> u32 rx_sgl_pos;
> u32 rx_sgl_start;
> bool wait_second_completion;
> - struct completion txn_done;
> - struct dma_async_tx_descriptor *last_data_desc;
> - struct dma_async_tx_descriptor *last_cmd_desc;
> };
>
> /*
> * This data type corresponds to the nand dma descriptor
> + * @dma_desc - low level DMA engine descriptor
> * @list - list for desc_info
> - * @dir - DMA transfer direction
> + *
> * @adm_sgl - sgl which will be used for single sgl dma descriptor. Only used by
> * ADM
> * @bam_sgl - sgl which will be used for dma descriptor. Only used by BAM
> * @sgl_cnt - number of SGL in bam_sgl. Only used by BAM
> - * @dma_desc - low level DMA engine descriptor
> + * @dir - DMA transfer direction
> */
> struct desc_info {
> + struct dma_async_tx_descriptor *dma_desc;
> struct list_head node;
>
> - enum dma_data_direction dir;
> union {
> struct scatterlist adm_sgl;
> struct {
> @@ -293,7 +294,7 @@ struct desc_info {
> int sgl_cnt;
> };
> };
> - struct dma_async_tx_descriptor *dma_desc;
> + enum dma_data_direction dir;
> };
>
> /*
> @@ -337,52 +338,64 @@ struct nandc_regs {
> /*
> * NAND controller data struct
> *
> - * @controller: base controller structure
> - * @host_list: list containing all the chips attached to the
> - * controller
> * @dev: parent device
> + *
> * @base: MMIO base
> - * @base_phys: physical base address of controller registers
> - * @base_dma: dma base address of controller registers
> + *
> * @core_clk: controller clock
> * @aon_clk: another controller clock
> *
> + * @regs: a contiguous chunk of memory for DMA register
> + * writes. contains the register values to be
> + * written to controller
> + *
> + * @props: properties of current NAND controller,
> + * initialized via DT match data
> + *
> + * @controller: base controller structure
> + * @host_list: list containing all the chips attached to the
> + * controller
> + *
> * @chan: dma channel
> * @cmd_crci: ADM DMA CRCI for command flow control
> * @data_crci: ADM DMA CRCI for data flow control
> + *
> * @desc_list: DMA descriptor list (list of desc_infos)
> *
> * @data_buffer: our local DMA buffer for page read/writes,
> * used when we can't use the buffer provided
> * by upper layers directly
> - * @buf_size/count/start: markers for chip->legacy.read_buf/write_buf
> - * functions
> * @reg_read_buf: local buffer for reading back registers via DMA
> + *
> + * @base_phys: physical base address of controller registers
> + * @base_dma: dma base address of controller registers
> * @reg_read_dma: contains dma address for register read buffer
> - * @reg_read_pos: marker for data read in reg_read_buf
> *
> - * @regs: a contiguous chunk of memory for DMA register
> - * writes. contains the register values to be
> - * written to controller
> - * @cmd1/vld: some fixed controller register values
> - * @props: properties of current NAND controller,
> - * initialized via DT match data
> + * @buf_size/count/start: markers for chip->legacy.read_buf/write_buf
> + * functions
> * @max_cwperpage: maximum QPIC codewords required. calculated
> * from all connected NAND devices pagesize
> + *
> + * @reg_read_pos: marker for data read in reg_read_buf
> + *
> + * @cmd1/vld: some fixed controller register values
> */
> struct qcom_nand_controller {
> - struct nand_controller controller;
> - struct list_head host_list;
> -
> struct device *dev;
>
> void __iomem *base;
> - phys_addr_t base_phys;
> - dma_addr_t base_dma;
>
> struct clk *core_clk;
> struct clk *aon_clk;
>
> + struct nandc_regs *regs;
> + struct bam_transaction *bam_txn;
> +
> + const struct qcom_nandc_props *props;
> +
> + struct nand_controller controller;
> + struct list_head host_list;
> +
> union {
> /* will be used only by QPIC for BAM DMA */
> struct {
> @@ -400,22 +413,22 @@ struct qcom_nand_controller {
> };
>
> struct list_head desc_list;
> - struct bam_transaction *bam_txn;
>
> u8 *data_buffer;
> + __le32 *reg_read_buf;
> +
> + phys_addr_t base_phys;
> + dma_addr_t base_dma;
> + dma_addr_t reg_read_dma;
> +
> int buf_size;
> int buf_count;
> int buf_start;
> unsigned int max_cwperpage;
>
> - __le32 *reg_read_buf;
> - dma_addr_t reg_read_dma;
> int reg_read_pos;
>
> - struct nandc_regs *regs;
> -
> u32 cmd1, vld;
> - const struct qcom_nandc_props *props;
> };
>
> /*
> @@ -431,19 +444,21 @@ struct qcom_nand_controller {
> * and reserved bytes
> * @cw_data: the number of bytes within a codeword protected
> * by ECC
> - * @use_ecc: request the controller to use ECC for the
> - * upcoming read/write
> - * @bch_enabled: flag to tell whether BCH ECC mode is used
> * @ecc_bytes_hw: ECC bytes used by controller hardware for this
> * chip
> - * @status: value to be returned if NAND_CMD_STATUS command
> - * is executed
> + *
> * @last_command: keeps track of last command on this chip. used
> * for reading correct status
> *
> * @cfg0, cfg1, cfg0_raw..: NANDc register configurations needed for
> * ecc/non-ecc mode for the current nand flash
> * device
> + *
> + * @status: value to be returned if NAND_CMD_STATUS command
> + * is executed
> + * @use_ecc: request the controller to use ECC for the
> + * upcoming read/write
> + * @bch_enabled: flag to tell whether BCH ECC mode is used
> */
> struct qcom_nand_host {
> struct nand_chip chip;
> @@ -452,12 +467,10 @@ 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;
> - u8 status;
> +
> int last_command;
>
> u32 cfg0, cfg1;
> @@ -466,23 +479,27 @@ struct qcom_nand_host {
> u32 ecc_bch_cfg;
> u32 clrflashstatus;
> u32 clrreadstatus;
> +
> + u8 status;
> + bool use_ecc;
> + bool bch_enabled;
> };
>
> /*
> * This data type corresponds to the NAND controller properties which varies
> * among different NAND controllers.
> * @ecc_modes - ecc mode for NAND
> + * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
> * @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
> - * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
> */
> struct qcom_nandc_props {
> u32 ecc_modes;
> + u32 dev_cmd_reg_start;
> bool is_bam;
> bool is_qpic;
> bool qpic_v2;
> - u32 dev_cmd_reg_start;
> };
>
> /* Frees the BAM transaction memory */
> --
> 2.36.1
>

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

2022-06-16 20:59:50

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v8 3/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

On Thu, Jun 16, 2022 at 02:18:35AM +0200, Christian Marangi 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: Christian Marangi <[email protected]>
> ---

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> .../devicetree/bindings/mtd/qcom,nandc.yaml | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> index 84ad7ff30121..482a2c068740 100644
> --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> @@ -102,6 +102,31 @@ 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 ...>
> + and should be declared in ascending order.
> +
> + Refer to the ipq8064 example on how to use this special binding.
> +
> required:
> - compatible
> - reg
> @@ -135,6 +160,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
>

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

2022-06-17 06:35:04

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v8 3/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding

On Thu, 2022-06-16 at 00:18:35 UTC, Christian Marangi 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: Christian Marangi <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

2022-06-17 07:00:32

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v8 1/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct

On Thu, 2022-06-16 at 00:18:33 UTC, Christian Marangi wrote:
> Reorder structs in nandc driver to save holes.
>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

2022-06-17 07:24:23

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v8 2/3] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages

On Thu, 2022-06-16 at 00:18:34 UTC, Christian Marangi 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: Christian Marangi <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel