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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <bu2msy7ngxzjk54rdq3fmgvww5rrn5ogjm4uq2vt2pwp6toxfw@ytvk6x6wcgi5>
Date: Tue, 25 Mar 2025 20:02:54 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Md Sadre Alam <quic_mdalam@...cinc.com>
Cc: miquel.raynal@...tlin.com, richard@....at, vigneshr@...com, 
	broonie@...nel.org, bbrezillon@...nel.org, linux-mtd@...ts.infradead.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org
Subject: Re: [PATCH v3 1/4] mtd: rawnand: qcom: Pass 18 bit offset from QPIC
 base address to BAM

On Thu, Mar 20, 2025 at 11:23:40AM +0530, Md Sadre Alam wrote:
> 
> 
> 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".

Isn't it, (NANDc base - BAM base)? 'offset' is not valid here.

And also, you are just mixing the names everywhere. Here you say, NANDc base
and BAM base, but in patch 4:

"NAND register addresses to be computed based on the NAND register offset from
QPIC base". So the second address is BAM or QPIC?

Please be consistent with naming.

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

And what about 'nandc_offset'? NANDc is already the term used for NAND
controller which has the base address of 0x01b30000 in DT. So clearly, the name
of the offset variable is not correct.

> 	Previously, the NAND driver was incorrectly specifying the
> 	NAND register offset directly in the BAM descriptor.
> 

No. Previously, the driver was specifying the NANDc base address in the BAM
descriptor. You are now trying to pass the register offset.

- Mani

-- 
மணிவண்ணன் சதாசிவம்