[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] hw/cxl: Add utility functions decoder interleave ways an
From: |
Jonathan Cameron |
Subject: |
Re: [PATCH 1/2] hw/cxl: Add utility functions decoder interleave ways and target count. |
Date: |
Thu, 7 Sep 2023 12:27:28 +0100 |
On Tue, 5 Sep 2023 18:55:23 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 5/9/23 17:06, Jonathan Cameron wrote:
> > On Tue, 5 Sep 2023 15:56:39 +0100
> > Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
> >
> >> On Mon, 4 Sep 2023 20:26:59 +0200
> >> Philippe Mathieu-Daudé <philmd@linaro.org> 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 <Jonathan.Cameron@huawei.com>
> >>>> ---
> >>>> 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é <philmd@linaro.org>
>
>
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
[PATCH 2/2] hw/cxl: Support 4 HDM decoders at all levels of topology, Jonathan Cameron, 2023/09/04