mbox series

[0/5] Introduce the for_each_set_clump macro

Message ID cover.1608963094.git.syednwaris@gmail.com (mailing list archive)
Headers show
Series Introduce the for_each_set_clump macro | expand

Message

Syed Nayyar Waris Dec. 26, 2020, 6:41 a.m. UTC
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

Comments

Linus Walleij Dec. 27, 2020, 9:26 p.m. UTC | #1
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
Michal Simek Jan. 5, 2021, 11:09 a.m. UTC | #2
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
Bartosz Golaszewski Jan. 5, 2021, 2:19 p.m. UTC | #3
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
Andy Shevchenko Jan. 5, 2021, 2:39 p.m. UTC | #4
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.
Bartosz Golaszewski Jan. 6, 2021, 7:27 a.m. UTC | #5
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
William Breathitt Gray Jan. 6, 2021, 8:19 a.m. UTC | #6
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