| Message ID | 20250310120906.1577292-2-quic_mdalam@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | QPIC v2 fixes for SDX75 | expand |
On Mon, Mar 10, 2025 at 05:39:03PM +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 What is this address? Is it coming from DT? > 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 You gave no explanation on how this 0x30000 offset came into picture. I gave the reasoning in v2: "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." Then you replied: "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." No one outside Qcom knows what QPIC_BASE and QPIC_EBI2NAND are. We just know the NANDc address mentioned in DT, which corresponds to 0x01b30000 for SDX55. Please reword the commit message to present the full picture and not half baked info. This is v3, I see no improvement in the commit message, sorry. > 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 There is no address as '0x1B00000' in DT. - Mani
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. Regards, Gabor
On 3/18/2025 1:03 PM, Manivannan Sadhasivam wrote: > On Mon, Mar 10, 2025 at 05:39:03PM +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 > > What is this address? Is it coming from DT? > >> 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 > > You gave no explanation on how this 0x30000 offset came into picture. I gave the > reasoning in v2: > > "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." > > Then you replied: > > "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." > > No one outside Qcom knows what QPIC_BASE and QPIC_EBI2NAND are. We just know the > NANDc address mentioned in DT, which corresponds to 0x01b30000 for SDX55. > > Please reword the commit message to present the full picture and not half baked > info. This is v3, I see no improvement in the commit message, sorry. > >> 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 > > There is no address as '0x1B00000' in DT. Mani, Please see if this commit message would be acceptable? The BAM command descriptor provides only 18 bits to specify the NAND register offset. Additionally, in the BAM command descriptor, the NAND register offset is supposed to be specified as "(NANDc offset - BAM base offset) + reg_off". Since, the nand driver isn't aware of the BAM base offset, have the value of "NANDc offset - BAM base offset" in a new field 'nandc_offset' in the NAND properties structure and use it while preparing the descriptor. Previously, the NAND driver was incorrectly specifying the NAND register offset directly in the BAM descriptor. Thanks Alam
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
On Thu, Mar 20, 2025 at 11:23:40AM +0530, Md Sadre Alam wrote: > > > On 3/18/2025 1:03 PM, Manivannan Sadhasivam wrote: > > On Mon, Mar 10, 2025 at 05:39:03PM +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 > > > > What is this address? Is it coming from DT? > > > > > 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 > > > > You gave no explanation on how this 0x30000 offset came into picture. I gave the > > reasoning in v2: > > > > "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." > > > > Then you replied: > > > > "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." > > > > No one outside Qcom knows what QPIC_BASE and QPIC_EBI2NAND are. We just know the > > NANDc address mentioned in DT, which corresponds to 0x01b30000 for SDX55. > > > > Please reword the commit message to present the full picture and not half baked > > info. This is v3, I see no improvement in the commit message, sorry. > > > > > 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 > > > > There is no address as '0x1B00000' in DT. > > Mani, > > Please see if this commit message would be acceptable? > > The BAM command descriptor provides only 18 bits to specify > the NAND register offset. Additionally, in the BAM command > descriptor, the NAND register offset is supposed to be > specified as "(NANDc offset - BAM base offset) + reg_off". Isn't it, (NANDc base - BAM base)? 'offset' is not valid here. And also, you are just mixing the names everywhere. Here you say, NANDc base and BAM base, but in patch 4: "NAND register addresses to be computed based on the NAND register offset from QPIC base". So the second address is BAM or QPIC? Please be consistent with naming. > Since, the nand driver isn't aware of the BAM base offset, > have the value of "NANDc offset - BAM base offset" in a new > field 'nandc_offset' in the NAND properties structure and use > it while preparing the descriptor. > And what about 'nandc_offset'? NANDc is already the term used for NAND controller which has the base address of 0x01b30000 in DT. So clearly, the name of the offset variable is not correct. > Previously, the NAND driver was incorrectly specifying the > NAND register offset directly in the BAM descriptor. > No. Previously, the driver was specifying the NANDc base address in the BAM descriptor. You are now trying to pass the register offset. - Mani
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index 5eaa0be367cd..5443cb918e0b 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -2360,6 +2360,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = { .supports_bam = false, .use_codeword_fixup = true, .dev_cmd_reg_start = 0x0, + .nandc_offset = 0x30000, }; static const struct qcom_nandc_props ipq4019_nandc_props = { @@ -2367,6 +2368,7 @@ static const struct qcom_nandc_props ipq4019_nandc_props = { .supports_bam = true, .nandc_part_of_qpic = true, .dev_cmd_reg_start = 0x0, + .nandc_offset = 0x30000, }; static const struct qcom_nandc_props ipq8074_nandc_props = { @@ -2374,6 +2376,7 @@ static const struct qcom_nandc_props ipq8074_nandc_props = { .supports_bam = true, .nandc_part_of_qpic = true, .dev_cmd_reg_start = 0x7000, + .nandc_offset = 0x30000, }; static const struct qcom_nandc_props sdx55_nandc_props = { @@ -2382,6 +2385,7 @@ static const struct qcom_nandc_props sdx55_nandc_props = { .nandc_part_of_qpic = true, .qpic_version2 = true, .dev_cmd_reg_start = 0x7000, + .nandc_offset = 0x30000, }; /* 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)) /* Returns the dma address for reg read buffer */ #define reg_buf_dma_addr(chip, vaddr) \ @@ -458,6 +458,7 @@ struct qcom_nandc_props { bool nandc_part_of_qpic; bool qpic_version2; bool use_codeword_fixup; + u32 nandc_offset; }; void qcom_free_bam_transaction(struct qcom_nand_controller *nandc);
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> --- Change in [v3] * Updated commit message * Removed dev_cmd_reg_start = 0 , which was wrongely got added 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 | 4 ++++ include/linux/mtd/nand-qpic-common.h | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-)