On Tue, 10 Nov 2020 21:43:53 -0800
Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
> Provide enough functionality to utilize the mailbox of a memory device.
> The mailbox is used to interact with the firmware running on the memory
> device.
>
> The CXL specification defines separate capabilities for the mailbox and the
> memory device. While we can confirm the mailbox is ready, in order to actually
> interact with the memory device, the driver must also confirm the device's
> firmware is ready.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
Hi Ben,
A few minor suggestions.
J
> ---
> drivers/cxl/cxl.h | 28 ++++++++++++++++++++++
> drivers/cxl/mem.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 02858ae63d6d..482fc9cdc890 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -19,14 +19,41 @@
> #define CXLDEV_MB_CAPS 0x00
> #define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F)
> #define CXLDEV_MB_CTRL 0x04
> +#define CXLDEV_MB_CTRL_DOORBELL BIT(0)
> #define CXLDEV_MB_CMD 0x08
> #define CXLDEV_MB_STATUS 0x10
> #define CXLDEV_MB_BG_CMD_STATUS 0x18
>
> +/* Memory Device */
> +#define CXLMDEV_STATUS 0
As mentioned earlier, I'd make sure there is a clear way of telling what is a
register/memory offset and what is a field.
> +#define CXLMDEV_DEV_FATAL BIT(0)
> +#define CXLMDEV_FW_HALT BIT(1)
> +#define CXLMDEV_MEDIA_STATUS_SHIFT 2
> +#define CXLMDEV_MEDIA_STATUS_MASK 0x3
Problem with masks defined like this is it's not apparent from name if they
are pre or post shift. Could we use FIELD_GET etc and GENMASK to
ensure there is only one unambiguous definition?
> +#define CXLMDEV_READY(status) \
> + ((((status) >> CXLMDEV_MEDIA_STATUS_SHIFT) & \
> + CXLMDEV_MEDIA_STATUS_MASK) == CXLMDEV_MS_READY)
> +#define CXLMDEV_MS_NOT_READY 0
> +#define CXLMDEV_MS_READY 1
> +#define CXLMDEV_MS_ERROR 2
> +#define CXLMDEV_MS_DISABLED 3
> +#define CXLMDEV_MBOX_IF_READY BIT(4)
> +#define CXLMDEV_RESET_NEEDED_SHIFT 5
> +#define CXLMDEV_RESET_NEEDED_MASK 0x7
> +#define CXLMDEV_RESET_NEEDED(status) \
> + (((status) >> CXLMDEV_RESET_NEEDED_SHIFT) & CXLMDEV_RESET_NEEDED_MASK)
> +#define CXLMDEV_RESET_NEEDED_NOT 0
> +#define CXLMDEV_RESET_NEEDED_COLD 1
> +#define CXLMDEV_RESET_NEEDED_WARM 2
> +#define CXLMDEV_RESET_NEEDED_HOT 3
> +#define CXLMDEV_RESET_NEEDED_CXL 4
> +
> struct cxl_mem {
> struct pci_dev *pdev;
> void __iomem *regs;
>
> + spinlock_t mbox_lock; /* Protects device mailbox and firmware */
> +
> /* Cap 0000h */
> struct {
> void __iomem *regs;
> @@ -72,6 +99,7 @@ struct cxl_mem {
>
> cxl_reg(status)
> cxl_reg(mbox)
> +cxl_reg(mem)
>
> static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg)
> {
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 4109ef7c3ecb..9fd2d1daa534 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -7,6 +7,56 @@
> #include "pci.h"
> #include "cxl.h"
>
> +static int cxl_mem_mbox_get(struct cxl_mem *cxlm)
> +{
> + u64 md_status;
> + u32 ctrl;
> + int rc = -EBUSY;
> +
> + spin_lock(&cxlm->mbox_lock);
> +
> + ctrl = cxl_read_mbox_reg32(cxlm, CXLDEV_MB_CTRL);
> + if (ctrl & CXLDEV_MB_CTRL_DOORBELL)
Perhaps a comment on what this path means? I know from the spec, but
not super obvious from the code here. If we do hit this path the device
will fail to come up and we won't have a clue why.
> + goto out;
> +
> + md_status = cxl_read_mem_reg64(cxlm, CXLMDEV_STATUS);
> + if (md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status)) {
> + /*
> + * Hardware shouldn't allow a ready status but also have failure
> + * bits set. Spit out an error, this should be a bug report
> + */
> + if (md_status & CXLMDEV_DEV_FATAL) {
> + dev_err(&cxlm->pdev->dev,
> + "CXL device reporting ready and fatal\n");
> + rc = -EFAULT;
> + goto out;
> + }
> + if (md_status & CXLMDEV_FW_HALT) {
> + dev_err(&cxlm->pdev->dev,
> + "CXL device reporting ready and halted\n");
> + rc = -EFAULT;
> + goto out;
> + }
> + if (CXLMDEV_RESET_NEEDED(md_status)) {
> + dev_err(&cxlm->pdev->dev,
> + "CXL device reporting ready and reset needed\n");
> + rc = -EFAULT;
> + goto out;
> + }
> +
> + return 0;
> + }
> +
> +out:
> + spin_unlock(&cxlm->mbox_lock);
> + return rc;
> +}
> +
> +static void cxl_mem_mbox_put(struct cxl_mem *cxlm)
> +{
> + spin_unlock(&cxlm->mbox_lock);
> +}
> +
> static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> {
> u64 cap_array;
> @@ -88,6 +138,8 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo, u32 reg_
> return ERR_PTR(-ENOMEM);
> }
>
> + spin_lock_init(&cxlm->mbox_lock);
> +
> regs = pcim_iomap_table(pdev)[bar];
> cxlm->pdev = pdev;
> cxlm->regs = regs + offset;
> @@ -160,6 +212,13 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> + /* Check that hardware "looks" okay. */
> + rc = cxl_mem_mbox_get(cxlm);
> + if (rc)
> + return rc;
> +
> + cxl_mem_mbox_put(cxlm);
> + dev_dbg(&pdev->dev, "CXL Memory Device Interface Up\n");
> pci_set_drvdata(pdev, cxlm);
>
> return 0;
