Re: [PATCH] mtd: rawnand: ingenic: Convert the driver to exec_op()

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

On Tue, 19 May 2020 17:10:12 +0200
Paul Cercueil <paul@crapouillou.net> wrote:

> Le mar. 19 mai 2020 à 17:01, Boris Brezillon 
> <boris.brezillon@collabora.com> a écrit :
> > On Tue, 19 May 2020 16:52:27 +0200
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >   
> >>  Hi Boris,
> >> 
> >>  Le lun. 18 mai 2020 à 21:24, Boris Brezillon
> >>  <boris.brezillon@collabora.com> a écrit :  
> >>  > On Mon, 18 May 2020 19:50:04 +0200
> >>  > Paul Cercueil <paul@crapouillou.net> wrote:
> >>  >  
> >>  >>  Hi Boris,
> >>  >>
> >>  >>  Le lun. 18 mai 2020 à 18:56, Boris Brezillon
> >>  >>  <boris.brezillon@collabora.com> a écrit :  
> >>  >>  > Let's convert the driver to exec_op() to have one less driver  
> >>  >> relying  
> >>  >>  > on the legacy interface.  
> >>  >>
> >>  >>  Great work, thanks for that.
> >>  >>
> >>  >>  However it does not work :( nand_scan() returns error -145.  
> >>  >
> >>  > Looks like the R/B signal is inverted. Can you try with the
> >>  > following diff applied?  
> >> 
> >>  Still doesn't work properly. I get -ENODEV in nand_detect(), at the
> >>  "second ID read did not match..." pr_info().
> >> 
> >>  The R/B signal doesn't seem to be the primary cause, if I use
> >>  nand_soft_waitrdy() it doesn't work any better.  
> > 
> > Well, it does solve the ETIMEDOUT issue, so we're one step further 
> > ;-).
> > Can you print the returned ID?  
> 
> It reads 00/00, so it doesn't seem to be able to read any data.
> 
> >> 
> >>  One thing I noticed that jz4780_nemc_assert() is called with
> >>  assert=true unconditionally, while before it was called with (ctrl &
> >>  NAND_NCE), whatever that is. Whether or not that's a problem, I 
> >> have no
> >>  idea.  
> > 
> > Yes, we really want to assert the CE signal unconditionally here, but
> > maybe we should add a delay after asserting it/before de-asserting it.  
> 
> I added some udelay() here and there, unfortunately it didn't change 
> anything.

Can you try with this diff? It's basically the same as before except
there's an additional fix (s/cmd_offset/addr_offset/ in the address
emission path).

--->8---
diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index dcecd54af20b..2668135f2d26 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -249,6 +249,26 @@ static int ingenic_nand_attach_chip(struct nand_chip *chip)
        return 0;
 }
 
+static int ingenic_nand_gpio_waitrdy(struct gpio_desc *gpiod,
+                                    unsigned long timeout_ms)
+{
+       /*
+        * Wait until R/B pin indicates chip is ready or timeout occurs.
+        * +1 below is necessary because if we are now in the last fraction
+        * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+        * small jiffy fraction - possibly leading to false timeout.
+       */
+       timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
+       do {
+               if (!gpiod_get_value_cansleep(gpiod))
+                       return 0;
+
+               cond_resched();
+       } while (time_before(jiffies, timeout_ms));
+
+       return !gpiod_get_value_cansleep(gpiod) ? 0 : -ETIMEDOUT;
+};
+
 static int ingenic_nand_exec_instr(struct nand_chip *chip,
                                   struct ingenic_nand_cs *cs,
                                   const struct nand_op_instr *instr)
@@ -265,7 +285,7 @@ static int ingenic_nand_exec_instr(struct nand_chip *chip,
        case NAND_OP_ADDR_INSTR:
                for (i = 0; i < instr->ctx.addr.naddrs; i++)
                        writeb(instr->ctx.addr.addrs[i],
-                              cs->base + nfc->soc_info->cmd_offset);
+                              cs->base + nfc->soc_info->addr_offset);
                return 0;
        case NAND_OP_DATA_IN_INSTR:
                if (instr->ctx.data.force_8bit ||
@@ -294,8 +314,8 @@ static int ingenic_nand_exec_instr(struct nand_chip *chip,
                        return nand_soft_waitrdy(chip,
                                                 instr->ctx.waitrdy.timeout_ms);
 
-               return nand_gpio_waitrdy(chip, nand->busy_gpio,
-                                        instr->ctx.waitrdy.timeout_ms);
+               return ingenic_nand_gpio_waitrdy(nand->busy_gpio,
+                                                instr->ctx.waitrdy.timeout_ms);
        default:
                break;
        }
@@ -322,6 +342,9 @@ static int ingenic_nand_exec_op(struct nand_chip *chip,
                ret = ingenic_nand_exec_instr(chip, cs, &op->instrs[i]);
                if (ret)
                        break;
+
+               if (op->instrs[i].delay_ns)
+                       ndelay(op->instrs[i].delay_ns);
        }
        jz4780_nemc_assert(nfc->dev, cs->bank, false);
 

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