Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config

[Paul Cercueil <paul@crapouillou.net>]

Le mer., mars 16 2022 at 18:20:27 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Mar 16, 2022 at 6:03 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil
>>  >> <paul@crapouillou.net>
>>  >>  > 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)
> 
> ?

Well, for starters you're comparing a bit position [0:31] with a bit 
mask (1 << [0:31])
;)

I believe you meant:
ffs(enabled_socs) >= version

With my former example, the first case (ID_JZ4740) would give you a 
compile-time constant of 1, so it works.

The second case (ID_JZ4755) would cause the jzpc->info->version to be 
checked, which works too.

The third case (ID_JZ4770) however, would also cause the 
jzpc->info->version to be checked, which is not the wanted behaviour. 
Since in my example we only support up to JZ4760, this should instead 
resolve to a compile-time constant 0.

Cheers,
-Paul