[nvmewin] patch review status

[Luse, Paul E]

Thanks Alex, see below.

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Wednesday, August 15, 2012 8:51 AM
To: Luse, Paul E; 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/c51dcee2/attachment.html>