Re: [PATCH 13/17] mtd: rawnand: cafe: Add exec_op() support

[Boris Brezillon <boris.brezillon@collabora.com>]

On Sat,  2 May 2020 13:14:10 +0200
Lubomir Rintel <lkundrak@v3.sk> wrote:

> Boris Brezillon wrote:
> > Implementing exec_op() will help us get rid of the legacy interface and
> > should make drivers much cleaner too.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/mtd/nand/raw/cafe_nand.c | 137 ++++++++++++++++++++++++++++++-
> >  1 file changed, 136 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> > index edf65197604b..ada9c8b06a41 100644
> > --- a/drivers/mtd/nand/raw/cafe_nand.c
> > +++ b/drivers/mtd/nand/raw/cafe_nand.c  
> ...
> 
> > +	ret = readl_poll_timeout(cafe->mmio + CAFE_NAND_IRQ, status,
> > +				 (status & wait) == wait, 1, USEC_PER_SEC);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (ctrl1 & CAFE_NAND_DMA_CTRL_DATA_IN)  
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> s/CAFE_NAND_DMA_CTRL_DATA_IN/CAFE_NAND_CTRL1_HAS_DATA_IN/ here please.
> 
> 
> > +		cafe_read_buf(chip, data_instr->ctx.data.buf.in,
> > +			      data_instr->ctx.data.len);
> > +
> > +	return 0;
> > +}  
> ...
> 
> Other than that, when DMA is in use, only CAFE_NAND_IRQ_DMA_DONE seem to pop
> up in CAFE_NAND_IRQ when the command completes, not CAFE_NAND_IRQ_CMD_DONE.
> I suppose you ought to do this or something equivalent:

I suspect it has to do with the fact that you might have operations with
DATA_IN() instructions only. I pushed an alternate fix [1] to my branch.
Would you mind testing it?

> 
> diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> index 307c9a68afa9..356a07e61c88 100644
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -778,7 +778,7 @@ static int cafe_nand_exec_subop(struct nand_chip *chip,
>  {
>         struct cafe_priv *cafe = nand_get_controller_data(chip);
>         u32 ctrl1 = 0, ctrl2 = cafe->ctl2, addr1 = 0, addr2 = 0;
> -       u32 status, wait = CAFE_NAND_IRQ_CMD_DONE;
> +       u32 status, wait = 0;
>         int ret, data_instr = -1;
>         bool waitrdy = false;
>         unsigned int i, j;
> @@ -856,6 +856,8 @@ static int cafe_nand_exec_subop(struct nand_chip *chip,
>                         dmactrl |= CAFE_NAND_DMA_CTRL_DATA_IN;
>  
>                 cafe_writel(cafe, dmactrl, NAND_DMA_CTRL);
> +       } else {
> +               wait |= CAFE_NAND_IRQ_CMD_DONE;
>         }
>  
>         /* Clear the pending interrupts before starting the operation. */
> 
> cafe_nand_cmdfunc() seems to do the same thing (note the "=" instead of
> "|=") though the use of word "just" in the comment is somewhat misleading:
> 
>  244 static void cafe_nand_cmdfunc(struct nand_chip *chip, unsigned command,
>  245                               int column, int page_addr)
>  246 {
>  ...
>  359                 /* If WR or RD bits set, set up DMA */
>  360                 if (ctl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) {
>  361                         /* It's a read */
>  362                         dmactl |= CAFE_NAND_DMA_CTRL_DATA_IN;
>  363                         /* ... so it's done when the DMA is done, not just
>  364                            the command. */
>  365                         doneint = CAFE_NAND_IRQ_DMA_DONE;
>  366                 }
> 
> With the changes I suggested above, you can add:
> 
> Reviewed-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> Thank you!
> Lubo

[1]https://github.com/bbrezillon/linux/commit/ecf93c3c2e94ab0710babe856f272ff2e8b2a35b

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