2022-06-15 01:16:01

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v7 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.

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

Ansuel Smith (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 | 310 +++++++++++++++---
2 files changed, 287 insertions(+), 50 deletions(-)

--
2.36.1


2022-06-15 01:30:38

by Christian Marangi

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

Reorder structs in nandc driver to save holes.

Signed-off-by: Ansuel Smith <[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-15 17:40:36

by Manivannan Sadhasivam

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

On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> Reorder structs in nandc driver to save holes.
>
> Signed-off-by: Ansuel Smith <[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 00:50:35

by Christian Marangi

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

On Wed, Jun 15, 2022 at 10:41:32PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> > Reorder structs in nandc driver to save holes.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>
> Thanks,
> Mani
>

I'm sending v8 with a different Sob so I'm not adding the review tag (in
v8).
In short the new Sob is what I will use onwards, wanted to keep the
Ansuel reference but it was suggested to use Christian Marangi and
nothing more. It's just a name change and we are the same person and
nobody is stealing ownership of the patch.
Sorry for the mess.

> > ---
> > 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
> >
>
> --
> மணிவண்ணன் சதாசிவம்

--
Ansuel

2022-06-16 14:56:23

by Christian Marangi

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

On Thu, Jun 16, 2022 at 04:37:51PM +0200, Miquel Raynal wrote:
> Hi Ansuel/Christian,
>
> [email protected] wrote on Thu, 16 Jun 2022 02:18:08 +0200:
>
> > On Wed, Jun 15, 2022 at 10:41:32PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> > > > Reorder structs in nandc driver to save holes.
> > > >
> > > > Signed-off-by: Ansuel Smith <[email protected]>
> > >
> > > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> > >
> > > Thanks,
> > > Mani
> > >
> >
> > I'm sending v8 with a different Sob so I'm not adding the review tag (in
> > v8).
> > In short the new Sob is what I will use onwards, wanted to keep the
> > Ansuel reference but it was suggested to use Christian Marangi and
> > nothing more. It's just a name change and we are the same person and
> > nobody is stealing ownership of the patch.
> > Sorry for the mess.
>
> Mmmh strange, but okay. You are supposed to contribute with your real
> identity, not under pseudonym anyway.
>

You are right, it's something I'm trying to fix... Fact is that the
original series was old so I didn't want to change the name.

> Also, you could have kept Mani's R-by in v8 but anyway. Mani, can
> you resend them?
>

Didn't want to make changes to the patch with the R-by tag just to make
sure. Better safe than sorry.

> Thanks,
> Miqu?l

--
Ansuel

2022-06-16 15:19:52

by Miquel Raynal

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

Hi Ansuel/Christian,

[email protected] wrote on Thu, 16 Jun 2022 02:18:08 +0200:

> On Wed, Jun 15, 2022 at 10:41:32PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> > > Reorder structs in nandc driver to save holes.
> > >
> > > Signed-off-by: Ansuel Smith <[email protected]>
> >
> > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> >
> > Thanks,
> > Mani
> >
>
> I'm sending v8 with a different Sob so I'm not adding the review tag (in
> v8).
> In short the new Sob is what I will use onwards, wanted to keep the
> Ansuel reference but it was suggested to use Christian Marangi and
> nothing more. It's just a name change and we are the same person and
> nobody is stealing ownership of the patch.
> Sorry for the mess.

Mmmh strange, but okay. You are supposed to contribute with your real
identity, not under pseudonym anyway.

Also, you could have kept Mani's R-by in v8 but anyway. Mani, can
you resend them?

Thanks,
Miquèl

2022-06-16 20:30:07

by Manivannan Sadhasivam

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

On Thu, Jun 16, 2022 at 02:18:08AM +0200, Ansuel Smith wrote:
> On Wed, Jun 15, 2022 at 10:41:32PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> > > Reorder structs in nandc driver to save holes.
> > >
> > > Signed-off-by: Ansuel Smith <[email protected]>
> >
> > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> >
> > Thanks,
> > Mani
> >
>
> I'm sending v8 with a different Sob so I'm not adding the review tag (in
> v8).
> In short the new Sob is what I will use onwards, wanted to keep the
> Ansuel reference but it was suggested to use Christian Marangi and
> nothing more. It's just a name change and we are the same person and
> nobody is stealing ownership of the patch.
> Sorry for the mess.
>

That's fine but you could've kept the review tags... Anyway, I'll give mine.

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
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
>
> --
> Ansuel

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

2022-06-16 20:59:39

by Manivannan Sadhasivam

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

On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> Reorder structs in nandc driver to save holes.
>
> Signed-off-by: Ansuel Smith <[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 21:34:06

by Manivannan Sadhasivam

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

On Thu, Jun 16, 2022 at 04:37:51PM +0200, Miquel Raynal wrote:
> Hi Ansuel/Christian,
>
> [email protected] wrote on Thu, 16 Jun 2022 02:18:08 +0200:
>
> > On Wed, Jun 15, 2022 at 10:41:32PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jun 15, 2022 at 02:06:10AM +0200, Ansuel Smith wrote:
> > > > Reorder structs in nandc driver to save holes.
> > > >
> > > > Signed-off-by: Ansuel Smith <[email protected]>
> > >
> > > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> > >
> > > Thanks,
> > > Mani
> > >
> >
> > I'm sending v8 with a different Sob so I'm not adding the review tag (in
> > v8).
> > In short the new Sob is what I will use onwards, wanted to keep the
> > Ansuel reference but it was suggested to use Christian Marangi and
> > nothing more. It's just a name change and we are the same person and
> > nobody is stealing ownership of the patch.
> > Sorry for the mess.
>
> Mmmh strange, but okay. You are supposed to contribute with your real
> identity, not under pseudonym anyway.
>
> Also, you could have kept Mani's R-by in v8 but anyway. Mani, can
> you resend them?
>

Done!

Thanks,
Mani

> Thanks,
> Miquèl

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