Bug 484 - Write VHDL to expose CR and XER from Microwatt so single-stepping is possible
Summary: Write VHDL to expose CR and XER from Microwatt so single-stepping is possible
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- enhancement
Assignee: Luke Kenneth Casson Leighton
URL:
Depends on:
Blocks: 383
  Show dependency treegraph
 
Reported: 2020-09-02 01:36 BST by Cole Poirier
Modified: 2022-07-21 22:30 BST (History)
2 users (show)

See Also:
NLnet milestone: NLNet.2019.10.043.Wishbone
total budget (EUR) for completion of task and all subtasks: 150
budget (EUR) for this task, excluding subtasks' budget: 150
parent task for budget allocation: 383
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
lkcl = { amount = 150, submitted = 2022-06-25, paid = 2022-07-21 }


Attachments
Microwatt add dbg_cr attempt 1 (3.20 KB, patch)
2020-09-02 19:58 BST, Cole Poirier
Details | Diff
compiles (ghdl) version of cr dmi (3.21 KB, patch)
2020-09-03 08:42 BST, Luke Kenneth Casson Leighton
Details | Diff
Unsure if it compiles (ghdl) version of cr and xer dmi (3.28 KB, patch)
2020-09-07 22:55 BST, Cole Poirier
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cole Poirier 2020-09-02 01:36:55 BST
Additions need to be made to the microwatt vhdl code to allow CR and XER values to be accessed through the DMI debug interface such that single-stepping microwatt is possible. Then we will be able to compare libre-soc against it much more easily, enabling us to find bugs faster.
Comment 1 Luke Kenneth Casson Leighton 2020-09-02 11:29:15 BST
there are a number of parts to this:  1) add CR (first) and (2) add XER second.
CR is a priority.

this patch - which is on libresoc dmi.py - adds CR as an "address" (0b1000)
and adds a "CR data fetching interface"

https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=e5ee8ce9589d73ef01ac0a0f25452978a586b607

given the similarity to microwatt core_debug.vhdl adding the same to
that should be pretty obvious, starting with:

        -- CR register read port
        dbg_cr_req     : out std_ulogic;
        dbg_cr_ack     : in std_ulogic;
        dbg_cr_data    : in std_ulogic_vector(32 downto 0);

next will be to add the same signals to core.vhdl

    -- Debug actions
    signal dbg_core_stop: std_ulogic;
    signal dbg_core_rst: std_ulogic;
    signal dbg_icache_rst: std_ulogic;

    <here>

    signal dbg_gpr_req : std_ulogic;
    signal dbg_gpr_ack : std_ulogic;
    signal dbg_gpr_addr : gspr_index_t;
    signal dbg_gpr_data : std_ulogic_vector(63 downto 0);

and connect them up in the debug_0 port

    debug_0: entity work.core_debug
        generic map (
            LOG_LENGTH => LOG_LENGTH
            )
        port map (
            clk => clk,
            rst => rst_dbg,
            dmi_addr => dmi_addr,
            dmi_din => dmi_din,
            ....
            ....

            <here>

            dbg_gpr_req => dbg_gpr_req,
            dbg_gpr_ack => dbg_gpr_ack,
            dbg_gpr_addr => dbg_gpr_addr,
            dbg_gpr_data => dbg_gpr_data,

and add them to the cr_0 file port map

    cr_file_0: entity work.cr_file
        generic map (
            SIM => SIM,
            LOG_LENGTH => LOG_LENGTH
            )
        port map (
            clk => clk,
            d_in => decode2_to_cr_file,
            d_out => cr_file_to_decode2,
            w_in => writeback_to_cr_file,

            <here>

            sim_dump => sim_cr_dump,
            log_out => log_data(184 downto 172)
            );

then, in cr_file.vhdl the same signals need adding as have been added
everywhere else (dbg_cr_*) followed finally by a section similar to this
that puts crs_updated into dbg_cr_data

    -- asynchronous reads
    cr_read_0: process(all)
    begin
        -- just return the entire CR to make mfcrf easier for now
        if d_in.read = '1' then   <- dbg_cr_req here
            report "Reading CR " & to_hstring(crs_updated);
        end if;
        d_out.read_cr_data <= crs_updated; <- dbg_cr_data here

        <set dbg_cr_ack here>

    end process;


as a *second* step, getting XER can be added, which can be done by adding
another DMI register at address 0b1001

https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=8d3ee4eac8868f4116a13b900228918dce7c51d9

let's do the first step first though
Comment 2 Cole Poirier 2020-09-02 19:58:04 BST
Created attachment 99 [details]
Microwatt add dbg_cr attempt 1

Hi Luke, here's my first attempt at implementing the dbg_cr dmi interface from your very helpful instructions above. I think it's all there, but I can't be sure so I await your feedback.
Comment 3 Luke Kenneth Casson Leighton 2020-09-03 08:42:02 BST
Created attachment 100 [details]
compiles (ghdl) version of cr dmi

took 2 minutes to get it to compile, some missing semicolons, missing
keyword "signal", not much.  a diff-of-the-diff will be very instructive.
Comment 4 Cole Poirier 2020-09-07 01:14:27 BST
(In reply to Luke Kenneth Casson Leighton from comment #3)
> Created attachment 100 [details]
> compiles (ghdl) version of cr dmi
> 
> took 2 minutes to get it to compile, some missing semicolons, missing
> keyword "signal", not much.  a diff-of-the-diff will be very instructive.

Very cool, thank you. I'm excited take a look at the diff of the diff tomorrow. What are the next steps I should complete?
Comment 5 Luke Kenneth Casson Leighton 2020-09-07 01:31:49 BST
(In reply to Cole Poirier from comment #4)
> (In reply to Luke Kenneth Casson Leighton from comment #3)
> > Created attachment 100 [details]
> > compiles (ghdl) version of cr dmi
> > 
> > took 2 minutes to get it to compile, some missing semicolons, missing
> > keyword "signal", not much.  a diff-of-the-diff will be very instructive.
> 
> Very cool, thank you. I'm excited take a look at the diff of the diff
> tomorrow. What are the next steps I should complete?

given that the XER bits are also in cr_file.vhdl it's probably a good
idea to simply return those alongside the dbg_cr_data.

for a first pass that's probably enough to submit to microwatt.
Comment 6 Cole Poirier 2020-09-07 01:34:16 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> (In reply to Cole Poirier from comment #4)
> > (In reply to Luke Kenneth Casson Leighton from comment #3)
> > > Created attachment 100 [details]
> > > compiles (ghdl) version of cr dmi
> > > 
> > > took 2 minutes to get it to compile, some missing semicolons, missing
> > > keyword "signal", not much.  a diff-of-the-diff will be very instructive.
> > 
> > Very cool, thank you. I'm excited take a look at the diff of the diff
> > tomorrow. What are the next steps I should complete?
> 
> given that the XER bits are also in cr_file.vhdl it's probably a good
> idea to simply return those alongside the dbg_cr_data.
> 
> for a first pass that's probably enough to submit to microwatt.

Thanks. Will make an attempt at this tomorrow, may have some questions once I make my first attempt at it.
Comment 7 Cole Poirier 2020-09-07 22:17:10 BST
Just reviewed the diff of the diff, yes, very instructive. Starting on returning the XER bits alongside the dbg_cr_data since they are also in cr_file.vhdl. Essentially I should replicate the process from adding CR to the DMI interface for XER?
Comment 8 Luke Kenneth Casson Leighton 2020-09-07 22:42:21 BST
(In reply to Cole Poirier from comment #7)
> Just reviewed the diff of the diff, yes, very instructive. Starting on
> returning the XER bits alongside the dbg_cr_data since they are also in
> cr_file.vhdl. Essentially I should replicate the process from adding CR to
> the DMI interface for XER?

no, that would result in unecessary duplicated effort. exactly as a said: *only* add dbg_xer_data alongside dbg_cr_data, and set it equal to xerc_updated.
put it just after the line that sets dbg_cr_data.
Comment 9 Cole Poirier 2020-09-07 22:55:10 BST
Created attachment 103 [details]
Unsure if it compiles (ghdl) version of cr and xer dmi
Comment 10 Cole Poirier 2020-09-07 22:56:20 BST
(In reply to Cole Poirier from comment #9)
> Created attachment 103 [details]
> Unsure if it compiles (ghdl) version of cr and xer dmi

Ok I did exactly as you said and added only *only* dbg_xer_data alongside dbg_cr_data, and set it equal to xerc_updated, putting it just after the line that sets dbg_cr_data.