<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] fd4deb1 - Deploying to main from @ amaranth-lang/amaranth@ccf87c62e4b4c471b1e86065ae0b7a2a486f0609 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] fa4f37d - Deploying to main from @ amaranth-lang/amaranth@fc81ff17f79a183dc9d0424f0f6694f4d66285d3 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] e116f1e - Deploying to main from @ amaranth-lang/amaranth@a72528275164cdf0d6af0a75f3e89fecdfe03b4a 🚀
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] d7a1744 - Deploying to main from @ amaranth-lang/amaranth@751e0f4b5737b4194b389131b79d3856d2d2c8b0 🚀
<whitequark[cis]>
we're down to <100 issues again! justadam
<_whitenotifier-7>
[amaranth] whitequark opened pull request #1159: Add stub documentation page for memories and complete some last TODOs in language guide - https://github.com/amaranth-lang/amaranth/pull/1159
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] whitequark 03e236f - Deploying to main from @ amaranth-lang/amaranth@a430c1d4a37c07328d30e37de15d077c90dc0aa5 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth] whitequark 77e41cc - docs: add stub `stdlib/memory`, mark `guide` as done.
<_whitenotifier-5>
[amaranth-lang/amaranth] whitequark 6dc7c27 - docs/guide: fix a bunch of TODOs.
<_whitenotifier-7>
[amaranth] whitequark closed pull request #1159: Add stub documentation page for memories and complete some last TODOs in language guide - https://github.com/amaranth-lang/amaranth/pull/1159
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 44057ae - Deploying to main from @ amaranth-lang/amaranth@6dc7c2718cc361485a847d68aeeb5f8f188d9f2f 🚀
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] db7e306 - Deploying to main from @ amaranth-lang/amaranth@1cb9d4384116db7fdc2c19a3fc3be2b53ef73ec3 🚀
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] b595b22 - Deploying to main from @ amaranth-lang/amaranth@c30585b47b6e0d85aef576e852873b1d9012c9ad 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 2397c1e - Deploying to main from @ amaranth-lang/amaranth@f524dd041aff2ec18ffc3a2487fb4cc0e3719447 🚀
<jfng[m]>
oops, it seems i messed up the SVG inlining
<whitequark[cis]>
it miiight be that github doesn't let you render SVGs inline or something
<whitequark[cis]>
I feel like it should live in amaranth_soc.gpio, not under amaranth_soc.csr
<whitequark[cis]>
it's not a part of the CSR infrastructure itself
<jfng[m]>
hmm
<jfng[m]>
what about Wishbone memories ? they would live in `.wishbone`, no ?
<jfng[m]>
though they are peripherals
<whitequark[cis]>
what about UART? does that also go into amaranth_soc.csr?
<jfng[m]>
how about, amaranth_soc.periph ?
<whitequark[cis]>
I don't like the name and I don't like the seemingly unnecessary level of nesting
<whitequark[cis]>
amaranth_soc.gpio is perfectly unambiguous
<whitequark[cis]>
and I think we decided to not have any cores in amaranth_soc, right?
<whitequark[cis]>
CPU cores, I mean
<jfng[m]>
whitequark[cis]: yeah, i don't really have anything against it tbh
<whitequark[cis]>
so we only have infrastructure and peripherals
<whitequark[cis]>
re Wishbone memories, we have already determined that those are a rather special case in most respects, so whatever ends up happening with them, isn't very useful as a guideline for others
<whitequark[cis]>
I'm actually not sure if they should be in amaranth_soc.sram or amaranth_soc.wishbone
<whitequark[cis]>
they are not really a peripheral, since they don't have any registers, and appear on the bus as themselves
<jfng[m]>
i used them as an example of something that is 90% bus-specific, but anyway, placing them in the bus module (e.g. `.wishbone`) is not viable: it will fall apart as soon as we have a peripheral that uses two different busses
<whitequark[cis]>
well, SRAMs are special in that they are wholly coupled to the bus interface, and there isn't anything that needs to be portable between buses; it's just the amaranth.lib.memory.Memory and some random logic
<whitequark[cis]>
no CSRs, no logic beside the bus adapter, no external ports
<jfng[m]>
that is true
<whitequark[cis]>
because of this, there's no real difference as to where they are placed. I vaguely feel like they should live under amaranth_soc.wishbone because amaranth_soc.sram is a name that is useful to reserve for external SRAMs (which are peripherals, will have wait states, etc)
<whitequark[cis]>
but you could also probably convince me otherwise
<jfng[m]>
we have until a Wishbone SRAM RFC is done to make up our minds
<whitequark[cis]>
yes
<jfng[m]>
do you mind if i include the SVG files in the rfc repo ?
<whitequark[cis]>
I feel that it's weird that GPIOPeripheral takes addr_width, even though I understand it is a consequence of our decisions
<whitequark[cis]>
jfng[m]: I'm gonna have to look up how Rust does it, I think some minor additional work will be needed
<jfng[m]>
it is unfortunate that addr_width does not have a default value here
<whitequark[cis]>
I think we can leave that interface as-is and then break it later when we have a better understanding of how to build a SoC
<whitequark[cis]>
or just leave it be
<jfng[m]>
as a user, i do not want to have to guess a good value, only to get rejected if i chose one that is too small
<jfng[m]>
but yeah
<whitequark[cis]>
ok, you can put the images under NNNN-rfc-name folder under text
<jfng[m]>
so `text/NNNN-rfc-name.md` is the RFC itself, and `text/NNNN-rfc-name/foo.svg` for images ?
<whitequark[cis]>
correct
<jfng[m]>
ack
<galibert[m]>
Catherine: when you say wishbone srams you mean internal fpga rams, not srams connected to fpga pins, right?
<whitequark[cis]>
yes
<whitequark[cis]>
tightly coupled memory
<whitequark[cis]>
they could be a ROM too
<whitequark[cis]>
(we probably should handle those two cases completely separately, too)
<whitequark[cis]>
O_MASK is a weird one
<whitequark[cis]>
I'm not sure its existence is justified?
<jfng[m]>
yeah, i.. just wanted a set/clear csr tbh
<jfng[m]>
also the sticky behavior is, uh, exotic
<whitequark[cis]>
I think O_MASK is a mistake and you should add a set/clear CSR, like on STM32
<jfng[m]>
well it is directly inspired from BSRR in STM32
<whitequark[cis]>
that's not how BSRR works?
<jfng[m]>
except that the latter is write-only
<whitequark[cis]>
have you used it?
<whitequark[cis]>
BSRR actually changes the contents of ODR
<jfng[m]>
yeah, that what i'm trying to say
<whitequark[cis]>
it doesn't have storage
<jfng[m]>
mine doesn't, that is the "exotic" part i mentioned
<jfng[m]>
it's a part of the RFC i wasn't sure at all
<whitequark[cis]>
yeah, I think we definitely should not do that, and should stick to the well known and proven semantics
<whitequark[cis]>
I am unsure about I_SYNC
<jfng[m]>
i tried to shoehorn the RW1S/RW1C builtins for this, but i'll happily make them modify O_DATA instead
<jfng[m]>
what part of I_SYNC are you unsure about ?
<whitequark[cis]>
well, it's not really safe to bypass the synchronizer
<jfng[m]>
ESP32-C3 does it :p
<whitequark[cis]>
I think the existence of this feature should be motivated by industry examples, and more than just one startup doing it
<whitequark[cis]>
the GPIO peripheral we're adding now is going to be the bedrock of our methodology
<whitequark[cis]>
if we add weird features because Espressif did it into something so foundational it sends a wrong message, I think
<jfng[m]>
making the number of stages a build-time parameter will make the design simpler, so i'm fine with it
<whitequark[cis]>
I would propose sticking to the absolute basics: direction, output, input, output set by mask, output clear by mask
<whitequark[cis]>
and the number of stages is a build-time parameter defaulting to 2, but possible to set to 0, yes
<jfng[m]>
if someone wants configurable stages, they can just extend the GPIOs with such a feature, i guess
<whitequark[cis]>
the reason I propose direction and not mode is that additional modes in a SoC generally require integration with the platform that is simply not possible with this particular peripheral
<whitequark[cis]>
analog mode? not possible. drive strengths? not possible, and not even in our abstractions
<jfng[m]>
yeah, as i mention in later parts of the RFC, some of those features cannot be implemented in the proposed design
<whitequark[cis]>
I think they shouldn't be there if they can't be implemented
<whitequark[cis]>
there is a backward compatibility concern, but I think reserving just one extra mode bit isn't a good way to resolve it
<whitequark[cis]>
consider code that tries to be forward-compatible, like a bitbanging algorithm in assembly that needs to toggle the mode bit (for QSPI or whatever)
<whitequark[cis]>
if we make 10 into "output drive strength medium" and 11 into "output drive strength high" then setting the drive strength and handing it off to the binary algorithm won't work
<jfng[m]>
so if we combine this with say, a pin mux, we would need additional registers anyway
<jfng[m]>
so reserving bits in MODE doesn't bring much
<jfng[m]>
and we can then rename it to direction, or something
<whitequark[cis]>
I think the direction register can be just "direction for the specific case of digital pin mode"
<jfng[m]>
(we have a meeting)
<whitequark[cis]>
then, more complex GPIO peripherals can build off this basic one by adding more registers at the end
<whitequark[cis]>
oh right
balrog_ is now known as balrog
<whitequark[cis]>
jfng: okay, to continue the review
<whitequark[cis]>
for the pin count, I see no reason to limit it to 16 (I don't think 16 is even meaningful at all given that data_width is configurable?)
<whitequark[cis]>
it will generally be a better experience if the entirety of your bit set/reset register (which is 2x as wide as the output register) can be written in one word, but that's a documentation concern
<whitequark[cis]>
our infra allows you to write the entire BSRR in one cycle no matter how big it is, so we should take advantage of that
<whitequark[cis]>
a CPU core will usually work at several times the frequency GPIOs can even toggle at all, so it's fine that it takes several bus cycles to write to the peripheral
<jfng[m]>
right, it is just a matter of ordering the accesses which isn't a big ask
<jfng[m]>
ok, i'll remove the upper bound
<whitequark[cis]>
it is a core part of our programming model anyway
<cr1901>
Alternatively, when I add fixed point to smolarith, I could add a divide function for fixed point that's intended for simulation, and have chat/users critique until it's good.
<whitequark[cis]>
jfng: regarding naming: I have a concrete motivated proposal for register/field names
<whitequark[cis]>
in Python, we name things LikeThis and like_this. in Rust, we name things LikeThis and like_this. so let's commit to this particular convention
<whitequark[cis]>
for C, just run .uppercase() or something in the BSP generator, it's going to look slightly worse for names LikeThis (whether or not we insert underscores in between aA for it to become A_A), but whatever, C BSPs are always ugly anyway
<jfng[m]>
when naming registers and fields, the tension is also for the length of the name (e.g. OutputDataReg vs. ODR)
<whitequark[cis]>
true
<jfng[m]>
if we use abbreviations, then camel case and snake case are identical, no ?
<whitequark[cis]>
there are also enum variants
<jfng[m]>
whitequark[cis]: happy with adopting this convention otherwise
<whitequark[cis]>
in this case, the options become "LT" and "LikeThis", which is a much narrower decision space (which feels nice)
<zyp[m]>
<whitequark[cis]> "the reason I propose direction..." <- open drain modes could be useful to have and ties directly into the direction mechanism
<zyp[m]>
an alternative would be passing the pin itself to the GPIO peripheral instead of using connect, letting it learn the direction from the signature
<_whitenotifier-5>
[amaranth] github-merge-queue[bot] created branch gh-readonly-queue/main/pr-1164-f524dd041aff2ec18ffc3a2487fb4cc0e3719447 - https://github.com/amaranth-lang/amaranth
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] de23bc2 - Deploying to main from @ amaranth-lang/amaranth@85bb5ee77c75fa1dbfdcc1ca5fc7fdaa649a4375 🚀
<whitequark[cis]>
I think GP_IO_s should be using bidi pins only
<jfng[m]>
<zyp[m]> "an alternative would be passing..." <- i prefer connecting it (or assigning it manually), as it decouples the peripheral from amaranth platform infrastructure, which is probably going to change soon-ish
<whitequark[cis]>
re open drain, usually the way you implement open drain is by toggling the output enable
<zyp[m]>
yes
<whitequark[cis]>
but we have BSRR, which lets you nicely do cycle accurate bitbanging
<whitequark[cis]>
so I think it's worth having Mode: Input, PushPull, OpenDrain
<zyp[m]>
BSRR is orthogonal of open drain
<whitequark[cis]>
or I guess InputOnly, PushPull, OpenDrain
<whitequark[cis]>
zyp[m]: I don't think so?
<whitequark[cis]>
if you implement open drain by toggling mode between input and output, you can't use BSRR
<whitequark[cis]>
so by adding open drain as a mode, you can now use BSRR
<zyp[m]>
right
<zyp[m]>
agreed
<whitequark[cis]>
BSRR is really handy for bitbanging in a way I didn't fully understand before, I think if any of the "extra" GPIO features is important to have, it's that
<whitequark[cis]>
if you really want a "GPIO" which doesn't have an input or an output buffer, I think you can just manually connect it
<whitequark[cis]>
this does occasionally happen with multifunction pins and it's usually not worth changing the programming model radically to accommodate it
<whitequark[cis]>
so the peripheral doesn't need to care
<Wanda[cis]>
I think #377 can be closed?
<whitequark[cis]>
mm, I think we still have a problem there
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 51b6980 - Deploying to main from @ amaranth-lang/amaranth@c6bc9b47ef4d29f1d1fa340c3e0048deccd56ae0 🚀
<cr1901>
This isn't actionable right now, but I think there should be a lint for: if you did "with m.If(my_sig_with_intenum_shape == MyVariant & cond1 & cond2)", you probably meant "with m.If((my_sig_with_intenum_shape == MyVariant) & cond1 & cond2)"