[PATCH v3 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[PATCH v3 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[Dave Jiang]

v3:
- Simplify to directly return devm_cxl_enable_mem() in
  cxl_hdm_decode_init(). (Jonathan)
- Move relevant changes from 6/8 to 5/8 and update kdoc. (Jonathan)
- Add kernel doc update. (Jonathan)

v2:
- Refactor to continue when size is 0. (Jonathan)
- Update kdoc comments (Jonathan)
- Use a bool for is_cxl_endpoint() to make it easier for static analysis
  (Jonathan)
- Update commit log to indicate cxl_hdm_decode_init() return additional
  error codes after change. (Jonathan)
- Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
- Skip HDM enabling if DVSEC range is active. (Jonathan)
- Set target_count to same as number of ranges. (Jonathan)
- Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
- Refactor to put error case out of line. (Jonathan)
- Drop 7/8 "cxl: suppress component register discovery failure warning for RCD"
- Add support for missing case where HDM decoders are present but no decoders
  are committed. (Case 5 below)

This series provides the emulation of HDM decoders from the programmed range
registers. From CXL 3.0 spec 8.1.3.8, there can be up to 2 ranges programmed.
Some devices may not implement HDM decoder registers and some may not be
programmed by the BIOS. Under such scenarios, if one of more range registers
are programmed, then we can create an emulated HDM decoder per active range
indicated by the range registers. The emulated HDM decoders will show up as
locked and cannot be reprogrammed.

Below is a table that indicates different scenarios the driver may encounter:

rr: Range registers not programmed
hdm: HDM decoders not programmed
RR: Range registers programmed by BIOS
HDM: HDM decoders programmed by BIOS

emulate HDM: Create HDM decoder software structs and use values from range registers.
keep HDM: Populate HDM decoder software structs with values in HDM decoder registers.

Case 1:        Case 2:        Case 3:    Case 4:     Case 5:       Case 6:
rr             RR             rr hdm	 rr HDM	     RR hdm        RR HDM
unsupported    emulate HDM    keep HDM	 keep HDM    emulate HDM   keep HDM

For convenience, the kernel branch can be retrieved here [1].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-emulate-hdm

---

Dave Jiang (8):
      cxl: break out range register decoding from cxl_hdm_decode_init()
      cxl: export cxl_dvsec_rr_decode() to cxl_port
      cxl: refactor cxl_hdm_decode_init()
      cxl: emulate HDM decoder from DVSEC range registers
      cxl: create emulated cxl_hdm for devices that do not have HDM decoders
      cxl: create emulated decoders for devices without HDM decoders
      cxl: Add emulation when HDM decoders are not committed
      cxl: remove locked check for dvsec_range_allowed()


 drivers/cxl/core/hdm.c | 120 ++++++++++++++++++++++---
 drivers/cxl/core/pci.c | 199 +++++++++++++++++++----------------------
 drivers/cxl/cxl.h      |  21 ++++-
 drivers/cxl/cxlmem.h   |  12 ---
 drivers/cxl/cxlpci.h   |   3 +-
 drivers/cxl/port.c     |  25 ++++--
 6 files changed, 239 insertions(+), 141 deletions(-)

base-commit: 88603b6dc419445847923fcb7fe5080067a30f98

[PATCH v3 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dave Jiang]

There are 2 scenarios that requires additional handling. 1. A device that
has active ranges in DVSEC range registers (RR) but no HDM decoder register
block. 2. A device that has both RR active and HDM, but the HDM decoders
are not programmed. The goal is to create emulated decoder software structs
based on the RR.

Move the CXL DVSEC range register decoding code block from
cxl_hdm_decode_init() to its own function. Refactor code in preparation for
the HDM decoder emulation.  There is no functionality change to the code.
Name the new function to cxl_dvsec_rr_decode().

The only change is to set range->start and range->end to CXL_RESOURCE_NONE
and skipping the reading of base registers if the range size is 0, which
equates to range not active.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v2:
- Refactor to continue when size is 0. (Jonathan)
---
 drivers/cxl/core/pci.c |   63 ++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 57764e9cd19d..a8ecc6ddb3d7 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -141,11 +141,10 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
 
-static int wait_for_valid(struct cxl_dev_state *cxlds)
+static int wait_for_valid(struct pci_dev *pdev, int d)
 {
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	int d = cxlds->cxl_dvsec, rc;
 	u32 val;
+	int rc;
 
 	/*
 	 * Memory_Info_Valid: When set, indicates that the CXL Range 1 Size high
@@ -334,20 +333,11 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 	return true;
 }
 
-/**
- * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
- * @cxlds: Device state
- * @cxlhdm: Mapped HDM decoder Capability
- *
- * Try to enable the endpoint's HDM Decoder Capability
- */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
+			       struct cxl_endpoint_dvsec_info *info)
 {
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	struct cxl_endpoint_dvsec_info info = { 0 };
 	int hdm_count, rc, i, ranges = 0;
 	struct device *dev = &pdev->dev;
-	int d = cxlds->cxl_dvsec;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -378,7 +368,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 	if (!hdm_count || hdm_count > 2)
 		return -EINVAL;
 
-	rc = wait_for_valid(cxlds);
+	rc = wait_for_valid(pdev, d);
 	if (rc) {
 		dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc);
 		return rc;
@@ -389,9 +379,9 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 	 * disabled, and they will remain moot after the HDM Decoder
 	 * capability is enabled.
 	 */
-	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
-	if (!info.mem_enabled)
-		goto hdm_init;
+	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
+	if (!info->mem_enabled)
+		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
 		u64 base, size;
@@ -410,6 +400,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 			return rc;
 
 		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
+		if (!size) {
+			info->dvsec_range[i] = (struct range) {
+				.start = CXL_RESOURCE_NONE,
+				.end = CXL_RESOURCE_NONE,
+			};
+			continue;
+		}
 
 		rc = pci_read_config_dword(
 			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
@@ -425,22 +422,42 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info.dvsec_range[i] = (struct range) {
+		info->dvsec_range[i] = (struct range) {
 			.start = base,
 			.end = base + size - 1
 		};
 
-		if (size)
-			ranges++;
+		ranges++;
 	}
 
-	info.ranges = ranges;
+	info->ranges = ranges;
+
+	return 0;
+}
+
+/**
+ * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
+ * @cxlds: Device state
+ * @cxlhdm: Mapped HDM decoder Capability
+ *
+ * Try to enable the endpoint's HDM Decoder Capability
+ */
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct cxl_endpoint_dvsec_info info = { 0 };
+	struct device *dev = &pdev->dev;
+	int d = cxlds->cxl_dvsec;
+	int rc;
+
+	rc = cxl_dvsec_rr_decode(pdev, d, &info);
+	if (rc < 0)
+		return rc;
 
 	/*
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-hdm_init:
 	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");

RE: [PATCH v3 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dan Williams]

Dave Jiang wrote:
> There are 2 scenarios that requires additional handling. 1. A device that
> has active ranges in DVSEC range registers (RR) but no HDM decoder register
> block. 2. A device that has both RR active and HDM, but the HDM decoders
> are not programmed. The goal is to create emulated decoder software structs
> based on the RR.
> 
> Move the CXL DVSEC range register decoding code block from
> cxl_hdm_decode_init() to its own function. Refactor code in preparation for
> the HDM decoder emulation.  There is no functionality change to the code.
> Name the new function to cxl_dvsec_rr_decode().
> 
> The only change is to set range->start and range->end to CXL_RESOURCE_NONE
> and skipping the reading of base registers if the range size is 0, which
> equates to range not active.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v2:
> - Refactor to continue when size is 0. (Jonathan)
> ---
>  drivers/cxl/core/pci.c |   63 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 57764e9cd19d..a8ecc6ddb3d7 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
[..]
> @@ -410,6 +400,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>  			return rc;
>  
>  		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
> +		if (!size) {
> +			info->dvsec_range[i] = (struct range) {
> +				.start = CXL_RESOURCE_NONE,
> +				.end = CXL_RESOURCE_NONE,
> +			};

This should just be .start = 0, and .end = -1, or .start =
CXL_RESOURCE_NONE, and .end = CXL_RESOURCE_NONE - 1, right? Otherwise,
range_len() on this range will be non-zero. Likely moot because the
range will be skipped because @ranges++ gets skipped, but it looks
strange written this way.

RE: [PATCH v3 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dan Williams]

Dan Williams wrote:
> Dave Jiang wrote:
> > There are 2 scenarios that requires additional handling. 1. A device that
> > has active ranges in DVSEC range registers (RR) but no HDM decoder register
> > block. 2. A device that has both RR active and HDM, but the HDM decoders
> > are not programmed. The goal is to create emulated decoder software structs
> > based on the RR.
> > 
> > Move the CXL DVSEC range register decoding code block from
> > cxl_hdm_decode_init() to its own function. Refactor code in preparation for
> > the HDM decoder emulation.  There is no functionality change to the code.
> > Name the new function to cxl_dvsec_rr_decode().
> > 
> > The only change is to set range->start and range->end to CXL_RESOURCE_NONE
> > and skipping the reading of base registers if the range size is 0, which
> > equates to range not active.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > 
> > ---
> > v2:
> > - Refactor to continue when size is 0. (Jonathan)
> > ---
> >  drivers/cxl/core/pci.c |   63 ++++++++++++++++++++++++++++++------------------
> >  1 file changed, 40 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 57764e9cd19d..a8ecc6ddb3d7 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> [..]
> > @@ -410,6 +400,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
> >  			return rc;
> >  
> >  		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
> > +		if (!size) {
> > +			info->dvsec_range[i] = (struct range) {
> > +				.start = CXL_RESOURCE_NONE,
> > +				.end = CXL_RESOURCE_NONE,
> > +			};
> 
> This should just be .start = 0, and .end = -1, or .start =
> CXL_RESOURCE_NONE, and .end = CXL_RESOURCE_NONE - 1, right? Otherwise,
> range_len() on this range will be non-zero. Likely moot because the
> range will be skipped because @ranges++ gets skipped, but it looks
> strange written this way.

Actually, IIUC, this update can just be deleted altogether because
@ranges++ being skipped already invalidates this entry.

Re: [PATCH v3 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dave Jiang]

On 2/7/23 2:58 PM, Dan Williams wrote:
> Dan Williams wrote:
>> Dave Jiang wrote:
>>> There are 2 scenarios that requires additional handling. 1. A device that
>>> has active ranges in DVSEC range registers (RR) but no HDM decoder register
>>> block. 2. A device that has both RR active and HDM, but the HDM decoders
>>> are not programmed. The goal is to create emulated decoder software structs
>>> based on the RR.
>>>
>>> Move the CXL DVSEC range register decoding code block from
>>> cxl_hdm_decode_init() to its own function. Refactor code in preparation for
>>> the HDM decoder emulation.  There is no functionality change to the code.
>>> Name the new function to cxl_dvsec_rr_decode().
>>>
>>> The only change is to set range->start and range->end to CXL_RESOURCE_NONE
>>> and skipping the reading of base registers if the range size is 0, which
>>> equates to range not active.
>>>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> v2:
>>> - Refactor to continue when size is 0. (Jonathan)
>>> ---
>>>   drivers/cxl/core/pci.c |   63 ++++++++++++++++++++++++++++++------------------
>>>   1 file changed, 40 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index 57764e9cd19d..a8ecc6ddb3d7 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>> [..]
>>> @@ -410,6 +400,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>>>   			return rc;
>>>   
>>>   		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
>>> +		if (!size) {
>>> +			info->dvsec_range[i] = (struct range) {
>>> +				.start = CXL_RESOURCE_NONE,
>>> +				.end = CXL_RESOURCE_NONE,
>>> +			};
>>
>> This should just be .start = 0, and .end = -1, or .start =
>> CXL_RESOURCE_NONE, and .end = CXL_RESOURCE_NONE - 1, right? Otherwise,
>> range_len() on this range will be non-zero. Likely moot because the
>> range will be skipped because @ranges++ gets skipped, but it looks
>> strange written this way.
> 
> Actually, IIUC, this update can just be deleted altogether because
> @ranges++ being skipped already invalidates this entry.

Ok, I'll remove and fix the follow on code bits that relies on this.

[PATCH v3 2/8] cxl: export cxl_dvsec_rr_decode() to cxl_port

[Dave Jiang]

Call cxl_dvsec_rr_decode() in the beginning of cxl_port_probe() and
preserve the decoded information in a local
'struct cxl_endpoint_dvsec_info'. This info can be passed to various
functions later on in order to support the HDM decoder emulation.
The invocation of cxl_dvsec_rr_decode() in cxl_hdm_decode_init() is
removed and a pointer to the 'struct cxl_endpoint_dvsec_info' is passed
in.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

 ---
v2:
- Update kdoc comments (Jonathan)
- Use a bool for is_cxl_endpoint() to make it easier for static analysis
  (Jonathan)
---
 drivers/cxl/core/pci.c |   18 +++++++-----------
 drivers/cxl/cxl.h      |   14 ++++++++++++++
 drivers/cxl/cxlmem.h   |   12 ------------
 drivers/cxl/cxlpci.h   |    3 ++-
 drivers/cxl/port.c     |   21 ++++++++++++++-------
 5 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index a8ecc6ddb3d7..2b68b5d462da 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -333,8 +333,8 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 	return true;
 }
 
-static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
-			       struct cxl_endpoint_dvsec_info *info)
+int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
+			struct cxl_endpoint_dvsec_info *info)
 {
 	int hdm_count, rc, i, ranges = 0;
 	struct device *dev = &pdev->dev;
@@ -434,31 +434,27 @@ static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_dvsec_rr_decode, CXL);
 
 /**
  * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
  * @cxlds: Device state
  * @cxlhdm: Mapped HDM decoder Capability
+ * @info: Cached DVSEC range registers info
  *
  * Try to enable the endpoint's HDM Decoder Capability
  */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
+			struct cxl_endpoint_dvsec_info *info)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	struct cxl_endpoint_dvsec_info info = { 0 };
 	struct device *dev = &pdev->dev;
-	int d = cxlds->cxl_dvsec;
-	int rc;
-
-	rc = cxl_dvsec_rr_decode(pdev, d, &info);
-	if (rc < 0)
-		return rc;
 
 	/*
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
+	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1b1cf459ac77..1057affb2db0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -630,10 +630,24 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
 
+/**
+ * struct cxl_endpoint_dvsec_info - Cached DVSEC info
+ * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
+ * @ranges: Number of active HDM ranges this device uses.
+ * @dvsec_range: cached attributes of the ranges in the DVSEC, PCIE_DEVICE
+ */
+struct cxl_endpoint_dvsec_info {
+	bool mem_enabled;
+	int ranges;
+	struct range dvsec_range[2];
+};
+
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
+			struct cxl_endpoint_dvsec_info *info);
 
 bool is_cxl_region(struct device *dev);
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ab138004f644..187a310780a9 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -181,18 +181,6 @@ static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
  */
 #define CXL_CAPACITY_MULTIPLIER SZ_256M
 
-/**
- * struct cxl_endpoint_dvsec_info - Cached DVSEC info
- * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
- * @ranges: Number of active HDM ranges this device uses.
- * @dvsec_range: cached attributes of the ranges in the DVSEC, PCIE_DEVICE
- */
-struct cxl_endpoint_dvsec_info {
-	bool mem_enabled;
-	int ranges;
-	struct range dvsec_range[2];
-};
-
 /**
  * struct cxl_dev_state - The driver device state
  *
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 920909791bb9..430e23345a16 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -64,6 +64,7 @@ enum cxl_regloc_type {
 
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
+			struct cxl_endpoint_dvsec_info *info);
 void read_cdat_data(struct cxl_port *port);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 5453771bf330..404639a1c3d0 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -32,12 +32,22 @@ static void schedule_detach(void *cxlmd)
 
 static int cxl_port_probe(struct device *dev)
 {
+	struct cxl_endpoint_dvsec_info info = { 0 };
 	struct cxl_port *port = to_cxl_port(dev);
+	bool is_ep = is_cxl_endpoint(port);
+	struct cxl_dev_state *cxlds;
+	struct cxl_memdev *cxlmd;
 	struct cxl_hdm *cxlhdm;
 	int rc;
 
-
-	if (!is_cxl_endpoint(port)) {
+	if (is_ep) {
+		cxlmd = to_cxl_memdev(port->uport);
+		cxlds = cxlmd->cxlds;
+		rc = cxl_dvsec_rr_decode(to_pci_dev(cxlds->dev),
+					 cxlds->cxl_dvsec, &info);
+		if (rc < 0)
+			return rc;
+	} else {
 		rc = devm_cxl_port_enumerate_dports(port);
 		if (rc < 0)
 			return rc;
@@ -49,10 +59,7 @@ static int cxl_port_probe(struct device *dev)
 	if (IS_ERR(cxlhdm))
 		return PTR_ERR(cxlhdm);
 
-	if (is_cxl_endpoint(port)) {
-		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
-		struct cxl_dev_state *cxlds = cxlmd->cxlds;
-
+	if (is_ep) {
 		/* Cache the data early to ensure is_visible() works */
 		read_cdat_data(port);
 
@@ -61,7 +68,7 @@ static int cxl_port_probe(struct device *dev)
 		if (rc)
 			return rc;
 
-		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
+		rc = cxl_hdm_decode_init(cxlds, cxlhdm, &info);
 		if (rc)
 			return rc;
 

RE: [PATCH v3 2/8] cxl: export cxl_dvsec_rr_decode() to cxl_port

[Dan Williams]

Dave Jiang wrote:
> Call cxl_dvsec_rr_decode() in the beginning of cxl_port_probe() and
> preserve the decoded information in a local
> 'struct cxl_endpoint_dvsec_info'. This info can be passed to various
> functions later on in order to support the HDM decoder emulation.
> The invocation of cxl_dvsec_rr_decode() in cxl_hdm_decode_init() is
> removed and a pointer to the 'struct cxl_endpoint_dvsec_info' is passed
> in.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 

Looks good to me.

[PATCH v3 3/8] cxl: refactor cxl_hdm_decode_init()

[Dave Jiang]

With the previous refactoring of DVSEC range registers out of
cxl_hdm_decode_init(), it basically becomes a skeleton function. Squash
__cxl_hdm_decode_init() with cxl_hdm_decode_init() to simplify the code.
cxl_hdm_decode_init() now returns more error codes than just -EBUSY.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v3:
- Simplify to directly return devm_cxl_enable_mem() in
  cxl_hdm_decode_init(). (Jonathan)
v2:
- Update commit log to indicate cxl_hdm_decode_init() return additional
  error codes after change. (Jonathan)
---
 drivers/cxl/core/pci.c |  135 +++++++++++++++++++-----------------------------
 1 file changed, 54 insertions(+), 81 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 2b68b5d462da..97690c429e05 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -259,80 +259,6 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
 	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
 }
 
-static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-				  struct cxl_hdm *cxlhdm,
-				  struct cxl_endpoint_dvsec_info *info)
-{
-	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	struct cxl_port *port = cxlhdm->port;
-	struct device *dev = cxlds->dev;
-	struct cxl_port *root;
-	int i, rc, allowed;
-	u32 global_ctrl;
-
-	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-
-	/*
-	 * If the HDM Decoder Capability is already enabled then assume
-	 * that some other agent like platform firmware set it up.
-	 */
-	if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
-		rc = devm_cxl_enable_mem(&port->dev, cxlds);
-		if (rc)
-			return false;
-		return true;
-	}
-
-	root = to_cxl_port(port->dev.parent);
-	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
-		root = to_cxl_port(root->dev.parent);
-	if (!is_cxl_root(root)) {
-		dev_err(dev, "Failed to acquire root port for HDM enable\n");
-		return false;
-	}
-
-	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
-		struct device *cxld_dev;
-
-		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
-					     dvsec_range_allowed);
-		if (!cxld_dev) {
-			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
-			continue;
-		}
-		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
-		put_device(cxld_dev);
-		allowed++;
-	}
-
-	if (!allowed) {
-		cxl_set_mem_enable(cxlds, 0);
-		info->mem_enabled = 0;
-	}
-
-	/*
-	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
-	 * [High,Low] when HDM operation is enabled the range register values
-	 * are ignored by the device, but the spec also recommends matching the
-	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
-	 * are expected even though Linux does not require or maintain that
-	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
-	 */
-	if (info->mem_enabled)
-		return false;
-
-	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
-	if (rc)
-		return false;
-
-	rc = devm_cxl_enable_mem(&port->dev, cxlds);
-	if (rc)
-		return false;
-
-	return true;
-}
-
 int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
 			struct cxl_endpoint_dvsec_info *info)
 {
@@ -449,18 +375,65 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	struct device *dev = &pdev->dev;
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	struct cxl_port *port = cxlhdm->port;
+	struct cxl_port *root;
+	int i, rc, allowed;
+	u32 global_ctrl;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
 	/*
-	 * If DVSEC ranges are being used instead of HDM decoder registers there
-	 * is no use in trying to manage those.
+	 * If the HDM Decoder Capability is already enabled then assume
+	 * that some other agent like platform firmware set it up.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, info)) {
-		dev_err(dev,
-			"Legacy range registers configuration prevents HDM operation.\n");
-		return -EBUSY;
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
+		return devm_cxl_enable_mem(&port->dev, cxlds);
+
+	root = to_cxl_port(port->dev.parent);
+	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
+		root = to_cxl_port(root->dev.parent);
+	if (!is_cxl_root(root)) {
+		dev_err(dev, "Failed to acquire root port for HDM enable\n");
+		return -ENODEV;
 	}
 
-	return 0;
+	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
+		struct device *cxld_dev;
+
+		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
+					     dvsec_range_allowed);
+		if (!cxld_dev) {
+			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
+			continue;
+		}
+		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
+		put_device(cxld_dev);
+		allowed++;
+	}
+
+	if (!allowed) {
+		cxl_set_mem_enable(cxlds, 0);
+		info->mem_enabled = 0;
+	}
+
+	/*
+	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
+	 * [High,Low] when HDM operation is enabled the range register values
+	 * are ignored by the device, but the spec also recommends matching the
+	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
+	 * are expected even though Linux does not require or maintain that
+	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable.
+	 */
+	if (info->mem_enabled)
+		return -EBUSY;
+
+	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+	if (rc)
+		return rc;
+
+	return devm_cxl_enable_mem(&port->dev, cxlds);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
 

RE: [PATCH v3 3/8] cxl: refactor cxl_hdm_decode_init()

[Dan Williams]

Dave Jiang wrote:
> With the previous refactoring of DVSEC range registers out of
> cxl_hdm_decode_init(), it basically becomes a skeleton function. Squash
> __cxl_hdm_decode_init() with cxl_hdm_decode_init() to simplify the code.
> cxl_hdm_decode_init() now returns more error codes than just -EBUSY.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Looks good.

[PATCH v3 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dave Jiang]

In the case where HDM decoder register block exists but is not programmed
and at the same time the DVSEC range register range is active, populate the
CXL decoder object 'cxl_decoder' with info from DVSEC range registers.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v2:
- Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
- Skip HDM enabling if DVSEC range is active. (Jonathan)
---
 drivers/cxl/core/hdm.c |   36 +++++++++++++++++++++++++++++++++---
 drivers/cxl/core/pci.c |    2 +-
 drivers/cxl/cxl.h      |    3 ++-
 drivers/cxl/port.c     |    2 +-
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index dcc16d7cb8f3..af1f5f906f52 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -679,9 +679,34 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
 	return 0;
 }
 
+static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
+					    struct cxl_decoder *cxld, int which,
+					    struct cxl_endpoint_dvsec_info *info)
+{
+	if (!is_cxl_endpoint(port))
+		return -EOPNOTSUPP;
+
+	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
+		return -ENXIO;
+
+	cxld->target_type = CXL_DECODER_EXPANDER;
+	cxld->commit = NULL;
+	cxld->reset = NULL;
+
+	cxld->hpa_range = (struct range) {
+		.start = info->dvsec_range[which].start,
+		.end = info->dvsec_range[which].end,
+	};
+
+	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
+	port->commit_end = cxld->id;
+
+	return 0;
+}
+
 static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
-			    u64 *dpa_base)
+			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_endpoint_decoder *cxled = NULL;
 	u64 size, base, skip, dpa_size;
@@ -717,6 +742,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		.end = base + size - 1,
 	};
 
+	if (cxled && !committed &&
+	    info->dvsec_range[which].start != CXL_RESOURCE_NONE)
+		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
+
 	/* decoders are enabled if committed */
 	if (committed) {
 		cxld->flags |= CXL_DECODER_F_ENABLE;
@@ -790,7 +819,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
  * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
  * @cxlhdm: Structure to populate with HDM capabilities
  */
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
@@ -842,7 +872,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 			cxld = &cxlsd->cxld;
 		}
 
-		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
+		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base, info);
 		if (rc) {
 			put_device(&cxld->dev);
 			return rc;
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 97690c429e05..81882ea94adf 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -427,7 +427,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	 * Decoder Capability Enable.
 	 */
 	if (info->mem_enabled)
-		return -EBUSY;
+		return 0;
 
 	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
 	if (rc)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1057affb2db0..ea9548cbc7eb 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -644,7 +644,8 @@ struct cxl_endpoint_dvsec_info {
 
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
 int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
 			struct cxl_endpoint_dvsec_info *info);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 404639a1c3d0..7f1b71c5cf15 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -79,7 +79,7 @@ static int cxl_port_probe(struct device *dev)
 		}
 	}
 
-	rc = devm_cxl_enumerate_decoders(cxlhdm);
+	rc = devm_cxl_enumerate_decoders(cxlhdm, &info);
 	if (rc) {
 		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
 		return rc;

RE: [PATCH v3 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dan Williams]

Dave Jiang wrote:
> In the case where HDM decoder register block exists but is not programmed
> and at the same time the DVSEC range register range is active, populate the
> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v2:
> - Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
> - Skip HDM enabling if DVSEC range is active. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c |   36 +++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/pci.c |    2 +-
>  drivers/cxl/cxl.h      |    3 ++-
>  drivers/cxl/port.c     |    2 +-
>  4 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index dcc16d7cb8f3..af1f5f906f52 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -679,9 +679,34 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> +					    struct cxl_decoder *cxld, int which,
> +					    struct cxl_endpoint_dvsec_info *info)
> +{
> +	if (!is_cxl_endpoint(port))
> +		return -EOPNOTSUPP;
> +
> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> +		return -ENXIO;
> +
> +	cxld->target_type = CXL_DECODER_EXPANDER;
> +	cxld->commit = NULL;
> +	cxld->reset = NULL;
> +
> +	cxld->hpa_range = (struct range) {
> +		.start = info->dvsec_range[which].start,
> +		.end = info->dvsec_range[which].end,
> +	};

They're both 'struct range' values so this can just be an implict
memcpy:

	cxld->hpa_range = info->dvsec_range[which];

> +	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	port->commit_end = cxld->id;

I think F_LOCK should ultimately come from whether the associated CFMWS is locked,
but for now perhaps a comment like:

/*
 * Set the emulated decoder as locked pending additional support to
 * change the range registers at run time.
 */

...basically just to indicate that there is no requirement that they be
locked, but that the unlock case needs quite a bit more work and
testing.

> +
> +	return 0;
> +}
> +
>  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
> -			    u64 *dpa_base)
> +			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_endpoint_decoder *cxled = NULL;
>  	u64 size, base, skip, dpa_size;
> @@ -717,6 +742,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		.end = base + size - 1,
>  	};
>  
> +	if (cxled && !committed &&
> +	    info->dvsec_range[which].start != CXL_RESOURCE_NONE)

Ah, here is where that CXL_RESOURCE_NONE is used, can this condition
just be:

    range_len(&info->dvsec_range[which])

...i.e. non-zero size?

> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +
>  	/* decoders are enabled if committed */
>  	if (committed) {
>  		cxld->flags |= CXL_DECODER_F_ENABLE;
> @@ -790,7 +819,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>   * @cxlhdm: Structure to populate with HDM capabilities
>   */
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	struct cxl_port *port = cxlhdm->port;
> @@ -842,7 +872,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  			cxld = &cxlsd->cxld;
>  		}
>  
> -		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
> +		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base, info);

Looks like a long line for clang-format to trim.

>  		if (rc) {
>  			put_device(&cxld->dev);
>  			return rc;
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 97690c429e05..81882ea94adf 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -427,7 +427,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	 * Decoder Capability Enable.
>  	 */
>  	if (info->mem_enabled)
> -		return -EBUSY;
> +		return 0;
>  
>  	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  	if (rc)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1057affb2db0..ea9548cbc7eb 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -644,7 +644,8 @@ struct cxl_endpoint_dvsec_info {
>  
>  struct cxl_hdm;
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info);
>  int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
>  int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
>  			struct cxl_endpoint_dvsec_info *info);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 404639a1c3d0..7f1b71c5cf15 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -79,7 +79,7 @@ static int cxl_port_probe(struct device *dev)
>  		}
>  	}
>  
> -	rc = devm_cxl_enumerate_decoders(cxlhdm);
> +	rc = devm_cxl_enumerate_decoders(cxlhdm, &info);
>  	if (rc) {
>  		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
>  		return rc;
> 
> 

Re: [PATCH v3 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dave Jiang]

On 2/7/23 3:39 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> In the case where HDM decoder register block exists but is not programmed
>> and at the same time the DVSEC range register range is active, populate the
>> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v2:
>> - Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
>> - Skip HDM enabling if DVSEC range is active. (Jonathan)
>> ---
>>   drivers/cxl/core/hdm.c |   36 +++++++++++++++++++++++++++++++++---
>>   drivers/cxl/core/pci.c |    2 +-
>>   drivers/cxl/cxl.h      |    3 ++-
>>   drivers/cxl/port.c     |    2 +-
>>   4 files changed, 37 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index dcc16d7cb8f3..af1f5f906f52 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -679,9 +679,34 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>>   	return 0;
>>   }
>>   
>> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>> +					    struct cxl_decoder *cxld, int which,
>> +					    struct cxl_endpoint_dvsec_info *info)
>> +{
>> +	if (!is_cxl_endpoint(port))
>> +		return -EOPNOTSUPP;
>> +
>> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
>> +		return -ENXIO;
>> +
>> +	cxld->target_type = CXL_DECODER_EXPANDER;
>> +	cxld->commit = NULL;
>> +	cxld->reset = NULL;
>> +
>> +	cxld->hpa_range = (struct range) {
>> +		.start = info->dvsec_range[which].start,
>> +		.end = info->dvsec_range[which].end,
>> +	};
> 
> They're both 'struct range' values so this can just be an implict
> memcpy:
> 
> 	cxld->hpa_range = info->dvsec_range[which];
> 

ok

>> +	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
>> +	port->commit_end = cxld->id;
> 
> I think F_LOCK should ultimately come from whether the associated CFMWS is locked,
> but for now perhaps a comment like:
> 
> /*
>   * Set the emulated decoder as locked pending additional support to
>   * change the range registers at run time.
>   */
> 
ok

> ...basically just to indicate that there is no requirement that they be
> locked, but that the unlock case needs quite a bit more work and
> testing.
> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>   			    int *target_map, void __iomem *hdm, int which,
>> -			    u64 *dpa_base)
>> +			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>>   {
>>   	struct cxl_endpoint_decoder *cxled = NULL;
>>   	u64 size, base, skip, dpa_size;
>> @@ -717,6 +742,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>   		.end = base + size - 1,
>>   	};
>>   
>> +	if (cxled && !committed &&
>> +	    info->dvsec_range[which].start != CXL_RESOURCE_NONE)
> 
> Ah, here is where that CXL_RESOURCE_NONE is used, can this condition
> just be:
> 
>      range_len(&info->dvsec_range[which])
> 
> ...i.e. non-zero size?

In order to do this, I think we need to do
.start = 0,
.end = CXL_RESOURCE_NONE,

So range_len() == 0 in the end.

> 
>> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
>> +
>>   	/* decoders are enabled if committed */
>>   	if (committed) {
>>   		cxld->flags |= CXL_DECODER_F_ENABLE;
>> @@ -790,7 +819,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>    * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>>    * @cxlhdm: Structure to populate with HDM capabilities
>>    */
>> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>> +				struct cxl_endpoint_dvsec_info *info)
>>   {
>>   	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>>   	struct cxl_port *port = cxlhdm->port;
>> @@ -842,7 +872,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>>   			cxld = &cxlsd->cxld;
>>   		}
>>   
>> -		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
>> +		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base, info);
> 
> Looks like a long line for clang-format to trim.

ok

> 
>>   		if (rc) {
>>   			put_device(&cxld->dev);
>>   			return rc;
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 97690c429e05..81882ea94adf 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -427,7 +427,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>>   	 * Decoder Capability Enable.
>>   	 */
>>   	if (info->mem_enabled)
>> -		return -EBUSY;
>> +		return 0;
>>   
>>   	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>>   	if (rc)
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 1057affb2db0..ea9548cbc7eb 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -644,7 +644,8 @@ struct cxl_endpoint_dvsec_info {
>>   
>>   struct cxl_hdm;
>>   struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
>> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
>> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>> +				struct cxl_endpoint_dvsec_info *info);
>>   int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
>>   int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
>>   			struct cxl_endpoint_dvsec_info *info);
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 404639a1c3d0..7f1b71c5cf15 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -79,7 +79,7 @@ static int cxl_port_probe(struct device *dev)
>>   		}
>>   	}
>>   
>> -	rc = devm_cxl_enumerate_decoders(cxlhdm);
>> +	rc = devm_cxl_enumerate_decoders(cxlhdm, &info);
>>   	if (rc) {
>>   		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
>>   		return rc;
>>
>>
> 
> 

[PATCH v3 5/8] cxl: create emulated cxl_hdm for devices that do not have HDM decoders

[Dave Jiang]

CXL rev3 spec 8.1.3

RCDs may not have HDM register blocks. Create a fake HDM with information
from the CXL PCIe DVSEC registers. The decoder count will be set to the
HDM count retrieved from the DVSEC cap register.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v3:
- Move relevant changes from next patch in series. (Jonathan)
- Add kernel doc update. (Jonathan)
v2:
- Set target_count to same as number of ranges. (Jonathan)
---
 drivers/cxl/core/hdm.c |   58 ++++++++++++++++++++++++++++++++++++++++--------
 drivers/cxl/core/pci.c |    9 +++++--
 drivers/cxl/cxl.h      |    3 ++
 drivers/cxl/port.c     |    2 +-
 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index af1f5f906f52..86fe1be2e961 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -101,11 +101,34 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
 				      BIT(CXL_CM_CAP_CAP_ID_HDM));
 }
 
+static struct cxl_hdm *devm_cxl_setup_emulated_hdm(struct cxl_port *port,
+						   struct cxl_endpoint_dvsec_info *info)
+{
+	struct device *dev = &port->dev;
+	struct cxl_hdm *cxlhdm;
+
+	if (!info->mem_enabled)
+		return ERR_PTR(-ENODEV);
+
+	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
+	if (!cxlhdm)
+		return ERR_PTR(-ENOMEM);
+
+	cxlhdm->port = port;
+	cxlhdm->decoder_count = info->ranges;
+	cxlhdm->target_count = info->ranges;
+	dev_set_drvdata(&port->dev, cxlhdm);
+
+	return cxlhdm;
+}
+
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
+ * @info: cached DVSEC range register info
  */
-struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
+struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
+				   struct cxl_endpoint_dvsec_info *info)
 {
 	struct device *dev = &port->dev;
 	struct cxl_hdm *cxlhdm;
@@ -119,6 +142,9 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
 	cxlhdm->port = port;
 	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
 	if (!crb) {
+		if (info->mem_enabled)
+			return devm_cxl_setup_emulated_hdm(port, info);
+
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);
 	}
@@ -815,19 +841,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	return 0;
 }
 
-/**
- * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
- * @cxlhdm: Structure to populate with HDM capabilities
- */
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
-				struct cxl_endpoint_dvsec_info *info)
+static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	struct cxl_port *port = cxlhdm->port;
-	int i, committed;
-	u64 dpa_base = 0;
+	int committed, i;
 	u32 ctrl;
 
+	if (!hdm)
+		return;
+
 	/*
 	 * Since the register resource was recently claimed via request_region()
 	 * be careful about trusting the "not-committed" status until the commit
@@ -844,6 +866,22 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 	/* ensure that future checks of committed can be trusted */
 	if (committed != cxlhdm->decoder_count)
 		msleep(20);
+}
+
+/**
+ * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
+ * @cxlhdm: Structure to populate with HDM capabilities
+ * @info: cached DVSEC range register info
+ */
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	struct cxl_port *port = cxlhdm->port;
+	int i;
+	u64 dpa_base = 0;
+
+	cxl_settle_decoders(cxlhdm);
 
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
 		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 81882ea94adf..be59f7271f62 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -379,16 +379,19 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	struct cxl_port *port = cxlhdm->port;
 	struct cxl_port *root;
 	int i, rc, allowed;
-	u32 global_ctrl;
+	u32 global_ctrl = 0;
 
-	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	if (hdm)
+		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
 	/*
 	 * If the HDM Decoder Capability is already enabled then assume
 	 * that some other agent like platform firmware set it up.
 	 */
-	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
 		return devm_cxl_enable_mem(&port->dev, cxlds);
+	else if (!hdm)
+		return -ENODEV;
 
 	root = to_cxl_port(port->dev.parent);
 	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ea9548cbc7eb..0ec047cced90 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -643,7 +643,8 @@ struct cxl_endpoint_dvsec_info {
 };
 
 struct cxl_hdm;
-struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
+struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
+				   struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 7f1b71c5cf15..875bf45db4ad 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -55,7 +55,7 @@ static int cxl_port_probe(struct device *dev)
 			return devm_cxl_add_passthrough_decoder(port);
 	}
 
-	cxlhdm = devm_cxl_setup_hdm(port);
+	cxlhdm = devm_cxl_setup_hdm(port, &info);
 	if (IS_ERR(cxlhdm))
 		return PTR_ERR(cxlhdm);
 

RE: [PATCH v3 5/8] cxl: create emulated cxl_hdm for devices that do not have HDM decoders

[Dan Williams]

Dave Jiang wrote:
> CXL rev3 spec 8.1.3
> 
> RCDs may not have HDM register blocks. Create a fake HDM with information
> from the CXL PCIe DVSEC registers. The decoder count will be set to the
> HDM count retrieved from the DVSEC cap register.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v3:
> - Move relevant changes from next patch in series. (Jonathan)
> - Add kernel doc update. (Jonathan)
> v2:
> - Set target_count to same as number of ranges. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c |   58 ++++++++++++++++++++++++++++++++++++++++--------
>  drivers/cxl/core/pci.c |    9 +++++--
>  drivers/cxl/cxl.h      |    3 ++
>  drivers/cxl/port.c     |    2 +-
>  4 files changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index af1f5f906f52..86fe1be2e961 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -101,11 +101,34 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  				      BIT(CXL_CM_CAP_CAP_ID_HDM));
>  }
>  
> +static struct cxl_hdm *devm_cxl_setup_emulated_hdm(struct cxl_port *port,
> +						   struct cxl_endpoint_dvsec_info *info)
> +{
> +	struct device *dev = &port->dev;
> +	struct cxl_hdm *cxlhdm;
> +
> +	if (!info->mem_enabled)
> +		return ERR_PTR(-ENODEV);
> +
> +	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
> +	if (!cxlhdm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cxlhdm->port = port;
> +	cxlhdm->decoder_count = info->ranges;
> +	cxlhdm->target_count = info->ranges;
> +	dev_set_drvdata(&port->dev, cxlhdm);
> +
> +	return cxlhdm;
> +}
> +
>  /**
>   * devm_cxl_setup_hdm - map HDM decoder component registers
>   * @port: cxl_port to map
> + * @info: cached DVSEC range register info
>   */
> -struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
> +struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> +				   struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct device *dev = &port->dev;
>  	struct cxl_hdm *cxlhdm;
> @@ -119,6 +142,9 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
>  	cxlhdm->port = port;
>  	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
>  	if (!crb) {
> +		if (info->mem_enabled)
> +			return devm_cxl_setup_emulated_hdm(port, info);
> +
>  		dev_err(dev, "No component registers mapped\n");
>  		return ERR_PTR(-ENXIO);
>  	}
> @@ -815,19 +841,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	return 0;
>  }
>  
> -/**
> - * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
> - * @cxlhdm: Structure to populate with HDM capabilities
> - */
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> -				struct cxl_endpoint_dvsec_info *info)
> +static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> -	struct cxl_port *port = cxlhdm->port;
> -	int i, committed;
> -	u64 dpa_base = 0;
> +	int committed, i;
>  	u32 ctrl;
>  
> +	if (!hdm)
> +		return;

Nice, I like the semantic of a 'struct cxl_hdm' with NULL
regs.hdm_decoder == emulated.

This patch looks good to me.

[PATCH v3 6/8] cxl: create emulated decoders for devices without HDM decoders

[Dave Jiang]

CXL rev3.0 spec 8.1.3

RCDs may not have HDM register blocks. Create fake decoders based on CXL
PCIe DVSEC registers. The DVSEC Range Registers provide the memory range
for these decoder structs. For the RCD, there can be up to 2 decoders
depending on the DVSEC Capability register HDM_count.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v3:
- Drop unrelated bits from patch. (Jonathan)
---
 drivers/cxl/core/hdm.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 86fe1be2e961..cbec955db4c9 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -748,6 +748,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	if (is_endpoint_decoder(&cxld->dev))
 		cxled = to_cxl_endpoint_decoder(&cxld->dev);
 
+	if (!hdm) {
+		if (!cxled)
+			return -EINVAL;
+
+		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
+	}
+
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));

Re: [PATCH v3 6/8] cxl: create emulated decoders for devices without HDM decoders

[Jonathan Cameron]

On Wed, 18 Jan 2023 11:09:12 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL rev3.0 spec 8.1.3
> 
> RCDs may not have HDM register blocks. Create fake decoders based on CXL
> PCIe DVSEC registers. The DVSEC Range Registers provide the memory range
> for these decoder structs. For the RCD, there can be up to 2 decoders
> depending on the DVSEC Capability register HDM_count.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v3:
> - Drop unrelated bits from patch. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 86fe1be2e961..cbec955db4c9 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -748,6 +748,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
> +	if (!hdm) {
> +		if (!cxled)
> +			return -EINVAL;
> +
> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +	}
> +
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
>  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> 
> 

RE: [PATCH v3 6/8] cxl: create emulated decoders for devices without HDM decoders

[Dan Williams]

Dave Jiang wrote:
> CXL rev3.0 spec 8.1.3
> 
> RCDs may not have HDM register blocks. Create fake decoders based on CXL
> PCIe DVSEC registers. The DVSEC Range Registers provide the memory range
> for these decoder structs. For the RCD, there can be up to 2 decoders
> depending on the DVSEC Capability register HDM_count.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v3:
> - Drop unrelated bits from patch. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 86fe1be2e961..cbec955db4c9 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -748,6 +748,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
> +	if (!hdm) {
> +		if (!cxled)

I think this wants a dev_dbg() for calling out a failure to setup
hdm_decoder registers for a non-endpoint port. Other than that this
looks good.

[PATCH v3 7/8] cxl: Add emulation when HDM decoders are not committed

[Dave Jiang]

For the case where DVSEC range register(s) are active and HDM decoders are
not committed, use RR to provide emulation. A first pass is done to note
whether any decoders are committed. If there are no committed endpoint
decoders, then DVSEC ranges will be used for emulation.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   37 ++++++++++++++++++++++++++++++-------
 drivers/cxl/cxl.h      |    1 +
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index cbec955db4c9..32728f63084a 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -730,6 +730,31 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
 	return 0;
 }
 
+static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	u32 ctrl;
+	int i;
+
+	if (!is_cxl_endpoint(cxlhdm->port))
+		return false;
+
+	if (!hdm)
+		return true;
+
+	/*
+	 * If any decoders are committed already, there should not be any
+	 * emulated DVSEC decoders.
+	 */
+	for (i = 0; i < cxlhdm->decoder_count; i++) {
+		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
+		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
+			return false;
+	}
+
+	return true;
+}
+
 static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
 			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
@@ -745,16 +770,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		unsigned char target_id[8];
 	} target_list;
 
+	if (info->emulate_decoders)
+		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
+
 	if (is_endpoint_decoder(&cxld->dev))
 		cxled = to_cxl_endpoint_decoder(&cxld->dev);
 
-	if (!hdm) {
-		if (!cxled)
-			return -EINVAL;
-
-		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
-	}
-
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
@@ -890,6 +911,8 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 
 	cxl_settle_decoders(cxlhdm);
 
+	info->emulate_decoders = should_emulate_decoders(cxlhdm);
+
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
 		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
 		int rc, target_count = cxlhdm->target_count;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0ec047cced90..f1aa57a95150 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -640,6 +640,7 @@ struct cxl_endpoint_dvsec_info {
 	bool mem_enabled;
 	int ranges;
 	struct range dvsec_range[2];
+	bool emulate_decoders;
 };
 
 struct cxl_hdm;

RE: [PATCH v3 7/8] cxl: Add emulation when HDM decoders are not committed

[Dan Williams]

Dave Jiang wrote:
> For the case where DVSEC range register(s) are active and HDM decoders are
> not committed, use RR to provide emulation. A first pass is done to note
> whether any decoders are committed. If there are no committed endpoint
> decoders, then DVSEC ranges will be used for emulation.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c |   37 ++++++++++++++++++++++++++++++-------
>  drivers/cxl/cxl.h      |    1 +
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index cbec955db4c9..32728f63084a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -730,6 +730,31 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
> +{
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 ctrl;
> +	int i;
> +
> +	if (!is_cxl_endpoint(cxlhdm->port))
> +		return false;
> +
> +	if (!hdm)
> +		return true;
> +
> +	/*
> +	 * If any decoders are committed already, there should not be any
> +	 * emulated DVSEC decoders.
> +	 */
> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
>  			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
> @@ -745,16 +770,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		unsigned char target_id[8];
>  	} target_list;
>  
> +	if (info->emulate_decoders)
> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +

This has a code smell of letting program logic leak into a data
structure. I think ->emulate_decoders can be killed in favor of
something like:

if (should_emulate_decoders(port))
	return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);

...and do "cxlhdm = dev_get_drvdata(&port->dev)" inside
should_emulate_decoders().


>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
> -	if (!hdm) {
> -		if (!cxled)
> -			return -EINVAL;
> -
> -		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> -	}
> -

Doh, just delete the previous patch from the series at this point since
all that code is gone now.

[PATCH v3 8/8] cxl: remove locked check for dvsec_range_allowed()

[Dave Jiang]

There is no reason that the CFMWS will always set the "Fixed Device
Configuration" bit in the "Window Restrictions" field. Remove the
CXL_DECODER_F_LOCK check.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index be59f7271f62..e1aadb411f2f 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -228,8 +228,6 @@ static int dvsec_range_allowed(struct device *dev, void *arg)
 
 	cxld = to_cxl_decoder(dev);
 
-	if (!(cxld->flags & CXL_DECODER_F_LOCK))
-		return 0;
 	if (!(cxld->flags & CXL_DECODER_F_RAM))
 		return 0;
 

RE: [PATCH v3 8/8] cxl: remove locked check for dvsec_range_allowed()

[Dan Williams]

Dave Jiang wrote:
> There is no reason that the  CFMWS will always set the "Fixed Device
> Configuration" bit in the "Window Restrictions" field. Remove the
> CXL_DECODER_F_LOCK check.

This description says "what", but not "why". How about:

"Remove the CXL_DECODER_F_LOCK check to be permissive of platform BIOSen
that allow CXL.mem to be remapped."