On Mon, 4 Sep 2023 20:36:02 +0200
Philippe Mathieu-Daudé <phi...@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.came...@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;
> > }
> > }
>
