Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne <helmut.grohne@intenta.de>]

Hi Naga,

On Thu, Jun 13, 2019 at 10:18:00AM +0000, Naga Sureshkumar Relli wrote:
> I spent much of time to address all your comments.
> All are addressed and tested. except the above one(address offset calculation)
> I didn't see any issue with the address calculation.

Let me first point out that this comment was not trying to imply a bug.
I was trying to understand the code by comparing it to similar code and
that turned up an inconsistency, which can be intentional or a bug in
either of the sides being compared.

> for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 	nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> 			 (8 * i);
> }
> If you go through the nand_base.c, there nand_fill_column_cycles() API, fills the first two or one address cycle
> Based on bus width and page size.
> That means, addrs[0]/[1] will be updated here.

The problem at hand is that `addrs` is imprecise. In this code, there
are `instr->ctx.addr.addrs`, `addrs`, and `nfc_op->addrs`. All of them
are different. My original remark was targeting the possible confusion
of these different `addrs`.

> And the page is updated to the next offsets.
> In the similar way we have to extract the offsets in driver.
> So the first four address bytes are stored using the above for() loop and if the
> Address cycles are more than 4, then store the remaining offsets as well.
> 
> I just compared the offsets that are updated in driver with the offsets(page and column) that the frame work(nand_base.c) is sending, and the offsets are same.
> I have also checked these offsets with older driver(not exec_op() implemented) and both are matching.
> 
> So I didn't see any issue with this addrs calculation.
> As per the statement mentioned by you, this driver consumes addr[0], addr[1], addr[2], addr[3] and
> If more address cycles needed, then addr[4] and addr[5]. This is correct.

Again, the lack of precision makes it difficult to discuss the matter.
You refer to `addr`, but there is no `addr`. I assume that you meant
`addrs` here. Based on that assumption, your second last statement is
wrong. The driver consumes `addrs[0]|addrs[-offset]` rather than
`addrs[0]` as the first byte.  Then it proceeds consuming
`addrs[1-offset]` instead of `addrs[1]`, `addrs[2-offset]` instead of
`addrs[2]`, and `addrs[3-offset]` instead of `addrs[3]`. Finally it
consumes `addrs[4]` and `addrs[5]` if more cycles are needed.

I would not have commented the code if it were actually using `addrs[0]`
through `addrs[5]`. Your description looks reasonable to me, but it
doesn't match the code.

I'm looking forward to the next version of the patch.

Helmut