| Message ID | 20241119092058.480363-2-quic_mdalam@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | QPIC v2 fixes for SDX75 | expand |
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
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 >
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 > >
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 >>> >
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, }; /*
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(-)