Bug 450 - Create MMU from microwatt mmu.vhdl
Summary: Create MMU from microwatt mmu.vhdl
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: Normal normal
Assignee: Luke Kenneth Casson Leighton
URL:
Depends on: 525
Blocks: 262 469 491
  Show dependency treegraph
 
Reported: 2020-08-06 19:00 BST by Cole Poirier
Modified: 2022-06-16 16:05 BST (History)
3 users (show)

See Also:
NLnet milestone: NLnet.2019.02.012
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments
add crc32 debug statements (2.31 KB, patch)
2021-05-03 12:51 BST, Luke Kenneth Casson Leighton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cole Poirier 2020-08-06 19:00:49 BST
The goal is to achieve a working MMU by translating microwatt's vhdl mmu code

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/experiment/mmu.py;hb=HEAD

WARNING about nextpnr-ecp5

https://freenode.irclog.whitequark.org/nmigen/2021-04-28#29784082;
Comment 1 Cole Poirier 2020-08-06 20:22:23 BST
It took a very long time to manually format everything to properly to 80 char width, but it was worth it, it's so clear, pretty, and easy to understand now.
Comment 2 Luke Kenneth Casson Leighton 2020-08-06 21:30:19 BST
(In reply to Cole Poirier from comment #1)
> It took a very long time to manually format everything to properly to 80
> char width, but it was worth it, it's so clear, pretty, and easy to
> understand now.

yyep.  and now you have room on-screen to open a lot more xterms and
see a lot more files side-by-side at the same time.

one of which is going to be common.vhdl

# entity mmu is
#     port (
#         clk   : in std_ulogic;
#         rst   : in std_ulogic;
#
#         l_in  : in Loadstore1ToMmuType;
#         l_out : out MmuToLoadstore1Type;
#
#         d_out : out MmuToDcacheType;
#         d_in  : in DcacheToMmuType;
#
#         i_out : out MmuToIcacheType
#         );
# end mmu;
    def __init__(self, l_in, l_out, d_out, d_in, i_out):
        self.l_in = l_in
        self.l_out = l_out
        self.d_out = d_out
        self.d_in = d_in
        self.i_out = i_out

this needs to be:

    def __init__(self):
        self.l_in = LoadStore1ToMMUType()
        self.l_out = ...
        self.d_out = ...
        self.d_in = ...
        self.i_out = ...

where you look in common.vhdl for Loststore1ToMmuType and turn that
into a class that inherits from RecordObject.

have a look in.... somewhere else i've done these translations...
soc/debug/dmi.py

and compare that to core_debug.vhdl.
Comment 3 Cole Poirier 2020-08-08 21:00:20 BST
(In reply to Luke Kenneth Casson Leighton from comment #2)
> (In reply to Cole Poirier from comment #1)
> > It took a very long time to manually format everything to properly to 80
> > char width, but it was worth it, it's so clear, pretty, and easy to
> > understand now.
> 
> yyep.  and now you have room on-screen to open a lot more xterms and
> see a lot more files side-by-side at the same time.
> 
> one of which is going to be common.vhdl
> 
> # entity mmu is
> #     port (
> #         clk   : in std_ulogic;
> #         rst   : in std_ulogic;
> #
> #         l_in  : in Loadstore1ToMmuType;
> #         l_out : out MmuToLoadstore1Type;
> #
> #         d_out : out MmuToDcacheType;
> #         d_in  : in DcacheToMmuType;
> #
> #         i_out : out MmuToIcacheType
> #         );
> # end mmu;
>     def __init__(self, l_in, l_out, d_out, d_in, i_out):
>         self.l_in = l_in
>         self.l_out = l_out
>         self.d_out = d_out
>         self.d_in = d_in
>         self.i_out = i_out
> 
> this needs to be:
> 
>     def __init__(self):
>         self.l_in = LoadStore1ToMMUType()
>         self.l_out = ...
>         self.d_out = ...
>         self.d_in = ...
>         self.i_out = ...
> 
> where you look in common.vhdl for Loststore1ToMmuType and turn that
> into a class that inherits from RecordObject.
> 
> have a look in.... somewhere else i've done these translations...
> soc/debug/dmi.py
> 
> and compare that to core_debug.vhdl.

Thanks, that's very helpful!

VHDL -> nmigen translation question, is the following correctly translated? or should (5 downto 4) be translated as [5:3:-1] ? or [3:6] ?
 #       case r.shift(5 downto 4) is
         with m.Switch(r.shift[0:2]):
 #           when "00" =>
 #               sh1 := r.addr(42 downto 12);
             with m.Case(0):
                 comb += sh1.eq(r.addr[20:52])
 #           when "01" =>
 #               sh1 := r.addr(58 downto 28);
Comment 4 Luke Kenneth Casson Leighton 2020-08-08 21:32:56 BST
(In reply to Cole Poirier from comment #3)

> VHDL -> nmigen translation question, is the following correctly translated?
> or should (5 downto 4) be translated


remember that 63 downto 0 was translated as Signal(64) and that those are accessed as 0 to 63 in that order (0 *upto* 63)?

therefore "5 downto 4" is "4 upto 5" however in python you add 1 to the end.

so "5 downto 4" becomes [4:5+1] which is [4:6]

the & operator when replacing it with Cat  is particular confusing but basically the same 

except that constants such as x'567' are NOT inverted, those you count the number of bits (12 in x'567') and translate it to

    Const(0x567, 12)
Comment 5 Cole Poirier 2020-08-08 21:41:41 BST
(In reply to Luke Kenneth Casson Leighton from comment #4)
> (In reply to Cole Poirier from comment #3)
> 
> > VHDL -> nmigen translation question, is the following correctly translated?
> > or should (5 downto 4) be translated
> 
> 
> remember that 63 downto 0 was translated as Signal(64) and that those are
> accessed as 0 to 63 in that order (0 *upto* 63)?
> 
> therefore "5 downto 4" is "4 upto 5" however in python you add 1 to the end.
> 
> so "5 downto 4" becomes [4:5+1] which is [4:6]

Ok very, very helpful, thank you.

> the & operator when replacing it with Cat  is particular confusing but
> basically the same 
> 
> except that constants such as x'567' are NOT inverted, those you count the
> number of bits (12 in x'567') and translate it to
> 
>     Const(0x567, 12)

Well.. that's a tad brain melting, still fun though :)
Comment 6 Luke Kenneth Casson Leighton 2020-08-10 10:22:51 BST
 566 #               pt_valid := r.pt3_valid;
 567             with m.Else():
 568                 comb += pgtbl.eq(r.pt3_valid)
 569 #           end if;

messed up.  should be similar to previous lines except 3 not 0
Comment 7 Luke Kenneth Casson Leighton 2020-08-10 10:43:29 BST
 572 #           rts := unsigned('0' & pgtbl(62 downto 61) & pgtbl(7 downto 5));
 573             # rts == radix tree size, number of address bits being translated
 574             comb += rts.eq((0 & pgtbl[61:63] & pgtbl[5:8]).as_unsigned())
 575 

this is a Cat like i explained in the earlier comment.  you swap the order of the things separated by & but DO NOT swap the order of bits in the const.

    x'1243' & foo

becomes

     Cat(foo, Const(0x1243, 16))

and

     '0111' & bar

becomes

    Cat(bar, Const(0b0111, 4))
Comment 8 Luke Kenneth Casson Leighton 2020-08-10 11:20:44 BST
delete this entire block and de-indent the if else.

nmigen automatically handles reset with an argument "reset=x" to Signal constructor which defaults to zero.

given that all the values here are zero the entire block, including rst and including the if else is totally unnecessary.



 199             with m.If(rst == 1):
 200 #               r.state <= IDLE;
 201 #               r.valid <= '0';
 202 #               r.pt0_valid <= '0';
 203 #               r.pt3_valid <= '0';
 204 #               r.prtbl <= (others => '0');
 205                 sync += r.state.eq(State.IDLE)
 206                 sync += r.valid.eq(0)
 207                 sync += r.pt0_valid.eq(0)
 208                 sync += r.pt3_valid.eq(0)
 209                 # TODO value should be vhdl (others => '0') in nmigen
 210                 sync += r.prtbl.eq(0)
 211 #           else
Comment 9 Luke Kenneth Casson Leighton 2020-08-10 11:23:19 BST
 196 #       if rising_edge(clk) then
 197         with m.If(rising_edge):

this is "sync".  there is absolutely no need for rising edge detection because that is what sync is and is for.

therefore the if may be removed entirely and the block inside de-indented 4 spaces.
Comment 10 Luke Kenneth Casson Leighton 2020-08-10 11:34:10 BST
delete these exact lines.  do not do anything other than delete these lines exactly as instructed.  do not try to do anything fancy such as "play guessing games" such as occurred in the mul unit test.  just... delete lines 181 to 194.

the reason is that nmigen does not have the concept of "processes".  it is an artificial construct.

therefore the creation of a totally separate module named MMU0 was unnecessary.

by deleting these lines, the lines *below* join the elaborate function *above*.  this is what is desired.  do not try to do anything else or try to guess what might be needed: just delete lines 181 to 194.

 181 # mmu_0: process(clk)
 182 class MMU0(Elaboratable):
 183     def __init__(self, clk):
 184         self.clk = clk
 185 
 186 # begin
 187     def elaborate(self, platform):
 188 
 189         m = Module()
 190 
 191         comb = m.d.comb
 192         sync = m.d.sync
 193 
 194         rst = ResetSignal()
Comment 11 Luke Kenneth Casson Leighton 2020-08-10 11:47:28 BST
^ is the exclusive-or operator.  you mean to use |

 603 #                   -- RB[IS] != 0 or RB[AP] != 0, or for slbia
 604 #                   v.inval_all := l_in.slbia or l_in.addr(11) or l_in.
 605 #                                  addr(10) or l_in.addr(7) or l_in.addr(6)
 606 #                                  or l_in.addr(5);
 607                     # Invalidate all iTLB/dTLB entries for tlbie with
 608                     # RB[IS] != 0 or RB[AP] != 0, or for slbia
 609                     comb += v.inval_all.eq(l_in.slbia ^ l_in.addr[11] ^
 610                                            l_in.addr[10] ^ l_in.addr[7] ^
 611                                            l_in.addr[6] ^ l_in.addr[5])
Comment 12 Luke Kenneth Casson Leighton 2020-08-10 11:49:17 BST
btw do these modifications *one at a time*, committing them with a link to the relevant comment.

*DO NOT* do all of them in one single massive commit.
Comment 13 Cole Poirier 2020-08-10 17:28:54 BST
(In reply to Luke Kenneth Casson Leighton from comment #12)
> btw do these modifications *one at a time*, committing them with a link to
> the relevant comment.
> 
> *DO NOT* do all of them in one single massive commit.

Thanks for all the help! Did an individual commit per comment for most of them, combined two comments into one commit where it made sense, i.e. the deletions at the top of what was formerly MMU0(process)
Comment 14 Luke Kenneth Casson Leighton 2020-08-10 17:30:58 BST
note how the git diff wraps?  did you remember to do "git diff" before each
commit?


diff --git a/src/soc/experiment/mmu.py b/src/soc/experiment/mmu.py
index e4779962..d5cba5a2 100644
--- a/src/soc/experiment/mmu.py
+++ b/src/soc/experiment/mmu.py
@@ -572,7 +572,7 @@ class AddrShifter(Elaboratable):
 #           -- rts == radix tree size, # address bits being translated
 #           rts := unsigned('0' & pgtbl(62 downto 61) & pgtbl(7 downto 5));
             # rts == radix tree size, number of address bits being translated
-            comb += rts.eq(((Cat(Const(0b0, 1) , Cat(pgtbl[61:63], pgtbl[5:8]))).as_unsigned())
+            comb += rts.eq((0 & pgtbl[61:63] & pgtbl[5:8]).as_unsigned())
 
 #           -- mbits == # address bits to index top level of tree
 #           mbits := unsigned('0' & pgtbl(4 downto 0));
Comment 15 Luke Kenneth Casson Leighton 2020-08-10 17:33:46 BST
(In reply to Cole Poirier from comment #13)
> (In reply to Luke Kenneth Casson Leighton from comment #12)
> > btw do these modifications *one at a time*, committing them with a link to
> > the relevant comment.
> > 
> > *DO NOT* do all of them in one single massive commit.
> 
> Thanks for all the help! Did an individual commit per comment for most of
> them, combined two comments into one commit where it made sense, i.e. the
> deletions at the top of what was formerly MMU0(process)

sense to whom? :)

now i cannot review and confirm each commit.... because they're merged.

how can i tell that this does what is needed, by looking at it?

can you tell, easily and at a glance, which lines were deleted and which
were indented?


diff --git a/src/soc/experiment/mmu.py b/src/soc/experiment/mmu.py
index 083b2b66..e4779962 100644
--- a/src/soc/experiment/mmu.py
+++ b/src/soc/experiment/mmu.py
@@ -176,47 +176,83 @@ class MMU(Elaboratable):
         with m.Else():
             comb += l_out.sprval.eq(0x00000000 & r)
 
-#       if rin.valid = '1' then
-#           report "MMU got tlb miss for " & to_hstring(rin.addr);
-#       end if;
-        with m.If(rin.valid == 1):
-            print(f"MMU got tlb miss for {rin.addr}")
 
-#       if l_out.done = '1' then
-#           report "MMU completing op without error";
-#       end if;
-        with m.If(l_out.done == 1):
-            print("MMU completing op without error")
 
-#       if l_out.err = '1' then
-#           report "MMU completing op with err invalid=" &
+# mmu_0: process(clk)
+class MMU0(Elaboratable):
+    def __init__(self, clk):
+        self.clk = clk
+
+# begin
+    def elaborate(self, platform):
+
+        m = Module()
+
+        comb = m.d.comb
+        sync = m.d.sync
+
+        rst = ResetSignal()
+
+#       if rising_edge(clk) then
+        with m.If(rising_edge):
+#           if rst = '1' then
+            with m.If(rst == 1):
+#               r.state <= IDLE;
+#               r.valid <= '0';
+#               r.pt0_valid <= '0';
+#               r.pt3_valid <= '0';
+#               r.prtbl <= (others => '0');
+                sync += r.state.eq(State.IDLE)
+                sync += r.valid.eq(0)
+                sync += r.pt0_valid.eq(0)
+                sync += r.pt3_valid.eq(0)
+                # TODO value should be vhdl (others => '0') in nmigen
+                sync += r.prtbl.eq(0)
+#           else
+            with m.Else():
+#               if rin.valid = '1' then
+#                   report "MMU got tlb miss for " & to_hstring(rin.addr);
+#               end if;
+                with m.If(rin.valid == 1):
+                    print(f"MMU got tlb miss for {rin.addr}")
+
+#               if l_out.done = '1' then
+#                   report "MMU completing op without error";
+#               end if;
+                with m.If(l_out.done == 1):
+                    print("MMU completing op without error")
+
+#               if l_out.err = '1' then
+#                   report "MMU completing op with err invalid=" &
 #                   std_ulogic'image(l_out.invalid) & " badtree=" &
Comment 16 Cole Poirier 2020-08-10 17:35:16 BST
(In reply to Luke Kenneth Casson Leighton from comment #14)
> note how the git diff wraps?  did you remember to do "git diff" before each
> commit?
> 
> 
> diff --git a/src/soc/experiment/mmu.py b/src/soc/experiment/mmu.py
> index e4779962..d5cba5a2 100644
> --- a/src/soc/experiment/mmu.py
> +++ b/src/soc/experiment/mmu.py
> @@ -572,7 +572,7 @@ class AddrShifter(Elaboratable):
>  #           -- rts == radix tree size, # address bits being translated
>  #           rts := unsigned('0' & pgtbl(62 downto 61) & pgtbl(7 downto 5));
>              # rts == radix tree size, number of address bits being
> translated
> -            comb += rts.eq(((Cat(Const(0b0, 1) , Cat(pgtbl[61:63],
> pgtbl[5:8]))).as_unsigned())
> +            comb += rts.eq((0 & pgtbl[61:63] & pgtbl[5:8]).as_unsigned())
>  
>  #           -- mbits == # address bits to index top level of tree
>  #           mbits := unsigned('0' & pgtbl(4 downto 0));

Oops sorry about that! Will fix now.
Comment 17 Cole Poirier 2020-08-10 17:39:39 BST
(In reply to Luke Kenneth Casson Leighton from comment #15)
> (In reply to Cole Poirier from comment #13)
> > (In reply to Luke Kenneth Casson Leighton from comment #12)
> > > btw do these modifications *one at a time*, committing them with a link to
> > > the relevant comment.
> > > 
> > > *DO NOT* do all of them in one single massive commit.
> > 
> > Thanks for all the help! Did an individual commit per comment for most of
> > them, combined two comments into one commit where it made sense, i.e. the
> > deletions at the top of what was formerly MMU0(process)
> 
> sense to whom? :)
> 
> now i cannot review and confirm each commit.... because they're merged.
> 
> how can i tell that this does what is needed, by looking at it?
> 
> can you tell, easily and at a glance, which lines were deleted and which
> were indented?

Yes, but that's only because I *just* did it. Sorry, should have followed your directions to the letter. I being far less experience in git than you didn't realize the indentation would show up as additions. So, yes should have followed to the letter, given that you foresaw this negative consequence.
 
> diff --git a/src/soc/experiment/mmu.py b/src/soc/experiment/mmu.py
> index 083b2b66..e4779962 100644
> --- a/src/soc/experiment/mmu.py
> +++ b/src/soc/experiment/mmu.py
> @@ -176,47 +176,83 @@ class MMU(Elaboratable):
>          with m.Else():
>              comb += l_out.sprval.eq(0x00000000 & r)
>  
> -#       if rin.valid = '1' then
> -#           report "MMU got tlb miss for " & to_hstring(rin.addr);
> -#       end if;
> -        with m.If(rin.valid == 1):
> -            print(f"MMU got tlb miss for {rin.addr}")
>  
> -#       if l_out.done = '1' then
> -#           report "MMU completing op without error";
> -#       end if;
> -        with m.If(l_out.done == 1):
> -            print("MMU completing op without error")
>  
> -#       if l_out.err = '1' then
> -#           report "MMU completing op with err invalid=" &
> +# mmu_0: process(clk)
> +class MMU0(Elaboratable):
> +    def __init__(self, clk):
> +        self.clk = clk
> +
> +# begin
> +    def elaborate(self, platform):
> +
> +        m = Module()
> +
> +        comb = m.d.comb
> +        sync = m.d.sync
> +
> +        rst = ResetSignal()
> +
> +#       if rising_edge(clk) then
> +        with m.If(rising_edge):
> +#           if rst = '1' then
> +            with m.If(rst == 1):
> +#               r.state <= IDLE;
> +#               r.valid <= '0';
> +#               r.pt0_valid <= '0';
> +#               r.pt3_valid <= '0';
> +#               r.prtbl <= (others => '0');
> +                sync += r.state.eq(State.IDLE)
> +                sync += r.valid.eq(0)
> +                sync += r.pt0_valid.eq(0)
> +                sync += r.pt3_valid.eq(0)
> +                # TODO value should be vhdl (others => '0') in nmigen
> +                sync += r.prtbl.eq(0)
> +#           else
> +            with m.Else():
> +#               if rin.valid = '1' then
> +#                   report "MMU got tlb miss for " & to_hstring(rin.addr);
> +#               end if;
> +                with m.If(rin.valid == 1):
> +                    print(f"MMU got tlb miss for {rin.addr}")
> +
> +#               if l_out.done = '1' then
> +#                   report "MMU completing op without error";
> +#               end if;
> +                with m.If(l_out.done == 1):
> +                    print("MMU completing op without error")
> +
> +#               if l_out.err = '1' then
> +#                   report "MMU completing op with err invalid=" &
>  #                   std_ulogic'image(l_out.invalid) & " badtree=" &
Comment 18 Cole Poirier 2020-08-10 17:42:34 BST
(In reply to Cole Poirier from comment #16)
> (In reply to Luke Kenneth Casson Leighton from comment #14)
> > note how the git diff wraps?  did you remember to do "git diff" before each
> > commit?
> > 
> > 
> > diff --git a/src/soc/experiment/mmu.py b/src/soc/experiment/mmu.py
> > index e4779962..d5cba5a2 100644
> > --- a/src/soc/experiment/mmu.py
> > +++ b/src/soc/experiment/mmu.py
> > @@ -572,7 +572,7 @@ class AddrShifter(Elaboratable):
> >  #           -- rts == radix tree size, # address bits being translated
> >  #           rts := unsigned('0' & pgtbl(62 downto 61) & pgtbl(7 downto 5));
> >              # rts == radix tree size, number of address bits being
> > translated
> > -            comb += rts.eq(((Cat(Const(0b0, 1) , Cat(pgtbl[61:63],
> > pgtbl[5:8]))).as_unsigned())
> > +            comb += rts.eq((0 & pgtbl[61:63] & pgtbl[5:8]).as_unsigned())
> >  
> >  #           -- mbits == # address bits to index top level of tree
> >  #           mbits := unsigned('0' & pgtbl(4 downto 0));
> 
> Oops sorry about that! Will fix now.

Hold on... Isn't that git diff showing me fixing formatting that wrapped? Cause that's what it looks like now that I'm taking a second closer look. Can you clarify?
Comment 19 Luke Kenneth Casson Leighton 2020-08-10 18:01:26 BST
(In reply to Cole Poirier from comment #18)

> Hold on... Isn't that git diff showing me fixing formatting that wrapped?
> Cause that's what it looks like now that I'm taking a second closer look.
> Can you clarify?

it's over 80 characters.

you _are_ doing "git diff" prior to "git commit" each and every time without fail, and you _are_ doing that in an 80 wide xterm therefore you should have noticed that the diff wrapped even though you _are_ keeping the editor window width to an 80 char hard limit and just happened just this once not to notice...

... yes?
Comment 20 Cole Poirier 2020-08-10 18:05:35 BST
(In reply to Luke Kenneth Casson Leighton from comment #19)
> (In reply to Cole Poirier from comment #18)
> 
> > Hold on... Isn't that git diff showing me fixing formatting that wrapped?
> > Cause that's what it looks like now that I'm taking a second closer look.
> > Can you clarify?
> 
> it's over 80 characters.
> 
> you _are_ doing "git diff" prior to "git commit" each and every time without
> fail, and you _are_ doing that in an 80 wide xterm therefore you should have
> noticed that the diff wrapped even though you _are_ keeping the editor
> window width to an 80 char hard limit and just happened just this once not
> to notice...
> 
> ... yes?

Ah found it, I thought you were refering to the line of code, then I saw it was the comment that wrapped. Right?

```
> >              # rts == radix tree size, number of address bits being
> > translated
```
Comment 21 Cole Poirier 2020-08-10 18:09:54 BST
(In reply to Luke Kenneth Casson Leighton from comment #19)
> (In reply to Cole Poirier from comment #18)
> 
> > Hold on... Isn't that git diff showing me fixing formatting that wrapped?
> > Cause that's what it looks like now that I'm taking a second closer look.
> > Can you clarify?
> 
> it's over 80 characters.
> 
> you _are_ doing "git diff" prior to "git commit" each and every time without
> fail, and you _are_ doing that in an 80 wide xterm therefore you should have
> noticed that the diff wrapped even though you _are_ keeping the editor
> window width to an 80 char hard limit and just happened just this once not
> to notice...
> 
> ... yes?

Ummm... I'm using git commit -v? I just didn't carefully go over it line by line like I should have, sorry, working on correcting that. I'm using 80 char xterms, in the file it looks like there's two characters worth of margin to the right of that line... Oh!! It's the commit diff that is wrapping because it adds chars in the left margin! I finally understand what you've been trying to tell me. That even if my files are < 80 wide, the commit diff needs them to actually be 78 chars wide?
Comment 22 Cole Poirier 2020-08-10 23:20:13 BST
Is this going to require modifications/additions to LDSTUnit, L0Cache, L1Cache?

Just taking a look at microwatt's:

* common.vhdl https://github.com/antonblanchard/microwatt/blob/master/common.vhdl
* loadstore1.vhdl https://github.com/antonblanchard/microwatt/blob/master/loadstore1.vhdl
* icache.vhdl https://github.com/antonblanchard/microwatt/blob/master/icache.vhdl
* dcache.vhdl https://github.com/antonblanchard/microwatt/blob/master/dcache.vhdl

that's the impression I'm getting..

I know a lot of work has been done on LDSTUnit, and the cache was part of this. To what extent are the I-cache and D-cache of the 180nm test ASIC complete?
Comment 23 Luke Kenneth Casson Leighton 2020-08-10 23:32:55 BST
(In reply to Cole Poirier from comment #22)

> I know a lot of work has been done on LDSTUnit, and the cache was part of
> this. To what extent are the I-cache and D-cache of the 180nm test ASIC
> complete?

they are part of minerva so should "work" however the code had to be expanded from 32 to 64 bit.

given the slow speed of the FSM issuer they are not a high priority.

l.
Comment 24 Cole Poirier 2020-08-10 23:59:51 BST
(In reply to Luke Kenneth Casson Leighton from comment #23)
> (In reply to Cole Poirier from comment #22)
> 
> > I know a lot of work has been done on LDSTUnit, and the cache was part of
> > this. To what extent are the I-cache and D-cache of the 180nm test ASIC
> > complete?
> 
> they are part of minerva so should "work" however the code had to be
> expanded from 32 to 64 bit.
> 
> given the slow speed of the FSM issuer they are not a high priority.
> 
> l.

Ah, that's good. I'll take a proper look at the minerva code now, then.
Comment 25 Luke Kenneth Casson Leighton 2020-08-11 00:04:23 BST
 737 #           nonzero := or(r.addr(61 downto 31) and not finalmask(
 738 #                      30 downto 0));
 739             comb += mbits.eq(0 & r.mask_size)
 740             comb += v.shift.eq(r.shift + (31 -12) - mbits)
 741             comb += nonzero.eq('''TODO wrap in or (?)'''r.addr[31:62]
 742                                & (~finalmask[0:31]))


ok.

so.

or(somebits) in nmigen is somebits.bool()

however, this produces a 1 bit "thing" which, if &'ed with a multi-bit "thing" will ONLY TEST THE FIRST BIT.

at line 742 you have taken 32 bits of finslmask and inverted them.

this produces a 32 bit inverted signal.

then, you expected that to be ANDed with a test of whether raddr[31:62] is nonzero but this test is only 1 bit.

therefore the & will take ONLY THE FIRST BIT OF FINALMASK.

what you actually want is this:

(r.addr[31:62] & ~finalmask[0:31]).bool()
Comment 26 Luke Kenneth Casson Leighton 2020-08-11 00:07:05 BST
(In reply to Cole Poirier from comment #24)

> Ah, that's good. I'll take a proper look at the minerva code now, then.

not worth spending time on yet excrpt out of curiosity in passing.

eventually once all pieces dcache.vhdl icache.vhdl plru.vhdl mmu.vhdl are done then it is worthwhile looking and converting to LoadStoreInterface and FetchInterface.

until then which will be several days yet it is a distraction.
Comment 27 Cole Poirier 2020-08-11 01:11:57 BST
(In reply to Luke Kenneth Casson Leighton from comment #26)
> (In reply to Cole Poirier from comment #24)
> 
> > Ah, that's good. I'll take a proper look at the minerva code now, then.
> 
> not worth spending time on yet excrpt out of curiosity in passing.
> 
> eventually once all pieces dcache.vhdl icache.vhdl plru.vhdl mmu.vhdl are
> done then it is worthwhile looking and converting to LoadStoreInterface and
> FetchInterface.
> 
> until then which will be several days yet it is a distraction.

Oh... I took your previous comment to mean that I would *not* be converting dcache.vhdl nor icache.vhdl. Am I correct now in saying that all of "all pieces dcache.vhdl icache.vhdl plru.vhdl mmu.vhdl" must be converted to get mmu.py to work?
Comment 28 Luke Kenneth Casson Leighton 2020-08-11 03:11:45 BST
(In reply to Cole Poirier from comment #27)
> (In reply to Luke Kenneth Casson Leighton from comment #26)
> > (In reply to Cole Poirier from comment #24)
> > 
> > > Ah, that's good. I'll take a proper look at the minerva code now, then.
> > 
> > not worth spending time on yet excrpt out of curiosity in passing.
> > 
> > eventually once all pieces dcache.vhdl icache.vhdl plru.vhdl mmu.vhdl are
> > done then it is worthwhile looking and converting to LoadStoreInterface and
> > FetchInterface.
> > 
> > until then which will be several days yet it is a distraction.
> 
> Oh... I took your previous comment to mean that I would *not* be converting
> dcache.vhdl nor icache.vhdl. Am I correct now in saying that all of "all
> pieces dcache.vhdl icache.vhdl plru.vhdl mmu.vhdl" must be converted to get
> mmu.py to work?

oh it will "work", it just will be useless unless tightly integrated into a d and i cache (just like in microwatt)

without us having to have a seriously indepth technical understanding of computer architectural design ... yes.

given that minerva's code does not include an mmu, examining minerva's cache code is of little value in guiding us towards such an integrated mmu-cache-memory subsystem (just like in microwatt)... *except* the one abstract base class: LoadStoreInterface because that is what the vhdl code must be modified to conform to (eventually)
Comment 29 Cole Poirier 2020-08-11 03:25:21 BST
(In reply to Luke Kenneth Casson Leighton from comment #28)

> oh it will "work", it just will be useless unless tightly integrated into a
> d and i cache (just like in microwatt)

Ok, that makes more sense.
 
> without us having to have a seriously indepth technical understanding of
> computer architectural design ... yes.

Makes further sesne.

> given that minerva's code does not include an mmu, examining minerva's cache
> code is of little value in guiding us towards such an integrated
> mmu-cache-memory subsystem (just like in microwatt)... *except* the one
> abstract base class: LoadStoreInterface because that is what the vhdl code
> must be modified to conform to (eventually)

Ah good to know, thanks :)
Comment 30 Cole Poirier 2020-08-12 01:24:23 BST
Several questions:

How do I specify a signal as a non-zero positive integer in nmigen? in vhdl it's specified with the keyword 'positive'.


How should the assignment of a a value to a variable with the representation
"(others => '0')" in vhdl, *outside* of a case statement, be translated into nmieng?

For example:
```
tlb_data := (others => '0')
```

Is this translated correctly? i.e. the number of bits in a hex constant?
```
#       -- mask_count has to be >= 5
#       m := x"001f";
# mask_count has to be >= 5
# TODO check hex conts with lkcl
comb += mask.eq(Const(0x001F, 5)
```

Is this correct? If a statement X is wrapped in an or in vhdl, in nmigen it is 
or'd with 0? i.e. 0 | X?

```
# nonzero := or(r.addr(61 downto 31) and
#            not finalmask(30 downto 0));
# TODO need lkcl to check this is correct
comb += nonzero.eq(0 | Cat((~finalmask[0:31]),
                   r.addr[31:62]
                  ))
```

How do I start testing/getting this module to work? Should I just move on to translating icache.vhdl(~800 lines), d.cache.vhdl(~1600 lines), etc. because their incredibly tight integration with mmu.vhdl means that I should work concurrently on making them 'run' (work all of them together once they have all been translated 99%)?

Also should AddrShifter, MaskGen, etc. classes be made submodules of MMU?

Am I corectly initing things, and inheriting in nmigen in a way that properly corresponds to the vhdl, or even correct in terms of general python?

I know this is a lot of questions, but I'm unsure as to how I should progress from here, as I know I'm doing things wrong, but I don't know what specifically I have done wrong. My plan as of this moment is to start translating icache.vhdl into nmigen tomorrow.
Comment 31 Luke Kenneth Casson Leighton 2020-08-12 09:33:55 BST
(In reply to Cole Poirier from comment #30)
> Several questions:
> 
> How do I specify a signal as a non-zero positive integer in nmigen?

unsigned is the default.

> How should the assignment of a a value to a variable with the representation
> "(others => '0')" in vhdl,

drop it.

> For example:
> ```
> tlb_data := (others => '0')

assign to zero.

> ```
> 
> Is this translated correctly?

no.

> #       m := x"001f";

m = x"NNNN".  length of N (a hexadecimal digit) is 4.  4 time 4 is 16.

> Is this correct? If a statement X is wrapped in an or in vhdl, in nmigen it
> is 
> or'd with 0? i.e. 0 | X?
> 
> ```
> # nonzero := or(r.addr(61 downto 31) and
> #            not finalmask(30 downto 0));

i have already told you the answer to this one.  please read the comments from the web interface, not via email, and read them *in full*, going back repeatedly to make sure that you have understood them and not missed any.

> How do I start testing/getting this module to work?

usual way: writing a unit test (for example translating mmu_tb.vhdl)

> Should I just move on to
> translating icache.vhdl(~800 lines), d.cache.vhdl(~1600 lines), etc. because
> their incredibly tight integration with mmu.vhdl means that I should work
> concurrently on making them 'run' (work all of them together once they have
> all been translated 99%)?

no.  start on the unit test.  first, though, you've missed these RecordObjects:

 133         self.l_in  = Loadstore1ToMmuType()
 134         self.l_out = MmuToLoadstore1Type()
 135         self.d_out = MmuToDcacheType()
 136         self.d_in  = DcacheToMmuType()
 137         self.i_out = MmuToIcacheType()


> Also should AddrShifter, MaskGen, etc. classes be made submodules of MMU?

AddrShifter needs to go, to be replaced by simply "shift" i.e. "<<".

MaskGen i will review and work out if they are needed.

> Am I corectly initing things, and inheriting in nmigen in a way that
> properly corresponds to the vhdl, or even correct in terms of general python?

no idea, i will need to do a line-by-line compare.

> I know this is a lot of questions, but I'm unsure as to how I should
> progress from here, as I know I'm doing things wrong, but I don't know what
> specifically I have done wrong. My plan as of this moment is to start
> translating icache.vhdl into nmigen tomorrow.

add those RecordObjects first.
Comment 32 Cole Poirier 2020-08-12 17:43:54 BST
(In reply to Luke Kenneth Casson Leighton from comment #31)
> (In reply to Cole Poirier from comment #30)
> > Several questions:
> > 
> > How do I specify a signal as a non-zero positive integer in nmigen?
> 
> unsigned is the default.

I'm aware. What I'm looking for is not n >= 0 but n > 0. Does this exist in nmigen?

> > How should the assignment of a a value to a variable with the representation
> > "(others => '0')" in vhdl,
> 
> drop it.

As in no variable assignment to tlb_data? Then why bother putting that line in the vhdl at all?

> > For example:
> > ```
> > tlb_data := (others => '0')
> 
> assign to zero.

This conflicts with your above statement. So assign tlb_data the value of 0? Why is it not then written as 'tlb_data := '0'
 
> > ```
> > 
> > Is this translated correctly?
> 
> no.
> 
> > #       m := x"001f";
> 
> m = x"NNNN".  length of N (a hexadecimal digit) is 4.  4 time 4 is 16.

Ah perfect, that makes sense. Will fix.

> > Is this correct? If a statement X is wrapped in an or in vhdl, in nmigen it
> > is 
> > or'd with 0? i.e. 0 | X?
> > 
> > ```
> > # nonzero := or(r.addr(61 downto 31) and
> > #            not finalmask(30 downto 0));
> 
> i have already told you the answer to this one.  please read the comments
> from the web interface, not via email, and read them *in full*, going back
> repeatedly to make sure that you have understood them and not missed any.

Ah sorry was just that comment I somehow managed to miss, I have this thread open in a dedicated window next to my nmigen and vhdl xterms, just an accidental miss.

> > How do I start testing/getting this module to work?
> 
> usual way: writing a unit test (for example translating mmu_tb.vhdl)

Sounds good. What does the suffix _tb stand for by the way?

> > Should I just move on to
> > translating icache.vhdl(~800 lines), d.cache.vhdl(~1600 lines), etc. because
> > their incredibly tight integration with mmu.vhdl means that I should work
> > concurrently on making them 'run' (work all of them together once they have
> > all been translated 99%)?
> 
> no.  start on the unit test.  first, though, you've missed these
> RecordObjects:

I did, will fix now.

>  133         self.l_in  = Loadstore1ToMmuType()
>  134         self.l_out = MmuToLoadstore1Type()
>  135         self.d_out = MmuToDcacheType()
>  136         self.d_in  = DcacheToMmuType()
>  137         self.i_out = MmuToIcacheType()
> 
> 
> > Also should AddrShifter, MaskGen, etc. classes be made submodules of MMU?
> 
> AddrShifter needs to go, to be replaced by simply "shift" i.e. "<<".

Oh are we that lucky? I will delete that class now then.

> MaskGen i will review and work out if they are needed.

Thanks.

> > Am I corectly initing things, and inheriting in nmigen in a way that
> > properly corresponds to the vhdl, or even correct in terms of general python?
> 
> no idea, i will need to do a line-by-line compare.

Indeed.
 
> > I know this is a lot of questions, but I'm unsure as to how I should
> > progress from here, as I know I'm doing things wrong, but I don't know what
> > specifically I have done wrong. My plan as of this moment is to start
> > translating icache.vhdl into nmigen tomorrow.
> 
> add those RecordObjects first.

Doing so now.
Comment 33 Luke Kenneth Casson Leighton 2020-08-12 17:48:13 BST
(In reply to Cole Poirier from comment #32)
> (In reply to Luke Kenneth Casson Leighton from comment #31)
> > (In reply to Cole Poirier from comment #30)
> > > Several questions:
> > > 
> > > How do I specify a signal as a non-zero positive integer in nmigen?
> > 
> > unsigned is the default.
> 
> I'm aware. What I'm looking for is not n >= 0 but n > 0. Does this exist in
> nmigen?

with unsigned, "> 0" is equivalent to != 0.

> > > How should the assignment of a a value to a variable with the representation
> > > "(others => '0')" in vhdl,
> > 
> > drop it.
> 
> As in no variable assignment to tlb_data?

correct.

> Then why bother putting that line
> in the vhdl at all?

convenience for vhdl.  we are not doing vhdl.
 
> > > For example:
> > > ```
> > > tlb_data := (others => '0')
> > 
> > assign to zero.
> 
> This conflicts with your above statement. So assign tlb_data the value of 0?
> Why is it not then written as 'tlb_data := '0'

because it specifies all "other" bits not specified in the assignment between the brackets.


> > usual way: writing a unit test (for example translating mmu_tb.vhdl)
> 
> Sounds good. What does the suffix _tb stand for by the way?

standard convention for files that include a "**T**est **B**ench"

> > AddrShifter needs to go, to be replaced by simply "shift" i.e. "<<".
> 
> Oh are we that lucky? I will delete that class now then.

it's an optimisation based on the assumption that yosys will not be able to correctly produce optimal FPGA connections.

this will interfere with us doing an ASIC.
Comment 34 Cole Poirier 2020-08-12 17:55:32 BST
(In reply to Luke Kenneth Casson Leighton from comment #33)
> (In reply to Cole Poirier from comment #32)
> with unsigned, "> 0" is equivalent to != 0.

Ok, my question remains, how to I specify a signal that *must* be != 0?

> > As in no variable assignment to tlb_data?
> 
> correct.

Cool.

> > Then why bother putting that line
> > in the vhdl at all?
> 
> convenience for vhdl.  we are not doing vhdl.

Thank goodness for that.

> > This conflicts with your above statement. So assign tlb_data the value of 0?
> > Why is it not then written as 'tlb_data := '0'
> 
> because it specifies all "other" bits not specified in the assignment
> between the brackets.

Hmm, interesting.

> > Sounds good. What does the suffix _tb stand for by the way?
> 
> standard convention for files that include a "**T**est **B**ench"

Makes sense, thanks.
 
> > Oh are we that lucky? I will delete that class now then.
> 
> it's an optimisation based on the assumption that yosys will not be able to
> correctly produce optimal FPGA connections.
> 
> this will interfere with us doing an ASIC.

Woohoo! ;-)
Comment 35 Jacob Lifshay 2020-08-12 17:59:13 BST
(In reply to Cole Poirier from comment #34)
> (In reply to Luke Kenneth Casson Leighton from comment #33)
> > (In reply to Cole Poirier from comment #32)
> > with unsigned, "> 0" is equivalent to != 0.
> 
> Ok, my question remains, how to I specify a signal that *must* be != 0?

nmigen doesn't have that, just use an unsigned Signal.
Comment 36 Cole Poirier 2020-08-12 18:00:49 BST
(In reply to Jacob Lifshay from comment #35)
> (In reply to Cole Poirier from comment #34)
> > (In reply to Luke Kenneth Casson Leighton from comment #33)
> > > (In reply to Cole Poirier from comment #32)
> > > with unsigned, "> 0" is equivalent to != 0.
> > 
> > Ok, my question remains, how to I specify a signal that *must* be != 0?
> 
> nmigen doesn't have that, just use an unsigned Signal.

Ok, thanks, will do. Basically I am to ignore vhdl keywords for signals unless they determine signedness or cont-ness?
Comment 37 Cole Poirier 2020-08-12 18:05:41 BST
Fixed everything in https://bugs.libre-soc.org/show_bug.cgi?id=450#c31 except for starting unit tests and converting the types into record objects. Going to do my daily mediation, should have them for you in about half an hour.
Comment 38 Luke Kenneth Casson Leighton 2020-08-12 18:19:26 BST
(In reply to Cole Poirier from comment #34)
> (In reply to Luke Kenneth Casson Leighton from comment #33)
> > (In reply to Cole Poirier from comment #32)
> > with unsigned, "> 0" is equivalent to != 0.
> 
> Ok, my question remains, how to I specify a signal that *must* be != 0?

as jacob says: you don't.  it will be a formal proof's job to assert that it is never zero.
Comment 39 Cole Poirier 2020-08-12 18:56:04 BST
(In reply to Luke Kenneth Casson Leighton from comment #38)
> (In reply to Cole Poirier from comment #34)
> > (In reply to Luke Kenneth Casson Leighton from comment #33)
> > > (In reply to Cole Poirier from comment #32)
> > > with unsigned, "> 0" is equivalent to != 0.
> > 
> > Ok, my question remains, how to I specify a signal that *must* be != 0?
> 
> as jacob says: you don't.  it will be a formal proof's job to assert that it
> is never zero.

Aha! That's exactly the answer I was searching for, thank you.
Comment 40 Cole Poirier 2020-08-12 18:58:03 BST
Finished adding RecordObject classes for the common.vhdl input types to mmu.

Moving on to translating tests from mmu_tb.vhdl.
Comment 41 Cole Poirier 2020-08-12 19:06:44 BST
(In reply to Cole Poirier from comment #40)
> Moving on to translating tests from mmu_tb.vhdl.

There is no mmu_tb.vhdl, however, there is microwatt/tests/mmu/mmu.c which has tests similar to the ones we have in mul/test/test_pipe_caller. I'm going to try and set up mul and div-like tests for mmu.py. To keep that organized I will make a folder mmu under soc/src/experiment/.
Comment 42 Luke Kenneth Casson Leighton 2020-08-12 19:32:58 BST
(In reply to Cole Poirier from comment #41)
> (In reply to Cole Poirier from comment #40)
> > Moving on to translating tests from mmu_tb.vhdl.
> 
> There is no mmu_tb.vhdl, 

argh ok.  well there is dcache_tb.vhdl which is a bit 

> however, there is microwatt/tests/mmu/mmu.c which
> has tests similar to the ones we have in mul/test/test_pipe_caller. 

these are c programs that need to be compiled then executed (test_issuer.py)
and/or executed under litex-sim.

they therefore rely on having _everything_ in place.

> I'm
> going to try and set up mul and div-like tests for mmu.py. To keep that
> organized I will make a folder mmu under soc/src/experiment/.

no, i'd keep it much more basic than that.  similar to the regfile unit tests.
Comment 43 Cole Poirier 2020-08-12 19:58:27 BST
(In reply to Luke Kenneth Casson Leighton from comment #42)
> (In reply to Cole Poirier from comment #41)
> > (In reply to Cole Poirier from comment #40)
> > > Moving on to translating tests from mmu_tb.vhdl.
> > 
> > There is no mmu_tb.vhdl, 
> 
> argh ok.  well there is dcache_tb.vhdl which is a bit 

I'll need to first translate dcache.vhdl in order to use that right?

> > however, there is microwatt/tests/mmu/mmu.c which
> > has tests similar to the ones we have in mul/test/test_pipe_caller. 
> 
> these are c programs that need to be compiled then executed (test_issuer.py)
> and/or executed under litex-sim.
> 
> they therefore rely on having _everything_ in place.

Ah, I see, understood.

> > I'm
> > going to try and set up mul and div-like tests for mmu.py. To keep that
> > organized I will make a folder mmu under soc/src/experiment/.
> 
> no, i'd keep it much more basic than that.  similar to the regfile unit
> tests.

Gotcha, done. I've pushed a commit with the sim and test functions from regfile/regfile.py renamed to mmu, but I think I will need your help in beginning to write the sim function. Does that make sense? or am I confused?
Comment 44 Cole Poirier 2020-08-14 06:11:04 BST
Am I allowed to use typing from the python standard library? With them I can see how to translate dcache.vhdl... without them, I may need some guidance here.
Comment 45 Luke Kenneth Casson Leighton 2020-08-14 10:04:23 BST
(In reply to Cole Poirier from comment #44)
> Am I allowed to use typing from the python standard library? With them I can
> see how to translate dcache.vhdl... without them, I may need some guidance
> here.

if they're in the .py file, i can't stand it.  the increased unnecessary
verbiage is so frustrating and irrelevant i can't tolerate it.  not only
do line lengths increase, they increase so far that function declarations
which would otherwise easily fit on a single compact line have to end up
as massive multi-line declarations, pretty much every time.

for no good reason.

if you absolutely must use them put them in a separate .pyi file and make
sure to take responsibility for keeping them up-to-date (because i won't).
Comment 46 Cole Poirier 2020-08-14 18:15:33 BST
(In reply to Luke Kenneth Casson Leighton from comment #45)
> (In reply to Cole Poirier from comment #44)
> > Am I allowed to use typing from the python standard library? With them I can
> > see how to translate dcache.vhdl... without them, I may need some guidance
> > here.
> 
> if they're in the .py file, i can't stand it.  the increased unnecessary
> verbiage is so frustrating and irrelevant i can't tolerate it.  not only
> do line lengths increase, they increase so far that function declarations
> which would otherwise easily fit on a single compact line have to end up
> as massive multi-line declarations, pretty much every time.
> 
> for no good reason.
> 
> if you absolutely must use them put them in a separate .pyi file and make
> sure to take responsibility for keeping them up-to-date (because i won't).

Ok types aren't actually going to help with my problem, what I need is a way to express constraints on types, like the ranges of 'subtyped integers'.

For example from what I'm struggling to translate in dcache.vhdl:
```
# Number of ways
NUM_WAYS     =  4

subtype way_t is integer range 0 to NUM_WAYS-1;

subtype cache_tag_t is std_logic_vector(TAG_BITS-1 downto 0);

type cache_tags_set_t is array(way_t) of cache_tag_t;
type cache_tags_array_t is array(index_t) of cache_tags_set_t;
```

How should I translate `type cache_tags_set_t is array(way_t) of cache_tag_t;` into nmigen?
Comment 47 Luke Kenneth Casson Leighton 2020-08-14 18:33:43 BST
(In reply to Cole Poirier from comment #46)

> For example from what I'm struggling to translate in dcache.vhdl:
> ```
> # Number of ways
> NUM_WAYS     =  4

that's a parameter to the class (i'm assuming you've called it Dcache
   def __init__(self, NUM_WAYS=4)
      self.NUM_WAYS=4

> How should I translate `type cache_tags_set_t is array(way_t) of
> cache_tag_t;` into nmigen?

you don't.  manually substitute it.  it's a "type".

whenever you see "cache_tags_set_t", replace it with Array(....)
Comment 48 Cole Poirier 2020-08-14 18:56:18 BST
(In reply to Luke Kenneth Casson Leighton from comment #47)
> (In reply to Cole Poirier from comment #46)
> 
> > For example from what I'm struggling to translate in dcache.vhdl:
> > ```
> > # Number of ways
> > NUM_WAYS     =  4
> 
> that's a parameter to the class (i'm assuming you've called it Dcache
>    def __init__(self, NUM_WAYS=4)
>       self.NUM_WAYS=4
> 
> > How should I translate `type cache_tags_set_t is array(way_t) of
> > cache_tag_t;` into nmigen?
> 
> you don't.  manually substitute it.  it's a "type".
> 
> whenever you see "cache_tags_set_t", replace it with Array(....)

Sorry that's not that helpful, can you expand on this. As of right now what I'm interpreting is that any type that contains the keyword array() in vhdl should be translated into an empty array? Well actually that's not what I'm understanding, I'm unable to reach any useful conclusions from what you've instructed here.
Comment 49 Luke Kenneth Casson Leighton 2020-08-14 19:23:47 BST
(In reply to Cole Poirier from comment #48)
> (In reply to Luke Kenneth Casson Leighton from comment #47)
> > (In reply to Cole Poirier from comment #46)
> > 
> > > For example from what I'm struggling to translate in dcache.vhdl:
> > > ```
> > > # Number of ways
> > > NUM_WAYS     =  4
> > 
> > that's a parameter to the class (i'm assuming you've called it Dcache
> >    def __init__(self, NUM_WAYS=4)
> >       self.NUM_WAYS=4
> > 
> > > How should I translate `type cache_tags_set_t is array(way_t) of
> > > cache_tag_t;` into nmigen?
> > 
> > you don't.  manually substitute it.  it's a "type".
> > 
> > whenever you see "cache_tags_set_t", replace it with Array(....)
> 
> Sorry that's not that helpful, can you expand on this.

apologies, i assumed you'd know to expand "..." in the way that all typedefs (c and other languages) work.

wherever you see a subtype, LITERALLY replace the subtype with its declaration.


subtype way_t is integer range 0 to NUM_WAYS-1;

subtype cache_tag_t is std_logic_vector(TAG_BITS-1 downto 0);

type cache_tags_set_t is array(way_t) of cache_tag_t;
type cache_tags_array_t is array(index_t) of cache_tags_set_t;


becomes

 

subtype cache_tag_t is std_logic_vector(TAG_BITS-1 downto 0);

type cache_tags_set_t is array(range 0 to NUM_WAYS-1;) of cache_tag_t;
type cache_tags_array_t is array(index_t) of cache_tags_set_t;



becomes


type cache_tags_set_t is array(range 0 to NUM_WAYS-1;) of std_logic_vector(TAG_BITS-1 downto 0);

type cache_tags_array_t is array(index_t) of cache_tags_set_t;


becomes



type cache_tags_array_t is array(index_t) of 
array(range 0 to NUM_WAYS-1;) of std_logic_vector(TAG_BITS-1 downto 0);



and the substitution is now complete as far as you have given.

of course, it is itself also a type, therefore there will be an additional substitution.

translating that to nmigen, will invve a 2D Array of Signals.
Comment 50 Cole Poirier 2020-08-14 20:37:46 BST
(In reply to Luke Kenneth Casson Leighton from comment #49)
> (In reply to Cole Poirier from comment #48)

> > Sorry that's not that helpful, can you expand on this.
> 
> apologies, i assumed you'd know to expand "..." in the way that all typedefs
> (c and other languages) work.

Aha! Thanks very much for taking the time to explain this to me, I’m learning a lot very quickly, but I am still very ignorant.

> wherever you see a subtype, LITERALLY replace the subtype with its
> declaration.

Ok, that makes sense.

> subtype way_t is integer range 0 to NUM_WAYS-1;
> 
> subtype cache_tag_t is std_logic_vector(TAG_BITS-1 downto 0);
> 
> type cache_tags_set_t is array(way_t) of cache_tag_t;
> type cache_tags_array_t is array(index_t) of cache_tags_set_t;
> 
> 
> becomes
> 
>  
> 
> subtype cache_tag_t is std_logic_vector(TAG_BITS-1 downto 0);
> 
> type cache_tags_set_t is array(range 0 to NUM_WAYS-1;) of cache_tag_t;
> type cache_tags_array_t is array(index_t) of cache_tags_set_t;
> 
> 
> 
> becomes
> 
> 
> type cache_tags_set_t is array(range 0 to NUM_WAYS-1;) of
> std_logic_vector(TAG_BITS-1 downto 0);
> 
> type cache_tags_array_t is array(index_t) of cache_tags_set_t;
> 
> 
> becomes
> 
> 
> 
> type cache_tags_array_t is array(index_t) of 
> array(range 0 to NUM_WAYS-1;) of std_logic_vector(TAG_BITS-1 downto 0);
> 
> 
> 
> and the substitution is now complete as far as you have given.
> 
> of course, it is itself also a type, therefore there will be an additional
> substitution.

That’s very helpful, thank you.


> translating that to nmigen, will invve a 2D Array of Signals.

The above us all helpful but I think this is the key insight. AFK right now but will try this shortly and get back to you with a more useful question. Thanks Luke :)
Comment 51 Luke Kenneth Casson Leighton 2020-08-15 04:17:43 BST
 942         comb += prtable_addr.eq(
 943                  Cat(
 944                   Cat(
 945                    Cat(
 946                     Cat(Const(0b0000, 4), effpid[0:8]),
 947                     (
 948                      (r.prtble[12:36] & ~finalmask[0:24])
 949                      | effpid[8:32]   &  finalmask[0:24]
 950                     )
 951                    ),
 952                    r.prtbl[36:56]
 953                   ),
 954                   Const(0x00, 8)
 955                  )
 956                 )


no need for Cat(Cat(Cat(...)))

just make it Cat(x,y,z,w,a,b)


also lose any call to as_unsigned() because the result of Cat() is always unsigned.  no need to convert unsigned to unsigned.

also: wherever there is an eq into a full Signal (*not* a slice), and the last Cat is zeros, then because the default for an assignment *is* zeros if nothing is assigned, the last zeros can be REMOVED.

if that results in only one item in Cat, then Cat can go, too.

this will allow single lines in a huge number of places.

835   comb += mbits.eq(data[0:5])
Comment 52 Luke Kenneth Casson Leighton 2020-08-15 04:21:45 BST
 958 #       pgtable_addr := x"00" & r.pgbase(55 downto 19) &
 959 #                       ((r.pgbase(18 downto 3) and not mask) or
 960 #                       (addrsh and mask)) & "000";
 961         comb += pgtable_addr.eq(
 962                  Cat(
 963                   Cat(
 964                    Const(0b000, 3),
 965                    (
 966                     (r.pgbase[3:19] & ~mask)
 967                     | (addrsh       &  mask)
 968                    )
 969                   ),
 970                   Const(0x00, 8)
 971                  )
 972                 )

pgbase[19:56] is missing around line 968
Comment 53 Luke Kenneth Casson Leighton 2020-08-15 04:29:10 BST
 997         with m.If(tlbie_req):
 998 #           addr := r.addr;
 999 #           tlb_data := (others => '0');
1000             comb += addr.eq(r.addr)

comb += tlb_data.eq(0) is missing.

ok, it is and it isn't.  the default is the reset value, which in this case is zero.  however check all prior lines: if tlb_data has previously been assigned to, then it is critically important not to skip the assign to zero.


1004 #           tlb_data := pte;

this however is definitely missing.
Comment 54 Luke Kenneth Casson Leighton 2020-08-15 04:31:19 BST
 505                     comb += v.store.eq(~(l_in.load | l_in.siside))

spelling error.
Comment 55 Luke Kenneth Casson Leighton 2020-08-15 04:38:32 BST
 719 #               mbits := '0' & r.mask_size;
 720 #               v.shift := r.shift + (31 - 12) - mbits;
 721 #               nonzero := or(r.addr(61 downto 31) and
 722 #                           not finalmask(30 downto 0));

you can't assign to nonexistent variables.

mbits = Signal(r.mask_size.width+1)
nonzero = Signal()

put those lines just before they are used.

do not add "self." in front of them.  they are local not members of the class.
Comment 56 Luke Kenneth Casson Leighton 2020-08-15 04:39:47 BST
again do each of these individually with a single commit, full link to the comment, and do not skip "git diff".
Comment 57 Cole Poirier 2020-08-16 00:30:54 BST
(In reply to Luke Kenneth Casson Leighton from comment #55)
>  719 #               mbits := '0' & r.mask_size;
>  720 #               v.shift := r.shift + (31 - 12) - mbits;
>  721 #               nonzero := or(r.addr(61 downto 31) and
>  722 #                           not finalmask(30 downto 0));
> 
> you can't assign to nonexistent variables.
> 
> mbits = Signal(r.mask_size.width+1)
> nonzero = Signal()
> 
> put those lines just before they are used.
> 
> do not add "self." in front of them.  they are local not members of the
> class.

They actually are members of the class, declared here:

```
348         self.mbits = Signal(6)
349         self.pgtable_addr = Signal(64)
350         self.pte = Signal(64)
351         self.tlb_data = Signal(64)
352         self.nonzero = Signal()

385         mbits = self.mbits
386         pgtable_addr = self.pgtable_addr
387         pte = self.pte
388         tlb_data = self.tlb_data
389         nonzero = self.nonzero
```

Should I still make the fix you've suggested in this comment?
Comment 58 Cole Poirier 2020-08-16 00:54:23 BST
Done all the other comments/fixes :)
Comment 59 Jacob Lifshay 2020-08-16 07:53:17 BST
(In reply to Luke Kenneth Casson Leighton from comment #51)
> also: wherever there is an eq into a full Signal (*not* a slice), and the
> last Cat is zeros, then because the default for an assignment *is* zeros if
> nothing is assigned, the last zeros can be REMOVED.
> 
> if that results in only one item in Cat, then Cat can go, too.

in that case the Cat will need to be converted to .as_unsigned() instead of just being removed if the input Signal is signed -- don't want to accidentally change the semantics :)
Comment 60 Luke Kenneth Casson Leighton 2020-08-16 09:56:24 BST
(In reply to Cole Poirier from comment #57)

> > do not add "self." in front of them.  they are local not members of the
> > class.
> 
> They actually are members of the class, declared here:

none of them should have been made members of the class, because they're
all local variables, used locally.

the only reason to make variables members of a class is to modify them
externally, as part of the public and external API.
Comment 61 Luke Kenneth Casson Leighton 2020-08-16 10:02:06 BST
commit d1b3e59ded7dc54da9cbccd425b52a062cdc94f6 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Aug 16 10:01:51 2020 +0100

    begin tidyup, removing comments after line-by-line review, remove MMU1, self.
Comment 62 Luke Kenneth Casson Leighton 2020-08-16 10:05:42 BST
commit 415ccff73534d45c74efa1e5f9642bccb56f865a (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Aug 16 10:05:30 2020 +0100

    fix batch of syntax errors found by running mmu.py
Comment 63 Luke Kenneth Casson Leighton 2020-08-16 10:16:02 BST
(In reply to Jacob Lifshay from comment #59)

> > if that results in only one item in Cat, then Cat can go, too.
> 
> in that case the Cat will need to be converted to .as_unsigned() instead of
> just being removed if the input Signal is signed -- don't want to
> accidentally change the semantics :)

yes :)  luckily, signed is never used in any of the mmu code.
Comment 64 Luke Kenneth Casson Leighton 2020-08-16 10:21:19 BST
(In reply to Cole Poirier from comment #58)
> Done all the other comments/fixes :)

#               v.pgbase := pgtbl(55 downto 8) & x"00";
                # set v.shift to rts so that we can use finalmask
                # for the segment check
                comb += v.shift.eq(rts)
                comb += v.mask_size.eq(mbits[0:5])
                comb += v.pgbase.eq(pgtbl[8:56])

note that i said, "at the *end*".

> also: wherever there is an eq into a full Signal (*not* a slice), AND THE
> LAST Cat IS ZEROS

if they are at the start, they are LSBs.  therefore, by removing them, the
assignment has shifted in this case by 8 bits.

commit 9af0f3f969658fe7b9903a134e0bfe52ddcb8631 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Sun Aug 16 10:21:09 2020 +0100

    restore incorrect removal of zero-Cat at LHS (should never do that)
Comment 65 Luke Kenneth Casson Leighton 2020-08-16 10:31:33 BST
#               elsif mbits < 5 or mbits > 16 or mbits
#                       > (r.shift + (31 - 12)) then
#                   v.state := RADIX_FINISH;
#                   v.badtree := '1';
                with m.Elif((mbits < 5) | (mbits > 16)
                          | (mbits > (r.shift + (31-12)))):

Elif, not If.
Comment 66 Luke Kenneth Casson Leighton 2020-08-16 10:36:03 BST
                                v.state := RADIX_ERROR;
                                v.perm_err := not perm_ok;
                                -- permission error takes precedence over RC error
                                v.rc_error := perm_ok;



                                comb += vl.state.eq(State.RADIX_FINISH)
                                comb += v.perm_err.eq(~perm_ok)
                                # permission error takes precedence
                                # over RC error
                                comb += v.rc_error.eq(perm_ok)


cut/paste error.
Comment 67 Luke Kenneth Casson Leighton 2020-08-16 10:39:34 BST
        with m.If((v.state == State.RADIX_FINISH)
                  | ((v.state == State.RADIX_LOAD_TLB) & r.iside)):

sigh this is really obscure.  & is a bit-operator.  it actually takes
precedence over "==".  therefore you're forced to put brackets round
any "==" or ">" or "<=" etc. tests
Comment 68 Luke Kenneth Casson Leighton 2020-08-16 10:45:32 BST
            comb += addr.eq(Cat(C(0, 12), r.addr[12:64]))
            comb += tlb_data.eq(pte)
#       elsif prtbl_rd = '1' then
        with m.If(prtbl_rd):

more LHS Cat removal, more with m.Elif.
Comment 69 Cole Poirier 2020-08-16 19:03:01 BST
Thanks for all the corrections Luke! I pulled the latest changes and looked for yours in the file and it appears you've done all the changes, just want to confirm, there are no changes for me to make right?

Now I should be working on the sim and unit testing? I need some guidance as to how I should go about starting to write the sim.
Comment 70 Luke Kenneth Casson Leighton 2020-08-17 11:40:52 BST
(In reply to Cole Poirier from comment #69)
> Thanks for all the corrections Luke! I pulled the latest changes and looked
> for yours in the file and it appears you've done all the changes, 

go over them and make sure you understand.  also review.

> Now I should be working on the sim and unit testing? I need some guidance as
> to how I should go about starting to write the sim.

i have no idea.  just do it.  set inputs, print outputs, see what happens.

the thing is that realistically it is only when it is integrated with loadstore (and dcache) that we get "meaningful" answers.

until then we just have to do... something.
Comment 71 Cole Poirier 2020-08-17 17:54:33 BST
(In reply to Luke Kenneth Casson Leighton from comment #70)
> (In reply to Cole Poirier from comment #69)
> > Thanks for all the corrections Luke! I pulled the latest changes and looked
> > for yours in the file and it appears you've done all the changes, 
> 
> go over them and make sure you understand.  also review.

I did, very clean, gives me great insight into the translation of dcache.vhdl which I am early in the process of converting.

> > Now I should be working on the sim and unit testing? I need some guidance as
> > to how I should go about starting to write the sim.
> 
> i have no idea.  just do it.  set inputs, print outputs, see what happens.
> 
> the thing is that realistically it is only when it is integrated with
> loadstore (and dcache) that we get "meaningful" answers.
> 
> until then we just have to do... something.

Hmm... My energies are probably best spend converting dcache then... We already have loadstore in LoadStoreCompUnit correct?
Comment 72 Luke Kenneth Casson Leighton 2020-08-17 18:40:26 BST
(In reply to Cole Poirier from comment #71)

> > until then we just have to do... something.
> 
> Hmm... My energies are probably best spend converting dcache then... 

for now yes, however we will need to come back and do one.

> We already have loadstore in LoadStoreCompUnit correct?

yes however dcache will need "conversion" (*not now*) to the LoadStoreInterface.
or, we work it out - but *after* conversion of dcache.vhdl.
Comment 73 Cole Poirier 2020-08-17 19:02:36 BST
(In reply to Luke Kenneth Casson Leighton from comment #72)
> (In reply to Cole Poirier from comment #71)
> 
> > > until then we just have to do... something.
> > 
> > Hmm... My energies are probably best spend converting dcache then... 
> 
> for now yes, however we will need to come back and do one.

Definitely, will probably be easier when it's all converted so the interfaces/connections can be tested.

> > We already have loadstore in LoadStoreCompUnit correct?
> 
> yes however dcache will need "conversion" (*not now*) to the
> LoadStoreInterface.
> or, we work it out - but *after* conversion of dcache.vhdl.

Sounds good! What else needs to be converted form microwatt for dcache, mmu, and LoadStoreCompUnit to function properly? I'm assuming icache.vhdl, plru.vhdl, and...?
Comment 74 Luke Kenneth Casson Leighton 2020-08-17 19:15:34 BST
(In reply to Cole Poirier from comment #73)

> Sounds good! What else needs to be converted form microwatt for dcache, mmu,
> and LoadStoreCompUnit to function properly? I'm assuming icache.vhdl,
> plru.vhdl, and...?

it'll show up by way of the data structure links.  records used in one "thing" and connected to other "things".

l.
Comment 75 Luke Kenneth Casson Leighton 2020-09-15 00:32:36 BST
preliminary tests show mmu.py to be functional, including the link to dcache.py
Comment 76 Luke Kenneth Casson Leighton 2021-04-30 20:21:17 BST
now linked to the MMU Function Unit FSM, a new class LoadStore1 has been set up as a PortInterface, it can be established through ConfigMemoryPortInterface.

currently there are nmigen driver conflicts that need tracking down.
Comment 77 Luke Kenneth Casson Leighton 2021-05-02 23:28:46 BST
diff --git a/src/soc/fu/mmu/fsm.py b/src/soc/fu/mmu/fsm.py
index b7ee3d57..8b660b02 100644
--- a/src/soc/fu/mmu/fsm.py
+++ b/src/soc/fu/mmu/fsm.py
@@ -62,6 +62,7 @@ class LoadStore1(PortInterfaceBase):
         m.d.comb += self.d_in.load.eq(0)
         m.d.comb += self.d_in.byte_sel.eq(mask)
         m.d.comb += self.d_in.addr.eq(addr)
+        m.d.comb += self.d_in.nc.eq(1)
         return None
 
     def set_rd_addr(self, m, addr, mask):
@@ -80,6 +81,7 @@ class LoadStore1(PortInterfaceBase):
         # this is for peripherals.  same thing done in Microwatt loadstore1.vhdl
         with m.If(addr[28:] == Const(0xc, 4)):
             m.d.comb += self.d_in.nc.eq(1)
+        m.d.comb += self.d_in.nc.eq(1)
         return None #FIXME return value
 
     def set_wr_data(self, m, data, wen):


when switching off cacheing, litexbios completes its checksum correctly.
this tells us that there's an issue associated with reading a sequential
block of 64k of RAM.

investigating the first few LOADs shows no issue at all.  current dcache
unit tests already go up to 4k words (and pass): clearly this isn't enough.
Comment 78 Luke Kenneth Casson Leighton 2021-05-03 12:51:49 BST
Created attachment 133 [details]
add crc32 debug statements

this is what we call close to a "worst case scenario" bug, in hardware: it's
in a *range* of memory addresses, but the CRC32 being computed is just a
*symptom*.

it could be the case that, actually, the memory location being corrupted
was written to much earlier than detected by the CRC.
Comment 79 Luke Kenneth Casson Leighton 2021-05-03 13:19:01 BST
iiinteresting.  the area affected (modified) is the area that is being
printed out by debug printfs

00006ba0  00 00 00 00 00 00 00 00  20 42 49 4f 53 20 62 75  |........ BIOS bu|
00006bb0  69 6c 74 20 6f 6e 20 4d  61 79 20 20 33 20 32 30  |ilt on May  3 20|
00006bc0  32 31 20 31 33 3a 31 33  3a 34 30 0a 00 00 00 00  |21 13:13:40.....|
00006bd0  20 4d 69 67 65 6e 20 67  69 74 20 73 68 61 31 3a  | Migen git sha1:|
00006be0  20 37 62 63 34 65 62 31  0a 00 00 00 00 00 00 00  | 7bc4eb1........|
00006bf0  20 4c 69 74 65 58 20 67  69 74 20 73 68 61 31 3a  | LiteX git sha1:|
00006c00  20 33 35 39 32 39 63 30  66 0a 00 00 00 00 00 00  | 35929c0f.......|
00006c10  2d 2d 3d 3d 3d 3d 3d 3d  3d 3d 3d 3d 3d 3d 3d 3d  |--==============|

this is a hexdump of the bios.bin.  printf statements when the bios is running
will print that out, by reading it, one character at a time.

i suspect what is happening is that during that read process, which is followed
by a write to the UART, something happens which results in the read turning into
a write, and the end result is data corruption.
Comment 80 Luke Kenneth Casson Leighton 2021-05-03 14:55:10 BST
right.  i think i have a clue as to what's going on:
set_wr_addr and set_wr_data may be getting called out of
sync

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/experiment/pimem.py;h=1a66b914b5885d29a676bcd6ddbf67b4363bbc2f;hb=bf868f4961705d62229fe40b3bb04368894ca722#l274

i will have to think how to deal with this
Comment 81 Luke Kenneth Casson Leighton 2021-05-03 16:28:36 BST
commit 43f240c1942c9f9b88c77d05fbc2d1591900d11f (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Mon May 3 16:28:14 2021 +0100

    MMU: get store to activate only when data is available, and to wait till done


exccellent, got it working.  horribly slow, but hey.
Comment 82 Jacob Lifshay 2021-05-03 20:35:22 BST
(In reply to Luke Kenneth Casson Leighton from comment #81)
> commit 43f240c1942c9f9b88c77d05fbc2d1591900d11f (HEAD -> master)
> Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
> Date:   Mon May 3 16:28:14 2021 +0100
> 
>     MMU: get store to activate only when data is available, and to wait till
> done
> 
> 
> exccellent, got it working.  horribly slow, but hey.

Yay! slow is always better than broken