<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 755930a - Deploying to main from @ amaranth-lang/amaranth@7e291a26e380cedc5ace7f1b875e145b78f37d60 🚀
<whitequark[cis]>
Wanda: do you think we can define iteration on data.Const? this would be iterating through an array for ArrayLayout, iterating through a dict for StructLayout, and forbidden for any other layout. I think that could be quite helpful in tests since then you'd be able to easily convert a const struct to a dict for comparison
<whitequark[cis]>
alternatively, define equality on data.Const, with the same purpose. alternatively, define both
<whitequark[cis]>
I encountered this need while working on Open Beam Interface, where I was writing tests for streams
<whitequark[cis]>
zyp: do you think you could address feedback on RFC 36 before the Monday meeting? I put it on the agenda in the hopes that you can, but if you wouldn't be able to I should probably take it off
<whitequark[cis]>
anyone else reading it, should read the comments in addition to the RFC text itself at least
<zyp[m]>
<whitequark[cis]> "zyp: do you think you could..." <- yeah, I'll try to get it done tonight or tomorrow
notgull has joined #amaranth-lang
mwk has quit [Ping timeout: 252 seconds]
mwk has joined #amaranth-lang
<Wanda[cis]>
<whitequark[cis]> "alternatively, define equality..." <- we already have equality, but only to other `data.Const`; would you want to beef it up to compare with lists/dict?
<Wanda[cis]>
I can definitely add iteration, that sounds useful
<whitequark[cis]>
<Wanda[cis]> "we already have equality, but..." <- yeah I think so
<whitequark[cis]>
<Wanda[cis]> "I can definitely add iteration..." <- thinking again of it, the keys are just fed to `__getitem__`, so no type restriction is needed
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 1040d1e - Deploying to main from @ amaranth-lang/amaranth@715a8d4934c64b4c1a312b4e6188cd1b7d773a80 🚀
<cr1901_>
jfng[m]: I'm not feeling well today, I doubt my opinion on the GPIO peripheral ("it's fine") will change dramatically, so I'll catch up on the meeting later when I'm feeling better.
<jfng[m]>
hi everyone! it is time for our weekly SoC meeting
<jfng[m]>
how about @vegard_e:matrix.org @galibert:matrix.org @fatsiefs:matrix.org ?
<jfng[m]>
anyway, we can resume discussion
<jfng[m]>
@whitequark:matrix.org
<jfng[m]>
> The entire register is always accessed simultaneously. In general, on a 32-bit MCU, I will recommend using a 32-bit CSR bus unless otherwise needed. In this case every access is guaranteed to be 1-cycle, 0-latency. Otherwise the bridge will take 4 cycles to do a register write, which is rarely worth it if you can spend the area.
<jfng[m]>
This won't work with CSR bridged over WB, if the wishbone has a granularity of 8 bits
<whitequark[cis]>
I believe galibert is abstaining as the peripheral doesn't support input-only/output-only pins specially
<tpw_rules>
should there be a latch on o and oe after the mode switch logic?
<jfng[m]>
because the `WishboneCSRBridge` uses the CSR bus data width as WB granularity
<jfng[m]>
and `wishbone.Decoder` disallows subordinates with a greater granularity than the initiator bus
<whitequark[cis]>
but it's common for peripherals to simply ignore accesses with the incorrect alignment or width, so this is what we could do
<jfng[m]>
however, i agree with your observation, and i think that this problem needs to be addressed, but outside of this RFC
<whitequark[cis]>
we emphasize the importance of using generated code to access peripherals, so this shouldn't be a practical problem for almost anyone
<jfng[m]>
jfng[m]: (i.e. a 32-bit CSR bus should be viable)
<whitequark[cis]>
tpw_rules: what kind of latch? I don't understand what's proposed
<tpw_rules>
whitequark[cis]: i think i meant register, sorry. but have the o and oe pins driven directly from a register to avoid glitches and improve timing
<tpw_rules>
or d flip flop
<whitequark[cis]>
that doesn't seem particularly important to me--we designed the peripheral in such a way that you shouldn't be doing routine mode switches
<whitequark[cis]>
meanwhile an extra cycle of latency on the output bites
<tpw_rules>
okay. i think the highest possibility for problems would be the open drain mode but i don't think it's a major concern
<jfng[m]>
but users can still put registers after `o` and `oe` if they wished, since we don't implement the actual I/O buffer
<whitequark[cis]>
if you have a bidirectional bus, a short glitch on OE doesn't really do anything
<whitequark[cis]>
you usually need a single cycle of turnaround where the bus state is undefined
<tpw_rules>
yes, sounds good
<whitequark[cis]>
so what if the output transistor is enabled for a few hundred ps there?
<whitequark[cis]>
you probably won't even be able to measure it
<tpw_rules>
if latency is a thing that bites, should it be defined in the RFC?
<whitequark[cis]>
the RFC does define it by the schematic
<jfng[m]>
it, kind of is
<tpw_rules>
okay, i agree there
<jfng[m]>
any other comments ?
<tpw_rules>
i had another couple concerns on the bus side. first is the csr.Builder granularity, it being fixed to 8 would mean i couldn't generate correct docs for e.g. Boneless. but i think the granularity needs to be taken out of the registers themselves
<tpw_rules>
also the issue of calculating addr_width. that could easily be deferred to a future RFC but i'd prefer not to
<tpw_rules>
(also not sure how that would interact with the backwards compatibility policy)
<jfng[m]>
one thing we could do is expose the `granularity` parameter (used by `csr.Builder`) on the `gpio.Peripheral` constructor
<whitequark[cis]>
that's so many parameters though
<whitequark[cis]>
I guess a future "peripheral API" RFC will clean that up
<jfng[m]>
and say that for a given `data_width`,`granularity` and `pin_count` combination, the register addresses should be constant
<tpw_rules>
i imagine it would be better for the future to keep it un-exposed. but like i said i think it needs to be removed entirely
<whitequark[cis]>
amaranth soc doesn't have any backward compatibility concerns at the moment because there's no code using amaranth-soc peripherals
<whitequark[cis]>
it also doesn't need to commit to backwards compatibility
<jfng[m]>
but if possible in the future, i'd like to go in the opposite direction and simplify this constructor by removing `addr_width`
<tpw_rules>
whitequark[cis]: i meant your comment about backwards compatible extensions. but that is listed under unresolved questions for now
<jfng[m]>
so maybe we can just leave non-8-bit granularity in the "unresolved questions" of the RFC
<whitequark[cis]>
the way I see it is that we'll gain some sort of SoC-wide "peripheral configuration" object that says how big peripheral memory blocks are, what is the bus size, whether there are ID registers at the end of the address space, etc
<whitequark[cis]>
and you'd just pass that, instead of fiddling with 50 little separate parameters
<whitequark[cis]>
but we don't have to add that right now
<jfng[m]>
that would already be much more convenient, and i think that trying to solve this in this RFC is premature
<tpw_rules>
i agree
<jfng[m]>
we'd arrive to a frustrating solution that wouldn't satisfy all use cases anyway
<tpw_rules>
(that particular idea seems slightly over constraining but it's not for discussion now)
<tpw_rules>
i think all my remaining concerns are with the CSR system as a whole and don't affect this peripheral, especially with the revised register diagrams
<tpw_rules>
(random note: is there a particular reason for unsigned(n) in the RFC)
<jfng[m]>
not really besides that this is implicitly what happens if you give `n` as a shape, iirc
<whitequark[cis]>
it is, yes
<tpw_rules>
yes, so it seems unnecessary to give it explicitly. just more stuff to parse for my brain
<whitequark[cis]>
I think that's more a matter of taste--I mildly prefer seeing it explicitly, though not enough to actually do that
<whitequark[cis]>
(because unsigned(1) is unambiguously a Shape and 1 is just an int)
<tpw_rules>
yes, fair
<tpw_rules>
okay, i don't have any other input. i think it looks good. attendance seems very low today, not sure if we want to vote
<jfng[m]>
if there are no further questions/comments, i guess we can vote
<whitequark[cis]>
merge
<tpw_rules>
i vote merge
<jfng[m]>
assuming @libera_cr1901_:catircservices.org 's vote is merge, and @galibert:matrix.org remains abstain
<galibert[m]>
Three quarters of the I/o on my platforms are defined unidirectional on the specific board they are, I don’t see the point of a gpio peripheral that ignores them
<whitequark[cis]>
it remains beyond me why you insist on using a GPIO peripheral with non-general-purpose-IO
<galibert[m]>
Because it’s what the very first example of the rfc and your go to example are, leds. They’re output only
<whitequark[cis]>
have you ever used a microcontroller?
<galibert[m]>
Anyway, abstain
<jfng[m]>
i mean, the example could have requested a GPIO header resource or something, but blinking leds has the benefit of being straightforward
<whitequark[cis]>
I think the example should be made more complicated and harder to understand just to address galibert's concern
<tpw_rules>
i have to head out, i'd like to discuss the other stuff surrounding CSR sometime soon
<tpw_rules>
it could even be RFC-worthy
<jfng[m]>
right, let's conclude
<jfng[m]>
merge: Catherine, tpw_rules, cr1901_
<jfng[m]>
abstain: galibert
<jfng[m]>
the final decision is merge
<jfng[m]>
thanks everyone! that's our first peripheral
<tpw_rules>
yay!
<jfng[m]>
tpw_rules: looking forward to your thoughts on this
<galibert[m]>
Catherine: I think, but I can be wrong, that our fundamental disagreement is that you consider presenting the fpga capabilities is what is important, while I consider that what should be presented is the semantics as defined by the environment the fpga is implanted it. E.g. if you solder a led to a pin, its semantics become output only. And I consider that to avoid mistakes the interface presented to the firmware should enforce
<galibert[m]>
the semantics (e.g. no DDR or equivalent to a monodirectional pin for instance)
<whitequark[cis]>
I'm actually entirely in agreement that if you have, say, a LED, it should be connected to a simple RW register with one bit for the entire thing
<galibert[m]>
(was out shopping, talking on the phone while walking is nontrivial)
<whitequark[cis]>
for your case of HPS, one register for outputs and one register for inputs is what to me sounds like a good solution
<whitequark[cis]>
though I don't know the details about the HPS module, so it may be more intricate than that
<whitequark[cis]>
which is the solution where the shape of the peripheral itself prevents any possible misuse; you can't even express it, much less perform it
<galibert[m]>
Catherine: for sure, and that's what I consider a generic gpio should optimize to when presented with unidirectional lines
<whitequark[cis]>
the GPIO peripheral is useful for ASIC emulation (an important use case I'm thinking of that most FPGA-focused people miss), or a PMOD connected to an FPGA, or just an extension header, or things like that
<whitequark[cis]>
the GPIO peripheral is not useful for LEDs, the HPS case, or things like that, because they are not general purpose IO
<galibert[m]>
in fact, I suspect the gpio class should be used for extension headers or equivalents only
<whitequark[cis]>
having the firmware be able to express an incorrect mode setting and it just be ignored, is basically as bad as the firmware being able to express an incorrect mode setting and it be seen on the pins of the device
<whitequark[cis]>
in both cases, the shape of the peripheral has enabled a firmware writer to make a mistake, which may be costly in terms of debugging time
<whitequark[cis]>
the LED example, morally, should have been something like "now hook up your oscilloscope probe / logic analyzer / signal generator to this pin and observe the changes"
<whitequark[cis]>
however we are targeting FPGA people who have no tools and no money and the only thing their cheap devboard has is LEDs and buttons (sometimes even not those)
<galibert[m]>
which is why I initially thought the class was auto-dropping the direction registers when they were not needed or fixing some bits when mixed. With the idea that you could then unify all the miscellaneous I/O under a unique format that could be picked up by the memory map description tools
<whitequark[cis]>
any register you make is picked up by the tooling generating headers
<whitequark[cis]>
you can make a single peripheral (or even just create a CSR bridge directly) and a single LED register and you'll have something like `LEDS0->on = LEDS0_LED_0;` at your disposal
<whitequark[cis]>
there isn't anything we're going to be doing specially for the GPIO peripheral that's about pins specifically. it has those registers exposed in the same way as your trivial LED peripheral would have them exposed
<whitequark[cis]>
I don't think anyone ever planned to add pin capability, pin mux, or the like data aggregation and output generation code to Amaranth SoC. at least, if they did, they never talked about it anywhere I could hear it
<whitequark[cis]>
the utility is obvious but it's questionable whether we even can. I'm not aware of any industry prior art that isn't deeply tailored for the chip in general (I'm ignoring debug-only solutions here like the one in STM32Cube)
<whitequark[cis]>
in any case, this isn't something that's even on the roadmap
<galibert[m]>
pins/bits names for documentation would be an interesting evolution though
<galibert[m]>
but integrated documentation is hard to do sanely
<whitequark[cis]>
I don't know how much utility that has. I suppose for a fixed function product that ships with an FPGA that has a user-accessible CPU core inside (like an SDR or something) that would be useful
<whitequark[cis]>
it's pretty niche though
<galibert[m]>
Dunno, amaranth has the potential to make lego-building of stuff on a fpga a lot more palatable than verilog ever will
<whitequark[cis]>
for an ASIC you have a separate pin map anyway, since you probably want multiple packages, and for most FPGA workloads you want to use the resource system directly instead of going thru GPIO
<galibert[m]>
I was more thinking about the upper csr level at that point
<galibert[m]>
and I was thinking that if you lego-build a bunch of stuff on a wishbone bus it can be nice to have the map dumped with full documentation even for yourself
<whitequark[cis]>
oh yeah, that's a clear goal
<whitequark[cis]>
there isn't any significant difference between that and building docs for an ASIC anyway
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] whitequark 83bb172 - Deploying to main from @ amaranth-lang/rfcs@c1573d9767eb8492f6611b61b0616f59b9dad6f4 🚀
<jfng[m]>
let's see if those SVGs rendered properly..
<jfng[m]>
they did, nice
<whitequark[cis]>
they don't look that great on the dark backgrounds
antoinevg[m] has joined #amaranth-lang
<antoinevg[m]>
<whitequark[cis]> "amaranth soc doesn't have any..." <- **cough** -- though seriously, it's okay we're tracking upstream 😅
<whitequark[cis]>
note the "peripherals"--the GPIO peripheral is the first one
<whitequark[cis]>
so you couldn't have possibly used it yet!
<tpw_rules>
jfng[m]: so i think granularity should be removed entirely from csr.Builder
<tpw_rules>
i can see a slight concession in that it could make calculating addresses of peripherals with a variable data width easier if you do not want to use the auto layout. but i think its use is a little misleading and i don't want to influence docs and stuff down the line
Chips4MakersakaS has joined #amaranth-lang
<Chips4MakersakaS>
*too
<Chips4MakersakaS>
Missed the meeting sorry; to deep into other stuff. Anyway late merge from me on RFC49.
<whitequark[cis]>
thanks Staf!
<zyp[m]>
Catherine: going through your megacomment now (ended up printing it so I can cross out the parts I've addressed)
<whitequark[cis]>
oh dear
<zyp[m]>
thinking about the tick/reset stuff now and there's one seemingly obvious solution you've not mentioned; can't we just raise an exception if the awaited domain is being reset?
<Wanda[cis]>
I'm.. pretty sure that was mentioned
<zyp[m]>
(unless you count «crash all the waiting processes»)
<whitequark[cis]>
I think thats one of the strawman proposals yes
<whitequark[cis]>
yes that is what I meant by crash
<whitequark[cis]>
come to think of it, you could catch the exception
<zyp[m]>
exactly
<whitequark[cis]>
as a way to do control flow
<whitequark[cis]>
resets are fairly exceptional after all
<whitequark[cis]>
not a bad idea, and if it's too slow you could wait on ClockSignal() I guess
<zyp[m]>
it seems the most reasonable tradeoff, catch it and handle it if you expect it, let it propagate and crash the process if you don't
<tpw_rules>
are there hooks for memory in the platform?
<tpw_rules>
to create memory primitives
<zyp[m]>
Catherine: I really like your `async for a_val, b_val in sim.changed(a, b):` and it seems obvious enough to also allow `async for clk_val, rst_val in sim.posedge(clk).edge(rst, rst_edge):` in the third example
<tpw_rules>
idk if we would want to add some sort of "hint" to the Memory to do vendor-specific tweaks
<Wanda[cis]>
tpw_rules: it's literally a full hook on `Memory.elaborate` and replaces it completely if present
<tpw_rules>
okay, did not realize it was that new or capable
<tpw_rules>
bah i hate how the render link is broken in merged RFCs
<whitequark[cis]>
but they're merged! you can see them on the website...
<Wanda[cis]>
also wrt unhinged platforms and diff pairs
<tpw_rules>
what is "the usual way" for attrs on `amaranth.lib.memory.Memory`? being associated as verilog attrs on the module?
<Wanda[cis]>
Virtex E (the first xilinx fpga to support diff pairs) has an unusual property of not having fully fixed p-n pairs
<Wanda[cis]>
you have a diff input buffer between any two sequential pads
<Wanda[cis]>
ie. you can have pairings of PAD1-PAD2, PAD2-PAD3, PAD3-PAD4, ...
<galibert[m]>
funky
<whitequark[cis]>
right
<galibert[m]>
cyclone v goes by fixed packs of... 16 I think
<Wanda[cis]>
that's not actually exposed to the user, since for any given package they bless a particular set of this pairings
<Wanda[cis]>
s/ exposed/_exposed_/, s/this/these/
<galibert[m]>
which are themselves 4 groups of 4, and within a group the pairs are fixed
<Wanda[cis]>
and the reason for that is, I believe, retrofitting diff pairs to the Virtex architecture which originally wasn't designed with pairing in mind
<tpw_rules>
could a platform have the user add custom junk to the memory attrs and remove it from the hook
<Wanda[cis]>
this sometimes results in a differential pair being split across two IO tiles. since the IO logic has no native support for diff output, in such a case the pair is not usable for asynchronous differential output because you cannot really ensure the two pads switching at the exact same time
<Wanda[cis]>
(but it's fine to use it for sync diff output, thanks to clock distribution)
<whitequark[cis]>
huh
<Wanda[cis]>
this is all documented in the datasheet for every pair of every package.
<Wanda[cis]>
unhinged platform.
<Wanda[cis]>
tpw_rules: I'm not quite sure what you're asking here?
<tpw_rules>
Wanda[cis]: i'm thinking for the platform memory hook, some platforms might want to be able to specify attributes to influence how the memory is lowered. but they're not necessarily destined for the synthesis tool, just the hook
<galibert[m]>
hmmm, exapunks is getting complicated
<Wanda[cis]>
once again, the interface with the hook is simple
<Wanda[cis]>
the first thing Memory.elaborate does is calling the hook with itself as argument
<Wanda[cis]>
if the result is not None, it replaces elaborate result wholesale; otherwise, normal lowering happens
<Wanda[cis]>
you could have the hook look at attributes and use them to select between custom stuff and emitting a MemoryInstance itself
<tpw_rules>
how would the user add per-platform attributes though?
<tpw_rules>
is in "attrs" an okay place?
<Wanda[cis]>
there aren't currently any provisions for that
<Wanda[cis]>
attrs would... work
<tpw_rules>
ok, that's what i was wondering about
<tpw_rules>
would you hate me if i did that
<Wanda[cis]>
well
<Wanda[cis]>
if you make a good case that we need such functionality, we can add a better place on lib.Memory itself
<tpw_rules>
ok, we'll see
<tpw_rules>
it depends a lot on how quartus ends up reacting anyway
<tpw_rules>
i don't think i've ever cared much as a designer about e.g. the different memory block types
<Wanda[cis]>
like, memory lowering is a complex topic
<Wanda[cis]>
attrs are, in many ways, an inconvenient thing because they can basically break normal Amaranth contracts in so many ways
<Wanda[cis]>
yet, a switch like "should this be LUT RAM, BRAM, or huge RAM" tends to be useful
<Wanda[cis]>
it's also horribly platform-specific
<tpw_rules>
yeah
<Wanda[cis]>
I compiled a list of all the ways various toolchains provide to do that selection, it was ... a mess
<Wanda[cis]>
(by the way IEEE 1364.1 actually standardises the way to do it! and I don't think I've seen a single toolchain that actually followed that standard)