Hi Steven, steven.seeger@xxxxxxxxxxxxxxxxx wrote on Tue, 14 May 2024 17:57:46 +0000: > On Tuesday, May 7, 2024, Miquel Raynal wrote: > > >So, if the fields are empty, especially mtd->writesize which is *always* > >set quite rapidly after identification, let's skip the sanity checks. > > I noticed this when first looking at my board with the bitflip in a NAND chip's parameter page. I just assumed that since I was setting it up to do column change operations, I needed to add this in at the time. Looking at it now, since this information is being supplied by me before the scan, it's wrong. So I agree it's a bug, but I didn't think about it again since I was tackling the bug of trying to read additional parameter page copies further down the page due to the bitflip. > > I don't have access to the board right now, but when I get to it again I can try this patch. I will need to remove what I already added in to check and reply back. It may be a few weeks, though. > > On another note, I think that this entire API would benefit from discouraging hybrid approaches. I implement function overloads for things like ecc.read_page, write_page, read_page_raw, etc, but also use the exec function for things like erase, read id, read parameter page, etc. I maybe did it "wrong" but it works. Past drivers I've done use the legacy cmdfunc, so this was my first attempt at using the command parser. I suspect there are a lot of people like me writing drivers for proprietary hardware that uses FPGAs to do some of the NAND interaction, rather than direct chip access as the API was originally designed for. I don't know what you mean with direct chip access. ->cmd_ctrl() and ->cmdfunc() were desperately too simple and many drivers started guessing what the core was trying to do, making it very hard to extend/modify the core without breaking them all. This was sign of an inadequate design and hence ->exec_op() (providing all the operation) was introduced. Just to make it clear, the original APIs were totally fine back then, but controllers evolved, became smarter^W more complex, until the APIs were no longer fitting. > So, to explain further, read_page triggers my addr/chip select, read page command, and retrieving the buffer. Read parameter page goes through the command parser, as does the column change op, with some state variables to keep track of where in the read cycle we are so that each copy of the parameter page data can be accessed in the buffer. I lament the lack of consistency here. But, it works and the customer is unlikely to want to change anything at this point. :) The logic is: - Early at boot you need to identify the chip, its parameters, its configuration, etc. -> exec_op() is used - During normal operation, it's time for I/Os. Using ->exec_op() again can work, but most of the time these operations can be done faster with a more custom approach, especially since most controller drivers embed and ECC engine that also needs to be managed during these accesses. -> your page helpers are here for that - During debugging you might want to perform raw page reads, performance does not matter here but the data layout in the chip is NAND controller and ECC engine specific, while the user expected layout is: [all the data][all the oob]. -> your raw page helpers are here for that And there are standard helpers provided in the core if your controller does not need specific implementations. You may want to use them because it makes your life easier, they will use ->exec_op(). Thanks, Miquèl
