[Libre-soc-bugs] [Bug 469] Create D-cache from microwatt dcache.vhdl

[bugzilla-daemon at libre-soc.org]

https://bugs.libre-soc.org/show_bug.cgi?id=469

--- Comment #24 from Cole Poirier <colepoirier at gmail.com> ---
Thank you for all the corrections luke *very* helpful! I actually saw all of
these last week but wasn't at a computer and able to respond here on bugzilla.
Will be reply 'noted.' to each comment that I had seen last week and thus tried
to adhere to for the.. I'd say second half of icache.py's translation from
icache.vhdl. I plan on going over all of mmu.py, dcache.py, and icache.py with
my new understanding after finishing the translation of the last two processes
from icache.vhdl.

(In reply to Luke Kenneth Casson Leighton from comment #15)
> cole, only comb += defaults to equal-to-the-reset.
> 
> for sync the value is stored in a latch and so if it is not set it will
> *remain* at the (incorrect) value.
> 
> 
>     #                 r.req := d_in;
>     #                 r.tlbie := '0';
>     #                 r.doall := '0';
>     #                 r.tlbld := '0';
>     #                 r.mmu_req := '0';
>                 sync += r.req.eq(d_in)
>                 sync += r.req.tlbie.eq(0)
>                 sync += r.req.doall.eq(0)
>                 sync += r.req.tlbd.eq(0)
>                 sync += r.req.mmu_req.eq(0)

Noted.

(In reply to Luke Kenneth Casson Leighton from comment #16)
>         index    = Signal(log2_int(TLB_SET_BITS), False)
>         addrbits = Signal(TLB_SET_BITS)
> 
>         amin = TLB_LG_PGSZ
>         amax = TLB_LG_PGSZ + TLB_SET_BITS
> 
>         with m.If(m_in.valid):
>             comb += addrbits.eq(m_in.addr[amin : amax])
>         with m.Else():
>             comb += addrbits.eq(d_in.addr[amin : amax])
>         comb += index.eq(addrbits)
> 
>         #             if r0_stall = '0' then
>         # If we have any op and the previous op isn't finished,
>         # then keep the same output for next cycle.
>         with m.If(~r0_stall):
>             sync += tlb_valid_way.eq(dtlb_valid_bits[index])
>             sync += tlb_tag_way.eq(dtlb_tags[index])
>             sync += tlb_pte_way.eq(dtlb_ptes[index])
> 
> previously, you had sync += addrbits and sync += index.
> 
> what this would do is:
> 
> * on the first clock cycle, m_in.valid would be tested
> * on the second cycle, a value of addrbits would be valid
> * on the third cycle, the value of addrbits would become valid in index
> * on the fourth cycle, the test of r0_stall would finally result in
>   tlb_*_way being updated...
> 
> ... with values that were far, far too late.
> 
> if you look at the original dcache.vhdl you will see that index and addrbits
> were "variables", not signals.  this is a sign that needs to be looked out
> for.

Hmmm. Very interesting! This is probably going to be the most difficult one for
me to do properly. This was within an "if rising_edge(clk) then" statement
correct? Is it the case that only signals are used with sync and variables are
used with comb within a sync block?

(In reply to Luke Kenneth Casson Leighton from comment #17)
> #                   (ROW_OFF_BITS-1 downto 0 => '0');
>             comb += ra.eq(Cat(
>                      Const(0, ROW_OFF_BITS),
> 
> you had a constant ROW_OFF_BITS of length ROW_OFF_BITS

(In reply to Luke Kenneth Casson Leighton from comment #18)
>     #         variable tagset : tlb_way_tags_t;
>     #         variable pteset : tlb_way_ptes_t;
>     #type tlb_tags_t is array(tlb_index_t) of tlb_way_tags_t;
>     # --> Array([Signal(log(way_tags length)) for i in range(number of
> tlbs)])
> 
> a function called TLBWayTags() could i suppose be defined to return that
> nmigen Array.

Noted. I do this for a lot if not all the types that are "is array(*)"
statements.

(In reply to Luke Kenneth Casson Leighton from comment #19)
> cole also needed is cache_ram.vhdl.

(In reply to Luke Kenneth Casson Leighton from comment #20)
>                 comb += wr_addr.eq(r1.store_row)
>                 comb += wr_sel.eq(~0) # all 1s
> 
> #                 if r1.state = RELOAD_WAIT_ACK and
> #                 wishbone_in.ack = '1' and replace_way = i then
>             with m.If((r1.state == State.RELOAD_WAIT_ACK)
>                       & wishbone_in.ack & (relpace_way == i)):
> 
> you need to make sure that any comparisons are in brackets when included
> with "&" or "|" bit-wise operators.
> 
> the reason is because unlike "and" and "or", "&/|" are HIGHER precedence
> than comparisons.
> 
> in python you would normally do "if x & mask == 0b110" and that would of
> course be taken to mean "bit-wise AND x together THEN compare against 0b110".
> 
> so it is critically important to do "if (x & mask) == 0b110" and it is
> ESPECIALLY important to do "value & (something == somethingelse)" otherwise
> it is interpreted as "(value & something) == somethingelse"

Very helpful. Noted. I will have to review the first half of icache.py as well
as mmu and dcache for this specifically later.

(In reply to Luke Kenneth Casson Leighton from comment #21)
>                 r1.forward_sel1 <= (others => '1');
> 
> that's not "just set the 1st bit to 1" it's "set *ALL* bits to 1".
> whitequark informs us that the best way to do this is:
> 
>             sync += r1.forward_sel1.eq(~0) # all 1s
> 
> *definitely* not
> 
>             sync += r1.forward_sel1.eq(1) # sets only the LSB to 1.

Ahhhh. *THAT'S* what (others => 0 *or* 1) meant! Thank you, makes so much sense
now.

(In reply to Luke Kenneth Casson Leighton from comment #22)
> #                     when OP_STORE_HIT | OP_STORE_MISS =>
>                         with m.Case(Op.OP_STORE_HIT, Op.OP_STORE_MISS):
> 
> cases are not ORed together, they are comma-listed.

Ah thank you, will note for my full review later today.

(In reply to Luke Kenneth Casson Leighton from comment #23)
> #               stbs_done := true;
>                                 sync += stbs_done.eq(0)
> 
> 
> error.  should be eq(1)

Thank you.

-- 
You are receiving this mail because:
You are on the CC list for the bug.