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;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date:   Fri, 2 Jun 2023 17:58:49 +0200
From:   Robert Richter <rrichter@amd.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Terry Bowman <terry.bowman@amd.com>, <alison.schofield@intel.com>,
        <vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
        <bwidawsk@kernel.org>, <dan.j.williams@intel.com>,
        <dave.jiang@intel.com>, <linux-cxl@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bhelgaas@google.com>
Subject: Re: [PATCH v4 10/23] cxl/port: Store the port's Component Register
 mappings in struct cxl_port
Message-ID: <ZHoRubXqiwlH3mgu@rric.localdomain>
References: <20230523232214.55282-1-terry.bowman@amd.com>
 <20230523232214.55282-11-terry.bowman@amd.com>
 <20230601140603.00007ba2@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230601140603.00007ba2@Huawei.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jun 2023 15:58:57.6381
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 15e08617-69a5-4b7b-fb4b-08db63824625
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT034.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB8969
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 01.06.23 14:06:03, Jonathan Cameron wrote:
> On Tue, 23 May 2023 18:22:01 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
> 
> > From: Robert Richter <rrichter@amd.com>
> > 
> > CXL capabilities are stored in the Component Registers. To use them,
> > the specific I/O ranges of the capabilities must be determined by
> > probing the registers. For this, the whole Component Register range
> > needs to be mapped temporarily to detect the offset and length of a
> > capability range.
> > 
> > In order to use more than one capability of a component (e.g. RAS and
> > HDM) the Component Register are probed and its mappings created
> > multiple times. This also causes overlapping I/O ranges as the whole
> > Component Register range must be mapped again while a capability's I/O
> > range is already mapped.
> > 
> > Different capabilities cannot be setup at the same time. E.g. the RAS
> > capability must be made available as soon as the PCI driver is bound,
> > the HDM decoder is setup later during port enumeration. Moreover,
> > during early setup it is still unknown if a certain capability is
> > needed. A central capability setup is therefore not possible,
> > capabilities must be individually enabled once needed during
> > initialization.
> > 
> > To avoid a duplicate register probe and overlapping I/O mappings, only
> > probe the Component Registers one time and store the Component
> > Register mapping in struct port. The stored mappings can be used later
> > to iomap the capability register range when enabling the capability,
> > which will be implemented in a follow-on patch.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Some minor comments...
> 
> 
> > ---
> >  drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h       |  2 ++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index eff91f141fde..34e929f1723b 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -686,6 +686,28 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> >  	return ERR_PTR(rc);
> >  }
> >  
> > +static int cxl_setup_comp_regs(struct device *dev, struct cxl_register_map *map,
> > +			       resource_size_t component_reg_phys)
> > +{
> > +	if (component_reg_phys == CXL_RESOURCE_NONE)
> > +		return -ENODEV;
> > +
> > +	memset(map, 0, sizeof(*map));
> > +	map->dev = dev;
> > +	map->reg_type = CXL_REGLOC_RBI_COMPONENT;
> > +	map->resource = component_reg_phys;
> > +	map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
> setting most elements. Maybe
> 	*map = (struct cxl_register_map) {
> 		.dev = ...
> etc is cleaner and effectively the same thing.
> 	};

I really like the memset() pattern for this. See my comment in an
earlier mail for this.

> > +
> > +	return cxl_setup_regs(map);
> > +}
> > +
> > +static inline int cxl_port_setup_regs(struct cxl_port *port,
> > +				      resource_size_t component_reg_phys)
> > +{
> > +	return cxl_setup_comp_regs(&port->dev, &port->comp_map,
> > +				   component_reg_phys);
> > +}
> > +
> >  static struct cxl_port *__devm_cxl_add_port(struct device *host,
> >  					    struct device *uport,
> >  					    resource_size_t component_reg_phys,
> > @@ -709,6 +731,10 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> >  	if (rc)
> >  		goto err;
> >  
> > +	rc = cxl_port_setup_regs(port, component_reg_phys);
> > +	if (rc && rc != -ENODEV)
> > +		goto err;
> 
> I'd add a comment on why not being present is fine here.

Yes, will add that.

"Some components may no capablities or implementation is optional. So
do not fail here if no component register block exists and
component_reg_phys is unset. Instead run the check later when setting
up the capabilities."

Thanks,

-Robert

> 
> 
> > +
> >  	rc = device_add(dev);
> >  	if (rc)
> >  		goto err;
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index e5ae5f4e6669..c76e1f84ba61 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -552,6 +552,7 @@ struct cxl_dax_region {
> >   * @regions: cxl_region_ref instances, regions mapped by this port
> >   * @parent_dport: dport that points to this port in the parent
> >   * @decoder_ida: allocator for decoder ids
> > + * @comp_map: component register capability mappings
> >   * @nr_dports: number of entries in @dports
> >   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> >   * @commit_end: cursor to track highest committed decoder for commit ordering
> > @@ -571,6 +572,7 @@ struct cxl_port {
> >  	struct xarray regions;
> >  	struct cxl_dport *parent_dport;
> >  	struct ida decoder_ida;
> > +	struct cxl_register_map comp_map;
> >  	int nr_dports;
> >  	int hdm_end;
> >  	int commit_end;
>