[nvmewin] patch review status

Chang, Alex Alex.Chang at idt.com
Wed Aug 15 10:20:48 PDT 2012


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]
Sent: Wednesday, August 15, 2012 9:56 AM
To: Chang, Alex; 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]
Sent: Wednesday, August 15, 2012 9:52 AM
To: Luse, Paul E; 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/ef52b0e2/attachment.html>


More information about the nvmewin mailing list