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 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