[RFC] mtd: rawnand: ams-delta: use ->exec_op()

Message ID 20181003120028.9257-1-jmkrzyszt@gmail.com
State Superseded
Delegated to: Miquel Raynal
Headers show
Series
  • [RFC] mtd: rawnand: ams-delta: use ->exec_op()
Related show

Commit Message

Janusz Krzysztofik Oct. 3, 2018, noon
Replace legacy callbacks with ->select_chip() and ->exec_op().

Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
nand_wait_ready(), otherwise that function would probabaly have to be
reimplemented inside the driver.  Hence, legacy callback ->dev_ready()
is still used.

Use of IO_ADDR_R and IO_ADDR_W legacy structure members will be dropped
later, as soon as the driver is converted to use GPIO API for data I/O.

Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Hi,

I've not tested the change on hardware yet as I'm not sure if:
- handling of NCE limited to that inside ->select_chip() is
  sufficient,
- releasing ALE / CLE immediately after ams_delta_write_buf() is
  correct.
Please advise before I break my test hardware :-).

Thanks,
Janusz

 drivers/mtd/nand/raw/ams-delta.c | 83 +++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 31 deletions(-)

Comments

Boris Brezillon Oct. 3, 2018, 12:30 p.m. | #1
Hi Janusz,

On Wed,  3 Oct 2018 14:00:28 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Replace legacy callbacks with ->select_chip() and ->exec_op().

Thanks for working on that, that's really appreciated.

> 
> Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> nand_wait_ready(),

I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
are doing, but is shouldn't be too hard to replace them by an
ams_delta_wait_ready() func.

> otherwise that function would probabaly have to be

				^ probably


> reimplemented inside the driver.  Hence, legacy callback ->dev_ready()
> is still used.
> 
> Use of IO_ADDR_R and IO_ADDR_W legacy structure members will be dropped
> later, as soon as the driver is converted to use GPIO API for data I/O.

In the meantime, can you move the iomem pointer to the ams_delta
private struct so that this driver no longer uses the ->IO_ADDR_R/W
fields?

> 
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
> Hi,
> 
> I've not tested the change on hardware yet as I'm not sure if:
> - handling of NCE limited to that inside ->select_chip() is
>   sufficient,

I think it is.

> - releasing ALE / CLE immediately after ams_delta_write_buf() is
>   correct.

Well, you should probably consider waiting for instr->ctx.delay_ns
nanoseconds after each instruction, but, if it was working before the
conversion to ->exec_op(), it should work just fine now.

Regards,

Boris
Janusz Krzysztofik Oct. 3, 2018, 1:55 p.m. | #2
On Wednesday, October 3, 2018 2:30:54 PM CEST Boris Brezillon wrote:
> Hi Janusz,
> 
> On Wed,  3 Oct 2018 14:00:28 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Replace legacy callbacks with ->select_chip() and ->exec_op().
> 
> Thanks for working on that, that's really appreciated.
> 
> > 
> > Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> > nand_wait_ready(),
> 
> I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
> are doing, but is shouldn't be too hard to replace them by an
> ams_delta_wait_ready() func.

Default nand_wait() is used as ->waitfunc(), and ->dev_ready() returns R/B 
GPIO pin status.

> > otherwise that function would probabaly have to be
> 
> 				^ probably

Do you think other drivers which now provide ->dev_ready() won't require 
reimplementation of nand_wait_ready()?

> > reimplemented inside the driver.  Hence, legacy callback ->dev_ready()
> > is still used.
> > 
> > Use of IO_ADDR_R and IO_ADDR_W legacy structure members will be dropped
> > later, as soon as the driver is converted to use GPIO API for data I/O.
> 
> In the meantime, can you move the iomem pointer to the ams_delta
> private struct so that this driver no longer uses the ->IO_ADDR_R/W
> fields?

OK

> > 
> > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > ---
> > Hi,
> > 
> > I've not tested the change on hardware yet as I'm not sure if:
> > - handling of NCE limited to that inside ->select_chip() is
> >   sufficient,
> 
> I think it is.
> 
> > - releasing ALE / CLE immediately after ams_delta_write_buf() is
> >   correct.
> 
> Well, you should probably consider waiting for instr->ctx.delay_ns
> nanoseconds after each instruction, but, if it was working before the
> conversion to ->exec_op(), it should work just fine now.

OK, I'll give it a try.

Thanks,
Janusz
Boris Brezillon Oct. 3, 2018, 2:06 p.m. | #3
On Wed, 03 Oct 2018 15:55:25 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
  
> > > 
> > > Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> > > nand_wait_ready(),  
> > 
> > I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
> > are doing, but is shouldn't be too hard to replace them by an
> > ams_delta_wait_ready() func.  
> 
> Default nand_wait() is used as ->waitfunc(), and ->dev_ready() returns R/B 
> GPIO pin status.

Okay. Then it might make sense to provide a generic helper to poll a
gpio.

void nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod)
{
	...
}

> 
> > > otherwise that function would probabaly have to be  
> > 
> > 				^ probably  
> 
> Do you think other drivers which now provide ->dev_ready() won't require 
> reimplementation of nand_wait_ready()?

It depends. I mean, most controllers support native R/B sensing, and in
case they do actually require you to poll the RB pin status, duplicating
the wait_ready() logic shouldn't be a problem. On the other hand, I
really want to get rid of ->dev_ready().
Janusz Krzysztofik Oct. 4, 2018, 1:52 p.m. | #4
Hi Boris,

On Wednesday, October 3, 2018 4:06:34 PM CEST Boris Brezillon wrote:
> On Wed, 03 Oct 2018 15:55:25 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>   
> > > > 
> > > > Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> > > > nand_wait_ready(),  
> > > 
> > > I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
> > > are doing, but is shouldn't be too hard to replace them by an
> > > ams_delta_wait_ready() func.  
> > 
> > Default nand_wait() is used as ->waitfunc(), and ->dev_ready() returns R/B 
> > GPIO pin status.
> 
> Okay. Then it might make sense to provide a generic helper to poll a
> gpio.
> 
> void nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod)
> {
> 	...
> }

How about a still more generic helper which accepts dev_ready() callback as an 
argument?

Thanks,
Janusz
Boris Brezillon Oct. 4, 2018, 1:59 p.m. | #5
On Thu, 04 Oct 2018 15:52:57 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Hi Boris,
> 
> On Wednesday, October 3, 2018 4:06:34 PM CEST Boris Brezillon wrote:
> > On Wed, 03 Oct 2018 15:55:25 +0200
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >     
> > > > > 
> > > > > Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> > > > > nand_wait_ready(),    
> > > > 
> > > > I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
> > > > are doing, but is shouldn't be too hard to replace them by an
> > > > ams_delta_wait_ready() func.    
> > > 
> > > Default nand_wait() is used as ->waitfunc(), and ->dev_ready() returns R/B 
> > > GPIO pin status.  
> > 
> > Okay. Then it might make sense to provide a generic helper to poll a
> > gpio.
> > 
> > void nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod)
> > {
> > 	...
> > }  
> 
> How about a still more generic helper which accepts dev_ready() callback as an 
> argument?

Nope, I still prefer the GPIO based one. We'll see if others need a
a more generic helper, but I doubt it.
Janusz Krzysztofik Oct. 4, 2018, 2:11 p.m. | #6
On Thursday, October 4, 2018 3:59:33 PM CEST Boris Brezillon wrote:
> On Thu, 04 Oct 2018 15:52:57 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Hi Boris,
> > 
> > On Wednesday, October 3, 2018 4:06:34 PM CEST Boris Brezillon wrote:
> > > On Wed, 03 Oct 2018 15:55:25 +0200
> > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > >     
> > > > > > 
> > > > > > Implementation of NAND_OP_WAITRDY_INSTR has been based on legacy
> > > > > > nand_wait_ready(),    
> > > > > 
> > > > > I don't remember what the ams-delta ->dev_ready()/->waitfunc() hooks
> > > > > are doing, but is shouldn't be too hard to replace them by an
> > > > > ams_delta_wait_ready() func.    
> > > > 
> > > > Default nand_wait() is used as ->waitfunc(), and ->dev_ready() returns 
R/B 
> > > > GPIO pin status.  
> > > 
> > > Okay. Then it might make sense to provide a generic helper to poll a
> > > gpio.
> > > 
> > > void nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod)
> > > {
> > > 	...
> > > }  
> > 
> > How about a still more generic helper which accepts dev_ready() callback 
as an 
> > argument?
> 
> Nope, I still prefer the GPIO based one. We'll see if others need a
> a more generic helper, but I doubt it.

OK.

Legacy nand_wait_ready() uses a hardcoded timeout value of 400 ms.   Should we 
follow the same approach in nand_gpio_waitrdy(), or should we rather let 
drivers pass the timeout value, like in case of nand_soft_waitrdy()?

Thanks,
Janusz
Boris Brezillon Oct. 4, 2018, 2:22 p.m. | #7
On Thu, 04 Oct 2018 16:11:42 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> 
> Legacy nand_wait_ready() uses a hardcoded timeout value of 400 ms.   Should we 
> follow the same approach in nand_gpio_waitrdy(), or should we rather let 
> drivers pass the timeout value, like in case of nand_soft_waitrdy()?

The latter.

Patch

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 5ba180a291eb..90c283a2c1b7 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -124,46 +124,69 @@  static void ams_delta_read_buf(struct nand_chip *this, u_char *buf, int len)
 		buf[i] = ams_delta_io_read(priv);
 }
 
-static u_char ams_delta_read_byte(struct nand_chip *this)
+static int ams_delta_nand_ready(struct nand_chip *this)
 {
-	u_char res;
-
-	ams_delta_read_buf(this, &res, 1);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
 
-	return res;
+	return gpiod_get_value(priv->gpiod_rdy);
 }
 
-/*
- * Command control function
- *
- * ctrl:
- * NAND_NCE: bit 0 -> bit 2
- * NAND_CLE: bit 1 -> bit 7
- * NAND_ALE: bit 2 -> bit 6
- */
-static void ams_delta_hwcontrol(struct nand_chip *this, int cmd,
-				unsigned int ctrl)
+static void ams_delta_select_chip(struct nand_chip *this, int n)
 {
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
 
-	if (ctrl & NAND_CTRL_CHANGE) {
-		gpiod_set_value(priv->gpiod_nce, !(ctrl & NAND_NCE));
-		gpiod_set_value(priv->gpiod_cle, !!(ctrl & NAND_CLE));
-		gpiod_set_value(priv->gpiod_ale, !!(ctrl & NAND_ALE));
-	}
-
-	if (cmd != NAND_CMD_NONE) {
-		u_char byte = cmd;
+	if (n > 0)
+		return;
 
-		ams_delta_write_buf(this, &byte, 1);
-	}
+	gpiod_set_value(priv->gpiod_nce, n < 0);
 }
 
-static int ams_delta_nand_ready(struct nand_chip *this)
+static int ams_delta_exec_op(struct nand_chip *this,
+			     const struct nand_operation *op, bool check_only)
 {
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	const struct nand_op_instr *instr;
+	int i;
 
-	return gpiod_get_value(priv->gpiod_rdy);
+	for (i = 0; i < op->ninstrs; i++) {
+		instr = &op->instrs[i];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			gpiod_set_value(priv->gpiod_cle, 1);
+			ams_delta_write_buf(this, &instr->ctx.cmd.opcode, 1);
+			gpiod_set_value(priv->gpiod_cle, 0);
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			gpiod_set_value(priv->gpiod_ale, 1);
+			ams_delta_write_buf(this, instr->ctx.addr.addrs,
+					    instr->ctx.addr.naddrs);
+			gpiod_set_value(priv->gpiod_ale, 0);
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			ams_delta_read_buf(this, instr->ctx.data.buf.in,
+					   instr->ctx.data.len);
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			ams_delta_write_buf(this, instr->ctx.data.buf.out,
+					    instr->ctx.data.len);
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			if (this->legacy.dev_ready) {
+				nand_wait_ready(this);
+				break;
+			}
+
+			return nand_soft_waitrdy(this,
+						 instr->ctx.waitrdy.timeout_ms);
+		}
+	}
+
+	return 0;
 }
 
 
@@ -213,10 +236,8 @@  static int ams_delta_init(struct platform_device *pdev)
 	/* Set address of NAND IO lines */
 	this->legacy.IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
 	this->legacy.IO_ADDR_W = io_base + OMAP_MPUIO_OUTPUT;
-	this->legacy.read_byte = ams_delta_read_byte;
-	this->legacy.write_buf = ams_delta_write_buf;
-	this->legacy.read_buf = ams_delta_read_buf;
-	this->legacy.cmd_ctrl = ams_delta_hwcontrol;
+	this->select_chip = ams_delta_select_chip;
+	this->exec_op = ams_delta_exec_op;
 
 	priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
 	if (IS_ERR(priv->gpiod_rdy)) {