On Wed, 13 Sep 2023 08:53:55 +0200
Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
> On 11/9/23 13:43, Jonathan Cameron wrote:
> > In order to avoid having the size of the per HDM decoder register block
> > repeated in lots of places, create the register definitions for HDM
> > decoder 1 and use the offset between the first registers in HDM decoder 0
> > and
> > HDM decoder 1 to establish the offset.
> >
> > Calculate in each function as this is more obvious and leads to shorter
> > line lengths than a single #define which would need a long name
> > to be specific enough.
> >
> > Note that the code currently only supports one decoder, so the bugs this
> > fixes don't actually affect anything. Previously the offset didn't
> > take into account that the write_msk etc are 4 byte fields.
> >
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> >
> > --
> > v3:
> > New patch to separate this out from the addition of HDM decoders.
> > ---
> > include/hw/cxl/cxl_component.h | 2 ++
> > hw/cxl/cxl-component-utils.c | 19 +++++++++++--------
> > hw/cxl/cxl-host.c | 4 +++-
> > hw/mem/cxl_type3.c | 24 +++++++++++++++---------
> > 4 files changed, 31 insertions(+), 18 deletions(-)
>
>
> > @@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
> > /* TODO: Support multiple HDM decoders and DPA skip */
> > static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t
> > *dpa)
> > {
> > + int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> > uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> > uint64_t decoder_base, decoder_size, hpa_offset;
> > uint32_t hdm0_ctrl;
> > int ig, iw;
> > + int i = 0;
> >
> > - decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] <<
> > 32) |
> > - cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
> > + decoder_base =
> > + (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] <<
> > 32) |
> > + cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
>
> Alternatively easier to review as (matter of taste ?):
>
> decoder_base = deposit64(cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i *
> hdm_inc], 32, 32,
> cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i *
> hdm_inc]);
I'll leave if for now for consistency in the CXL code. Might make
sense to consider this as a cross subsystem cleanup at some point though!
Thanks for the suggestion.
>
> Regardless:
>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Thanks.
Jonathan
>
>
