Hi everybody.

I'd like to propose some changes to the way the QMEM bus is currently implemented in or1200. Here's the gist:
- remove embedded memory from or1200_qmem_top
- remove iack / dack arbitration state machine from or1200_qmem_top
- keep separate instruction and data qmem buses and pull the bus to the top level - or1200_top

My reasoning is this:
- from a hierarchical point of view, instantiating memory 'inside' the cpu is not right (I guess the same should be done for cache memories)
- if QMEM is disabled, there is no change at all to how the or1200 works / behaves
- if anyone requires QMEM as a tightly-coupled memory, it can be still used as before, it is only connected on the top level. Since instruction and data QMEM buses are kept seperate, you could either use just one memory with external arbiter, or use two memories on each bus.
- QMEM can be used as a fully-functional bus by itself, either together with Wishbone or alone. Since QMEM has pipelined (one level deep) reads, it can actually be faster than Wishbone. For example, if you'd need a small, stripped-down version of or1200, without caches, MMUs, the QMEM bus is a perfect choice.

Two files are changed by my proposal - or1200_top.v and or1200_qmem_top.v. I attached the full files and also diffs to the ORPSOC version.

Please share your thoughts (or questions) on this matter.

Enjoy!

or1200_top.v attached.

or1200_qmem_top attached.

diff --git a/rtl/or1200/or1200_top.v b/rtl/or1200/or1200_top.v
index 1537acf..2c941bd 100644
--- a/rtl/or1200/or1200_top.v
+++ b/rtl/or1200/or1200_top.v
@@ -85,6 +85,12 @@ module or1200_top(
// RAM BIST
mbist_si_i, mbist_so_o, mbist_ctrl_i,
`endif
+
+`ifdef OR1200_QMEM_IMPLEMENTED
+ dqmem_cs_o, dqmem_we_o, dqmem_sel_o, dqmem_adr_o, dqmem_dat_o, dqmem_dat_i, dqmem_ack_i, dqmem_err_i,
+ iqmem_cs_o, iqmem_we_o, iqmem_sel_o, iqmem_adr_o, iqmem_dat_o, iqmem_dat_i, iqmem_ack_i, iqmem_err_i,
+`endif
+
// Power Management
pm_cpustall_i,
pm_clksd_o, pm_dc_gate_o, pm_ic_gate_o, pm_dmmu_gate_o,
@@ -181,6 +187,28 @@ input [`OR1200_MBIST_CTRL_WIDTH - 1:0] mbist_ctrl_i;
output mbist_so_o;
`endif

+`ifdef OR1200_QMEM_IMPLEMENTED
+//
+// QMEM bus
+//
+output dqmem_cs_o;
+output dqmem_we_o;
+output [3:0] dqmem_sel_o;
+output [31:0] dqmem_adr_o;
+output [31:0] dqmem_dat_o;
+input [31:0] dqmem_dat_i;
+input dqmem_ack_i;
+input dqmem_err_i;
+output iqmem_cs_o;
+output iqmem_we_o;
+output [3:0] iqmem_sel_o;
+output [31:0] iqmem_adr_o;
+output [31:0] iqmem_dat_o;
+input [31:0] iqmem_dat_i;
+input iqmem_ack_i;
+input iqmem_err_i;
+`endif
+
//
// Power Management
//
@@ -813,6 +841,27 @@ or1200_qmem_top or1200_qmem_top(
.mbist_ctrl_i(mbist_ctrl_i),
`endif

+`ifdef OR1200_QMEM_IMPLEMENTED
+ .du_stall (du_stall ),
+ // QMEM
+ .dqmem_cs_o (dqmem_cs_o ),
+ .dqmem_we_o (dqmem_we_o ),
+ .dqmem_sel_o (dqmem_sel_o),
+ .dqmem_adr_o (dqmem_adr_o),
+ .dqmem_dat_o (dqmem_dat_o),
+ .dqmem_dat_i (dqmem_dat_i),
+ .dqmem_ack_i (dqmem_ack_i),
+ .dqmem_err_i (dqmem_err_i),
+ .iqmem_cs_o (iqmem_cs_o ),
+ .iqmem_we_o (iqmem_we_o ),
+ .iqmem_sel_o (iqmem_sel_o),
+ .iqmem_adr_o (iqmem_adr_o),
+ .iqmem_dat_o (iqmem_dat_o),
+ .iqmem_dat_i (iqmem_dat_i),
+ .iqmem_ack_i (iqmem_ack_i),
+ .iqmem_err_i (iqmem_err_i),
+`endif
+
// QMEM and CPU/IMMU
.qmemimmu_adr_i(qmemimmu_adr_immu),
.qmemimmu_cycstb_i(qmemimmu_cycstb_immu),



diff --git a/rtl/or1200/or1200_qmem_top.v b/rtl/or1200/or1200_qmem_top.v
index c8d7fd0..21d9ed6 100644
--- a/rtl/or1200/or1200_qmem_top.v
+++ b/rtl/or1200/or1200_qmem_top.v
@@ -93,6 +93,27 @@ module or1200_qmem_top(
mbist_si_i, mbist_so_o, mbist_ctrl_i,
`endif

+`ifdef OR1200_QMEM_IMPLEMENTED
+ du_stall,
+
+ dqmem_cs_o,
+ dqmem_we_o,
+ dqmem_sel_o,
+ dqmem_adr_o,
+ dqmem_dat_o,
+ dqmem_dat_i,
+ dqmem_ack_i,
+ dqmem_err_i,
+ iqmem_cs_o,
+ iqmem_we_o,
+ iqmem_sel_o,
+ iqmem_adr_o,
+ iqmem_dat_o,
+ iqmem_dat_i,
+ iqmem_ack_i,
+ iqmem_err_i,
+`endif
+
// QMEM and CPU/IMMU
qmemimmu_adr_i,
qmemimmu_cycstb_i,
@@ -168,6 +189,30 @@ input [`OR1200_MBIST_CTRL_WIDTH - 1:0] mbist_ctrl_i;
output mbist_so_o;
`endif

+`ifdef OR1200_QMEM_IMPLEMENTED
+input du_stall;
+
+//
+// QMEM bus
+//
+output dqmem_cs_o;
+output dqmem_we_o;
+output [3:0] dqmem_sel_o;
+output [31:0] dqmem_adr_o;
+output [31:0] dqmem_dat_o;
+input [31:0] dqmem_dat_i;
+input dqmem_ack_i;
+input dqmem_err_i;
+output iqmem_cs_o;
+output iqmem_we_o;
+output [3:0] iqmem_sel_o;
+output [31:0] iqmem_adr_o;
+output [31:0] iqmem_dat_o;
+input [31:0] iqmem_dat_i;
+input iqmem_ack_i;
+input iqmem_err_i;
+`endif
+
//
// QMEM and CPU/IMMU
//
@@ -235,60 +280,62 @@ input [3:0] dcqmem_tag_i;
//
wire iaddr_qmem_hit;
wire daddr_qmem_hit;
-reg [2:0] state;
reg qmem_dack;
reg qmem_iack;
-wire [31:0] qmem_di;
-wire [31:0] qmem_do;
-wire qmem_en;
-wire qmem_we;
-`ifdef OR1200_QMEM_BSEL
-wire [3:0] qmem_sel;
-`endif
-wire [31:0] qmem_addr;
-`ifdef OR1200_QMEM_ACK
-wire qmem_ack;
-`else
-wire qmem_ack = 1'b1;
-`endif
+reg qmem_derr;
+reg qmem_ierr;
+
+//
+// QMEM
+//
+assign dqmem_cs_o = daddr_qmem_hit & qmemdmmu_cycstb_i;
+assign dqmem_we_o = qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i;
+assign dqmem_sel_o = qmemdcpu_sel_i;
+assign dqmem_adr_o = qmemdmmu_adr_i;
+assign dqmem_dat_o = qmemdcpu_dat_i;
+assign iqmem_cs_o = iaddr_qmem_hit & qmemimmu_cycstb_i;
+assign iqmem_we_o = 1'b0;
+assign iqmem_sel_o = qmemicpu_sel_i;
+assign iqmem_adr_o = qmemimmu_adr_i;
+assign iqmem_dat_o = 32'hxxxxxxxx;

//
// QMEM and CPU/IMMU
//
-assign qmemicpu_dat_o = qmem_iack ? qmem_do : icqmem_dat_i;
-assign qmemicpu_ack_o = qmem_iack ? 1'b1 : icqmem_ack_i;
-assign qmemimmu_rty_o = qmem_iack ? 1'b0 : icqmem_rty_i;
-assign qmemimmu_err_o = qmem_iack ? 1'b0 : icqmem_err_i;
-assign qmemimmu_tag_o = qmem_iack ? 4'h0 : icqmem_tag_i;
+assign qmemicpu_dat_o = qmem_iack ? iqmem_dat_i : icqmem_dat_i;
+assign qmemicpu_ack_o = qmem_iack ? 1'b1 : icqmem_ack_i;
+assign qmemimmu_rty_o = qmem_iack ? 1'b0 : icqmem_rty_i;
+assign qmemimmu_err_o = qmem_iack ? 1'b0 : icqmem_err_i;
+assign qmemimmu_tag_o = qmem_iack ? 4'h0 : icqmem_tag_i;

//
// QMEM and IC
//
-assign icqmem_adr_o = iaddr_qmem_hit ? 32'h0000_0000 : qmemimmu_adr_i;
-assign icqmem_cycstb_o = iaddr_qmem_hit ? 1'b0 : qmemimmu_cycstb_i;
-assign icqmem_ci_o = iaddr_qmem_hit ? 1'b0 : qmemimmu_ci_i;
-assign icqmem_sel_o = iaddr_qmem_hit ? 4'h0 : qmemicpu_sel_i;
-assign icqmem_tag_o = iaddr_qmem_hit ? 4'h0 : qmemicpu_tag_i;
+assign icqmem_adr_o = iaddr_qmem_hit ? 32'h0000_0000 : qmemimmu_adr_i;
+assign icqmem_cycstb_o = iaddr_qmem_hit ? 1'b0 : qmemimmu_cycstb_i;
+assign icqmem_ci_o = iaddr_qmem_hit ? 1'b0 : qmemimmu_ci_i;
+assign icqmem_sel_o = iaddr_qmem_hit ? 4'h0 : qmemicpu_sel_i;
+assign icqmem_tag_o = iaddr_qmem_hit ? 4'h0 : qmemicpu_tag_i;

//
// QMEM and CPU/DMMU
//
-assign qmemdcpu_dat_o = daddr_qmem_hit ? qmem_do : dcqmem_dat_i;
-assign qmemdcpu_ack_o = daddr_qmem_hit ? qmem_dack : dcqmem_ack_i;
-assign qmemdcpu_rty_o = daddr_qmem_hit ? ~qmem_dack : dcqmem_rty_i;
-assign qmemdmmu_err_o = daddr_qmem_hit ? 1'b0 : dcqmem_err_i;
-assign qmemdmmu_tag_o = daddr_qmem_hit ? 4'h0 : dcqmem_tag_i;
+assign qmemdcpu_dat_o = daddr_qmem_hit ? dqmem_dat_i : dcqmem_dat_i;
+assign qmemdcpu_ack_o = daddr_qmem_hit ? qmem_dack : dcqmem_ack_i;
+assign qmemdcpu_rty_o = daddr_qmem_hit ? ~qmem_dack : dcqmem_rty_i;
+assign qmemdmmu_err_o = daddr_qmem_hit ? qmem_derr : dcqmem_err_i;
+assign qmemdmmu_tag_o = daddr_qmem_hit ? 4'h0 : dcqmem_tag_i;

//
// QMEM and DC
//
-assign dcqmem_adr_o = daddr_qmem_hit ? 32'h0000_0000 : qmemdmmu_adr_i;
-assign dcqmem_cycstb_o = daddr_qmem_hit ? 1'b0 : qmemdmmu_cycstb_i;
-assign dcqmem_ci_o = daddr_qmem_hit ? 1'b0 : qmemdmmu_ci_i;
-assign dcqmem_we_o = daddr_qmem_hit ? 1'b0 : qmemdcpu_we_i;
-assign dcqmem_sel_o = daddr_qmem_hit ? 4'h0 : qmemdcpu_sel_i;
-assign dcqmem_tag_o = daddr_qmem_hit ? 4'h0 : qmemdcpu_tag_i;
-assign dcqmem_dat_o = daddr_qmem_hit ? 32'h0000_0000 : qmemdcpu_dat_i;
+assign dcqmem_adr_o = daddr_qmem_hit ? 32'h0000_0000 : qmemdmmu_adr_i;
+assign dcqmem_cycstb_o = daddr_qmem_hit ? 1'b0 : qmemdmmu_cycstb_i;
+assign dcqmem_ci_o = daddr_qmem_hit ? 1'b0 : qmemdmmu_ci_i;
+assign dcqmem_we_o = daddr_qmem_hit ? 1'b0 : qmemdcpu_we_i;
+assign dcqmem_sel_o = daddr_qmem_hit ? 4'h0 : qmemdcpu_sel_i;
+assign dcqmem_tag_o = daddr_qmem_hit ? 4'h0 : qmemdcpu_tag_i;
+assign dcqmem_dat_o = daddr_qmem_hit ? 32'h0000_0000 : qmemdcpu_dat_i;

//
// Address comparison whether QMEM was hit
@@ -306,141 +353,39 @@ assign daddr_qmem_hit = 1'b0;
`endif

//
-//
-//
-assign qmem_en = iaddr_qmem_hit & qmemimmu_cycstb_i | daddr_qmem_hit & qmemdmmu_cycstb_i;
-assign qmem_we = qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i;
-`ifdef OR1200_QMEM_BSEL
-assign qmem_sel = (qmemdmmu_cycstb_i & daddr_qmem_hit) ? qmemdcpu_sel_i : qmemicpu_sel_i;
-`endif
-assign qmem_di = qmemdcpu_dat_i;
-assign qmem_addr = (qmemdmmu_cycstb_i & daddr_qmem_hit) ? qmemdmmu_adr_i : qmemimmu_adr_i;
-
-//
-// QMEM control FSM
-//
-always @(`OR1200_RST_EVENT rst or posedge clk)
- if (rst == `OR1200_RST_VALUE) begin
- state - qmem_dack - qmem_iack - end
- else case (state) // synopsys parallel_case
- `OR1200_QMEMFSM_IDLE: begin
- if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemimmu_cycstb_i & iaddr_qmem_hit & qmem_ack) begin
- state - qmem_iack - qmem_dack - end
- end
- `OR1200_QMEMFSM_STORE: begin
- if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemimmu_cycstb_i & iaddr_qmem_hit & qmem_ack) begin
- state - qmem_iack - qmem_dack - end
- else begin
- state - qmem_dack - qmem_iack - end
- end
- `OR1200_QMEMFSM_LOAD: begin
- if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemimmu_cycstb_i & iaddr_qmem_hit & qmem_ack) begin
- state - qmem_iack - qmem_dack - end
- else begin
- state - qmem_dack - qmem_iack - end
- end
- `OR1200_QMEMFSM_FETCH: begin
- if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmemdcpu_we_i & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemdmmu_cycstb_i & daddr_qmem_hit & qmem_ack) begin
- state - qmem_dack - qmem_iack - end
- else if (qmemimmu_cycstb_i & iaddr_qmem_hit & qmem_ack) begin
- state - qmem_iack - qmem_dack - end
- else begin
- state - qmem_dack - qmem_iack - end
- end
- default: begin
- state - qmem_dack - qmem_iack - end
- endcase
-
-//
-// Instantiation of embedded memory
-//
-or1200_spram_2048x32 or1200_qmem_ram(
- .clk(clk),
- .rst(rst),
-`ifdef OR1200_BIST
- // RAM BIST
- .mbist_si_i(mbist_si_i),
- .mbist_so_o(mbist_so_o),
- .mbist_ctrl_i(mbist_ctrl_i),
-`endif
- .addr(qmem_addr[12:2]),
-`ifdef OR1200_QMEM_BSEL
- .sel(qmem_sel),
-`endif
-`ifdef OR1200_QMEM_ACK
- .ack(qmem_ack),
-`endif
- .ce(qmem_en),
- .we(qmem_we),
- .oe(1'b1),
- .di(qmem_di),
- .doq(qmem_do)
-);
+// QMEM ack
+//
+always @(posedge rst or posedge clk)
+begin
+ if (rst) begin
+ qmem_dack + qmem_iack + end else if(du_stall) begin
+ qmem_dack + qmem_iack + end else begin
+ qmem_dack + qmem_iack + end
+end
+
+//
+// QMEM err
+//
+always @(posedge rst or posedge clk)
+begin
+ if (rst) begin
+ qmem_derr + qmem_ierr + end else if(du_stall) begin
+ qmem_derr + qmem_ierr + end else begin
+ qmem_derr + qmem_ierr + end
+end
+

`else // OR1200_QMEM_IMPLEMENTED

I agree. It's really ugly to have an embedded memory inside the core. Moving it out is a good decision. One thing though, could you replace your `ifdefs with parameters? I'm not sure it's possible in the port declaration

No ifdefs ... parameters please.

You can use parameters in port declarations as well; no problem.

Richard

Hi Richard, olof.

I tried to follow existing or1200 code formatting / practices as closely as possible, with minimal changes to existing code, but I can of course modify my patches.

To which ifdefs specifically are you referring to? I only added the ones in or1200_top port declaration, and I don't know of any way you can use parameters to enable / disable specific port declarations.

I could leave qmem ports on top always defined and use parameters to enable / disable qmem functionality, is that what you meant?

Thanks for the QMEM patch.

Regarding the `ifdefs/parameters - I think consistency is best.

I know the `define thing is not pretty, but currently it's how it's done throughout all of the RTL. What is worse is having a combination of parameters and `defines for controlling the enabling of features.

To be honest, I don't really care so much right now that the OR1200's features are controlled with `ifdefs. However I reckon it's something that would be nice to see changed over to parameters instead, and I think this should occur in one go, rather than piecemeal.

I don't agree here. There is already a mess of defines, named parameters, unnamed parameters and defparams in the code base. There is no reason to use defines in newly written code, except when it will be too messy to use parameters. Conditional ports is the main reason I can see for idfefs (even though they technically could be left hanging if they are unused)

Don't let this hold anything back though. I'm just voicing my opinion, and if others are ok with the patch, then I'm all for it too.

I fully agree with Olof.

There are plans to do an ASIC version of the OR1200. Trust me, you do not want ifdefs !!!
Formal verification is going to be a bitch!!

Richard

There is a reason, which I gave above. That is the optional features of the OR1200 (caches, MMU, FPU, integer multiply divide, MAC, QMEM, DU) are enabled/disabled via `ifdefs. There are no parameters in or1200_defines.v. No parameters inside the design (or accessible via the top level) control anything significant. Nothing is usefully configurable by hacking parameters in the RTL either.

I'm not saying this is the best way to go - I think it should have been (and should become) parameterizable. But am saying we should remain consistent with how every other internal module of the OR1200 is enabled or disabled for now.

Why don't we accept this patch with `ifdefs (should it all work OK) and then put the task of switching all of the`ifdefs to parameters on the TODO list for the OR1200?

Sorry... I was thinking of ORPSoC. You're right. or122 is all define-based.



I agree. However, I would like to wait with applying patches until we have back-ported the ORPSoC patches. Further diverging the code base is not what we need right now

thanks alot for moving QMEM outside or1200, to top-level. It really simplifies many architectural things.
Has this change already gone into the main-trunk ?