<zyp[m]>
<whitequark[cis]> "everyone: RFC 27 (https://github..." <- I'm a bit iffy on removing the domain argument, I'd prefer to not have to pass the domain explicitly into every simulation helper function, I propose renaming it to `default_domain` or something instead
<anuejn>
while we are at it, could we deprecate adding memory Read & Write ports as a submodule?
<zyp[m]>
and I might also add a with sim.default_domain(): context manager to RFC 36
<whitequark[cis]>
zyp: but we're killing `yield` by replacing it with `async`... so does it really matter?
<anuejn>
it seems a bit cursed to have both work but produce different code
<whitequark[cis]>
anuejn: they produce equivalent code (just with different names)
<anuejn>
ah huh interesting okay
<whitequark[cis]>
but yes, we should move to having only memory be an elaboratable, not the ports
<anuejn>
agreed
<whitequark[cis]>
zyp: in general i think i want to have less global or global-ish state, not more
<whitequark[cis]>
i am pretty sure i do not want a context manager
<anuejn>
do you think this RFC would be the right place to deprecate it?
<whitequark[cis]>
zyp: we repeat the domain all the time in the code, it seems fine to do the same in the testbench
<whitequark[cis]>
and people do get confused about yield / yield Tick() / yield Tick("sync") esp with multiclock designs
<whitequark[cis]>
anuejn: yes definitely, it is an oversight that it's not explicitly deprecated
<anuejn>
okay, nice :)
<zyp[m]>
I disagree, in the code, the domain is usually implicit through DomainsRenamer unless writing a module with multiple domains
<whitequark[cis]>
anuejn: actually, reading the text closely, i think it is: ReadPort/WritePort are interface objects, not elaboratables
<whitequark[cis]>
so the old behavior dies with amaranth.hdl.Memory itself
<whitequark[cis]>
zyp: you literally write `m.d.sync +=` each time
<anuejn>
okay, thats also good but I think it would help to have it explicitly stated
<anuejn>
I wasnt sure while reading :D
<zyp[m]>
whitequark[cis]: yeah, regardless of which domain it actually ends up being
<whitequark[cis]>
zyp: actually, i think i should clarify one other bit: i propose that in add_sync_process, you can *only* yield, not yield Tick
<whitequark[cis]>
zyp: there is nothing special about "sync"
<whitequark[cis]>
it is a domain like any other
<zyp[m]>
I know
<whitequark[cis]>
oh, is your point essentially that there is no equivalent of DomainRenamer for testbenches?
<zyp[m]>
but convention makes it the default domain for modules that can later be instanced into different domains
<zyp[m]>
yes
<whitequark[cis]>
that is a good point
<whitequark[cis]>
I think I'm maybe fine with just having it as an argument everywhere?
<whitequark[cis]>
that would be inconsistent with the RTL DSL, that is true
<zyp[m]>
I find it preferable to not have to think about domains except when you've got multiple
<whitequark[cis]>
I think I would not object to amending RFC 27 as follows: yield Tick() is deprecated from add_sync_process together with yield Settle(); add_testbench gets a domain="sync" argument, in recognition of sync being the default; add_testbench does not accept bare yield; it does accept yield Tick() where no argument or None argument means the one from add_testbench is used
<whitequark[cis]>
I don't know if I accept yet that context manager but I don't want to block RFC 27 over that
<zyp[m]>
that would be a RFC 36 matter in any case
<whitequark[cis]>
yes, but if I do something in RFC 27 that is discongruous with your vision of simulation you'll probably object to it
<whitequark[cis]>
e.g. introduce a domain argument but without a default
<zyp[m]>
from an RFC 36 perspective, there's no bare yield equivalent, await sim.tick() is equivalent to yield Tick()
<whitequark[cis]>
I guess one objection I have is that default_domain context manager is strictly worse than DomainRenamer
<whitequark[cis]>
it doesn't let you rename non-sync. so I think that context manager is not a good solution
<whitequark[cis]>
actually, I think I have a better proposal
<whitequark[cis]>
RFC 27 remains as-is and is just a vehicle to introduce the settling semantics of testbench processes
<whitequark[cis]>
and we add a domain argument to add_testbench() in RFC 36 or do some other coherent solution
<whitequark[cis]>
there are good questions about domains that are unanswered. it should be some sort of binding environment where you can rebind things that is populated from the DUT
<whitequark[cis]>
but it's not really clear exactly how
<whitequark[cis]>
I think people will want to discuss that in depth and I think I want to do this separately from RFC 27
<zyp[m]>
that works for me, I'm happy to get RFC 27 through quickly
<whitequark[cis]>
thank you
<whitequark[cis]>
updated to include your feedback
<jfng[m]>
ah, indeed; the guide should mention it too
<whitequark[cis]>
I have a single remaining concern about the proposed API, which is that RegisterMap, which is a builder class that has implicit internal state, also tries to be an introspectable container. this is a bad idea because it invites all sorts of corner cases, such as: what exactly is the result of flattening a RegisterMap while you are constructing it?
<galibert[m]>
There was the question of the array of ports and the arrays in ports iirc?
<galibert[m]>
And the arrays of ports... structures? Organizations?
<whitequark[cis]>
I believe that RegisterMap should only expose add, Cluster, Index (the builder methods) and some single way to consume it and turn it into something concrete. I think having it be called RegisterMap was a bad idea for a builder style class and invited this conflation of purpose
<jfng[m]>
jfng[m]: also, `RegisterMap` has been redesigned to support more complex layouts, such as arrays of registers, or even arrays of clusters of registers
<whitequark[cis]>
I think I'm OK with having it not even have any other methods, have it be consumed by csr.Bridge, and have the API essentially require creating and assigning self._bridge = csr.Bridge(regs) in the constructor
<jfng[m]>
whitequark[cis]: agreed, except for testability concerns
<jfng[m]>
i think we should keep at least one iteration mechanism
<whitequark[cis]>
you don't need that to be public
<whitequark[cis]>
the Module builder class in Amaranth itself provides no introspection and never will by design. instead it can be lowered into Fragment. (Fragment was intended to be public API but will not for unrelated reasons)
<whitequark[cis]>
I think having iteration is problematic for the exact reason I've mentioned, which is that mutating state at the same time as iterating state is fraught with edge cases
<galibert[m]>
and not so edge cases, it's always a mess
<whitequark[cis]>
I don't think there is any reason to have testability, which is something internal to Amaranth SoC, require a public method on the builder class
<whitequark[cis]>
I think having an iterator on the CSR bridge is fine, or at least I can't think of any reason not to
<whitequark[cis]>
this will solve downstream user testability, and a private method that Amaranth SoC itself can use to not couple to the bridge should solve internal testability
<jfng[m]>
yeah, for now, the sole purpose of a RegisterMap is to be converted to a MemoryMap and passed to a csr.Bridge
<galibert[m]>
So, a RegisterRegistry in fact?
<whitequark[cis]>
RegisterBuilder
<galibert[m]>
Builder sounds good
<whitequark[cis]>
it's the builder pattern
<jfng[m]>
as for iterating an incomplete RegisterMap; it is of course unsound (as its contents cannot be depended upon)
<whitequark[cis]>
RegisterMapBuilder is long and also we don't actually have a RegisterMap
<jfng[m]>
so i agree with making `.flatten()` a private method (i.e `._flatten()`)
<jfng[m]>
`RegisterBuilder` is weird: it does not build registers
<jfng[m]>
they are created before being passed to `.add()`
<jfng[m]>
RegisterMap at least conveys the fact that we are assigning names to registers
<whitequark[cis]>
BridgeBuilder?
<jfng[m]>
..ok for `RegisterBuilder`, it is the better suggestion so far
Chips4MakersakaS has joined #amaranth-lang
<Chips4MakersakaS>
CSRBuilder ?
<jfng[m]>
`csr.RegisterBuilder` is a bit of a pleonasm
<Chips4MakersakaS>
Just csr.Builder ?
<jfng[m]>
hmm
<jfng[m]>
not bad
<jfng[m]>
i'm fine with that one
<whitequark[cis]>
fine with csr.Builder
<jfng[m]>
any other question or comments ? (i'd be interested in hearing from @vegard_e:matrix.org @libera_tpw_rules:catircservices.org and @libera_cr1901:catircservices.org also!)
<galibert[m]>
Arrays, any conclusion?
<jfng[m]>
i added an example for 2D array in the RFC
<jfng[m]>
the idea is that arrays (and clusters, fwiw) don't really materialize as Amaranth SoC data structures
<jfng[m]>
the RFC is only concerned about providing a way to declare them
<galibert[m]>
The builder does not get the index information, that’s going to make automatic generation of headers problematic no?
<whitequark[cis]>
I feel like 2D arrays will be incomprehensibly painful to export to SVD and stuff, but I have no objection to it since it fits into the model as they stand
<jfng[m]>
collecting informations about clusters and arrays is punted to a later date, as part of our work on the BSP generator
<galibert[m]>
That means breaking change though. Hard to collect information that’s not there
<jfng[m]>
so, as of right now, that info does not make it to the MemoryMap, no
<jfng[m]>
<galibert[m]> "The builder does not get the..." <- actually, what do you mean by "the builder does not get the index information" ?
<jfng[m]>
(assuming that by builder you mean `csr.Builder`, the new name we chose for `RegisterMap` ?)
<whitequark[cis]>
galibert: the design that I proposed materializes clusters and arrays by looking at the nesting of the names and comparing the actual types of the things that are inside the array
<whitequark[cis]>
if the array is "well formed" i.e. matches SVD expectations for an array element, which is done by deep comparison, it is turned into an SVD array
<whitequark[cis]>
if it is not (for example if it is heterogeneous), it is turned into a collection of registers with a _%d suffix or something, whose displayName is [%d] to make it look like an array
<whitequark[cis]>
for clusters, I don't think there's any issue, as we plan to pass the hierarchical name to the MemoryMap where it would be accessible to the BSP generator
<galibert[m]>
The example just allocates core*whatever register pairs in a row, and never tells anything which core or whatever it’s allocating
<whitequark[cis]>
oh, that part will be in the Peripheral API, which is a future RFC
<whitequark[cis]>
can't do everything at the same time
<jfng[m]>
whitequark[cis]: we could put a parameter in the SVD generator to enable a "strict" mode that would error out rather than try to workaround unsupported layouts
<jfng[m]>
but that's in the future
<whitequark[cis]>
yeah
<galibert[m]>
I just mean that when it’s time to add the information it will be a code breaking change
<galibert[m]>
And it’s a certainty that the information will be needed at some point
<whitequark[cis]>
you should expect breaking changes in a 0.1 version library
<galibert[m]>
If you’re ok with that, I’m ok with that
<jfng[m]>
i'm not sure that restricting access to some registers to some cores only would introduce breaking changes to this API
<jfng[m]>
it would maybe add additional metadata to indicate such information
<galibert[m]>
It's not about restricting anything
<galibert[m]>
It's just that in self.ie[i][j] = regs.add("IE", self.IE(width=32)) regs does not get any information about i or j and you'll need it at some point that probably isn't now but fairly certainly will exist in the future
<whitequark[cis]>
the information is provided in with regs.Index
<galibert[m]>
But that's ok
<galibert[m]>
Ohhhhhhhhhhhhhh I missed it, sorry
<galibert[m]>
That's it's 100% good
<galibert[m]>
* Then it's 100% good
<jfng[m]>
`i` or `j` are indeed used to assign the name (rather the "path") of those registers
<galibert[m]>
(that will teach me about looking at the example on my phone)
<zyp[m]>
sorry, I was out today, I'm gonna read through the backlog after I've put the kid to bed
<zyp[m]>
okay, now I'm caught up, the last changes sounds reasonable to me and FWIW my vote would also have been to merge with them
<zyp[m]>
I also have one question that I don't found an answer for in the RFC text
<zyp[m]>
for RW1C and RW1S, what is the priority between port.w_data and clear/set if both are asserted in the same cycle?
notgull has joined #amaranth-lang
notgull has quit [Ping timeout: 252 seconds]
cyrozap has quit [Quit: ZNC 1.8.2+deb3.1 - https://znc.in]
cyrozap has joined #amaranth-lang
notgull has joined #amaranth-lang
notgull has quit [Ping timeout: 256 seconds]
Wanda[cis] has joined #amaranth-lang
<Wanda[cis]>
... why is Signal(range(1)) 1-bit
<Wanda[cis]>
oh gods.
<Wanda[cis]>
it uses bits_for, which has the special case for 0, to implement Const(0)
Guest89 has joined #amaranth-lang
<Wanda[cis]>
Catherine: ... do we actually want `bits_for` as a public API? this certainly feels like a trap
<Wanda[cis]>
(I am implementing the MemoryInstance part of RFC 45 and I wrote assert len(port._addr) == ceil_log2(self._depth), which just blew up in my face on a depth=1 memory)