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

[Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@xxxxxxxxxx>
> Sent: Monday, April 29, 2019 5:48 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: bbrezillon@xxxxxxxxxx; miquel.raynal@xxxxxxxxxxx; richard@xxxxxx;
> dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> nagasureshkumarrelli@xxxxxxxxx
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
> > But just wanted to know, do you see issues with these __force and __iomem castings?
> 
> I only see a minor issue: They're (deliberately) lengthy. Using many of them diverts attention
> of the reader. Therefore, my proposal attempted to reduce their frequency. The only issue I see
> here is readability.
> 
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't constant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
> 
> Not quite. You're getting a clock reference in probe and then repeatedly access the frequency
> elswhere. I am suggesting that you get the clock frequency during probe and never save the
> clock reference to a struct.
> 
> > > > +		case NAND_OP_ADDR_INSTR:
> > > > +			offset = nand_subop_get_addr_start_off(subop, op_id);
> > > > +			naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
> > > > +			addrs = &instr->ctx.addr.addrs[offset];
> > > > +			nfc_op->addrs = instr->ctx.addr.addrs[offset];
> > > > +			for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> > > > +				nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you really meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for
> loop?
> 
> I compared this code to marvell_nand.c and noticed a subtle difference.
> Both snippets read 6 address bytes and consume them in a driver-specific way. Now which
> address bytes are consumed differs.
> 
> marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
> offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
> instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
> (In my previous mail, I didn't notice that it was also consuming the offset index.)
> 
> I would have expected this behaviour to be consistent between different drivers. If I assume
> marvell_nand.c to do the right thing and pl353_nand.c to be wrong (which is not necessarily a
> correct assumption), then the code woule likely becom:
> 
> 	addrs = &instr->ctx.addr.addrs[offset];
> 	for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 		nfc_op->addrs |= addrs[i] << (8 * i);
> 		              // ^^^^^
> 	}
> 
> Hope this helps.
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.
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.
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.

Please let me know, what exactly I am missing.

Thank you for your inputs and sorry for the delay, as I am on leave for some days.

Regards,
Naga Sureshkumar Relli
> 
> Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/