amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.58k stars 174 forks source link

Question: reset-less signals in interfaces #1220

Closed daniestevez closed 7 months ago

daniestevez commented 8 months ago

This is more of a question (or perhaps a feature request) than an issue.

I was reading the documentation of the wiring module in the context of https://github.com/amaranth-lang/rfcs/pull/61. I've seen that there isn't a straightforward way to specify if a signal is reset-less or not in a Component that uses a Signature. Also, the documentation about is_compliant() mentions the following:

for port members, is a value-like object casting to a Signal or a Const whose width and signedness is the same as that of the member, and (in case of a Signal) which is not reset-less and whose initial value is that of the member;

Usually, when implementing an AXI-Stream producer, I will make TDATA reset-less. This makes sense because the consumer isn't allowed to look at the value of TDATA after the producer has asserted TVALID some time after reset is deasserted (and when asserting TVALID, a good value has been put into TDATA). Doing this helps reduce the load on the reset net, which can be important in large designs.

For example, consider the following AXI-S producer (the example can be pasted in the playground). Here self.tdata is declared as reset-less on purpose, reducing the load on the reset net and not affecting the functionality. I wonder if/how this example can be re-written with a Component that uses a Signature for the AXI-S port. A workaround is to have an "internal" tdata = Signal(64, reset_less=True) created in elaborate() and then doing m.d.comb += self.tdata.eq(tdata), but being able to do this directly would save some boilerplate code.

from amaranth import *
from amaranth.sim import Simulator, Tick
from amaranth.back import rtlil, verilog
import amaranth_playground
import random

class AXISProducer(Elaboratable):
    def __init__(self):
        self.tdata = Signal(64, reset_less=True)
        self.tvalid = Signal()
        self.tready = Signal()

    def elaborate(self, platform):
        m = Module()
        counter_a = Signal(64)
        counter_b = Signal(64)
        with m.If(~self.tvalid):
            m.d.sync += [
                counter_a.eq(counter_a + 3),
                counter_b.eq(counter_b + 5),
                self.tdata.eq(counter_a ^ counter_b),
                self.tvalid.eq(1),
            ]
        with m.If(self.tvalid & self.tready):
            m.d.sync += self.tvalid.eq(0)
        return m

m = Module()
m.submodules.producer = producer = AXISProducer()

def testbench():
    for _ in range(100):
        yield Tick("sync")
        yield producer.tready.eq(random.randrange(2))

sim = Simulator(m)
sim.add_clock(1e-6)
sim.add_process(testbench)
with amaranth_playground.show_waveforms(sim):
    sim.run()

amaranth_playground.show_verilog(verilog.convert(m, ports=[producer.tdata, producer.tvalid, producer.tready]))
# amaranth_playground.show_rtlil(rtlil.convert(m, ports=[producer.tdata, producer.tvalid, producer.tready]))
whitequark commented 8 months ago

I understand the issue you're raising. I don't have a direct answer to it, however, I would like to explain the reasoning behind the choice of prohibiting connections to reset-less signals.

connect() tries very hard to prevent "invalid" connections from happening. This includes obvious things such as an output to output connection, but also less obvious things, such as connecting several inputs with different init values (e.g. in an output-to-many-inputs fan-out arrangement, which can be useful with streams specifically). While this isn't a problem when the inputs are all actively driven, connect() will also make a connection where only inputs are connected together, as long as their init value is the same, which simulates a pull-up-like or pull-down-like behavior.

When designing connect() I opted to treat reset-less signals conservatively, because of a quirk associated with them. Since they do not get reset with the clock domain, it means that after the domain reset (which may come at any point), their value is unpredictable. Although Amaranth does not track whether a signal bit has a defined value or not yet, once it does, the value of a reset-less signal will be considered by the simulator to be undefined (similar to 'x). This is to make sure that when you are testing your design piece-by-piece, you do not miss initialization-related issues that could invalidate your verification effort when the building block becomes a part of the larger system, which does have a post-power-on reset.

Since I am considering init a part of the signature in connect(), it seemed logical to consider reset_less also in some way. But at the same time, it felt too onerous to require reset_less to match on every signal being connected, since, as you correctly note, it is often used to address implementation constraints, like reset network fan-out. Requiring it to match would effectively require all standard interconnect to be generic over reset_less of every signal, which is completely impractical.

I am starting to suspect that the right approach here is to relax the restriction on reset_less so that connect() ignores it, but I'm not yet confident this is the right choice.

daniestevez commented 8 months ago

I was going to suggest that reset_less could be a property of Signature, so we could have Out(64, reset_less=True) and Out(64, reset_less=False), as well as In(64, reset_less=True) and In(64, reset_less=False). The semantics would be that reset_less=True means that the producer doesn't guarantee a well-defined value after reset on an Out, and that the consumer doesn't require a well-defined value after reset on an In. So In(64, reset_less=True) can be connected to any Out(64) regardless of its reset_less setting, but In(64, reset_less=False) can only be connected to Out(64, reset_less=False). In this way, an AXI-S interface could perhaps be declared {"tdata": Out(64, reset_less=True), "tvalid": Out(1, reset_less=False), "tready": In(1, reset_less=False)}. This would mean that tvalid and tready must be set to well-defined values after reset, but tdata doesn't need to, under the assumption that tvalid would be set to 0 at reset, and tdata will be have been set to a defined value by the moment that tvalid becomes 1.

However, thinking more about this I don't think that reset_less is a property that should be tracked by Signature. To me it seems that it is an implementation detail about how a component manages to initialize itself coming out of reset in a way that complies with the protocol. The example I gave for AXI-S sounds reasonable, but for more complex protocols where there are more dependencies between the signals, it doesn't seem possible to say whether they're reset-less or not in the Signature. For example, in AXI4, a producer can have BREADY set to an undefined value after reset provided that it sets it to a correct value before initiating any transactions (because the value of BREADY doesn't matter before the producer makes any transactions, since BVALID shouldn't become high). Thus whether BREADY can be reset-less can depend on other signals, such as for instance whether AWVALID and WVALID are reset to zero or set to Const(1) (for a producer that tries to write in every clock cycle). For the latter, probably BREADY needs to be set to a well-defined value immediately after reset.

Additionally, there is also the example I gave. A component that has data: Out(64) can then have in elaborate()

my_data = Signal(64, reset_less=something, reset=whatever)
m.d.comb += self.data.eq(my_data)

So for component ports that are not Const(), it really seems that whether they're reset-less or not, and if they're not, what is their reset value, is really an implementation detail that is open to how elaborate() is implemented, rather than something that the Signature can enforce.

whitequark commented 7 months ago

So for component ports that are not Const(), it really seems that whether they're reset-less or not [...] is really an implementation detail that is open to how elaborate() is implemented, rather than something that the Signature can enforce.

Yes.

So for component ports that are not Const(), it really seems that [...] what is their reset value, is really an implementation detail that is open to how elaborate() is implemented, rather than something that the Signature can enforce.

No, because undriven inputs assume their reset/init values, and connect() needs to be able to ensure that all of the undriven inputs have the same tie-off value.

daniestevez commented 7 months ago

No, because undriven inputs assume their reset/init values, and connect() needs to be able to ensure that all of the undriven inputs have the same tie-off value.

Can't you do something like this?

class SimpleStreamSignature(wiring.Signature):
    def __init__(self, data_shape):
        super().__init__({
            "data": Out(data_shape),
            "valid": Out(1),
            "ready": In(1)
        })

    def __eq__(self, other):
        return self.members == other.members

class StreamProducer(wiring.Component):
    en: In(1)
    source: Out(SimpleStreamSignature(8))

    def elaborate(self, platform):
        m = Module()
        source = Signal(8, reset_less=True)
        m.d.comb += [self.source.data.eq(source), self.source.valid.eq(self.en)]
        return m
whitequark commented 7 months ago

I don't understand how is this relevant?

daniestevez commented 7 months ago

(Just edited a couple mistakes in the example)

If this example works as I think it works, then the source.data port of this StreamProducer is (or behaves as) reset-less. So it shows that the SimpleStreamSignature cannot enforce whether a Component using this Signature makes some of the Signature ports be reset-less or not.

whitequark commented 7 months ago

I agree with that, but that's not what I was highlighting in this comment, please take another look at exactly the bits I quoted.

daniestevez commented 7 months ago

Reading your comment more carefully, I realize that I don't understand what you mean by this.

No, because undriven inputs assume their reset/init values, and connect() needs to be able to ensure that all of the undriven inputs have the same tie-off value.

I guess that I can edit my example to set source = Signal(8, reset=42). Does this have any implications for what you said?

How does connect() allow undriven inputs? This isn't mentioned in the docs.

whitequark commented 7 months ago

How does connect() allow undriven inputs? This isn't mentioned in the docs.

It is not just mentioned in the docs but it's highlighted in bold.

whitequark commented 7 months ago

I guess that I can edit my example to set source = Signal(8, reset=42). Does this have any implications for what you said?

But to answer this: no, since that would make source (or rather self.source.data to which it's connected) an unconnected output, which have no speical semantics.

daniestevez commented 7 months ago

Maybe this example (it works in the playground) illustrates better what I have in mind. It's heavily based in the examples in the docs. The StreamProducer can set the reset value of its source.data to any arbitrary value, and it can be connected to a StreamConsumer that doesn't know anything about this reset value. To me, this shows that SimpleStreamSignature cannot enforce the reset values of the ports that it describes.

from amaranth import *
from amaranth.sim import Simulator, Tick
from amaranth.back import rtlil, verilog
from amaranth.lib import wiring
from amaranth.lib.wiring import In, Out
import amaranth_playground
import random

# example from https://amaranth-lang.org/docs/amaranth/latest/stdlib/wiring.html#amaranth.lib.wiring.connect
class SimpleStreamSignature(wiring.Signature):
    def __init__(self, data_shape):
        super().__init__({
            "data": Out(data_shape),
            "valid": Out(1),
            "ready": In(1)
        })

    def __eq__(self, other):
        return self.members == other.members

# example from https://amaranth-lang.org/docs/amaranth/latest/stdlib/wiring.html#amaranth.lib.wiring.connect
# with an extra __init__() and elaborate() filled in
class StreamProducer(wiring.Component):
    en: In(1)
    source: Out(SimpleStreamSignature(8))

    def __init__(self, data_reset_value):
        super().__init__()
        self.data_reset_value = data_reset_value

    def elaborate(self, platform):
        m = Module()
        source = Signal(8, reset=self.data_reset_value)
        with m.If(self.source.valid & self.source.ready):
            m.d.sync += source.eq(source + 1)
        m.d.comb += self.source.data.eq(source)
        m.d.sync += self.source.valid.eq(self.en)
        return m

# example from https://amaranth-lang.org/docs/amaranth/latest/stdlib/wiring.html#amaranth.lib.wiring.connect
# with additional data and ready ports and with elaborate() filled in
class StreamConsumer(wiring.Component):
    sink: Out(SimpleStreamSignature(8).flip())
    data: Out(8)
    ready: In(1)

    def elaborate(self, platform):
        m = Module()
        with m.If(self.sink.valid & self.sink.ready):
            m.d.sync += self.data.eq(self.sink.data)
        m.d.comb += self.sink.ready.eq(self.ready)
        return m

m = Module()
m.submodules.producer = producer = StreamProducer(data_reset_value=0xab)
m.submodules.consumer = consumer = StreamConsumer()
wiring.connect(m, producer.source, consumer.sink)

def testbench():
    for _ in range(4):
        yield Tick("sync")
    yield producer.en.eq(1)
    for _ in range(100):
        yield Tick("sync")
        yield consumer.ready.eq(random.randrange(2))

sim = Simulator(m)
sim.add_clock(1e-6)
sim.add_process(testbench)
with amaranth_playground.show_waveforms(sim):
    sim.run()

amaranth_playground.show_verilog(verilog.convert(m, ports=[consumer.data, consumer.ready]))
# amaranth_playground.show_rtlil(rtlil.convert(m, ports=[producer.tdata, producer.tvalid, producer.tready]))
daniestevez commented 7 months ago

It is not just mentioned in the docs but it's highlighted in bold.

Thanks! I saw it now:

(If no interface object has an output for a given path, no connection at all is made.)

However you said:

No, because undriven inputs assume their reset/init values, and connect() needs to be able to ensure that all of the undriven inputs have the same tie-off value.

I don't see why in the case where no interface object has an output for a given path, connect() needs to do something, rather that just leaving the inputs for this path undriven, and it is true that in Amaranth undriven signals will get their reset value, but I don't see why this has to be the same for all inputs.

wanda-phi commented 7 months ago

It is not just mentioned in the docs but it's highlighted in bold.

Thanks! I saw it now:

(If no interface object has an output for a given path, no connection at all is made.)

However you said:

No, because undriven inputs assume their reset/init values, and connect() needs to be able to ensure that all of the undriven inputs have the same tie-off value.

I don't see why in the case where no interface object has an output for a given path, connect() needs to do something, rather that just leaving the inputs for this path undriven, and it is true that in Amaranth undriven signals will get their reset value, but I don't see why this has to be the same for all inputs.

Because after I did a connect(a, b, c), "all signals of a have identical values to corresponding ones in b and c" is a reasonable assumption that I want to hold.

daniestevez commented 7 months ago

Oh, so connect() is supposed to tie all the inputs with the same path together even if there is no output driving those inputs?

wanda-phi commented 7 months ago

Oh, so connect() is supposed to tie all the inputs with the same path together even if there is no output driving those inputs?

It's supposed to ensure they have the same state, which it does by making sure the initial values are the same.

whitequark commented 7 months ago

Check will be relaxed in https://github.com/amaranth-lang/amaranth/pull/1268.