| Message ID | 20180902120144.6855-5-jmkrzyszt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array | expand |
Hi All, On 2018-09-02 14:01, Janusz Krzysztofik wrote: > Certain GPIO descriptor arrays returned by gpio_get_array() may contain > information on direct mapping of array members to pins of a single GPIO > chip in hardware order. In such cases, bitmaps of values can be passed > directly from/to the chip's .get/set_multiple() callbacks without > wasting time on iterations. > > Add respective code to gpiod_get/set_array_bitmap_complex() functions. > Pins not applicable for fast path are processed as before, skipping > over the 'fast' ones. > > Cc: Jonathan Corbet <corbet@lwn.net> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> I've just noticed that this patch landed in today's linux-next. Sadly it breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, device-tree source arch/arm/boot/dts/exynos5250-snow.dts). Booting hangs after detecting MMC cards. Reverting this patch fixes the boot. I will try later to add some debugs and investigate it further what really happens when booting hangs. > --- > Documentation/driver-api/gpio/board.rst | 15 ++++++ > Documentation/driver-api/gpio/consumer.rst | 8 +++ > drivers/gpio/gpiolib.c | 87 ++++++++++++++++++++++++++++-- > 3 files changed, 105 insertions(+), 5 deletions(-) > > diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst > index 2c112553df84..c66821e033c2 100644 > --- a/Documentation/driver-api/gpio/board.rst > +++ b/Documentation/driver-api/gpio/board.rst > @@ -193,3 +193,18 @@ And the table can be added to the board code as follows:: > > The line will be hogged as soon as the gpiochip is created or - in case the > chip was created earlier - when the hog table is registered. > + > +Arrays of pins > +-------------- > +In addition to requesting pins belonging to a function one by one, a device may > +also request an array of pins assigned to the function. The way those pins are > +mapped to the device determines if the array qualifies for fast bitmap > +processing. If yes, a bitmap is passed over get/set array functions directly > +between a caller and a respective .get/set_multiple() callback of a GPIO chip. > + > +In order to qualify for fast bitmap processing, the pin mapping must meet the > +following requirements: > +- it must belong to the same chip as other 'fast' pins of the function, > +- its index within the function must match its hardware number within the chip. > + > +Open drain and open source pins are excluded from fast bitmap output processing. > diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst > index 0afd95a12b10..cf992e5ab976 100644 > --- a/Documentation/driver-api/gpio/consumer.rst > +++ b/Documentation/driver-api/gpio/consumer.rst > @@ -388,6 +388,14 @@ array_info should be set to NULL. > Note that for optimal performance GPIOs belonging to the same chip should be > contiguous within the array of descriptors. > > +Still better performance may be achieved if array indexes of the descriptors > +match hardware pin numbers of a single chip. If an array passed to a get/set > +array function matches the one obtained from gpiod_get_array() and array_info > +associated with the array is also passed, the function may take a fast bitmap > +processing path, passing the value_bitmap argument directly to the respective > +.get/set_multiple() callback of the chip. That allows for utilization of GPIO > +banks as data I/O ports without much loss of performance. > + > The return value of gpiod_get_array_value() and its variants is 0 on success > or negative on error. Note the difference to gpiod_get_value(), which returns > 0 or 1 on success to convey the GPIO value. With the array functions, the GPIO > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index cef6ee31fe05..b9d083fb13ee 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2789,7 +2789,36 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > struct gpio_array *array_info, > unsigned long *value_bitmap) > { > - int i = 0; > + int err, i = 0; > + > + /* > + * Validate array_info against desc_array and its size. > + * It should immediately follow desc_array if both > + * have been obtained from the same gpiod_get_array() call. > + */ > + if (array_info && array_info->desc == desc_array && > + array_size <= array_info->size && > + (void *)array_info == desc_array + array_info->size) { > + if (!can_sleep) > + WARN_ON(array_info->chip->can_sleep); > + > + err = gpio_chip_get_multiple(array_info->chip, > + array_info->get_mask, > + value_bitmap); > + if (err) > + return err; > + > + if (!raw && !bitmap_empty(array_info->invert_mask, array_size)) > + bitmap_xor(value_bitmap, value_bitmap, > + array_info->invert_mask, array_size); > + > + if (bitmap_full(array_info->get_mask, array_size)) > + return 0; > + > + i = find_first_zero_bit(array_info->get_mask, array_size); > + } else { > + array_info = NULL; > + } > > while (i < array_size) { > struct gpio_chip *chip = desc_array[i]->gdev->chip; > @@ -2820,7 +2849,12 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > int hwgpio = gpio_chip_hwgpio(desc); > > __set_bit(hwgpio, mask); > - i++; > + > + if (array_info) > + find_next_zero_bit(array_info->get_mask, > + array_size, i); > + else > + i++; > } while ((i < array_size) && > (desc_array[i]->gdev->chip == chip)); > > @@ -2831,7 +2865,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > return ret; > } > > - for (j = first; j < i; j++) { > + for (j = first; j < i; ) { > const struct gpio_desc *desc = desc_array[j]; > int hwgpio = gpio_chip_hwgpio(desc); > int value = test_bit(hwgpio, bits); > @@ -2840,6 +2874,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > value = !value; > __assign_bit(j, value_bitmap, value); > trace_gpio_value(desc_to_gpio(desc), 1, value); > + > + if (array_info) > + find_next_zero_bit(array_info->get_mask, i, j); > + else > + j++; > } > > if (mask != fastpath) > @@ -3041,6 +3080,32 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, > { > int i = 0; > > + /* > + * Validate array_info against desc_array and its size. > + * It should immediately follow desc_array if both > + * have been obtained from the same gpiod_get_array() call. > + */ > + if (array_info && array_info->desc == desc_array && > + array_size <= array_info->size && > + (void *)array_info == desc_array + array_info->size) { > + if (!can_sleep) > + WARN_ON(array_info->chip->can_sleep); > + > + if (!raw && !bitmap_empty(array_info->invert_mask, array_size)) > + bitmap_xor(value_bitmap, value_bitmap, > + array_info->invert_mask, array_size); > + > + gpio_chip_set_multiple(array_info->chip, array_info->set_mask, > + value_bitmap); > + > + if (bitmap_full(array_info->set_mask, array_size)) > + return 0; > + > + i = find_first_zero_bit(array_info->set_mask, array_size); > + } else { > + array_info = NULL; > + } > + > while (i < array_size) { > struct gpio_chip *chip = desc_array[i]->gdev->chip; > unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]; > @@ -3068,7 +3133,14 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, > int hwgpio = gpio_chip_hwgpio(desc); > int value = test_bit(i, value_bitmap); > > - if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags)) > + /* > + * Pins applicable for fast input but not for > + * fast output processing may have been already > + * inverted inside the fast path, skip them. > + */ > + if (!raw && !(array_info && > + test_bit(i, array_info->invert_mask)) && > + test_bit(FLAG_ACTIVE_LOW, &desc->flags)) > value = !value; > trace_gpio_value(desc_to_gpio(desc), 0, value); > /* > @@ -3087,7 +3159,12 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, > __clear_bit(hwgpio, bits); > count++; > } > - i++; > + > + if (array_info) > + find_next_zero_bit(array_info->set_mask, > + array_size, i); > + else > + i++; > } while ((i < array_size) && > (desc_array[i]->gdev->chip == chip)); > /* push collected bits to outputs */ > Best regards
On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: > Hi All, > > On 2018-09-02 14:01, Janusz Krzysztofik wrote: > > Certain GPIO descriptor arrays returned by gpio_get_array() may contain > > information on direct mapping of array members to pins of a single GPIO > > chip in hardware order. In such cases, bitmaps of values can be passed > > directly from/to the chip's .get/set_multiple() callbacks without > > wasting time on iterations. > > > > Add respective code to gpiod_get/set_array_bitmap_complex() functions. > > Pins not applicable for fast path are processed as before, skipping > > over the 'fast' ones. > > > > Cc: Jonathan Corbet <corbet@lwn.net> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > I've just noticed that this patch landed in today's linux-next. Sadly it > breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, > device-tree source arch/arm/boot/dts/exynos5250-snow.dts). > > Booting hangs after detecting MMC cards. Reverting this patch fixes the > boot. I will try later to add some debugs and investigate it further what > really happens when booting hangs. Hi Marek, Thanks for reporting. Could you please try the following fix? Thanks, Janusz From d7ecd435bfb4972766b63ac383a43875700c7452 Mon Sep 17 00:00:00 2001 From: Janusz Krzysztofik <jmkrzyszt@gmail.com> Date: Thu, 20 Sep 2018 17:37:21 +0200 Subject: [PATCH] gpiolib: Fix bitmap index not updated While skipping fast path bits, bitmap index is not updated with next found zero bit position. Fix it. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- drivers/gpio/gpiolib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index a53d17745d21..5bc3447949c9 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, __set_bit(hwgpio, mask); if (array_info) - find_next_zero_bit(array_info->get_mask, + i = find_next_zero_bit(array_info->get_mask, array_size, i); else i++; @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, trace_gpio_value(desc_to_gpio(desc), 1, value); if (array_info) - find_next_zero_bit(array_info->get_mask, i, j); + i = find_next_zero_bit(array_info->get_mask, i, + j); else j++; } @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, } if (array_info) - find_next_zero_bit(array_info->set_mask, + i = find_next_zero_bit(array_info->set_mask, array_size, i); else i++;
On Thu, Sep 20, 2018 at 3:11 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > I've just noticed that this patch landed in today's linux-next. Sadly it > breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, > device-tree source arch/arm/boot/dts/exynos5250-snow.dts). Thanks for testing on this platform! > Booting hangs after detecting MMC cards. Reverting this patch fixes the > boot. I will try later to add some debugs and investigate it further what > really happens when booting hangs. How typical. I hope we can fix it, because this should mean speedups for your platform. Yours, Linus Walleij
On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote: > On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: > > Hi All, > > > > On 2018-09-02 14:01, Janusz Krzysztofik wrote: > > > Certain GPIO descriptor arrays returned by gpio_get_array() may contain > > > information on direct mapping of array members to pins of a single GPIO > > > chip in hardware order. In such cases, bitmaps of values can be passed > > > directly from/to the chip's .get/set_multiple() callbacks without > > > wasting time on iterations. > > > > > > Add respective code to gpiod_get/set_array_bitmap_complex() functions. > > > Pins not applicable for fast path are processed as before, skipping > > > over the 'fast' ones. > > > > > > Cc: Jonathan Corbet <corbet@lwn.net> > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > > > I've just noticed that this patch landed in today's linux-next. Sadly it > > breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, > > device-tree source arch/arm/boot/dts/exynos5250-snow.dts). > > > > Booting hangs after detecting MMC cards. Reverting this patch fixes the > > boot. I will try later to add some debugs and investigate it further what > > really happens when booting hangs. > > Hi Marek, > > Thanks for reporting. Could you please try the following fix? Hi again, I realized the patch was not correct, j, not i, should be updated in second hunk. Please try the following one. Thanks, Janusz From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 From: Janusz Krzysztofik <jmkrzyszt@gmail.com> Date: Thu, 20 Sep 2018 17:37:21 +0200 Subject: [PATCH] gpiolib: Fix bitmap index not updated While skipping fast path bits, bitmap index is not updated with next found zero bit position. Fix it. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- drivers/gpio/gpiolib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index a53d17745d21..369bdd358fcc 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, __set_bit(hwgpio, mask); if (array_info) - find_next_zero_bit(array_info->get_mask, + i = find_next_zero_bit(array_info->get_mask, array_size, i); else i++; @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, trace_gpio_value(desc_to_gpio(desc), 1, value); if (array_info) - find_next_zero_bit(array_info->get_mask, i, j); + j = find_next_zero_bit(array_info->get_mask, i, + j); else j++; } @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, } if (array_info) - find_next_zero_bit(array_info->set_mask, + i = find_next_zero_bit(array_info->set_mask, array_size, i); else i++;
On Thu, Sep 20, 2018 at 05:48:22PM +0200, Janusz Krzysztofik wrote: > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index a53d17745d21..5bc3447949c9 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > __set_bit(hwgpio, mask); > > if (array_info) > - find_next_zero_bit(array_info->get_mask, > + i = find_next_zero_bit(array_info->get_mask, > array_size, i); We could mark find_next_zero_bit() and friends as a __must_check functions so we avoid this bug in the future. I have a more complicated idea how to detect these bugs in a generic way using Smatch but it will take longer to implement. regards, dan carpenter
Hi Janusz, On 2018-09-20 18:21, Janusz Krzysztofik wrote: > On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote: >> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: >>> On 2018-09-02 14:01, Janusz Krzysztofik wrote: >>>> Certain GPIO descriptor arrays returned by gpio_get_array() may contain >>>> information on direct mapping of array members to pins of a single GPIO >>>> chip in hardware order. In such cases, bitmaps of values can be passed >>>> directly from/to the chip's .get/set_multiple() callbacks without >>>> wasting time on iterations. >>>> >>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions. >>>> Pins not applicable for fast path are processed as before, skipping >>>> over the 'fast' ones. >>>> >>>> Cc: Jonathan Corbet <corbet@lwn.net> >>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>> I've just noticed that this patch landed in today's linux-next. Sadly it >>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, >>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts). >>> >>> Booting hangs after detecting MMC cards. Reverting this patch fixes the >>> boot. I will try later to add some debugs and investigate it further what >>> really happens when booting hangs. >> Hi Marek, >> >> Thanks for reporting. Could you please try the following fix? > Hi again, > > I realized the patch was not correct, j, not i, should be updated in second > hunk. Please try the following one. > > Thanks, > Janusz > > >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 > From: Janusz Krzysztofik <jmkrzyszt@gmail.com> > Date: Thu, 20 Sep 2018 17:37:21 +0200 > Subject: [PATCH] gpiolib: Fix bitmap index not updated > While skipping fast path bits, bitmap index is not updated with next > found zero bit position. Fix it. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> This one also doesn't help. A quick compare of logs with this version and a working system shows, that with your patch (and fix) there are no calls to gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If you need any more information (what kind of logs will help?), let me know. > --- > drivers/gpio/gpiolib.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index a53d17745d21..369bdd358fcc 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > __set_bit(hwgpio, mask); > > if (array_info) > - find_next_zero_bit(array_info->get_mask, > + i = find_next_zero_bit(array_info->get_mask, > array_size, i); > else > i++; > @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, > trace_gpio_value(desc_to_gpio(desc), 1, value); > > if (array_info) > - find_next_zero_bit(array_info->get_mask, i, j); > + j = find_next_zero_bit(array_info->get_mask, i, > + j); > else > j++; > } > @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, > } > > if (array_info) > - find_next_zero_bit(array_info->set_mask, > + i = find_next_zero_bit(array_info->set_mask, > array_size, i); > else > i++; Best regards
Hi Marek, 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: > Hi Janusz, > > On 2018-09-20 18:21, Janusz Krzysztofik wrote: >> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote: >>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: >>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote: >>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may >>>>> contain >>>>> information on direct mapping of array members to pins of a single >>>>> GPIO >>>>> chip in hardware order. In such cases, bitmaps of values can be >>>>> passed >>>>> directly from/to the chip's .get/set_multiple() callbacks without >>>>> wasting time on iterations. >>>>> >>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions. >>>>> Pins not applicable for fast path are processed as before, skipping >>>>> over the 'fast' ones. >>>>> >>>>> Cc: Jonathan Corbet <corbet@lwn.net> >>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>>> I've just noticed that this patch landed in today's linux-next. Sadly >>>> it >>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, >>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts). >>>> >>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the >>>> boot. I will try later to add some debugs and investigate it further >>>> what >>>> really happens when booting hangs. >>> Hi Marek, >>> >>> Thanks for reporting. Could you please try the following fix? >> Hi again, >> >> I realized the patch was not correct, j, not i, should be updated in >> second >> hunk. Please try the following one. >> >> Thanks, >> Janusz >> >> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 >> From: Janusz Krzysztofik <jmkrzyszt@gmail.com> >> Date: Thu, 20 Sep 2018 17:37:21 +0200 >> Subject: [PATCH] gpiolib: Fix bitmap index not updated >> While skipping fast path bits, bitmap index is not updated with next >> found zero bit position. Fix it. >> >> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > This one also doesn't help. A quick compare of logs with this version and > a working system shows, that with your patch (and fix) there are no calls > to > gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If > you need any more information (what kind of logs will help?), let me know. There is a debug message on array_info content available at the end of gpiod_get_array(), could you please activate it and post the message so we can understand better what is going on? On the other hand, I've had a look your device-tree configuration and it looks like that specific setup won't benefit from the fast bitmap path. You have pin 2 at position 0 and pin 1 at position 1 of the array. Hence, the fast bitmap path covers only pin 1, and pin 2 is processed by the old path with apparently buggy code for skipping over fast pins. As a temporary workaround, you could try to revert the order of pins in your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code should work for you again by taking the original old path, not skipping over fast pins. Results of such check may also help us to better understand and resolve the issue. Thanks, Janusz > >> --- >> drivers/gpio/gpiolib.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index a53d17745d21..369bdd358fcc 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool >> can_sleep, >> __set_bit(hwgpio, mask); >> >> if (array_info) >> - find_next_zero_bit(array_info->get_mask, >> + i = find_next_zero_bit(array_info->get_mask, >> array_size, i); >> else >> i++; >> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool >> can_sleep, >> trace_gpio_value(desc_to_gpio(desc), 1, value); >> >> if (array_info) >> - find_next_zero_bit(array_info->get_mask, i, j); >> + j = find_next_zero_bit(array_info->get_mask, i, >> + j); >> else >> j++; >> } >> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool >> can_sleep, >> } >> >> if (array_info) >> - find_next_zero_bit(array_info->set_mask, >> + i = find_next_zero_bit(array_info->set_mask, >> array_size, i); >> else >> i++; > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > >
Hi Marek, 2018-09-21 12:51 GMT+02:00, Janusz Krzysztofik <jmkrzyszt@gmail.com>: > Hi Marek, > > 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: >> Hi Janusz, >> >> On 2018-09-20 18:21, Janusz Krzysztofik wrote: >>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik >>> wrote: >>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski >>>> wrote: >>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote: >>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may >>>>>> contain >>>>>> information on direct mapping of array members to pins of a single >>>>>> GPIO >>>>>> chip in hardware order. In such cases, bitmaps of values can be >>>>>> passed >>>>>> directly from/to the chip's .get/set_multiple() callbacks without >>>>>> wasting time on iterations. >>>>>> >>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() >>>>>> functions. >>>>>> Pins not applicable for fast path are processed as before, skipping >>>>>> over the 'fast' ones. >>>>>> >>>>>> Cc: Jonathan Corbet <corbet@lwn.net> >>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>>>> I've just noticed that this patch landed in today's linux-next. Sadly >>>>> it >>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, >>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts). >>>>> >>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes >>>>> the >>>>> boot. I will try later to add some debugs and investigate it further >>>>> what >>>>> really happens when booting hangs. >>>> Hi Marek, >>>> >>>> Thanks for reporting. Could you please try the following fix? >>> Hi again, >>> >>> I realized the patch was not correct, j, not i, should be updated in >>> second >>> hunk. Please try the following one. >>> >>> Thanks, >>> Janusz >>> >>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 >>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>> Date: Thu, 20 Sep 2018 17:37:21 +0200 >>> Subject: [PATCH] gpiolib: Fix bitmap index not updated >>> While skipping fast path bits, bitmap index is not updated with next >>> found zero bit position. Fix it. >>> >>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >> >> This one also doesn't help. A quick compare of logs with this version and >> a working system shows, that with your patch (and fix) there are no calls >> to >> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If >> you need any more information (what kind of logs will help?), let me >> know. One more question. You said before that booting hanged after detecting MMC cards. Without the fix, I could imagine it keeps iterating with index not updated and simply never returns from gpiod_get/set_array_bitmap_complex(). Is the behaviour you observe the same with the fix applied? Thanks, Janusz > There is a debug message on array_info content available at the end of > gpiod_get_array(), could you please activate it and post the message so > we can understand better what is going on? > > On the other hand, I've had a look your device-tree configuration and > it looks like that specific setup won't benefit from the fast bitmap path. > You have pin 2 at position 0 and pin 1 at position 1 of the array. > Hence, the fast bitmap path covers only pin 1, and pin 2 is processed > by the old path with apparently buggy code for skipping over fast pins. > > As a temporary workaround, you could try to revert the order of pins in > your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code > should work for you again by taking the original old path, not skipping > over fast pins. Results of such check may also help us to better > understand and resolve the issue. > > Thanks, > Janusz > >> >>> --- >>> drivers/gpio/gpiolib.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >>> index a53d17745d21..369bdd358fcc 100644 >>> --- a/drivers/gpio/gpiolib.c >>> +++ b/drivers/gpio/gpiolib.c >>> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool >>> can_sleep, >>> __set_bit(hwgpio, mask); >>> >>> if (array_info) >>> - find_next_zero_bit(array_info->get_mask, >>> + i = find_next_zero_bit(array_info->get_mask, >>> array_size, i); >>> else >>> i++; >>> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool >>> can_sleep, >>> trace_gpio_value(desc_to_gpio(desc), 1, value); >>> >>> if (array_info) >>> - find_next_zero_bit(array_info->get_mask, i, j); >>> + j = find_next_zero_bit(array_info->get_mask, i, >>> + j); >>> else >>> j++; >>> } >>> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool >>> can_sleep, >>> } >>> >>> if (array_info) >>> - find_next_zero_bit(array_info->set_mask, >>> + i = find_next_zero_bit(array_info->set_mask, >>> array_size, i); >>> else >>> i++; >> >> Best regards >> -- >> Marek Szyprowski, PhD >> Samsung R&D Institute Poland >> >> >
Hi Janusz, On 2018-09-21 12:51, Janusz Krzysztofik wrote: > 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: >> On 2018-09-20 18:21, Janusz Krzysztofik wrote: >>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote: >>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: >>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote: >>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may >>>>>> contain >>>>>> information on direct mapping of array members to pins of a single >>>>>> GPIO >>>>>> chip in hardware order. In such cases, bitmaps of values can be >>>>>> passed >>>>>> directly from/to the chip's .get/set_multiple() callbacks without >>>>>> wasting time on iterations. >>>>>> >>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions. >>>>>> Pins not applicable for fast path are processed as before, skipping >>>>>> over the 'fast' ones. >>>>>> >>>>>> Cc: Jonathan Corbet <corbet@lwn.net> >>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>>>> I've just noticed that this patch landed in today's linux-next. Sadly >>>>> it >>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, >>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts). >>>>> >>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the >>>>> boot. I will try later to add some debugs and investigate it further >>>>> what >>>>> really happens when booting hangs. >>>> Hi Marek, >>>> >>>> Thanks for reporting. Could you please try the following fix? >>> Hi again, >>> >>> I realized the patch was not correct, j, not i, should be updated in >>> second >>> hunk. Please try the following one. >>> >>> Thanks, >>> Janusz >>> >>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 >>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>> Date: Thu, 20 Sep 2018 17:37:21 +0200 >>> Subject: [PATCH] gpiolib: Fix bitmap index not updated >>> While skipping fast path bits, bitmap index is not updated with next >>> found zero bit position. Fix it. >>> >>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >> This one also doesn't help. A quick compare of logs with this version and >> a working system shows, that with your patch (and fix) there are no calls >> to >> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If >> you need any more information (what kind of logs will help?), let me know. > There is a debug message on array_info content available at the end of > gpiod_get_array(), could you please activate it and post the message so > we can understand better what is going on? With debug enabled on next-20180919: [ 2.499153] pwrseq_simple mmc3_pwrseq: GPIO array info: chip=gpx0, size=2, get_mask=2, set_mask=2, invert_mask=2 On next-20180920 I get no this message and booting hangs. Same with next-20180920 + your second fix from this thread. I will try to debug this more on Monday. > On the other hand, I've had a look your device-tree configuration and > it looks like that specific setup won't benefit from the fast bitmap path. > You have pin 2 at position 0 and pin 1 at position 1 of the array. > Hence, the fast bitmap path covers only pin 1, and pin 2 is processed > by the old path with apparently buggy code for skipping over fast pins. > > As a temporary workaround, you could try to revert the order of pins in > your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code > should work for you again by taking the original old path, not skipping > over fast pins. Results of such check may also help us to better > understand and resolve the issue. Changing the order of mmc pwrseq gpio pins fixes boot hang. Best regards
On Friday, September 21, 2018 4:14:06 PM CEST Marek Szyprowski wrote: > Hi Janusz, > > > On 2018-09-21 12:51, Janusz Krzysztofik wrote: > > 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: > >> On 2018-09-20 18:21, Janusz Krzysztofik wrote: > >>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote: > >>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote: > >>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote: > >>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may > >>>>>> contain > >>>>>> information on direct mapping of array members to pins of a single > >>>>>> GPIO > >>>>>> chip in hardware order. In such cases, bitmaps of values can be > >>>>>> passed > >>>>>> directly from/to the chip's .get/set_multiple() callbacks without > >>>>>> wasting time on iterations. > >>>>>> > >>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions. > >>>>>> Pins not applicable for fast path are processed as before, skipping > >>>>>> over the 'fast' ones. > >>>>>> > >>>>>> Cc: Jonathan Corbet <corbet@lwn.net> > >>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > >>>>> I've just noticed that this patch landed in today's linux-next. Sadly > >>>>> it > >>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit, > >>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts). > >>>>> > >>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the > >>>>> boot. I will try later to add some debugs and investigate it further > >>>>> what > >>>>> really happens when booting hangs. > >>>> Hi Marek, > >>>> > >>>> Thanks for reporting. Could you please try the following fix? > >>> Hi again, > >>> > >>> I realized the patch was not correct, j, not i, should be updated in > >>> second > >>> hunk. Please try the following one. > >>> > >>> Thanks, > >>> Janusz > >>> > >>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001 > >>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com> > >>> Date: Thu, 20 Sep 2018 17:37:21 +0200 > >>> Subject: [PATCH] gpiolib: Fix bitmap index not updated > >>> While skipping fast path bits, bitmap index is not updated with next > >>> found zero bit position. Fix it. > >>> > >>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > >> This one also doesn't help. A quick compare of logs with this version and > >> a working system shows, that with your patch (and fix) there are no calls > >> to > >> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If > >> you need any more information (what kind of logs will help?), let me know. > > There is a debug message on array_info content available at the end of > > gpiod_get_array(), could you please activate it and post the message so > > we can understand better what is going on? > > With debug enabled on next-20180919: > [ 2.499153] pwrseq_simple mmc3_pwrseq: GPIO array info: chip=gpx0, > size=2, get_mask=2, set_mask=2, invert_mask=2 Looks good to me, i..e., in line with what one could expect. However, ... > On next-20180920 I get no this message and booting hangs. > > Same with next-20180920 + your second fix from this thread. > > I will try to debug this more on Monday. > > > On the other hand, I've had a look your device-tree configuration and > > it looks like that specific setup won't benefit from the fast bitmap path. > > You have pin 2 at position 0 and pin 1 at position 1 of the array. > > Hence, the fast bitmap path covers only pin 1, and pin 2 is processed > > by the old path with apparently buggy code for skipping over fast pins. > > > > As a temporary workaround, you could try to revert the order of pins in > > your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code > > should work for you again by taking the original old path, not skipping > > over fast pins. Results of such check may also help us to better > > understand and resolve the issue. > > Changing the order of mmc pwrseq gpio pins fixes boot hang. Not being able to discover more coding bugs in the code modified by the series, I'm wondering if the reason for the issue you are observing comes from the fact both pins are no longer manipulated together within a single .set_multiple() chip callback. I'm working on a fix which prevents from that. Thanks, Janusz
diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst index 2c112553df84..c66821e033c2 100644 --- a/Documentation/driver-api/gpio/board.rst +++ b/Documentation/driver-api/gpio/board.rst @@ -193,3 +193,18 @@ And the table can be added to the board code as follows:: The line will be hogged as soon as the gpiochip is created or - in case the chip was created earlier - when the hog table is registered. + +Arrays of pins +-------------- +In addition to requesting pins belonging to a function one by one, a device may +also request an array of pins assigned to the function. The way those pins are +mapped to the device determines if the array qualifies for fast bitmap +processing. If yes, a bitmap is passed over get/set array functions directly +between a caller and a respective .get/set_multiple() callback of a GPIO chip. + +In order to qualify for fast bitmap processing, the pin mapping must meet the +following requirements: +- it must belong to the same chip as other 'fast' pins of the function, +- its index within the function must match its hardware number within the chip. + +Open drain and open source pins are excluded from fast bitmap output processing. diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst index 0afd95a12b10..cf992e5ab976 100644 --- a/Documentation/driver-api/gpio/consumer.rst +++ b/Documentation/driver-api/gpio/consumer.rst @@ -388,6 +388,14 @@ array_info should be set to NULL. Note that for optimal performance GPIOs belonging to the same chip should be contiguous within the array of descriptors. +Still better performance may be achieved if array indexes of the descriptors +match hardware pin numbers of a single chip. If an array passed to a get/set +array function matches the one obtained from gpiod_get_array() and array_info +associated with the array is also passed, the function may take a fast bitmap +processing path, passing the value_bitmap argument directly to the respective +.get/set_multiple() callback of the chip. That allows for utilization of GPIO +banks as data I/O ports without much loss of performance. + The return value of gpiod_get_array_value() and its variants is 0 on success or negative on error. Note the difference to gpiod_get_value(), which returns 0 or 1 on success to convey the GPIO value. With the array functions, the GPIO diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index cef6ee31fe05..b9d083fb13ee 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2789,7 +2789,36 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, struct gpio_array *array_info, unsigned long *value_bitmap) { - int i = 0; + int err, i = 0; + + /* + * Validate array_info against desc_array and its size. + * It should immediately follow desc_array if both + * have been obtained from the same gpiod_get_array() call. + */ + if (array_info && array_info->desc == desc_array && + array_size <= array_info->size && + (void *)array_info == desc_array + array_info->size) { + if (!can_sleep) + WARN_ON(array_info->chip->can_sleep); + + err = gpio_chip_get_multiple(array_info->chip, + array_info->get_mask, + value_bitmap); + if (err) + return err; + + if (!raw && !bitmap_empty(array_info->invert_mask, array_size)) + bitmap_xor(value_bitmap, value_bitmap, + array_info->invert_mask, array_size); + + if (bitmap_full(array_info->get_mask, array_size)) + return 0; + + i = find_first_zero_bit(array_info->get_mask, array_size); + } else { + array_info = NULL; + } while (i < array_size) { struct gpio_chip *chip = desc_array[i]->gdev->chip; @@ -2820,7 +2849,12 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, int hwgpio = gpio_chip_hwgpio(desc); __set_bit(hwgpio, mask); - i++; + + if (array_info) + find_next_zero_bit(array_info->get_mask, + array_size, i); + else + i++; } while ((i < array_size) && (desc_array[i]->gdev->chip == chip)); @@ -2831,7 +2865,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, return ret; } - for (j = first; j < i; j++) { + for (j = first; j < i; ) { const struct gpio_desc *desc = desc_array[j]; int hwgpio = gpio_chip_hwgpio(desc); int value = test_bit(hwgpio, bits); @@ -2840,6 +2874,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, value = !value; __assign_bit(j, value_bitmap, value); trace_gpio_value(desc_to_gpio(desc), 1, value); + + if (array_info) + find_next_zero_bit(array_info->get_mask, i, j); + else + j++; } if (mask != fastpath) @@ -3041,6 +3080,32 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, { int i = 0; + /* + * Validate array_info against desc_array and its size. + * It should immediately follow desc_array if both + * have been obtained from the same gpiod_get_array() call. + */ + if (array_info && array_info->desc == desc_array && + array_size <= array_info->size && + (void *)array_info == desc_array + array_info->size) { + if (!can_sleep) + WARN_ON(array_info->chip->can_sleep); + + if (!raw && !bitmap_empty(array_info->invert_mask, array_size)) + bitmap_xor(value_bitmap, value_bitmap, + array_info->invert_mask, array_size); + + gpio_chip_set_multiple(array_info->chip, array_info->set_mask, + value_bitmap); + + if (bitmap_full(array_info->set_mask, array_size)) + return 0; + + i = find_first_zero_bit(array_info->set_mask, array_size); + } else { + array_info = NULL; + } + while (i < array_size) { struct gpio_chip *chip = desc_array[i]->gdev->chip; unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]; @@ -3068,7 +3133,14 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, int hwgpio = gpio_chip_hwgpio(desc); int value = test_bit(i, value_bitmap); - if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags)) + /* + * Pins applicable for fast input but not for + * fast output processing may have been already + * inverted inside the fast path, skip them. + */ + if (!raw && !(array_info && + test_bit(i, array_info->invert_mask)) && + test_bit(FLAG_ACTIVE_LOW, &desc->flags)) value = !value; trace_gpio_value(desc_to_gpio(desc), 0, value); /* @@ -3087,7 +3159,12 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, __clear_bit(hwgpio, bits); count++; } - i++; + + if (array_info) + find_next_zero_bit(array_info->set_mask, + array_size, i); + else + i++; } while ((i < array_size) && (desc_array[i]->gdev->chip == chip)); /* push collected bits to outputs */
Certain GPIO descriptor arrays returned by gpio_get_array() may contain information on direct mapping of array members to pins of a single GPIO chip in hardware order. In such cases, bitmaps of values can be passed directly from/to the chip's .get/set_multiple() callbacks without wasting time on iterations. Add respective code to gpiod_get/set_array_bitmap_complex() functions. Pins not applicable for fast path are processed as before, skipping over the 'fast' ones. Cc: Jonathan Corbet <corbet@lwn.net> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- Documentation/driver-api/gpio/board.rst | 15 ++++++ Documentation/driver-api/gpio/consumer.rst | 8 +++ drivers/gpio/gpiolib.c | 87 ++++++++++++++++++++++++++++-- 3 files changed, 105 insertions(+), 5 deletions(-)