| Message ID | 20180718235710.18242-8-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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
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
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 --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)
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(-)