<_whitenotifier-6>
[amaranth] whitequark opened pull request #1224: Expand the installation docs quite a bit and fix incorrect Yosys version constraint - https://github.com/amaranth-lang/amaranth/pull/1224
<_whitenotifier-6>
[amaranth] whitequark edited pull request #1224: Expand the installation docs quite a bit and fix incorrect Yosys version constraint - https://github.com/amaranth-lang/amaranth/pull/1224
<_whitenotifier-6>
[amaranth-lang/amaranth] whitequark 12b4b18 - docs/install: link to playground.
<_whitenotifier-5>
[amaranth-lang/amaranth] whitequark 81eae1d - docs/install: link to YoWASP.
<_whitenotifier-6>
[amaranth] whitequark closed pull request #1224: Expand the installation docs quite a bit and fix incorrect Yosys version constraint - https://github.com/amaranth-lang/amaranth/pull/1224
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 3319528 - Deploying to main from @ amaranth-lang/amaranth@81eae1dd35d952391889e0f17c283f5434aef1df 🚀
<_whitenotifier-5>
[amaranth] github-merge-queue[bot] created branch gh-readonly-queue/main/pr-1215-81eae1dd35d952391889e0f17c283f5434aef1df - https://github.com/amaranth-lang/amaranth
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] fd9b6c8 - Deploying to main from @ amaranth-lang/amaranth@456dcaeb7b7bf6ff57217022001ca97c7ba5223b 🚀
<tpw_rules>
whitequark[cis]: so the remaining accepted RFCs are being deferred to 0.6? you mentioned this specifically for the init value on memory ports (54), but i'm also thinking of the memory width RFC in particular (56)
<cr1901>
(It was a gigantic switch will All The Logic. I hated reading it, so I split it. Thankfully yosys optimizes it correctly still- actually a bit better now)
<cr1901>
whitequark[cis]: Thanks for the feedback, I'm hitting the hay for tonight. Feel free to add more if you find anything
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 11e125f - Deploying to main from @ amaranth-lang/amaranth@8861b8a3ebebb37025365c3327bc9b600d54a398 🚀
<tpw_rules>
jfng[m]: i read the updated RFC. how is the PHY supposed to know to go into reset?
<jfng[m]>
oh right
<jfng[m]>
it needs an `enable` port in the signature
<tpw_rules>
yes
<whitequark[cis]>
or maybe rst
<jfng[m]>
yeah
<jfng[m]>
i was thinking of that just now
<jfng[m]>
and in the simplest case, you'd wrap the PHY in a reset inserter or something
<whitequark[cis]>
Chips4Makers (aka Staf Verhaegen): tpw_rules: re: convenience method, what I think will happen is that after we have 3-4 peripherals, common patterns will emerge that make any convenience method made now obsolete
<tpw_rules>
whitequark[cis]: fwiw, is the equation in your baud rate calculator valid?
<whitequark[cis]>
and I think it's not really a huge burden to ask early adopters to write some boilerplate
<whitequark[cis]>
tpw_rules: dunno. probably not *shrug*
<tpw_rules>
what do you mean by obsolete? like something more convenient will come up, or the boilerplate patterns will become common?
<whitequark[cis]>
as in we figure out better ways to do the same thing
<tpw_rules>
okay
<whitequark[cis]>
it's the same with the CSR interface, there's almost certainly going to be some kind of csr.Peripheral wrapper that simplifies building one
<whitequark[cis]>
especially when we add interrupts and multi-bridge peripherals become more common
<tpw_rules>
i'm still concerned that the example in the RFC is just plain broken due to the limitations of amaranth-stdio
<whitequark[cis]>
I did say that this RFC should be a draft at this moment
<whitequark[cis]>
for that specific reason
<whitequark[cis]>
that this development is all done in a weird order is also my fault, btw
<whitequark[cis]>
there's multiple things on which I'm on a critical path for (lib.memory, lib.io, stdio testing methodology, SimulationPlatform potentially) and so instead of being blocked, JF is working on the SoC bits first
<whitequark[cis]>
I've taken a hard stance against shipping more technical debt (of which the longest to address is documentation), as it's already completely unsustainable and harms our credibility, but that means making hard tradeoffs, like this one
<whitequark[cis]>
for the stdio testing methodology, I have an uhhh private google doc where I describe how I think it should look. never found time to turn it into an RFC
<jfng[m]>
for the record, this RFC isn't going to be merged before we have an -stdio UART
<whitequark[cis]>
(it's actually not uncommon for me to draft a google doc with someone else, like Wanda for example, and then hand it over to be completed and polished; that's why our progress has been so much faster recently)
<zyp[m]>
I'm probably not gonna be around for today's meeting, but I read through the updated RFC and have one suggestion
<zyp[m]>
I think it could make sense to add an opaque «config» register that the phy can interpret freely in the same manner as the divisor
<whitequark[cis]>
another option is to reorder "data" to be after "control" and "status" (so that "data" is always at the same offset) and put the config blob at the end
<whitequark[cis]>
* put the PHY config blob
<whitequark[cis]>
but now we're approaching PHY config blob being a separate cluster
<zyp[m]>
I'm thinking more about signature compatibility now, having an opaque config signal in it means you can plug in a more flexible PHY without modifying the peripheral component
<whitequark[cis]>
I guess you could plug in a PHY config as a shape?
<whitequark[cis]>
the BSP generator could recognize that and generate pseudo-fields (documentation only) for each of the lib.data bits
<whitequark[cis]>
then that goes into the signature and the register definition
<zyp[m]>
yeah, although in that case it'd make sense to include baudrate into it
<whitequark[cis]>
yes, I'm thinking the entire thing including the divisor would be in it
<whitequark[cis]>
in that case, if you're using an UART that doesn't have a configurable baud rate, you just don't put anything there (empty struct or sth)
<whitequark[cis]>
and it saves resources too, since you can conditionally exclude that register from ever being generated
<whitequark[cis]>
I like the symmetry of the same source of truth going into the signature and into the field
<whitequark[cis]>
jfng: what do you think?
<whitequark[cis]>
I guess this also means that you can rather easily extend the peripheral to handle parity, weird modes, etc, without having to do anything to the soc peripheral
underst0rm[m] has quit [Quit: Idle timeout reached: 172800s]
<whitequark[cis]>
the downside is that the extra level of indirection means you really want the BSP generator to understand what's going on with your particular UART instantiation to program it
<whitequark[cis]>
I guess the reason it's a workable compromise vs having separate PHY register cluster because it has very specific semantics: a bunch of signals that can be changed iff the PHY is in reset
<whitequark[cis]>
this means you can still painlessly do CDC on the config word
<jfng[m]>
i like the idea, but i'd be more comfortable doing this later once we also have SPI/I2C etc
<jfng[m]>
i don't mind doing breaking changes for this
<whitequark[cis]>
sgtm
<whitequark[cis]>
jfng: I think `symbol` should be `symbols` since it's a stream of multiple of themn
<whitequark[cis]>
s/themn/them/
<jfng[m]>
right, just like we don't say "datum"
<whitequark[cis]>
the FIFO would have something like r_stream where the _stream part implies plurality
<whitequark[cis]>
but here that would be unwieldy
<whitequark[cis]>
though just stream would also work
<whitequark[cis]>
or output and input
<jfng[m]>
i don't like `stream` actually (stream of what?)
<whitequark[cis]>
yeah, point
<whitequark[cis]>
jfng[m]: actually, in this case, `bus` should probably be called `csr` (which bus?)
<whitequark[cis]>
in CSR infra itself it's obvious, here not so much
<jfng[m]>
yeah, and we must do so consistently in amaranth-soc
<jfng[m]>
WishboneCSRBridge does it already (by necessity, granted)
<whitequark[cis]>
I think it's fine in principle to differ between different toplevel modules--they have different scope after all
<whitequark[cis]>
(I have no strong opinion on whether it's called csr_bus everywhere or bus in CSR and csr in peripherals)
<jfng[m]>
i'd prefer `<protocol>_bus` everywhere
<jfng[m]>
hi! it is time for our weekly SoC meeting (... in 5mins, i need a coffee)
<jfng[m]>
on the agenda today: RFC 60 (UART peripheral)
<jfng[m]>
we cannot vote on the RFC until amaranth-stdio has a proper UART, so today's goal is only discussion
<cr1901>
If a UART impl ignores divisor, does divisor become a 16-bit scratch register, or is it possible for it to be optimized out/writes/reads are a noop?
<polysci_00232[m]>
hi kinda just gonna lurk
vipqualitypost[m has joined #amaranth-lang
<vipqualitypost[m>
also lurking
<whitequark[cis]>
cr1901: I think it should be a noop
<jfng[m]>
cr1901: divisor's behavior would be up to the implementation
<tpw_rules>
if it's a no-op, then is it distinguishable from a register that doesn't exist?
<cr1901>
I agree, but how do you detect this at elaboration time? If the divisor reg is attached to the CSR bus, then writes/reads will still be honored
<cr1901>
they just won't do anything useful except scratch space
<jfng[m]>
tpw_rules: from a metadata perspective, they are different
<jfng[m]>
it would still be implemented, but with no-op behavior
<tpw_rules>
so the action would change? how would the peripheral know?
<jfng[m]>
through documentation, or however the BSP handles it
<tpw_rules>
that doesn't make sense
<jfng[m]>
hmm
<jfng[m]>
ResR0W0 ?
<cr1901>
(Basically, if divisor is ignored, then the FFs used for the reg are a waste, and I think divisor shouldn't have a backing store in that case)
<tpw_rules>
i agree as well, but i think fixing that is going to be reinventing the register system
<galibert[m]>
Who would not be using divisor?
* cr1901
raises hand hesitantly
<jfng[m]>
galibert[m]: it is written in the RFC: e.g. a CXXRTL model that uses a pty and doesn't care about baud rate
<galibert[m]>
the -stdio component would honor it, so it means the -soc code is the one wiring it up to a constant. So the -soc code is supposed not to add a dummy register in the csr
<Wanda[cis]>
galibert: virtual UARTs like CDC-ADC implementations
<jfng[m]>
ah yes, also
<galibert[m]>
(that meaning of who)
<Wanda[cis]>
s/ADC/ACM/
<jfng[m]>
tpw_rules: otherwise we can just leave the RFC as it is, and it would be a de-facto scratch register
<tpw_rules>
given that registers have no width constraints, maybe the user can just supply zero or one one Register to the constructor (perhaps one each for read and write) and then add the guarantees of only changeable while disabled
<tpw_rules>
that could mix poorly with custom field actions though?
<galibert[m]>
Wouldn't it just be a fixed_frequency = divider_value in the -soc component constructor?
<jfng[m]>
that complicates the peripheral's API significantly (currently we just as for simple stuff like a `divisor_init` and `symbol_width`)
<jfng[m]>
ask*
<cr1901>
Maybe I shouldn't worry about "how do we detect whether a register is effectively unused?" for now, and just accept that divisor is a scratch reg if it's unused for now
<tpw_rules>
why would it be significantly complicated? a register is not much harder to construct than a shape
<cr1901>
(by unused I mean "the reg's outputs are attached to something besides the read bus")
<cr1901>
aren't* attached
<jfng[m]>
galibert[m]: we can make no assumption about the frequency at which operates the PHY
<whitequark[cis]>
it's more conceptually complicated, as in it's harder to reason about the behavior
<whitequark[cis]>
a register is an elaboratable so it's considerably more complex
<tpw_rules>
okay, that is fair, especially like i said with custom field actions
<tpw_rules>
but having a shape being an extra dimension to the BSP generator, kind of specifically for this peripheral, is more complexity too
<whitequark[cis]>
I think the BSP generator should handle fields with weird shapes
<whitequark[cis]>
just in general this seems like a thing that will come up repeatedly
<jfng[m]>
ah there is the question of how to communicate an implementation-specific encoding of the `divisor` field to the BSP generator
<galibert[m]>
jfng: Who's "we" there? The -soc component? It just uses the divider value it's told to use. The component that lays out the bus? It better know about whether it's setting up a fixed-frequency or a variable-frequency serial, no? And the phy freq, the bus freq, all those details. Or have them passed in.
<whitequark[cis]>
I don't think it should be added solely for this specific peripheral; I foresee it being generally useful
<tpw_rules>
also just to be clear, what do you mean by "weird shape"?
<whitequark[cis]>
not unsigned/signed
<whitequark[cis]>
lib.data
<whitequark[cis]>
jfng[m]: so actually passing a shape for the PHY config word solves this
<tpw_rules>
okay, so perhaps a "layout", they are shape-like
<whitequark[cis]>
what it doesn't solve is the documentation that needs to appear in the generated BSP docs, which is a little annoying
<cr1901>
jfng[m]: To be clear, I like divisor being impl specific, and would prefer that an unused divisor's backing store be optimized out. Aside from that, I'm happy with the peripheral as described.
<tpw_rules>
would it support only StructLayout without children?
<whitequark[cis]>
but I think we can add a way of attaching docstrings to lib.data layouts?
<jfng[m]>
galibert[m]: the UART peripheral: that base frequency may be a flat 100MHz, or anything else, and may also be configurable at runtime
<tpw_rules>
or arbitrary stuff?
<cr1901>
I'm a bit too scatterbrained right now to contribute beyond that lol
<galibert[m]>
Catherine: adding free-ish documentation for the docs generation would be extremely useful
<whitequark[cis]>
tpw_rules: I'm thinking that it would support almost arbitrary `lib.data` layouts, since fields and lib.data have very similar structure
<whitequark[cis]>
namely, struct and array layouts can be translated to fields trivially and can be treated the same by the BSP generator
<tpw_rules>
whitequark[cis]: well CSR fields can't be nested can they?
<whitequark[cis]>
tpw_rules: that doesn't matter; each CSR field has a path
<galibert[m]>
jfng: I thought y'all were talking about the special case "the divider is fixed, what happens to the register on the bus". Weren't you?
<whitequark[cis]>
if you nest a lib.data thing in a field, the path gets replicated with stuff appended to it
<whitequark[cis]>
it works out just fine
<tpw_rules>
so then the BSP generator "just" needs to work for arbitrary paths in a CSR register rather than precisely one level
<whitequark[cis]>
it already has to
<tpw_rules>
(or zero with the "shortcut" RFC)
<whitequark[cis]>
because you can put fields in field arrays and field maps, the BSP generator has to be able to understand that fields have arbitrary paths
<jfng[m]>
whitequark[cis]: yeah, i'm just worried about doing this prematurely; and i'd prefer to tackle alongside a peripheral API work
<whitequark[cis]>
adding lib.data support does not require anything radically new
<tpw_rules>
oh duh you're right
<whitequark[cis]>
it's really not an issue
<whitequark[cis]>
it's just a matter of teaching the BSP generator about the slightly different metadata for lib.data (actually we don't currently emit it I think; so a core RFC will be needed)
<tpw_rules>
so to the BSP/doc generator a field with a "layout" shape would be exactly equivalent to having sub-fields with "simple" shapes
<whitequark[cis]>
correct
<jfng[m]>
galibert[m]: we were talking about "the divisor is handled separately from the `Divisor` register, or not at all" i think
<tpw_rules>
yes, that makes sense, i like it
<whitequark[cis]>
a field becomes a field map, virtually
<whitequark[cis]>
the path system is incredibly flexible, so that you can do things like this without ending up with cursed BSPs
<tpw_rules>
and i like that now you pass a shape to the UART peripheral to specify the config specific to your PHY. then pass None to not get a register
<tpw_rules>
i like that a lot better than having a fixed "divisor" register
<whitequark[cis]>
well, JF didn't agree to the change yet
<whitequark[cis]>
but I also like that idea
<whitequark[cis]>
I expect we may find a decent amount of use for it elsewhere in the project
<tpw_rules>
brb
<jfng[m]>
would there be a maximum width for this "config" shape ?
<jfng[m]>
considering that we are going to put it in a register
<whitequark[cis]>
I feel that a maximum width for it would have to be pretty generous if it's present at all
<whitequark[cis]>
32 bits at least
<tpw_rules>
i don't think it should be present? the CSR logic will handle arbitrary widths
<jfng[m]>
even more than 32, possibly; at which point it is unclear whether this should be a small cluster instead
<jfng[m]>
however, we have strong atomicity guarantees, thanks to the CSR bus
<jfng[m]>
so a single register isn't an issue
<whitequark[cis]>
I think it's quite useful that the entire thing is writable iff the peripheral is kept in reset
<whitequark[cis]>
which is a benefit over an arbitrary cluster
<tpw_rules>
yeah i don't think there's technical justifications, the only upside would be to discourage silly designs
<whitequark[cis]>
I wonder how our C language API would look like
<galibert[m]>
the issue I see with passing a shape is that the -soc needs to know what to connect the register to, no? So you need more than a shape, you need an interface or equivalent
<jfng[m]>
ok, let's try having an implementation-defined `Config` register
<jfng[m]>
as an experiment
<whitequark[cis]>
no? you just need a matching shape on PHY and SoC sides
<whitequark[cis]>
so the (strawman) amaranth_soc.basic_uart.UART will have a like CONFIG_SHAPE constant or something that can be plugged into the SoC can use
<whitequark[cis]>
* so the (strawman) amaranth_soc.basic_uart.UART will have a like CONFIG_SHAPE constant or something that can be plugged into the SoC
<jfng[m]>
jfng[m]: it's not like we can't go backwards later on if it doesn't work out
<whitequark[cis]>
I think the PHY config should be separate from the SoC peripheral config
<galibert[m]>
you want the -soc component to be independant of the -stdio one? You want it to interface with any PHY component?
<whitequark[cis]>
so it would be Config + PhyConfig
<whitequark[cis]>
galibert[m]: not necessarily any, but with more than one potentially, yes
<whitequark[cis]>
there's a reason Amaranth SoC is called a toolkit
<whitequark[cis]>
and there's a reason there is a SoC/stdio split
<whitequark[cis]>
(multiple reasons)
<galibert[m]>
Oh, I thought the exercise was "we have a nice component in -stdio, let's put it on a bus"
<tpw_rules>
there's no Config is there?
<tpw_rules>
right now? only divisor, which would become PhyConfig
<whitequark[cis]>
galibert[m]: we can do both
<whitequark[cis]>
I have more ambition than MiSoC or LiteX
<jfng[m]>
i have trouble distinguishing between Config and PhyConfig
<jfng[m]>
to me, they feel like the same
<whitequark[cis]>
jfng: `Config` affects the SoC part, `PhyConfig` affects the stdio part
<whitequark[cis]>
they have different reset and potentially live in different clock domains
<whitequark[cis]>
I think having a register whose writability partially changes in response to another bit in that register is not well defined, also
<tpw_rules>
is there anything in Config right now? or would you want to reserve it for stuff like interrupts and DMA?
<whitequark[cis]>
like, do you need to clear the enable bit first or can you do it with the same write?
<whitequark[cis]>
finally, if I give the SoC peripheral a phy_config_shape, I expect it to put that on the bus as-is
<tpw_rules>
right now that bit is in a register called Control
<whitequark[cis]>
instead of requiring me to do a weird shift dance
<whitequark[cis]>
tpw_rules: "enable"
<tpw_rules>
that's in Control. do you propose renaming that register to Config too?
<jfng[m]>
whitequark[cis]: good point
<whitequark[cis]>
hm
<galibert[m]>
Catherine: you sound that >< far for wanting a component taking an arbitrary Interface and putting it on a bus, serial or not
<whitequark[cis]>
oh.
<whitequark[cis]>
I thought Control was called Config
<jfng[m]>
it's only one renaming away from being called that
<whitequark[cis]>
Control is good since we have Control and Status on the Control and Status Register Bus
<tpw_rules>
i think Control + PhyConfig so that maybe later we can add Config for DMA etc.
<whitequark[cis]>
but yeah, I think PhyConfig should live in a different register than Control.enable or Config.enable or whatever it's called
<tpw_rules>
yes, for sure
<whitequark[cis]>
DMA sounds wildly out of scope for basic UART but interrupts are not
<whitequark[cis]>
and they will also need some bits (interrupt enable, interrupt flag)
<galibert[m]>
DMA is sooooooo nice though
<whitequark[cis]>
galibert[m]: there are multiple different strategies for this
<galibert[m]>
yeah
<whitequark[cis]>
in one, the component gains a WB/AXI/etc initiator. that is definitely out of scope, it would make the component way too complicated
<whitequark[cis]>
in another, there is an external DMA initiator that can poke existing registers
<whitequark[cis]>
that requires little to no changes to the UART component
<jfng[m]>
before working on interrupts, i'd like to reconsider the existing event API (`event.Monitor` and friends)
<jfng[m]>
which predates the CSR register API, and its underlying ideas
<whitequark[cis]>
galibert[m]: I want a peripheral that Universally supports (within reason) any Asynchronous Receiver and/or Transmitter
<galibert[m]>
I've seen a lot of different strategies there. The simplest one seemed to be a generic dma at the cpu level with dedicated drq lines, the dma doing writes to a register over the bus
<whitequark[cis]>
I don't see the word "serial" in it
<Chips4MakersakaS>
(leaving the meeting now)
<whitequark[cis]>
but it's also rather far from "any arbitrary interface" which is just not feasible anyway
<tpw_rules>
i think at this point with the config register it should be called a StreamPort or something
<tpw_rules>
plus a recipe for the future better amaranth-stdio UART
<whitequark[cis]>
it's true that this is more ambitious than your average UART component, but also a lot of this ambition is achieved by trimming it down, not extending it
<whitequark[cis]>
tpw_rules: I think UART is fine, it's very descriptive and appropriate
<tpw_rules>
like, so openeth wikipedia "A universal asynchronous receiver-transmitter (UART /ˈjuːɑːrt/) is a peripheral device for asynchronous serial communication in which the data format and transmission speeds are configurable. It sends data bits one by one". it's not wrong to infer serial into there
<whitequark[cis]>
it's just an UART designed like you would design a modern SerDes based interface
<whitequark[cis]>
tpw_rules: yes, I'm aware
<galibert[m]>
so it would essentially means that a component that wants dma needs to have a register the cpu would write to send the data, and a dedicated drq line that's independant than the interrupt
<whitequark[cis]>
I'm intentionally exploiting the ambiguity
<galibert[m]>
(read from too)
<tpw_rules>
that seems unnecessarily confusing
<jfng[m]>
whitequark[cis]: in a previous call, your motivation for the UART peripheral was to "standardize an interface to transmit/receive bytes asynchronously"
<whitequark[cis]>
because I think it can be used in interesting, novel, and useful ways, rather than just endlessly replicating the same dumb 16550 register map
<whitequark[cis]>
jfng[m]: yeah
<whitequark[cis]>
that's the same thing
<galibert[m]>
I said that? I don't remember it, that makes me sound smarter than I am
<whitequark[cis]>
tpw_rules: I think it's a beautiful approach
<tpw_rules>
whitequark[cis]: i think it is too (now with the arbitrary config register shape), so i don't get why we hang the baggage of the term "UART" onto it
<whitequark[cis]>
largely because we need an UART of some kind and this is all that most people will use it for
<galibert[m]>
I could see myself using it for a nice SD card controller
<whitequark[cis]>
... okay, that's kind of cursed
<galibert[m]>
especially once dma is sorted out
<whitequark[cis]>
(as in, calling it basic_uart is cursed in that case)
<tpw_rules>
then i think the docs should have a "UART recipe" which takes this StreamPort or whatever and adds a UART phy from amaranth-stdio to direct users
<whitequark[cis]>
tpw_rules: I wouldn't mind that personally
<tpw_rules>
or, though this was already rejected, have a UART which does the above in code as well
<whitequark[cis]>
it wasn't rejected; I said that I think it's premature
<tpw_rules>
plus a note saying "looking to do weird things with your UART? check out StreamPort!"
<whitequark[cis]>
premature and rejected are not the same thing
<tpw_rules>
well rejected for this RFC
<tpw_rules>
that is correct though
<whitequark[cis]>
right
<jfng[m]>
i don't like `StreamPort`, as it is too abstract
<whitequark[cis]>
yeah me neither
<jfng[m]>
the word play with UART is ok to, and adds a vibe to amaranth-soc that i like
<tpw_rules>
what about UDRT? universal data receiver transmitter? there's no reason i can't hook this up to an SPI PHY, especially with a few mode bits in the phy config register
<whitequark[cis]>
I'm going to check out from this particular bikeshed personally, I'm happy with UART, unhappy with StreamPort, and trust JF to get it right
<tpw_rules>
ok, we don't have to finalize it now either
<whitequark[cis]>
(also, I'd just like to remind everyone that it's pre-0.1 times and we aren't beholden to things in a way like we are in Amaranth)
<tpw_rules>
it can be renamed if users end up confused too
<whitequark[cis]>
yes, exactly
<tpw_rules>
well thank you all for the discussion time, i'm glad we got things into a clearer and more useful place
<jfng[m]>
yeah, the `Config`/`PhyConfig` scheme is something that i'm going to experiment with
<jfng[m]>
thanks for attending
<zyp[m]>
if it's becoming a generic transmitter/receiver, you might want to have a configurable data shape as well
<tpw_rules>
i don't see how the user would need to provide Config bits fwiw, i figure that should be controlled entirely by the peripheral itself
<zyp[m]>
which should just be a matter of passing in another shape that defaults to unsigned(8)
<tpw_rules>
it's currently termed "width" but it is already configurable?
<zyp[m]>
ah, right
<tpw_rules>
though the signature def looks like it will not be suitable for arbitrary complex shapes, but it's unclear what the value is, you can't read half of the data register to get a particular field because you'll pop the other half too
<galibert[m]>
not sure arbitrary shapes mean something there, because it needs to be something that can be transmitted as-is over the data bus. Unless it's for documentation purposes I guess
<zyp[m]>
you'd pop it all and then pick out the field you want
<tpw_rules>
does the BSP generator know the difference? i guess we would need to pick a compatible implementation for sub-field access
<tpw_rules>
sub-register*
<galibert[m]>
the bsp generator still needs to be written, so :-)
<jfng[m]>
the BSP generator in our heads
<tpw_rules>
(it's kind of unclear if we should even have a sub-field access function, considering the way the atomic CSR access works)
<tpw_rules>
(but i get grossed out when my HALs spend hundreds of cycles bit packing junk, although i'm led to believe there are some good instructions for that these days)
<jfng[m]>
i have to go, i'll sum up today's discussion later this evening or tomorrow
<tpw_rules>
okay, have a nice weekend
<jfng[m]>
thanks
<whitequark[cis]>
from the "abyss domain expert" dept
<tpw_rules>
whitequark[cis]: this all seems right, i think transparency should be mentioned here too though. e.g. cyclone v memories can only be transparent on the same port and only non-transparent across ports
<tpw_rules>
transparency is what's given the most inference issues to me
<whitequark[cis]>
soft transparency will be added by the toolchain
<whitequark[cis]>
it's just a comparator and a mux
<tpw_rules>
can it be removed? i was more thinking about the auto lowering
<tpw_rules>
with the platform hook
<whitequark[cis]>
i don't understand the question
<whitequark[cis]>
transparency is added by the synthesis tool, not amaranth
<tpw_rules>
there are cases in cyclone v where transparency is mandatory, i think a circuit to remove it would need a register
<whitequark[cis]>
uhm... what
<galibert[m]>
I don't think you can remove transparency?
<galibert[m]>
you can add it, but you can't remove it
<whitequark[cis]>
that sounds batshit
<tpw_rules>
specifically on the same port on a true dual port ram
<galibert[m]>
more like page 21, cv has mlab and m10k
<whitequark[cis]>
that's completely batshit
<galibert[m]>
That's Intel for you
<whitequark[cis]>
you'd need like... two RAMs, one on a single cycle delay?!
<galibert[m]>
well Altera(Intel(Altera))
<tpw_rules>
whitequark[cis]: you can do it if you set it up cross-port but then you sort of only get one port
<whitequark[cis]>
ok, so page 32 says you get non-transparent behavior only across ports
<whitequark[cis]>
so it has transparent TDP RAM and non-transparent SDP RAM
<whitequark[cis]>
I... don't think I want to go into that, the warning is already long enough and if the toolchain duplicates the RAM it's fine I guess
<whitequark[cis]>
why is quartus
<galibert[m]>
it's not quartus at that point, it's the hardware design itself
<whitequark[cis]>
why is altera
<galibert[m]>
now that's a good question
<polysci_00232[m]>
With add_sync_process being deprecated, what is the intended replacement for testing in multiple clock domains with add_process/add_testbench?
<whitequark[cis]>
add_testbench + yield Tick()
<polysci_00232[m]>
got it
<tpw_rules>
whitequark[cis]: i guess also your big cursed memory blurb means that failure is not an option for platform hooks? if they don't like it they should just punt to the default implementation
<tpw_rules>
(i think they should warn if doing this though)
<whitequark[cis]>
how so?
<whitequark[cis]>
the blurb doesn't really constrain the implementation in any way
<whitequark[cis]>
it just says which things usually work
<polysci_00232[m]>
I cant seem to find the RFC for deprecating add_sync_process in the RFC repo and im running into further issues porting to add_testbench. Could anyone point me to where I can find it or take a second to answer some questions?
<polysci_00232[m]>
Oh there appears to be a typo in the warning message, it pointed me to RFC47 which is non-existent
<whitequark[cis]>
oh! can you submit a PR please?
<polysci_00232[m]>
yep, after I get the test bench to work
<whitequark[cis]>
thanks!
<polysci_00232[m]>
ehh reading that did not resolve my confusion
<polysci_00232[m]>
So I am trying to simulate a module containing two submodles in different clock domains
<polysci_00232[m]>
I am using DomainRenamer to change the sync domain of one submodule to a new domain called "ftclk"
<polysci_00232[m]>
but when I run the simulation the domain renamed submodule appears to have stuck with the sync clock
<polysci_00232[m]>
The simulation I am using worked just fine with add_sync_process(domain="ftclk"). My first pass solution was just to replace all the yields with yield Tick(ftclk) but that doesnt seem to be sufficient?
Lunaphied[m] has quit [Quit: Idle timeout reached: 172800s]
jjsuperpower has quit [Remote host closed the connection]
<tpw_rules>
whitequark[cis]: to be clear, the RAM read port holds its previous value when en is not asserted?
<whitequark[cis]>
yeah
<tpw_rules>
ok. i'm kind of pondering a memory test gateware that you download to a chip and tests all the combinations to make sure memories actually work how we describe for each platform. particularly to avoid bugs with platform-specific lowering
<whitequark[cis]>
sure. you can check that it fails by getting Vivado to infer LUTRAM
<whitequark[cis]>
there's an open bug about that
<whitequark[cis]>
but also just running it in simulation with vendor libraries should be enough
<tpw_rules>
i don't see that issue just searching "LUTRAM"
<whitequark[cis]>
actually i think i know how to improve it
notgull has joined #amaranth-lang
<tpw_rules>
can the memory lowerer get the domain of a port in more concrete terms than the text name? i seem to remember work on that being in progress
<tpw_rules>
what exactly do you get if you call ReadPort.Signature.create()? where's the backing memory come from?
<tpw_rules>
i also don't see anything explicit about what subset of objects need to/can be submodules, the examples just have the memory which seems to be the new wisdom but it would be nice to put a note in the constructors
<tpw_rules>
other than that it all looks great, thanks again
<whitequark[cis]>
<tpw_rules> "what exactly do you get if you..." <- nowhere, it's `None`
notgull has quit [Ping timeout: 264 seconds]
<whitequark[cis]>
<tpw_rules> "i also don't see anything..." <- wrote a note about which things are components and which are interface objects
<whitequark[cis]>
in any case you can't add lib.memory ports as submodules, it's a hard error. and not adding Memory shows a warning. so I'm not super concerned
<whitequark[cis]>
<tpw_rules> "can the memory lowerer get the..." <- the situation on that is kind of complicated, me and Wanda came to the conclusion that it both creates issues that aren't easy to resolve, and isn't in fact enough for memory inference
<whitequark[cis]>
it's more readable and it should now be really clear what transparency really does
<tpw_rules>
cool
<tpw_rules>
so i'm not sure what the purpose of the memory hook is then? in fact i see it's evaluated in the memory's elaborate, so not all the ports might even exist? i also was thinking it's not possible for the lowerer to check the equality of addresses on two separate ports for true dual port memory?
<_whitenotifier-5>
[amaranth] github-merge-queue[bot] created branch gh-readonly-queue/main/pr-1228-8861b8a3ebebb37025365c3327bc9b600d54a398 - https://github.com/amaranth-lang/amaranth
<tpw_rules>
i think the best that could be done is to put in a verilog template which one knows the synthesis tool can infer properly, but that is not very satisfying
<tpw_rules>
but even then if you can't safely get the clocks or assert that they are all the same?
<whitequark[cis]>
> in fact i see it's evaluated in the memory's elaborate, so not all the ports might even exist?
<whitequark[cis]>
that is not true, memory's elaborate is going to be called after all of the ports are created
<adamgreig[m]>
Catherine: do you think an rfc for new stdlib stuff (lfsr...) should use lib.wiring signatures?
<adamgreig[m]>
a lot of the interface atm is very similar to crc, but crc predated lib.wiring
<galibert[m]>
Ideally crc should be upgaded to have a signature
<tpw_rules>
whitequark[cis]: how would amaranth know that?
<whitequark[cis]>
tpw_rules: it's just how elaboration is done. i mean, nothing really stops you from modifying an elaboratable in some really bizarre place but then the behavior is just undefined. in basically all normal circumstances you can think of the order works out
<whitequark[cis]>
i'm not sure i want to go into that rn, Wanda may explain it better
<tpw_rules>
ok, you had talked about leaving open the possibility of eager elaboration a while ago and this sounds like it closes it
<whitequark[cis]>
I think eager elaboration is already a non-starter with just the existing code
<whitequark[cis]>
> i also was thinking it's not possible for the lowerer to check the equality of addresses on two separate ports for true dual port memory?
<whitequark[cis]>
that's correct, and this is why I was arguing to Wanda that the LateElaboratable thing she proposed (basically a way to get a second `clock_domains` argument with the concrete clock domains) isn't good enough. we even drafted a design that I'm happy with, but it's kind of too little too late (pun intended)
<whitequark[cis]>
> so i'm not sure what the purpose of the memory hook is then?
<whitequark[cis]>
well, we want to make it work somehow. you can already implement lowering for some memories that sort of works, which is unsatisfying but actually useful to make e.g. ASIC SRAMs work, so the hook is not useless
<whitequark[cis]>
adamgreig[m]: good question. do you think we can upgrade LFSR to streams first? (you can propose an RFC depending on another proposed RFC)
<whitequark[cis]>
err, CRC
<adamgreig[m]>
I guess crc could have an rfc for lib.wiring + streams
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 654ae24 - Deploying to main from @ amaranth-lang/amaranth@6ce82848d960b368cb226c80e495c2dc5de98172 🚀
<adamgreig[m]>
and then use that updated api for lfsr too?
<whitequark[cis]>
and have LFSR stuff in parallel, nothing wrong with that
<whitequark[cis]>
yep
<galibert[m]>
streams would be nice, not should how eventual granularity would cope with that though
<adamgreig[m]>
sounds good
<galibert[m]>
s/should/sure/
<whitequark[cis]>
galibert: whenever we get stream lanes we can upgrade CRC for that too
<whitequark[cis]>
adamgreig: do you know how to implement lane-aware CRC in principle, btw?
<adamgreig[m]>
well streams don't currently support stb/lanes and neither does crc, so yea, both can happen later right :P
<whitequark[cis]>
is it just like, four different CRC blocks ?
<galibert[m]>
adamgreig: crc really needs it though :/
<adamgreig[m]>
Catherine: the only design I know about is essentially duplicating the generation matrix for each set of possible stb values, which is at least only the number of lanes
<whitequark[cis]>
interestingly, stb may not be a very good abstraction for that
<whitequark[cis]>
it should be something like last_nonempty
<tpw_rules>
ok the ASIC case i hadn't thought about, but i'm not sure it would be useful for the workaround to FPGA inference deficiencies i had imagined, that might have to be put on the back burner
<whitequark[cis]>
and actually stream transformers (in the ministream RFC appendix) will let you have both last_nonempty and stb if you want, provided you can interconvert
<whitequark[cis]>
or I guess CRC can just assert that there are no holes in stb
<tpw_rules>
unless we're okay without true dual port (whatever) and have a big scary warning sign that domainrenamer might do unsound things?
<adamgreig[m]>
yea, it's sort of stb with additional requirement that all the zero bits are at the end
<adamgreig[m]>
no holes but also all set bits are at the start
<adamgreig[m]>
hmm, maybe? I haven't thought about handling having e.g. only the last bit set but it might be OK
<whitequark[cis]>
tpw_rules: before we do anything like that we really need some form of test suite that exercises memory inference in all the proprietary toolchains (and I suppose Yosys for good measure) and actually checks which cells end up in the netlist
<whitequark[cis]>
if you write that it would be extremely useful and we could merge it in the testsuite, gated with the relevant env vars
<whitequark[cis]>
can't run it on CI but still useful
<whitequark[cis]>
adamgreig[m]: I guess you'd have a mux that collapses all the lanes into the beginning
<galibert[m]>
isn't there going to be license issues?
<whitequark[cis]>
adamgreig: that sounds expensive, it's kind of like a barrel shifter almost
<whitequark[cis]>
galibert: didn't know you need a license to distribute code that interopreates with Vivado. if so Amaranth is already violating it :p
<galibert[m]>
you need vivado/etc to actually run the test
<tpw_rules>
what do you mean by exercising inference? the way i imagined it is that the amaranth lowerer would just create a bunch of the IP blocks that the e.g. quartus manual calls for and sets up their options, rather than doing in ference
<galibert[m]>
and you may need to run the tests on fpgas that are not free tier, no?
<whitequark[cis]>
galibert[m]: so you run the test only if you have vivado
<whitequark[cis]>
galibert[m]: I think now that we have a spartan-7 we should have most primitives covered in the free tier
<whitequark[cis]>
but in any case, I don't see a problem; only the subset of people who has access to the FPGA toolchains will be able to run the tests, which is ok
<whitequark[cis]>
it's strictly better than the status quo
<whitequark[cis]>
tpw_rules: well we have the Verilog-based flow for memories, via `MemoryInstance`, right?
<whitequark[cis]>
we know it works for some cases and breaks for some others, with more RFCs in queue that can break and/or fix it
<whitequark[cis]>
but we currently have no knowledge of exactly where it works and where it doesn't
<tpw_rules>
whitequark[cis]: yes, i get that. they sort of get transmogrified into something that gets inferred
<whitequark[cis]>
ideally, we would be just using that, as it saves us labor
<tpw_rules>
i guess i don't get the "actually check which cells end up in the netlist". like whether or not it has memory blocks or not?
<whitequark[cis]>
yes
<tpw_rules>
oh, on first read i thought you meant like which black box the tool used or something
<tpw_rules>
okay, that makes more sense
<whitequark[cis]>
whether it has the memory blocks and how many are there (maybe even check how they are wired, if we can roundtrip it through BLIF or something)
<tpw_rules>
yeah that last part seems dubious
<whitequark[cis]>
i think that should work fine
<whitequark[cis]>
most toolchains can do BLIF or EDIF
<whitequark[cis]>
and then yosys can ingest it and assert on it
<whitequark[cis]>
it's not pretty but it's doable
<whitequark[cis]>
also we can also simulate the synthesized netlist with vendor libraries, uh, many of them are readable by iverilog or something I think
<tpw_rules>
well dubious for my current knowledge. perhaps i could expand that
<galibert[m]>
what are those?
<whitequark[cis]>
galibert: google them
<tpw_rules>
i think quartus's are all encrypted
<whitequark[cis]>
tpw_rules: some of the vendor libraries are definitely protected (encrypted). while that is trivial to break in most cases it's too much of a pain for tests
<galibert[m]>
ah nice, for once google is useful on a short name
<whitequark[cis]>
(i am pretty sure it is completely legal for me to circumvent the protection on them for the purpose of ensuring interoperability)
<whitequark[cis]>
(it's just a pain in the ass)
<galibert[m]>
Catherine: let me know if you need to key
<galibert[m]>
s/to/the/
<tpw_rules>
yeah galibert[m] has already done it, i don't think vendoring those files is smart though
<galibert[m]>
(for quartus)
<whitequark[cis]>
does quartus have like... strings quartus_syn | grep 'BEGIN RSA PRIVATE KEY'
<tpw_rules>
now cursed shit like that i can get behind, especially with nix :P
<whitequark[cis]>
tpw_rules: it's definitely not ok to redistribute them but it's probably ok to redistribute a script that extracts the key and deprotects the files
<whitequark[cis]>
as long as you don't distribute it in the US
<whitequark[cis]>
which is why I just don't want to bother
<galibert[m]>
There's the annoying EUCD here too
<tpw_rules>
well uh i have bad news about my country of residence
<tpw_rules>
i guess ultimately i'm unsure about the statement "many are readable by iverilog", idk if quartus has unprotected memory IP but i think a lot of xilinx are encrypted too
<tpw_rules>
but that's sort of a stretch stretch goal anyway
<galibert[m]>
I honestly don't remember how I got the key (extracted it myself or found it on the internet?)
<tpw_rules>
a convenient environment for running the tests could live outside amaranth too
<galibert[m]>
MGC-DVT-MTI.pem in google finds it for you though
<whitequark[cis]>
tpw_rules: we could also use something like fusesoc to drive modelsim or whatever
<whitequark[cis]>
but it's a paaain
<galibert[m]>
and that name is in the encrypted files :-)
<whitequark[cis]>
also Yosys has FOSS vendor libraries
<tpw_rules>
why fusesoc
<whitequark[cis]>
cause i know it can drive modelsim and i have no idea what else can
<whitequark[cis]>
er
<whitequark[cis]>
edalize
<whitequark[cis]>
well, same difference
<tpw_rules>
edalize looks more right, and amenable to packaging
<tpw_rules>
but again that's a stretch stretch goal
<tpw_rules>
i'm more concerned with behavior personally, but checking the tools would be good too
<whitequark[cis]>
tpw_rules: what this would do is to cause their `addr` member to be physically the same signal, making it completely unambiguous that they're shared
<tpw_rules>
physically as in `is`? that doesn't sound unreasonable. it could confuse users who try to assign different things to both?
<whitequark[cis]>
well, yes
<whitequark[cis]>
there are downsides
<whitequark[cis]>
but it's a way to resolve this that doesn't involve, like, a SAT solver on the new IR
<tpw_rules>
could it just not have an addr attribute
<tpw_rules>
(i think it would also have to be the other way around for multiple read ports paired with the same write port)
<tpw_rules>
idk if like a TandemReadPort is too many types
<whitequark[cis]>
ReadWritePort is another option, but it's more heavyweight
<whitequark[cis]>
I wanted to avoid introducing more types, indeed
<zyp[m]>
from a user perspective, a .read_write_port() method sounds like the cleanest approach
<tpw_rules>
also i did not internalize earlier that ports are no longer elaboratable, so they can't have their own domain anyway. it should be perfectly valid to simply take the domain name and pass it to ClockSignal, it can't get smuggled out and put under a renamer or soemthing
<tpw_rules>
(i know we are trying to kill the domain name hierarchy)
<galibert[m]>
Domain name inheritance is not necessarily a bad thing
<whitequark[cis]>
zyp[m]: yes, but do we want that long term?
<whitequark[cis]>
considering that it's only needed for like, quartus
<tpw_rules>
ok then i don't immediately see a need for concrete clock domains
<galibert[m]>
It’s when it’s going back up it’s messy
<whitequark[cis]>
i don't want to have core features that exist just because quartus can't into memory
<tpw_rules>
vivado can infer true dual port too?
<tpw_rules>
(i don't actually know if quartus can, i haven't tried. i just wanted to take its agency away)
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 22c8472 - Deploying to main from @ amaranth-lang/amaranth@9ed83b6aff794f172bc2e9c8bba7467e1317a2c5 🚀
<galibert[m]>
Does read-write port kinda match real hardware though? Not only altéra one?
<tpw_rules>
yes, true dual port ram
<whitequark[cis]>
vivado should definitely be able to infer TDP RAM
<zyp[m]>
I don't have enough experience with memories to really argue either way, but to me it makes sense to ask for a RW port if that's what you want, instead of having to ask for two independent ports and wire them so they can be inferred to be a single RW port
<whitequark[cis]>
asking for an RW port was the original plan when i was building amaranth 0.1
<whitequark[cis]>
however we never got it because yosys only had individual read/write ports
<whitequark[cis]>
and by the time yosys got readwrite ports it also got the ability to combine them based on a SAT solver query (!)