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

[Fan Ni]

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>


> v3: No changes, picked up tags.
> v2: Thanks to Philippe Mathieu-Daudé
>  - Expand both enc() and dec() functions to include full set of values
>    defined in CXL r3.0
>  - Pushed implementation down into the .c file.
> ---
>  include/hw/cxl/cxl_component.h |  2 ++
>  hw/cxl/cxl-component-utils.c   | 59 ++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index bdb3881a6b..ef9e033919 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -226,8 +226,10 @@ void cxl_component_create_dvsec(CXLComponentState 
> *cxl_cstate,
>                                  uint16_t type, uint8_t rev, uint8_t *body);
>  
>  int cxl_decoder_count_enc(int count);
> +int cxl_decoder_count_dec(int enc_cnt);
>  
>  uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
> +int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp);
>  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
>  
>  hwaddr cxl_decode_ig(int ig);
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index ea2d4770ec..352d0dace2 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -13,15 +13,45 @@
>  #include "hw/pci/pci.h"
>  #include "hw/cxl/cxl.h"
>  
> +/* CXL r3.0 Section 8.2.4.19.1 CXL HDM Decoder Capability Register */
>  int cxl_decoder_count_enc(int count)
>  {
>      switch (count) {
> -    case 1: return 0;
> -    case 2: return 1;
> -    case 4: return 2;
> -    case 6: return 3;
> -    case 8: return 4;
> -    case 10: return 5;
> +    case 1: return 0x0;
> +    case 2: return 0x1;
> +    case 4: return 0x2;
> +    case 6: return 0x3;
> +    case 8: return 0x4;
> +    case 10: return 0x5;
> +    /* Switches and Host Bridges may have more than 10 decoders */
> +    case 12: return 0x6;
> +    case 14: return 0x7;
> +    case 16: return 0x8;
> +    case 20: return 0x9;
> +    case 24: return 0xa;
> +    case 28: return 0xb;
> +    case 32: return 0xc;
> +    }
> +    return 0;
> +}
> +
> +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 */

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
> 
>