[RFC,v2,05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

Message ID 20180806222918.12644-6-jmkrzyszt@gmail.com
State New
Headers show
Series
  • [RFC,v2,01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner
Related show

Commit Message

Janusz Krzysztofik Aug. 6, 2018, 10:29 p.m.
Don't readw()/writew() data directly from/to GPIO port which is under
control of gpio-omap driver, use GPIO API instead.

Degrade of performance on Amstrad Delta is completely not acceptable.

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

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 97 ++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 59 deletions(-)

Comments

Marek Vasut Aug. 6, 2018, 11:57 p.m. | #1
On 08/07/2018 12:29 AM, Janusz Krzysztofik wrote:
> Don't readw()/writew() data directly from/to GPIO port which is under
> control of gpio-omap driver, use GPIO API instead.
> 
> Degrade of performance on Amstrad Delta is completely not acceptable.

I'd expect that changing from direct PIO to access through GPIO API
would degrade the performance. Maybe I misunderstood something ?

> The driver should work with any 8+-bit bidirectional GPIO port, not
> only OMAP.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 97 ++++++++++++++++------------------------
>  1 file changed, 38 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 09d6901fc94d..78996ddf82e0 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -24,13 +24,10 @@
>  #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)
>   */
> @@ -44,7 +41,7 @@ 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_descs	*data_gpiods;
>  };
>  
>  /*
> @@ -76,10 +73,14 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> -	void __iomem *io_base = priv->io_base;
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	unsigned long bits = byte;
> +	int i;
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		gpiod_direction_output_raw(data_gpiods->desc[i],
> +					   test_bit(i, &bits));
>  
> -	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
> -	writew(byte, this->IO_ADDR_W);
>  	gpiod_set_value(priv->gpiod_nwe, 0);
>  	ndelay(40);
>  	gpiod_set_value(priv->gpiod_nwe, 1);
> @@ -87,18 +88,28 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  
>  static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  {
> -	u_char res;
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> -	void __iomem *io_base = priv->io_base;
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	unsigned long bits = 0;
> +	int i, value_array[data_gpiods->ndescs];
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		gpiod_direction_input(data_gpiods->desc[i]);
>  
>  	gpiod_set_value(priv->gpiod_nre, 0);
>  	ndelay(40);
> -	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
> -	res = readw(this->IO_ADDR_R);
> +
> +	gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
> +				  value_array);
> +
>  	gpiod_set_value(priv->gpiod_nre, 1);
>  
> -	return res;
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		if (value_array[i])
> +			__set_bit(i, &bits);
> +
> +	return bits;
>  }
>  
>  static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
> @@ -159,14 +170,8 @@ 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;
>  	int 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);
> @@ -179,25 +184,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;
> @@ -207,7 +195,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)
> @@ -225,66 +213,60 @@ 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 input */
> -	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
> -	if (IS_ERR(data_gpiods)) {
> -		err = PTR_ERR(data_gpiods);
> +	priv->data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
> +	if (IS_ERR(priv->data_gpiods)) {
> +		err = PTR_ERR(priv->data_gpiods);
>  		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	/* Scan to find existence of the device */
>  	err = nand_scan(mtd, 1);
>  	if (err)
> -		goto out_mtd;
> +		return err;
>  
>  	/* 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;
>  }
>  
>  /*
> @@ -294,13 +276,10 @@ 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;
>  
> -	/* Release resources, unregister device */
> +	/* Unregister device */
>  	nand_release(mtd);
>  
> -	iounmap(io_base);
> -
>  	return 0;
>  }
>  
>
Boris Brezillon Aug. 7, 2018, 5:06 p.m. | #2
On Tue,  7 Aug 2018 00:29:11 +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 API instead.
> 
> Degrade of performance on Amstrad Delta is completely not acceptable.

Can we have numbers along with information about where the overhead is
when using gpiod_{get,set}_raw_array_value()?

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

That's cool!
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Janusz Krzysztofik Aug. 7, 2018, 5:11 p.m. | #3
Hi Boris,

On Tuesday, August 7, 2018 7:06:27 PM CEST Boris Brezillon wrote:
> On Tue,  7 Aug 2018 00:29:11 +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 API instead.
> > 
> > Degrade of performance on Amstrad Delta is completely not acceptable.
> 
> Can we have numbers along with information about where the overhead is
> when using gpiod_{get,set}_raw_array_value()?

Yes, as soon as I get physical access to the device, probably this or next 
weekend.

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 10, 2018, 10:25 a.m. | #4
On Tue, Aug 7, 2018 at 12:29 AM 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 API instead.
>
> Degrade of performance on Amstrad Delta is completely not acceptable.
>
> The driver should work with any 8+-bit bidirectional GPIO port, not
> only OMAP.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Patch

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 09d6901fc94d..78996ddf82e0 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -24,13 +24,10 @@ 
 #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)
  */
@@ -44,7 +41,7 @@  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_descs	*data_gpiods;
 };
 
 /*
@@ -76,10 +73,14 @@  static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
-	void __iomem *io_base = priv->io_base;
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	unsigned long bits = byte;
+	int i;
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		gpiod_direction_output_raw(data_gpiods->desc[i],
+					   test_bit(i, &bits));
 
-	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
-	writew(byte, this->IO_ADDR_W);
 	gpiod_set_value(priv->gpiod_nwe, 0);
 	ndelay(40);
 	gpiod_set_value(priv->gpiod_nwe, 1);
@@ -87,18 +88,28 @@  static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 
 static u_char ams_delta_read_byte(struct mtd_info *mtd)
 {
-	u_char res;
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
-	void __iomem *io_base = priv->io_base;
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	unsigned long bits = 0;
+	int i, value_array[data_gpiods->ndescs];
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		gpiod_direction_input(data_gpiods->desc[i]);
 
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
-	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
-	res = readw(this->IO_ADDR_R);
+
+	gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
+				  value_array);
+
 	gpiod_set_value(priv->gpiod_nre, 1);
 
-	return res;
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		if (value_array[i])
+			__set_bit(i, &bits);
+
+	return bits;
 }
 
 static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
@@ -159,14 +170,8 @@  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;
 	int 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);
@@ -179,25 +184,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;
@@ -207,7 +195,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)
@@ -225,66 +213,60 @@  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 input */
-	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
-	if (IS_ERR(data_gpiods)) {
-		err = PTR_ERR(data_gpiods);
+	priv->data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
+	if (IS_ERR(priv->data_gpiods)) {
+		err = PTR_ERR(priv->data_gpiods);
 		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	/* Scan to find existence of the device */
 	err = nand_scan(mtd, 1);
 	if (err)
-		goto out_mtd;
+		return err;
 
 	/* 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;
 }
 
 /*
@@ -294,13 +276,10 @@  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;
 
-	/* Release resources, unregister device */
+	/* Unregister device */
 	nand_release(mtd);
 
-	iounmap(io_base);
-
 	return 0;
 }