| Message ID | 20230426-cxl-fixes-v1-3-870c4c8b463a@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Random clean ups | expand |
On 5/17/23 2:28 PM, Ira Weiny wrote: > The devices created, their relationship, and intended testing purpose is > not extremely clear, especially for those unfamiliar with cxl-test. > > Document the purpose of each hierarchy. Add ASCII art to show the > relationship of devices. Group the device declarations together based > on the hierarchies. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Nice artwork :) Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 73 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > index bf00dc52fe96..bd38a5fb60ae 100644 > --- a/tools/testing/cxl/test/cxl.c > +++ b/tools/testing/cxl/test/cxl.c > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > #define NR_CXL_PORT_DECODERS 8 > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > +/* > + * Interleave testing > + * > + * +---------------+ +---------------+ > + * | host_bridge[0]| | host_bridge[1]| > + * +-/---------\---+ +--/---------\--+ > + * /- -\ /- -\ > + * /- -\ /- -\ > + * +-------------+ +-------------+ +-------------+ +-------------+ > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > + * +------|------+ +------|------+ +------|------+ +------|------+ > + * | | | | > + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ > + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| > + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ > + * | \ / \ / \ / \ > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ > + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ > + * | | | | | | | | > + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ > + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| > + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ > + */ > static struct platform_device *cxl_acpi; > static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES]; > #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS) > @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT]; > #define NR_MEM_MULTI \ > (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS) > static struct platform_device *cxl_switch_dport[NR_MEM_MULTI]; > +struct platform_device *cxl_mem[NR_MEM_MULTI]; > > +/* > + * 1) Preconfigured region support (Simulated BIOS configured region) > + * 2) 'Pass-through' decoder > + * > + * +---------------+ > + * | hb_single | > + * +------|--------+ > + * | > + * +------|--------+ > + * | root_single | > + * +------|--------+ > + * | > + * +----------|----------+ > + * | swu_single | > + * +-----|-----------|---+ > + * | | > + * +-----|-----+ +--|--------+ > + * |swd_single | | swd_single| > + * +-----|-----+ +----|------+ > + * | | > + * +------|-----+ +----|-------+ > + * |mem_single | |mem_single | > + * +------------+ +------------+ > + */ > static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST]; > static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST]; > static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST]; > #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS) > static struct platform_device *cxl_swd_single[NR_MEM_SINGLE]; > - > -struct platform_device *cxl_mem[NR_MEM_MULTI]; > struct platform_device *cxl_mem_single[NR_MEM_SINGLE]; > > +/* > + * 1) RCD > + * 2) Type-2 (Accelerator) > + * > + * +-----+ > + * | rch | > + * +--|--+ > + * | > + * +-|--+ > + * |rcd | > + * +----+ > + */ > static struct platform_device *cxl_rch[NR_CXL_RCH]; > static struct platform_device *cxl_rcd[NR_CXL_RCH]; > > @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev) > return false; > } > > +/* > + * +---------------+ +---------------+ > + * | host_bridge[0]| | host_bridge[1]| > + * +---------------+ +---------------+ > + * +---------------+ > + * | hb_single | (host_bridge[2]) > + * +---------------+ > + * +-----+ > + * | rch | (host_bridge[3]) > + * +-----+ > + */ > static struct acpi_device acpi0017_mock; > static struct acpi_device host_bridge[NR_BRIDGES] = { > [0] = { >
On Wed, 17 May 2023 14:28:12 -0700 Ira Weiny <ira.weiny@intel.com> wrote: > The devices created, their relationship, and intended testing purpose is > not extremely clear, especially for those unfamiliar with cxl-test. > > Document the purpose of each hierarchy. Add ASCII art to show the > relationship of devices. Group the device declarations together based > on the hierarchies. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Trivial nitpicks below :) Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 73 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > index bf00dc52fe96..bd38a5fb60ae 100644 > --- a/tools/testing/cxl/test/cxl.c > +++ b/tools/testing/cxl/test/cxl.c > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > #define NR_CXL_PORT_DECODERS 8 > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > +/* > + * Interleave testing Doesn't include the cfmws, which will be tricky to draw, but maybe you could add something to indicate they interleave over the two HB sometimes? > + * > + * +---------------+ +---------------+ > + * | host_bridge[0]| | host_bridge[1]| > + * +-/---------\---+ +--/---------\--+ Text for host bridges is right aligned. > + * /- -\ /- -\ > + * /- -\ /- -\ > + * +-------------+ +-------------+ +-------------+ +-------------+ > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > + * +------|------+ +------|------+ +------|------+ +------|------+ and root ports are left aligned. I'd shrink both boxes so they are same as the switches below - or expand them to give a space on either side of the text. > + * | | | | > + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ > + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| > + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ > + * | \ / \ / \ / \ > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ > + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ > + * | | | | | | | | > + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ > + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| > + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ > + */ > static struct platform_device *cxl_acpi; > static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES]; > #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS) > @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT]; > #define NR_MEM_MULTI \ > (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS) > static struct platform_device *cxl_switch_dport[NR_MEM_MULTI]; > +struct platform_device *cxl_mem[NR_MEM_MULTI]; > > +/* > + * 1) Preconfigured region support (Simulated BIOS configured region) > + * 2) 'Pass-through' decoder > + * > + * +---------------+ > + * | hb_single | > + * +------|--------+ > + * | > + * +------|--------+ > + * | root_single | > + * +------|--------+ > + * | > + * +----------|----------+ > + * | swu_single | > + * +-----|-----------|---+ > + * | | > + * +-----|-----+ +--|--------+ > + * |swd_single | | swd_single| > + * +-----|-----+ +----|------+ > + * | | > + * +------|-----+ +----|-------+ > + * |mem_single | |mem_single | > + * +------------+ +------------+ mem[0] etc? Also swd_single[0] etc? For consistency with above. > + */ > static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST]; > static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST]; > static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST]; > #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS) > static struct platform_device *cxl_swd_single[NR_MEM_SINGLE]; > - > -struct platform_device *cxl_mem[NR_MEM_MULTI]; > struct platform_device *cxl_mem_single[NR_MEM_SINGLE]; > > +/* > + * 1) RCD > + * 2) Type-2 (Accelerator) > + * > + * +-----+ > + * | rch | > + * +--|--+ > + * | > + * +-|--+ > + * |rcd | > + * +----+ > + */ > static struct platform_device *cxl_rch[NR_CXL_RCH]; > static struct platform_device *cxl_rcd[NR_CXL_RCH]; > > @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev) > return false; > } > > +/* > + * +---------------+ +---------------+ > + * | host_bridge[0]| | host_bridge[1]| > + * +---------------+ +---------------+ > + * +---------------+ > + * | hb_single | (host_bridge[2]) > + * +---------------+ > + * +-----+ > + * | rch | (host_bridge[3]) > + * +-----+ > + */ Not sure what this diagram is illustrating... > static struct acpi_device acpi0017_mock; > static struct acpi_device host_bridge[NR_BRIDGES] = { > [0] = { >
On Thu, 18 May 2023 10:50:20 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Wed, 17 May 2023 14:28:12 -0700 > Ira Weiny <ira.weiny@intel.com> wrote: > > > The devices created, their relationship, and intended testing purpose is > > not extremely clear, especially for those unfamiliar with cxl-test. > > > > Document the purpose of each hierarchy. Add ASCII art to show the > > relationship of devices. Group the device declarations together based > > on the hierarchies. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > Trivial nitpicks below :) > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Actually scrub that RB - the last question on what the final diagram means probably makes an RB inappropriate for now... > > > --- > > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > > index bf00dc52fe96..bd38a5fb60ae 100644 > > --- a/tools/testing/cxl/test/cxl.c > > +++ b/tools/testing/cxl/test/cxl.c > > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > > #define NR_CXL_PORT_DECODERS 8 > > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > > > +/* > > + * Interleave testing > > Doesn't include the cfmws, which will be tricky to draw, but maybe you could > add something to indicate they interleave over the two HB sometimes? > > > + * > > + * +---------------+ +---------------+ > > + * | host_bridge[0]| | host_bridge[1]| > > + * +-/---------\---+ +--/---------\--+ > Text for host bridges is right aligned. > > + * /- -\ /- -\ > > + * /- -\ /- -\ > > + * +-------------+ +-------------+ +-------------+ +-------------+ > > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > > + * +------|------+ +------|------+ +------|------+ +------|------+ > and root ports are left aligned. > I'd shrink both boxes so they are same as the switches below - or expand them to give > a space on either side of the text. > > > > + * | | | | > > + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ > > + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| > > + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ > > + * | \ / \ / \ / \ > > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ > > + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | > > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| > > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ > > + * | | | | | | | | > > + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ > > + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| > > + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ > > + */ > > static struct platform_device *cxl_acpi; > > static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES]; > > #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS) > > @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT]; > > #define NR_MEM_MULTI \ > > (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS) > > static struct platform_device *cxl_switch_dport[NR_MEM_MULTI]; > > +struct platform_device *cxl_mem[NR_MEM_MULTI]; > > > > +/* > > + * 1) Preconfigured region support (Simulated BIOS configured region) > > + * 2) 'Pass-through' decoder > > + * > > + * +---------------+ > > + * | hb_single | > > + * +------|--------+ > > + * | > > + * +------|--------+ > > + * | root_single | > > + * +------|--------+ > > + * | > > + * +----------|----------+ > > + * | swu_single | > > + * +-----|-----------|---+ > > + * | | > > + * +-----|-----+ +--|--------+ > > + * |swd_single | | swd_single| > > + * +-----|-----+ +----|------+ > > + * | | > > + * +------|-----+ +----|-------+ > > + * |mem_single | |mem_single | > > + * +------------+ +------------+ > mem[0] etc? Also swd_single[0] etc? > > For consistency with above. > > > + */ > > static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST]; > > static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST]; > > static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST]; > > #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS) > > static struct platform_device *cxl_swd_single[NR_MEM_SINGLE]; > > - > > -struct platform_device *cxl_mem[NR_MEM_MULTI]; > > struct platform_device *cxl_mem_single[NR_MEM_SINGLE]; > > > > +/* > > + * 1) RCD > > + * 2) Type-2 (Accelerator) > > + * > > + * +-----+ > > + * | rch | > > + * +--|--+ > > + * | > > + * +-|--+ > > + * |rcd | > > + * +----+ > > + */ > > static struct platform_device *cxl_rch[NR_CXL_RCH]; > > static struct platform_device *cxl_rcd[NR_CXL_RCH]; > > > > @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev) > > return false; > > } > > > > +/* > > + * +---------------+ +---------------+ > > + * | host_bridge[0]| | host_bridge[1]| > > + * +---------------+ +---------------+ > > + * +---------------+ > > + * | hb_single | (host_bridge[2]) > > + * +---------------+ > > + * +-----+ > > + * | rch | (host_bridge[3]) > > + * +-----+ > > + */ > > Not sure what this diagram is illustrating... > > > static struct acpi_device acpi0017_mock; > > static struct acpi_device host_bridge[NR_BRIDGES] = { > > [0] = { > > >
Jonathan Cameron wrote: > On Wed, 17 May 2023 14:28:12 -0700 > Ira Weiny <ira.weiny@intel.com> wrote: > [snip] > > --- > > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > > index bf00dc52fe96..bd38a5fb60ae 100644 > > --- a/tools/testing/cxl/test/cxl.c > > +++ b/tools/testing/cxl/test/cxl.c > > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > > #define NR_CXL_PORT_DECODERS 8 > > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > > > +/* > > + * Interleave testing > > Doesn't include the cfmws, which will be tricky to draw, but maybe you could > add something to indicate they interleave over the two HB sometimes? I was mainly looking to document the devices below. Because they are all 'platform_device' and they are assigned type in the code which made things a bit harder for me to follow when I was going through it the other day. > > > + * > > + * +---------------+ +---------------+ > > + * | host_bridge[0]| | host_bridge[1]| > > + * +-/---------\---+ +--/---------\--+ > Text for host bridges is right aligned. Ah true. I used an online ascii editor for these. :-D So I did not pay any attention when I copied pasted. > > + * /- -\ /- -\ > > + * /- -\ /- -\ > > + * +-------------+ +-------------+ +-------------+ +-------------+ > > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > > + * +------|------+ +------|------+ +------|------+ +------|------+ > and root ports are left aligned. > I'd shrink both boxes so they are same as the switches below - or expand them to give > a space on either side of the text. Done. > > > > +/* > > + * 1) Preconfigured region support (Simulated BIOS configured region) > > + * 2) 'Pass-through' decoder > > + * > > + * +---------------+ > > + * | hb_single | > > + * +------|--------+ > > + * | > > + * +------|--------+ > > + * | root_single | > > + * +------|--------+ > > + * | > > + * +----------|----------+ > > + * | swu_single | > > + * +-----|-----------|---+ > > + * | | > > + * +-----|-----+ +--|--------+ > > + * |swd_single | | swd_single| > > + * +-----|-----+ +----|------+ > > + * | | > > + * +------|-----+ +----|-------+ > > + * |mem_single | |mem_single | > > + * +------------+ +------------+ > mem[0] etc? Also swd_single[0] etc? > > For consistency with above. > Actually mem_single[0,1]. yea swd_single[0,1]. > > > > +/* > > + * +---------------+ +---------------+ > > + * | host_bridge[0]| | host_bridge[1]| > > + * +---------------+ +---------------+ > > + * +---------------+ > > + * | hb_single | (host_bridge[2]) > > + * +---------------+ > > + * +-----+ > > + * | rch | (host_bridge[3]) > > + * +-----+ > > + */ > > Not sure what this diagram is illustrating... Just showing how the acpi_devices array below ties in with the above diagrams. Mainly that their is not a 1:1 corelation between cxl_host_bridge[] and host_bridge[]. That index 2 and 3 are other platform devices as shown. I could probably make that equivalency note in the diagrams above where hb_single and rch are defined/documented. Let me do that. Ira
Ira Weiny wrote: > The devices created, their relationship, and intended testing purpose is > not extremely clear, especially for those unfamiliar with cxl-test. > > Document the purpose of each hierarchy. Add ASCII art to show the > relationship of devices. Group the device declarations together based > on the hierarchies. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 73 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > index bf00dc52fe96..bd38a5fb60ae 100644 > --- a/tools/testing/cxl/test/cxl.c > +++ b/tools/testing/cxl/test/cxl.c > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > #define NR_CXL_PORT_DECODERS 8 > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > +/* > + * Interleave testing > + * > + * +---------------+ +---------------+ > + * | host_bridge[0]| | host_bridge[1]| > + * +-/---------\---+ +--/---------\--+ > + * /- -\ /- -\ > + * /- -\ /- -\ > + * +-------------+ +-------------+ +-------------+ +-------------+ > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > + * +------|------+ +------|------+ +------|------+ +------|------+ > + * | | | | > + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ > + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| > + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ > + * | \ / \ / \ / \ > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ > + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ > + * | | | | | | | | > + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ > + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| > + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ > + */ Circling back to merge this I realize that the numbering is off. For example a snippet from "cxl list -BPT -b cxl_test" "ports:root3":[ { "port":"port5", "host":"cxl_host_bridge.1", "depth":1, "nr_dports":2, "dports":[ { "dport":"cxl_root_port.1", "id":1 }, { "dport":"cxl_root_port.3", "id":3 } ], This is due to the modulo math at setup time. I only noticed this because I wanted a diagram to refer to when doing some recent extensions. I wonder if we could just use "cxl list" to maintain this diagram, or maybe circle back and use this to keep an image up to date on a web page somewhere: https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/
+Matthew Ho Dan Williams wrote: > Ira Weiny wrote: > > The devices created, their relationship, and intended testing purpose is > > not extremely clear, especially for those unfamiliar with cxl-test. > > > > Document the purpose of each hierarchy. Add ASCII art to show the > > relationship of devices. Group the device declarations together based > > on the hierarchies. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c > > index bf00dc52fe96..bd38a5fb60ae 100644 > > --- a/tools/testing/cxl/test/cxl.c > > +++ b/tools/testing/cxl/test/cxl.c > > @@ -23,6 +23,31 @@ static int interleave_arithmetic; > > #define NR_CXL_PORT_DECODERS 8 > > #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) > > > > +/* > > + * Interleave testing > > + * > > + * +---------------+ +---------------+ > > + * | host_bridge[0]| | host_bridge[1]| > > + * +-/---------\---+ +--/---------\--+ > > + * /- -\ /- -\ > > + * /- -\ /- -\ > > + * +-------------+ +-------------+ +-------------+ +-------------+ > > + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | > > + * +------|------+ +------|------+ +------|------+ +------|------+ > > + * | | | | > > + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ > > + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| > > + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ > > + * | \ / \ / \ / \ > > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ > > + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | > > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| > > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ > > + * | | | | | | | | > > + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ > > + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| > > + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ > > + */ > > Circling back to merge this I realize that the numbering is off. For > example a snippet from "cxl list -BPT -b cxl_test" > > "ports:root3":[ > { > "port":"port5", > "host":"cxl_host_bridge.1", > "depth":1, > "nr_dports":2, > "dports":[ > { > "dport":"cxl_root_port.1", > "id":1 > }, > { > "dport":"cxl_root_port.3", > "id":3 > } > ], > > This is due to the modulo math at setup time. I only noticed this > because I wanted a diagram to refer to when doing some recent > extensions. :-( I did not realize this detail. > > I wonder if we could just use "cxl list" to maintain this diagram, or > maybe circle back and use this to keep an image up to date on a web page > somewhere: > > https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/ I forgot about this and this is a nice idea. Did that support land? Ira
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c index bf00dc52fe96..bd38a5fb60ae 100644 --- a/tools/testing/cxl/test/cxl.c +++ b/tools/testing/cxl/test/cxl.c @@ -23,6 +23,31 @@ static int interleave_arithmetic; #define NR_CXL_PORT_DECODERS 8 #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH) +/* + * Interleave testing + * + * +---------------+ +---------------+ + * | host_bridge[0]| | host_bridge[1]| + * +-/---------\---+ +--/---------\--+ + * /- -\ /- -\ + * /- -\ /- -\ + * +-------------+ +-------------+ +-------------+ +-------------+ + * |root_port[0] | |root_port[1] | |root_port[2] | |root_port[3] | + * +------|------+ +------|------+ +------|------+ +------|------+ + * | | | | + * +-------|-------+ +-------|-------+ +-------|-------+ +-------|-------+ + * |switch_uport[0]| |switch_uport[1]| |switch_uport[2]| |switch_uport[3]| + * +---|-------|---+ +---/-------|---+ +---/-------|---+ +---|-------\---+ + * | \ / \ / \ / \ + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+ + * |switch ||switch ||switch ||switch ||switch ||switch ||switch ||switch | + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]| + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+ + * | | | | | | | | + * +---|--+ +-|----+ +---|--+ +---|--+ +--|---+ +---|--+ +---|--+ +---|--+ + * |mem[0]| |mem[1]| |mem[2]| |mem[3]| |mem[4]| |mem[5]| |mem[6]| |mem[7]| + * +------+ +------+ +------+ +------+ +------+ +------+ +------+ +------+ + */ static struct platform_device *cxl_acpi; static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES]; #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS) @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT]; #define NR_MEM_MULTI \ (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS) static struct platform_device *cxl_switch_dport[NR_MEM_MULTI]; +struct platform_device *cxl_mem[NR_MEM_MULTI]; +/* + * 1) Preconfigured region support (Simulated BIOS configured region) + * 2) 'Pass-through' decoder + * + * +---------------+ + * | hb_single | + * +------|--------+ + * | + * +------|--------+ + * | root_single | + * +------|--------+ + * | + * +----------|----------+ + * | swu_single | + * +-----|-----------|---+ + * | | + * +-----|-----+ +--|--------+ + * |swd_single | | swd_single| + * +-----|-----+ +----|------+ + * | | + * +------|-----+ +----|-------+ + * |mem_single | |mem_single | + * +------------+ +------------+ + */ static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST]; static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST]; static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST]; #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS) static struct platform_device *cxl_swd_single[NR_MEM_SINGLE]; - -struct platform_device *cxl_mem[NR_MEM_MULTI]; struct platform_device *cxl_mem_single[NR_MEM_SINGLE]; +/* + * 1) RCD + * 2) Type-2 (Accelerator) + * + * +-----+ + * | rch | + * +--|--+ + * | + * +-|--+ + * |rcd | + * +----+ + */ static struct platform_device *cxl_rch[NR_CXL_RCH]; static struct platform_device *cxl_rcd[NR_CXL_RCH]; @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev) return false; } +/* + * +---------------+ +---------------+ + * | host_bridge[0]| | host_bridge[1]| + * +---------------+ +---------------+ + * +---------------+ + * | hb_single | (host_bridge[2]) + * +---------------+ + * +-----+ + * | rch | (host_bridge[3]) + * +-----+ + */ static struct acpi_device acpi0017_mock; static struct acpi_device host_bridge[NR_BRIDGES] = { [0] = {
The devices created, their relationship, and intended testing purpose is not extremely clear, especially for those unfamiliar with cxl-test. Document the purpose of each hierarchy. Add ASCII art to show the relationship of devices. Group the device declarations together based on the hierarchies. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)