NVMe driver init sequence

[Warner Losh]

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> wrote:

> On Fri, Aug 18, 2017 at 2:04 PM, Kinjal Patel
> <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
> > 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]
> 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>
> > 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>>
> 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> mailing
> list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
> > To unsubscribe, send any mail to "freebsd-drivers-unsubscribe@
> freebsd.org<mailto:freebsd-drivers-unsubscribe at freebsd.org>"
> >
> > _______________________________________________
> > freebsd-drivers at freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
> > To unsubscribe, send any mail to "freebsd-drivers-unsubscribe@
> freebsd.org"
>