search

enjoy-digital / litex

Build your hardware, easily!
Other
3k stars 569 forks source link

picorv32 doesn't check (nor use) the "variant" argument. #117

Closed mithro closed 5 years ago

mithro commented 6 years ago

Was looking at that "variants" the picorv32 supported and found out it is not checking (or using) the variant CPU config at all.

See https://github.com/enjoy-digital/litex/blob/a44181e7166da1d15d68ffe00d6ed36db0dda72a/litex/soc/cores/cpu/picorv32/core.py#L15

mithro commented 6 years ago

I'm looking at adding a minimal and lite variants to picorv32 config.

mithro commented 6 years ago 
diff --git a/litex/soc/cores/cpu/picorv32/core.py b/litex/soc/cores/cpu/picorv32/core.py
index 7fe830d6..d003bffb 100644
--- a/litex/soc/cores/cpu/picorv32/core.py
+++ b/litex/soc/cores/cpu/picorv32/core.py
@@ -9,7 +9,7 @@ class PicoRV32(Module):
     name = "picorv32"
     endianness = "little"
     gcc_triple = ("riscv64-unknown-elf", "riscv32-unknown-elf")
-    gcc_flags = "-D__picorv32__ -mno-save-restore -march=rv32im -mabi=ilp32"
+    gcc_flags = "-D__picorv32__ -mno-save-restore -march=rv32i -mabi=ilp32"
     linker_output_format = "elf32-littleriscv"

     def __init__(self, platform, progaddr_reset, variant):
@@ -31,19 +31,19 @@ class PicoRV32(Module):

         self.specials += Instance("picorv32",
             # parameters
-            p_ENABLE_COUNTERS=1,
-            p_ENABLE_COUNTERS64=1,
-            p_ENABLE_REGS_16_31=1,
-            p_ENABLE_REGS_DUALPORT=1,
+            p_ENABLE_COUNTERS=0,
+            p_ENABLE_COUNTERS64=0,
+            p_ENABLE_REGS_16_31=1,      # Register file, so no difference?
+            p_ENABLE_REGS_DUALPORT=1,   # Register file, so no difference?
             p_LATCHED_MEM_RDATA=0,
-            p_TWO_STAGE_SHIFT=1,
+            p_TWO_STAGE_SHIFT=0,
             p_TWO_CYCLE_COMPARE=0,
             p_TWO_CYCLE_ALU=0,
-            p_CATCH_MISALIGN=1,
+            p_CATCH_MISALIGN=0,
             p_CATCH_ILLINSN=1,
             p_ENABLE_PCPI=0,
-            p_ENABLE_MUL=1,
-            p_ENABLE_DIV=1,
+            p_ENABLE_MUL=0,
+            p_ENABLE_DIV=0,
             p_ENABLE_FAST_MUL=0,
             p_ENABLE_IRQ=1,
             p_ENABLE_IRQ_QREGS=1,

Before;

Info: Device utilisation:                                                                                                                                                
Info:            ICESTORM_LC:  5668/ 5280   107%                                                                                                                 
Info:           ICESTORM_RAM:     4/   30    13%                                                                                                                          
Info:                  SB_IO:     7/   96     7%                                                                                                                      
Info:                  SB_GB:     8/    8   100%                              
Info:           ICESTORM_PLL:     0/    1     0%                                                                                                                         
Info:            SB_WARMBOOT:     0/    1     0%                                                                                                                       
Info:           ICESTORM_DSP:     0/    8     0%                              
Info:         ICESTORM_HFOSC:     0/    1     0%                                                                                                                      
Info:         ICESTORM_LFOSC:     0/    1     0%                                                                                                                          
Info:                 SB_I2C:     0/    2     0%                             
Info:                 SB_SPI:     0/    2     0%                                                                                                                       
Info:                 IO_I3C:     0/    2     0%                                                                                                                          
Info:            SB_LEDDA_IP:     0/    1     0%                             
Info:            SB_RGBA_DRV:     0/    1     0%                             
Info:         ICESTORM_SPRAM:     4/    4   100%

After;

Info: Device utilisation:
Info:            ICESTORM_LC:  3967/ 5280    75%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

FYI - @cr1901

mithro commented 6 years ago

Disabling p_ENABLE_REGS_16_31=0 and p_ENABLE_REGS_DUALPORT=0 actually seems to make it use more resources;

Info: Device utilisation:
Info:            ICESTORM_LC:  5000/ 5280    94%
Info:           ICESTORM_RAM:     0/   30     0%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

The increase in ICESTORM_LC usage seems to be caused by setting p_ENABLE_REGS_DUALPORT=0 to zero.

Using p_ENABLE_REGS_16_31=0 and p_ENABLE_REGS_DUALPORT=1 gives you;

Info: Device utilisation:
Info:            ICESTORM_LC:  3988/ 5280    75%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

So, looks like p_ENABLE_REGS_16_31=0 increases the ICESTORM_LC usage from 3967 -> 3988!?

mithro commented 6 years ago

Setting p_ENABLE_IRQ_QREGS=0 and p_ENABLE_IRQ_TIMER=0 ended up with;

Info: Device utilisation:
Info:            ICESTORM_LC:  3872/ 5280    73%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%

So they also seem to increase the resource usage....

mithro commented 6 years ago

Scratch that 3872 is less than 3967, so it saves about 95 ICESTORM_LC cells....

mithro commented 6 years ago

Setting p_CATCH_ILLINSN=0 ended up with;

Info: Device utilisation:
Info:            ICESTORM_LC:  3922/ 5280    74%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

Setting p_CATCH_MISALIGN=1 definately increases the resource usage.

mithro commented 6 years ago

The smallest configuration I have gotten so far is the following;

Info: Device utilisation:                           
Info:            ICESTORM_LC:  3872/ 5280    73%    
Info:           ICESTORM_RAM:     4/   30    13%    
Info:                  SB_IO:     7/   96     7%    
Info:                  SB_GB:     8/    8   100%    
Info:           ICESTORM_PLL:     0/    1     0%    
Info:            SB_WARMBOOT:     0/    1     0%    
Info:           ICESTORM_DSP:     0/    8     0%    
Info:         ICESTORM_HFOSC:     0/    1     0%    
Info:         ICESTORM_LFOSC:     0/    1     0%    
Info:                 SB_I2C:     0/    2     0%    
Info:                 SB_SPI:     0/    2     0%    
Info:                 IO_I3C:     0/    2     0%       
Info:            SB_LEDDA_IP:     0/    1     0%             
Info:            SB_RGBA_DRV:     0/    1     0%     
Info:         ICESTORM_SPRAM:     4/    4   100%

It comes from the following patch;

diff --git a/litex/soc/cores/cpu/picorv32/core.py b/litex/soc/cores/cpu/picorv32/core.py
index 7fe830d6..b94fb7d7 100644
--- a/litex/soc/cores/cpu/picorv32/core.py
+++ b/litex/soc/cores/cpu/picorv32/core.py
@@ -9,7 +9,7 @@ class PicoRV32(Module):
     name = "picorv32"
     endianness = "little"
     gcc_triple = ("riscv64-unknown-elf", "riscv32-unknown-elf")
-    gcc_flags = "-D__picorv32__ -mno-save-restore -march=rv32im -mabi=ilp32"
+    gcc_flags = "-D__picorv32__ -mno-save-restore -march=rv32i -mabi=ilp32"
     linker_output_format = "elf32-littleriscv"

     def __init__(self, platform, progaddr_reset, variant):
@@ -31,23 +31,25 @@ class PicoRV32(Module):

         self.specials += Instance("picorv32",
             # parameters
-            p_ENABLE_COUNTERS=1,
-            p_ENABLE_COUNTERS64=1,
+            p_ENABLE_COUNTERS=0,
+            p_ENABLE_COUNTERS64=0,
+            # Changing REGS has no effect as on FPGAs, the registers are
+            # implemented using a register file stored in DPRAM.
             p_ENABLE_REGS_16_31=1,
             p_ENABLE_REGS_DUALPORT=1,
             p_LATCHED_MEM_RDATA=0,
-            p_TWO_STAGE_SHIFT=1,
+            p_TWO_STAGE_SHIFT=0,
             p_TWO_CYCLE_COMPARE=0,
             p_TWO_CYCLE_ALU=0,
-            p_CATCH_MISALIGN=1,
+            p_CATCH_MISALIGN=0,
             p_CATCH_ILLINSN=1,
             p_ENABLE_PCPI=0,
-            p_ENABLE_MUL=1,
-            p_ENABLE_DIV=1,
+            p_ENABLE_MUL=0,
+            p_ENABLE_DIV=0,
             p_ENABLE_FAST_MUL=0,
             p_ENABLE_IRQ=1,
-            p_ENABLE_IRQ_QREGS=1,
-            p_ENABLE_IRQ_TIMER=1,
+            p_ENABLE_IRQ_QREGS=0,
+            p_ENABLE_IRQ_TIMER=0,
             p_ENABLE_TRACE=0,
             p_MASKED_IRQ=0x00000000,
             p_LATCHED_IRQ=0xffffffff,
mithro commented 6 years ago

Other random comments;

I tried disabling the XIP spiflash core (using the following patch) and got the following resource usage;

Info: Device utilisation:
Info:            ICESTORM_LC:  3708/ 5280    70%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     3/   96     3%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
diff --git a/targets/ice40_up5k_b_evn/base.py b/targets/ice40_up5k_b_evn/base.py
index d638094..dbaa814 100644
--- a/targets/ice40_up5k_b_evn/base.py
+++ b/targets/ice40_up5k_b_evn/base.py
@@ -78,14 +78,14 @@ class BaseSoC(SoCCore):
         # self.submodules.cas = cas.ControlAndStatus(platform, clk_freq)

         # SPI flash peripheral
-        self.submodules.spiflash = spi_flash.SpiFlashSingle(
-            platform.request("spiflash"),
-            dummy=platform.spiflash_read_dummy_bits,
-            div=platform.spiflash_clock_div)
+#        self.submodules.spiflash = spi_flash.SpiFlashSingle(
+#            platform.request("spiflash"),
+#            dummy=platform.spiflash_read_dummy_bits,
+#            div=platform.spiflash_clock_div)
         self.add_constant("SPIFLASH_PAGE_SIZE", platform.spiflash_page_size)
         self.add_constant("SPIFLASH_SECTOR_SIZE", platform.spiflash_sector_size)
-        self.register_mem("spiflash", self.mem_map["spiflash"],
-            self.spiflash.bus, size=platform.spiflash_total_size)
+#        self.register_mem("spiflash", self.mem_map["spiflash"],
+#            self.spiflash.bus, size=platform.spiflash_total_size)

         bios_size = 0x8000
         self.add_constant("ROM_DISABLE", 1)

So it looks like that core only uses 164 ICESTOM_LC parts?

mithro commented 6 years ago

Looks like disabling the following, decreases the resource count;

+        kwargs['ident'] = False
+        kwargs['with_timer'] = False
+        kwargs['with_ctrl'] = False

To;

Info: Device utilisation:
Info:            ICESTORM_LC:  3270/ 5280    61%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     3/   96     3%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

Doing that seems to remove all interrupt sources, see the warning below;

Warning: Wire top.\picorv32.irq [31] is used but has no driver.                                                                                         
Warning: Wire top.\picorv32.irq [30] is used but has no driver.               
Warning: Wire top.\picorv32.irq [29] is used but has no driver.               
Warning: Wire top.\picorv32.irq [28] is used but has no driver.               
Warning: Wire top.\picorv32.irq [27] is used but has no driver.               
Warning: Wire top.\picorv32.irq [26] is used but has no driver.              
Warning: Wire top.\picorv32.irq [25] is used but has no driver.                  
Warning: Wire top.\picorv32.irq [24] is used but has no driver.                 
Warning: Wire top.\picorv32.irq [23] is used but has no driver.                 
Warning: Wire top.\picorv32.irq [22] is used but has no driver.                
Warning: Wire top.\picorv32.irq [21] is used but has no driver.                                                                                        
Warning: Wire top.\picorv32.irq [20] is used but has no driver.                  
Warning: Wire top.\picorv32.irq [19] is used but has no driver.                                                                                        
Warning: Wire top.\picorv32.irq [18] is used but has no driver.               
Warning: Wire top.\picorv32.irq [17] is used but has no driver.               
Warning: Wire top.\picorv32.irq [16] is used but has no driver.                                                                                                           
Warning: Wire top.\picorv32.irq [15] is used but has no driver.               
Warning: Wire top.\picorv32.irq [14] is used but has no driver.               
Warning: Wire top.\picorv32.irq [13] is used but has no driver.                                                                                                           
Warning: Wire top.\picorv32.irq [12] is used but has no driver.              
Warning: Wire top.\picorv32.irq [11] is used but has no driver.              
Warning: Wire top.\picorv32.irq [10] is used but has no driver.                                                                                                          
Warning: Wire top.\picorv32.irq [9] is used but has no driver.               
Warning: Wire top.\picorv32.irq [8] is used but has no driver.               
Warning: Wire top.\picorv32.irq [7] is used but has no driver.                                                                                                        
Warning: Wire top.\picorv32.irq [6] is used but has no driver.               
Warning: Wire top.\picorv32.irq [5] is used but has no driver.                
Warning: Wire top.\picorv32.irq [4] is used but has no driver.                                                                                                         
Warning: Wire top.\picorv32.irq [3] is used but has no driver.                
Warning: Wire top.\picorv32.irq [1] is used but has no driver.                
Warning: Wire top.\picorv32.irq [0] is used but has no driver.

Re-enabling the timer ends up with;

Info: Device utilisation:
Info:            ICESTORM_LC:  3604/ 5280    68%
Info:           ICESTORM_RAM:     4/   30    13%
Info:                  SB_IO:     3/   96     3%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

So it looks like a lot of resources is going into the FIFO buffers for the UART. https://github.com/enjoy-digital/litex/blob/f916705313f6b556f66d931a0c2b0d5d18825f09/litex/soc/cores/uart.py#L165-L166

Change the values from the default of 16, to the value of 2 does the following:

mithro commented 6 years ago

The issue seems to be the FIFOs provided by stream.py (see below) are not getting mapped to the block ram inside the iCE40. https://github.com/enjoy-digital/litex/blob/f916705313f6b556f66d931a0c2b0d5d18825f09/litex/soc/interconnect/stream.py#L58-L108

mithro commented 6 years ago

The actual FIFOs come from the migen.genlib.fifo module.

https://github.com/m-labs/migen/blob/657c0c72e63597162837809dfe3635d69a98cfd9/migen/genlib/fifo.py#L80-L250

mithro commented 6 years ago

I'm guessing the issue is actually with the Verilog code being emitted by the migen.genlib.fhdl.specials.Memory object.

mithro commented 6 years ago

This seems to be the Verilog code generated for the FIFOs used in the UART;

reg [9:0] storage[0:15];
reg [9:0] memdat;
always @(posedge sys_clk) begin
    if (uart_tx_fifo_wrport_we)
        storage[uart_tx_fifo_wrport_adr] <= uart_tx_fifo_wrport_dat_w;
    memdat <= storage[uart_tx_fifo_wrport_adr];
end

always @(posedge sys_clk) begin
end

assign uart_tx_fifo_wrport_dat_r = memdat;
assign uart_tx_fifo_rdport_dat_r = storage[uart_tx_fifo_rdport_adr];

reg [9:0] storage_1[0:15];
reg [9:0] memdat_1;
always @(posedge sys_clk) begin
    if (uart_rx_fifo_wrport_we)
        storage_1[uart_rx_fifo_wrport_adr] <= uart_rx_fifo_wrport_dat_w;
    memdat_1 <= storage_1[uart_rx_fifo_wrport_adr];
end

always @(posedge sys_clk) begin
end

assign uart_rx_fifo_wrport_dat_r = memdat_1;
assign uart_rx_fifo_rdport_dat_r = storage_1[uart_rx_fifo_rdport_adr];
mithro commented 6 years ago

With just the change in https://github.com/enjoy-digital/litex/pull/118 the resource count goes from Info: ICESTORM_LC: 5668/ 5280 107% to Info: ICESTORM_LC: 5005/ 5280 94%.

mithro commented 6 years ago

The changes in https://github.com/enjoy-digital/litex/compare/master...mithro:picorv32-minimal?expand=1 seem to reduce the resource usage down to Info: ICESTORM_LC: 3129/ 5280 59%.

mithro commented 6 years ago

This the minimal picorv32 shown at https://github.com/mithro/litex/blob/01ce22b0c15e281983084eec60ba11c6babac176/litex/soc/cores/cpu/picorv32/core.py and pull request #118 a pretty normal litex-soc using XIP spiflash has the following usage;

Info: Device utilisation:
Info:            ICESTORM_LC:  3439/ 5280    65%
Info:           ICESTORM_RAM:     6/   30    20%
Info:                  SB_IO:     7/   96     7%
Info:                  SB_GB:     8/    8   100%
Info:           ICESTORM_PLL:     0/    1     0%
Info:            SB_WARMBOOT:     0/    1     0%
Info:           ICESTORM_DSP:     0/    8     0%
Info:         ICESTORM_HFOSC:     0/    1     0%
Info:         ICESTORM_LFOSC:     0/    1     0%
Info:                 SB_I2C:     0/    2     0%
Info:                 SB_SPI:     0/    2     0%
Info:                 IO_I3C:     0/    2     0%
Info:            SB_LEDDA_IP:     0/    1     0%
Info:            SB_RGBA_DRV:     0/    1     0%
Info:         ICESTORM_SPRAM:     4/    4   100%
mithro commented 6 years ago

This is small enough I'm going to hand over to @cr1901 to create a proper variant and check things actually work.

enjoy-digital commented 6 years ago

Thanks for the number. To allow the FIFO to use block_ram, you can probably add buffered=True to https://github.com/enjoy-digital/litex/blob/master/litex/soc/cores/uart.py#L160. But for small configuration, we should probably reduce FIFO depth as you did.

enjoy-digital commented 6 years ago

Sorry, i just saw https://github.com/enjoy-digital/litex/pull/118 :)

mithro commented 5 years ago

@cr1901 - so what is the status here now?

cr1901 commented 5 years ago

@mithro This can be safely closed.