[nvmewin] patch review status

Robles, Raymond C raymond.c.robles at intel.com
Wed Aug 15 11:09:12 PDT 2012 

Sounds good.

Alex, please wait for the push of Paul's changes before you send out your patch review request so that you can rebase his changes.

Thanks,
Ray

From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Luse, Paul E
Sent: Wednesday, August 15, 2012 10:50 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org
Subject: Re: [nvmewin] patch review status

So I heard from Rick and Alex I assume you're OK with the 2 small changes below.  I'll go ahead and make the changes and do some regression and get this to Ray for pushing later this week.  Next up... Alex's sets of changes

Thanks Everyone
Paul

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 10:47 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Thanks, Paul.

Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 10:43 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status
OK, I think I can get behind that.  I was thinking STS might not be valid when EN is 0 but looking at the spec I think this is fine

Ready (RDY): This field is set to '1' when the controller is ready to process commands after CC.EN is set to '1'. This field shall be cleared to '0' when CC.EN is cleared to '0'. Commands shall not be issued to the controller until this field is set to '1' after the CC.EN bit is set to '1'. Failure to follow this requirement produces undefined results. Host software shall wait a minimum of CAP.TO seconds for this field to be set to '1' after setting CC.EN to '1' from a previous value of '0'.

So I'll just chance the check below for EN == 1 to RDY == 1 then

Thanks!
Paul

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 10:21 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Absolutely. Now, the tricky part is when CC.EN=1 and STS.RDY never becomes 1. I'd say simply check STS.RDY only. If it's 1, then reset the device, otherwise, skip resetting it. What do you think?

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 9:56 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status
OK, I see what you're saying I think.   We don't want to check for RDY without first looking at EN so does this make sense:


    if (CC.EN == 1) {

                -- now wait for RDY -

        /* Now reset */
        CC.EN = 0;
        StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    }


From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 9:52 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Hi Paul,

I agree with the new logic on #3. However, instead of checking CC.EN, should we check STS.RDY or both?

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 9:03 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status
Thanks Alex, see below.

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 8:51 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Hi Paul,

Some questions after browsing the difference between current source and your changes:
1. Why you removed "CC.SHN  = 1;" in NVMeNormalShutdown routine?
PL>  Good eye, that was a mistake.  I'll add it back before pushing

2. In nvme.h, why you changed the opcode of NVM_DATASET_MANAGEMENT from 0x06 to 0x09?
PL>    6 was wrong, 9 is correct per spec

3. Is there any specific reason(s), you added the following in NVMeInitialize, and then immediately clear EN without checking RDY = 1?
    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
PL>    I wrote this up in the patch submission and I can't find that email right now.  I didn't add code, I changed code.  Before, we use to set EN to 1, read it back, and then set it to 0 to kick off a reset and then later we wait for RDY in the init state machine.  We had an IHV complain that their implementation required waiting on RDY after setting EN 1 to even before clearing EN.  So, our old implementation was a problem.  So what I did now is to not force a reset unless EN is set to 1 when we come in.  If its set to 0 then either (a) we're coming from power on or (b) the pre_OS reset the card when it was done.  Either way if its already 0 then we're in a known state so we do nothing and then later we set to 1 and wait on RDY.  If its 1 when we enter here we set to 0 and then later set to 1 and wait on RDY.  Make sense?  This assures we never enable without waiting for RDY

***** WAS *******
    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    /* Read it back */
    CC.AsUlong =
        StorPortReadRegisterUlong(pAE, (PULONG)(&pAE->pCtrlRegister->CC));

    /* Now reset */
    CC.EN = 0;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

    CC.AsUlong =
        StorPortReadRegisterUlong(pAE, (PULONG)(&pAE->pCtrlRegister->CC));

*** NOW IS *******
    if (CC.EN == 1) {
        /* Now reset */
        CC.EN = 0;
        StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    }


Thanks,
Alex

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Luse, Paul E
Sent: Wednesday, August 15, 2012 8:12 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] patch review status
Just checking in... any questions to date and how are folks tracking for completing their review by this Fri?

Thx
Paul

____________________________________
Paul Luse
Sr. Staff Engineer
PCG Server Software Engineering
Desk: 480.554.3688, Mobile: 480.334.4630

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120815/4827153e/attachment.html>

More information about the nvmewin mailing list