NVMe driver init sequence

[Kinjal Patel]

Great!
Thanks Warner.


Regards,
Kinjal Patel

From: wlosh at bsdimp.com [mailto:wlosh at bsdimp.com] On Behalf Of Warner Losh
Sent: Thursday, August 24, 2017 8:17 PM
To: Jim Harris
Cc: Kinjal Patel; freebsd-drivers at freebsd.org
Subject: Re: NVMe driver init sequence

Greetings,

I've just committed r322872 which implements this, with a slightly tweaked comment.

Warner

On Fri, Aug 18, 2017 at 4:09 PM, Jim Harris <jim.harris at gmail.com<mailto:jim.harris at gmail.com>> wrote:
On Fri, Aug 18, 2017 at 2:04 PM, Kinjal Patel
<Kinjal.Patel at taec.toshiba.com<mailto:Kinjal.Patel at taec.toshiba.com>> wrote:
> Created a bug #22166 (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=221616)
> Updated the description and patch to it.
>
> Regards,
> Kinjal Patel
>
> From: Kinjal Patel
> Sent: Friday, August 18, 2017 1:51 PM
> To: 'Warner Losh'
> Cc: freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org>
> Subject: RE: NVMe driver init sequence
>
> Here is a the change I propose. This will make the init sequence right.

Looks good to me.

>
> Index: sys/dev/nvme/nvme.c
> ===================================================================
> --- sys/dev/nvme/nvme.c (revision 322672)
> +++ sys/dev/nvme/nvme.c (working copy)
> @@ -253,6 +253,9 @@
>                 return (status);
>         }
>
> +    /* make device bus-master before attempting to enable controller */
> +    pci_enable_busmaster(dev);
> +
>         /*
>          * Reset controller twice to ensure we do a transition from cc.en==1
>          *  to cc.en==0.  This is because we don't really know what status
> @@ -270,8 +273,6 @@
>                 return (status);
>         }
>
> -       pci_enable_busmaster(dev);
> -
>         ctrlr->config_hook.ich_func = nvme_ctrlr_start_config_hook;
>         ctrlr->config_hook.ich_arg = ctrlr;
>
>
>
> regards,
> Kinjal Patel
>
> From: wlosh at bsdimp.com<mailto:wlosh at bsdimp.com><mailto:wlosh at bsdimp.com<mailto:wlosh at bsdimp.com>> [mailto:wlosh at bsdimp.com<mailto:wlosh at bsdimp.com>] On Behalf Of Warner Losh
> Sent: Thursday, August 17, 2017 3:10 PM
> To: Kinjal Patel
> Cc: freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org><mailto:freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org>>
> Subject: Re: NVMe driver init sequence
>
> This sounds right to me....  Most BIOSes seem to enable BME=1.
>
> Warner
>
> On Thu, Aug 17, 2017 at 2:45 PM, Kinjal Patel <Kinjal.Patel at taec.toshiba.com<mailto:Kinjal.Patel at taec.toshiba.com><mailto:Kinjal.Patel at taec.toshiba.com<mailto:Kinjal.Patel at taec.toshiba.com>>> wrote:
> Hi,
>
> I have a query on NVMe driver init sequence in FreeBSD 10.3(stable).
>
> The driver init sequence is,
>
> 1)      Enable controller (CC.EN=1)
>
> 2)      Wait for controller ready (CSTS.RDY=1)
>
> 3)      Set PCI bus master enable (BME=1)
>
> As per NVMe spec, when NVMe controller becomes ready it has to process command.
>
> "Enable (EN): When set to '1', then the controller shall process commands based on Submission Queue Tail doorbell writes"
>
> And per PCI Express spec when BME is not set, the PCI Express Function is not allowed to issue any Memory or I/O requests.
> "Bus Master Enable - Controls the ability of a PCI Express Endpoint to issue Memory95 and I/O Read/Write Requests, and
> the ability of a Root or Switch Port to forward Memory and I/O Read/Write Requests in the Upstream direction"
>
> Enabling controller before setting BME=1 is violation of spec, as controller has to accept commands but BME is prerequisite for that.
>
> The Linux device driver init sequence is,
>
> 1)     Set PCI bus master enable (BME=1)
>
> 2)     Enable Controller (CC.EN=1)
>
> 3)     Wait for controller ready (CSTS.RDY=1)
>
> The FreeBSD NVMe driver sequence should be changed to set BME=1 before attempting to enable controller.
>
>
> Regards,
> Kinjal Patel
> _______________________________________________
> freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org><mailto:freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org>> mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
> To unsubscribe, send any mail to "freebsd-drivers-unsubscribe at freebsd.org<mailto:freebsd-drivers-unsubscribe at freebsd.org><mailto:freebsd-drivers-unsubscribe at freebsd.org<mailto:freebsd-drivers-unsubscribe at freebsd.org>>"
>
> _______________________________________________
> freebsd-drivers at freebsd.org<mailto:freebsd-drivers at freebsd.org> mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
> To unsubscribe, send any mail to "freebsd-drivers-unsubscribe at freebsd.org<mailto:freebsd-drivers-unsubscribe at freebsd.org>"