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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Sat, 2 May 2020 21:18:43 +0200
Lubomir Rintel <lkundrak@xxxxx> wrote:

> On Sat, May 02, 2020 at 03:18:11PM +0200, Boris Brezillon wrote:
> > On Sat,  2 May 2020 13:14:10 +0200
> > Lubomir Rintel <lkundrak@xxxxx> 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@xxxxxxxxxxxxx>
> > > > ---
> > > >  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?  
> 
> That sounded plausible, but it doesn't seem to be to be the case. With
> the patch the operations doing DMA transfers still seem to time out (the
> identification succeeded, because at that point DMA is turned off):
> 
>    CAFÉ NAND 0000:00:0c.0: enabling device (0000 -> 0002)
>    nand: device found, Manufacturer ID: 0xad, Chip ID: 0xdc
>    nand: Hynix NAND 512MiB 3,3V 8-bit
>    nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>    nand: 2 chips detected
>    Bad block table not found for chip 0
>    Bad block table not found for chip 0
>    Scanning device for bad blocks
>    nand_bbt: error while erasing BBT block -5
>    nand_bbt: error -30 while marking block 8191 bad
>    nand_bbt: error while erasing BBT block -5
>    nand_bbt: error -30 while marking block 8190 bad
>    nand_bbt: error while erasing BBT block -5
>    nand_bbt: error -30 while marking block 8189 bad
>    nand_bbt: error while erasing BBT block -5
>    nand_bbt: error -30 while marking block 8188 bad
>    No space left to write bad block table
>    nand_bbt: error while writing bad block table -28
> 
> I've done this on top of your branch:
> 
> diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> index 761d103e438f..2a769033392e 100644
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -642,6 +642,11 @@ static int cafe_nand_exec_subop(struct nand_chip *chip,
>  
>         ret = readl_poll_timeout(cafe->mmio + CAFE_NAND_IRQ, status,
>                                  (status & wait) == wait, 1, USEC_PER_SEC);
> +       for (i = 0; i < subop->ninstrs; i++) {
> +               const struct nand_op_instr *instr = &subop->instrs[i];
> +               printk("%d: ret=%d instr=%d status=%08x wait=%08x\n", i, ret, instr->type, status, wait);
> +       }
> +
>         if (ret)
>                 return ret;
> 
> It indeed looks like CAFE_NAND_IRQ_CMD_DONE is never raised if there's a
> data operation involving DMA -- the status remains at 0x50000000. Full log:

I see. The reason I was not entirely happy with the "wait on DMA_DONE
when there's a DMA transfer" is because this transfer might not be the
last instruction in a sub operation, and I feared we would not wait for
the full operation to be done but only the DMA transfer itself. So I
went back to the spec [1], and there's an interesting note page 38:

"
Software waits for <dma_done> field in the Interrupt Register (Table 91
p. 93) for read operation because DMA is the last step of read
operation and waits for <cmd_done>field in the Interrupt Register
(Table 91 p. 93) for write operation because Command execution is the
last step of write operation.
"

I just pushed a new fixup commit implementing this logic. Let me know
if that solves the problem.

[1]http://wiki.laptop.org/images/5/5c/88ALP01_Datasheet_July_2007.pdf

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