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;
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.
(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.
(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);
(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)
(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 :)
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
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))
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
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.
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()
^ 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])
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.
(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)
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));
(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=" &
(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.
(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=" &
(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?
(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?
(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 ```
(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?
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?
(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.
(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.
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()
(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.
(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?
(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)
(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 :)
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.
(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.
(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.
(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.
(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! ;-)
(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.
(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?
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.
(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.
(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.
Finished adding RecordObject classes for the common.vhdl input types to mmu. Moving on to translating tests from mmu_tb.vhdl.
(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/.
(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.
(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?
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.
(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).
(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?
(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(....)
(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.
(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.
(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 :)
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])
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
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.
505 comb += v.store.eq(~(l_in.load | l_in.siside)) spelling error.
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.
again do each of these individually with a single commit, full link to the comment, and do not skip "git diff".
(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?
Done all the other comments/fixes :)
(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 :)
(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.
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.
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
(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.
(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)
# 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.
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.
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
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.
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.
(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.
(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?
(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.
(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...?
(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.
preliminary tests show mmu.py to be functional, including the link to dcache.py
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.
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.
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.
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.
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
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.
(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