[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 at vger.kernel.org
>> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
>> Signed-off-by: Md Sadre Alam <quic_mdalam at 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
>