[PATCH 0/2] QPIC v2 fixes for SDX75

[PATCH 0/2] QPIC v2 fixes for SDX75

[Md Sadre Alam]

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 (2):
  mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM
  mtd: rawnand: qcom: Fix onfi param page read

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

-- 
2.34.1

[PATCH 1/2] 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.

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

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

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---
 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..34ee8555fb8a 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->offset_from_qpic + (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 offset_from_qpic;
 };
 
 /* 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,
+	.offset_from_qpic = 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,
+	.offset_from_qpic = 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,
+	.offset_from_qpic = 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,
+	.offset_from_qpic = 0x30000,
 };
 
 /*
-- 
2.34.1

[PATCH 2/2] 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 we will 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.

Also there is 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.

Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 34ee8555fb8a..6487f2126833 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,14 +2916,17 @@ 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);
 		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

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

[Manivannan Sadhasivam]

On Tue, Nov 19, 2024 at 02:50:57PM +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

You mean 'QPIC IP' here? But is it QPIC or NANDc? I guess the later.

> address to be configured in cmd descriptors. This is leading to a
> different address actually being used in HW, leading to wrong value
> read.
> 

This doesn't clearly say what the actual issue is. IIUC, the 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 sdxpinn,

Please use actual product names and not internal names. I believe you are
referring to SDX55/SDX65 here.

> ipq etc. but issue is masked in older targets due to lower 18 bits of QPIC
> base address being zero leading to expected address generation.
> 
> Sdxpinn     : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
> Sdxnightjar : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
> older targets.

Same here.

> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

Please add relevant Fixes tag.

> ---
>  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..34ee8555fb8a 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->offset_from_qpic + (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 offset_from_qpic;

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,
> +	.offset_from_qpic = 0x30000,

How 0x30000 is supposed to work? You said the NANDc ignores lower 18 bits, but
this has 17th and 18th bits set.

- Mani

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

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

[Manivannan Sadhasivam]

On Tue, Nov 19, 2024 at 02:50:58PM +0530, Md Sadre Alam wrote:
> 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 we will configure number of code words to 1 in QPIC_NAND_DEV0_CFG0

No 'we' in commit message. Also use imperative tone.

> 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.
> 
> Also there is 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.
> 

This is a separate fix and should be in a separate patch.

> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")

Please describe the impact of the issue. Add relevant failure messages, affected
SoC names etc...

Finally, you should also CC stable list to backport the fixes.

- Mani

> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 34ee8555fb8a..6487f2126833 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,14 +2916,17 @@ 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);
>  		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
> 

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

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

[Md Sadre Alam]

On 11/20/2024 12:31 PM, Manivannan Sadhasivam wrote:
> On Tue, Nov 19, 2024 at 02:50:57PM +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
> 
> You mean 'QPIC IP' here? But is it QPIC or NANDc? I guess the later.
It's QPIC IP only.
> 
>> address to be configured in cmd descriptors. This is leading to a
>> different address actually being used in HW, leading to wrong value
>> read.
>>
> 
> This doesn't clearly say what the actual issue is. IIUC, the 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.
Yes correct. If QPIC address = 0x07980000 and QPIC_EBI2NAND address = 0x079b0000
the the diff is 0x30000, this is the actual offset expected by QPIC RTL code.
and RTL needs only 18-bit offset.
> 
>> Older targets also used same configuration (lower 24 bits) like sdxpinn,
> 
> Please use actual product names and not internal names. I believe you are
> referring to SDX55/SDX65 here.
Ok , will change in next revision.
> 
>> ipq etc. but issue is masked in older targets due to lower 18 bits of QPIC
>> base address being zero leading to expected address generation.
>>
>> Sdxpinn     : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
>> Sdxnightjar : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
>> older targets.
> 
> Same here.
Ok
> 
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> 
> Please add relevant Fixes tag.
Ok
> 
>> ---
>>   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..34ee8555fb8a 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->offset_from_qpic + (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 offset_from_qpic;
> 
> nandc_offset?
Ok
> 
>>   };
>>   
>>   /* 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,
>> +	.offset_from_qpic = 0x30000,
> 
> How 0x30000 is supposed to work? You said the NANDc ignores lower 18 bits, but
> this has 17th and 18th bits set.
Not this address 0x30000, this the diff b/w QPIC base and EBI2NAND base. The 18-bits we have see
on this address 0x07980000 and this address 0x01C98000.
> 
> - Mani
> 

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

[Md Sadre Alam]

On 11/20/2024 12:36 PM, Manivannan Sadhasivam wrote:
> On Tue, Nov 19, 2024 at 02:50:58PM +0530, Md Sadre Alam wrote:
>> 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 we will configure number of code words to 1 in QPIC_NAND_DEV0_CFG0
> 
> No 'we' in commit message. Also use imperative tone.
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.
>>
>> Also there is 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.
>>
> 
> This is a separate fix and should be in a separate patch.
Ok
> 
>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
> 
> Please describe the impact of the issue. Add relevant failure messages, affected
> SoC names etc...
Sure, Will update in next revision.
> 
> Finally, you should also CC stable list to backport the fixes.
Ok
> 
> - Mani
> 
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>   drivers/mtd/nand/raw/qcom_nandc.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index 34ee8555fb8a..6487f2126833 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,14 +2916,17 @@ 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);
>>   		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
>>
> 

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

[Manivannan Sadhasivam]

On Thu, Nov 21, 2024 at 11:33:13AM +0530, Md Sadre Alam wrote:
> 
> 
> On 11/20/2024 12:31 PM, Manivannan Sadhasivam wrote:
> > On Tue, Nov 19, 2024 at 02:50:57PM +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
> > 
> > You mean 'QPIC IP' here? But is it QPIC or NANDc? I guess the later.
> It's QPIC IP only.

Hmm, so what is the difference between QPIC and NANDc?

> > 
> > > address to be configured in cmd descriptors. This is leading to a
> > > different address actually being used in HW, leading to wrong value
> > > read.
> > > 
> > 
> > This doesn't clearly say what the actual issue is. IIUC, the 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.
> Yes correct. If QPIC address = 0x07980000 and QPIC_EBI2NAND address = 0x079b0000
> the the diff is 0x30000, this is the actual offset expected by QPIC RTL code.
> and RTL needs only 18-bit offset.

Okay. So the driver used to pass 0x30000 + offset in older targets and on newer
ones starting from SDX75, 0x30000 is not passed correctly due to the changed
QPIC base address.

Please mention it clearly in description.

- Mani

> > 
> > > Older targets also used same configuration (lower 24 bits) like sdxpinn,
> > 
> > Please use actual product names and not internal names. I believe you are
> > referring to SDX55/SDX65 here.
> Ok , will change in next revision.
> > 
> > > ipq etc. but issue is masked in older targets due to lower 18 bits of QPIC
> > > base address being zero leading to expected address generation.
> > > 
> > > Sdxpinn     : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
> > > Sdxnightjar : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
> > > older targets.
> > 
> > Same here.
> Ok
> > 
> > > 
> > > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> > 
> > Please add relevant Fixes tag.
> Ok
> > 
> > > ---
> > >   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..34ee8555fb8a 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->offset_from_qpic + (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 offset_from_qpic;
> > 
> > nandc_offset?
> Ok
> > 
> > >   };
> > >   /* 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,
> > > +	.offset_from_qpic = 0x30000,
> > 
> > How 0x30000 is supposed to work? You said the NANDc ignores lower 18 bits, but
> > this has 17th and 18th bits set.
> Not this address 0x30000, this the diff b/w QPIC base and EBI2NAND base. The 18-bits we have see
> on this address 0x07980000 and this address 0x01C98000.
> > 
> > - Mani
> > 

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

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

[Md Sadre Alam]

On 11/21/2024 12:29 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 21, 2024 at 11:33:13AM +0530, Md Sadre Alam wrote:
>>
>>
>> On 11/20/2024 12:31 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Nov 19, 2024 at 02:50:57PM +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
>>>
>>> You mean 'QPIC IP' here? But is it QPIC or NANDc? I guess the later.
>> It's QPIC IP only.
> 
> Hmm, so what is the difference between QPIC and NANDc?
QPIC is wrapper which integrates NANDc. So only QPIC (Qualcomm Parallel
Interface Controller) will be exposed for interface.
> 
>>>
>>>> address to be configured in cmd descriptors. This is leading to a
>>>> different address actually being used in HW, leading to wrong value
>>>> read.
>>>>
>>>
>>> This doesn't clearly say what the actual issue is. IIUC, the 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.
>> Yes correct. If QPIC address = 0x07980000 and QPIC_EBI2NAND address = 0x079b0000
>> the the diff is 0x30000, this is the actual offset expected by QPIC RTL code.
>> and RTL needs only 18-bit offset.
> 
> Okay. So the driver used to pass 0x30000 + offset in older targets and on newer
> ones starting from SDX75, 0x30000 is not passed correctly due to the changed
> QPIC base address.
Yes, correct. In SDX75 the first 18-bits of QPIC base address are non-zero.
> 
> Please mention it clearly in description.
Ok
> 
> - Mani
> 
>>>
>>>> Older targets also used same configuration (lower 24 bits) like sdxpinn,
>>>
>>> Please use actual product names and not internal names. I believe you are
>>> referring to SDX55/SDX65 here.
>> Ok , will change in next revision.
>>>
>>>> ipq etc. but issue is masked in older targets due to lower 18 bits of QPIC
>>>> base address being zero leading to expected address generation.
>>>>
>>>> Sdxpinn     : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
>>>> Sdxnightjar : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
>>>> older targets.
>>>
>>> Same here.
>> Ok
>>>
>>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>
>>> Please add relevant Fixes tag.
>> Ok
>>>
>>>> ---
>>>>    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..34ee8555fb8a 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->offset_from_qpic + (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 offset_from_qpic;
>>>
>>> nandc_offset?
>> Ok
>>>
>>>>    };
>>>>    /* 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,
>>>> +	.offset_from_qpic = 0x30000,
>>>
>>> How 0x30000 is supposed to work? You said the NANDc ignores lower 18 bits, but
>>> this has 17th and 18th bits set.
>> Not this address 0x30000, this the diff b/w QPIC base and EBI2NAND base. The 18-bits we have see
>> on this address 0x07980000 and this address 0x01C98000.
>>>
>>> - Mani
>>>
> 

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

[Miquel Raynal]

Hello,

On 19/11/2024 at 14:50:56 +0530, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:

> 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

Would you mind adding Fixes and Cc: stable tags to each patch?

Thanks,
Miquèl

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

[Md Sadre Alam]

On 12/2/2024 10:20 PM, Miquel Raynal wrote:
> 
> Hello,
> 
> On 19/11/2024 at 14:50:56 +0530, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
> 
>> 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
> 
> Would you mind adding Fixes and Cc: stable tags to each patch?
Ok, will add in next revision.
> 
> Thanks,
> Miquèl
>