Re: [RFC PATCH 1/2] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Fri, 12 Jan 2018 13:58:11 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 2018-01-12 11:59, Boris Brezillon wrote:
> > Hi Stefan,
> > 
> > On Fri, 12 Jan 2018 00:50:36 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> This reworks the driver to make use of ->exec_op() callback. The
> >> command sequencer of the VF610 NFC aligns well with the new ops
> >> interface.  
> > 
> > That's great news!
> >   
> >>
> >> Currently we handle reset and read id pattern separately. The
> >> read id pattern uses registers for the returned data, using a
> >> dedicated function helps to special case the data in op.
> >>
> >> The parameter page handling is rather ad-hoc currently: The
> >> stack calls exec_op twice, once to read the paramter page and
> >> a second time to tranfer data. The code/controller can not
> >> handle this steps separately, hence just transfer data already
> >> in the first call and just memcpy from the buffer in the second
> >> call. This needs a small change in the nand_base.c file.  
> > 
> > Hm, I think it's using change_column after the read_param_page command,
> > so it's not exactly a 'data xfer' only step. Can't your controller send
> > only a command or address cycle without any associated data xfers on
> > the bus?  
> 
> Hm, I was looking at ONFI, where it does not do the column change:
> http://git.infradead.org/linux-mtd.git/blob/8878b126df769831cb2fa4088c3806538e8305f5:/drivers/mtd/nand/nand_base.c#l5110
> 
> I am pretty sure that the controller can handle command and address
> separately, but not sure about the successive data transfer...
> 
> > 
> > Anyway, if you really have to do the READ_PARAM_PAGE in a single step,
> > you'll have to patch the core to pass a valid buffer and len != 0 when
> > calling nand_read_param_page_op().  
> 
> I see.
> 
> >   
> >>
> >> The current state seems to pass MTD tests on a Colibri VF61.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> Hi Boris, Hi Miquel,
> >>
> >> Thanks for your work on the new NAND interface. The VF610 NFC
> >> definitly matches better the ->exec_op() interface.  
> > 
> > And thanks for transitioning to this new interface.
> >   
> >>
> >> This is still in early stage but seems to boot a Linux as rootfs
> >> successfully. Before working on further simplification I wanted
> >> to get some feedback on the general idea.  
> > 
> > Sure.
> >   
> >>
> >> If needed, the data sheet of the controller can be optained
> >> publicly (Vybrid Reference Manual, Chapter 10.3).  
> > 
> > I'll have a look, thanks for the pointer.
> >   
> >>
> >> Some questions from my side:
> >> - Parameter page: Why is exec_op called twice currently? This
> >>   seems to be problematic for this controller. Are there plans
> >>   to integrate the two calls in a single op sequence?  
> > 
> > I don't know, but nothing is set in stone.
> >   
> >> - Row/Column addresses: The controller seems to separate those
> >>   two, hence the driver "guesses" which address bytes need to
> >>   go where.. Maybe it would be better to separate that on ops
> >>   level?  
> > 
> > Miquel had a similar need for the new marvell NAND driver, so maybe we
> > can store this information at the nand_chip level:
> > 
> > 	unsigned int row_cycles;
> > 	unsigned int column_cycles;
> > 
> > I still want to keep the address data in an array of bytes, but thanks
> > to the [row,column]_cycles information you'll be able to easily convert
> > this array of bytes into row and column addresses.  
> 
> That sounds good enough for my case.
> 
> >   
> >> - Separation is often needed by command. One can make an educated
> >>   guess what kind of command is going to be passed by filtering
> >>   the appropriate ops. Maybe it would be good if the ops parser
> >>   can filter by command?  
> > 
> > I'll have to check the datasheet you pointed out. We're not closed to
> > the idea of filtering things by particular opcodes, but most of the time
> > the controller is not hardcoded this way, and what they refer as READID
> > or READSTATUS is not about sending a NAND_READ_ID or a NAND_READ_STATUS
> > opcode, but following the sequence imposed by these operations (READID
> > is CMD+ADDR+DATA_IN, READSTATUS is CMD+DATA_IN). Which means, if you
> > have another operation which uses the same sequence but a different
> > opcode, the controller is able to handle it.
> >   
> 
> The chapter 10.3.4.7 Flash Command Code Description is probably most
> interesting in that regard. With the old interface, I used predefined
> command codes, with the new interface I can dynamically generate the
> command code from the operations provided by the stack.
> 
> However, some seem to be handled specially: There is single bit for
> "Read ID". And the bytes returned by the controller are stored in
> register instead of the regular data area. These are the reasons I do
> use special handling right now...

Just don't use those special handling if you can avoid it. I mean, a
READID is just a regular READ with only 1 address cycle and X bytes of
DATA:
	NFC_SECSZ = X (information passed by the core)
	NFC_CMD2.CODE = SEND_CMD1 | SEND_COL_ADDR1 | READ_DATA
	...

This way you should have the READID data in the same region you have
regular data.

Same goes for the READ_STATUS operation:
	NFC_SECSZ = 1 (or 0 if no DATA_OUT is requested)
	NFC_CMD2.CODE = SEND_CMD1 [| READ_DATA]

And AFAICT, you can do naked READ/WRITE operations as well:

	NFC_SECSZ = X (the size of the read/write op)
	NFC_CMD2.CODE = READ_DATA or WRITE_DATA


With this approach, the final implementation should be much simpler
than what we have today.