On Wed, Mar 16, 2022 at 6:03 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote: > Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko > <andy.shevchenko@xxxxxxxxx> a écrit : > > On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx> > > wrote: > >> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko > >> <andy.shevchenko@xxxxxxxxx> a écrit : > >> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil > >> <paul@xxxxxxxxxxxxxxx> > >> > wrote: ... > > It was given below, i.e. using test_bit(). > > Again, I am not checking a bit, but a mask. Yes, that's why "was". ... > >> >> + return (enabled_socs >> version) && > >> >> + (!(enabled_socs & GENMASK((unsigned int)version > >> - > >> >> 1, 0)) > >> > > >> > Why casting? Why not use BIT()? > >> > >> Casting just to be explicit about what we're doing here - I don't > >> like > >> doing arithmetic on enums. But I can certainly remove it. > >> > >> > But these two lines seem like a very interesting way to reinvent > >> the > >> > test_bit(). > >> > >> I don't use BIT() or test_bit() because I am not checking a bit, > >> but a > >> mask: > >> - (enabled_socs >> version) checks that the config supports either > >> (version) or anything above; > >> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the config > >> does > >> not support anything below. If true, the actual version check can be > >> skipped and the operation is a compile-time constant, and GCC will > >> trim > >> the dead branches accordingly. > > > > You can still simplify the code, no? Calling ffs() (or similar, I > > don't remember by heart all of the details) will give you the result > > in one op. And it may also be optimized away by the compiler. > > ffs() gives me the first bit set, this is absolutely not what I want. > > Let's say that my kernel supports the JZ4750, JZ4755, JZ4760 SoCs. > That means that I have enabled_socs == 0x38. > > Calling is_soc_or_above(jzpc, ID_JZ4740) would resolve to: > (0x38 >> 1) && (!(0x38 & GENMASK(0, 0)) || jzpc->info->version >= > version); > == 1 && (1 || jzpc->info->version >= version) > > Which is a compile-time constant equal to 1. > > Calling is_soc_or_above(jzpc, ID_JZ4755) would resolve to: > (0x38 >> 4) && (!(0x38 & GENMASK(3, 0)) || jzpc->info->version >= > version); > == 1 && (0 || jzpc->info->version >= version) > > which is not a compile-time constant, so the jzpc->info->version must > be checked. > > Calling is_soc_or_above(jzpc, ID_JZ4770) would resolve to: > (0x38 >> 6) && (!(0x38 & GENMASK(5, 0)) || jzpc->info->version >= > version); > == 0 && ... > > which is a compile-time constant equal to 0. And what's wrong with ffs(enabled_socs) >= BIT(version) ? -- With Best Regards, Andy Shevchenko
