[PATCH] nvme: Quirks for PM1725 controllers

[PATCH] nvme: Quirks for PM1725 controllers

[Martin K. Petersen]

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(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 32a98e2740ad..343263bcb49a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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) },
-- 
2.13.0

[PATCH] nvme: Quirks for PM1725 controllers

[Sagi Grimberg]

> 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)

> @@ -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?

[PATCH] nvme: Quirks for PM1725 controllers

[Keith Busch]

On Wed, Jun 28, 2017@10:27:09AM +0300, Sagi Grimberg wrote:
> 
> > 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)
> 
> > @@ -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?

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.

[PATCH] nvme: Quirks for PM1725 controllers

[Martin K. Petersen]

Keith,

>> 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?
>
> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] nvme: Quirks for PM1725 controllers

[Martin K. Petersen]

> 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.

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.

-- 
Martin K. Petersen	Oracle Linux Engineering