<_whitenotifier>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 67ffa2a - Deploying to main from @ amaranth-lang/amaranth@db7e64960cb9e101dcddd38cea519dca63a2f371 🚀
<jfng[m]>
who else is attending ? @vegard_e:matrix.org @libera_cr1901:catircservices.org ?
<whitequark[cis]>
me
<jfng[m]>
the changes requested during the previous meeting have been made:
<jfng[m]>
> add a FieldAction base class for field action components;
<jfng[m]>
> rename FieldMap and FieldArray to FieldActionMap and FieldActionArray.
<galibert[m]>
I'm reading the current state, I have some questions
<galibert[m]>
What is the interaction with the bus width?
<jfng[m]>
the csr.Multiplexer hides the influence of the bus width, by ensuring that a register access is atomic
<galibert[m]>
so, if you mess up and create a 31-bits wide register and plonk it in the middle of your csr range, everything is shifted by one bit transparently?
<jfng[m]>
if you add a 31bit register to a csr multiplexer that has a 32bit data width, then its last bit is unwired
<jfng[m]>
iirc, i don't even know if that is the case
<galibert[m]>
so a register is padded to a multiple of the bus width?
<jfng[m]>
maybe it is an error, let me check
<whitequark[cis]>
I have one comment. tpw_rules, I think you wanted `<reg>.r_stb` and `<reg>.w_stb` and I was recorded in opposition to it, right?
<galibert[m]>
I have not found the information in the rfc, but I may have missed it
<tpw_rules>
whitequark[cis]: yes. specifically for csr.action.R and csr.action.W where access to port.r_stb and port.w_stb is necessary to use the action correctly. if not, at least document this fact
<whitequark[cis]>
tpw_rules: I think there was either a miscommunication or I made a bad decision, I think `<reg>.r_stb` is unambiguously better than `<reg>.f.<field>.port.r_stb` in peripheral code
<tpw_rules>
whitequark[cis]: i think i wanted `<reg>.f.<field>.w_stb` and you suggested `<reg>.f.<field>.port.w_stb` as the alternative
<whitequark[cis]>
hm
<tpw_rules>
i don't know if `<reg>.w_stb` exists now that i read more carefully
<whitequark[cis]>
that is weird because I thought I also wanted `<reg>.f.<field>.w_stb`
<tpw_rules>
i don't have any stake on whether or not you should be "able to assert writes on only some fields" (i think it's weird at first glance) but i do think that a) <reg>.f.<field>.* should contain all signals necessary to use it properly as designed and that b) <reg>.f.<field>.port.* access should be discouraged/forbidden from the peripheral
<whitequark[cis]>
I don't believe I ever suggested `<reg>.f.<field>.port.w_stb` because I fully agree with (b)
<whitequark[cis]>
that sounds like a miscommunication
<jfng[m]>
iirc, the rationale for `port.w_stb` was that it made it clear that it was directly driven from the bus
<tpw_rules>
okay. i think i mis-remembered "only `w_stb` on the register" as implying b)
<whitequark[cis]>
I'm happy going with (a)
<whitequark[cis]>
I am ambivalent between having w_stb only on the field's public interface or only on the register. I think (a) is a good argument in favor of the former
<jfng[m]>
and it wasn't a miscommunication, the subject was brought up in a previous meeting, there are logs of it
<galibert[m]>
jfng: it should be in the rfc, I think
<whitequark[cis]>
jfng: I can't find it in the logs
notgull has joined #amaranth-lang
<galibert[m]>
Second question, can I have an array of n registers of type X in the middle? Is there such a concept?
<whitequark[cis]>
what does it mean "in the middle"?
<galibert[m]>
I mean somewhere in the pile of registers your component has
<galibert[m]>
not necessarily at the start or the end
<tpw_rules>
(and implies that `<reg>.w_stb` does not exist)
<whitequark[cis]>
tpw_rules: yes, I definitely disagree with (b) and I cannot imagine suggesting it, which is why I think a miscommunication occurred. it's a blatant violation of the field's encapsulation
<whitequark[cis]>
galibert: the RFC doesn't define a register array as an explicit structure
<whitequark[cis]>
but you can define several registers "X0", "X1", ... of the same type easily
<tpw_rules>
are there any restrictions on names? could you define X[0]?
<whitequark[cis]>
Amaranth doesn't care but the BSP generators will probably be unhappy
<whitequark[cis]>
and whoever is tasked with using the generated code will definitely be unhappy and will probably implement a cursed yaml-based scheme to map things back to a register array
<whitequark[cis]>
jfng: I think the RFC should either have a provision for adding a register array or explicitly leave it for future work. SVD has support for it, right?
<galibert[m]>
well, when you have a bank of ten whatever, it's annoying not to be able to index :-)
<galibert[m]>
and a cluster array too
<tpw_rules>
yeah i've seen peripherals with "striped" arrays too, not sure if that's what you mean by cluster array
<whitequark[cis]>
I know it's a real pain in the downstream code when you don't have a way to index into an array of registers, and the workarounds are horrible
<galibert[m]>
tpw_rules: I mean what you do with add_cluster
<whitequark[cis]>
tpw_rules: you could make a striped array by using add_cluster, yeah
<galibert[m]>
Also, just to be sure I understand, the only way to create the final map is programatically through add_register/add_cluster
<jfng[m]>
<tpw_rules> "i don't have any stake on..." <- both points are valid from a usability point of view, i think; i'll look at the logs in detail afterwards to see if there was another counterpoint; but i am tempted to go with your suggestion otherwise
<galibert[m]>
but the recommended way to create registers is through annotations?
<jfng[m]>
it depends on your use-case
<jfng[m]>
variable annotations make it difficult to parameterize your register definition, similarly to lib.wiring
<jfng[m]>
galibert[m]: yes, it cannot be done declaratively
<jfng[m]>
not that i am against the idea; the decision was made for simplicity's sake
<tpw_rules>
jfng[m]: to expand a little, i specifically think csr.action.W must have w_stb because otherwise it's impossible to use correctly. if a FieldAction should imply r_stb and w_stb in addition to port, that might be another way, but could be weird for reserved fields (though they wouldn't get connected anyway).
<zyp[m]>
sorry, I'm joining a bit late, and I'm not sure I have the full context on the strobes, but my preference would be to have it available on both fields and registers
<whitequark[cis]>
I feel like they should be either on fields or on registers to reduce the mental workload for the reader, who is trying to understand whether these two do the same thing or not
<whitequark[cis]>
it is not obvious that they are the same at all
<whitequark[cis]>
I will not object to either having them on registers. I think I initially proposed them to be on fields and later advocated for them to be on registers
<whitequark[cis]>
* them on fields or on registers. I
<jfng[m]>
hmm
<cr1901>
I'm barely here, whatever everyone else decides I'm sure will be fine. I don't think I have a pref for stb on fields or registers
<tpw_rules>
okay, they are not currently on registers. they obviously won't be removed from elements. i propose they be on all fields with a doc note that it's a mirror of the element signal
<zyp[m]>
I figure that normally you'd want to trigger actions directly from the field, but if you've got e.g. a calculation that takes multiple fields as input, it'll read better triggered by the register than either of the fields
<tpw_rules>
that only matters if the fields are in the same register
<zyp[m]>
obviously
<galibert[m]>
hmmm, if you have a register build on say 4 byte-wide write-only fields, and the bus is 32/8, is the w_stb of the fields independant or are they all toggled even when the write is byte-sized?
<zyp[m]>
that's handled by the atomicity logic, right?
<whitequark[cis]>
yes
<tpw_rules>
yes, i don't think 32/8 is a concept here
<whitequark[cis]>
galibert: if you do 32/8, you have to write all four chunks of the register in order or the behavior is undefined
<whitequark[cis]>
(the current implementation triggers the actual write on the last chunk)
<galibert[m]>
that's... important to know
<whitequark[cis]>
for sure
<whitequark[cis]>
that's in the CSR bridge documentation, I think
<jfng[m]>
csr multiplexer documentation
<whitequark[cis]>
galibert: (I say it's undefined because if you race a write with an ISR writing to the same field, you can get really wacky behavior)
Hoernchen_ has quit [Ping timeout: 268 seconds]
Hoernchen has joined #amaranth-lang
<jfng[m]>
ok, what would be a good case *against* forwarding strobes on each field (that has the proper access mode, etc)
<tpw_rules>
zyp[m]: point being there's still a common situation where you'll have to combine multiple strobes
<galibert[m]>
so that means you're not supposed to have small mailboxes within the csr range
<tpw_rules>
i think it might not make sense for all actions is my only problem
<tpw_rules>
for example it feels dirty to me for csr.action.RW to have easily accessible strobes
<jfng[m]>
a point in favor forwarding them to fields is to reduce coupling between the register machinery and fields
<zyp[m]>
if I had to choose between strobes on fields or registers, I think I'd prefer registers, but I'd also like fields to have all relevant signals available on their own
<tpw_rules>
due to the data latency and the fact that RW feels something more like a variable store rather than a trigger to me
<zyp[m]>
having both lets me pick whichever is appropriate when I'm implementing registers
<zyp[m]>
tpw_rules: you still want a strobe that tells you when the field is updated
<jfng[m]>
by forwarding them to fields, you would have both in practice, as you could connect `reg.element.w_stb` to your logic, for example
<zyp[m]>
although, a strobe on action.RW should probably go active one cycle later than the strobe on the register
<tpw_rules>
if i could have everything, i would keep `reg.element.x_stb`, not have `reg.x_stb`, and have `reg.f.<field>.x_stb` where it made sense, and forbid `reg.f.<field>.port.x_stb` access from peripheral code
<zyp[m]>
is strobe timing properly thought though?
<whitequark[cis]>
you want to allow port.x_stb but ban element.x_stb?
<tpw_rules>
whitequark[cis]: i'm sorry, that's the exact opposite of what i wish for?
<whitequark[cis]>
er, sorry, swap the two
<zyp[m]>
«new value in the register» happens one cycle later than «new value on the bus», and which we want depends on the action
<whitequark[cis]>
my point is they're both private-ish
<tpw_rules>
i guess that is true
<jfng[m]>
zyp[m]: what about a read strobe on `action.RW` ? i don't think it should be delayed
<zyp[m]>
indeed
<jfng[m]>
as for the write strobe, users that need to register it can do so in their user logic, as long as we document it ofc
<jfng[m]>
so i'm in favor of leaving it comb
<tpw_rules>
would it be too much to say that if you want to do things with the strobe, you should make a custom acton
<tpw_rules>
action*
<whitequark[cis]>
that makes RW1C useless
<zyp[m]>
tpw_rules: yes, it needs to be possible to know a field register changed without making a custom action
<tpw_rules>
whitequark[cis]: i don't understand
<jfng[m]>
let's go with the addition of per-field w_stb/r_stb lines (that would be connected to the register if their access mode is compatible)
<jfng[m]>
which allows per-register or per-field strobes, depending on user preference
<tpw_rules>
zyp[m]: okay, i don't immediately see. if you need to compare with a previous value you could do that in your logic?
<whitequark[cis]>
tpw_rules: well, if you don't have a public strobe anywhere, why even have RW1C as a type? you'd never use it, you'd only write custom actions
<tpw_rules>
whitequark[cis]: you know it changed because the bit in .data changed?
<zyp[m]>
I don't think you'd typically use strobes on a RW1C field
<whitequark[cis]>
hm
<zyp[m]>
RW1C would be something like interrupt flags, that you mask against a RW register and feed to a level triggered interrupt input
<whitequark[cis]>
if you want to know when an RW1C field was cleared, you will need to do something like `with m.If(w_stb & ~w_data)`
<whitequark[cis]>
I guess you don't have w_data either and data is updated only on the next cycle
<tpw_rules>
for me, if i want just like a system variable i'd use an RW. if i want reading or writing to do something, i'd use R or W. maybe we need an RW that doesn't have internal storage. if you want read to equal write, that's a simple connection in your logic, and eliminates the ambiguity in the delay in the strobe
<jfng[m]>
RW1S would be for interrupt enables, or other kinds of mask, that would just be sampled when needed; so strobes wouldn't be needed here either, i think
<tpw_rules>
exactly
<whitequark[cis]>
the argument that the delay of the strobe is ambiguous is compelling to me
<whitequark[cis]>
so, the resolution would be: have strobes nowhere, leave the user to make a new custom field whenever they want a strobe?
<zyp[m]>
I think actions with a register should have a strobe when the register has a new value
<zyp[m]>
i.e. one cycle later than the bus strobe
<tpw_rules>
i think csr.action.R and csr.action.W need to pass through the element strobe
<jfng[m]>
whitequark[cis]: that is the current status quo (except the example in the rfc actually uses a strobe, but the wrong one (`port` instead of `reg.element`)
<tpw_rules>
i think there's room for an action which combines .R and .W without having internal storage like .RW but idk what i would call it, and it's really the same as having a custom action cause it exposes all port signals and does nothing to them
<whitequark[cis]>
ah right, action.R and action.W
<jfng[m]>
tpw_rules: that could work
<whitequark[cis]>
yeah, I think those two actions passing the strobes through and not having them elsewhere sounds good
<tpw_rules>
(but it might be nice in some circumstances to avoid encapsulating the logic)
<jfng[m]>
R and W would cover the kind of use cases where you want a side-effect, and therefore strobe signal is sampled
<whitequark[cis]>
strobe there is in-phase with data, so it's all good
<tpw_rules>
could W have a variant that reads back the last thing written, but doesn't expose it to the peripheral? that would cover zyp[m]'s use case i think. you'd still need your own storage in your logic but i think a synthesis tool could merge it
<jfng[m]>
we are 5mins to the expected end of the meeting
<whitequark[cis]>
that sounds really weird and difficult to explain in the docs
<zyp[m]>
I think we can leave out the storage strobe and introduce that with a later RFC if desired
<tpw_rules>
okay. that could be its own action type even
<jfng[m]>
do you think it is reasonable to vote now ?
<jfng[m]>
i'm happy with @libera_tpw_rules:catircservices.org proposal for R and W
<zyp[m]>
do we keep strobes available on register?
<tpw_rules>
they never were there
<whitequark[cis]>
I think it needs two amendments: (a) the R and W strobes as tpw_rules described, and (b) a provision for register arrays (and possibly cluster arrays)
<jfng[m]>
they are available through `.element`, so they were never and always there
<whitequark[cis]>
I think we will definitely need register/cluster arrays eventually, and they are in SVD. we don't strictly speaking have to have them in this RFC, but we may have to break the interface later if we don't think about them now
<tpw_rules>
i have various niggles about how the RFC is written but those can be discussed later if they even matter. as for substantive changes, the new FieldAction stuff looks great, and the only remainder for me personally is whitequark[cis]'s (a)
<zyp[m]>
what about field arrays?
<whitequark[cis]>
we have those
<whitequark[cis]>
tpw_rules: the reference docs are usually not verbatim copies of the RFC
<zyp[m]>
can we have field arrays that span registers?
<galibert[m]>
Fields arrays are complicated with the atomicity
<whitequark[cis]>
no
<jfng[m]>
whitequark[cis]: there would need to be a way to replicate a register map, this would require a similar mechanism as the Field/FieldAction feature
<zyp[m]>
e.g. stm32 adcs have two-three registers with 5x6 (or was it 6x5) fields each that all make up one field array
<zyp[m]>
what do we do if we want a field array longer than a single register?
<jfng[m]>
and a way to group registers declaratively, similarly to FieldMap/FieldArray
<whitequark[cis]>
jfng: yes, which is why I'm raising my concern now rather than deferring it for a future RFC
<whitequark[cis]>
it would be unfortunate to completely break every peripheral
<whitequark[cis]>
zyp: I think that's out of scope, really
<whitequark[cis]>
I think being able to atomically read out the entire ADC's contents is a great feature to have
<jfng[m]>
ok, then i am against merging the RFC today
<zyp[m]>
I'm inclined to agree, I don't see an obvious way to solve it in a sane manner
<whitequark[cis]>
and if you don't want atomicity you just use multiple registers
<jfng[m]>
as i suspect that this will require significant changes to RegisterMap
<whitequark[cis]>
I think for use case like ADC readout you generally want to have them all in a single register
<jfng[m]>
and possibly a replacement
<zyp[m]>
the stm32 adc example are the sample sequence config registers
<whitequark[cis]>
same difference? updating the configuration atomically seems valuable
<zyp[m]>
sounds unnecessarily expensive for something you only do when the peripheral is inactive
<whitequark[cis]>
if you are that concerned about a few flops, then you split it into multiple registers manually, yeah
<whitequark[cis]>
note that the flops that keep the intermediate values are actually shared by the multiplexer between different registers in the same bridge
<whitequark[cis]>
so it's less expensive than it first looks
<jfng[m]>
yes, the underlying assumption is that the initiator has full ownership of the peripheral
<jfng[m]>
i.e. accesses are *not* interleaved between initiators
<jfng[m]>
full ownership, until they don't anymore
<whitequark[cis]>
we have a goal of not generating gateware that is broken by default in relatively common edge cases, unless you actively prevent that
<whitequark[cis]>
even at expense of some silicon area
<whitequark[cis]>
also, I will now leave the discussion as I have other things to do after the meeting
<zyp[m]>
same, need to go put the kid to bed
<jfng[m]>
alas, a vote didn't happen
<jfng[m]>
thanks everyone, today's session had good results