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@xxxxxxxxxxx>
> >
> > 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
> >
--
மணிவண்ணன் சதாசிவம்
