On Wed, 24 Jan 2024 15:23:16 +0000
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Tue, 7 Nov 2023 10:07:08 -0800
> nifan....@gmail.com wrote:
>
> > From: Fan Ni <fan...@samsung.com>
> >
> > With the change, when setting up memory for type3 memory device, we can
> > create DC regions.
> > A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> > number of DC regions to create. To make it easier, other region parameters
> > like region base, length, and block size are hard coded. If needed,
> > these parameters can be added easily.
> >
> > With the change, we can create DC regions with proper kernel side
> > support as below:
> >
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> > echo $region> /sys/bus/cxl/devices/decoder0.0/create_dc_region
> > echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> > echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
> >
> > echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> > echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
> >
> > echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> > echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> > echo 1 > /sys/bus/cxl/devices/$region/commit
> > echo $region > /sys/bus/cxl/drivers/cxl_region/bind
> >
> > Signed-off-by: Fan Ni <fan...@samsung.com>
> Hi Fan, a few comments inline.
>
> Jonathan
>
> > ---
> > hw/mem/cxl_type3.c | 35 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 754c885cd1..2d67d2015c 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -721,6 +721,36 @@ static void ct3d_reg_write(void *opaque, hwaddr
> > offset, uint64_t value,
> > }
> > }
> >
> > +static int cxl_create_dc_regions(CXLType3Dev *ct3d)
> > +{
> > + int i;
> > + uint64_t region_base = 0;
> > + uint64_t region_len = 2 * GiB;
> > + uint64_t decode_len = 8; /* 8*256MB */
>
> If decode len is going to be div 256MiB then we need
> a name for that field that makes it clear that it is.
>
> decode_len_256mbytes or something like that and maybe
> region_len_bytes to keep things consistent.
>
> Why the spec didn't make our life easier and define decode length
> in bytes with some bits that must be zero is beyond me...
>
>
> I think we need to make this at least optionally configurable or based
> in some fashion on the provided memory backend (divide that up
> by number of regions with appropriate rounding perhaps?)
This seems to be a mid patch set confusion.. It's fixed in patch 7.
Whilst applying I've made this 2GiB here.
Jonathan
