Re: [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs

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

On Wed, 29 Mar 2017 12:02:56 +0200
Jan Glauber <jan.glauber@caviumnetworks.com> wrote:

> > > +static void cvm_nand_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
> > > +{
> > > +	struct nand_chip *nand = mtd_to_nand(mtd);
> > > +	struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > > +
> > > +	memcpy(tn->buf.dmabuf + tn->buf.data_len, buf, len);
> > > +	tn->buf.data_len += len;
> > > +}  
> > 
> > It seems that cvm_nand_read/write_byte/buf() are returning data that
> > have already been retrieved (problably during the ->cmdfunc() phase).  
> 
> Yes.
> 
> > That's not how it's supposed to work. The core is expecting the data
> > transfer to be done when ->read/write_buf() is called. Doing that in  
> > ->cmdfunc() is risky, because when you're there you have no clue about  
> > how much bytes the core expect.  
> 
> It seems to work fine, I've never seen the core trying to do more bytes in
> the read/write_buf() then requested in ->cmdfunc().

We already had problems in the past: when the core evolves to handle
new NAND chips it might decide to read a bit more data than it used to
be, and assuming that your driver will always take the right decision
based on the information passed to ->cmdfunc() is a bit risky.

I still have the plan to provide a better interface allowing drivers to
execute the whole operation sequence (cmd+addr+data cycles), but it's
not there yet (see [1] for more details).
If you're okay to volunteer, I can help you with design this new hook
which should probably make your life easier for the rest of the driver
code (and also help me improve existing drivers ;-)).

Otherwise, you should try to implement ->cmd_ctrl() and try to transfer
data on the bus only when ->read/write_buf() are called (sometime it's
not possible).


> >   
> > > +
> > > +static void cvm_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
> > > +				  int column, int page_addr)  
> > 
> > Did you try implementing ->cmd_ctrl() instead of ->cmdfunc(). Your
> > controller seems to be highly configurable and, at first glance, I think
> > you can simplify the driver by implementing ->cmd_ctrl() and relying on
> > the default ->cmdfunc() implementation.
> >   
> 
> I've looked at the sunxi_nand driver but ->cmd_ctrl() is very different
> from ->cmdfunc() and the later looks like a better match for our controller.
> 
> The Cavium controller needs to write the commands (NAND_CMD_READ0, etc.)
> into its pseudo instructions (see ndf_queue_cmd_cle()).
> So how can I do this low-level stuff with ->cmd_ctrl()?

I'd say that it's actually matching pretty well what is passed to
->cmd_ctrl().
For each call to ->cmd_ctrl() you have the information about the type
of access that is made on the bus:
- if the NAND_CLE flag is set in ctrl (the 3rd argument) you have a CLE
  cycle
- if NAND_ALE is set in ctrl you have an ALE cycle
- if NAND_CMD_NONE is passed in cmd (2nd argument), you should issue
  the whole operation

You can update your cavium command each time NAND_CLE or NAND_ALE is
passed (update the command information after each call), and then issue
the command when NAND_CMD_NONE is passed.
The only missing part in ->cmd_ctrl() are the data transfer cycles
which are handled in ->read/write_buf().

> 
> For instance for reading data I have ndf_page_read() that is used for
> both NAND_CMD_READ0 and NAND_CMD_READOOB. Without hacking into ->cmdfunc(),
> how would I differentiate between the two commands in read_buf()?

Do you have to? Can't you just issue a command that is solely doing
data transfer cycles without the CMD and ADDR ones?

[...]
> > > +union ndf_cmd {
> > > +	u64 val[2];
> > > +	union {
> > > +		struct ndf_nop_cmd		nop;
> > > +		struct ndf_wait_cmd		wait;
> > > +		struct ndf_bus_cmd		bus_acq_rel;
> > > +		struct ndf_chip_cmd		chip_en_dis;
> > > +		struct ndf_cle_cmd		cle_cmd;
> > > +		struct ndf_rd_cmd		rd_cmd;
> > > +		struct ndf_wr_cmd		wr_cmd;
> > > +		struct ndf_set_tm_par_cmd	set_tm_par;
> > > +		struct ndf_ale_cmd		ale_cmd;
> > > +		struct ndf_wait_status_cmd	wait_status;
> > > +	} u;
> > > +};  
> > 
> > I need some time to process all these information, but your controller
> > seems to be a complex/highly-configurable beast. That's really
> > interesting :-).
> > I'll come up with more comments/question after reviewing more carefully
> > the command creation logic.  
> 
> Great. I'm afraid out controller is quite different from existing
> hardware, at least I didn't find a driver that does things simalar (like
> the command building and queueing).

Hm, not so different actually, except you seem to have fine grained
control on the sequencing, which is a really good thing because your
driver can evolve with new NAND chip requirements.

> 
> I'm happy to help with any more information you need about our hardware.

Thanks,

Boris

[1]http://free-electrons.com/pub/conferences/2016/elc/brezillon-nand-framework/brezillon-nand-framework.pdf