timing lookup table typos?

[cmu-safari]

timing lookup table typos?

[arthasSin]

I think you meant the .cpp files DDR4.cpp and DDR3.cpp. Yes, seems like you are right about the typos. It should definitely be WR-to-WR timing as RD-to-WR timing is defined later. We will get it fixed. Thanks for the heads up.

[hmhmhey]

@arthasSin
It seems there's another typo in DDR4.cpp. I made some comments starting with 'hmhmhey'.

/*** Bank Group ***/ 
t = timing[int(Level::BankGroup)];
// CAS <-> CAS
t[int(Command::RD)].push_back({Command::RD, 1, s.nCCDL});
t[int(Command::RD)].push_back({Command::RDA, 1, s.nCCDL});
t[int(Command::RDA)].push_back({Command::RD, 1, s.nCCDL});
t[int(Command::RDA)].push_back({Command::RDA, 1, s.nCCDL});

t[int(Command::WR)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WR)].push_back({Command::WRA, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WRA, 1, s.nCCDL});

// hmhmhey: looks like this part should define RD > WR
t[int(Command::WR)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WR)].push_back({Command::WRA, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WRA, 1, s.nCCDL});

t[int(Command::WR)].push_back({Command::RD, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WR)].push_back({Command::RDA, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WRA)].push_back({Command::RD, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WRA)].push_back({Command::RDA, 1, s.nCWL + s.nBL + s.nWTRL});

I'd appreciate it if you could check whether the following code is correct.

/*** Bank Group ***/ 
t = timing[int(Level::BankGroup)];
// CAS <-> CAS
t[int(Command::RD)].push_back({Command::RD, 1, s.nCCDL});
t[int(Command::RD)].push_back({Command::RDA, 1, s.nCCDL});
t[int(Command::RDA)].push_back({Command::RD, 1, s.nCCDL});
t[int(Command::RDA)].push_back({Command::RDA, 1, s.nCCDL});

t[int(Command::WR)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WR)].push_back({Command::WRA, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WR, 1, s.nCCDL});
t[int(Command::WRA)].push_back({Command::WRA, 1, s.nCCDL});

// hmhmhey: this part!   
t[int(Command::RD)].push_back({Command::WR, 1, s.nCL + s.nCCDL + 2 - s.nCWL});
t[int(Command::RD)].push_back({Command::WRA, 1, s.nCL + s.nCCDL + 2 - s.nCWL});
t[int(Command::RDA)].push_back({Command::WR, 1, s.nCL + s.nCCDL + 2 - s.nCWL});
t[int(Command::RDA)].push_back({Command::WRA, 1, s.nCL + s.nCCDL + 2 - s.nCWL});

t[int(Command::WR)].push_back({Command::RD, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WR)].push_back({Command::RDA, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WRA)].push_back({Command::RD, 1, s.nCWL + s.nBL + s.nWTRL});
t[int(Command::WRA)].push_back({Command::RDA, 1, s.nCWL + s.nBL + s.nWTRL});

[arthasSin]

@hmhmhey yes, we are aware of that issue. I will go over all timing parameters soon to make sure everything is in order. Your changes make sense but I am not sure if it is entirely correct. I will need to check the DDR4 specification. Thanks.

[RSpliet]

@hmhmhey Your proposed change is incorrect. The correct timing constraints are defined at the rank level, and hold regardless of whether you hit the same or a different bank group (because tCL is trivially larger than both tCCDS and tCCDL). At the rank level, technically tCCDS needs to be replaced with tBL. In practice you'll find that the two are both equal to 4 for DDR4. tCCDS is lower bound by tBL, and any higher value for tCCDS implies the clock frequency is poorly chosen.

I have opened a pull request (#57) that corrects several timing issues in DDR4. One of the patches addresses the issue you pointed out by removing the four items wholesale. Another patch corrects the definition of RD->WR issue latency by replacing tCCDS with tBL. Feel free to check out my DDR4-fix branch if you wish to test my adjustments.

[RSpliet]

Pull request #57 has just been merged, meaning @hmhmhey's issue has been resolved. As for the original issue posted by @stevekulick , my current experimental set-up has only a single rank. As such, I am unable to test or verify any changes in that area.