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

[Md Sadre 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@xxxxxxxxxxxxxxx
> > Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
> > Tested-by: Lakshmi Sowjanya D <quic_laksd@xxxxxxxxxxx>
> > Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
> > ---
>
> <...>
>
> >   /*
> > 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