<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 5a783a9 - Deploying to main from @ amaranth-lang/amaranth-soc@fe75f404adf5b37f0fa02486fe6cae0eaf09987c 🚀
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] e1cc824 - Deploying to main from @ amaranth-lang/amaranth-soc@19235589cb79ec5670445f64fe22ddd3a130e91d 🚀
<tpw_rules>
is it finally time to close the CSR request issues on amaranth-soc too?
<whitequark[cis]>
good evening. it is the time for our regularly scheduled Amaranth language meeting. note: I am attending from a train, which being British is atrocious in most of its properties. I will also have to change trains in the middle of a meeting due to hours of delay, so this meeting will be a little sketchier than usual. who is attending?
<tpw_rules>
hello, i am here
<jfng[m]>
o/
<zyp[m]>
I'm here
galibert[m] has joined #amaranth-lang
<galibert[m]>
waves
<whitequark[cis]>
Wanda?
<Wanda[cis]>
meow.
<cr1901>
here
<whitequark[cis]>
Chips4Makers (aka Staf Verhaegen) is absent today due to travel, but I don't think anything that is being discussed are things he'd have strong opinions on
<whitequark[cis]>
this functionality will synchronize Amaranth with both CXXRTL and SystemVerilog, providing identical capabilities; it will also enable better RTL debugging once the extension work I've been heading at ChipFlow is publicly released [no ETA]
<whitequark[cis]>
* once the VS Code extension work, * publicly released \[no ETA, * [no ETA\]
<cr1901>
why are f-strings horrifying?
<whitequark[cis]>
they aren't, it's just that they're not extensible
<tpw_rules>
Signal is a ValueCastable?
<galibert[m]>
tpw_rules: not yet
<galibert[m]>
maybe never
<tpw_rules>
it's just a Value then?
<galibert[m]>
Yes
<Wanda[cis]>
Signal is a plain value
<whitequark[cis]>
to make f-strings work with Amaranth we'd have to introduce a magic token which turns into a string that deserializes back to an Amaranth value via a global map
<whitequark[cis]>
that would be horrifying
<whitequark[cis]>
Signal is ValueLike but not ValueCastable
<Wanda[cis]>
however, note that if you create a Signal for a ShapeCastable, the constructor can return a custom ValueCastable that is not a Signal
<tpw_rules>
so if you tried to use an f-string you'd just get like Signal(3) or something in your string
<whitequark[cis]>
it would fail, iirc
<whitequark[cis]>
because Signal doesn't implement Format
<whitequark[cis]>
well, if you use {} then it'd be (sig foo)
<Wanda[cis]>
note: there was a Python proposal, PEP 501, that'd introduce i-strings, which would do what we want
<Wanda[cis]>
but it's been deferred indefinitely
<cr1901>
If you use Print/Format functionality that's not supported in Verilog, will output to Verilog gracefully degrade or error or?
<Wanda[cis]>
cr1901: degrade
<whitequark[cis]>
Wanda: I think the references to Verilog strings are a bit misleading since, AFAIK, the byte ordering is different (Amaranth has first character in LSB, Verilog in MSB)
<Wanda[cis]>
wait what
<Wanda[cis]>
oh.
<Wanda[cis]>
Amaranth is sane
<whitequark[cis]>
yeah the Verilog strings are batshit
<cr1901>
Okay merge then
<galibert[m]>
Verilog hates you and really wants to show it to you
<Wanda[cis]>
I
<whitequark[cis]>
it sure does
<Wanda[cis]>
do we have to byteswap in RTLIL emitter?
<Wanda[cis]>
I
<Wanda[cis]>
hate this
<zyp[m]>
I also vote merge
<whitequark[cis]>
... good question
<Wanda[cis]>
I think we do
<whitequark[cis]>
my vote is merge, no modification required
<jfng[m]>
`Assert(test, message=None)` -> how about `msg` instead, to be consistent with the `unittest` library ?
<whitequark[cis]>
unittest library has plenty of bad decisions like using Java-inspired assertEqual
<whitequark[cis]>
I don't think we should be try to be consistent with the least consistent library in wide use
<jfng[m]>
ok
<jfng[m]>
merge then
<Wanda[cis]>
hm, is the message parameter supposed to be positional?
<whitequark[cis]>
Assert is supposed to be consistent with the assert keyword, which doesn't have keyword args
<Wanda[cis]>
... wait I wrote that RFC
<whitequark[cis]>
lol
<whitequark[cis]>
yeah I think we can have it positional, Python's assert is like assert foo, f"Blah"
<Wanda[cis]>
yeah positional
<Wanda[cis]>
in which case it doesn't matter that much
<whitequark[cis]>
tpw_rules: your vote is the last remaining I think?
<tpw_rules>
yes, i vote merge. the f-string foibles are as you describe, which seems not stupendous but reasonable. maybe a future patch could warn if it sees "(sig" in the input string
<whitequark[cis]>
oh, we can just define __format__ on Value or something
<whitequark[cis]>
I think that'll be enough?
<Wanda[cis]>
I think so, yes
<Wanda[cis]>
unless hm
<Wanda[cis]>
do we have to define __str__ too? let me check
<tpw_rules>
f"{Signal(3)}" -> '(sig $signal)'
<whitequark[cis]>
anyway, disposition on RFC 50: merge 🎉, with the clarification on byte ordering in strings
<tpw_rules>
f"{Signal(3):3d}" -> TypeError: unsupported format string passed to Signal.__format__
<galibert[m]>
it's nice sometimes to be able to print an object just to be sure what the hell it is
<whitequark[cis]>
any comments on this RFC *besides* naming?
<whitequark[cis]>
galibert[m]: there was this, Wanda
<cr1901>
used when a ValueCastable expression is requested <-- can you elaborate? The Memory motivation I understand
<galibert[m]>
Wanda: ok, we need that capability indeed, I'm ok with the rfc then (and I don't care much about the naming)
<Wanda[cis]>
galibert: I find the `data` tooling also lacking in that there is no nice way to *construct* a structure-typed value from fields, which I wanted to write an RFC for, but that's a different issue I haven't gotten around to yet
<cr1901>
anyways merge
<galibert[m]>
Yeah, that would be quite nice too
<zyp[m]>
Catherine: I think it's useful to be able to do compile time calculation on value-castable literals, and this is as you said inconsistent with `Const`
<tpw_rules>
i vote abstain, i haven't used this part enough
<zyp[m]>
and whatever naming we go for here applies for RFC #41 too
<whitequark[cis]>
zyp[m]: only if the computations are mutating
<whitequark[cis]>
if they're not mutating you're free to do it. we could have a non destructive update on data.Const
<Wanda[cis]>
also: I'd like to note that I intend to beef up LiteralView or whatever it ends up being called with more functionality afterwards (see future possibilities)
<zyp[m]>
I figure consts/literals are by definition immutable
<Wanda[cis]>
in particular having a constructor from fields and settable fields would be nice
<Wanda[cis]>
(it ties in with the goal of replacing Glasgow's bitstruct)
<whitequark[cis]>
in RFC 41 you should definitely not do destructive updates as numbers must be immutable
<galibert[m]>
Wanda: why is a new type needed and whatever is the underlying view of the ValueCastable insufficient?
<whitequark[cis]>
galibert[m]: we already rejected the proposal to allow View of Const to participate in computation (a year ago or so)
<galibert[m]>
Oh, that's why
<zyp[m]>
Wanda: what is the backwards incompatible change of returning a `LiteralView` from `.const()`?
<jfng[m]>
merge
<Wanda[cis]>
zyp: well it changes the returned type
<Wanda[cis]>
recursively, too (if you request a field on the result, you get another from_bits result instead of a ValueLike)
<whitequark[cis]>
Wanda[cis]: my view on naming: Const if immutable, Literal if we want to allow mutability later
<whitequark[cis]>
zyp[m]: I think we should do this fwiw
<zyp[m]>
I agree
<whitequark[cis]>
most uses of .const are inside Signal constructor, where the change is fine
<zyp[m]>
we also changed the .const() return type recently in #29
<zyp[m]>
and this would be a smaller change
<Wanda[cis]>
oh, there is an unresolved question that I missed
<Wanda[cis]>
do we want to constrain from_bits to return a ValueLike?
<whitequark[cis]>
Wanda[cis]: yes
<Wanda[cis]>
I feel this is not strictly necessary, as long as const can convert it back, and could allow using some pre-existing types for funny ShapeCastables
<whitequark[cis]>
I think we've already constrained .const to do that?
<whitequark[cis]>
hmm
<Wanda[cis]>
but it may be a useful property
<Wanda[cis]>
in the end I couldn't reach a decision myself
<whitequark[cis]>
tbh we can leave this to after implementation
<galibert[m]>
I'm completely lost between the different types and accessors
<galibert[m]>
oh well
<zyp[m]>
.const() could e.g. take a list, so .from_bits() could be allowed to return a list
<whitequark[cis]>
writing the docs is very useful to nail down things like that
<whitequark[cis]>
zyp[m]: oh, good poiny
<whitequark[cis]>
* oh, good point
<whitequark[cis]>
I guess we should rather allow that then
<Wanda[cis]>
like if Python happened to have an existing first-class fixed-point type, the fixed-point RFC could then just use it instead of having to provide its own
<zyp[m]>
yep
<galibert[m]>
(I suspect I need a graph of relations between all those types that does not look like the L-word relations graph)
<Wanda[cis]>
oh yes, that would be handy.
<Wanda[cis]>
<whitequark[cis]> "my view on naming: Const if..." <- oh yeah this may be why I changed to `Literal`
<zyp[m]>
so my vote would be to merge with the name changed and also updating Layout.const while we're at it
<Wanda[cis]>
but in that case, I think we should reach a decision on that now
<Wanda[cis]>
do we want to allow __setitem__ / __setattr__ on the class?
<zyp[m]>
is a plain Const immutable?
<zyp[m]>
(and if it's not, should it be?)
<whitequark[cis]>
zyp: it is
<Wanda[cis]>
(I'd say it's useful because bitstruct works like that, but ... this is kinda unprecedented in Amaranth, we are moving away from mutable data types)
<whitequark[cis]>
Wanda: we can fix `bitstruct` tbh
<jfng[m]>
you can set `Const.value` after the fact, though it is extra cursed
<Wanda[cis]>
(and I think we can make do with a .like()-like constructor)
<Wanda[cis]>
jfng: not anymore, you cannot
<whitequark[cis]>
jfng: no, we removed that API
<whitequark[cis]>
a few days ago
<jfng[m]>
oh, didn't check out; ok
<whitequark[cis]>
Wanda: we can also make the Glasgow thing mutable, I guess? but conform to the same API
<Wanda[cis]>
we discussed it a few meetings ago, all AST types are now immutable
<whitequark[cis]>
basically, my gut feeling is that in Amaranth code (that generates RTL), we will rarely if ever want to mutate any consts
<Wanda[cis]>
(except for source locations and signal names, for ... reasons)
<whitequark[cis]>
the most of that want will come from tests
<Wanda[cis]>
you mean sim testbenches?
<whitequark[cis]>
yeah
<Wanda[cis]>
hmm
<zyp[m]>
I propose we call it data.Const, leave it immutable, but castable to a list/dict if the user wants a mutable copy
<whitequark[cis]>
we could also just return a dict from from_bits
<Wanda[cis]>
actually in sim you can also just .set() directly to a view field...
<Wanda[cis]>
yeah
<whitequark[cis]>
oh, yeah
<Wanda[cis]>
I'm leaning towards immutable and data.Const
<zyp[m]>
same
<whitequark[cis]>
I'm happy with immutable data.Const
<Wanda[cis]>
(and providing the functionality via Const.like() constructor or something)
<cr1901>
I like data.Const
<Wanda[cis]>
also, any thoughts on the from_bits name? it looks kinda odd but I didn't come up with any alternatives
<whitequark[cis]>
from_value?
<whitequark[cis]>
no, not a great one either
<whitequark[cis]>
I don't have any objections to from_bits
<whitequark[cis]>
... from_int?
<cr1901>
from_pyint?
<whitequark[cis]>
we don't do py prefixes like that, but yeah, from_int is fine with me
<Wanda[cis]>
from_int would not work great on fixed-point
<whitequark[cis]>
though I'm ambivalent between that and from_bits
<whitequark[cis]>
Wanda: oh yeah, good point
<Wanda[cis]>
(ie. it'd imply a cast, not a reinterpret-cast)
<jfng[m]>
from_bits looks like the better choice so far
<cr1901>
transmute :P?
<cr1901>
from_bits is fine
<zyp[m]>
const_view?
<Wanda[cis]>
very well; my vote: merge, data.Const (immutable) and from_bits, from_bits is allowed to return arbitrary type as long as const knows how to deal with it, const amended to return data.Const
<Wanda[cis]>
zyp[m]: the problem with that is that we need to differentiate from `const`, which also returns a const view
<zyp[m]>
yeah…
<whitequark[cis]>
likewise
<zyp[m]>
I vote the same
<cr1901>
merge
<tpw_rules>
(abstain, if you missed it earlier)
<jfng[m]>
merge
<whitequark[cis]>
galibert jfng
<galibert[m]>
I said merge way earlier :-)
<whitequark[cis]>
okay, disposition on RFC 51: merge, with amendments
<cr1901>
I have still not replaced old style sims w/ the changes earmarked for 0.5, so no comments/will abstain
<tpw_rules>
i still don't really grok and will let those with more stake decide and explain to me :) (also going to formally depart here)
<Wanda[cis]>
hm.
<Wanda[cis]>
so, we want to have a bunch of helper functions on elaboratables, like the send/recv mentioned
<zyp[m]>
Catherine: I thought a bit more on what type `sim.time()` should return, and I figure we should either use fractional seconds as I proposed yesterday, or make a custom type to represent seconds with femtosecond resolution
<galibert[m]>
Could there be pseudo-async triggers?
<Wanda[cis]>
and we also have two kinds of processes with different semantics
<whitequark[cis]>
I'm happy with a custom type yeah (mirroring cxxrtl::time)
<galibert[m]>
E.g. wait for an expression to become true but do not specify after which domain tick to look
<Wanda[cis]>
this means that a particular helper function is likely valid only in one kind of process, and will do something nonsensical in the other
<Wanda[cis]>
it feels like we should have some way to mitigate this at least (some way to get current process type? idk)
<zyp[m]>
yeah, we should probably have an attribute on the simulation context that we can assert on
<galibert[m]>
Also, could we have a time as a exact number of ticks on a given domain?
<jfng[m]>
`sim.active()` -> `sim.activate()` ?
<galibert[m]>
Without having to mess up with rounding issues?
<whitequark[cis]>
galibert[m]: if you express everything as periods in femtos there are no rounding issues
<whitequark[cis]>
jfng[m]: the naming issue I bring up is with the "active" name in general
<jfng[m]>
ah right, I didn't get to that unresolved question
<jfng[m]>
my point can be changed to "this should be a verb"
<zyp[m]>
I think a passive process might be a reasonable enough term, but active doesn't work as well as a counterpart
<whitequark[cis]>
jfng[m]: yes
<galibert[m]>
Is what it means documented somewhere or should I continue to swim in beatific ignorance?
<whitequark[cis]>
sim.run() won't exit while any process is active
<zyp[m]>
galibert[m]: simulation runs until all active processes are done
<zyp[m]>
i.e. if you've got a process with an infinite loop, it should probably be passive
<galibert[m]>
So if you give run() a time, it's a maybe?
<galibert[m]>
or a "at least"?
<whitequark[cis]>
that's run_until() not run()
<galibert[m]>
oh
<zyp[m]>
the point is, once you've only got passive processes left, the simulation doesn't do anything useful and can terminate
<zyp[m]>
a passive process would be e.g. simulating an IO register or something, just shoveling data from one point to another as long as the simulation is running
<galibert[m]>
Ok. I think it should not be a parameter of add_* but instead the process doing a sim.stop_me_whenever()
<zyp[m]>
no, that's what we're changing from
<galibert[m]>
why?
<galibert[m]>
you're just increasing the probability that the user forgets to put it in
<Wanda[cis]>
what?
<galibert[m]>
it's a property of the code, not a property of the add_*
<Wanda[cis]>
galibert[m]: what are you proposing here?
<Wanda[cis]>
a button to stop the entire simulation that needs to be pressed manually?
<whitequark[cis]>
galibert[m]: that is hard to miss though, since an infinite loop is quite visivle
<whitequark[cis]>
* that is hard to miss though, since an infinite loop is quite visible
<galibert[m]>
it's visible in the function the infinite loop is, not necessarily in the code wich invokes add_* which may have import-ed it
<zyp[m]>
in the old API, all processes are active by default, which means passive processes start by doing yield Passive()
<galibert[m]>
zyp: I think the old api is better in that area then
<galibert[m]>
(outside of moving that to a method of sim of course)
<Wanda[cis]>
anyway, something I'm missing: what's the rationale for having with sim.active() at all? in what circumstances do you really need to essentially hold a lock on simulation not ending?
<galibert[m]>
Wanda: disk backup of memory writes?
<zyp[m]>
Wanda[cis]: when you've got e.g. a packet printer and want it to finish the final packet before ending
<jfng[m]>
what if you have two back-to-back `with sim.active()` scopes, and the simulator exits in between ?
<Wanda[cis]>
hm
<Wanda[cis]>
fair enough, thanks
<whitequark[cis]>
jfng[m]: that is part of intended behavior, I think
<zyp[m]>
jfng[m]: I figure if there's no active processes when you leave the first scope, simulation will end there
<whitequark[cis]>
anyway, the meeting is done with two merged RFCs, thanks everyone! let's continue the discussion
<whitequark[cis]>
galibert[m]: that's not bad
<jfng[m]>
zyp[m]: in this case i kind of agree with @galibert:matrix.org that testbenches should be active by default
<zyp[m]>
I've been thinking «background» instead of passive
<zyp[m]>
jfng[m]: they are
<jfng[m]>
and once made passive, you don't go back
<whitequark[cis]>
testbenches *are* active by default
<cr1901>
I like background a lot, but guessing there's a reason that name wasn't used
<whitequark[cis]>
no one is changing that
<whitequark[cis]>
jfng[m]: the problem is that this defeats most of the purpose, taking a lock
<whitequark[cis]>
since you want to finish the transaction whenever your passive thing goes active
<galibert[m]>
jfng: nah, there are good reasons, with logging for instance, that want to be able to complete something. Kind of a light critical section
<galibert[m]>
s/that/to/
<Wanda[cis]>
wasn't there some name for this already
<Wanda[cis]>
eh
<Wanda[cis]>
Python has "daemon threads"
<whitequark[cis]>
`with sim.critical():`?
<Wanda[cis]>
but they cannot be un-daemoned
<zyp[m]>
daemon/critical works for me
<whitequark[cis]>
daemon is a terrible replacement for passive imo
<Wanda[cis]>
yeah...
<zyp[m]>
background/critical?
<jfng[m]>
galibert[m]: so if i understand, the designer needs to carefully ensure that as long as they need the simulation to run, there is at least one active process
<whitequark[cis]>
background= kwarg, critical context manager works for me
<whitequark[cis]>
jfng[m]: yeah. usually this will be your testbench
<whitequark[cis]>
but sometimes you want to extend the sim past your testbench's last action
<galibert[m]>
jfng: you tend to have one orchestrator which runs the test scenario
<Wanda[cis]>
they're... a rather horrible hack, but yes
<zyp[m]>
okay, so I figure memory.read(sim, address) and memory.write(sim, address, value[, mask]) should be the main API then, that could build on sim.memory_instance_read(memory, address) and sim.memory_instance_write(memory, address, value[, mask]) then, did I get the arguments right?
<zyp[m]>
how does mask work?
<Wanda[cis]>
conceptually it does `memory[address] = (memory[address] & ~mask) | (value & mask)`
<whitequark[cis]>
hm, good question
<whitequark[cis]>
I guess it needs to be similar to granularity/en
<galibert[m]>
Wanda: it's not the lane enables?
<Wanda[cis]>
for the lower-level MemoryInstance API at least
<galibert[m]>
Ah no, that's a port property, not a memory property
<whitequark[cis]>
so: only available for ArrayLayout and bare Shape, enables per lane, not per bit
<whitequark[cis]>
oh, for the lower-level API per-bit, yes
<zyp[m]>
should the higher level API be on the memory itself or the port?
<Wanda[cis]>
also memory_instance_read/write first argument would actually be a MemoryIdentity
<Wanda[cis]>
on the memory.
<whitequark[cis]>
zyp: simulation doesn't use ports
<whitequark[cis]>
it wouldn't really make sense for it to
<Wanda[cis]>
sim accesses don't commandeer a port, they go around them
<zyp[m]>
yeah, just thinking that granularity is a port property, and how that then interacts with mask
<whitequark[cis]>
granularity is a port property but lanes are a data type property
<whitequark[cis]>
or I guess masking is a data type property
<Wanda[cis]>
whitequark[cis]: unsigned bare `Shape`, to match the granularity constraint?
<whitequark[cis]>
yes
<zyp[m]>
but the argument itself is still a bitmask?
<Wanda[cis]>
a lane mask, in the case of ArrayLayout
<Wanda[cis]>
but yeah, it's an int
<whitequark[cis]>
that seems like the reasonable option, yeah
<zyp[m]>
maybe we should add a granularity argument then, to be consistent with the ports
<whitequark[cis]>
zyp[m]: that seems heavyweight
<zyp[m]>
e.g. for a 32-bit memory with bytelanes: write(…, granularity=8, mask=0b0001)
<jfng[m]>
i think a mask is enough
<jfng[m]>
granularity is a write port property
<galibert[m]>
That’s where it shows that bitmask is simpler
<whitequark[cis]>
I guess we could have a granularity defaulting to 1
<jfng[m]>
having to provide one for each simulatrd access is awkward
<jfng[m]>
s/simulatrd/simulated/
<whitequark[cis]>
idk
<jfng[m]>
hmm
<zyp[m]>
if there's no demand for it, I'd say we avoid scope creep and leave it out
<whitequark[cis]>
yeah
<jfng[m]>
jfng[m]: i retract that, using granularity and mask like suggested previously is indeed better
<jfng[m]>
which defaults to 1, as a kwarg
<whitequark[cis]>
it can't quite default to 1
<whitequark[cis]>
because of value castables
<zyp[m]>
it'd have to default to None, for checking to not produce false positives
<zyp[m]>
and None would then implicitly be 1 if a mask is provided
<jfng[m]>
if we can default to 1, then we can also add support for it later in a backward compatible change
<jfng[m]>
otherwise, we have to decide whether we want custom granularities right now
<whitequark[cis]>
I think we can just leave it for later always?
<whitequark[cis]>
the behavior with no kwarg is whatever we implement now is
<whitequark[cis]>
and granularity being provided forces it to that particular value
<jfng[m]>
yeah
<jfng[m]>
granularity defaults to None, and use `en` instead of `mask`, and follow the same behavior as memory.WritePort ?
<whitequark[cis]>
I don't think `en` is appropriate
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] whitequark 9d2a3d2 - Deploying to main from @ amaranth-lang/rfcs@c36f7816e27c665dec5bd6295f1b7d42b3b429aa 🚀
<whitequark[cis]>
zyp: there's a fair bit of overlap between `sim.changed` and `trigger_object.until`
<zyp[m]>
no
<whitequark[cis]>
how so?
<zyp[m]>
the former is an edge trigger, the latter is a level check
<whitequark[cis]>
what happens if signal is already at value for changed()?
<whitequark[cis]>
doesn't seem to be in the RFC
<zyp[m]>
it will wait until the next time it changes to value
<whitequark[cis]>
hmm
<whitequark[cis]>
this seems not entirely obvious
<Wanda[cis]>
alright, updated RFC 51 as well
<whitequark[cis]>
or maybe changed() says it all
<zyp[m]>
changed is pretty much equivalent to tick except it works with arbitrary signals instead of clock signals, and not just the value set by the clock domain's polarity
<whitequark[cis]>
right
<whitequark[cis]>
... with sim.clocked() as a clock counterpart for with sim.changed()?
<Wanda[cis]>
... I guess now I get to implement two RFCs
<zyp[m]>
I'm not sure clocked reads better than tick
<whitequark[cis]>
* ... sim.clocked() as a clock counterpart for sim.changed()?
<whitequark[cis]>
Wanda: sowing, reaping, etc
<whitequark[cis]>
(joking)
<Wanda[cis]>
it do be like that
<whitequark[cis]>
Wanda: I think you can remove the "Unresolved questions" bit from RFC 51
<whitequark[cis]>
since we did in fact bikeshed it
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] whitequark d51229c - Deploying to main from @ amaranth-lang/rfcs@953ddfe1dde6b0b2e47cf2804a895cea3d89a0a9 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] whitequark 018ec13 - Deploying to main from @ amaranth-lang/rfcs@0852d0ed1d60bdaf492a145c05df8754af3b4d4e 🚀
<Wanda[cis]>
the tricky part is figuring out what and how to print for our ValueCastables
<galibert[m]>
42?
<whitequark[cis]>
should it be on ValueCastable or ShapeCastable?
<Wanda[cis]>
and eg. if we're going to end up needing the Format-as-value thing, which would result in us getting blocked on substantial yosys changes
<Wanda[cis]>
well, you may not have a ShapeCastable
<tpw_rules>
whitequark[cis]: re the docs, i had in my mind's eye sort of an ideal doc page for a register; i know you said you were planning to discuss with jfng[m]. i also see a couple issues with the current CSR behavior which might need bug reports or RFCs
<whitequark[cis]>
right
<whitequark[cis]>
tpw_rules: the ALTERNATE thing was covered in my discussion of it
<whitequark[cis]>
(in ALTERNATE mode the meaning of i/o/oe becomes impl-specific)
<whitequark[cis]>
`addr_width` being mandatory is unavoidable, I think
<Wanda[cis]>
(and then RFC 41 gets merged and its format is just "lol nope")
<tpw_rules>
oh, hm, idk how i missed that. but the impl of what? wouldn't they become meaningless in ALTERNATE?
<jfng[m]>
the implementation of the peripheral itself
<whitequark[cis]>
tpw_rules: of the wider SoC
<tpw_rules>
how is it unavoidable? in the absurd case the peripheral could start at 1 and bump it up until it doesn't get errors
<jfng[m]>
e.g. users can design their own register-compatible peripheral
<whitequark[cis]>
jfng: re SetClr, I think an array of {set,clr} structs per pin is how we encode interleaving in our type system
<whitequark[cis]>
tpw_rules: yes and we decided to not do that in the CSR machinery redesign
<whitequark[cis]>
there used to be a mechanism called extend that we had to rip out
<tpw_rules>
but more sensibly the peripheral has access to everything it needs to calculate that
<whitequark[cis]>
jfng[m]: or you could reconnect them in the pin mux
<tpw_rules>
testing it could be difficult
<whitequark[cis]>
remember, the GPIO peripheral does not connect to pins directly
<jfng[m]>
my concern with the set/clear layout mostly pertains to ergonomics for handwritten asm, and possibly the complexity (in terms of say, CPU insns) to implement accesses
<whitequark[cis]>
how much handwritten asm exactly are we expecting
<whitequark[cis]>
tpw_rules: our methodology for defining address maps is top-down, not bottom-up; this isn't about what the implementation can do but what we should do
<jfng[m]>
write value to r0; write set mask to a r1; shift r1 left by N bits; or r1 with clear mask
<jfng[m]>
and store to the CSR, etc
<whitequark[cis]>
also, I'd like to introduce identifications registers for each peripheral we ship, in the high part of its address space
<whitequark[cis]>
that you cannot enable without giving explicit addr_width
<tpw_rules>
whitequark[cis]: how do we define acceptable values for addr_width? enough that it doesn't die?
<jfng[m]>
jfng[m]: come to think of it, interleaving is *not* that much more complicated actually..
<whitequark[cis]>
tpw_rules: this is a good question, and we probably need a class level function for that (I mentioned that briefly during the previous discussion)
<whitequark[cis]>
jfng[m]: interleaving is the same computationally as manipulating the mode fields in asm
<jfng[m]>
write set mask to r1; shift r1 left by 1 bit; or r1 with clear mask
<jfng[m]>
yeah
<whitequark[cis]>
so if you can switch input to output and back for I2C on STM32, you can use SetClr
<tpw_rules>
that would do the calculation and then you passed it in? i think i missed that. that could be a good solution but letting it be None seems pretty similar; then you could check what it computed by reading the signature attributes
<tpw_rules>
s/passed/pass the result/
<tpw_rules>
but in any case determining the addr_width by trial and error is not acceptable to me. we can come back to that
<jfng[m]>
whitequark[cis]: yes, that idea came up
<jfng[m]>
i am also not satisfied with the current status quo wrt. addr_width
<tpw_rules>
with regards to the ALTERNATE mode, this is a non sequitur to me: "When pin mode is set to ALTERNATE, the meaning of i, o, and oe fields for a pin would be implementation-defined rather than defined by the GPIO peripheral itself." are we talking about the pin as in the whole SoC? i'm confused why that is being introduced if so. the only pin in the RFC is `amaranth_soc.gpio.PinSignature`, and those fields (should) cease to have
<tpw_rules>
meaning in ALTERNATE mode
<jfng[m]>
as said previously, it is a consequence of having an immutable addr_width in a memory map; that cannot grow as resources are added to it
<whitequark[cis]>
tpw_rules: I just wrote another comment that is clearer
<whitequark[cis]>
> In my proposal, selecting ALTERNATE mode delegates the meaning of bits in Input and Output registers to the wider SoC implementation, or perhaps a custom derived implementation/wrapper of this GPIO register. Nothing is guaranteed about them at all. The pin multiplexer could choose to wire inputs to the pins, or disable the buffers entirely, or something else completely.
<tpw_rules>
i see, thank you
<whitequark[cis]>
re CSR padding / in-register padding: honestly good question, I think it's just unanswered right now
<tpw_rules>
i'm not sure my understanding of granularity is complete. afaict it only seems to be used to scale offset values.
<whitequark[cis]>
should probably not be a part of this RFC though
<tpw_rules>
i don't think it should be either, but i wonder if an RFC to define that and the docs should be put in before this one then, so we don't have to change it later
<jfng[m]>
and we don't pad CSRs
<jfng[m]>
we have an 8-bit CSR bus bridged to a 32-bit WB bus
<jfng[m]>
it would be desirable to allow said CSRs to be aligned on 32-bit boundaries
<Wanda[cis]>
Catherine: (I think the issue also needs the needs-rfc label)
<tpw_rules>
jfng[m]: did you read my comments on the issue? i think "pad" is an ambiguous term fwiw
<jfng[m]>
no, i haven't yet actually
<tpw_rules>
ok. do you still have more to say about the 24 pin example?
<jfng[m]>
> Do we round up registers to power-of-2 (multiple of data_width, which need not be a power of 2) sizes? If yes, then there's excessive latency from narrow busses.
<jfng[m]>
> It appears this is the behavior since csr.RegisterMap became csr.Bridge, however, this aspect was not a part of the RFC since this happened after that was decided to be merged.
<jfng[m]>
I was going to bring this up, indeed
<jfng[m]>
the rationale was that rounding to the next po2 in csr.RegisterMap would be a reasonable default that would avoid us from having to deal with the `alignment` parameter of a memory map
<jfng[m]>
we should probably expose it to csr.RegisterMap, and gpio.Peripheral in turn
<tpw_rules>
you mean csr.Bridge here?
<jfng[m]>
no, RegisterMap
<tpw_rules>
idk why i said Bridge
<jfng[m]>
the bridge is created from a MemoryMap, that is produced by the RegisterMap
<tpw_rules>
i meant Builder in both cases
<jfng[m]>
ahh
<tpw_rules>
RegisterMap is dead
<jfng[m]>
builder
<jfng[m]>
correct, sorry
<jfng[m]>
yeah
<tpw_rules>
fixed my comment
<tpw_rules>
ok, i think it's fine as a default but it currently can't be overridden, that should be changed. it also doesn't work correctly if indeed data_width is not a po2
<tpw_rules>
i don't like how it can cause additional latency due to the quirk mentioned in the next part of my comment
<tpw_rules>
(but then there's sort of a third type of hole, which is not great)
<jfng[m]>
i will reply in depth once i'm done eating, but i don't see a use-case for a non-po2 data_width
<whitequark[cis]>
neither wishbone nor axi support it
<jfng[m]>
csr.Builder uses a granularity because although humans tend to prefer 8-bit bytes, but amaranth-soc can deal with any byte size
<jfng[m]>
s/but//
<tpw_rules>
pdp-8 enjoyers? i'm not sure i see such either, but if not then it should be prohibited explicitly by the CSR machinery. idk if it would ever be useful for something like ECC though
<cr1901>
wb has tag bits for ECC stuff
<jfng[m]>
tpw_rules: what kind of CSR breakage are you thinking of ?
<tpw_rules>
i guess also granularity confuses me a little because the addresses are not byte-sized
<galibert[m]>
CycloneV's m10k supports an amusing number of "bytes" sizes
<tpw_rules>
jfng[m]: i don't understand?
<cr1901>
(although last I asked amaranth-soc doesn't support WB tags and Idk if jfng[m] plans to support them)
<jfng[m]>
tpw_rules: in what scenarios would a csr.Builder with granularity != 8 break stuff ?
<galibert[m]>
5, 8, 9 and 10
<jfng[m]>
(the gpio peripheral will not allow granularities different than 8)
<tpw_rules>
jfng[m]: i never asserted that it did? i just am confused how granularity doesn't seem to matter except for the offset passed to add. idk if it affects the docs, it doesn't affect addr_width, it doesn't affect addr
<tpw_rules>
why does the gpio peripheral need to assert on the granularity?
<jfng[m]>
tpw_rules: it doesn't; it will only use a Builder with its default granularity, 8
<jfng[m]>
because MemoryMap has addresses in units of its `data_width`
<tpw_rules>
yes
<jfng[m]>
and `data_width` is a propery of the CSR bus
<jfng[m]>
so if you have a CSR bus with a 32-bit data_width, you still want to have addresses that are in units of 8-bits, right ?
<jfng[m]>
for documentation purposes, etc
<jfng[m]>
so you use a csr.Builder with a data_width of 32, and a granularity of 8
<tpw_rules>
i don't know, to be honest. i'd like them but the fact that it's not the same as what you pass into the addr port concerns me a bit
<galibert[m]>
unless your cpu only addresses 32-bits values
<tpw_rules>
i see also that wishbone does have variable granularity. axi does not
<jfng[m]>
CSR granularity is not Wishbone granularity
<jfng[m]>
it is a bus-specific concept, to me
<tpw_rules>
i agree, just saying that there's precdent
<jfng[m]>
tpw_rules: that is also why we use the keyword `offset=` when adding registers to a Builder
<tpw_rules>
i guess i disagree that the granularity should be able to not match the address bus. i.e. if you have a data width of 32 and granularity of 8, there should be two extra always zero bits on the low end. so your documentation and your peripheral match up
<tpw_rules>
and your offset and your values you get back from the memory map and etc
<tpw_rules>
(this does raise the question of what happens if they're not zero, but then maybe granularity should be a property of the generated docs instead of the builder/register?)
<tpw_rules>
galibert[m] is right, good old Boneless has a 16 bit byte so i could not use it with this peripheral, or at least not generate accurate docs
<jfng[m]>
if the peripheral designer and the firmware developper have a different interpretation of an address, it is not ideal
<jfng[m]>
by defaulting to 8-bit bytes, we allow both to use the same units
<tpw_rules>
i don't think this is true though? does the MemoryMap take into account the granularity or not?
<tpw_rules>
(this also leaves out the FPGA developer)
<jfng[m]>
it doesn't
<tpw_rules>
ok. well the granularity issue is not a main concern for me but it's good to know this stuff, and i do suspect there's room for adjustment. it's completely unrelated to the GPIO peripheral though
<jfng[m]>
yeah
<jfng[m]>
i expect that granularity will come up again when designing the BSP generator
<jfng[m]>
i do, however, think that adding an `alignment` parameter to csr.Builder and gpio.Peripheral will remove the need for rounding-up register sizes
<tpw_rules>
(last whine: csr.Decoder.add again doesn't take into account the granularity. i really think now that granularity needs to be a property of the generation of something rather than the register/memory map)
<tpw_rules>
(or FPGA logic at all)
<galibert[m]>
csr is just one detail in supporting generating documentation for non-byte-addressing initiators in any case. But it's important to remember that not everything is a riscv
<whitequark[cis]>
Wanda: huh, python has a `graphlib`
<whitequark[cis]>
it can only do toposort
<tpw_rules>
yall are uncovering all sorts of stuff from python's stdlib
<Wanda[cis]>
oh what
<Wanda[cis]>
wtf.
<tpw_rules>
i had never heard of bisect before
<whitequark[cis]>
tpw_rules: this was added in 3.9
<tpw_rules>
jfng[m]: what about the introduction of additional latency?
<jfng[m]>
tpw_rules: we use it in MemoryMap (i guess that's where you stumbled upon it)
<tpw_rules>
yes
<jfng[m]>
tpw_rules: access latency is influenced by the alignment parameter
<tpw_rules>
jfng[m]: oh, i see what you mean. "Are non-power-of-two data_width sizes intended to be supported in general? A quick glance shows this might break some stuff.". there's several places which just do ceil_log2 etc. which would break. automatic calculation of the CSR alignment for one
<jfng[m]>
e.g. if you have a data_width of 8 and alignment of 2, then a transaction on a 24-bit CSR will require 4 accesses
<jfng[m]>
if alignment was 0, it would require only 3
<tpw_rules>
yes, i recognize that. that seems to be an implementation quirk though, should it be separated?
<Wanda[cis]>
... and I just learned about bisect as well
<tpw_rules>
(ugh what about endianness actually :( )
<whitequark[cis]>
bisect?
<jfng[m]>
tpw_rules: i think this is a useful parameter to expose; it gives control to users over the latency of CSR accesses
<tpw_rules>
jfng[m]: i think that increasing the alignment should not increase the latency is what i'm saying. if you want to align that's good, but the fact that it virtually increases the register width and latency is unfortunate and could be changed i think
<jfng[m]>
e.g. you could have some registers of variable sizes, but all are <32; and from the POV of a driver, they all have the same width and latency
<jfng[m]>
also, this information cannot be decided by the peripheral designer, even
<jfng[m]>
only the soc designer, which has knowledge of the interconnect, cpu cores, etc, can make this judgement call
<tpw_rules>
yes, i think it's good to expose alignment, i just don't think it needs to be connected to latency
<jfng[m]>
tpw_rules: i forgot the details and would need to regain familiarity with the matter, but iirc there is a good reason for coupling alignment and CSR latency
<tpw_rules>
ok. i think the documentation on that is not quite right fwiw
<tpw_rules>
i link it in my comment
<tpw_rules>
(it refers to a memory map alignment when each resource can have an individual alignment)
<jfng[m]>
the MemoryMap documentation is very complicated to read and should be significantly rewritten
<jfng[m]>
as in, needlessly complex
<tpw_rules>
ok, it would be good to document that reason
<tpw_rules>
it seemed to mostly be an implementation detail to me
<tpw_rules>
for the GPIO peripheral, are all registers going to be the same width?
<jfng[m]>
Mode and SetClr would be `2 * pin_count` bits
<jfng[m]>
Input and Output would be `pin_count` bits
<tpw_rules>
ok. this is not how the diagrams in the document look now
<jfng[m]>
yes, they haven't yet been updated based on the comments in this issue thread
<tpw_rules>
ok
<tpw_rules>
(how did you create the simplified diagram btw?)