[libre-riscv-dev] pipeline sync issues

[Jacob Lifshay]

On Tue, Apr 9, 2019, 23:00 Luke Kenneth Casson Leighton <lkcl at lkcl.net>
wrote:

> On Wed, Apr 10, 2019 at 5:45 AM Jacob Lifshay <programmerjake at gmail.com>
> wrote:
>  that sounds like the industry-standard write-enable semantics :
> p_o_ready & p_i_valid both need to be asserted for input to be
> ready-and-valid.
>
good.

>
>  it also doesn't explain why UnbufferedPipeline (et al) *ignore*
> p_o_ready entirely.  it's really important information as it
> determines (or more like "declares") when the data is to *be* valid.
>
>  the point being: the truth table for UnbufferedPipeline has a case
> where p_o_ready was *not* set, p_i_valid *is* set, and consequently
> *invalid* data is sent on through the system.

p_o_data should be an output set by the current stage. if p_o_data is not
set then the state of p_i_valid is a don't-care and no transfer logically
occurs.

 look again closely at the truth tables, side-by-side.
> UnbufferedPipeline *ignores* the case where p_o_ready is LOW and
> p_i_valid is HIGH.
>
>  that's i believe the source of the problem.
>
> so i agree that the semantics (the way the paragraphs have been
> stated) are correct, as they sound precisely like the
> industry-standard write-enable semantics deployed by wishbone and
> AXI4...
>
> ... it's just that the implementation is wrong and doesn't conform with it.
>
> > which is different from the semantics I was using
> > in that, in the code you wrote, ready has to be asserted earlier than in
> > the code I wrote.
>
>  that's down to the fact that the previous stage has to be told in the
> *previous* cycle that it is free and clear to send the data.
>
>  the contract from the sender's perspective:
>
>  * data and n_o_valid are sent *together* (to arrive on a future
> cycle) and are only sent when n_i_ready is valid (in THIS cycle)
>
The way I had designed it was that n_o_valid is set and data is valid
whenever the current stage has data that's ready to be sent, irrespective
of if the next stage is ready yet, because it reduces the amount of logic
necessary, though either way works.

>
> the contract from the recipient's perspective: bear in mind that
>
> (1) n_i_ready of the sender was set by p_o_ready from the *recipient* and
> (2) what was the "future" cycle as far as the recipient is concerned
> is now the PRESENT cycle of the sender, and
> (3) what was the "present" cycle of the sender is the PAST cycle of
> the recipient
>
I like to think of it from the perspective of the pipeline stages
themselves rather than the perspective of the data flowing through, that
way you don't have to keep track of any past, present, or future other than
will the data be transferred at the next clock edge or not and making sure
the data isn't deleted or duplicated by accident.

>
>  * because p_o_ready has to be set in what was the PAST, its present
> state MUST be utilised to determine if the incoming data is valid, IN
> COMBINATION with the fact that data-PLUS-valid are both synchronously
> set.
>
> this is why we see (p_o_ready & p_i_valid) being used to determine if
> the incoming data is to be placed into a register.
>
> and if those two are not tested together, it *will* result in invalid
> data being accepted.
>
yeah. keep in mind that since p_o_ready is an output that testing p_o_ready
& p_i_valid may not have that exact expression in it.

One possible way to fix the truth table is to write in each row how many
data elements are stored in the stage and how many will be in the stage the
next clock cycle and make sure that that matches. This is similar to the
"transfer" column in
https://salsa.debian.org/Kazan-team/simple-barrel-processor/blob/master/doc/BreakReadyChainStage.rst

Once the truth table is fixed then you just have to change the nmigen code
to match.

>
> note: if we don't conform to the industry-standard practices for
> ready/valid, we will not be able to use SyncFIFO in our designs.
>
I haven't checked the api, but building adaptors should be quite simple.

Jacob