[PATCH v2 0/3] QPIC v2 fixes for SDX75

[PATCH v2 0/3] QPIC v2 fixes for SDX75

[Md Sadre Alam]

v2:
 * Updated commit message
 * Added stable kernel tag
 * Added Fixes tag
 * Renamed the variable from offset_from_qpic to nandc_offset
 * Set buf_count to 512 in the parameter page read
 * Replaced the buf_count value of 512 with the len in bytes 

v1:
 * These patches will fix the following:
 * 1) onfi param page read which was broken by exec_op() patch.
 * 2) Fixed offset passed to BAM from QPIC base

Md Sadre Alam (3):
  mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
  mtd: rawnand: qcom: Fix onfi param page read
  mtd: rawnand: qcom: Fix read len for onfi param page

 drivers/mtd/nand/raw/qcom_nandc.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/3] mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM

[Md Sadre Alam]

Currently we are configuring lower 24 bits of address in descriptor
whereas QPIC design expects 18 bit register offset from QPIC base
address to be configured in cmd descriptors. This is leading to a
different address actually being used in HW, leading to wrong value
read.

the actual issue is that the NANDc base address is different from the
QPIC base address. But the driver doesn't take it into account and just
used the QPIC base as the NANDc base. This used to work as the NANDc IP
only considers the lower 18 bits of the address passed by the driver to
derive the register offset. Since the base address of QPIC used to contain
all 0 for lower 18 bits (like 0x07980000), the driver ended up passing the
actual register offset in it and NANDc worked properly. But on newer SoCs
like SDX75, the QPIC base address doesn't contain all 0 for lower 18 bits
(like 0x01C98000). So NANDc sees wrong offset as per the current logic

Older targets also used same configuration (lower 24 bits) like SDX55,
SDX65, IPQ8074, IPQ6018  etc. but issue is masked in older targets due
to lower 18 bits of QPIC base address being zero leading to expected
address generation.

The address should be passed to BAM 0x30000 + offset. In older targets
the lower 18-bits are zero so that correct address being paased. But
in newer targets the lower 18-bits are non-zero in QPIC base so that
0x300000 + offset giving the wrong value.

SDX75 : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
SDX55 : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
older targets.

Cc: stable@vger.kernel.org
Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v2]

* Updated commit message

* Added Fixes tag

* Added stable kernel tag

* Renamed the variable from offset_from_qpic to nandc_offset

Change in [v1]

* Preliminary correction for the register address forwarded to BAM
 
 drivers/mtd/nand/raw/qcom_nandc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b8cff9240b28..cc59461df72e 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -207,7 +207,7 @@ nandc_set_reg(chip, reg,			\
 #define dev_cmd_reg_addr(nandc, reg) ((nandc)->props->dev_cmd_reg_start + (reg))
 
 /* Returns the NAND register physical address */
-#define nandc_reg_phys(chip, offset) ((chip)->base_phys + (offset))
+#define nandc_reg_phys(chip, offset)  ((nandc)->props->nandc_offset + (offset))
 
 /* Returns the dma address for reg read buffer */
 #define reg_buf_dma_addr(chip, vaddr) \
@@ -561,6 +561,7 @@ struct qcom_nandc_props {
 	bool is_qpic;
 	bool qpic_v2;
 	bool use_codeword_fixup;
+	u32 nandc_offset;
 };
 
 /* Frees the BAM transaction memory */
@@ -3477,6 +3478,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = {
 	.is_bam = false,
 	.use_codeword_fixup = true,
 	.dev_cmd_reg_start = 0x0,
+	.nandc_offset = 0x30000,
 };
 
 static const struct qcom_nandc_props ipq4019_nandc_props = {
@@ -3484,6 +3486,7 @@ static const struct qcom_nandc_props ipq4019_nandc_props = {
 	.is_bam = true,
 	.is_qpic = true,
 	.dev_cmd_reg_start = 0x0,
+	.nandc_offset = 0x30000,
 };
 
 static const struct qcom_nandc_props ipq8074_nandc_props = {
@@ -3491,6 +3494,7 @@ static const struct qcom_nandc_props ipq8074_nandc_props = {
 	.is_bam = true,
 	.is_qpic = true,
 	.dev_cmd_reg_start = 0x7000,
+	.nandc_offset = 0x30000,
 };
 
 static const struct qcom_nandc_props sdx55_nandc_props = {
@@ -3498,7 +3502,8 @@ static const struct qcom_nandc_props sdx55_nandc_props = {
 	.is_bam = true,
 	.is_qpic = true,
 	.qpic_v2 = true,
-	.dev_cmd_reg_start = 0x7000,
+	.dev_cmd_reg_start = 0x0,
+	.nandc_offset = 0x30000,
 };
 
 /*
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/3] mtd: rawnand: qcom: Fix onfi param page read

[Md Sadre Alam]

For QPIC V2 onwards there is a separate register to read
last code word "QPIC_NAND_READ_LOCATION_LAST_CW_n".

qcom_param_page_type_exec() is used to read only one code word
If it will get configure number of code words to 1 in QPIC_NAND_DEV0_CFG0
register then QPIC controller thinks its reading the last code word,
since we are having separate register to read the last code word,
we have to configure "QPIC_NAND_READ_LOCATION_LAST_CW_n" register
to fetch data from QPIC buffer to system memory.

Without this change page read was failing with timeout error

/ # hexdump -C /dev/mtd1
[  129.206113] qcom-nandc 1cc8000.nand-controller: failure to read page/oob
hexdump: /dev/mtd1: Connection timed out

This issue only seen on SDX targets since SDX target used QPICv2. But
same working on IPQ targets since IPQ used QPICv1.

Cc: stable@vger.kernel.org
Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v2]

* Updated commit message

* Added stable kernel tag

* Replaced the buf_count value of 512 with the len in bytes.

Change in [v1]

* Resolved the issue with reading a single code word in the parameter
  page read

 drivers/mtd/nand/raw/qcom_nandc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index cc59461df72e..31ec3db1246d 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2859,7 +2859,12 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	const struct nand_op_instr *instr = NULL;
 	unsigned int op_id = 0;
 	unsigned int len = 0;
-	int ret;
+	int ret, reg_base;
+
+	reg_base = NAND_READ_LOCATION_0;
+
+	if (nandc->props->qpic_v2)
+		reg_base = NAND_READ_LOCATION_LAST_CW_0;
 
 	ret = qcom_parse_instructions(chip, subop, &q_op);
 	if (ret)
@@ -2911,7 +2916,10 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	op_id = q_op.data_instr_idx;
 	len = nand_subop_get_data_len(subop, op_id);
 
-	nandc_set_read_loc(chip, 0, 0, 0, len, 1);
+	if (nandc->props->qpic_v2)
+		nandc_set_read_loc_last(chip, reg_base, 0, len, 1);
+	else
+		nandc_set_read_loc_first(chip, reg_base, 0, len, 1);
 
 	if (!nandc->props->qpic_v2) {
 		write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/3] mtd: rawnand: qcom: Fix read len for onfi param page

[Md Sadre Alam]

The minimum size to fetch the data from device to QPIC buffer
is 512-bytes. If size is less than 512-bytes the data will not be
protected by ECC as per QPIC standard. So while reading onfi parameter
page from NAND device setting nandc->buf_count = 512.

Cc: stable@vger.kernel.org
Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v2]

* Set buf_count to 512 in the parameter page read

Change in [v1]

* This patch was not included in v1

 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 31ec3db1246d..e1dca4857754 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2926,7 +2926,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 		write_reg_dma(nandc, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
 	}
 
-	nandc->buf_count = len;
+	nandc->buf_count = 512;
 	memset(nandc->data_buffer, 0xff, nandc->buf_count);
 
 	config_nand_single_cw_page_read(chip, false, 0);
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Lakshmi Sowjanya D (QUIC)]

> -----Original Message-----
> From: Md Sadre Alam (QUIC) <quic_mdalam@quicinc.com>
> Sent: Friday, November 22, 2024 2:30 PM
> To: manivannan.sadhasivam@linaro.org; miquel.raynal@bootlin.com;
> richard@nod.at; vigneshr@ti.com; bbrezillon@kernel.org; linux-
> mtd@lists.infradead.org; linux-arm-msm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Sricharan Ramabadhran (QUIC) <quic_srichara@quicinc.com>;
> Varadarajan Narayanan (QUIC) <quic_varada@quicinc.com>; Md Sadre Alam
> (QUIC) <quic_mdalam@quicinc.com>; Naina Mehta (QUIC)
> <quic_nainmeht@quicinc.com>; Lakshmi Sowjanya D (QUIC)
> <quic_laksd@quicinc.com>
> Subject: [PATCH v2 0/3] QPIC v2 fixes for SDX75
> 
> v2:
>  * Updated commit message
>  * Added stable kernel tag
>  * Added Fixes tag
>  * Renamed the variable from offset_from_qpic to nandc_offset
>  * Set buf_count to 512 in the parameter page read
>  * Replaced the buf_count value of 512 with the len in bytes
> 
> v1:
>  * These patches will fix the following:
>  * 1) onfi param page read which was broken by exec_op() patch.
>  * 2) Fixed offset passed to BAM from QPIC base
> 
> Md Sadre Alam (3):
>   mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
>   mtd: rawnand: qcom: Fix onfi param page read
>   mtd: rawnand: qcom: Fix read len for onfi param page
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> --

Tested-by: Lakshmi Sowjanya D <quic_laksd@quicinc.com>     # on SDX75

-- 
Regards
Lakshmi Sowjanya


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM

[Manivannan Sadhasivam]

On Fri, Nov 22, 2024 at 02:29:31PM +0530, Md Sadre Alam wrote:
> Currently we are configuring lower 24 bits of address in descriptor
> whereas QPIC design expects 18 bit register offset from QPIC base
> address to be configured in cmd descriptors. This is leading to a
> different address actually being used in HW, leading to wrong value
> read.
> 
> the actual issue is that the NANDc base address is different from the
> QPIC base address. But the driver doesn't take it into account and just
> used the QPIC base as the NANDc base. This used to work as the NANDc IP
> only considers the lower 18 bits of the address passed by the driver to
> derive the register offset. Since the base address of QPIC used to contain
> all 0 for lower 18 bits (like 0x07980000), the driver ended up passing the

SDX55's NANDc base is 0x01b30000 and it has bits 17 and 18 set corresponding to
0x30000. So it is correct that the IP only considers lower 18 bits and it used
to work as the driver ended up passing 0x3000 + register offset.

Your wording is not correct.

> actual register offset in it and NANDc worked properly. But on newer SoCs
> like SDX75, the QPIC base address doesn't contain all 0 for lower 18 bits
> (like 0x01C98000). So NANDc sees wrong offset as per the current logic
> 

'all 0 for lower 18 bits' is not true.

> Older targets also used same configuration (lower 24 bits) like SDX55,
> SDX65, IPQ8074, IPQ6018  etc. but issue is masked in older targets due
> to lower 18 bits of QPIC base address being zero leading to expected
> address generation.
> 

This paragraph is redundant now.

> The address should be passed to BAM 0x30000 + offset. In older targets
> the lower 18-bits are zero so that correct address being paased. But
> in newer targets the lower 18-bits are non-zero in QPIC base so that
> 0x300000 + offset giving the wrong value.
> 
> SDX75 : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
> SDX55 : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for

This address is wrong as I mentioned above.

> older targets.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v2]
> 
> * Updated commit message
> 
> * Added Fixes tag
> 
> * Added stable kernel tag
> 
> * Renamed the variable from offset_from_qpic to nandc_offset
> 
> Change in [v1]
> 
> * Preliminary correction for the register address forwarded to BAM
>  
>  drivers/mtd/nand/raw/qcom_nandc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b8cff9240b28..cc59461df72e 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -207,7 +207,7 @@ nandc_set_reg(chip, reg,			\
>  #define dev_cmd_reg_addr(nandc, reg) ((nandc)->props->dev_cmd_reg_start + (reg))
>  
>  /* Returns the NAND register physical address */
> -#define nandc_reg_phys(chip, offset) ((chip)->base_phys + (offset))
> +#define nandc_reg_phys(chip, offset)  ((nandc)->props->nandc_offset + (offset))
>  
>  /* Returns the dma address for reg read buffer */
>  #define reg_buf_dma_addr(chip, vaddr) \
> @@ -561,6 +561,7 @@ struct qcom_nandc_props {
>  	bool is_qpic;
>  	bool qpic_v2;
>  	bool use_codeword_fixup;
> +	u32 nandc_offset;
>  };
>  
>  /* Frees the BAM transaction memory */
> @@ -3477,6 +3478,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = {
>  	.is_bam = false,
>  	.use_codeword_fixup = true,
>  	.dev_cmd_reg_start = 0x0,
> +	.nandc_offset = 0x30000,
>  };
>  
>  static const struct qcom_nandc_props ipq4019_nandc_props = {
> @@ -3484,6 +3486,7 @@ static const struct qcom_nandc_props ipq4019_nandc_props = {
>  	.is_bam = true,
>  	.is_qpic = true,
>  	.dev_cmd_reg_start = 0x0,
> +	.nandc_offset = 0x30000,
>  };
>  
>  static const struct qcom_nandc_props ipq8074_nandc_props = {
> @@ -3491,6 +3494,7 @@ static const struct qcom_nandc_props ipq8074_nandc_props = {
>  	.is_bam = true,
>  	.is_qpic = true,
>  	.dev_cmd_reg_start = 0x7000,
> +	.nandc_offset = 0x30000,
>  };
>  
>  static const struct qcom_nandc_props sdx55_nandc_props = {
> @@ -3498,7 +3502,8 @@ static const struct qcom_nandc_props sdx55_nandc_props = {
>  	.is_bam = true,
>  	.is_qpic = true,
>  	.qpic_v2 = true,
> -	.dev_cmd_reg_start = 0x7000,
> +	.dev_cmd_reg_start = 0x0,

What is this change?

- Mani

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/3] mtd: rawnand: qcom: Fix onfi param page read

[Manivannan Sadhasivam]

On Fri, Nov 22, 2024 at 02:29:32PM +0530, Md Sadre Alam wrote:

Please change subject to:

mtd: rawnand: qcom: Fix last codeword read in qcom_param_page_type_exec()

> For QPIC V2 onwards there is a separate register to read
> last code word "QPIC_NAND_READ_LOCATION_LAST_CW_n".
> 
> qcom_param_page_type_exec() is used to read only one code word
> If it will get configure number of code words to 1 in QPIC_NAND_DEV0_CFG0

"If it configures the number of..."

> register then QPIC controller thinks its reading the last code word,
> since we are having separate register to read the last code word,
> we have to configure "QPIC_NAND_READ_LOCATION_LAST_CW_n" register
> to fetch data from QPIC buffer to system memory.
> 
> Without this change page read was failing with timeout error
> 
> / # hexdump -C /dev/mtd1
> [  129.206113] qcom-nandc 1cc8000.nand-controller: failure to read page/oob
> hexdump: /dev/mtd1: Connection timed out
> 
> This issue only seen on SDX targets since SDX target used QPICv2. But
> same working on IPQ targets since IPQ used QPICv1.
> 
> Cc: stable@vger.kernel.org
> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> 
> Change in [v2]
> 
> * Updated commit message
> 
> * Added stable kernel tag
> 
> * Replaced the buf_count value of 512 with the len in bytes.
> 
> Change in [v1]
> 
> * Resolved the issue with reading a single code word in the parameter
>   page read
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index cc59461df72e..31ec3db1246d 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2859,7 +2859,12 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>  	const struct nand_op_instr *instr = NULL;
>  	unsigned int op_id = 0;
>  	unsigned int len = 0;
> -	int ret;
> +	int ret, reg_base;
> +
> +	reg_base = NAND_READ_LOCATION_0;
> +
> +	if (nandc->props->qpic_v2)
> +		reg_base = NAND_READ_LOCATION_LAST_CW_0;
>  
>  	ret = qcom_parse_instructions(chip, subop, &q_op);
>  	if (ret)
> @@ -2911,7 +2916,10 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>  	op_id = q_op.data_instr_idx;
>  	len = nand_subop_get_data_len(subop, op_id);
>  
> -	nandc_set_read_loc(chip, 0, 0, 0, len, 1);

nandc_set_read_loc() does changes the register offset based on QPIC version. So
what exactly you are trying to fix here?

- Mani

> +	if (nandc->props->qpic_v2)
> +		nandc_set_read_loc_last(chip, reg_base, 0, len, 1);
> +	else
> +		nandc_set_read_loc_first(chip, reg_base, 0, len, 1);
>  
>  	if (!nandc->props->qpic_v2) {
>  		write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
> -- 
> 2.34.1
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/3] mtd: rawnand: qcom: Fix read len for onfi param page

[Manivannan Sadhasivam]

On Fri, Nov 22, 2024 at 02:29:33PM +0530, Md Sadre Alam wrote:
> The minimum size to fetch the data from device to QPIC buffer
> is 512-bytes. If size is less than 512-bytes the data will not be
> protected by ECC as per QPIC standard. So while reading onfi parameter
> page from NAND device setting nandc->buf_count = 512.

s/setting/set

> 
> Cc: stable@vger.kernel.org
> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> 
> Change in [v2]
> 
> * Set buf_count to 512 in the parameter page read
> 
> Change in [v1]
> 
> * This patch was not included in v1
> 
>  drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 31ec3db1246d..e1dca4857754 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2926,7 +2926,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>  		write_reg_dma(nandc, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
>  	}
>  
> -	nandc->buf_count = len;
> +	nandc->buf_count = 512;
>  	memset(nandc->data_buffer, 0xff, nandc->buf_count);
>  
>  	config_nand_single_cw_page_read(chip, false, 0);
> -- 
> 2.34.1
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/3] mtd: rawnand: qcom: Fix read len for onfi param page

[Manivannan Sadhasivam]

On Tue, Nov 26, 2024 at 11:15:35AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Nov 22, 2024 at 02:29:33PM +0530, Md Sadre Alam wrote:
> > The minimum size to fetch the data from device to QPIC buffer
> > is 512-bytes. If size is less than 512-bytes the data will not be
> > protected by ECC as per QPIC standard. So while reading onfi parameter
> > page from NAND device setting nandc->buf_count = 512.
> 
> s/setting/set
> 
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
> > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 

This was a mistake. I didn't intend to give this tag.

- Mani

> - Mani
> 
> > ---
> > 
> > Change in [v2]
> > 
> > * Set buf_count to 512 in the parameter page read
> > 
> > Change in [v1]
> > 
> > * This patch was not included in v1
> > 
> >  drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 31ec3db1246d..e1dca4857754 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -2926,7 +2926,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
> >  		write_reg_dma(nandc, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
> >  	}
> >  
> > -	nandc->buf_count = len;
> > +	nandc->buf_count = 512;
> >  	memset(nandc->data_buffer, 0xff, nandc->buf_count);
> >  
> >  	config_nand_single_cw_page_read(chip, false, 0);
> > -- 
> > 2.34.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM

[Md Sadre Alam]

On 11/26/2024 11:00 AM, Manivannan Sadhasivam wrote:
> On Fri, Nov 22, 2024 at 02:29:31PM +0530, Md Sadre Alam wrote:
>> Currently we are configuring lower 24 bits of address in descriptor
>> whereas QPIC design expects 18 bit register offset from QPIC base
>> address to be configured in cmd descriptors. This is leading to a
>> different address actually being used in HW, leading to wrong value
>> read.
>>
>> the actual issue is that the NANDc base address is different from the
>> QPIC base address. But the driver doesn't take it into account and just
>> used the QPIC base as the NANDc base. This used to work as the NANDc IP
>> only considers the lower 18 bits of the address passed by the driver to
>> derive the register offset. Since the base address of QPIC used to contain
>> all 0 for lower 18 bits (like 0x07980000), the driver ended up passing the
> 
> SDX55's NANDc base is 0x01b30000 and it has bits 17 and 18 set corresponding to
> 0x30000. So it is correct that the IP only considers lower 18 bits and it used
> to work as the driver ended up passing 0x3000 + register offset.
This address 0x30000 is the address from QPIC_BASE to QPIC_EBI2NAND
e.g for SDX55 and SDX65 the QPIC_BASE is 0x01B00000. So here lower 18-bits
are zero only.
> 
> Your wording is not correct.
Ok, will fix in next revision.
> 
>> actual register offset in it and NANDc worked properly. But on newer SoCs
>> like SDX75, the QPIC base address doesn't contain all 0 for lower 18 bits
>> (like 0x01C98000). So NANDc sees wrong offset as per the current logic
>>
> 
> 'all 0 for lower 18 bits' is not true.
The lower 18 bits zero we have to see in QPIC_BASE not in EBI2NAND_BASE.
e.g SDX55 & SDX65 the QPIC_BASE = 0x01B00000, all lower 18 bits are zero
but in SDX75 the QPIC_BASE = 0x01C98000, all lower 18 bits are non-zero.
> 
>> Older targets also used same configuration (lower 24 bits) like SDX55,
>> SDX65, IPQ8074, IPQ6018  etc. but issue is masked in older targets due
>> to lower 18 bits of QPIC base address being zero leading to expected
>> address generation.
>>
> 
> This paragraph is redundant now.
Ok, will remove in next revision.
> 
>> The address should be passed to BAM 0x30000 + offset. In older targets
>> the lower 18-bits are zero so that correct address being paased. But
>> in newer targets the lower 18-bits are non-zero in QPIC base so that
>> 0x300000 + offset giving the wrong value.
>>
>> SDX75 : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
>> SDX55 : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
> 
> This address is wrong as I mentioned above.
Ok, will fix in next revision.
> 
>> older targets.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Updated commit message
>>
>> * Added Fixes tag
>>
>> * Added stable kernel tag
>>
>> * Renamed the variable from offset_from_qpic to nandc_offset
>>
>> Change in [v1]
>>
>> * Preliminary correction for the register address forwarded to BAM
>>   
>>   drivers/mtd/nand/raw/qcom_nandc.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index b8cff9240b28..cc59461df72e 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -207,7 +207,7 @@ nandc_set_reg(chip, reg,			\
>>   #define dev_cmd_reg_addr(nandc, reg) ((nandc)->props->dev_cmd_reg_start + (reg))
>>   
>>   /* Returns the NAND register physical address */
>> -#define nandc_reg_phys(chip, offset) ((chip)->base_phys + (offset))
>> +#define nandc_reg_phys(chip, offset)  ((nandc)->props->nandc_offset + (offset))
>>   
>>   /* Returns the dma address for reg read buffer */
>>   #define reg_buf_dma_addr(chip, vaddr) \
>> @@ -561,6 +561,7 @@ struct qcom_nandc_props {
>>   	bool is_qpic;
>>   	bool qpic_v2;
>>   	bool use_codeword_fixup;
>> +	u32 nandc_offset;
>>   };
>>   
>>   /* Frees the BAM transaction memory */
>> @@ -3477,6 +3478,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = {
>>   	.is_bam = false,
>>   	.use_codeword_fixup = true,
>>   	.dev_cmd_reg_start = 0x0,
>> +	.nandc_offset = 0x30000,
>>   };
>>   
>>   static const struct qcom_nandc_props ipq4019_nandc_props = {
>> @@ -3484,6 +3486,7 @@ static const struct qcom_nandc_props ipq4019_nandc_props = {
>>   	.is_bam = true,
>>   	.is_qpic = true,
>>   	.dev_cmd_reg_start = 0x0,
>> +	.nandc_offset = 0x30000,
>>   };
>>   
>>   static const struct qcom_nandc_props ipq8074_nandc_props = {
>> @@ -3491,6 +3494,7 @@ static const struct qcom_nandc_props ipq8074_nandc_props = {
>>   	.is_bam = true,
>>   	.is_qpic = true,
>>   	.dev_cmd_reg_start = 0x7000,
>> +	.nandc_offset = 0x30000,
>>   };
>>   
>>   static const struct qcom_nandc_props sdx55_nandc_props = {
>> @@ -3498,7 +3502,8 @@ static const struct qcom_nandc_props sdx55_nandc_props = {
>>   	.is_bam = true,
>>   	.is_qpic = true,
>>   	.qpic_v2 = true,
>> -	.dev_cmd_reg_start = 0x7000,
>> +	.dev_cmd_reg_start = 0x0,
> 
> What is this change?
Sorry, by mistake this get modified. Will fix in next revision.
> 
> - Mani
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/3] mtd: rawnand: qcom: Fix onfi param page read

[Md Sadre Alam]

On 11/26/2024 11:11 AM, Manivannan Sadhasivam wrote:
> On Fri, Nov 22, 2024 at 02:29:32PM +0530, Md Sadre Alam wrote:
> 
> Please change subject to:
> 
> mtd: rawnand: qcom: Fix last codeword read in qcom_param_page_type_exec()
Ok
> 
>> For QPIC V2 onwards there is a separate register to read
>> last code word "QPIC_NAND_READ_LOCATION_LAST_CW_n".
>>
>> qcom_param_page_type_exec() is used to read only one code word
>> If it will get configure number of code words to 1 in QPIC_NAND_DEV0_CFG0
> 
> "If it configures the number of..."
Ok
> 
>> register then QPIC controller thinks its reading the last code word,
>> since we are having separate register to read the last code word,
>> we have to configure "QPIC_NAND_READ_LOCATION_LAST_CW_n" register
>> to fetch data from QPIC buffer to system memory.
>>
>> Without this change page read was failing with timeout error
>>
>> / # hexdump -C /dev/mtd1
>> [  129.206113] qcom-nandc 1cc8000.nand-controller: failure to read page/oob
>> hexdump: /dev/mtd1: Connection timed out
>>
>> This issue only seen on SDX targets since SDX target used QPICv2. But
>> same working on IPQ targets since IPQ used QPICv1.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 
>> ---
>>
>> Change in [v2]
>>
>> * Updated commit message
>>
>> * Added stable kernel tag
>>
>> * Replaced the buf_count value of 512 with the len in bytes.
>>
>> Change in [v1]
>>
>> * Resolved the issue with reading a single code word in the parameter
>>    page read
>>
>>   drivers/mtd/nand/raw/qcom_nandc.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index cc59461df72e..31ec3db1246d 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2859,7 +2859,12 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>>   	const struct nand_op_instr *instr = NULL;
>>   	unsigned int op_id = 0;
>>   	unsigned int len = 0;
>> -	int ret;
>> +	int ret, reg_base;
>> +
>> +	reg_base = NAND_READ_LOCATION_0;
>> +
>> +	if (nandc->props->qpic_v2)
>> +		reg_base = NAND_READ_LOCATION_LAST_CW_0;
>>   
>>   	ret = qcom_parse_instructions(chip, subop, &q_op);
>>   	if (ret)
>> @@ -2911,7 +2916,10 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>>   	op_id = q_op.data_instr_idx;
>>   	len = nand_subop_get_data_len(subop, op_id);
>>   
>> -	nandc_set_read_loc(chip, 0, 0, 0, len, 1);
> 
> nandc_set_read_loc() does changes the register offset based on QPIC version. So
> what exactly you are trying to fix here?
QPICv2 having separate register to copy last code word data from QPIC buffer to Memory.
e.g for 2K page nand total code word = 4, so to copy first three code word need to configure
NAND_LOCATIONn register , but to copy last code word need to configure NAND_LOCATIONn_LAST
register.
> 
> - Mani
> 
>> +	if (nandc->props->qpic_v2)
>> +		nandc_set_read_loc_last(chip, reg_base, 0, len, 1);
>> +	else
>> +		nandc_set_read_loc_first(chip, reg_base, 0, len, 1);
>>   
>>   	if (!nandc->props->qpic_v2) {
>>   		write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
>> -- 
>> 2.34.1
>>
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Manivannan Sadhasivam]

On Fri, Nov 22, 2024 at 02:29:30PM +0530, Md Sadre Alam wrote:
> v2:
>  * Updated commit message
>  * Added stable kernel tag
>  * Added Fixes tag
>  * Renamed the variable from offset_from_qpic to nandc_offset
>  * Set buf_count to 512 in the parameter page read
>  * Replaced the buf_count value of 512 with the len in bytes 
> 
> v1:
>  * These patches will fix the following:
>  * 1) onfi param page read which was broken by exec_op() patch.
>  * 2) Fixed offset passed to BAM from QPIC base
> 
> Md Sadre Alam (3):
>   mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
>   mtd: rawnand: qcom: Fix onfi param page read
>   mtd: rawnand: qcom: Fix read len for onfi param page
> 

Do you plan to respin this series? FYI, these are regressions, so should go in
early as possible.

- Mani

>  drivers/mtd/nand/raw/qcom_nandc.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> -- 
> 2.34.1
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Md Sadre Alam]

On 12/20/2024 3:00 PM, Manivannan Sadhasivam wrote:
> On Fri, Nov 22, 2024 at 02:29:30PM +0530, Md Sadre Alam wrote:
>> v2:
>>   * Updated commit message
>>   * Added stable kernel tag
>>   * Added Fixes tag
>>   * Renamed the variable from offset_from_qpic to nandc_offset
>>   * Set buf_count to 512 in the parameter page read
>>   * Replaced the buf_count value of 512 with the len in bytes
>>
>> v1:
>>   * These patches will fix the following:
>>   * 1) onfi param page read which was broken by exec_op() patch.
>>   * 2) Fixed offset passed to BAM from QPIC base
>>
>> Md Sadre Alam (3):
>>    mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
>>    mtd: rawnand: qcom: Fix onfi param page read
>>    mtd: rawnand: qcom: Fix read len for onfi param page
>>
> 
> Do you plan to respin this series? FYI, these are regressions, so should go in
> early as possible.
Waiting for SPI NAND series patches [1] to be merged (raw nand change 
patch 2-5). On top of that will repost these patches.

Thanks,
Alam.

1. 
https://lore.kernel.org/linux-arm-msm/20241120091507.1404368-1-quic_mdalam@quicinc.com/

> 
> - Mani
> 
>>   drivers/mtd/nand/raw/qcom_nandc.c | 23 ++++++++++++++++++-----
>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> -- 
>> 2.34.1
>>
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Miquel Raynal]

>> Do you plan to respin this series? FYI, these are regressions, so
>> should go in
>> early as possible.
> Waiting for SPI NAND series patches [1] to be merged (raw nand change
> patch 2-5). On top of that will repost these patches.

When there are comments on a series, the entire series usually gets
discarded, so it is now out of my sight. If you want the spi bits to be
applied, it must make sense to have them alone, but you can send a new
version of the spi bits alone if you feel like the raw nand patches
aren't ready. But otherwise please fix the series and send a new
version.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Md Sadre Alam]

On 12/24/2024 1:39 PM, Miquel Raynal wrote:
>>> Do you plan to respin this series? FYI, these are regressions, so
>>> should go in
>>> early as possible.
>> Waiting for SPI NAND series patches [1] to be merged (raw nand change
>> patch 2-5). On top of that will repost these patches.
> 
> When there are comments on a series, the entire series usually gets
> discarded, so it is now out of my sight. If you want the spi bits to be
> applied, it must make sense to have them alone, but you can send a new
> version of the spi bits alone if you feel like the raw nand patches
> aren't ready. But otherwise please fix the series and send a new
> version.
Sure Will create 2 different series one for raw nand along with SDX75 
bug fixes added and another series for SPI NAND support and repost.
> 
> Thanks,
> Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] QPIC v2 fixes for SDX75

[Manivannan Sadhasivam]

On Tue, Dec 24, 2024 at 10:52:03AM +0530, Md Sadre Alam wrote:
> 
> 
> On 12/20/2024 3:00 PM, Manivannan Sadhasivam wrote:
> > On Fri, Nov 22, 2024 at 02:29:30PM +0530, Md Sadre Alam wrote:
> > > v2:
> > >   * Updated commit message
> > >   * Added stable kernel tag
> > >   * Added Fixes tag
> > >   * Renamed the variable from offset_from_qpic to nandc_offset
> > >   * Set buf_count to 512 in the parameter page read
> > >   * Replaced the buf_count value of 512 with the len in bytes
> > > 
> > > v1:
> > >   * These patches will fix the following:
> > >   * 1) onfi param page read which was broken by exec_op() patch.
> > >   * 2) Fixed offset passed to BAM from QPIC base
> > > 
> > > Md Sadre Alam (3):
> > >    mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
> > >    mtd: rawnand: qcom: Fix onfi param page read
> > >    mtd: rawnand: qcom: Fix read len for onfi param page
> > > 
> > 
> > Do you plan to respin this series? FYI, these are regressions, so should go in
> > early as possible.
> Waiting for SPI NAND series patches [1] to be merged (raw nand change patch
> 2-5). On top of that will repost these patches.

This series fixes regression, so these should be merged first. Do not make fixes
depend on feature.

- Mani

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/