Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 May 2023 10:51:04 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Ira Weiny <ira.weiny@intel.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] tools/testing/cxl: Document test configurations
Message-ID: <20230518105104.00005f62@Huawei.com>
In-Reply-To: <20230518105020.0000424a@Huawei.com>
References: <20230426-cxl-fixes-v1-0-870c4c8b463a@intel.com>
        <20230426-cxl-fixes-v1-3-870c4c8b463a@intel.com>
        <20230518105020.0000424a@Huawei.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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] = {
> >   
>