[nvmewin] patch review status
Karthik_Rajagopalan at Dell.com
Karthik_Rajagopalan at Dell.com
Wed Aug 15 16:11:08 PDT 2012
Why not unconditionally set CC.EN to 0 as soon as the driver loads ?
- There will be an issue with this iff someone before (opROM, UEFI ?) had set CC.EN to 1 & somehow STS.RDY never went to 1... but things still continued (unlikely)
i.e.,
/* Disable / Reset */
CC.EN = 0;
StorPortWriteRegisterUlong(pAE,
(PULONG)(&pAE->pCtrlRegister->CC),
CC.AsUlong);
/* Set EN to 1 (all others zero) */
CC.EN = 1;
StorPortWriteRegisterUlong(pAE,
(PULONG)(&pAE->pCtrlRegister->CC),
CC.AsUlong);
/* Wait for RDY */
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 5:59 PM
To: Verma, Vishal L; Chang, Alex; nvmewin at lists.openfabrics.org
Subject: Re: [nvmewin] patch review status
So maybe a better proposal for this section of code... go back to what we had before (sorta) but include a wait on RDY after setting EN to 1 and only set EN to 1 if its zero to start. This is the most generic soln and since the behavior of the HW at this detailed level isn't specified we can call this the least common denominator and if IHVs want to optimize for their implementations they can. If this first RDY wait files, then we fail driver init. This code assures we always reset and we never touch EN after setting it to 1 without waiting for RDY
IF EN == 0 {
/* Set EN to 1 (all others zero) */
CC.EN = 1;
StorPortWriteRegisterUlong(pAE,
(PULONG)(&pAE->pCtrlRegister->CC),
CC.AsUlong);
WAIT ON RDY
}
/* Now reset */
CC.EN = 0;
StorPortWriteRegisterUlong(pAE,
(PULONG)(&pAE->pCtrlRegister->CC),
CC.AsUlong);
From: Luse, Paul E
Sent: Wednesday, August 15, 2012 3:21 PM
To: Verma, Vishal L; Chang, Alex; nvmewin at lists.openfabrics.org
Subject: RE: [nvmewin] patch review status
I was thinking about that too... If EN was already 1 and we caught RDY before it was 0 then we would indeed set EN to 1 again (possibly before, during or after RDY is set). The spec, of course, does not dictate behavior in this case but one would assume it would be treated as a NOP and our init state machine would properly wait for RDY. I'm not a HW guy though, anyone else have a comment/concern on setting EN to 1 if tis already 1?
From: Verma, Vishal L
Sent: Wednesday, August 15, 2012 1:46 PM
To: Luse, Paul E; Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] patch review status
Paul,
Is this correct though?
Making CC.EN 0 implies that RDY will become 0, but RDY being 0 does not guarantee that CC.EN is 0.
If what I'm thinking is right, we could be in a state where CC.EN was 1 when the driver comes up, but the controller never became ready. In this case we will think everything is alright, and we will try to make CC.EN 1 again - Which is not something we'd want to do, right?
Vishal
From: <Luse>, Paul E <paul.e.luse at intel.com<mailto:paul.e.luse at intel.com>>
Date: Wednesday, August 15, 2012 11:43 AM
To: "Chang, Alex" <Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>>, "nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>" <nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>>
Subject: Re: [nvmewin] 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]
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/5db39c9f/attachment.html>
More information about the nvmewin
mailing list