Skip to contentSkip to conversation

nvme: Quirks for PM1725 controllers

PATCH New Martin K. Petersen 3 participants 5 messages Jun 28, 2017 – Jul 1, 2017 NVMe

lore
Share & tools
lore

Martin K. Petersen

author
#0 lore raw

PATCH 1/1 nvme: Quirks for PM1725 controllers

PM1725 controllers have a couple of quirks that need to be handled in the driver:

- I/O queue depth must be limited to 64 entries on controllers that do not report MQES.

- The host interface registers go offline briefly while resetting the chip. Thus a delay is needed before checking whether the controller is ready.

Note that the admin queue depth is also limited to 64 on older versions of this board. Since our NVME_AQ_DEPTH is now 32 that is no longer an issue.

Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>
---
 drivers/nvme/host/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/nvme/host/pci.c +10 -0
index 32a98e2740ad..343263bcb49a 100644
@@ -1908,6 +1908,12 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 		dev_warn(dev->ctrl.device, "detected Apple NVMe controller, "
 			"set queue depth=%u to work around controller resets\n",
 			dev->q_depth);
+	} else if (pdev->vendor == PCI_VENDOR_ID_SAMSUNG &&
+		   (pdev->device == 0xa821 || pdev->device == 0xa822) &&
+		   NVME_CAP_MQES(cap) == 0) {
+		dev->q_depth = 64;
+		dev_err(dev->ctrl.device, "detected PM1725 NVMe controller, "
+                        "set queue depth=%u\n", dev->q_depth);
 	}
 
 	/*

@@ -2454,6 +2460,10 @@ static const struct pci_device_id nvme_id_table[] = {
 		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
 	{ PCI_DEVICE(0x1c5f, 0x0540),	/* Memblaze Pblaze4 adapter */
 		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
+	{ PCI_DEVICE(0x144d, 0xa821),   /* Samsung PM1725 */
+		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
+	{ PCI_DEVICE(0x144d, 0xa822),   /* Samsung PM1725a */
+		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
Signature · 1 line
2.13.0
maintainer
#1 lore raw

PATCH 1/1 nvme: Quirks for PM1725 controllers

Quoted 6 lines

PM1725 controllers have a couple of quirks that need to be handled in the driver:

- I/O queue depth must be limited to 64 entries on controllers that do not report MQES.

I think this can be a new quirk (NVME_QUIRK_QD_LIMIT_64 or something)

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1913

Quoted 7 lines
drivers/nvme/host/pci.c +3 -0
@@ -1908,6 +1908,12 @@ static int nvme_pci_enable(struct nvme_dev *dev)
  		dev_warn(dev->ctrl.device, "detected Apple NVMe controller, "
  			"set queue depth=%u to work around controller resets\n",
  			dev->q_depth);
+	} else if (pdev->vendor == PCI_VENDOR_ID_SAMSUNG &&
+		   (pdev->device == 0xa821 || pdev->device == 0xa822) &&
+		   NVME_CAP_MQES(cap) == 0) {

and the this is:

	} else if (dev->ctrl.quirks & NVME_QUIRK_QD_LIMIT_64) {

While its inside the pci driver, I think it would be cleaner to use the quirks mechanism.

Kieth? thoughts?

#2 lore raw

PATCH 1/1 nvme: Quirks for PM1725 controllers

Quoted 19 lines · Sagi Grimberg
Quoted 6 lines

PM1725 controllers have a couple of quirks that need to be handled in the driver:

- I/O queue depth must be limited to 64 entries on controllers that do not report MQES.

I think this can be a new quirk (NVME_QUIRK_QD_LIMIT_64 or something)

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1913

Quoted 7 lines
drivers/nvme/host/pci.c +3 -0
@@ -1908,6 +1908,12 @@ static int nvme_pci_enable(struct nvme_dev *dev)
  		dev_warn(dev->ctrl.device, "detected Apple NVMe controller, "
  			"set queue depth=%u to work around controller resets\n",
  			dev->q_depth);
+	} else if (pdev->vendor == PCI_VENDOR_ID_SAMSUNG &&
+		   (pdev->device == 0xa821 || pdev->device == 0xa822) &&
+		   NVME_CAP_MQES(cap) == 0) {

and the this is:

Quoted 6 lines
	} else if (dev->ctrl.quirks & NVME_QUIRK_QD_LIMIT_64) {

While its inside the pci driver, I think it would be cleaner to use the quirks mechanism.

Kieth? thoughts?

Yeah, that's better than VID:DID checks. We only get 32 defined quirks using driver_data, and while we currently can spare 2 of them for q-depth quirks, I was hoping to use these bits more sparingly.

author
#3 lore raw

PATCH 1/1 nvme: Quirks for PM1725 controllers

Keith,

Quoted 4 lines
Quoted 4 lines

and the this is:

	} else if (dev->ctrl.quirks & NVME_QUIRK_QD_LIMIT_64) {
Quoted 8 lines
Quoted 4 lines

While its inside the pci driver, I think it would be cleaner to use the quirks mechanism.

Kieth? thoughts?

Yeah, that's better than VID:DID checks. We only get 32 defined quirks using driver_data, and while we currently can spare 2 of them for q-depth quirks, I was hoping to use these bits more sparingly.

That's fine. I did tinker with quirks for the queue depth at some point but I think I had problems due to needing to clamp the admin queue depth as well. I had to shuffle a bunch of stuff and introduce wrappers for accessing the AQ depth. But with Sagi's recent patch that's no longer necessary. So most of that churn went away when I rebased yesterday.

Will resend.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author
#4 lore raw

PATCH 1/1 nvme: Quirks for PM1725 controllers

In reply to Martin K. Petersen
Quoted 7 lines

PM1725 controllers have a couple of quirks that need to be handled in the driver:

- I/O queue depth must be limited to 64 entries on controllers that do not report MQES.

Quoted 7 lines

- The host interface registers go offline briefly while resetting the chip. Thus a delay is needed before checking whether the controller is ready.

Note that the admin queue depth is also limited to 64 on older versions of this board. Since our NVME_AQ_DEPTH is now 32 that is no longer an issue.

Discussed this with Keith privately and he prefers the current non-quirks approach to limiting the queue depth.

Thus no changes needed, please review/apply.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering