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

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

[Boris Brezillon]

Let's convert the driver to exec_op() to have one less driver relying
on the legacy interface.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 .../mtd/nand/raw/ingenic/ingenic_nand_drv.c   | 136 ++++++++++--------
 1 file changed, 80 insertions(+), 56 deletions(-)

diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index e7bd845fdbf5..dcecd54af20b 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -27,9 +27,6 @@
 
 #define DRV_NAME	"ingenic-nand"
 
-/* Command delay when there is no R/B pin. */
-#define RB_DELAY_US	100
-
 struct jz_soc_info {
 	unsigned long data_offset;
 	unsigned long addr_offset;
@@ -49,7 +46,6 @@ struct ingenic_nfc {
 	struct nand_controller controller;
 	unsigned int num_banks;
 	struct list_head chips;
-	int selected;
 	struct ingenic_nand_cs cs[];
 };
 
@@ -142,51 +138,6 @@ static const struct mtd_ooblayout_ops jz4725b_ooblayout_ops = {
 	.free = jz4725b_ooblayout_free,
 };
 
-static void ingenic_nand_select_chip(struct nand_chip *chip, int chipnr)
-{
-	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
-	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
-	struct ingenic_nand_cs *cs;
-
-	/* Ensure the currently selected chip is deasserted. */
-	if (chipnr == -1 && nfc->selected >= 0) {
-		cs = &nfc->cs[nfc->selected];
-		jz4780_nemc_assert(nfc->dev, cs->bank, false);
-	}
-
-	nfc->selected = chipnr;
-}
-
-static void ingenic_nand_cmd_ctrl(struct nand_chip *chip, int cmd,
-				  unsigned int ctrl)
-{
-	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
-	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
-	struct ingenic_nand_cs *cs;
-
-	if (WARN_ON(nfc->selected < 0))
-		return;
-
-	cs = &nfc->cs[nfc->selected];
-
-	jz4780_nemc_assert(nfc->dev, cs->bank, ctrl & NAND_NCE);
-
-	if (cmd == NAND_CMD_NONE)
-		return;
-
-	if (ctrl & NAND_ALE)
-		writeb(cmd, cs->base + nfc->soc_info->addr_offset);
-	else if (ctrl & NAND_CLE)
-		writeb(cmd, cs->base + nfc->soc_info->cmd_offset);
-}
-
-static int ingenic_nand_dev_ready(struct nand_chip *chip)
-{
-	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
-
-	return !gpiod_get_value_cansleep(nand->busy_gpio);
-}
-
 static void ingenic_nand_ecc_hwctl(struct nand_chip *chip, int mode)
 {
 	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
@@ -298,8 +249,88 @@ static int ingenic_nand_attach_chip(struct nand_chip *chip)
 	return 0;
 }
 
+static int ingenic_nand_exec_instr(struct nand_chip *chip,
+				   struct ingenic_nand_cs *cs,
+				   const struct nand_op_instr *instr)
+{
+	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
+	struct ingenic_nfc *nfc = to_ingenic_nfc(chip->controller);
+	unsigned int i;
+
+	switch (instr->type) {
+	case NAND_OP_CMD_INSTR:
+		writeb(instr->ctx.cmd.opcode,
+		       cs->base + nfc->soc_info->cmd_offset);
+		return 0;
+	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);
+		return 0;
+	case NAND_OP_DATA_IN_INSTR:
+		if (instr->ctx.data.force_8bit ||
+		    !(chip->options & NAND_BUSWIDTH_16))
+			ioread8_rep(cs->base + nfc->soc_info->data_offset,
+				    instr->ctx.data.buf.in,
+				    instr->ctx.data.len);
+		else
+			ioread16_rep(cs->base + nfc->soc_info->data_offset,
+				     instr->ctx.data.buf.in,
+				     instr->ctx.data.len);
+		return 0;
+	case NAND_OP_DATA_OUT_INSTR:
+		if (instr->ctx.data.force_8bit ||
+		    !(chip->options & NAND_BUSWIDTH_16))
+			iowrite8_rep(cs->base + nfc->soc_info->data_offset,
+				     instr->ctx.data.buf.out,
+				     instr->ctx.data.len);
+		else
+			iowrite16_rep(cs->base + nfc->soc_info->data_offset,
+				      instr->ctx.data.buf.out,
+				      instr->ctx.data.len);
+		return 0;
+	case NAND_OP_WAITRDY_INSTR:
+		if (!nand->busy_gpio)
+			return nand_soft_waitrdy(chip,
+						 instr->ctx.waitrdy.timeout_ms);
+
+		return nand_gpio_waitrdy(chip, nand->busy_gpio,
+					 instr->ctx.waitrdy.timeout_ms);
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int ingenic_nand_exec_op(struct nand_chip *chip,
+				const struct nand_operation *op,
+				bool check_only)
+{
+	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
+	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
+	struct ingenic_nand_cs *cs;
+	unsigned int i;
+	int ret = 0;
+
+	if (check_only)
+		return 0;
+
+	cs = &nfc->cs[op->cs];
+	jz4780_nemc_assert(nfc->dev, cs->bank, true);
+	for (i = 0; i < op->ninstrs; i++) {
+		ret = ingenic_nand_exec_instr(chip, cs, &op->instrs[i]);
+		if (ret)
+			break;
+	}
+	jz4780_nemc_assert(nfc->dev, cs->bank, false);
+
+	return ret;
+}
+
 static const struct nand_controller_ops ingenic_nand_controller_ops = {
 	.attach_chip = ingenic_nand_attach_chip,
+	.exec_op = ingenic_nand_exec_op,
 };
 
 static int ingenic_nand_init_chip(struct platform_device *pdev,
@@ -339,8 +370,6 @@ static int ingenic_nand_init_chip(struct platform_device *pdev,
 		ret = PTR_ERR(nand->busy_gpio);
 		dev_err(dev, "failed to request busy GPIO: %d\n", ret);
 		return ret;
-	} else if (nand->busy_gpio) {
-		nand->chip.legacy.dev_ready = ingenic_nand_dev_ready;
 	}
 
 	nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
@@ -359,12 +388,7 @@ static int ingenic_nand_init_chip(struct platform_device *pdev,
 		return -ENOMEM;
 	mtd->dev.parent = dev;
 
-	chip->legacy.IO_ADDR_R = cs->base + nfc->soc_info->data_offset;
-	chip->legacy.IO_ADDR_W = cs->base + nfc->soc_info->data_offset;
-	chip->legacy.chip_delay = RB_DELAY_US;
 	chip->options = NAND_NO_SUBPAGE_WRITE;
-	chip->legacy.select_chip = ingenic_nand_select_chip;
-	chip->legacy.cmd_ctrl = ingenic_nand_cmd_ctrl;
 	chip->ecc.mode = NAND_ECC_HW;
 	chip->controller = &nfc->controller;
 	nand_set_flash_node(chip, np);
-- 
2.25.4


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

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

[Paul Cercueil]

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.

- Paul


> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  .../mtd/nand/raw/ingenic/ingenic_nand_drv.c   | 136 
> ++++++++++--------
>  1 file changed, 80 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c 
> b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> index e7bd845fdbf5..dcecd54af20b 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -27,9 +27,6 @@
> 
>  #define DRV_NAME	"ingenic-nand"
> 
> -/* Command delay when there is no R/B pin. */
> -#define RB_DELAY_US	100
> -
>  struct jz_soc_info {
>  	unsigned long data_offset;
>  	unsigned long addr_offset;
> @@ -49,7 +46,6 @@ struct ingenic_nfc {
>  	struct nand_controller controller;
>  	unsigned int num_banks;
>  	struct list_head chips;
> -	int selected;
>  	struct ingenic_nand_cs cs[];
>  };
> 
> @@ -142,51 +138,6 @@ static const struct mtd_ooblayout_ops 
> jz4725b_ooblayout_ops = {
>  	.free = jz4725b_ooblayout_free,
>  };
> 
> -static void ingenic_nand_select_chip(struct nand_chip *chip, int 
> chipnr)
> -{
> -	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> -	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
> -	struct ingenic_nand_cs *cs;
> -
> -	/* Ensure the currently selected chip is deasserted. */
> -	if (chipnr == -1 && nfc->selected >= 0) {
> -		cs = &nfc->cs[nfc->selected];
> -		jz4780_nemc_assert(nfc->dev, cs->bank, false);
> -	}
> -
> -	nfc->selected = chipnr;
> -}
> -
> -static void ingenic_nand_cmd_ctrl(struct nand_chip *chip, int cmd,
> -				  unsigned int ctrl)
> -{
> -	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> -	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
> -	struct ingenic_nand_cs *cs;
> -
> -	if (WARN_ON(nfc->selected < 0))
> -		return;
> -
> -	cs = &nfc->cs[nfc->selected];
> -
> -	jz4780_nemc_assert(nfc->dev, cs->bank, ctrl & NAND_NCE);
> -
> -	if (cmd == NAND_CMD_NONE)
> -		return;
> -
> -	if (ctrl & NAND_ALE)
> -		writeb(cmd, cs->base + nfc->soc_info->addr_offset);
> -	else if (ctrl & NAND_CLE)
> -		writeb(cmd, cs->base + nfc->soc_info->cmd_offset);
> -}
> -
> -static int ingenic_nand_dev_ready(struct nand_chip *chip)
> -{
> -	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> -
> -	return !gpiod_get_value_cansleep(nand->busy_gpio);
> -}
> -
>  static void ingenic_nand_ecc_hwctl(struct nand_chip *chip, int mode)
>  {
>  	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> @@ -298,8 +249,88 @@ static int ingenic_nand_attach_chip(struct 
> nand_chip *chip)
>  	return 0;
>  }
> 
> +static int ingenic_nand_exec_instr(struct nand_chip *chip,
> +				   struct ingenic_nand_cs *cs,
> +				   const struct nand_op_instr *instr)
> +{
> +	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> +	struct ingenic_nfc *nfc = to_ingenic_nfc(chip->controller);
> +	unsigned int i;
> +
> +	switch (instr->type) {
> +	case NAND_OP_CMD_INSTR:
> +		writeb(instr->ctx.cmd.opcode,
> +		       cs->base + nfc->soc_info->cmd_offset);
> +		return 0;
> +	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);
> +		return 0;
> +	case NAND_OP_DATA_IN_INSTR:
> +		if (instr->ctx.data.force_8bit ||
> +		    !(chip->options & NAND_BUSWIDTH_16))
> +			ioread8_rep(cs->base + nfc->soc_info->data_offset,
> +				    instr->ctx.data.buf.in,
> +				    instr->ctx.data.len);
> +		else
> +			ioread16_rep(cs->base + nfc->soc_info->data_offset,
> +				     instr->ctx.data.buf.in,
> +				     instr->ctx.data.len);
> +		return 0;
> +	case NAND_OP_DATA_OUT_INSTR:
> +		if (instr->ctx.data.force_8bit ||
> +		    !(chip->options & NAND_BUSWIDTH_16))
> +			iowrite8_rep(cs->base + nfc->soc_info->data_offset,
> +				     instr->ctx.data.buf.out,
> +				     instr->ctx.data.len);
> +		else
> +			iowrite16_rep(cs->base + nfc->soc_info->data_offset,
> +				      instr->ctx.data.buf.out,
> +				      instr->ctx.data.len);
> +		return 0;
> +	case NAND_OP_WAITRDY_INSTR:
> +		if (!nand->busy_gpio)
> +			return nand_soft_waitrdy(chip,
> +						 instr->ctx.waitrdy.timeout_ms);
> +
> +		return nand_gpio_waitrdy(chip, nand->busy_gpio,
> +					 instr->ctx.waitrdy.timeout_ms);
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ingenic_nand_exec_op(struct nand_chip *chip,
> +				const struct nand_operation *op,
> +				bool check_only)
> +{
> +	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
> +	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
> +	struct ingenic_nand_cs *cs;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	if (check_only)
> +		return 0;
> +
> +	cs = &nfc->cs[op->cs];
> +	jz4780_nemc_assert(nfc->dev, cs->bank, true);
> +	for (i = 0; i < op->ninstrs; i++) {
> +		ret = ingenic_nand_exec_instr(chip, cs, &op->instrs[i]);
> +		if (ret)
> +			break;
> +	}
> +	jz4780_nemc_assert(nfc->dev, cs->bank, false);
> +
> +	return ret;
> +}
> +
>  static const struct nand_controller_ops ingenic_nand_controller_ops 
> = {
>  	.attach_chip = ingenic_nand_attach_chip,
> +	.exec_op = ingenic_nand_exec_op,
>  };
> 
>  static int ingenic_nand_init_chip(struct platform_device *pdev,
> @@ -339,8 +370,6 @@ static int ingenic_nand_init_chip(struct 
> platform_device *pdev,
>  		ret = PTR_ERR(nand->busy_gpio);
>  		dev_err(dev, "failed to request busy GPIO: %d\n", ret);
>  		return ret;
> -	} else if (nand->busy_gpio) {
> -		nand->chip.legacy.dev_ready = ingenic_nand_dev_ready;
>  	}
> 
>  	nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
> @@ -359,12 +388,7 @@ static int ingenic_nand_init_chip(struct 
> platform_device *pdev,
>  		return -ENOMEM;
>  	mtd->dev.parent = dev;
> 
> -	chip->legacy.IO_ADDR_R = cs->base + nfc->soc_info->data_offset;
> -	chip->legacy.IO_ADDR_W = cs->base + nfc->soc_info->data_offset;
> -	chip->legacy.chip_delay = RB_DELAY_US;
>  	chip->options = NAND_NO_SUBPAGE_WRITE;
> -	chip->legacy.select_chip = ingenic_nand_select_chip;
> -	chip->legacy.cmd_ctrl = ingenic_nand_cmd_ctrl;
>  	chip->ecc.mode = NAND_ECC_HW;
>  	chip->controller = &nfc->controller;
>  	nand_set_flash_node(chip, np);
> --
> 2.25.4
> 



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

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

[Boris Brezillon]

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?

--->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..9206792629a8 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)
@@ -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/

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

[Boris Brezillon]

On Mon, 18 May 2020 21:24:22 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> 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?

I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
which explain why the test is inverted. Because of DT ABI stability it
might not be an option to change that, but the signal should actually be
declared GPIO_ACTIVE_HIGH.

> 
> --->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..9206792629a8 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)
> @@ -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/

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

[Paul Cercueil]

Le lun. 18 mai 2020 à 21:35, Boris Brezillon 
<boris.brezillon@collabora.com> a écrit :
> On Mon, 18 May 2020 21:24:22 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
>>  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?
> 
> I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
> which explain why the test is inverted. Because of DT ABI stability it
> might not be an option to change that, but the signal should actually 
> be
> declared GPIO_ACTIVE_HIGH.

It depends what you consider what is the active state, is it when 
"busy" or "ready"? ;)

I can fix it in the devicetree, and the driver would return 
(gpiod_get_value_cansleep(gpiod) ^ gpiod_is_active_low(gpiod)) for 
compatibility with the old devicetree.

Cheers,

-Paul

>> 
>>  --->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..9206792629a8 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)
>>  @@ -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/

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

[Boris Brezillon]

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

> Le lun. 18 mai 2020 à 21:35, Boris Brezillon 
> <boris.brezillon@collabora.com> a écrit :
> > On Mon, 18 May 2020 21:24:22 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> >>  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?  
> > 
> > I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
> > which explain why the test is inverted. Because of DT ABI stability it
> > might not be an option to change that, but the signal should actually 
> > be
> > declared GPIO_ACTIVE_HIGH.  
> 
> It depends what you consider what is the active state, is it when 
> "busy" or "ready"? ;)

True, this should really be documented in the generic binding part. As
you probably guessed from this discussion, all other drivers (and the
framework) is assuming "ready" is the state we're monitoring, so it's
effectively active high.

> 
> I can fix it in the devicetree, and the driver would return 
> (gpiod_get_value_cansleep(gpiod) ^ gpiod_is_active_low(gpiod)) for 
> compatibility with the old devicetree.

Or you could read the raw value (gpiod_get_raw_value_cansleep()),
but that still means you can't move away from the old semantics without
breaking the existing DT with the erroneous active-low. I mean,
active-low is still valid if someone has the R/B signal inverted,
but you can't discriminate when it's valid and when it's not.

I guess having a custom helper, and documenting that the active state
for ingenic is BUSY would be okay. Unless you'd be willing to break
the backward compat for the only board that has a rb-gpios property
defined.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L4166

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

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

[Paul Cercueil]

Le mar. 19 mai 2020 à 19:28, Boris Brezillon 
<boris.brezillon@collabora.com> a écrit :
> On Tue, 19 May 2020 17:04:37 +0200
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Le lun. 18 mai 2020 à 21:35, Boris Brezillon
>>  <boris.brezillon@collabora.com> a écrit :
>>  > On Mon, 18 May 2020 21:24:22 +0200
>>  > Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>  >
>>  >>  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?
>>  >
>>  > I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
>>  > which explain why the test is inverted. Because of DT ABI 
>> stability it
>>  > might not be an option to change that, but the signal should 
>> actually
>>  > be
>>  > declared GPIO_ACTIVE_HIGH.
>> 
>>  It depends what you consider what is the active state, is it when
>>  "busy" or "ready"? ;)
> 
> True, this should really be documented in the generic binding part. As
> you probably guessed from this discussion, all other drivers (and the
> framework) is assuming "ready" is the state we're monitoring, so it's
> effectively active high.
> 
>> 
>>  I can fix it in the devicetree, and the driver would return
>>  (gpiod_get_value_cansleep(gpiod) ^ gpiod_is_active_low(gpiod)) for
>>  compatibility with the old devicetree.
> 
> Or you could read the raw value (gpiod_get_raw_value_cansleep()),
> but that still means you can't move away from the old semantics 
> without
> breaking the existing DT with the erroneous active-low. I mean,
> active-low is still valid if someone has the R/B signal inverted,
> but you can't discriminate when it's valid and when it's not.
> 
> I guess having a custom helper, and documenting that the active state
> for ingenic is BUSY would be okay. Unless you'd be willing to break
> the backward compat for the only board that has a rb-gpios property
> defined.

What I suggest, in the probe:

if (of_machine_is_compatible("qi,lb60") && 
gpiod_is_active_low(nand->busy_gpio)) {
    gpiod_toggle_active_low(nand->busy_gpio);
}

Then it's backward-compatible, would allow me to fix the rb-gpios in 
devicetree, and wouldn't require a custom nand_gpio_waitrdy() function.

Cheers,
-Paul



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

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

[Boris Brezillon]

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

> Le mar. 19 mai 2020 à 19:28, Boris Brezillon 
> <boris.brezillon@collabora.com> a écrit :
> > On Tue, 19 May 2020 17:04:37 +0200
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >   
> >>  Le lun. 18 mai 2020 à 21:35, Boris Brezillon
> >>  <boris.brezillon@collabora.com> a écrit :  
> >>  > On Mon, 18 May 2020 21:24:22 +0200
> >>  > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>  >  
> >>  >>  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?  
> >>  >
> >>  > I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
> >>  > which explain why the test is inverted. Because of DT ABI   
> >> stability it  
> >>  > might not be an option to change that, but the signal should   
> >> actually  
> >>  > be
> >>  > declared GPIO_ACTIVE_HIGH.  
> >> 
> >>  It depends what you consider what is the active state, is it when
> >>  "busy" or "ready"? ;)  
> > 
> > True, this should really be documented in the generic binding part. As
> > you probably guessed from this discussion, all other drivers (and the
> > framework) is assuming "ready" is the state we're monitoring, so it's
> > effectively active high.
> >   
> >> 
> >>  I can fix it in the devicetree, and the driver would return
> >>  (gpiod_get_value_cansleep(gpiod) ^ gpiod_is_active_low(gpiod)) for
> >>  compatibility with the old devicetree.  
> > 
> > Or you could read the raw value (gpiod_get_raw_value_cansleep()),
> > but that still means you can't move away from the old semantics 
> > without
> > breaking the existing DT with the erroneous active-low. I mean,
> > active-low is still valid if someone has the R/B signal inverted,
> > but you can't discriminate when it's valid and when it's not.
> > 
> > I guess having a custom helper, and documenting that the active state
> > for ingenic is BUSY would be okay. Unless you'd be willing to break
> > the backward compat for the only board that has a rb-gpios property
> > defined.  
> 
> What I suggest, in the probe:
> 
> if (of_machine_is_compatible("qi,lb60") && 
> gpiod_is_active_low(nand->busy_gpio)) {
>     gpiod_toggle_active_low(nand->busy_gpio);
> }
> 

Oh, I didn't notice this gpiod_toggle_active_low() in the API.

> Then it's backward-compatible, would allow me to fix the rb-gpios in 
> devicetree, and wouldn't require a custom nand_gpio_waitrdy() function.

That's indeed the best option IMHO. I'll add a patch doing that in my
v2. Thanks for the suggestion.

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

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

[Paul Cercueil]

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.

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.

-Paul

> --->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..9206792629a8 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)
> @@ -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/

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

[Boris Brezillon]

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?

> 
> 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.

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

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

[Paul Cercueil]

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.

-Paul



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

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

[Boris Brezillon]

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/

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

[Paul Cercueil]

Hi Boris,

Le mar. 19 mai 2020 à 19:35, Boris Brezillon 
<boris.brezillon@collabora.com> a écrit :
> 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).

Oops, I'm sorry I didn't even spot that.

With the diff below, it works, so:
Tested-by: Paul Cercueil <paul@crapouillou.net>

Thank you for the patch.

Cheers,
-Paul

> 
> --->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/