Re: [PATCH 2/2] hw/cxl: Support 4 HDM decoders at all levels of topology

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

On Mon, 4 Sep 2023 20:36:02 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Jonathan,
> 
> Few style comments inlined.
> 
> On 4/9/23 18:47, Jonathan Cameron wrote:
> > Support these decoders in CXL host bridges (pxb-cxl), CXL Switch USP
> > and CXL Type 3 end points.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
Hi Philippe,

Thanks for the particularly quick reviews! 

...

> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index e96398e8af..79b9369756 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -42,6 +42,9 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
> >   
> >       switch (offset) {
> >       case A_CXL_HDM_DECODER0_CTRL:
> > +    case A_CXL_HDM_DECODER1_CTRL:
> > +    case A_CXL_HDM_DECODER2_CTRL:
> > +    case A_CXL_HDM_DECODER3_CTRL:
> >           should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> >           should_uncommit = !should_commit;
> >           break;
> > @@ -81,7 +84,7 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> >       }
> >   
> >       if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > -        offset <= A_CXL_HDM_DECODER0_TARGET_LIST_HI) {
> > +        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> >           dumb_hdm_handler(cxl_cstate, offset, value);
> >       } else {
> >           cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > @@ -161,7 +164,7 @@ static void ras_init_common(uint32_t *reg_state, uint32_t *write_msk)
> >   static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> >                               enum reg_type type)
> >   {
> > -    int decoder_count = 1;
> > +    int decoder_count = 4;  
> 
>    unsigned decoder_count = HDM_DECODER_COUNT;
> 
> >       int i;
> >   
> >       ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, DECODER_COUNT,
> > @@ -174,19 +177,22 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> >                        HDM_DECODER_ENABLE, 0);
> >       write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
> >       for (i = 0; i < decoder_count; i++) {  
> 
> Alternatively:
> 
>          for (i = 0; i < decoder_count; i++, write_msk += 8) {
>              write_msk[R_CXL_HDM_DECODER0_BASE_LO] = 0xf0000000;

That's a bit nasty and fragile given we are offsetting the base register than
indexing into it (so applying a later offset).

> 
> > -        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * 0x20] = 0xf0000000;
> > -        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * 0x20] = 0xffffffff;
> > -        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf0000000;
> > -        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0xffffffff;
> > -        write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > +        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * 0x20 / 4] = 0xf0000000;  
> 
> (this 0x20 / 4 bugs me a bit).

Instead, I've gone with a local variable which leaves me room for deriving
this based on the step between the registers for decoders 0 and 1.

hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;

I haven't added a define for this because it would probably have to be
long enough that it will cause line length problems :(
So it is replicated in a few different places which isn't ideal
but definitely better than the 0x20 / 4

> 
> > +        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * 0x20 / 4]  = 0xffffffff;
> > +        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20 / 4] = 0xf0000000;
> > +        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20 / 4] = 0xffffffff;
> > +        write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20 / 4] = 0x13ff;
> >           if (type == CXL2_DEVICE ||
> >               type == CXL2_TYPE3_DEVICE ||
> >               type == CXL2_LOGICAL_DEVICE) {
> > -            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xf0000000;
> > +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20 / 4] =
> > +                0xf0000000;
> >           } else {
> > -            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xffffffff;
> > +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20 / 4] =
> > +                0xffffffff;
> >           }
> > -        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0xffffffff;
> > +        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20 / 4] =
> > +            0xffffffff;
> >       }
> >   }  
>