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:
>> >> + 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.
