On Tue, 5 Sep 2023 18:55:23 +0200 Philippe Mathieu-Daudé <[email protected]> wrote:
> On 5/9/23 17:06, Jonathan Cameron wrote: > > On Tue, 5 Sep 2023 15:56:39 +0100 > > Jonathan Cameron via <[email protected]> wrote: > > > >> On Mon, 4 Sep 2023 20:26:59 +0200 > >> Philippe Mathieu-Daudé <[email protected]> wrote: > >> > >>> On 4/9/23 18:47, Jonathan Cameron wrote: > >>>> As an encoded version of these key configuration parameters is > >>>> a register, provide functions to extract it again so as to avoid > >>>> the need for duplicating the storage. > >>>> > >>>> Signed-off-by: Jonathan Cameron <[email protected]> > >>>> --- > >>>> include/hw/cxl/cxl_component.h | 14 ++++++++++++++ > >>>> hw/cxl/cxl-component-utils.c | 17 +++++++++++++++++ > >>>> 2 files changed, 31 insertions(+) > >>>> > >>>> diff --git a/include/hw/cxl/cxl_component.h > >>>> b/include/hw/cxl/cxl_component.h > >>>> index 42c7e581a7..f0ad9cf7de 100644 > >>>> --- a/include/hw/cxl/cxl_component.h > >>>> +++ b/include/hw/cxl/cxl_component.h > >>>> @@ -238,7 +238,21 @@ static inline int cxl_decoder_count_enc(int count) > >>>> return 0; > >>>> } > >>>> > >>>> +static inline int cxl_decoder_count_dec(int enc_cnt) > >>>> +{ > >>>> + switch (enc_cnt) { > >>>> + case 0: return 1; > >>>> + case 1: return 2; > >>>> + case 2: return 4; > >>>> + case 3: return 6; > >>>> + case 4: return 8; > >>>> + case 5: return 10; > >>>> + } > >>>> + return 0; > >>>> +} > >>> > >>> Why inline? > >>> > >> > >> Bad habit. > > Nope. I'm being slow. This is in a header so if I don't > > mark it inline I get a bunch of defined but not used warnings. > > > > Obviously I could move the implementation of this and the matching > > encoding routines out of the header. I haven't done so for now. > > Inlined function in hw/ are hardly justifiable. They make the headers > and debugging sessions harder to read in my experience. Compilers are > becoming clever and clever, and we have LTO, so I rather privilege > code maintainability. My 2 cents :) > > >>> Alternatively: > >>> > >>> unsigned cxl_decoder_count_dec(unsigned enc_cnt) > >>> { > >>> return enc_cnt <= 5 ? 2 * enc_cnt : 0; > >> > >> It gets a little more fiddly than the code I'm proposing implies. > >> For Switches and Host Bridges larger values are defined > >> (we just don't emulate them yet and may never do so) and those > >> don't have a sensible mapping. > >> > >> I guess there is no harm in adding the full decode however > >> which will make it more obvious why it was a switch statement. > > Right, no problem. > > Preferably having this tiny function not inlined I'll push this and the enc() version down into the cxl-component-utils.c as a precursor patch. > > Reviewed-by: Philippe Mathieu-Daudé <[email protected]> > > Thanks, but the changes to do make these non inline, and include the larger decode and encode values are big enough I won't pick up the RB - too much changing (that I might mess up ;) Jonathan
