Re: [PATCH v3 2/4] hw/cxl: Add utility functions decoder interleave ways and target count.

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 12 Sep 2023 17:20:05 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Mon, Sep 11, 2023 at 12:43:11PM +0100, Jonathan Cameron wrote:
> 
> > As an encoded version of these key configuration parameters is available
> > in a register, provide functions to extract it again so as to avoid
> > the need for duplicating the storage.
> > 
> > Whilst here update the _enc() function to include additional values
> > as defined in the CXL 3.0 specification. Whilst they are not
> > currently used in the emulation, they may be in future and it is
> > easier to compare with the specification if all values are covered.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---  
> 
> LGTM. Only one minor comment inline.
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
Thanks!

> > +
> > +int cxl_decoder_count_dec(int enc_cnt)
> > +{
> > +    switch (enc_cnt) {
> > +    case 0x0: return 1;
> > +    case 0x1: return 2;
> > +    case 0x2: return 4;
> > +    case 0x3: return 6;
> > +    case 0x4: return 8;
> > +    case 0x5: return 10;
> > +    /* Switches and Host Bridges may have more than 10 decoders */
> > +    case 0x6: return 12;
> > +    case 0x7: return 14;
> > +    case 0x8: return 16;
> > +    case 0x9: return 20;
> > +    case 0xa: return 24;
> > +    case 0xb: return 28;
> > +    case 0xc: return 32;
> >      }
> >      return 0;
> >  }
> > @@ -410,6 +440,23 @@ uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
> >      }
> >  }
> >    
> 
> Similar as decoder count dec/enc, maybe we want to add a line of comment below.
> /* CXL r3.0 Section 8.2.4.19.7 CXL HDM Decoder n Control Register */

I'll do it before cxl_interleave_ways_enc() - one function up in the file
as applies equally well there.
> 
> Fan
> > +int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp)
> > +{
> > +    switch (iw_enc) {
> > +    case 0x0: return 1;
> > +    case 0x1: return 2;
> > +    case 0x2: return 4;
> > +    case 0x3: return 8;
> > +    case 0x4: return 16;
> > +    case 0x8: return 3;
> > +    case 0x9: return 6;
> > +    case 0xa: return 12;
> > +    default:
> > +        error_setg(errp, "Encoded interleave ways: %d not supported", iw_enc);
> > +        return 0;
> > +    }
> > +}
> > +
> >  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp)
> >  {
> >      switch (gran) {
> > -- 
> > 2.39.2
> > 
> >