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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Boris,

Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Mon, 11 May
2020 17:32:35 +0200:

> On Mon, 11 May 2020 17:07:29 +0200
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Sun, 10 May
> > 2020 09:03:14 +0200:
> >   
> > > On Fri,  8 May 2020 19:13:38 +0200
> > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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.
> 
> 	/*
> 	 * 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?  
> 



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