diff mbox

[RFC,8/8] mtd: rawnand: ams-delta: Use GPIO callbacks for data I/O

Message ID 20180718235710.18242-9-jmkrzyszt@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Janusz Krzysztofik July 18, 2018, 11:57 p.m. UTC 
Don't readw()/writew() data directly from/to GPIO port which is under
control of gpio-omap driver, use GPIO chip callbacks instead.

Thanks to utilization of get/set_multiple() callbacks, performance
degrade is minor for typical data transfers.

The driver should now work with any 8+-bit bidirectional GPIO port,
not only OMAP.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

# Conflicts:
#	drivers/mtd/nand/raw/ams-delta.c
---
 arch/arm/mach-omap1/board-ams-delta.c |  11 ----
 drivers/mtd/nand/raw/ams-delta.c      | 119 +++++++++++++++-------------------
 2 files changed, 52 insertions(+), 78 deletions(-)

Comments

Boris Brezillon July 19, 2018, 6:47 a.m. UTC | #1 
On Thu, 19 Jul 2018 01:57:10 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Don't readw()/writew() data directly from/to GPIO port which is under
> control of gpio-omap driver, use GPIO chip callbacks instead.
> 
> Thanks to utilization of get/set_multiple() callbacks, performance
> degrade is minor for typical data transfers.

Same comment here, don't use the gpio_chip hooks directly, use the
consumer API instead.

> 
> The driver should now work with any 8+-bit bidirectional GPIO port,
> not only OMAP.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> 
> # Conflicts:
> #	drivers/mtd/nand/raw/ams-delta.c

Oh, some leftovers from a conflict resolution :).
Janusz Krzysztofik July 20, 2018, 6:38 p.m. UTC | #2 
On Thursday, July 19, 2018 8:47:49 AM CEST Boris Brezillon wrote:
> On Thu, 19 Jul 2018 01:57:10 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Don't readw()/writew() data directly from/to GPIO port which is under
> > control of gpio-omap driver, use GPIO chip callbacks instead.
> > 
> > Thanks to utilization of get/set_multiple() callbacks, performance
> > degrade is minor for typical data transfers.
> 
> Same comment here, don't use the gpio_chip hooks directly, use the
> consumer API instead.

I tired but performance was not acceptable.

I see your point but please understand, what I'm trying to do here is not to 
develop a shiny general purpose fully GPIO based NAND driver, I'm trying to 
resolve issues with NAND driver for Amstrad Delta I like to play with, without 
loosing much performance.

I'm going to reconsider all possible options, not only doing data I/O over 
GPIO inside the driver, to have the task completed.  Once done,  I can get 
back to the GPIO based code I developed so far and create a new generic driver 
as my free time permits, or anyone can do that if needed, the code is open 
source after all. 

Thanks,
Janusz
Boris Brezillon July 20, 2018, 7:48 p.m. UTC | #3 
Janusz,

On Fri, 20 Jul 2018 20:38:15 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Thursday, July 19, 2018 8:47:49 AM CEST Boris Brezillon wrote:
> > On Thu, 19 Jul 2018 01:57:10 +0200
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >   
> > > Don't readw()/writew() data directly from/to GPIO port which is under
> > > control of gpio-omap driver, use GPIO chip callbacks instead.
> > > 
> > > Thanks to utilization of get/set_multiple() callbacks, performance
> > > degrade is minor for typical data transfers.  
> > 
> > Same comment here, don't use the gpio_chip hooks directly, use the
> > consumer API instead.  
> 
> I tired but performance was not acceptable.

You tried to use gpiod_{get,set}_array_value(), right? Did you
investigate on where the overhead comes from?

> 
> I see your point but please understand, what I'm trying to do here is not to 
> develop a shiny general purpose fully GPIO based NAND driver, I'm trying to 
> resolve issues with NAND driver for Amstrad Delta I like to play with, without 
> loosing much performance.

That's not a reason to violate the consumer/driver separation provided
by the GPIO framework. I'm not saying the current consumer APIs are
good enough for what you want to do (bit-bang a parallel data bus in
an efficient way), but bypassing the GPIO core like you do is
definitely not a good thing. Maybe you should discuss with Linus the
possibility of introducing a gpio_bitbang API that would provide you
some guarantees on the access time by making sure the pins all belong
to the same bank (and can thus be accessed in an atomic way). And maybe
provide a way to read/write several bytes by defining a delay between
each access, the size of the bus and the control pin if any (in
our case NRE/NWE). 

> 
> I'm going to reconsider all possible options, not only doing data I/O over 
> GPIO inside the driver, to have the task completed.  Once done,  I can get 
> back to the GPIO based code I developed so far and create a new generic driver 
> as my free time permits, or anyone can do that if needed, the code is open 
> source after all.

Let's forget the generic nand-gpio driver for now. All I'm asking is
that you do not bypass the GPIO framework like you intend do in this
patch.

Regards,

Boris
diff mbox

Patch

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 16f7bbe47607..08e732bc1cd2 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -321,20 +321,9 @@  struct modem_private_data {
 
 static struct modem_private_data modem_priv;
 
-static struct resource ams_delta_nand_resources[] = {
-	[0] = {
-		.start	= OMAP1_MPUIO_BASE,
-		.end	= OMAP1_MPUIO_BASE +
-				OMAP_MPUIO_IO_CNTL + sizeof(u32) - 1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
 static struct platform_device ams_delta_nand_device = {
 	.name	= "ams-delta-nand",
 	.id	= -1,
-	.num_resources	= ARRAY_SIZE(ams_delta_nand_resources),
-	.resource	= ams_delta_nand_resources,
 };
 
 #define OMAP_GPIO_LABEL		"gpio-0-15"
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index bd501f385e78..8fac8d2a444a 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -25,13 +25,11 @@ 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
-#include <linux/platform_data/gpio-omap.h>
+#include <linux/platform_device.h>
 
 #include <asm/io.h>
 #include <asm/sizes.h>
 
-#include <mach/hardware.h>
-
 /*
  * MTD structure for E3 (Delta)
  */
@@ -45,7 +43,9 @@  struct ams_delta_nand {
 	struct gpio_desc	*gpiod_nwe;
 	struct gpio_desc	*gpiod_ale;
 	struct gpio_desc	*gpiod_cle;
-	void __iomem		*io_base;
+	struct gpio_chip	*data_gpioc;
+	unsigned long		data_mask;
+	int			data_width;
 };
 
 /*
@@ -73,43 +73,55 @@  static const struct mtd_partition partition_info[] = {
 	  .size		=  3 * SZ_256K },
 };
 
-static void ams_delta_write_next_byte(struct ams_delta_nand *priv, u_char byte)
+static void ams_delta_write_commit(struct ams_delta_nand *priv)
 {
-	struct nand_chip *this = &priv->nand_chip;
-
-	writew(byte, this->IO_ADDR_W);
 	gpiod_set_value(priv->gpiod_nwe, 0);
 	ndelay(40);
 	gpiod_set_value(priv->gpiod_nwe, 1);
 }
 
+static void ams_delta_write_next_byte(struct ams_delta_nand *priv, u_char byte)
+{
+	struct gpio_chip *data_gpioc = priv->data_gpioc;
+	unsigned long bits = byte;
+
+	data_gpioc->set_multiple(data_gpioc, &priv->data_mask, &bits);
+
+	ams_delta_write_commit(priv);
+}
+
 static void ams_delta_write_first_byte(struct ams_delta_nand *priv, u_char byte)
 {
-	void __iomem *io_base = priv->io_base;
+	struct gpio_chip *data_gpioc = priv->data_gpioc;
+	unsigned long bits = byte;
+	int i;
 
-	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
+	for (i = 0; i < priv->data_width; i++)
+		data_gpioc->direction_output(data_gpioc, i, test_bit(i, &bits));
 
-	ams_delta_write_next_byte(priv, byte);
+	ams_delta_write_commit(priv);
 }
 
 static u_char ams_delta_read_next_byte(struct ams_delta_nand *priv)
 {
-	struct nand_chip *this = &priv->nand_chip;
-	u_char res;
+	struct gpio_chip *data_gpioc = priv->data_gpioc;
+	unsigned long bits;
 
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
-	res = readw(this->IO_ADDR_R);
+	data_gpioc->get_multiple(data_gpioc, &priv->data_mask, &bits);
 	gpiod_set_value(priv->gpiod_nre, 1);
 
-	return res;
+	return bits;
 }
 
 static u_char ams_delta_read_first_byte(struct ams_delta_nand *priv)
 {
-	void __iomem *io_base = priv->io_base;
+	struct gpio_chip *data_gpioc = priv->data_gpioc;
+	int i;
 
-	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
+	for (i = 0; i < priv->data_width; i++)
+		data_gpioc->direction_input(data_gpioc, i);
 
 	return ams_delta_read_next_byte(priv);
 }
@@ -188,16 +200,11 @@  static int ams_delta_init(struct platform_device *pdev)
 	struct ams_delta_nand *priv;
 	struct nand_chip *this;
 	struct mtd_info *mtd;
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	void __iomem *io_base;
 	struct gpio_descs *data_gpiods;
 	struct gpio_chip *data_gpioc;
 	unsigned long mask, bits;
 	int i, err = 0;
 
-	if (!res)
-		return -ENXIO;
-
 	/* Allocate memory for MTD device structure and private data */
 	priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
 			    GFP_KERNEL);
@@ -210,25 +217,8 @@  static int ams_delta_init(struct platform_device *pdev)
 	mtd = nand_to_mtd(this);
 	mtd->dev.parent = &pdev->dev;
 
-	/*
-	 * Don't try to request the memory region from here,
-	 * it should have been already requested from the
-	 * gpio-omap driver and requesting it again would fail.
-	 */
-
-	io_base = ioremap(res->start, resource_size(res));
-	if (io_base == NULL) {
-		dev_err(&pdev->dev, "ioremap failed\n");
-		err = -EIO;
-		goto out_free;
-	}
-
-	priv->io_base = io_base;
 	nand_set_controller_data(this, priv);
 
-	/* Set address of NAND IO lines */
-	this->IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
-	this->IO_ADDR_W = io_base + OMAP_MPUIO_OUTPUT;
 	this->read_byte = ams_delta_read_byte;
 	this->write_buf = ams_delta_write_buf;
 	this->read_buf = ams_delta_read_buf;
@@ -238,7 +228,7 @@  static int ams_delta_init(struct platform_device *pdev)
 	if (IS_ERR(priv->gpiod_rdy)) {
 		err = PTR_ERR(priv->gpiod_rdy);
 		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	if (priv->gpiod_rdy)
@@ -256,49 +246,49 @@  static int ams_delta_init(struct platform_device *pdev)
 	if (IS_ERR(priv->gpiod_nwp)) {
 		err = PTR_ERR(priv->gpiod_nwp);
 		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nce)) {
 		err = PTR_ERR(priv->gpiod_nce);
 		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nre)) {
 		err = PTR_ERR(priv->gpiod_nre);
 		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nwe)) {
 		err = PTR_ERR(priv->gpiod_nwe);
 		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->gpiod_ale)) {
 		err = PTR_ERR(priv->gpiod_ale);
 		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->gpiod_cle)) {
 		err = PTR_ERR(priv->gpiod_cle);
 		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 	/* Request array of data pins, initialize them as output and set low */
 	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_OUT_LOW);
 	if (IS_ERR(data_gpiods)) {
 		err = PTR_ERR(data_gpiods);
 		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	/* Use GPIO chip of first data GPIO pin descriptor */
@@ -312,7 +302,7 @@  static int ams_delta_init(struct platform_device *pdev)
 		err = -EINVAL;
 		dev_err(&pdev->dev,
 			"data GPIO chip does not support get/set_multiple()\n");
-		goto out_mtd;
+		return err;
 	}
 
 	/* Verify if get_multiple() returns all pins low as initialized above */
@@ -321,13 +311,13 @@  static int ams_delta_init(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"data GPIO chip get_multiple() failed: %d\n", err);
-		goto out_mtd;
+		return err;
 	}
 	if (bits) {
 		err = -EINVAL;
 		dev_err(&pdev->dev,
 			"mismmatch of data GPIO initial value: %lu\n", bits);
-		goto out_mtd;
+		return err;
 	}
 
 	/* Verify each data GPIO pin */
@@ -337,7 +327,7 @@  static int ams_delta_init(struct platform_device *pdev)
 			err = -EINVAL;
 			dev_err(&pdev->dev, "GPIO chip mismatch of data bit %d\n",
 				i);
-			goto out_mtd;
+			return err;
 		}
 
 		/* Require all pins active high (not inverted) */
@@ -346,7 +336,7 @@  static int ams_delta_init(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"unsupported polarity of data GPIO bit %d\n",
 				i);
-			goto out_mtd;
+			return err;
 		}
 
 		/*
@@ -360,13 +350,13 @@  static int ams_delta_init(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"data bit %d GPIO chip get() failed: %d\n", i,
 				err);
-			goto out_mtd;
+			return err;
 		}
 		if (!err) {
 			err = -EINVAL;
 			dev_err(&pdev->dev, "mismatch of data GPIO bit %d value\n",
 				i);
-			goto out_mtd;
+			return err;
 		}
 
 		/*
@@ -379,14 +369,18 @@  static int ams_delta_init(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"unsupported mode of data GPIO bit %d\n",
 				i);
-			goto out_mtd;
+			return err;
 		}
 	}
 
+	priv->data_gpioc = data_gpioc;
+	priv->data_width = data_gpiods->ndescs;
+	priv->data_mask = (1 << data_gpiods->ndescs) - 1;
+
 	/* Scan to find existence of the device */
 	err = nand_scan(mtd, 1);
 	if (err)
-		goto out_mtd;
+		return err;
 
 	/* As soon as the device is found, release write protection */
 	gpiod_set_value(priv->gpiod_nwp, 1);
@@ -394,13 +388,7 @@  static int ams_delta_init(struct platform_device *pdev)
 	/* Register the partitions */
 	mtd_device_register(mtd, partition_info, ARRAY_SIZE(partition_info));
 
-	goto out;
-
- out_mtd:
-	iounmap(io_base);
-out_free:
- out:
-	return err;
+	return 0;
 }
 
 /*
@@ -410,16 +398,13 @@  static int ams_delta_cleanup(struct platform_device *pdev)
 {
 	struct ams_delta_nand *priv = platform_get_drvdata(pdev);
 	struct mtd_info *mtd = nand_to_mtd(&priv->nand_chip);
-	void __iomem *io_base = priv->io_base;
 
 	/* Apply write protection */
 	gpiod_set_value(priv->gpiod_nwp, 0);
 
-	/* Release resources, unregister device */
+	/* Unregister device */
 	nand_release(mtd);
 
-	iounmap(io_base);
-
 	return 0;
 }