Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

Hi Marek,

2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>:
> 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@xxxxxxx>
>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx>
>>>> 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@xxxxxxxxx>
>> 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@xxxxxxxxx>
>
> 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
>
>