From 06cf8807c3bcfaa3b9b6e9a7ad5b47b2dfc340cc Mon Sep 17 00:00:00 2001 From: Peter McGoron Date: Tue, 20 Feb 2024 15:28:51 +0000 Subject: [PATCH] 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 | 5 +- doc/controller_manual.md | 7 ++ gateware/Makefile | 5 +- gateware/csr2mp.py | 167 ++----------------------------- gateware/rtl/bram/bram.v | 65 ------------ gateware/rtl/picorv32/picorv32.v | 3 + gateware/soc.py | 121 +++++++++++----------- swic/load_exec.py | 25 +++++ swic/test.c | 2 + 9 files changed, 108 insertions(+), 292 deletions(-) delete mode 100644 gateware/rtl/bram/bram.v create mode 100644 swic/load_exec.py diff --git a/build/Makefile b/build/Makefile index fb88b92..c377902 100644 --- a/build/Makefile +++ b/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 diff --git a/doc/controller_manual.md b/doc/controller_manual.md index b2d4cb0..5af47d4 100644 --- a/doc/controller_manual.md +++ b/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. diff --git a/gateware/Makefile b/gateware/Makefile index 02365fa..e11cefb 100644 --- a/gateware/Makefile +++ b/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 diff --git a/gateware/csr2mp.py b/gateware/csr2mp.py index 5613566..61ea08e 100644 --- a/gateware/csr2mp.py +++ b/gateware/csr2mp.py @@ -6,8 +6,7 @@ # source distribution. ####################################################################### # -# This file generates a Micropython module "mmio" with functions that -# do raw reads and writes to MMIO registers. +# This file generates memory locations # # TODO: Devicetree? @@ -17,163 +16,13 @@ import json import sys import mmio_descr -class CSRHandler: - """ - 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. +with open(sys.argv[1], 'rt') as f: + j = json.load(f) - :param csrjson: Filename of a LiteX "csr.json" file. - :param registers: A list of ``mmio_descr`` ``Descr``s. - :param outf: Output file. - """ - self.registers = registers - self.csrs = json.load(open(csrjson)) +print("from micropython import const") - def update_reg(self, reg): - """ - Fill in size information from bitwidth json file. +for key in j["csr_registers"]: + if key.startswith("picorv32"): + print(f'{key} = const({j["csr_registers"][key]["addr"]})') - :param reg: The register. - :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() +print(f'picorv32_ram = const({j["memories"]["picorv32_ram"]["base"]})') diff --git a/gateware/rtl/bram/bram.v b/gateware/rtl/bram/bram.v deleted file mode 100644 index 3815e9b..0000000 --- a/gateware/rtl/bram/bram.v +++ /dev/null @@ -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 diff --git a/gateware/rtl/picorv32/picorv32.v b/gateware/rtl/picorv32/picorv32.v index a9198a4..21fdae1 100644 --- a/gateware/rtl/picorv32/picorv32.v +++ b/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]); diff --git a/gateware/soc.py b/gateware/soc.py index 0ff7efa..ad3f97b 100644 --- a/gateware/soc.py +++ b/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 +# Copyright (c) 2013-2014 Sebastien Bourdeauducq +# Copyright (c) 2019 Gabriel L. Somlo # Copyright (c) 2015-2019 Florent Kermarrec # Copyright (c) 2020 Antmicro # Copyright (c) 2022 Victor Suarez Rovere @@ -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,24 +156,30 @@ 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 - statement. It assigns all output ports to avoid latches. + Avoiding 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): - asn += [ - self.buses[i].cyc.eq(self.slave.bus.cyc if i == j else 0), - self.buses[i].stb.eq(self.slave.bus.stb if i == j else 0), - self.buses[i].we.eq(self.slave.bus.we if i == j else 0), - self.buses[i].sel.eq(self.slave.bus.sel if i == j else 0), - self.buses[i].adr.eq(self.slave.bus.adr if i == j else 0), - self.buses[i].dat_w.eq(self.slave.bus.dat_w if i == j else 0), - self.buses[i].ack.eq(self.slave.bus.ack if i == j else 0), - self.buses[i].dat_r.eq(self.slave.bus.dat_r if i == j else 0), - ] + if current_case == j: + asn += [ + self.slave.bus.adr.eq(self.buses[j].adr), + self.slave.bus.dat_w.eq(self.buses[j].dat_w), + self.slave.bus.cyc.eq(self.buses[j].cyc), + self.slave.bus.stb.eq(self.buses[j].stb), + self.slave.bus.we.eq(self.buses[j].we), + self.slave.bus.sel.eq(self.buses[j].sel), + self.buses[j].dat_r.eq(self.slave.bus.dat_r), + 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 cases = {"default": assign_for_case(0)} @@ -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.bram_iface = self.bram_iface = bram_iface = PreemptiveInterface(2, bram) + self.submodules.ram = self.ram = SRAM(bramwid) + 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.trap = CSRStatus(1, name="picorv32_trap", description="Trap bit") + self.resetpin = CSRStorage(1, name="enable", description="PicoRV32 enable") + 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, @@ -316,10 +297,15 @@ class PicoRV32(Module, AutoCSR): "picorv32_io": SoCIORegion(origin=0x100000, size=0x100, mode="rw", cached=False), }, ) - - ic.add_slave("picorv32_bram", bram_iface.buses[1], bram.region) - ic.add_slave("picorv32_params", params.bus, params.region) - ic.add_master("picorv32_master", self.masterbus) + + self.ram_stb_cyc = CSRStatus(2) + self.ram_adr = CSRStatus(32) + 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: - d = self.ic.regions[region] + for region in self.bus.regions: + 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() - self.bus.add_slave("picorv32_master_bram", pr.bram_iface.buses[0], - SoCRegion(origin=None,size=pr.bram.region.size, cached=True)) - - def add_bram(self): - self.submodules.bram = br = BRAM(0x1FF) - self.bus.add_slave("bram", br.bus, br.region) + siz = 0x1000 + self.submodules.picorv32 = pr = PicoRV32(siz) + self.bus.add_slave("picorv32_ram", pr.ram_iface.buses[0], + SoCRegion(origin=None,size=siz, cached=True)) 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(): diff --git a/swic/load_exec.py b/swic/load_exec.py new file mode 100644 index 0000000..b5059df --- /dev/null +++ b/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] diff --git a/swic/test.c b/swic/test.c index c7235a1..e93b911 100644 --- a/swic/test.c +++ b/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 (;;) ; }