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:   Fri, 9 Jun 2023 19:18:20 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     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>, <Jonathan.Cameron@huawei.com>,
        <linux-cxl@vger.kernel.org>
CC:     <terry.bowman@amd.com>, <rrichter@amd.com>,
        <linux-kernel@vger.kernel.org>, <bhelgaas@google.com>
Subject: RE: [PATCH v5 12/26] cxl/port: Store the port's Component Register
 mappings in struct cxl_port
Message-ID: <6483dd6c77fe3_e067a29421@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230607221651.2454764-1-terry.bowman@amd.com>
 <20230607221651.2454764-13-terry.bowman@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230607221651.2454764-13-terry.bowman@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/WP9ICsSv/9spKlKGCnyznEo3VeRttPE5MrdveWpRWwxP94ORVfnAM2UQiYj?=
 =?us-ascii?Q?A3zfsxXOeRJK1vTrRL9D/UTjIZNf+X4QeMbcGjRm1KVJ544odoBRNqAgAoCO?=
 =?us-ascii?Q?qJiKKr/+4LUxeReRvVXKsz/s+eyIUVLn8cKUFIFUj3lGn5rP7YFuJoe3uGog?=
 =?us-ascii?Q?fDY3+gZP1LLVU0lCWPGOlkqS1hYWSyW+oUN1dzJSzuwaSb/py9lJ8rZDVJv+?=
 =?us-ascii?Q?SnOK6vItC41gr/rpx/oqCfhpCRzzSJXu2QistKHooYLyu6lt+e8unWPeAwq4?=
 =?us-ascii?Q?P98DKs67/qngnbi7N5kKFbv17bZpdtnUZy8xjh0Bkv9GY52xaUjs2hh45qCe?=
 =?us-ascii?Q?FroHGaJ7MHbXzPV+k/gcqXpeYZoaBkS0k6hMR23YQMMGru/RTg4Mp0ft/Qmv?=
 =?us-ascii?Q?9aTdMHHORY47ucvaPN7vXOriVMr+3q3yMKZFR4XuqOIBGy7Ilha9hIc3m1Uo?=
 =?us-ascii?Q?Nn1WfWifA+qCoaMvtRFaPqC/+9WTauB7AKrRMvct3SeM9ZMmW7RAN7FopvTX?=
 =?us-ascii?Q?UbkYXP2SjgnAkECP3zzYbC0fXm4t2uY6TrE8tUIlj2ag2bxx7oLf3E9/8HHn?=
 =?us-ascii?Q?oyF6xT2am6XzfvcWfqkd4b2V/qE9/HnPweE+4oXtgxtEsPNxPBJcfV5KBFuA?=
 =?us-ascii?Q?plRpwX0G7gJeasGwotmcrZJbaVcRBPl3kZ3wsHokVFiGR/mfjP2rqU+iRxYZ?=
 =?us-ascii?Q?jmz5NijQR8vSEFmlMqejmMHKAh/7cWVpjwlGNOCSVZ30H5Cpi0q8i8+Ql0L/?=
 =?us-ascii?Q?7ul34lT9NLRK21tEAIqPd2S5AWjRHewpXHKunqmRHn9/36ldCc7A2TyyNKus?=
 =?us-ascii?Q?mm1i5MK2rkD/YUuNP/QdF4sTZWOVFl5cMl83RVpGqfSvVFdNWhRYHKXufJoL?=
 =?us-ascii?Q?NGCXF/MEMvGlBzxmTyT4spP9wNkjTc5AVa5oq/AA+ee8HB6vZ996Yl5y4YFx?=
 =?us-ascii?Q?qtEHlP4DPANAWiuUMhxnaQBZccRHanbkXsnhCKerbd24/nMp7Pvx5jloAP4c?=
 =?us-ascii?Q?JwOtlLjB3RwkXCaWWavaBrz5WRchZRumrdoWIgavmgRAlcAVwmpwUK9nIJDb?=
 =?us-ascii?Q?j6cD0C6fPbvx9G9g87DEFU2Q7mchuewmN3w/2YVdVyvXBcUjRddvC4S2xH9J?=
 =?us-ascii?Q?bGpy9w+u1HKeR3rZCYcdly0RDv3SSwhaA6vffDvjgE6hYKXyTXQA7nMf8KlY?=
 =?us-ascii?Q?xZKM6dLo4rUaulMVERl29Kqy6BG15WFsTBUtAQz1UbwtXQB4IzN+ouAOw+tf?=
 =?us-ascii?Q?eGEHz0mGpyFl1VG78jrsrPnRIj5e6+6zx90fuUjnCTD+sEKLH9u2f5xQ1OCY?=
 =?us-ascii?Q?qBkFwVDkeaVaHwLDOrnGVziZxsM7mOMvVmPIQcb0LLC1ZFfhF45dDKfc9cBI?=
 =?us-ascii?Q?fXTXKX6P5zDAUQ08AaT77uzVQLvkegKWv65quZlnh2D0bZjk47Wy5/qQKckm?=
 =?us-ascii?Q?YoJfmPzQQF+KtKif766SwCGFEGBy8bO0yN9Rsc53g5zYobwE1ooZdzZLcdeo?=
 =?us-ascii?Q?NCwS/MmH5cq6Qd5z7sTnXV13lkScbjUY5u+Ttac2WXBpWwulV+7j3fJgmK4v?=
 =?us-ascii?Q?kauTJtG9yFTek9KiR/lbqjVFrSfMDEe9cIY4LJMOIoVhfFfBZSYOJWFADii1?=
 =?us-ascii?Q?4A=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e34464a-f42d-4a5e-ad6b-08db6958f847
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2023 02:18:24.8492
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: N762wsyJ+naL/viBV86bDJ8qPtHUNX7Ri7KLuqnQWnTHWlhHy+y9FpdfHGlPfPvtlX2ePm2vC6MnQ8MnSa/zTF/gu76NEIBxEZ7aq46sKZA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5162
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham 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

Terry Bowman 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>
> ---
>  drivers/cxl/core/port.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h       |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 8b688ac506ca..305125b193ce 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;
> +
> +	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,17 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>  	if (rc)
>  		goto err;
>  
> +	/*
> +	 * Some components may not use capablities or their
> +	 * implementation is optional. A component register block may
> +	 * not be present then and component_reg_phys is therefore
> +	 * unset. Instead run the check later when setting up the
> +	 * capabilities.
> +	 */
> +	rc = cxl_port_setup_regs(port, component_reg_phys);
> +	if (rc && rc != -ENODEV)
> +		goto err;

I would prefer to unwrap some of the levels and just do:

        port->comp_map = (struct cxl_register_map) {
                .dev = &port->dev;
                .reg_type = CXL_REGLOC_RBI_COMPONENT;
                .resource = component_reg_phys;
                .max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
        };

        if (component_reg_phys != CXL_RESOURCE_NONE) {
                rc = cxl_setup_regs(&port->comp_map);
                if (rc)
                        goto err;
        }

...earlier inside of cxl_port_alloc() since that part is common to all
ports. Then the confusing comment becomes unnecessary because it is
clear that CXL_RESOURCE_NONE is an acceptable case.

Other than that, the approach and rationale looks good.