Re: [PATCH v4 7/8] mtd: rawnand: arasan: Add new Arasan NAND controller

[Miquel Raynal <miquel.raynal@bootlin.com>]

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 11 May 2020
17:46:14 +0200:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 11 May
> 2020 17:32:35 +0200:
> 
> > On Mon, 11 May 2020 17:07:29 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 10 May
> > > 2020 09:03:14 +0200:
> > >     
> > > > On Fri,  8 May 2020 19:13:38 +0200
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >       
> > > > > +static int anfc_exec_op(struct nand_chip *chip,
> > > > > +			const struct nand_operation *op,
> > > > > +			bool check_only)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	if (check_only)
> > > > > +		return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> > > > > +					      check_only);        
> > > > 
> > > > You should also check the DATA_IN/OUT size here ^.      
> > > 
> > > Here is my proposal:
> > > 
> > > ---8<---
> > > 
> > > +static int anfc_check_op(struct nand_chip *chip,
> > > +                        const struct nand_operation *op)
> > > +{
> > > +       int op_id;
> > > +
> > > +       /*
> > > +        * The controller abstracts all the NAND operations and do not support
> > > +        * data only operations.    
> > 
> > 	* FIXME: The nand_op_parser framework should be extended to
> > 	* support custom checks on DATA instructions.  
> 
> Oh you really want to extend the core for that? I thought having a
> "check_op" helper like this was enough, as it gives enough freedom to
> the controller driver to return all the corner cases that are not very
> generic. See below for more details.
> 
> >   
> > > +        */    
> > 
> > You also didn't mention the fact that the number of data cycles should
> > be aligned on 4 bytes, and that the controller might read/write more
> > than requested when that's not the case. But maybe you have that
> > comment elsewhere in the code (where you do the round_up(4)?).  
> 
> Precisely, for me the previous check is not a problem from the core
> perspective (hence not deserving a FIXME) because the driver do not lie
> at any moment. Conversely, the driver limitations of what is supported
> and what is not is clear and accurate.
> 
> However for this round_up() operation you are talking about, this is an
> issue as we have currently no mean to say to the core that something
> different than ordered was actually requested by the driver, so there
> is lying involved and this deserves a FIXME.

Actually adding an exec_op parameter saying "this is the absolute
maximum that I can do" is not that invasive and would apply to many
drivers too.

Let's add these three FIXMEs for now.

> > 
> > 	/*
> > 	 * Number of DATA cycles must be aligned on 4, that means the
> > 	 * controller might read/write more than requested This is
> > 	 * harmless most of the time as extra DATA are discarded in
> > 	 * the write path and read pointer adjusted in the read path.
> > 	 * FIXME: The core should mark operations where reading/writing
> > 	 * more is allowed so the exec_op() implementation can take
> > 	 * the right decision when the alignment constraint is not met:
> > 	 * adjust the number of DATA cycles when it's allowed, and
> > 	 * reject the operation otherwise.
> > 	 */  
> 
> I want to put this comment where the round_up takes place.
> 
> >   
> > > +       for (op_id = 0; op_id < op->ninstrs; op_id++) {
> > > +               instr = &op->instrs[op_id];
> > > +
> > > +               switch (instr->type) {
> > > +               case NAND_OP_ADDR_INSTR:
> > > +                       if (instr->ctx.addr.naddrs > ANFC_MAX_ADDR_CYC)
> > > +                               return -ENOTSUPP;
> > > +                       break;
> > > +               case NAND_OP_DATA_IN_INSTR:
> > > +               case NAND_OP_DATA_OUT_INSTR:
> > > +                       if (instr->ctx.data.len > ANFC_MAX_CHUNK_SIZE)
> > > +                               return -ENOTSUPP;
> > > +                       break;
> > > +               default:
> > > +               }
> > > +       }
> > > +
> > > +       /*
> > > +        * The controller does not allow to proceed with a CMD+DATA_IN cycle
> > > +        * manually on the bus by reading data from the data register. Instead,
> > > +        * the controller abstract a status read operation with its own status
> > > +        * register after ordering a read status operation. Hence, we cannot
> > > +        * support any CMD+DATA_IN operation other than a READ STATUS.    
> > 
> > 	* FIXME: The nand_op_parser() framework should be extended to
> > 	* describe fixed patterns instead of open-coding this check
> > 	* here.  
> 
> For this one, I am not against a FIXME as this is something that might
> be useful for other drivers too.
> 
> >   
> > > +        */
> > > +       if (op->ninstrs == 2 &&
> > > +           op->instrs[0].type == NAND_OP_CMD_INSTR &&
> > > +           op->instrs[0].ctx.cmd.opcode != NAND_CMD_STATUS &&
> > > +           op->instrs[1].type == NAND_OP_DATA_IN_INSTR)
> > > +               return -ENOTSUPP;
> > > +
> > > +       return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> > > +                                     check_only);
> > > +}
> > > +
> > >  static int anfc_exec_op(struct nand_chip *chip,
> > >                         const struct nand_operation *op,
> > >                         bool check_only)
> > > @@ -774,8 +813,7 @@ static int anfc_exec_op(struct nand_chip *chip,
> > >         int ret;
> > >  
> > >         if (check_only)
> > > -               return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> > > -                                             check_only);
> > > +               return anfc_check_op(chip, op);
> > >  
> > >         ret = anfc_select_target(chip, op->cs);
> > >         if (ret)
> > >     
> > > --->8---      
> > > 
> > > What do you think?    
> >   
> 
>