[libre-riscv-dev] [Bug 64] data handling / io control / data routing API needed

[bugzilla-daemon at libre-riscv.org]

http://bugs.libre-riscv.org/show_bug.cgi?id=64

--- Comment #50 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Jacob Lifshay from comment #49)

> to clarify, I had meant having the enq and deq signals grouped into separate
> port objects, similar to how entry/exit signals are grouped in Block. I
> totally get that the signals are identical, I was just objecting to how they
> had been changed to be ungrouped.

they effectively still are, it's perhaps not clear/obvious.
i'm restoring the original code (managed to work out how to do it):

        ## prev: make the FIFO (Queue object) "look" like a PrevControl...
        m.submodules.fp = fp = PrevControl()
        fp.valid_i, fp._ready_o, fp.data_i = fifo.we, fifo.writable, fifo.din
        m.d.comb += fp._connect_in(self.p, fn=processfn)

        # next: make the FIFO (Queue object) "look" like a NextControl...
        m.submodules.fn = fn = NextControl()
        fn.valid_o, fn.ready_i, fn.data_o  = fifo.readable, fifo.re, fifo.dout
        connections = fn._connect_out(self.n, fn=nmoperator.cat)

so, in Block:

* EntryPort is the "grouping" that is absolutely identical in purpose and
  function to PrevControl.

  EntryPort has grouping of ready/valid/data

  PrevControl has grouping of ready/valid/data

* ExitPort is the "grouping" that is absolutely identical to NextControl.

  ditto

so, i do not believe you could be referring to that, as the grouping is
absolutely identical.

(yes i admit that it is a bit weird that PrevControl and NextControl are
 *not allowed* to set data_i and data_o in their constructor, that job
  is the responsibility of *ControlBase* - through the function now known
  as ControlBase.set_data, however that's fully and carefully documented)


in Queue, it's not clear that grouping exists explicitly
by way of separate classes/objects.  however, by deriving from a known
interface (FIFOInterface), the grouping becomes clear from looking
at the code:

class FIFOInterface:

        self.din      = Signal(width, reset_less=True)
        self.writable = Signal() # not full
        self.we       = Signal()

        self.dout     = Signal(width, reset_less=True)
        self.readable = Signal() # not empty
        self.re       = Signal()

from there, in Queue, the "convenience names" preserve the groupings:

        # convenience names
        p_ready_o = self.writable
        p_valid_i = self.we
        enq_data = self.din

        n_valid_o = self.readable
        n_ready_i = self.re
        deq_data = self.dout

and from there, the above changes (which explicitly drop those named-vars
into PrevControl and NextControl objects) make it, i believe, clear.

that in turn has the side-effect that:

* Queue is made to *look* like a PrevControl / NextControl pair

* Queue is used *inside* FIFOControl

* FIFOControl derives from ControlBase

* ControlBase is conceptually equivalent to Block

* Therefore, logically, Queue *is* directly equivalent to Block,
  only with the slight awkwardness that its members are separated.

now, we _could_ go one stage further and actually make Queue actually
*have* PrevControl and NextControl instances... however, again, the
"dependence" of the Queue class on the iocontrol.py module is not...
i'm not totally comfortable with the idea, as explained above.

point being: we could conceivably morph Queue so that it *becomes*
Block-like... however that's precisely what FIFOControl is supposed
to do... and does do.

more to the point: by morphing Queue to look absolutely nothing like
the nmigen FIFOInterface, we lose several opportunities, such as
being able to talk to the nmigen developers and say "hey, you know
those bugs and limitations of SyncFIFO / SyncFIFOBuffered...."

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