| Message ID | 20241119092058.480363-1-quic_mdalam@quicinc.com |
|---|---|
| Headers | show |
| Series | QPIC v2 fixes for SDX75 | expand |
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 >>> >
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
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 >