diff mbox

[RFC,7/8] mtd: rawnand: ams-delta: Check sanity of data GPIO resource

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

Commit Message

Janusz Krzysztofik July 18, 2018, 11:57 p.m. UTC
The plan is to replace data port readw()/writew() operations with GPIO
callbacks provided by gpio-omap driver.  For acceptable performance the
GPIO chip must support get/set_multiple() GPIO callbacks.

In order to avoid data corruption, we require the array of data GPIO
descriptors obtained with gpiod_get_array() to meet some strict
requirements:
- all pins must belong to the same single GPIO chip,
- array index of each pin descriptor must match its hardware number,
- pin polarity must not be inverted,
- pin hardware configuration must not be open drain nor open source.

Let's implement the above described sanity checks before replacing the
readw()/writew() operations witn GPIO callbacks.  If a check fails,
return -EINVAL to indicate the board provided GPIO setup is invalid.

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

Comments

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

> The plan is to replace data port readw()/writew() operations with GPIO
> callbacks provided by gpio-omap driver.  For acceptable performance the
> GPIO chip must support get/set_multiple() GPIO callbacks.
> 
> In order to avoid data corruption, we require the array of data GPIO
> descriptors obtained with gpiod_get_array() to meet some strict
> requirements:
> - all pins must belong to the same single GPIO chip,

You shouldn't care. The parallel NAND interface has WE/RE signals to
trigger a write/read on the data bus, that means you can change data
signals independently without risking data corruption as long as RE/WE
stay high (or low, I don't remember the active state on these pins). Of
course it's slower if you have to toggle data pins independently, but
that's not your problem. It's up to the HW designer to route things
correctly.

> - array index of each pin descriptor must match its hardware number,

Again, this is not really a problem. You'll just have to swap bits if
this is not the case. Not a big deal.

> - pin polarity must not be inverted,

Why?

> - pin hardware configuration must not be open drain nor open source.

This should be taken care of when requesting the pins.

> 
> Let's implement the above described sanity checks before replacing the
> readw()/writew() operations witn GPIO callbacks.  If a check fails,
> return -EINVAL to indicate the board provided GPIO setup is invalid.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 87 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index ad62c0245458..bd501f385e78 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -21,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>

Wow! This is a very very bad idea. There's a clear separation between
the GPIO consumer and the GPIO driver API for a good reason, and
you're violating this. linux/gpio/driver.h should only be included by
GPIO controller drivers.

>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
> @@ -190,7 +191,9 @@ static int ams_delta_init(struct platform_device *pdev)
>  	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	void __iomem *io_base;
>  	struct gpio_descs *data_gpiods;
> -	int err = 0;
> +	struct gpio_chip *data_gpioc;
> +	unsigned long mask, bits;
> +	int i, err = 0;
>  
>  	if (!res)
>  		return -ENXIO;
> @@ -298,6 +301,88 @@ static int ams_delta_init(struct platform_device *pdev)
>  		goto out_mtd;
>  	}
>  
> +	/* Use GPIO chip of first data GPIO pin descriptor */
> +	data_gpioc = gpiod_to_chip(data_gpiods->desc[0]);
> +
> +	/*
> +	 * For acceptable performance require the data GPIO
> +	 * chip to support get/set_multiple() callbacks.
> +	 */
> +	if (!data_gpioc->get_multiple || !data_gpioc->set_multiple) {
> +		err = -EINVAL;
> +		dev_err(&pdev->dev,
> +			"data GPIO chip does not support get/set_multiple()\n");
> +		goto out_mtd;
> +	}
> +
> +	/* Verify if get_multiple() returns all pins low as initialized above */
> +	mask = (1 << data_gpiods->ndescs) - 1;
> +	err = data_gpioc->get_multiple(data_gpioc, &mask, &bits);

And this, you shouldn't do. You should instead go through the GPIO
consumer API to get the pin state (gpiod_get_raw_array_value()).

I guess you'd prefer to have the pin values in a bitmap instead of an
array of integers. That's probably something you can discuss with
Linus, see if he would accept to change the prototype of
gpiod_get_raw_array_value().

> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"data GPIO chip get_multiple() failed: %d\n", err);
> +		goto out_mtd;
> +	}
> +	if (bits) {
> +		err = -EINVAL;
> +		dev_err(&pdev->dev,
> +			"mismmatch of data GPIO initial value: %lu\n", bits);
> +		goto out_mtd;
> +	}
> +
> +	/* Verify each data GPIO pin */
> +	for (i = 0; i < data_gpiods->ndescs; i++) {
> +		/* Require all pins belong to the same GPIO chip */
> +		if (gpiod_to_chip(data_gpiods->desc[i]) != data_gpioc) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev, "GPIO chip mismatch of data bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/* Require all pins active high (not inverted) */
> +		if (gpiod_is_active_low(data_gpiods->desc[i])) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev,
> +				"unsupported polarity of data GPIO bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/*
> +		 * Require pin gpiod array index to match hardware pin number.
> +		 * Verified by setting the pin high with gpiod_set_raw_value()
> +		 * then reading it back with gpiochip->get() for comparison.
> +		 */
> +		gpiod_set_raw_value(data_gpiods->desc[i], 1);
> +		err = data_gpioc->get(data_gpioc, i);
> +		if (err < 0) {
> +			dev_err(&pdev->dev,
> +				"data bit %d GPIO chip get() failed: %d\n", i,
> +				err);
> +			goto out_mtd;
> +		}
> +		if (!err) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev, "mismatch of data GPIO bit %d value\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/*
> +		 * Check for unsupported pin hardware configuration.  Use
> +		 * just verified gpiod array index as hardware pin number.
> +		 */
> +		if (gpiochip_line_is_open_drain(data_gpioc, i) ||
> +		    gpiochip_line_is_open_source(data_gpioc, i)) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev,
> +				"unsupported mode of data GPIO bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +	}
> +
>  	/* Scan to find existence of the device */
>  	err = nand_scan(mtd, 1);
>  	if (err)
Linus Walleij July 29, 2018, 8:33 p.m. UTC | #2
Hi Janusz!

Nice work overall! Some feedback:

On Thu, Jul 19, 2018 at 1:57 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> +#include <linux/gpio/driver.h>

Let's skip that.

> +       /*
> +        * For acceptable performance require the data GPIO
> +        * chip to support get/set_multiple() callbacks.
> +        */
> +       if (!data_gpioc->get_multiple || !data_gpioc->set_multiple) {
> +               err = -EINVAL;
> +               dev_err(&pdev->dev,
> +                       "data GPIO chip does not support get/set_multiple()\n");
> +               goto out_mtd;
> +       }

Since we know which platform it is, we know that we applied the previous
get/set multiple patch, so we need not check this if the patches are
applied in sequence.

As long as all patches go in the same merge window, no problem.

I'm BTW ready to apply the get/set multiple patch already.

Yours,
Linus Walleij
Linus Walleij July 29, 2018, 8:36 p.m. UTC | #3
On Thu, Jul 19, 2018 at 8:44 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> I guess you'd prefer to have the pin values in a bitmap instead of an
> array of integers. That's probably something you can discuss with
> Linus, see if he would accept to change the prototype of
> gpiod_get_raw_array_value().

I am not so smart as to see the overall effects but if what
you're saying is that we shouldn't have designed these functions
and callbacks using arrays of integers (or longs) and instead
pass bitmaps, you may be just right. It intuitively sounds better.
But I'm not good with bitmaps.

We would have to refactor the world though.

Yours,
Linus Walleij
Boris Brezillon July 29, 2018, 9:16 p.m. UTC | #4
On Sun, 29 Jul 2018 22:36:49 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Jul 19, 2018 at 8:44 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> 
> > I guess you'd prefer to have the pin values in a bitmap instead of an
> > array of integers. That's probably something you can discuss with
> > Linus, see if he would accept to change the prototype of
> > gpiod_get_raw_array_value().  
> 
> I am not so smart as to see the overall effects but if what
> you're saying is that we shouldn't have designed these functions
> and callbacks using arrays of integers (or longs) and instead
> pass bitmaps, you may be just right. It intuitively sounds better.

Maybe, maybe not. Only an real evaluation of where the overhead is can
tell us. So I'd suggest trying to use the existing interface and doing
the int_array -> u8 conversion in the NAND driver first, and see how
much moving to a bitmap imprsoves things.

> But I'm not good with bitmaps.
> 
> We would have to refactor the world though.

I checked and, AFAICT, there are no external users of this API (only
core code is using those funcs, for ioctls I guess). Anyway, I think
we should wait for real numbers before we consider doing this change
(It's not unusual to get this sort of things wrong).
diff mbox

Patch

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index ad62c0245458..bd501f385e78 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -21,6 +21,7 @@ 
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
@@ -190,7 +191,9 @@  static int ams_delta_init(struct platform_device *pdev)
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	void __iomem *io_base;
 	struct gpio_descs *data_gpiods;
-	int err = 0;
+	struct gpio_chip *data_gpioc;
+	unsigned long mask, bits;
+	int i, err = 0;
 
 	if (!res)
 		return -ENXIO;
@@ -298,6 +301,88 @@  static int ams_delta_init(struct platform_device *pdev)
 		goto out_mtd;
 	}
 
+	/* Use GPIO chip of first data GPIO pin descriptor */
+	data_gpioc = gpiod_to_chip(data_gpiods->desc[0]);
+
+	/*
+	 * For acceptable performance require the data GPIO
+	 * chip to support get/set_multiple() callbacks.
+	 */
+	if (!data_gpioc->get_multiple || !data_gpioc->set_multiple) {
+		err = -EINVAL;
+		dev_err(&pdev->dev,
+			"data GPIO chip does not support get/set_multiple()\n");
+		goto out_mtd;
+	}
+
+	/* Verify if get_multiple() returns all pins low as initialized above */
+	mask = (1 << data_gpiods->ndescs) - 1;
+	err = data_gpioc->get_multiple(data_gpioc, &mask, &bits);
+	if (err) {
+		dev_err(&pdev->dev,
+			"data GPIO chip get_multiple() failed: %d\n", err);
+		goto out_mtd;
+	}
+	if (bits) {
+		err = -EINVAL;
+		dev_err(&pdev->dev,
+			"mismmatch of data GPIO initial value: %lu\n", bits);
+		goto out_mtd;
+	}
+
+	/* Verify each data GPIO pin */
+	for (i = 0; i < data_gpiods->ndescs; i++) {
+		/* Require all pins belong to the same GPIO chip */
+		if (gpiod_to_chip(data_gpiods->desc[i]) != data_gpioc) {
+			err = -EINVAL;
+			dev_err(&pdev->dev, "GPIO chip mismatch of data bit %d\n",
+				i);
+			goto out_mtd;
+		}
+
+		/* Require all pins active high (not inverted) */
+		if (gpiod_is_active_low(data_gpiods->desc[i])) {
+			err = -EINVAL;
+			dev_err(&pdev->dev,
+				"unsupported polarity of data GPIO bit %d\n",
+				i);
+			goto out_mtd;
+		}
+
+		/*
+		 * Require pin gpiod array index to match hardware pin number.
+		 * Verified by setting the pin high with gpiod_set_raw_value()
+		 * then reading it back with gpiochip->get() for comparison.
+		 */
+		gpiod_set_raw_value(data_gpiods->desc[i], 1);
+		err = data_gpioc->get(data_gpioc, i);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"data bit %d GPIO chip get() failed: %d\n", i,
+				err);
+			goto out_mtd;
+		}
+		if (!err) {
+			err = -EINVAL;
+			dev_err(&pdev->dev, "mismatch of data GPIO bit %d value\n",
+				i);
+			goto out_mtd;
+		}
+
+		/*
+		 * Check for unsupported pin hardware configuration.  Use
+		 * just verified gpiod array index as hardware pin number.
+		 */
+		if (gpiochip_line_is_open_drain(data_gpioc, i) ||
+		    gpiochip_line_is_open_source(data_gpioc, i)) {
+			err = -EINVAL;
+			dev_err(&pdev->dev,
+				"unsupported mode of data GPIO bit %d\n",
+				i);
+			goto out_mtd;
+		}
+	}
+
 	/* Scan to find existence of the device */
 	err = nand_scan(mtd, 1);
 	if (err)