Skip to content

Commit

Permalink
soc/cores/vexiiriscv: update clocks + add video framebuffer support
Browse files Browse the repository at this point in the history
  • Loading branch information
@Dolu1990
Dolu1990 committed Sep 5, 2024
1 parent e62d84b commit 642cfbe
Showing 1 changed file with 60 additions and 5 deletions.
65 changes: 60 additions & 5 deletions litex/soc/cores/cpu/vexiiriscv/core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class VexiiRiscv(CPU):
with_axi3 = False
jtag_tap = False
jtag_instruction = False
with_cpu_clk = False
vexii_video = []
vexii_args = ""


Expand Down Expand Up @@ -131,12 +133,14 @@ def args_fill(parser):
cpu_group.add_argument("--with-jtag-instruction", action="store_true", help="Add a JTAG instruction port which implement tunneling for debugging (TAP not included).")
cpu_group.add_argument("--update-repo", default="recommended", choices=["latest","wipe+latest","recommended","wipe+recommended","no"], help="Specify how the VexiiRiscv & SpinalHDL repo should be updated (latest: update to HEAD, recommended: Update to known compatible version, no: Don't update, wipe+*: Do clean&reset before checkout)")
cpu_group.add_argument("--no-netlist-cache", action="store_true", help="Always (re-)build the netlist.")
cpu_group.add_argument("--with-cpu-clk", action="store_true", help="The CPUs will use a decoupled clock")
# cpu_group.add_argument("--with-fpu", action="store_true", help="Enable the F32/F64 FPU.")
# cpu_group.add_argument("--with-rvc", action="store_true", help="Enable the Compress ISA extension.")
cpu_group.add_argument("--l2-bytes", default=0, help="VexiiRiscv L2 bytes, default 128 KB.")
cpu_group.add_argument("--l2-ways", default=0, help="VexiiRiscv L2 ways, default 8.")
cpu_group.add_argument("--l2-self-flush", default=None, help="VexiiRiscv L2 ways will self flush on from,to,cycles")
cpu_group.add_argument("--with-axi3", action="store_true", help="mbus will be axi3 instead of axi4")
cpu_group.add_argument("--vexii-video", action="append", default=[], help="Add the memory coherent video controller")



Expand All @@ -148,7 +152,7 @@ def args_read(args):
vdir = get_data_mod("cpu", "vexiiriscv").data_location
ndir = os.path.join(vdir, "ext", "VexiiRiscv")

NaxRiscv.git_setup("VexiiRiscv", ndir, "https://github.com/SpinalHDL/VexiiRiscv.git", "dev", "36dad634", args.update_repo)
NaxRiscv.git_setup("VexiiRiscv", ndir, "https://github.com/SpinalHDL/VexiiRiscv.git", "dev", "a15ea92c", args.update_repo)

if not args.cpu_variant:
args.cpu_variant = "standard"
Expand Down Expand Up @@ -196,10 +200,12 @@ def args_read(args):
VexiiRiscv.cpu_count = args.cpu_count
if args.l2_bytes:
VexiiRiscv.l2_bytes = args.l2_bytes
VexiiRiscv.with_cpu_clk = args.with_cpu_clk
if args.l2_ways:
VexiiRiscv.l2_ways = args.l2_ways
if args.l2_self_flush:
VexiiRiscv.l2_self_flush = args.l2_self_flush
VexiiRiscv.vexii_video = args.vexii_video


def __init__(self, platform, variant):
Expand All @@ -220,8 +226,8 @@ def __init__(self, platform, variant):
# CPU Instance.
self.cpu_params = dict(
# Clk/Rst.
i_system_clk = ClockSignal("sys"),
i_system_reset = ResetSignal("sys") | self.reset,
i_litex_clk = ClockSignal("sys"),
i_litex_reset = ResetSignal("sys") | self.reset,

# Patcher/Tracer.
# o_patcher_tracer_valid = self.tracer_valid,
Expand Down Expand Up @@ -252,6 +258,12 @@ def __init__(self, platform, variant):
i_pBus_rresp = pbus.r.resp,
)

if VexiiRiscv.with_cpu_clk:
self.cpu_clk = Signal()
self.cpu_params.update(
i_cpu_clk = self.cpu_clk
)

if VexiiRiscv.with_dma:
self.dma_bus = dma_bus = axi.AXIInterface(data_width=VexiiRiscv.internal_bus_width, address_width=32, id_width=4)

Expand Down Expand Up @@ -306,6 +318,31 @@ def __init__(self, platform, variant):
o_dma_bus_rlast = dma_bus.r.last,
)

for video in VexiiRiscv.vexii_video:
args = {}
for i, val in enumerate(video.split(",")):
name, value = val.split("=")
args.update({name: value})
name = args["name"]
clk = Signal()
hsync = Signal()
vsync = Signal()
color_en = Signal()
color = Signal(16)
setattr(self, name + "_clk", clk)
setattr(self, name + "_hsync", hsync)
setattr(self, name + "_vsync", vsync)
setattr(self, name + "_color_en", color_en)
setattr(self, name + "_color", color)
self.cpu_params["o_" + name + "_clk"] = clk
self.cpu_params["o_" + name + "_hSync"] = hsync
self.cpu_params["o_" + name + "_vSync"] = vsync
self.cpu_params["o_" + name + "_colorEn"] = color_en
self.cpu_params["o_" + name + "_color"] = color




def set_reset_address(self, reset_address):
VexiiRiscv.reset_address = reset_address
VexiiRiscv.vexii_args += f" --reset-vector {reset_address}"
Expand All @@ -319,6 +356,7 @@ def generate_netlist_name():
md5_hash.update(str(VexiiRiscv.xlen).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.cpu_count).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.l2_bytes).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.with_cpu_clk).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.l2_ways).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.l2_self_flush).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.jtag_tap).encode('utf-8'))
Expand All @@ -327,6 +365,8 @@ def generate_netlist_name():
md5_hash.update(str(VexiiRiscv.with_axi3).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.memory_regions).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.vexii_args).encode('utf-8'))
md5_hash.update(str(VexiiRiscv.vexii_video).encode('utf-8'))

# md5_hash.update(str(VexiiRiscv.internal_bus_width).encode('utf-8'))


Expand All @@ -346,6 +386,8 @@ def generate_netlist():
gen_args.append(VexiiRiscv.vexii_args)
gen_args.append(f"--cpu-count={VexiiRiscv.cpu_count}")
gen_args.append(f"--l2-bytes={VexiiRiscv.l2_bytes}")
if VexiiRiscv.with_cpu_clk:
gen_args.append("--with-cpu-clk")
gen_args.append(f"--l2-ways={VexiiRiscv.l2_ways}")
if VexiiRiscv.l2_self_flush:
gen_args.append(f"--l2-self-flush={VexiiRiscv.l2_self_flush}")
Expand All @@ -361,6 +403,9 @@ def generate_netlist():
gen_args.append(f"--with-dma")
if(VexiiRiscv.with_axi3) :
gen_args.append(f"--with-axi3")
for arg in VexiiRiscv.vexii_video:
gen_args.append(f"--video {arg}")


cmd = f"""cd {ndir} && sbt "runMain vexiiriscv.soc.litex.SocGen {" ".join(gen_args)}\""""
print("VexiiRiscv generation command :")
Expand Down Expand Up @@ -469,7 +514,12 @@ def add_soc_components(self, soc):
if soc.get_build_name() == "sim":
self.comb += If(debug_ndmreset_rise, soc.crg.cd_sys.rst.eq(1))
else:
self.comb += If(debug_ndmreset_rise, soc.crg.rst.eq(1))
if hasattr(soc.crg.pll, "locked"):

This comment has been minimized.

Sign in to view

Copy link
@tristanitschner

tristanitschner Oct 12, 2024 •
edited
Loading

Sorry, first time using line comment feature, please be patient with me.

Locked is an output of the pll, but it is driven here, which currently breaks the soc vivado synthesis. I guess the intention here is to reset the pll either when debug_ndmreset high or locked is low.

This comment has been minimized.

Sign in to view

Copy link
@Dolu1990

Dolu1990 Oct 13, 2024

Author Collaborator

Hi ^^

The idea isn't realy to reset the PLL, but to have a way to reset the whole SoC without interrupting the clock generation. I didn't found a good way to do it so far, as the board targets generaly use the locked signal as a reset source ? By the past i was using the rst input of the PLL, but as this can interrupt the clock generation it make things go bad on efinix.

One fix would be to allow overriding the PLL locked signal value via something like :

    // In the xilinx PLL, add a signal buffer to locked
    def do_finalize(self):
        ....
        selfl.ocked_io = Signal()
        xxx.comb += self.locked.eq(self.locked_io)
        self.params.update(
            o_LOCKED       = self.locked_io,

But i'm don't think it is possible ?

This comment has been minimized.

Sign in to view

Copy link
@Dolu1990

Dolu1990 Oct 13, 2024

Author Collaborator

For reference, here is the current broken verilog :

wire          main_crg_locked;
always @(*) begin
    main_crg_locked <= 1'd0;
    if (main_basesoc_debug_ndmreset) begin
        main_crg_locked <= 1'd0;
    end
end
assign builder_impl_xilinxasyncresetsynchronizerimpl0 = (~main_crg_locked);
assign builder_impl_xilinxasyncresetsynchronizerimpl1 = (~main_crg_locked);
assign builder_impl_xilinxasyncresetsynchronizerimpl2 = (~main_crg_locked);
assign builder_impl_xilinxasyncresetsynchronizerimpl3 = (~main_crg_locked);
assign builder_impl_xilinxasyncresetsynchronizerimpl4 = (~main_crg_locked);
MMCME2_ADV #(
	// Parameters.
) MMCME2_ADV (
	// Inputs.
	// Outputs.
	.LOCKED   (main_crg_locked)
);

Realy need to intercept the reset going to all the syncronizer to be able to reset the whole system.

This comment has been minimized.

Sign in to view

Copy link
@tristanitschner

tristanitschner Oct 13, 2024

Yes of course, to reset the soc, the pll resetting itself wouldn't make any sense. So, the problem as you describe is, that adding a generic reset to all the board targets is too much change and breaks other projects?

This comment has been minimized.

Sign in to view

Copy link
@Dolu1990

Dolu1990 Oct 13, 2024

Author Collaborator

that adding a generic reset to all the board targets is too much change and breaks other projects?

Yes, right.
adding generic reset support to all boards would be pain.

Maybe adding such capability to, for instance :

class XilinxClocking(LiteXModule):
    def create_clkout(self, cd, freq, phase=0, buf="bufg", margin=1e-2, with_reset=True, reset_buf=None, ce=None):
       ...
        if with_reset: 
           ...
            self.specials += AsyncResetSynchronizer(cd, ~self.locked) # Would need to add an additional reset source here, which by default is set to 0

?

This comment has been minimized.

Sign in to view

Copy link
@Dolu1990

Dolu1990 Oct 15, 2024

Author Collaborator

Hi ^^

I pushed this workaround :
d5e4f9e

So now it should be ok on xilinx aswell.

Overall, i think we realy need a way to reset the whole SoC in a generic way.
The rocket port as the same issue, they just keept the ndmreset floating, so they can't reset the system via jtag.

Should i open an issue ?

This comment has been minimized.

Sign in to view

Copy link
@tristanitschner

tristanitschner Oct 15, 2024

The fix looks really clean to me.

Regarding a generic soc reset, I don't feel in a position to judge. I'm not that well-versed in the Litex codebase. If I find some time the coming days though, I will have a look at it and state my thoughts here.

This comment has been minimized.

Sign in to view

Copy link
@tristanitschner

tristanitschner Oct 26, 2024

While I haven't had time to look at the code, I though a little about resets.

Just to make clear: The problem with Efinix is that an unstable clock causes glitches which may potentially cause the circuit to go into an invalid (i.e. unreachable) state, due to no clock gating? If this is the case, this would indeed be very sad.

One of the advantages of FPGAs is that all registers can be initialized, such that hard resets are not necessary. I often design my circuits in such a way that there is no reset at all or merely a soft reset. This saves logic and circumvents the high fan-out of the hard reset.

Also for any circuit given a soft reset, there is an upper bound for the number of reset cycles. E.g. invalidating a one-way cache where the cache valid bit is part of the bram word needs at least the number of cache lines to reset. However, if the reset depends upon a bus response, the reset time may not even be deterministically upper bound.

Thus for handling a reset from all circuit components, either an upper bound must be negotiated at elaboration time or the reset must be a flow, where each circuit reset reports its reset status.

Sorry, if I went into too much detail here, but if a proper reset will be added to the framework, one might do it right from the start.

I think opening an issue here as you stated would be a good start.

This comment has been minimized.

Sign in to view

Copy link
@Dolu1990

Dolu1990 Oct 28, 2024

Author Collaborator

is that an unstable clock causes glitches which may potentially cause the circuit to go into an invalid (i.e. unreachable) state, due to no clock gating?

I don't think it is.
Instead i think it is that :
A register drive the reset pin the the PLL, this cut the clock instantly, so there is no more clock to make the register to clear himself and let the pll out of reset.
Dead lock of death

Me to i try to avoid reset as they have a cost on FPGA. But for this precise case, it is about allowing openocd to connect to the soft cpu jtag, and ask to reset the whole SoC excepted the debug logic itself.

So overall it idealy need this structure of reset :

- power on reset / external async reset
  -> Debug module
  -> Soc reset

- Debug module reset from JTAG (NDM_RESET)
  -> Soc reset

- Soc reset
  -> CPU / Interconnect / Peripherals

An issue has been open here :
#2109
self.comb += If(debug_ndmreset, soc.crg.pll.locked.eq(0))
elif hasattr(soc.crg, "rst"):
self.comb += If(debug_ndmreset_rise, soc.crg.rst.eq(1))
else:
raise Exception("Pll has no reset ?")

self.soc_bus = soc.bus # FIXME: Save SoC Bus instance to retrieve the final mem layout on finalization.

Expand All @@ -482,6 +532,7 @@ def add_memory_buses(self, address_width, data_width):
id_width = 8,
version = "axi3" if VexiiRiscv.with_axi3 else "axi4"
)
self.mBus_awallStrb = Signal()
self.memory_buses.append(mbus)

self.comb += mbus.aw.cache.eq(0xF)
Expand All @@ -507,7 +558,7 @@ def add_memory_buses(self, address_width, data_width):
o_mBus_awlen = mbus.aw.len,
o_mBus_awsize = mbus.aw.size,
o_mBus_awburst = mbus.aw.burst,
o_mBus_awallStrb = Open(),
o_mBus_awallStrb = self.mBus_awallStrb,
# W Channel.
o_mBus_wvalid = mbus.w.valid,
i_mBus_wready = mbus.w.ready,
Expand Down Expand Up @@ -573,6 +624,10 @@ def do_finalize(self):
mode += "c" if region.cached else ""
VexiiRiscv.memory_regions.append( (region.origin, region.size, mode, bus) )

from litex.build.efinix import EfinixPlatform
if isinstance(self.platform, EfinixPlatform):
VexiiRiscv.vexii_args = "--mmu-sync-read " + VexiiRiscv.vexii_args

self.generate_netlist_name()

# Do verilog instance.
Expand Down

0 comments on commit 642cfbe

Please sign in to comment.