<cr1901>
whitequark[cis]: I know I said "Friday is best" (and for the most part, it is), but I won't be here for the SoC meeting tomorrow due to rescheduled prior trip.
<jfng[m]>
there is one missing bit: register clusters, which are prototyped and tested, but i missed the deadline to add them for today
<jfng[m]>
anyway
<jfng[m]>
TL;DR, before, register storage was an all-or-nothing decision
<jfng[m]>
now, users can make the decisions at the field granularity
<galibert[m]>
Nice
<jfng[m]>
in particular, the predefined RW,RW1C and RW1C fields have built-in storage
<jfng[m]>
whereas the R and W fields do not
<whitequark[cis]>
from my point of view, the RFC is fundamentally done, in that it will fulfill any use cases I can think of after the last round of changes
<whitequark[cis]>
I have some observations on naming or minor stylistic ones that I will reserve until there is consensus on the core of the RFC
<galibert[m]>
Yeah, probably at the point where the only serious remarks will only come from actual experience in real projects
<jfng[m]>
an added benefit is that GenericField no longer imposes any constraint on the implementation of a field (storage, set/clear, etc)
<jfng[m]>
everything is done in elaborate, at the user discretion
<jfng[m]>
galibert[m]: we just discussed it w/ Catherine 1h ago, and we believe that the RFC should wait until a simple peripheral (e.g. GPIO driver) is implemented with it, before making the final decision
<galibert[m]>
Makes sense
<jfng[m]>
the RFC is big enough, that it is not easy to tell if it will indeed survive contact with reality
<jfng[m]>
the other change (not visible here, sorry!), is a `Cluster` object, to group registers
<jfng[m]>
this is a pure metadata class, which serves as a container for CSR registers
<jfng[m]>
the csr.Multiplexer primitive now has an add_cluster() method, in addition to the current add() (for individual registers)
<jfng[m]>
i see two benefits for this addition:
<jfng[m]>
- it allows registers to be grouped logically, without committing too early to a specific multiplexer/decoder implementation
<jfng[m]>
- it allows some constraints (e.g. access mode) to be enforced on a group of registers, so if you a RW register to a RO cluster, it will raise an error
<Chips4MakersakaS>
To be sure I follow; for a W register one can still ask for storage and ignore w_stb; e.g. only use data ?
<jfng[m]>
for the pre-defined a R or a W *field*, there is no built-in storage
<jfng[m]>
however, a user can make a write-only field by subclassing GenericField and implementing such storage in elaborate
<Chips4MakersakaS>
OK for me.
<jfng[m]>
the underlying assumption is that in most cases, a read-only or write-only field does not have storage
<jfng[m]>
as opposed to read/write fields
<jfng[m]>
s/a//, s/a//, s/*field*,/_field_,/
<whitequark[cis]>
<jfng[m]> "the RFC is big enough, that it..." <- from my experience, at large ASIC design companies, the CSR stuff is implemented in essentially the same way as what this RFC proposes
<jfng[m]>
the part of the RFC where i'm slightly worried is the interface between fields and peripherals, as the interactions can have many forms
<jfng[m]>
we do have a very effective escape hatch thanks to deferring policy to elaborate(), but still
<jfng[m]>
i guess it won't be the end of the world if, later on, we have to update our built-in field definitions
<whitequark[cis]>
yeah, that would be fine, and easy to do with a small follow up RFC
<jfng[m]>
any further question/comment about these changes ?
<Chips4MakersakaS>
about storage for RW field. Is data if field valid for the periphersl when w_stb is not asserted; e.g. is it the stored value ?
<jfng[m]>
so, for RW fields, data is always valid; it is the output of the register storage
<jfng[m]>
when w_stb is high, data holds the new value (given by w_data) on the next clock cycle
<Chips4MakersakaS>
is data the new value or the previous stored value when w_stb is asserted ?
<jfng[m]>
on the same clock cycle as `w_stb`: the previous one
<jfng[m]>
on the next clock cycle after `w_stb`: the new one
<jfng[m]>
also, r_data and data are the same value in RW fields
<Chips4MakersakaS>
so this is different behaviour than the W register ?
<jfng[m]>
indeed
<jfng[m]>
the W register does not have storage
<jfng[m]>
so data holds the same value as w_data
<jfng[m]>
W field, by the way
<Chips4MakersakaS>
I don't know that I like it that data is still the old value for RW when w_stb is asserted; I don't think the peripheral can than do much with this w_stb field.
<jfng[m]>
w_stb would be sampled by the peripheral to be notified of updates to data
<jfng[m]>
i guess the peripheral will often need to register the w_stb value for one cycle, or something
<whitequark[cis]>
we could make w_data and w_stb be valid on the same cycle, and data trail
<Chips4MakersakaS>
That would work.
<jfng[m]>
can you rephrase ? not sure i understood
<jfng[m]>
as in, bypass the value of data when w_stb is high ?
<whitequark[cis]>
forward w_stb+w_data from the field port
<jfng[m]>
still not sure i understand
<jfng[m]>
as in, if `w_stb` is high, then `w_data` is forwarded to `data` on the same cycle; otherwise, `data` outputs the stored value ?
<whitequark[cis]>
no, I mean the field has data (the register output) as well as w_stb+w_data
<jfng[m]>
ah, i think that's already the case
<jfng[m]>
from the peripheral side, you have access to all three signals
<whitequark[cis]>
but w_data and data mean different things
<whitequark[cis]>
* but w_data and data would mean different things
<jfng[m]>
yes, "current" and "next" value, when w_stb is high
<jfng[m]>
(in the opposite order)
<whitequark[cis]>
yes
<Chips4MakersakaS>
OK, then. I would even think of leaving out data field from W register.
<whitequark[cis]>
I think data is only for RW registers?
<Chips4MakersakaS>
Current RFC says it
<jfng[m]>
it's also in W fields, but it's just wirse driven by w_data
<Chips4MakersakaS>
s slice of w_data for write only register.
<jfng[m]>
* it's also in W fields, but it's just wires driven by w_data
<jfng[m]>
yeah, we could just remove it in `W` fields, i guess
<jfng[m]>
i left it in order to have a modicum of uniformity among fields
<Chips4MakersakaS>
Forgive my typing, saying same as jfng...
<whitequark[cis]>
I think it's best to design fields in a way that's the least confusing, over uniformity
<jfng[m]>
ok, so data is only for RW* fields, whereas R and W fields are directly driven by r_data/w_data
<Chips4MakersakaS>
+1
<jfng[m]>
this reduces the interface of a `GenericField` to a `FieldPort`, and that's it
<jfng[m]>
which is nicer, i guess
<whitequark[cis]>
yes, I like that
<jfng[m]>
time is running out, thanks for the feedback !
<jfng[m]>
i guess we can say this RFC has reached some degree of consensus
<jfng[m]>
so the next steps will be to add the missing bits (just a matter of 1-2h); then we'll see how it holds up with an actual peripheral
<jfng[m]>
i don't mind if it sits here in the meantime, so long as we are confident in its contents
<whitequark[cis]>
sounds good to me
<jfng[m]>
in any case, comments or questions on the GH issue are always welcome
<Chips4MakersakaS>
Looks good indeed and I approve,nice wrok jfng
<whitequark[cis]>
I agree; I'm quite happy to see the RFC in its current form
<jfng[m]>
thanks!
<jfng[m]>
whitequark[cis]: its underlying ideas and the direction it sets for amaranth-soc were direct results of discussions with you, so thank you !
vegard_e[m] has quit [Quit: Idle timeout reached: 172800s]