Progress on PicoRV32

1) The PicoRV32 bus was not generated correctly. Running "finalize" on
   the bus, which is what the SoC does, does not generate the bus logic
   correctly. I don't know if  this is a bug or if the SoC bus generator is
   only meant to be used in the main SoC.

   Currently the bus logic is copied from the LiteX finalize code.

2) Add test micropython code to load code.

3) Removed BRAM. The Wishbone cache was messing with the direct
   implementation of the BRAM because the BRAM did not implement all the
   bus features correctly. LiteX has a Wishbone "SRAM" module, and despite
   it's name it can also generate BRAM if there are available BRAM. This is
   how the ROM and the startup RAM are implemented. The PicoRV32 ram
   is now using this SRAM.

build/Makefile

@@ -45,11 +45,12 @@ hardware-execute:
 hardware-shell:
 	docker exec -ti upsilon-hardware /bin/bash -l
 hardware-get:
+	docker cp upsilon-hardware:/home/user/upsilon/gateware/build/digilent_arty/gateware/digilent_arty.v ../boot/
 	docker cp upsilon-hardware:/home/user/upsilon/gateware/build/digilent_arty/gateware/digilent_arty.bit ../boot/
 	docker cp upsilon-hardware:/home/user/upsilon/gateware/arty.dtb ../boot/
-#	docker cp upsilon-hardware:/home/user/upsilon/gateware/mmio.py ../boot/
 	docker cp upsilon-hardware:/home/user/upsilon/gateware/csr.json ../boot/
-#	docker cp upsilon-hardware:/home/user/upsilon/gateware/picorv32.json ../boot/
+	docker cp upsilon-hardware:/home/user/upsilon/gateware/picorv32.json ../boot/
+	docker cp upsilon-hardware:/home/user/upsilon/gateware/mmio.py ../boot/
 hardware-clean:
 	-docker container stop upsilon-hardware
 	-docker container rm upsilon-hardware

doc/controller_manual.md

@@ -42,3 +42,10 @@ automatically copied to `boot/mmio.py`).
 `comm` contains higher level wrappers for DAC and ADC pins. This module is
 documented well enough that you should be able to read it and understand
 how to use it.
+
+# FAQ
+
+## SCP Is Not Working
+
+SCP by default uses SFTP, which dropbear does not support. Pass `-O` to all
+SCP invocations to use the legacy SCP protocol.

gateware/Makefile

@@ -8,12 +8,15 @@
 
 DEVICETREE_GEN_DIR=.
 
-all: build/digilent_arty/digilent_arty.bit arty.dtb
+all: build/digilent_arty/digilent_arty.bit arty.dtb mmio.py
 
 csr.json build/digilent_arty/digilent_arty.bit: soc.py
 	cd rtl && make
 	python3 soc.py
 
+mmio.py: csr.json csr2mp.py
+	python3 csr2mp.py csr.json > mmio.py
+
 clean:
 	rm -rf build csr.json arty.dts arty.dtb mmio.py
 

gateware/csr2mp.py

@@ -6,8 +6,7 @@
 # source distribution.
 #######################################################################
 #
-# This file generates a Micropython module "mmio" with functions that
+# This file generates memory locations
-# do raw reads and writes to MMIO registers.
 #
 # TODO: Devicetree?
 
@@ -17,163 +16,13 @@ import json
 import sys
 import mmio_descr
 
-class CSRHandler:
+with open(sys.argv[1], 'rt') as f:
-    """
+    j = json.load(f)
-    Class that wraps the CSR file and fills in registers with information
-    from those files.
-    """
-    def __init__(self, csrjson, registers):
-        """
-        Reads in the CSR files.
 
-        :param csrjson: Filename of a LiteX "csr.json" file.
+print("from micropython import const")
-        :param registers: A list of ``mmio_descr`` ``Descr``s.
-        :param outf: Output file.
-        """
-        self.registers = registers
-        self.csrs = json.load(open(csrjson))
 
-    def update_reg(self, reg):
+for key in j["csr_registers"]:
-        """
+    if key.startswith("picorv32"):
-        Fill in size information from bitwidth json file.
+        print(f'{key} = const({j["csr_registers"][key]["addr"]})')
 
-        :param reg: The register.
+print(f'picorv32_ram = const({j["memories"]["picorv32_ram"]["base"]})')
-        :raises Exception: When the bit width exceeds 64.
-        """
-        regsize = None
-        b = reg.blen
-        if b <= 8:
-            regsize = 8
-        elif b <= 16:
-            regsize = 16
-        elif b <= 32:
-            regsize = 32
-        elif b <= 64:
-            regsize = 64
-        else:
-            raise Exception(f"unsupported width {b} in {reg.name}")
-        setattr(reg, "regsize", regsize)
-
-    def get_reg_addr(self, reg, num=None):
-        """
-        Get address of register.
-
-        :param reg: The register.
-        :param num: Select which register number. Registers without
-          numerical suffixes require ``None``.
-        :return: The address.
-        """
-        if num is None:
-            regname = f"base_{reg.name}"
-        else:
-            regname = f"base_{reg.name}_{num}"
-        return self.csrs["csr_registers"][regname]["addr"]
-
-class InterfaceGenerator:
-    """
-    Interface for file generation. Implement the unimplemented functions
-    to generate a CSR interface for another language.
-    """
-
-    def __init__(self, csr, outf):
-        """
-        :param CSRHandler csr: 
-        :param FileIO outf:
-        """
-        self.outf = outf
-        self.csr = csr
-
-    def print(self, *args):
-        """
-        Print to the file specified in the initializer and without
-        newlines.
-        """
-        print(*args, end='', file=self.outf)
-
-    def fun(self, reg, optype):
-        """ Print function for reads/writes to register. """
-        pass
-    def header(self):
-        """ Print header of file. """
-        pass
-
-    def print_file(self):
-        self.print(self.header())
-        for r in self.csr.registers:
-            self.print(self.fun(r, "read"))
-            if not r.rwperm != "read-only":
-                self.print(self.fun(r, "write"))
-
-class MicropythonGenerator(InterfaceGenerator):
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-
-    def get_accessor(self, reg, num):
-        addr = self.csr.get_reg_addr(reg, num)
-        if reg.regsize in [8, 16, 32]:
-            return [f"machine.mem{reg.regsize}[{addr}]"]
-        return [f"machine.mem32[{addr + 4}]", f"machine.mem32[{addr}]"]
-
-    def print_write_register(self, indent, varname, reg, num):
-        acc = self.get_accessor(reg, num)
-        if len(acc) == 1:
-            return f'{indent}{acc[0]} = {varname}\n'
-        else:
-            assert len(acc) == 2
-            # Little Endian. See linux kernel, include/linux/litex.h
-            return f'{indent}{acc[1]} = {varname} & 0xFFFFFFFF\n' + \
-                   f'{indent}{acc[0]} = {varname} >> 32\n'
-
-    def print_read_register(self, indent, varname, reg, num):
-        acc = self.get_accessor(reg, num)
-        if len(acc) == 1:
-            return f'{indent}return {acc[0]}\n'
-        else:
-            assert len(acc) == 2
-            return f'{indent}return {acc[0]} | ({acc[1]} << 32)\n'
-
-    def fun(self, reg, optype):
-        rs = ""
-        def a(s):
-            nonlocal rs
-            rs = rs + s
-        a(f'def {optype}_{reg.name}(')
-
-        printed_argument = False
-        if optype == 'write':
-            a('val')
-            printed_argument = True
-            pfun = self.print_write_register
-        else:
-            pfun = self.print_read_register
-
-        if reg.num != 1:
-            if printed_argument:
-                a(', ')
-            a('num')
-        a('):\n')
-
-        if reg.num == 1:
-            a(pfun('\t', 'val', reg, None))
-        else:
-            for i in range(0,reg.num):
-                if i == 0:
-                    a(f'\tif ')
-                else:
-                    a(f'\telif ')
-                a(f'num == {i}:\n')
-                a(pfun('\t\t', 'val', reg, i))
-            a(f'\telse:\n')
-            a(f'\t\traise Exception(regnum)\n')
-        a('\n')
-
-        return rs
-
-    def header(self):
-        return "import machine\n"
-
-if __name__ == "__main__":
-   csrh = CSRHandler(sys.argv[1], mmio_descr.registers)
-   for r in mmio_descr.registers:
-       csrh.update_reg(r)
-   MicropythonGenerator(csrh, sys.stdout).print_file()

gateware/rtl/bram/bram.v

@@ -1,65 +0,0 @@
-/* Copyright 2024 (C) Peter McGoron
- * This file is a part of Upsilon, a free and open source software project.
- * For license terms, refer to the files in `doc/copying` in the Upsilon
- * source distribution.
- *
- * This BRAM can only handle aligned accesses.
- */
-module bram #(
-	/* Width of the memory bus */
-	parameter BUS_WID = 32,
-	/* Width of a request. */
-	parameter WORD_WID = 32,
-	/* Bitmask used to extract the RAM location in the buffer. */
-	parameter ADDR_MASK = 32'h1FFF
-) (
-	input clk,
-
-	input wb_cyc,
-	input wb_stb,
-	input wb_we,
-	input [4-1:0] wb_sel,
-	input [BUS_WID-1:0] wb_addr,
-	input [BUS_WID-1:0] wb_dat_w,
-	output reg wb_ack,
-	output reg [BUS_WID-1:0] wb_dat_r
-);
-
-initial wb_ack <= 0;
-initial wb_dat_r <= 0;
-
-/* When the size of the memory is a power of 2, the mask is the
- * last addressable index in the array.
- *
- * Since this buffer stores words, this is divided by 4 (32 bits).
- * When accessing a single byte, the address
- * 0b......Xab
- * is shifted to the right by two bits, throwing away "ab". This indexes
- * the 32 bit word that contains the address. This applies to halfwords
- * and words as long as the accesses are aligned.
- */
-(* ram_style = "block" *)
-reg [WORD_WID-1:0] buffer [(ADDR_MASK >> 2):0];
-
-/* Current index into the buffer. */
-wire [13-1:0] ind = (wb_addr & ADDR_MASK) >> 2;
-
-always @ (posedge clk) if (wb_cyc && wb_stb && !wb_ack) begin
-	wb_ack <= 1;
-	if (wb_we) begin
-		if (wb_sel[0])
-			buffer[ind][7:0] <= wb_dat_w[7:0];
-		if (wb_sel[1])
-			buffer[ind][15:8] <= wb_dat_w[15:8];
-		if (wb_sel[2])
-			buffer[ind][23:16] <= wb_dat_w[23:16];
-		if (wb_sel[3])
-			buffer[ind][31:24] <= wb_dat_w[31:24];
-	end else begin
-		wb_dat_r <= buffer[ind];
-	end
-end else if (!wb_stb) begin
-	wb_ack <= 0;
-end
-
-endmodule

gateware/rtl/picorv32/picorv32.v

@@ -2892,6 +2892,8 @@ module picorv32_wb #(
 	output        trace_valid,
 	output [35:0] trace_data,
 
+	output debug_state,
+
 	output mem_instr
 );
 	wire        mem_valid;
@@ -2989,6 +2991,7 @@ module picorv32_wb #(
 	localparam WBEND = 2'b10;
 
 	reg [1:0] state;
+	assign debug_state = state;
 
 	wire we;
 	assign we = (mem_wstrb[0] | mem_wstrb[1] | mem_wstrb[2] | mem_wstrb[3]);

gateware/soc.py

@@ -3,6 +3,9 @@
 # Portions of this file incorporate code licensed under the
 # BSD 2-Clause License.
 #
+# Copyright (c) 2014-2022 Florent Kermarrec <florent@enjoy-digital.fr>
+# Copyright (c) 2013-2014 Sebastien Bourdeauducq <sb@m-labs.hk>
+# Copyright (c) 2019 Gabriel L. Somlo <somlo@cmu.edu>
 # Copyright (c) 2015-2019 Florent Kermarrec <florent@enjoy-digital.fr>
 # Copyright (c) 2020 Antmicro <www.antmicro.com>
 # Copyright (c) 2022 Victor Suarez Rovere <suarezvictor@gmail.com>
@@ -50,7 +53,7 @@ from litex.soc.integration.soc_core import SoCCore
 from litex.soc.integration.soc import SoCRegion, SoCBusHandler, SoCIORegion
 from litex.soc.cores.clock import S7PLL, S7IDELAYCTRL
 from litex.soc.interconnect.csr import AutoCSR, Module, CSRStorage, CSRStatus
-from litex.soc.interconnect.wishbone import Interface
+from litex.soc.interconnect.wishbone import Interface, SRAM, InterconnectShared
 
 from litedram.phy import s7ddrphy
 from litedram.modules import MT41K128M16
@@ -153,23 +156,29 @@ class PreemptiveInterface(Module, AutoCSR):
          the combinatorial block, the If statement is constructed in a
          for loop.
  
-         The "assign_for_case" function constructs the body of the If
+         Avoiding latches:
-         statement. It assigns all output ports to avoid latches.
+         Left hand sign (assignment) is always an input.
          """
  
-         def assign_for_case(i):
+         def assign_for_case(current_case):
              asn = [ ]
  
              for j in range(masters_len):
+                 if current_case == j:
                      asn += [
-                     self.buses[i].cyc.eq(self.slave.bus.cyc if i == j else 0),
+                         self.slave.bus.adr.eq(self.buses[j].adr),
-                     self.buses[i].stb.eq(self.slave.bus.stb if i == j else 0),
+                         self.slave.bus.dat_w.eq(self.buses[j].dat_w),
-                     self.buses[i].we.eq(self.slave.bus.we if i == j else 0),
+                         self.slave.bus.cyc.eq(self.buses[j].cyc),
-                     self.buses[i].sel.eq(self.slave.bus.sel if i == j else 0),
+                         self.slave.bus.stb.eq(self.buses[j].stb),
-                     self.buses[i].adr.eq(self.slave.bus.adr if i == j else 0),
+                         self.slave.bus.we.eq(self.buses[j].we),
-                     self.buses[i].dat_w.eq(self.slave.bus.dat_w if i == j else 0),
+                         self.slave.bus.sel.eq(self.buses[j].sel),
-                     self.buses[i].ack.eq(self.slave.bus.ack if i == j else 0),
+                         self.buses[j].dat_r.eq(self.slave.bus.dat_r),
-                     self.buses[i].dat_r.eq(self.slave.bus.dat_r if i == j else 0),
+                         self.buses[j].ack.eq(self.slave.bus.ack),
+                     ]
+                 else:
+                     asn += [
+                         self.buses[j].dat_r.eq(0),
+                         self.buses[j].ack.eq(self.buses[j].cyc & self.buses[j].stb),
                      ]
              return asn
  
@@ -190,7 +199,7 @@ class SPIMaster(Module):
                 spi_cycle_half_wait = 1,
         ):
 
-        self.bus = Interface(data_width = 32, address_width=32, addressing="byte")
+        self.bus = Interface(data_width = 32, address_width=32, addressing="word")
         self.region = SoCRegion(size=0x10, cached=False)
 
         self.comb += [
@@ -262,48 +271,20 @@ class ControlLoopParameters(Module, AutoCSR):
                  )
          ]
  
-class BRAM(Module):
-     """ A BRAM (block ram) is a memory store that is completely separate from
-     the system RAM. They are much smaller.
-     """
-     def __init__(self, addr_mask, origin=None):
-         """
-         :param addr_mask: Mask which defines the amount of bytes accessable
-           by the BRAM.
-         :param origin: Origin of the BRAM module region. This is seen by the
-           subordinate master, not the usual master.
-         """
-         self.bus = Interface(data_width=32, address_width=32, addressing="byte")
- 
-         # Non-IO (i.e. MMIO) regions need to be cached
-         self.region = SoCRegion(origin=origin, size=addr_mask+1, cached=True)
- 
-         self.specials += Instance("bram",
-                 p_ADDR_MASK = addr_mask,
-                 i_clk = ClockSignal(),
-                 i_wb_cyc = self.bus.cyc,
-                 i_wb_stb = self.bus.stb,
-                 i_wb_we = self.bus.we,
-                 i_wb_sel = self.bus.sel,
-                 i_wb_addr = self.bus.adr,
-                 i_wb_dat_w = self.bus.dat_w,
-                 o_wb_ack = self.bus.ack,
-                 o_wb_dat_r = self.bus.dat_r,
-         )
- 
 class PicoRV32(Module, AutoCSR):
      def __init__(self, bramwid=0x1000):
          self.submodules.params = params = ControlLoopParameters()
-         self.submodules.bram = self.bram = bram = BRAM(bramwid-1, origin=0x10000)
+         self.submodules.ram = self.ram = SRAM(bramwid)
-         self.submodules.bram_iface = self.bram_iface = bram_iface = PreemptiveInterface(2, bram)
+         ram_region = SoCRegion(size=bramwid, origin=0x10000, cached=True)
+         self.submodules.ram_iface = self.ram_iface = ram_iface = PreemptiveInterface(2, self.ram)
  
          # This is the PicoRV32 master
          self.masterbus = Interface(data_width=32, address_width=32, addressing="byte")
  
-         self.resetpin = CSRStorage(1, name="picorv32_reset", description="PicoRV32 reset")
+         self.resetpin = CSRStorage(1, name="enable", description="PicoRV32 enable")
-         self.trap = CSRStatus(1, name="picorv32_trap", description="Trap bit")
+         self.trap = CSRStatus(1, name="trap", description="Trap bit")
  
-         self.ic = ic = SoCBusHandler(
+         self.bus = bus = SoCBusHandler(
              standard="wishbone",
              data_width=32,
              address_width=32,
@@ -317,9 +298,14 @@ class PicoRV32(Module, AutoCSR):
              },
          )
 
-         ic.add_slave("picorv32_bram", bram_iface.buses[1], bram.region)
+         self.ram_stb_cyc = CSRStatus(2)
-         ic.add_slave("picorv32_params", params.bus, params.region)
+         self.ram_adr = CSRStatus(32)
-         ic.add_master("picorv32_master", self.masterbus)
+         self.comb += self.ram_stb_cyc.status.eq(ram_iface.buses[1].stb << 1 | ram_iface.buses[1].cyc)
+         self.comb += self.ram_adr.status.eq(ram_iface.buses[1].adr)
+
+         bus.add_slave("picorv32_ram", ram_iface.buses[1], ram_region)
+         bus.add_slave("picorv32_params", params.bus, params.region)
+         bus.add_master("picorv32_master", self.masterbus)
  
          # NOTE: need to compile to these extact instructions
          self.specials += Instance("picorv32_wb",
@@ -355,14 +341,24 @@ class PicoRV32(Module, AutoCSR):
  
              o_trace_valid = Signal(),
              o_trace_data = Signal(36),
+             o_debug_state = Signal(2),
+         )
+
+         # dumb hack: "self.bus.finalize()" in do_finalize()
+         # should work but doesn't
+         self.submodules.picobus = InterconnectShared(
+                 list(self.bus.masters.values()),
+                 slaves = [(self.bus.regions[n].decoder(self.bus), s) for n, s in self.bus.slaves.items()],
+                 register = self.bus.interconnect_register,
+                 timeout_cycles = self.bus.timeout,
          )
  
      def do_finalize(self):
-         self.ic.finalize()
+         #self.bus.finalize()
          jsondata = {}
  
-         for region in self.ic.regions:
+         for region in self.bus.regions:
-             d = self.ic.regions[region]
+             d = self.bus.regions[region]
              jsondata[region] = {
                      "origin": d.origin,
                      "size": d.size,
@@ -411,13 +407,10 @@ class UpsilonSoC(SoCCore):
             self.add_constant(f"{ip_name}{seg_num}", int(ip_byte))
 
     def add_picorv32(self):
-        self.submodules.picorv32 = pr = PicoRV32()
+        siz = 0x1000
-        self.bus.add_slave("picorv32_master_bram", pr.bram_iface.buses[0],
+        self.submodules.picorv32 = pr = PicoRV32(siz)
-                SoCRegion(origin=None,size=pr.bram.region.size, cached=True))
+        self.bus.add_slave("picorv32_ram", pr.ram_iface.buses[0],
-
+                SoCRegion(origin=None,size=siz, cached=True))
-    def add_bram(self):
-        self.submodules.bram = br = BRAM(0x1FF)
-        self.bus.add_slave("bram", br.bus, br.region)
 
     def __init__(self,
                  variant="a7-100",
@@ -445,7 +438,6 @@ class UpsilonSoC(SoCCore):
         platform.add_source("rtl/spi/spi_master_preprocessed.v")
         platform.add_source("rtl/spi/spi_master_ss.v")
         platform.add_source("rtl/spi/spi_master_ss_wb.v")
-        platform.add_source("rtl/bram/bram.v")
 
         # SoCCore does not have sane defaults (no integrated rom)
         SoCCore.__init__(self,
@@ -503,7 +495,6 @@ class UpsilonSoC(SoCCore):
         )
         self.bus.add_slave("spi0", self.spi0.bus, self.spi0.region)
         
-        self.add_bram()
         self.add_picorv32()
 
 def main():

swic/load_exec.py

@@ -0,0 +1,25 @@
+import machine
+from mmio import *
+
+def read_file(filename):
+    with open(filename, 'rb') as f:
+        return f.read()
+
+def run_program(prog, cl_I):
+    # Reset PicoRV32
+    machine.mem32[picorv32_enable] = 0
+    machine.mem32[picorv32_ram_iface_master_select] = 0
+
+    offset = picorv32_ram
+    for b in prog:
+        machine.mem8[offset] = b
+        offset += 1
+
+    for i in range(len(prog)):
+        assert machine.mem8[picorv32_ram + i] == prog[i]
+
+    machine.mem32[picorv32_ram_iface_master_select] = 1
+    assert machine.mem8[picorv32_ram] == 0
+    machine.mem32[picorv32_params_cl_I] = cl_I
+    machine.mem32[picorv32_enable] = 1
+    return machine.mem32[picorv32_params_zset]

swic/test.c

@@ -4,7 +4,9 @@ void _start(void)
 {
 	volatile uint32_t *write = (volatile uint32_t *)(0x100000 + 0x10);
 	volatile uint32_t *read = (volatile uint32_t *)( 0x100000 + 0x0);
+	volatile uint32_t *next_read = (volatile uint32_t *)(0x100FF);
 	*write = *read;
+	*next_read = 0xdeadbeef;
 
 	for (;;) ;
 }