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

[Md Sadre Alam <quic_mdalam@quicinc.com>]

On 3/18/2025 8:22 PM, Gabor Juhos wrote:
> 2025. 03. 10. 13:09 keltezéssel, Md Sadre Alam írta:
>> 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
>>
>> 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 | 0x1B00000 (Lower 18 bits are zero) Same for
>> older targets.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
>> Tested-by: Lakshmi Sowjanya D <quic_laksd@quicinc.com>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
> 
> <...>
> 
>>   /*
>> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
>> index cd7172e6c1bb..6268f08b9d19 100644
>> --- a/include/linux/mtd/nand-qpic-common.h
>> +++ b/include/linux/mtd/nand-qpic-common.h
>> @@ -200,7 +200,7 @@
>>   #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))
> 
> The macro has no parameter named 'nandc', so this works only when there is an
> identifier with that name in the code where the macro is used.
> 
> Additionally, the macro will no longer return the physical address of a register
> after the change, so both the comment before the macro and the name of the macro
> will be misleading.
> 
> Since the macro is used only in the qcom_prep_bam_dma_desc_cmd() function to
> compute the 'addr' parameter for the bam_prep_ce{_le32}() functions, maybe it
> would be better to get rid of it completely, and do the computation directly in
> the function instead.
Ok, Will handle in next revision.
> 
> Regards,
> Gabor

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