FPGA VGA Controller for 8-bit computer

[BrianHG]

OK, after testing, have you figured out why the endian option is a bad thing?
Do you have a better solution to cover all possibilities?

[nockieboy]

OK, after testing, have you figured out why the endian option is a bad thing?
Do you have a better solution to cover all possibilities?

Not yet, I still can't get the simulation to run.  I've fixed the assignment syntax errors - the error I'm getting now is this one:

# Z80_Bus_Interface_to_DDR3_tb.sdramddr3_0.file_io_open: at time                    0 WARNING: no +model_data option specified, using /tmp.
# ** Warning: (vsim-3533) [FOFIW] - Failed to open file "/tmp/Z80_Bus_Interface_to_DDR3_tb.sdramddr3_0.open_bank_file.0" for writing.
# No such file or directory. (errno = ENOENT)    : BrianHG_DDR3/ddr3.v(633)
#    Time: 0 ps  Iteration: 0  Instance: /Z80_Bus_Interface_to_DDR3_tb/sdramddr3_0
# Z80_Bus_Interface_to_DDR3_tb.sdramddr3_0.open_bank_file: at time 0 ERROR: failed to open /tmp/Z80_Bus_Interface_to_DDR3_tb.sdramddr3_0.open_bank_file.0.
# ** Note: $finish    : BrianHG_DDR3/ddr3.v(637)
#    Time: 0 ps  Iteration: 0  Instance: /Z80_Bus_Interface_to_DDR3_tb/sdramddr3_0

The simulation 'finishes' at this point.

[BrianHG]

How is that possible?
It's working fine on my side....
What did you do?
If I don't let you do this, you will always rely on me...
I even told you the 1 line to add to the 'setup_z80_to_DDR3.do' and 'run_z80_to_DDR3.do'.
Everything else you had except for the '   ' {  ' error once removed, it should have worked after typing:
do setup_z80_to_DDR3.do


To be safe, close modelsim, delete the 'work' directory and delete modelsim.ini, and delete vlog.opt and delete vsim.wlf to flush everything blank.

[nockieboy]

No idea.   I added HW_Regs.sv to the TB folder, updated setup.do to include HW_Regs.sv... I've attached the TB folder for info.

[BrianHG]

How is that possible?
It's working fine on my side....
What did you do?
If I don't let you do this, you will always rely on me...
I even told you the 1 line to add to the 'setup_z80_to_DDR3.do' and 'run_z80_to_DDR3.do'.
Everything else you had except for the '   ' {  ' error once removed, it should have worked after typing:
do setup_z80_to_DDR3.do


To be safe, close modelsim, delete the 'work' directory and delete modelsim.ini, and delete vlog.opt and delete vsim.wlf to flush everything blank.

[BrianHG]

before sending out any Modelsim .zip, please delete TB_v15.code-workspace and delete the 'work' directory and delete modelsim.ini, and delete vlog.opt and delete vsim.wlf to flush everything blank.  You just want a clean tiny project.

And what you sent me, (though I did the deleting first) seems to run completely fine.

Do you have the project in a folder where modelsim has write privileges?
What about the C:\tmp folder for the DDR3 bank files?
(If it is the C:\tmp write privileges, then I know you never ran a single DDR3 testbench of mine.)

[nockieboy]

before sending out any Modelsim .zip, please delete TB_v15.code-workspace and delete the 'work' directory and delete modelsim.ini, and delete vlog.opt and delete vsim.wlf to flush everything blank.  You just want a clean tiny project.

And what you sent me, (though I did the deleting first) seems to run completely fine.

Do you have the project in a folder where modelsim has write privileges?
What about the C:\tmp folder for the DDR3 bank files?
(If it is the C:\tmp write privileges, then I know you never ran a single DDR3 testbench of mine.)

Yeah, sorry about that, the DDR3 testbench always ran fine on the old one - then I realised I'd forgotten to update tmp_model_dir value in ddr3.v (with a Windoze-compatible path) in the new TB.

Got meetings now - might get a chance to review the simulation later this afternoon.

[nockieboy]

Okay, this is interesting.  According to the test bench (see screenshot below), the TAP port doesn't go live with data for quite a while after reset, presumably because it's waiting for more data to hit the DDR3 control buffer before writing to the memory to reduce writing?

In any case, as you can see in the screenshot, the simulation is writing to memory address 0x00000002 with data 0xAA.   This should be interpreted as HW_Reg[2], but it looks like TAP_ADDR is wrong?  It's showing an address of 0x00000000, but that should be 0x00000002?  The data and write mask appear correct, however the HW_Regs are not updating correctly either.  Instead of them changing to "10 00 AA 00 55 00 xx xx xx xx xx xx xx xx xx xx", they change to "10 00 10 00 55 00 xx xx xx xx xx xx 00 xx xx xx".

Is this to do with the endian issue you've alluded to or some other problem?  Either way, TAP_ADDR is wrong and the wrong data is being written to the wrong HW_Reg.  All in all, not a good start for me!

[BrianHG]

Why didn't you simplify your sim display to Z80 bus / HW_REGS only and click on the ' + ' on the HW_REGS, zoom out, so you can see what's going on like I did?

SEE:


Also, why do you have 16regs only, I said to use 64 so you can at least go above 1 single 128bit wide WDATA all at address 16'h0000.

And again, why did you choose such poor test write data which wouldn't give you a clue to the actual problem, why not try these Z80 writes (inside Z80_cmd_stimulus.txt) to see what is going on:

Code: [Select]

@LOG_FILE Z80_cmd_stimulus_log.txt
@RESET

@CMD WM 0000 aa     Write to memory address
@CMD WM 0001 11     Write to memory address
@CMD WM 0002 22     Write to memory address
@CMD WM 0003 33     Write to memory address
@CMD WM 0004 44     Write to memory address
@CMD WM 0005 55     Write to memory address
@CMD WM 0006 66     Write to memory address
@CMD WM 0007 77     Write to memory address
@CMD WM 0010 10     Write to memory address
@CMD WM 0020 20     Write to memory address
@CMD WM 0030 30     Write to memory address
@CMD WM 0040 40     Write to memory address
@CMD WM 0050 50     Write to memory address
@CMD WM 1110 BB     Write to memory address
@CMD WM 2210 CC     Write to memory address
@CMD WM 3310 DD     Write to memory address

Now do not look at this line, see if you can figure out the problem by yourself first:
for (i = 0; i < PORT_CACHE_BITS/8; i = i + 1) if (valid_wr && WMASK[i ^ ENDIAN]) HW_REGS[( ADDR_IN[HW_REGS_SIZE-1:0] | (i^(PORT_CACHE_BITS/8-1)) )] <= DATA_IN[i*8+:8] ;

Next, work out why the 'ENDIAN' only messes up then entire HW_REG system, why you need to get rid of it and why making new IO port with the same output reg renamed to 8bit and these new assigned output wires would help provide a better solution:

Code: [Select]

)(

        input                               RESET,
        input                               CLK,
        input                               WE,
        input          [PORT_ADDR_SIZE-1:0] ADDR_IN,
        input         [PORT_CACHE_BITS-1:0] DATA_IN,
        input       [PORT_CACHE_BITS/8-1:0] WMASK,
        output  reg                 [  7:0] HW_REGS_8bit[0:(2**HW_REGS_SIZE-1)]
        output  wire                [ 15:0] HW_REGS_16bit[0:(2**HW_REGS_SIZE-1)]
        output  wire                [ 31:0] HW_REGS_32bit[0:(2**HW_REGS_SIZE-1)]

);

You may also do the 'reset' values in the same way for defaults which are 8bit, 16bit, or 32bit.

(BTW: I do not have a "D:\tmp" drive.  Also, isn't there a way to define the tmp folrder in the code instead of modifying Micron's ddr3.v source code + make that folder in the same folder as the work directory...)

[BrianHG]

 
Here is the GPU project with the final BrianHG_DDR3_Controller_v15 release.
Consider all the files updated, including the .sdc file.
Let me know if it works properly.

The new complete BrianHG_DDR3_Controller v1.50 here:
https://www.eevblog.com/forum/fpga/brianhg_ddr3_controller-open-source-ddr3-controller/msg3606415/#msg3606415
And here:
https://github.com/BrianHGinc/BrianHG-DDR3-Controller

[BrianHG]

Well nockieboy, is my final release DDR3 v1.5 working fine?
Did you tune the Z80 read cache / WAIT output timing?
Did you solve the new HW_REGS code issue?

[nockieboy]

Had a busy weekend, unfortunately.  Have just had time to compile the new project and quickly try it out with the BBCBASIC tests - getting no artefacts in the graphics so far.

Did you tune the Z80 read cache / WAIT output timing?
Did you solve the new HW_REGS code issue?

Nope.  Will maybe get some time later this week, depends how things go. 

[nockieboy]

Did you tune the Z80 read cache / WAIT output timing?

From what I can tell from SignalTap, it looks like the default settings are working fine.  Not seeing any unnecessary WAITs being inserted when a cache-hit is achieved and no bad reads as a result of a cache miss and no WAIT.

[nockieboy]

Did you solve the new HW_REGS code issue?

Looks like it was writing each 16-bit value to the wrong address - i.e. for the first write (0xAA), it should have been writing to address 0, it was writing to 15-(the required address).  I don't know why it would do that and I'm struggling to get my head around what's going on with [i^ENDIAN] and OR-ing values together etc.

(BTW: I do not have a "D:\tmp" drive.  Also, isn't there a way to define the tmp folrder in the code instead of modifying Micron's ddr3.v source code + make that folder in the same folder as the work directory...)

Fixed this - although it still required an alteration to Micron's ddr3.v source code.  Changed the path from "D:\tmp" to just ".\tmp" - it now uses a folder called 'tmp' in the project directory.  Can find no reference to a command that allows me to change the tmp directory path outside of the source code, and frankly don't have the time to hunt it down when I have a working solution that (hopefully) won't cause issues at anyone else's end either.

Am I going along the right path with the changes I've made in the attached files?  Haven't sorted out the resets for the 16 and 32-bit regs, plus it all seems a bit wasteful in terms of gates duplicating all that data, but I'm sure there's probably a much simpler way of doing it all that you've got.

[BrianHG]

Did you solve the new HW_REGS code issue?

Looks like it was writing each 16-bit value to the wrong address - i.e. for the first write (0xAA), it should have been writing to address 0, it was writing to 15-(the required address).  I don't know why it would do that and I'm struggling to get my head around what's going on with [i^ENDIAN] and OR-ing values together etc.

(BTW: I do not have a "D:\tmp" drive.  Also, isn't there a way to define the tmp folrder in the code instead of modifying Micron's ddr3.v source code + make that folder in the same folder as the work directory...)

Fixed this - although it still required an alteration to Micron's ddr3.v source code.  Changed the path from "D:\tmp" to just ".\tmp" - it now uses a folder called 'tmp' in the project directory.  Can find no reference to a command that allows me to change the tmp directory path outside of the source code, and frankly don't have the time to hunt it down when I have a working solution that (hopefully) won't cause issues at anyone else's end either.

Am I going along the right path with the changes I've made in the attached files?  Haven't sorted out the resets for the 16 and 32-bit regs, plus it all seems a bit wasteful in terms of gates duplicating all that data, but I'm sure there's probably a much simpler way of doing it all that you've got.

Next, work out why the 'ENDIAN' only messes up then entire HW_REG system, why you need to get rid of it and why making new IO port with the same output reg renamed to 8bit and these new assigned output wires would help provide a better solution:

Code: [Select]

)(

        input                               RESET,
        input                               CLK,
        input                               WE,
        input          [PORT_ADDR_SIZE-1:0] ADDR_IN,
        input         [PORT_CACHE_BITS-1:0] DATA_IN,
        input       [PORT_CACHE_BITS/8-1:0] WMASK,
        output  reg                 [  7:0] HW_REGS_8bit[0:(2**HW_REGS_SIZE-1)]
        output  wire                [ 15:0] HW_REGS_16bit[0:(2**HW_REGS_SIZE-1)]
        output  wire                [ 31:0] HW_REGS_32bit[0:(2**HW_REGS_SIZE-1)]

);


I said assigned wires, not new regs.

IE:
for loop
assign HW_REGS_16bit[ i ] = {HW_REGS_8bit[ i+0 ],HW_REGS_8bit[ i+1 ]};

similar for the 32bit assigns.

Swap the +0 and +1 depending on the endian setting.

Unused regs are "PRUNED" from the design automatically, otherwise, your original 160kbits of regs would excede the DECA's available 50kbit logic cells.

I have a feeling you will soon need to look at my tiny code in the 2 point size.
It is kind of obvious if the address you are writing to is going into the wrong memory location that you need to correct the write address.  Well, at least you have seen that much with my chosen test 'write data'.  Look at the pattern of where each write goes to into the HW_REGS regardless of the data which is being written.  Look at the write address in the SIM and where the write data ends up.  Maybe this hint will help.

[nockieboy]

I said assigned wires, not new regs.

Darn it, missed that minor detail.

IE:
for loop
assign HW_REGS_16bit[ i ] = {HW_REGS_8bit[ i+0 ],HW_REGS_8bit[ i+1 ]};

similar for the 32bit assigns.

Swap the +0 and +1 depending on the endian setting.

This might seem a silly question, but where should I put these assigns?  For some reason I've got it into my head that you can't (or shouldn't) put assigns in an always block?  Do I need a separate comb block for this assign loop?

[nockieboy]

The address needs to be inverted..

Okay, had to go look at the tiny code you'd posted.  There was no way I was going to work out how to invert the address like that.

My question still stands re: where to put the assigns, though.

[BrianHG]

The assigns do not go inside the always_ff block.

Though, you can make an 'always_comb' block and just use a simple '16bit={8bit,8bit}' if you like.  IE: the 'assign' header is not needed inside an always_comb block.
 

[BrianHG]

There was no way I was going to work out how to invert the address like that.

That isn't the only way.

[nockieboy]

I'm missing something or doing something wrong here with the assigns.  According to ModelSim, the 16bit and 32bit HW_Regs are constantly hi-z and their values never change?

Code: [Select]

assign HW_Regs_16bit = enable ? { HW_REGS__8bit[( ADDR_IN[HW_REGS_SIZE-1:0] )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 1 )] } : 16'b0 ;
assign HW_Regs_32bit = enable ? { HW_REGS__8bit[( ADDR_IN[HW_REGS_SIZE-1:0] )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 1 )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 2 )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 3 )] } : 32'b0 ;
Both outputs should only show a value if enable is high.  Is that preferable to just outputting a value no matter what?

[BrianHG]

I'm missing something or doing something wrong here with the assigns.  According to ModelSim, the 16bit and 32bit HW_Regs are constantly hi-z and their values never change?

Code: [Select]

assign HW_Regs_16bit = enable ? { HW_REGS__8bit[( ADDR_IN[HW_REGS_SIZE-1:0] )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 1 )] } : 16'b0 ;
assign HW_Regs_32bit = enable ? { HW_REGS__8bit[( ADDR_IN[HW_REGS_SIZE-1:0] )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 1 )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 2 )], HW_REGS__8bit[(( ADDR_IN[HW_REGS_SIZE-1:0] ) + 3 )] } : 32'b0 ;
Both outputs should only show a value if enable is high.  Is that preferable to just outputting a value no matter what?

IE:
for loop
assign HW_REGS_16bit[ i ] = {HW_REGS_8bit[ i+0 ],HW_REGS_8bit[ i+1 ]};

[BrianHG]

Hello?  Did you get it working??

You can also try:

Code: [Select]

always_comb begin
for (i = big_endian ; i < (total_size-1) ; i ++ ) HW_REGS_16bit[ i ] = {HW_REGS_8bit[ i+little_endian ],HW_REGS_8bit[ i+big_endian ]};
end
Im sure I might have flipped around an endian or 2...
You will probably need to change the output wire to output logic for this to work.

[nockieboy]

For simplicity - and unless you really think we need it, I'm dropping endianness concerns for the moment.  Here's what I've done so far:

Code: [Select]

integer x ;
always_comb begin
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 2)  begin
        HW_REGS_16bit[x] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1] } ;
    end
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 4)  begin
        HW_REGS_32bit[x] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1], HW_REGS__8bit[x+2], HW_REGS__8bit[x+3] } ;
    end
end

integer i ;
always @( posedge CLK ) begin
   
    if ( RESET ) begin
        // reset registers to initial values
        for (i = 0; i < RST_PARAM_SIZE; i = i + 1) begin
            HW_REGS__8bit[{RESET_VALUES[i][29:17], 1'b0}] <= RESET_VALUES[i][ 7:0] ;
            HW_REGS__8bit[{RESET_VALUES[i][29:17], 1'b1}] <= RESET_VALUES[i][15:8] ;
        end
    end
    else
    begin
        for (i = 0; i < PORT_CACHE_BITS/8; i = i + 1)  begin
            if (valid_wr && WMASK[i]) begin
                HW_REGS__8bit[( ADDR_IN[HW_REGS_SIZE-1:0] | (i^(PORT_CACHE_BITS/8-1)) )] <= DATA_IN[i*8+:8] ;
            end
        end
    end
   
end
I've also had to change the 16-bit and 32-bit HW_REGS outputs to logic instead of wires as you said.  I guess these are being treated as regs now?

Seems there's a problem with the index value for 16- and 32-bit values, however:



So I changed the comb slightly to index the 16- and 32-bit values better:

Code: [Select]

integer x;
always_comb begin
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 2)  begin
        HW_REGS_16bit[x/2] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1] } ;
    end
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 4)  begin
        HW_REGS_32bit[x/4] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1], HW_REGS__8bit[x+2], HW_REGS__8bit[x+3] } ;
    end
end
But it seems no values are being updated in these two HW_REGS outputs in addresses >0x0F?



As you can see, values up to 0x0F are reflected in the 16- and 32-bit outputs, but writes to HW_REGS after that to 0x14, 0x15 etc. are not updated.  Not sure why this is happening?

[BrianHG]

What happens when you write a 16bit number to address 005 vs 006 in a Z80.
Shouldn't you want the option to read a 16bit number in the odd addresses?

Code: [Select]

integer x ;
always_comb begin
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 2)  begin
        HW_REGS_16bit[x] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1] } ;
    end
    for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 4)  begin
        HW_REGS_32bit[x] = { HW_REGS__8bit[x+0], HW_REGS__8bit[x+1], HW_REGS__8bit[x+2], HW_REGS__8bit[x+3] } ;
    end
end


 
for (x = 0; x < PORT_CACHE_BITS/8 - 1; x = x + 2)  begin
No...
This:
for (x = 0; x < (HW_REGS_SIZE**2) - 1; x = x + 1)  begin
You want all the HW regs.

And what's wrong with the endian?

Code: [Select]

parameter string ENDIAN = "Big Endian",   // Enter "B****" for Big Endian, anything else for Little Endian.

......

localparam endian_h16 = (ENDIAN[0] == "B") ? 1 : 0 ;
localparam endian_l16 = (ENDIAN[0] == "B") ? 0 : 1 ;
localparam endian_h32 = (ENDIAN[0] == "B") ? 3 : 0 ;
localparam endian_m32 = (ENDIAN[0] == "B") ? 2 : 1 ;
localparam endian_n32 = (ENDIAN[0] == "B") ? 1 : 2 ;
localparam endian_l32 = (ENDIAN[0] == "B") ? 0 : 3 ;

.......

for (x = 0; x < [color=red](HW_REGS_SIZE**2)[/color] - 3; x = x + 1)  begin // The -3 means the final 3 bytes cannot be used as wide words as they would exceed the HW_REGS_8bit final address.
HW_REGS_16bit[x] = { HW_REGS_8bit[x+endian_h16], HW_REGS_8bit[x+endian_l16] } ;
HW_REGS_32bit[x] = { HW_REGS_8bit[x+endian_h32], HW_REGS_8bit[x+endian_m32], HW_REGS_8bit[x+endian_n32], HW_REGS_8bit[x+endian_l32] } ;
end

Need I say more?

[BrianHG]

BTW, I still do not like your reset values system.
Remember, we want to specify the same Z80 address and data if it were a Z80 equivalent write.

If you do not want to complicate things on the reset, then just make it a defined byte/byte reset.

I would personally make 3 sets of input reset parameters, one for 8 bit values, one for 16bit values and one for 32bit values.  The 3 input parameters each would have their own counter for the number of presets and you can use the same 2&4 Endian localparams I created in the previous post to define how a 16bit or 32bit reset default ends up sorted into the 8bit regs.

The only thing left is an optional 'strobe' output for each 8bit address.  This will allow you to move the GPU geometry Z80 output ports to a Z80 write memory address.  (There are some weird caveats here when using this feature without the Z80 doing the writing, but we can work out a few work arounds.)