<zyp[m]>
I figure it doesn't need any stream features we don't need yet, makes it easier to hook up to something needing flow control, and the stream's ready/valid signal would be optimized out anyway if they're constant 1
<zyp[m]>
s/need/have/
<zyp[m]>
and no, it does not solve reset, just flow control
<zyp[m]>
for reset, I think ResetInserter is fine
<adamgreig[m]>
OK, I'm happy to update to use the new streams for the output
<adamgreig[m]>
I was wondering if the existing lib.crc should also be updated to use streams, but probably better to do this lfsr one first and use the experience from that
<zyp[m]>
CRC would probably benefit from framing, which we don't have yet
<tpw_rules>
should there be a state load input? that would also solve the reset case
<whitequark[cis]>
for streams, the reset is a special condition that's called out in the stream RFC/docs
<whitequark[cis]>
basically, when you reset something stream based, it's your problem to find out whether what's connected to the output can cope with it
<whitequark[cis]>
we should of course also call that out in the docs
<tpw_rules>
i think also with streams the usefulness of the state output is decreased
<whitequark[cis]>
I'm going to go ahead and go through my feedback on all of the questions in it
<whitequark[cis]>
"We could simplify the implementation and API by only supporting Galois or only Fibonacci implementations" <- this doesn't seem like it's a huge burden to keep supporting, but I also don't have the background to evaluate the tradeoff properly; we can always deprecate one option later since it doesn't really break the interface
<whitequark[cis]>
"We could not support multi-bit generation, but this is a useful feature for many applications" <- this is a huge part of the motivation for me to have this in the standard library in first place, so I think it should stay
<zyp[m]>
we should absolutely have multi-bit generation
<whitequark[cis]>
"The split between Algorithm and Parameters mirrors that in lib.crc, but with fewer parameters we could consider merging them or making the Parameters settings be part of the Processor creation." <- the split seems good to me, it allows us to have e.g. PRBS31 in the catalog, which we wouldn't easily have otherwise (we could make PRBS31 a function and duplicate all of the arguments, but this seems strictly worse)
<whitequark[cis]>
* "The split between Algorithm and Parameters mirrors that in lib.crc, but with fewer parameters we could consider merging them or making the Parameters settings be part of the Processor creation." \<- the split seems good to me, it allows us to have e.g. PRBS31 in the catalog, which we wouldn't easily have otherwise (we could make PRBS31 a function and duplicate all of the arguments for all of the sequences etc, but this seems
<whitequark[cis]>
strictly worse)
<whitequark[cis]>
"Not sure if we should add explicit ready/valid signals or a reset signal, or leave those to EnableInserter/ResetInserter." <- I think this should use a stream since there's no reason not to, and I'm undetermined on reset
<tpw_rules>
is multi-bit generation just generating a single bit several times and concatenating them? i know a lot of applications just read the state directly to get many bits
<whitequark[cis]>
"Should we support internally-inverted LFSRs using XNOR operations, permitting a valid all-0s internal state to make initialisation easier e.g. on ASICs?" <- this doesn't seem super useful, mainly because if you really *needed* all-0 internal state everywhere, you'd make a compiler pass that pre/post-inverts FFs; ice40 is like this already, transparently to RTL
<zyp[m]>
tpw_rules: in my perspective, multi-bit generation is being able to generate e.g. a full byte every cycle
<Wanda[cis]>
tpw_rules: the LFSR also has to be capable of actually shifting that many bits per cycle
<whitequark[cis]>
tpw_rules: if you have a sequence like PRBS31 used for checking link integrity, the actual bit sequence is very important
<tpw_rules>
Wanda[cis]: yes, not saying it's not valuable, just trying to distinguish between the two cases
<whitequark[cis]>
it's not just like applications where you want white noise
<whitequark[cis]>
ah
<adamgreig[m]>
multi-bit generation means generating several sequence bits in a single clock, yea
<tpw_rules>
(which i think are different)
<adamgreig[m]>
reading the state directly is a bit of a cop-out, but it's definitely a different sequence
<adamgreig[m]>
(a lower resource cop-out, so sometimes it has its place I guess)
<whitequark[cis]>
that's actually a good question: should we support configurable output?
<adamgreig[m]>
configurable output?
<whitequark[cis]>
we can only really have one stream
<adamgreig[m]>
the current rfc has a 'state' output available to directly read the state
<whitequark[cis]>
oh yeah, you could use the stream to advance the state, and the state itself as the data out
<adamgreig[m]>
so you could ignore the stream payload and use that... or there could be both payloads in the stream I guess
<tpw_rules>
i'm concerned the state output would get too confusing with the stream
<whitequark[cis]>
both payloads would be super annoying
<adamgreig[m]>
I think the only time you'd want the state is you need minimum resource usage and you don't care about the poor quality
<whitequark[cis]>
I think the viable options are "choice during construction between state out / bits out" and "state:Out plus a stream of bits"
<adamgreig[m]>
using state directly gives you some weird runs of numbers
<tpw_rules>
~every software application i've seen of LFSRs (in e.g. game RNG) uses state directly
<adamgreig[m]>
but it's true that a lot of applications do exactly that, yea
<whitequark[cis]>
personally, I feel that the use case of reading state directly isn't super important to support in the stdlib because of the low quality of the output
<whitequark[cis]>
it's not a good RNG
<tpw_rules>
i'm not sure it's important either, just familiar
<adamgreig[m]>
tpw_rules: are those applications in software or gateware?
<whitequark[cis]>
I may be wrong
<tpw_rules>
adamgreig[m]: software
<adamgreig[m]>
in software especially it's very appealing because it's much quicker and the state is right there, lol
<tpw_rules>
it should probably be at least noted as a trap for software familiar users
<Wanda[cis]>
the thing about LFSRs is: they're really easy to implement when you shift by one bit only and read the whole state; IMO the value proposition of this RFC is in the multi-bit-per-cycle case
<whitequark[cis]>
yes
<adamgreig[m]>
Wanda: I think the multi-bit is harder to DIY, but there's no reason to DIY the single-bit one either if the stdlib can readily provide it
<adamgreig[m]>
hence including the state output
<adamgreig[m]>
whitequark[cis]: yea, I think either is OK, not sure how to pick
<adamgreig[m]>
maybe the first is nicest
frgo has joined #amaranth-lang
<zyp[m]>
I guess state output (and probably setting the state even) could also be useful in simulations
<adamgreig[m]>
I don't think you ever want both
<adamgreig[m]>
also it's not clear to me that the state output will be what you want when there's simultaneously multi-bit output
<adamgreig[m]>
so the first option permits enforcing single-bit-per-cycle output when the state is being output
<adamgreig[m]>
s/output/generation/
<adamgreig[m]>
(since the state itself is your only output, that seems fair)
<whitequark[cis]>
zyp: for simulations, this could be a method or two on the class
<whitequark[cis]>
so it doesn't have to be a signal
<zyp[m]>
indeed, probably better to not mix the use cases
<zyp[m]>
I also think an option to select what's in the stream payload sounds the nicest
<whitequark[cis]>
should the state-as-stream output gateware be just a different class entirely?
<whitequark[cis]>
the Processor is not a huge thing
<adamgreig[m]>
being able to load the state is possibly useful too, rather than just resetting to 0, but it does complicate the interface a bit
<whitequark[cis]>
adamgreig[m]: you could put it as an unresolved question
<adamgreig[m]>
if it was a different class it would probably just be a wrapper around constructing the same thing with different parameters
<adamgreig[m]>
or a different constructor, basically
<whitequark[cis]>
I guess we could make that a part of Parameters maybe?
<adamgreig[m]>
hmmm, yea
<whitequark[cis]>
since that also lets you compute it in software
<zyp[m]>
that sounds good to me
<whitequark[cis]>
actually, that last bit convinces me it should be on Parameters, not Processor
<adamgreig[m]>
yea, that's nice
<tpw_rules>
how would you sync it with the processor?
<whitequark[cis]>
sync?
<adamgreig[m]>
then Parameters by default is output the actual output bits, with output_width per cycle, or otherwise outputs the state instead, in which case output_bits isn't used
<tpw_rules>
what the state is for the current output of the processor
<whitequark[cis]>
adamgreig: something like that, we can bikeshed the exact interface later
frgo has quit [Ping timeout: 258 seconds]
<tpw_rules>
or is just the switch between the state being the stream and the multi-bit version on Parameters
<whitequark[cis]>
sorry, I'm not sure I can parse that
<tpw_rules>
"actually, that last bit convinces me it should be on Parameters, not Processor" what is "it" here?
<zyp[m]>
the option to select the output type
<whitequark[cis]>
(quick note: "structure: either "galois" or "fibonacci" (default), which type of implementation to generate" <- I think this should be an enum, `lfsr.Structure` or something with those string values, not just a bare string)
<tpw_rules>
ah ok, then i had misunderstood earlier
<adamgreig[m]>
tpw_rules: the Parameters is used to construct a different Processor
<adamgreig[m]>
oh yep, will do an enum, and probably a second for selecting output type
<whitequark[cis]>
so... what should Processor's signature be? just a single output stream?
<zyp[m]>
unless you want an input to load a state as well, that sounds enough
<whitequark[cis]>
that could be left to future work, I guess
<adamgreig[m]>
yea, one output stream, with the payload width depending on Parameters
<zyp[m]>
agreed
<whitequark[cis]>
my proposed interface would be rst: In(1), state_init: In(n)
<adamgreig[m]>
you could have an initial_state in Parameters (default 0) which it gets reset to
<adamgreig[m]>
which is like a poor man's "load new state"
<whitequark[cis]>
that seems limiting as it can't be varied at runtime
<adamgreig[m]>
yea, the state_init input signal is more flexible
<tpw_rules>
i like the input signal
<adamgreig[m]>
you think rst: In rather than using ResetInserter then?
<adamgreig[m]>
I guess that makes sense alongside state_init
<adamgreig[m]>
maybe load and initial_state?
<whitequark[cis]>
or perhaps have just state_init and a check inside which uses state_init instead of the state whenever the state is 0
<whitequark[cis]>
this way, it would work fine if you do m.d.comb += state_init.eq(whatever) and use ResetInserter
<whitequark[cis]>
or the same with a clock domain reset
<whitequark[cis]>
that seems useful
<tpw_rules>
does that mean you could avoid getting stuck?
<zyp[m]>
we should maybe bikeshed name conventions for stream inputs/outputs if we don't have any already
<whitequark[cis]>
on the whole, I feel like there's enough uncertainty about it that it might be best left to future work
<whitequark[cis]>
we have not done that
<whitequark[cis]>
I think for components with just a single stream I call it stream, or i_stream/o_stream for FIFOs
<whitequark[cis]>
but this is not set in stone
<adamgreig[m]>
whitequark[cis]: yea, this seems like a nice option too
<adamgreig[m]>
the state signal would otherwise be init=1
<adamgreig[m]>
but happy to leave this to future work and get the other bits sorted first if you prefer
<whitequark[cis]>
I think the state_init option could be okay if everyone agrees on it, since it avoids getting stuck if that happens for some reason, lets you reload the state, and lets you defer the initial state to runtime instead of fixing it in `Parameters
<whitequark[cis]>
* I think the state_init option could be okay if everyone agrees on it, since it avoids getting stuck if that happens for some reason, lets you reload the state, and lets you defer the initial state to runtime instead of fixing it in Parameters
<whitequark[cis]>
but I guess it could be seen as slightly weird
<whitequark[cis]>
I think it's worth putting into alternatives at least
<whitequark[cis]>
having one less parameter in Parameters while losing no functionality is kind of nice
<adamgreig[m]>
I'd be tempted to have a reload or something input alongside it then, which can always be left unused, rather than requiring ResetInserter?
<whitequark[cis]>
why? ResetInserter is idiomatic for Amaranth and not particularly heavy
<whitequark[cis]>
I think it's generally a good idea to avoid inputs which are basically just a reset, since it adds ambiguity
<whitequark[cis]>
and in cases like this, there can actually be a semantic difference, depending on how it's implemented
<adamgreig[m]>
fair
<whitequark[cis]>
we can always trivially add reload later
<adamgreig[m]>
conceptually I imagine it as being a bit distinct to reset only in that "I'd like to start from a new state now", but it is identical to reset
<adamgreig[m]>
yea, true
<adamgreig[m]>
OK, so resetinserter + state_init input which is used (combinatorially I guess, to avoid a cycle of latency) whenever state==0
<whitequark[cis]>
combinationally, yeah
<whitequark[cis]>
this would only pessimize timings when it's dynamic, which feels KO
<whitequark[cis]>
s/KO/OK/
<adamgreig[m]>
yea
<whitequark[cis]>
I guess it would also add a mux on ASICs
<whitequark[cis]>
this still feels fine since you don't want your LFSR to be stuck ever
<adamgreig[m]>
yea, but it "shouldn't" ever get to a 0 state
<adamgreig[m]>
so idk if you'd normally add a check for that anyway
<adamgreig[m]>
if you reset to all-0 then maybe the resulting logic to have it reset to a non-zero signal looks very similar though
<adamgreig[m]>
shame we can't specialise elaboration for whether the input is const or not
<whitequark[cis]>
maybe you had a power glitch or something
<whitequark[cis]>
re: specializing elaboration, Wanda had a proposal providing something like that, but it would be extremely hard to commit to supporting it for arbitrary user code
<whitequark[cis]>
adamgreig, since you'd be implementing them anyway, do you think you can do the usual meeting summary on the RFC thread?
<whitequark[cis]>
I'm getting pretty tired and would like to log off early today
<adamgreig[m]>
yep no problem
<whitequark[cis]>
thanks!
<adamgreig[m]>
I'll try and get the rfc itself updated tonight too
<adamgreig[m]>
thanks for the feedback everyone!
<whitequark[cis]>
I think that covers nearly all of the questions in the RFC? unless anyone has any other comments
<whitequark[cis]>
yep, thanks from me as well, it was very productive
<tpw_rules>
thank you
<tpw_rules>
has anybody yet made AXI components i can steal? (preferably 3)