| Message ID | cover.1608963094.git.syednwaris@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce the for_each_set_clump macro | expand |
On Sat, Dec 26, 2020 at 7:41 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > Since this patchset primarily affects GPIO drivers, would you like > to pick it up through your GPIO tree? Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons. Yours, Linus Walleij
Hi, On 26. 12. 20 7:41, Syed Nayyar Waris wrote: > Hello Linus, > > Since this patchset primarily affects GPIO drivers, would you like > to pick it up through your GPIO tree? > > (Note: Patchset resent with the new macro and relevant > functions shifted to a new header clump_bits.h [Linus Torvalds]) > > Michal, > What do you think of [PATCH 5/5]? Is the conditional check needed? And > also does returning -EINVAL look good? As was said would be better to handle it out of this series. And I expect none is really describing fpga designs by hand and using DT generator for it. But I can't see any issue with checking that we are not exceeding certain limit. Just keep in your mind that every bank has max 32 lines. It means if you say bank0 40, bank1 10 which is in total 50 it will pass your condition in 5/5. It means maybe checking every bank separately is better approach. Thanks, Michal
On Sun, Dec 27, 2020 at 10:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sat, Dec 26, 2020 at 7:41 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > Since this patchset primarily affects GPIO drivers, would you like > > to pick it up through your GPIO tree? > > Actually Bartosz is handling the GPIO patches for v5.12. > I tried to merge the patch series before but failed for > various reasons. > > Yours, > Linus Walleij My info on this is a bit outdated - didn't Linus Torvalds reject these patches from Andrew Morton's PR? Or am I confusing this series with something else? Bart
On Tue, Jan 05, 2021 at 03:19:13PM +0100, Bartosz Golaszewski wrote: > On Sun, Dec 27, 2020 at 10:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Sat, Dec 26, 2020 at 7:41 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > > > Since this patchset primarily affects GPIO drivers, would you like > > > to pick it up through your GPIO tree? > > > > Actually Bartosz is handling the GPIO patches for v5.12. > > I tried to merge the patch series before but failed for > > various reasons. > My info on this is a bit outdated - didn't Linus Torvalds reject these > patches from Andrew Morton's PR? Or am I confusing this series with > something else? Linus T. told that it can be done inside GPIO realm. This version tries (badly in my opinion) to achieve that.
On Tue, Jan 5, 2021 at 3:38 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Jan 05, 2021 at 03:19:13PM +0100, Bartosz Golaszewski wrote: > > On Sun, Dec 27, 2020 at 10:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Sat, Dec 26, 2020 at 7:41 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > > > > > Since this patchset primarily affects GPIO drivers, would you like > > > > to pick it up through your GPIO tree? > > > > > > Actually Bartosz is handling the GPIO patches for v5.12. > > > I tried to merge the patch series before but failed for > > > various reasons. > > > My info on this is a bit outdated - didn't Linus Torvalds reject these > > patches from Andrew Morton's PR? Or am I confusing this series with > > something else? > > Linus T. told that it can be done inside GPIO realm. This version tries > (badly in my opinion) to achieve that. > I'm seeing William and Arnd have some unaddressed issues with patch 1 (with using __builtin_unreachable()). Admittedly I didn't follow the previous iterations too much so I may miss some history behind it. Why do the first two patches go into lib if this is supposed to be gpiolib-only? Bartosz
On Wed, Jan 06, 2021 at 08:27:43AM +0100, Bartosz Golaszewski wrote: > On Tue, Jan 5, 2021 at 3:38 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Jan 05, 2021 at 03:19:13PM +0100, Bartosz Golaszewski wrote: > > > On Sun, Dec 27, 2020 at 10:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > > > On Sat, Dec 26, 2020 at 7:41 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > > > > > > > Since this patchset primarily affects GPIO drivers, would you like > > > > > to pick it up through your GPIO tree? > > > > > > > > Actually Bartosz is handling the GPIO patches for v5.12. > > > > I tried to merge the patch series before but failed for > > > > various reasons. > > > > > My info on this is a bit outdated - didn't Linus Torvalds reject these > > > patches from Andrew Morton's PR? Or am I confusing this series with > > > something else? > > > > Linus T. told that it can be done inside GPIO realm. This version tries > > (badly in my opinion) to achieve that. > > > > I'm seeing William and Arnd have some unaddressed issues with patch 1 > (with using __builtin_unreachable()). > > Admittedly I didn't follow the previous iterations too much so I may > miss some history behind it. Why do the first two patches go into lib > if this is supposed to be gpiolib-only? > > Bartosz This patchset originally start out as a replacement for bitmap_get_value8/bitmap_set_value8/for_each_set_clump8, which are used outside of the GPIO subsystem. Over the course of the revisions, the scope of this patchset was reduced down and now it's only affecting GPIO drivers. You're right that this shouldn't be going into lib anymore because it's gpiolib-only now. I expect the next revision of this patchset Syed submits will address that. William Breathitt Gray
Hello Linus, Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree? (Note: Patchset resent with the new macro and relevant functions shifted to a new header clump_bits.h [Linus Torvalds]) Michal, What do you think of [PATCH 5/5]? Is the conditional check needed? And also does returning -EINVAL look good? This patchset introduces a new generic version of for_each_set_clump. The previous version of for_each_set_clump8 used a fixed size 8-bit clump, but the new generic version can work with clump of any size but less than or equal to BITS_PER_LONG. The patchset utilizes the new macro in several GPIO drivers. The earlier 8-bit for_each_set_clump8 facilitated a for-loop syntax that iterates over a memory region entire groups of set bits at a time. For example, suppose you would like to iterate over a 32-bit integer 8 bits at a time, skipping over 8-bit groups with no set bit, where XXXXXXXX represents the current 8-bit group: Example: 10111110 00000000 11111111 00110011 First loop: 10111110 00000000 11111111 XXXXXXXX Second loop: 10111110 00000000 XXXXXXXX 00110011 Third loop: XXXXXXXX 00000000 11111111 00110011 Each iteration of the loop returns the next 8-bit group that has at least one set bit. But with the new for_each_set_clump the clump size can be different from 8 bits. Moreover, the clump can be split at word boundary in situations where word size is not multiple of clump size. Following are examples showing the working of new macro for clump sizes of 24 bits and 6 bits. Example 1: clump size: 24 bits, Number of clumps (or ports): 10 bitmap stores the bit information from where successive clumps are retrieved. /* bitmap memory region */ 0x00aa0000ff000000; /* Most significant bits */ 0xaaaaaa0000ff0000; 0x000000aa000000aa; 0xbbbbabcdeffedcba; /* Least significant bits */ Different iterations of for_each_set_clump:- 'offset' is the bit position and 'clump' is the 24 bit clump from the above bitmap. Iteration first: offset: 0 clump: 0xfedcba Iteration second: offset: 24 clump: 0xabcdef Iteration third: offset: 48 clump: 0xaabbbb Iteration fourth: offset: 96 clump: 0xaa Iteration fifth: offset: 144 clump: 0xff Iteration sixth: offset: 168 clump: 0xaaaaaa Iteration seventh: offset: 216 clump: 0xff Loop breaks because in the end the remaining bits (0x00aa) size was less than clump size of 24 bits. In above example it can be seen that in iteration third, the 24 bit clump that was retrieved was split between bitmap[0] and bitmap[1]. This example also shows that 24 bit zeroes if present in between, were skipped (preserving the previous for_each_set_macro8 behaviour). Example 2: clump size = 6 bits, Number of clumps (or ports) = 3. /* bitmap memory region */ 0x00aa0000ff000000; /* Most significant bits */ 0xaaaaaa0000ff0000; 0x0f00000000000000; 0x0000000000000ac0; /* Least significant bits */ Different iterations of for_each_set_clump: 'offset' is the bit position and 'clump' is the 6 bit clump from the above bitmap. Iteration first: offset: 6 clump: 0x2b Loop breaks because 6 * 3 = 18 bits traversed in bitmap. Here 6 * 3 is clump size * no. of clumps. GCC gives warning in bitmap_set_value(): https://godbolt.org/z/rjx34r Add explicit check to see if the value being written into the bitmap does not fall outside the bitmap. The situation that it is falling outside would never be possible in the code because the boundaries are required to be correct before the function is called. The responsibility is on the caller for ensuring the boundaries are correct. The code change is simply to silence the GCC warning messages because GCC is not aware that the boundaries have already been checked. As such, we're better off using __builtin_unreachable() here because we can avoid the latency of the conditional check entirely. Syed Nayyar Waris (5): clump_bits: Introduce the for_each_set_clump macro lib/test_bitmap.c: Add for_each_set_clump test cases gpio: thunderx: Utilize for_each_set_clump macro gpio: xilinx: Utilize generic bitmap_get_value and _set_value gpio: xilinx: Add extra check if sum of widths exceed 64 drivers/gpio/clump_bits.h | 101 ++++++++++++++++++++++++ drivers/gpio/gpio-thunderx.c | 12 ++- drivers/gpio/gpio-xilinx.c | 72 ++++++++++-------- lib/test_bitmap.c | 144 +++++++++++++++++++++++++++++++++++ 4 files changed, 292 insertions(+), 37 deletions(-) create mode 100644 drivers/gpio/clump_bits.h base-commit: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb