Message ID | 168443110267.2957452.10368382491569241133.stgit@djiang5-mobl3 |
---|---|
State | New, archived |
Headers | show |
Series | ] cxl: Move operations after memory is ready | expand |
Dave Jiang wrote: > CXL rev3.0 8.1.3.8.2 Memory_Info_valid field > > The Memory_Info_Valid bit indicates that the CXL Range Size High and Size > Low registers are valid. The bit must be set within 1 second of reset > deassertion to the device. Check valid bit before we check the > Memory_Active bit when waiting for cxl_await_media_ready() to ensure that > the memory info is valid for consumption. Also ensures both DVSEC ranges > 1 and 2 are ready if DVSEC Capability indicates they are both supported. > > Fixes: 523e594d9cc0 ("cxl/pci: Implement wait for media active") > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v2: > - Check both ranges instead of just first offset. (Ira) > - Add to commit log. (Ira) > - Fix fixes tag. (Dan) > [snip] > + > +static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) > { > struct pci_dev *pdev = to_pci_dev(cxlds->dev); > int d = cxlds->cxl_dvsec; > bool active = false; > - u64 md_status; > int rc, i; > + u32 temp; > > - for (i = media_ready_timeout; i; i--) { > - u32 temp; > + if (id > CXL_DVSEC_RANGE_MAX) > + return -EINVAL; > > + /* Check MEM ACTIVE bit, up to 60s timeout by default */ > + for (i = media_ready_timeout; i; i--) { > rc = pci_read_config_dword( > - pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); > + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); I think this is still wrong. I think this should be 'id' shouldn't it? ... > if (rc) > return rc; > > @@ -134,6 +168,39 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) > return -ETIMEDOUT; > } > > + return 0; > +} > + > +/* > + * Wait up to @media_ready_timeout for the device to report memory > + * active. > + */ > +int cxl_await_media_ready(struct cxl_dev_state *cxlds) > +{ > + struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + int d = cxlds->cxl_dvsec; > + int rc, i, hdm_count; > + u64 md_status; > + u16 cap; > + > + rc = pci_read_config_word(pdev, > + d + CXL_DVSEC_CAP_OFFSET, &cap); > + if (rc) > + return rc; > + > + hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); > + for (i = 0; i < hdm_count; i++) { > + rc = cxl_dvsec_mem_range_valid(cxlds, i); > + if (rc) > + return rc; > + } > + > + for (i = 0; i < hdm_count; i++) { > + rc = cxl_dvsec_mem_range_active(cxlds, i); ... Based on this 'i'... Ira
On 5/18/23 12:01 PM, Ira Weiny wrote: > Dave Jiang wrote: >> CXL rev3.0 8.1.3.8.2 Memory_Info_valid field >> >> The Memory_Info_Valid bit indicates that the CXL Range Size High and Size >> Low registers are valid. The bit must be set within 1 second of reset >> deassertion to the device. Check valid bit before we check the >> Memory_Active bit when waiting for cxl_await_media_ready() to ensure that >> the memory info is valid for consumption. Also ensures both DVSEC ranges >> 1 and 2 are ready if DVSEC Capability indicates they are both supported. >> >> Fixes: 523e594d9cc0 ("cxl/pci: Implement wait for media active") >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> >> --- >> v2: >> - Check both ranges instead of just first offset. (Ira) >> - Add to commit log. (Ira) >> - Fix fixes tag. (Dan) >> > > [snip] > >> + >> +static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) >> { >> struct pci_dev *pdev = to_pci_dev(cxlds->dev); >> int d = cxlds->cxl_dvsec; >> bool active = false; >> - u64 md_status; >> int rc, i; >> + u32 temp; >> >> - for (i = media_ready_timeout; i; i--) { >> - u32 temp; >> + if (id > CXL_DVSEC_RANGE_MAX) >> + return -EINVAL; >> >> + /* Check MEM ACTIVE bit, up to 60s timeout by default */ >> + for (i = media_ready_timeout; i; i--) { >> rc = pci_read_config_dword( >> - pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); >> + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); > > I think this is still wrong. I think this should be 'id' shouldn't it? Yes. I was thinking of something else.... > ... > >> if (rc) >> return rc; >> >> @@ -134,6 +168,39 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) >> return -ETIMEDOUT; >> } >> >> + return 0; >> +} >> + >> +/* >> + * Wait up to @media_ready_timeout for the device to report memory >> + * active. >> + */ >> +int cxl_await_media_ready(struct cxl_dev_state *cxlds) >> +{ >> + struct pci_dev *pdev = to_pci_dev(cxlds->dev); >> + int d = cxlds->cxl_dvsec; >> + int rc, i, hdm_count; >> + u64 md_status; >> + u16 cap; >> + >> + rc = pci_read_config_word(pdev, >> + d + CXL_DVSEC_CAP_OFFSET, &cap); >> + if (rc) >> + return rc; >> + >> + hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); >> + for (i = 0; i < hdm_count; i++) { >> + rc = cxl_dvsec_mem_range_valid(cxlds, i); >> + if (rc) >> + return rc; >> + } >> + >> + for (i = 0; i < hdm_count; i++) { >> + rc = cxl_dvsec_mem_range_active(cxlds, i); > > ... Based on this 'i'... > > Ira
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 63f2f0b86fbc..9a38a6f91624 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -101,23 +101,57 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port) } EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, CXL); -/* - * Wait up to @media_ready_timeout for the device to report memory - * active. - */ -int cxl_await_media_ready(struct cxl_dev_state *cxlds) +static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id) +{ + struct pci_dev *pdev = to_pci_dev(cxlds->dev); + int d = cxlds->cxl_dvsec; + bool valid = false; + int rc, i; + u32 temp; + + if (id > CXL_DVSEC_RANGE_MAX) + return -EINVAL; + + /* Check MEM INFO VALID bit first, give up after 1s */ + i = 1; + do { + rc = pci_read_config_dword(pdev, + d + CXL_DVSEC_RANGE_SIZE_LOW(id), + &temp); + if (rc) + return rc; + + valid = FIELD_GET(CXL_DVSEC_MEM_INFO_VALID, temp); + if (valid) + break; + msleep(1000); + } while (i--); + + if (!valid) { + dev_err(&pdev->dev, + "Timeout awaiting memory range %d valid after 1s.\n", + id); + return -ETIMEDOUT; + } + + return 0; +} + +static int cxl_dvsec_mem_range_active(struct cxl_dev_state *cxlds, int id) { struct pci_dev *pdev = to_pci_dev(cxlds->dev); int d = cxlds->cxl_dvsec; bool active = false; - u64 md_status; int rc, i; + u32 temp; - for (i = media_ready_timeout; i; i--) { - u32 temp; + if (id > CXL_DVSEC_RANGE_MAX) + return -EINVAL; + /* Check MEM ACTIVE bit, up to 60s timeout by default */ + for (i = media_ready_timeout; i; i--) { rc = pci_read_config_dword( - pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp); if (rc) return rc; @@ -134,6 +168,39 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds) return -ETIMEDOUT; } + return 0; +} + +/* + * Wait up to @media_ready_timeout for the device to report memory + * active. + */ +int cxl_await_media_ready(struct cxl_dev_state *cxlds) +{ + struct pci_dev *pdev = to_pci_dev(cxlds->dev); + int d = cxlds->cxl_dvsec; + int rc, i, hdm_count; + u64 md_status; + u16 cap; + + rc = pci_read_config_word(pdev, + d + CXL_DVSEC_CAP_OFFSET, &cap); + if (rc) + return rc; + + hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap); + for (i = 0; i < hdm_count; i++) { + rc = cxl_dvsec_mem_range_valid(cxlds, i); + if (rc) + return rc; + } + + for (i = 0; i < hdm_count; i++) { + rc = cxl_dvsec_mem_range_active(cxlds, i); + if (rc) + return rc; + } + md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); if (!CXLMDEV_READY(md_status)) return -EIO; diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index 0465ef963cd6..7c02e55b8042 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -31,6 +31,8 @@ #define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) #define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) +#define CXL_DVSEC_RANGE_MAX 2 + /* CXL 2.0 8.1.4: Non-CXL Function Map DVSEC */ #define CXL_DVSEC_FUNCTION_MAP 2