| Message ID | 20180902120144.6855-5-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpiolib: speed up GPIO array processing | 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
While investigating possible reasons of GPIO fast bitmap processing
related boot hang on Samsung Snow Chromebook, reported by Marek
Szyprowski (thanks!), I've discovered one coding bug, addressed by
PATCH 1/2 of this series, and one potential regression introduced at
design level of the solution, hopefully fixed by PATCH 2/2. See
commit messages for details.
Janusz Krzysztofik (2):
gpiolib: Fix missing updates of bitmap index
gpiolib: Fix array members of same chip processed separately
The fixes should resolve the boot hang observed by Marek, however the
second change excludes that particular case from fast bitmap processing
and restores the old behaviour. Hence, it is possible still another
issue which have had an influence on that boot hang exists in the code.
In order to fully verify the fix, it would have to be tested on a
platform where an array of GPIO descriptors is used which starts from
at least two consecutive pins of one GPIO chip in hardware order,
starting ftom 0, followed by one or more pins belonging to other
chip(s).
In order to verify if separate calls to .set() chip callback for each
pin instead of one call to .set_multiple() is actually the reason of
boot hang on Samsung Snow Chromebook, the affected driver -
drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
modified for testing purposes so it calls gpiod_set_value() for each
pin instead of gpiod_set_array_value() for all of them. If that would
also result in boot hang, we could be sure the issue was really the
one addressed by the second fix. Marek, could you please try to
perform such test?
Thanks,
Janusz
diffstat:
Documentation/driver-api/gpio/board.rst | 19 +++++++++----
drivers/gpio/gpiolib.c | 46 +++++++++++++++++++++-----------
2 files changed, 45 insertions(+), 20 deletions(-)
Hi Janusz, On 2018-09-24 01:53, Janusz Krzysztofik wrote: > While investigating possible reasons of GPIO fast bitmap processing > related boot hang on Samsung Snow Chromebook, reported by Marek > Szyprowski (thanks!), I've discovered one coding bug, addressed by > PATCH 1/2 of this series, and one potential regression introduced at > design level of the solution, hopefully fixed by PATCH 2/2. See > commit messages for details. > > Janusz Krzysztofik (2): > gpiolib: Fix missing updates of bitmap index > gpiolib: Fix array members of same chip processed separately > > The fixes should resolve the boot hang observed by Marek, however the > second change excludes that particular case from fast bitmap processing > and restores the old behaviour. I confirm, that the above 2 patches fixes boot issue on Samsung Snow Chromebook with next-20180920. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Hence, it is possible still another > issue which have had an influence on that boot hang exists in the code. > In order to fully verify the fix, it would have to be tested on a > platform where an array of GPIO descriptors is used which starts from > at least two consecutive pins of one GPIO chip in hardware order, > starting ftom 0, followed by one or more pins belonging to other > chip(s). > > In order to verify if separate calls to .set() chip callback for each > pin instead of one call to .set_multiple() is actually the reason of > boot hang on Samsung Snow Chromebook, the affected driver - > drivers/mmc/core/pwrseq_simple.c - would have to be temporarily > modified for testing purposes so it calls gpiod_set_value() for each > pin instead of gpiod_set_array_value() for all of them. If that would > also result in boot hang, we could be sure the issue was really the > one addressed by the second fix. Marek, could you please try to > perform such test? Yes, I've just tested next-20180920 only with the first patch from this patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c. It boots fine, so indeed the issue is in handling of arrays of gpios. Just to be sure I did it right, this is my change to the mentioned file: diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c index 7f882a2bb872..9397dc1f2e38 100644 --- a/drivers/mmc/core/pwrseq_simple.c +++ b/drivers/mmc/core/pwrseq_simple.c @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq, int value) { struct gpio_descs *reset_gpios = pwrseq->reset_gpios; + int i; - if (!IS_ERR(reset_gpios)) { - DECLARE_BITMAP(values, BITS_PER_TYPE(value)); - int nvalues = reset_gpios->ndescs; - - values[0] = value; - - gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc, - reset_gpios->info, values); - } + if (!IS_ERR(reset_gpios)) + for (i = 0; i < reset_gpios->ndescs; i++) + gpiod_set_value_cansleep(reset_gpios->desc[i], value); } static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) Best regards
Hi Marek, 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: > Hi Janusz, > > On 2018-09-24 01:53, Janusz Krzysztofik wrote: >> While investigating possible reasons of GPIO fast bitmap processing >> related boot hang on Samsung Snow Chromebook, reported by Marek >> Szyprowski (thanks!), I've discovered one coding bug, addressed by >> PATCH 1/2 of this series, and one potential regression introduced at >> design level of the solution, hopefully fixed by PATCH 2/2. See >> commit messages for details. >> >> Janusz Krzysztofik (2): >> gpiolib: Fix missing updates of bitmap index >> gpiolib: Fix array members of same chip processed separately >> >> The fixes should resolve the boot hang observed by Marek, however the >> second change excludes that particular case from fast bitmap processing >> and restores the old behaviour. > > I confirm, that the above 2 patches fixes boot issue on Samsung Snow > Chromebook with next-20180920. > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> Hence, it is possible still another >> issue which have had an influence on that boot hang exists in the code. >> In order to fully verify the fix, it would have to be tested on a >> platform where an array of GPIO descriptors is used which starts from >> at least two consecutive pins of one GPIO chip in hardware order, >> starting ftom 0, followed by one or more pins belonging to other >> chip(s). >> >> In order to verify if separate calls to .set() chip callback for each >> pin instead of one call to .set_multiple() is actually the reason of >> boot hang on Samsung Snow Chromebook, the affected driver - >> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily >> modified for testing purposes so it calls gpiod_set_value() for each >> pin instead of gpiod_set_array_value() for all of them. If that would >> also result in boot hang, we could be sure the issue was really the >> one addressed by the second fix. Marek, could you please try to >> perform such test? > > Yes, I've just tested next-20180920 only with the first patch from this > patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c. > It boots fine, so indeed the issue is in handling of arrays of gpios. > > Just to be sure I did it right, this is my change to the mentioned file: Yeah, that's what I had on mind. However, I'd be more lucky if it didn't work for you. Setting the pins sequentially, not simultaneously as before, was exactly what I hoped was the reason of the hang. > diff --git a/drivers/mmc/core/pwrseq_simple.c > b/drivers/mmc/core/pwrseq_simple.c > index 7f882a2bb872..9397dc1f2e38 100644 > --- a/drivers/mmc/core/pwrseq_simple.c > +++ b/drivers/mmc/core/pwrseq_simple.c > @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct > mmc_pwrseq_simple *pwrseq, > int value) > { > struct gpio_descs *reset_gpios = pwrseq->reset_gpios; > + int i; > > - if (!IS_ERR(reset_gpios)) { > - DECLARE_BITMAP(values, BITS_PER_TYPE(value)); > - int nvalues = reset_gpios->ndescs; > - > - values[0] = value; > - > - gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc, > - reset_gpios->info, values); > - } > + if (!IS_ERR(reset_gpios)) > + for (i = 0; i < reset_gpios->ndescs; i++) The only difference from the behaviour when the hang was occurring is now the order the pins are manipulated. Maybe that matters? Could you please retry the same with the order of pins reversed, either in the .dts file or here inside this for loop? Thanks, Janusz > + gpiod_set_value_cansleep(reset_gpios->desc[i], value); > } > > static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) > > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > >
Hi Janusz, On 2018-09-24 13:08, Janusz Krzysztofik wrote: > 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: >> On 2018-09-24 01:53, Janusz Krzysztofik wrote: >>> While investigating possible reasons of GPIO fast bitmap processing >>> related boot hang on Samsung Snow Chromebook, reported by Marek >>> Szyprowski (thanks!), I've discovered one coding bug, addressed by >>> PATCH 1/2 of this series, and one potential regression introduced at >>> design level of the solution, hopefully fixed by PATCH 2/2. See >>> commit messages for details. >>> >>> Janusz Krzysztofik (2): >>> gpiolib: Fix missing updates of bitmap index >>> gpiolib: Fix array members of same chip processed separately >>> >>> The fixes should resolve the boot hang observed by Marek, however the >>> second change excludes that particular case from fast bitmap processing >>> and restores the old behaviour. >> I confirm, that the above 2 patches fixes boot issue on Samsung Snow >> Chromebook with next-20180920. >> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >>> Hence, it is possible still another >>> issue which have had an influence on that boot hang exists in the code. >>> In order to fully verify the fix, it would have to be tested on a >>> platform where an array of GPIO descriptors is used which starts from >>> at least two consecutive pins of one GPIO chip in hardware order, >>> starting ftom 0, followed by one or more pins belonging to other >>> chip(s). >>> >>> In order to verify if separate calls to .set() chip callback for each >>> pin instead of one call to .set_multiple() is actually the reason of >>> boot hang on Samsung Snow Chromebook, the affected driver - >>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily >>> modified for testing purposes so it calls gpiod_set_value() for each >>> pin instead of gpiod_set_array_value() for all of them. If that would >>> also result in boot hang, we could be sure the issue was really the >>> one addressed by the second fix. Marek, could you please try to >>> perform such test? >> Yes, I've just tested next-20180920 only with the first patch from this >> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c. >> It boots fine, so indeed the issue is in handling of arrays of gpios. >> >> Just to be sure I did it right, this is my change to the mentioned file: > Yeah, that's what I had on mind. However, I'd be more lucky if it didn't work > for you. Setting the pins sequentially, not simultaneously as before, was > exactly what I hoped was the reason of the hang. > >> diff --git a/drivers/mmc/core/pwrseq_simple.c >> b/drivers/mmc/core/pwrseq_simple.c >> index 7f882a2bb872..9397dc1f2e38 100644 >> --- a/drivers/mmc/core/pwrseq_simple.c >> +++ b/drivers/mmc/core/pwrseq_simple.c >> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct >> mmc_pwrseq_simple *pwrseq, >> int value) >> { >> struct gpio_descs *reset_gpios = pwrseq->reset_gpios; >> + int i; >> >> - if (!IS_ERR(reset_gpios)) { >> - DECLARE_BITMAP(values, BITS_PER_TYPE(value)); >> - int nvalues = reset_gpios->ndescs; >> - >> - values[0] = value; >> - >> - gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc, >> - reset_gpios->info, values); >> - } >> + if (!IS_ERR(reset_gpios)) >> + for (i = 0; i < reset_gpios->ndescs; i++) > The only difference from the behaviour when the hang was occurring is now > the order the pins are manipulated. Maybe that matters? > Could you please retry the same with the order of pins reversed, either in > the .dts file or here inside this for loop? I've switched the order of pins in dts and next-20180920 + first patch + above change also boots fine. Best regards
Hi Marek, 2018-09-24 13:38 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: > Hi Janusz, > > On 2018-09-24 13:08, Janusz Krzysztofik wrote: >> 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>: >>> On 2018-09-24 01:53, Janusz Krzysztofik wrote: >>>> While investigating possible reasons of GPIO fast bitmap processing >>>> related boot hang on Samsung Snow Chromebook, reported by Marek >>>> Szyprowski (thanks!), I've discovered one coding bug, addressed by >>>> PATCH 1/2 of this series, and one potential regression introduced at >>>> design level of the solution, hopefully fixed by PATCH 2/2. See >>>> commit messages for details. >>>> >>>> Janusz Krzysztofik (2): >>>> gpiolib: Fix missing updates of bitmap index >>>> gpiolib: Fix array members of same chip processed separately >>>> >>>> The fixes should resolve the boot hang observed by Marek, however the >>>> second change excludes that particular case from fast bitmap processing >>>> and restores the old behaviour. >>> I confirm, that the above 2 patches fixes boot issue on Samsung Snow >>> Chromebook with next-20180920. >>> >>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> >>>> Hence, it is possible still another >>>> issue which have had an influence on that boot hang exists in the code. >>>> In order to fully verify the fix, it would have to be tested on a >>>> platform where an array of GPIO descriptors is used which starts from >>>> at least two consecutive pins of one GPIO chip in hardware order, >>>> starting ftom 0, followed by one or more pins belonging to other >>>> chip(s). >>>> >>>> In order to verify if separate calls to .set() chip callback for each >>>> pin instead of one call to .set_multiple() is actually the reason of >>>> boot hang on Samsung Snow Chromebook, the affected driver - >>>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily >>>> modified for testing purposes so it calls gpiod_set_value() for each >>>> pin instead of gpiod_set_array_value() for all of them. If that would >>>> also result in boot hang, we could be sure the issue was really the >>>> one addressed by the second fix. Marek, could you please try to >>>> perform such test? >>> Yes, I've just tested next-20180920 only with the first patch from this >>> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c. >>> It boots fine, so indeed the issue is in handling of arrays of gpios. >>> >>> Just to be sure I did it right, this is my change to the mentioned file: >> Yeah, that's what I had on mind. However, I'd be more lucky if it didn't >> work >> for you. Setting the pins sequentially, not simultaneously as before, >> was >> exactly what I hoped was the reason of the hang. >> >>> diff --git a/drivers/mmc/core/pwrseq_simple.c >>> b/drivers/mmc/core/pwrseq_simple.c >>> index 7f882a2bb872..9397dc1f2e38 100644 >>> --- a/drivers/mmc/core/pwrseq_simple.c >>> +++ b/drivers/mmc/core/pwrseq_simple.c >>> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct >>> mmc_pwrseq_simple *pwrseq, >>> int value) >>> { >>> struct gpio_descs *reset_gpios = pwrseq->reset_gpios; >>> + int i; >>> >>> - if (!IS_ERR(reset_gpios)) { >>> - DECLARE_BITMAP(values, BITS_PER_TYPE(value)); >>> - int nvalues = reset_gpios->ndescs; >>> - >>> - values[0] = value; >>> - >>> - gpiod_set_array_value_cansleep(nvalues, >>> reset_gpios->desc, >>> - reset_gpios->info, >>> values); >>> - } >>> + if (!IS_ERR(reset_gpios)) >>> + for (i = 0; i < reset_gpios->ndescs; i++) >> The only difference from the behaviour when the hang was occurring is now >> the order the pins are manipulated. Maybe that matters? >> Could you please retry the same with the order of pins reversed, either >> in >> the .dts file or here inside this for loop? > > I've switched the order of pins in dts and next-20180920 + first patch + > above > change also boots fine. Thanks for performing those tests. Since we are not able to reproduce the issue by any means other than using the original code introduced by fast bitmap processing changes, regardless of the first fix being applied or not, and we are only able to resolve the hangup by excluding affected use case from the fast path, we have to assume one or more bugs which affect mixed arrays, i.e., those which apply for fast bitmap processing only in part, may still exist in the code introduced by the fast bitmap processing series. I hope we are able to resolve it soon, before the changes reach mainline. Thanks, Janusz > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > >
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(-)