rkrajnc / minimig-mist

Minimig for the MiST board
GNU General Public License v3.0
63 stars 40 forks source link

Wrong refresh counter value in SDRAM controller #63

Closed sonycman closed 7 years ago

sonycman commented 7 years ago

Sdram controller sdram_ctrl.v in Mist v1.2 release uses nine bits width memory refresh counter named refreshcnt. This counter decreases from 511 to zero once in every 16th tact of 114 MHz clock in phase 1 of sdram state machine.

Thus we have one refresh cycle for approx. 72 microseconds, while SDRAM chip need it to be once for 7.8 microsecond. We have a potential problem here, if I`am get it right.

rkrajnc commented 7 years ago

Hi,

thanks for the report. The SDRAM chip used requires 64ms refresh cycles (where did you get the info about 7.8ms refresh?) While the refresh cycle is a little longer than required, this shouldn't be a problem, as in my experience SDRAM usually runs just fine with slightly out-of-spec refresh timings.

Nevertheless, I'll try to decrease the counter value a little bit, but I doubt it will help with the general stability problems.

sonycman commented 7 years ago

Hi, Rok. Yes, but memory needs 8192 refresh cycles every 64 milliseconds, does it not? Thus we have one cycle every 7.8 microsecond. This is from datasheet for MT48LC16M16A2: Regardless of device width, the 256Mb SDRAM requires 8192 AUTO REFRESH cycles every 64ms (commercial and industrial) or 16ms (automotive). Providing a distributed AUTO REFRESH command every 7.813μs

And not one cycle in 72 microseconds - as in your code, which is violating the specs for almost ten times!

rkrajnc commented 7 years ago

It seems you are correct. There should be a autorefresh command for each row.

This is actually not my code, I think it was written by tobiflexx, and subsequently used in all minimig variants that have SDRAM, so it looks like all of them have a potential problem :S

Will definitely look into this, thanks again for reporting it.

sonycman commented 7 years ago

It seems that author is simply forget to correctly initialize the counter, as it have initial value of all the ones. But your DE1 minimig design has correctly working sdram controller (different version, maybe) with all the IDLE cycles set to auto refresh, without the need for special counter.

You are welcome, Rok.

rkrajnc commented 7 years ago

I thought idle cycles were used for refresh ;) I should really look into the sdram controller code ...

rkrajnc commented 7 years ago

Fixed in a464a52f6d8d1055a468f70a2ebd12e4a6b994fa.