<fl4shk[m]>
because I was hoping to switch to Amaranth's built-in Structs and things
<fl4shk[m]>
and Components
crzwdjk has joined #amaranth-lang
<crzwdjk>
By the way, I found an example in my own codebase of an Elaboratable that wouldn't be a Component (i.e. it wouldn't have a signature). The PLL wrapper class, which only wires up the clock and reset signals for a clock domain.
<whitequark>
hi everyone, it is time for our weekly meeting
<whitequark>
jfng: is your RFC in a way you would like to be discussed? if not we shouldn't do it today
<whitequark>
JF, could I ask you to introduce the RFC, clarify the scope of discussion, and conduct the meeting to the best of your ability?
<jfng[m]>
let's try
<jfng[m]>
as you may know, amaranth-soc is a library to users design system-on-chips
<jfng[m]>
amaranth-soc users may want to expose a PHY (e.g. an UART) to CPU cores of a given SoC
<jfng[m]>
to do so, the UART is encapsulated in a peripheral, whose interface with a CPU would be a set of CSR registers (Control and Status Register)
<jfng[m]>
amaranth-soc already has support for a CSR bus, that allows a set of registers to be exposed under a common bus interface
<jfng[m]>
however, it lacks the notion of a CSR register, which must currently be implemented by users
<jfng[m]>
this is a problem for two reasons:
<jfng[m]>
1) every user of amaranth-soc must provide their own register implementation
<jfng[m]>
2) amaranth-soc has no understanding of register fields, which provide crucial information that is needed by our BSP generator to provide safe accessors, documentation, etc to a firmware running on the SoC
<jfng[m]>
so this RFC tries to solve this by providing a customizable register class to its users
<jfng[m]>
a user may define its own registers by subclassing the csr.Register class, to define fields
<zyp[m]>
if you only need one instance of a given register type, what's the benefit of subclassing and calling super().__init__ vs just instancing csr.Register directly with the same argument?
<jfng[m]>
you won't necessarily have only one instance of a register type
<jfng[m]>
for example, an interrupt controller may instantiate a set of registers for every peripheral, or target ?
<jfng[m]>
Also, it allows a concise and readable syntax using annotations
<zyp[m]>
I'm not sure why it being an elaboratable is an argument for subclassing
<whitequark>
hm, that's actually a good point
<whitequark>
in my mind, subclassing was useful because then you get access to class variable annotations, and can (if you want) define your own behavior for the registers
<whitequark>
however it is not strictly required for base functionality
<jfng[m]>
readability is a strong selling point for subclassing, imo
<jfng[m]>
rather, would there be a reason not to ?
<zyp[m]>
yes, I absolutely see advantages of being able to subclass it, just not the advantage of forcing you to subclass it
<jfng[m]>
oh, i think you aren't forced
<jfng[m]>
you could always do csr.Register(FieldMap({"foo": csr.field.R(8)}))
<zyp[m]>
right
<jfng[m]>
but yeah, noted. we shouldn't prevent this use case
<zyp[m]>
I'm jumping forward a bit: I see there's ResRW0 and ResRWL to distinguish how to treat reserved bits -- what's the default for the upper bits that's not covered by any fields in a register that doesn't fill the entire width?
<jfng[m]>
yes, we haven't started covering them, but i can answer
<jfng[m]>
if you have say, an 8-bit register that is part of a group of registers aligned on a 32-bit boundary
<jfng[m]>
then its upper 24-bits are unimplemented
<jfng[m]>
they would be considered as reserved, as it may be tempting for future silicon to give them a meaning
<fl4shk[m]>
Yeah, that's a good point
<jfng[m]>
but on the register side, we have no way of knowing that
<jfng[m]>
however, the BSP generator would have that information, and would advertise those bits as a reserved field, `ResRWL` probably
<jfng[m]>
but yeah, let's go through the notion of register fields
<zyp[m]>
IMO ResRW0 would be preferable, otherwise every write has to be a RMW
<cr1901>
why does every write have to RMW otherwise?
<jfng[m]>
zyp: ResRWL is the least constraining of the two, from the point-of-view of forward compatibility
<zyp[m]>
yes, but it's the most expensive, runtime wise
<cr1901>
oh, "b/c the RFC says so"
<jfng[m]>
cr1901: because if you are accessing other fields in the register, you need to preserve the value of this one; an RMW cycle is the only way to do so
<jfng[m]>
but let's go over fields, before discussing this
<jfng[m]>
fields come in two flavors:
<jfng[m]>
- ordinary fields e.g. `field.R()`, `field.W()`, `field.RW()`
<jfng[m]>
- flag fields e.g. `field.RW1C()`, `field.RW1S()`
<jfng[m]>
they are defined by their access mode, from the point of view of the bus initiator (e.g. CPU) and from the point of view of the peripheral
<jfng[m]>
a field.RW() is read-write by a CPU, but read-only for a peripheral
<jfng[m]>
this is an important restriction to avoid having to arbitrate between simultaneous write from the bus and from the peripheral side
<jfng[m]>
s/write/writes/
<cr1901>
Setting a timer value seems appropriate for "read/write from both sides"
<jfng[m]>
in this RFC, we use the notion of "ownership" to describe access modes: whoever can owns a field can mutate its state
<zyp[m]>
I think it's a good rule of thumb, but it may be too restrictive if you're implementing an already defined register interface
<jfng[m]>
cr1901: this would happen using different registers, one to set/reload the timer, and another to read its value
<jfng[m]>
there is one significant exception to this rule: flag fields
<jfng[m]>
typically, an interrupt pending bit (edge-triggered), or a sticky error bit
<cr1901>
So if you have a 16-bit timer, you need a 17-bit register... one for the current timer value (not a flag), and another flag to say "hi, I need the timer value to be overwritten"
<zyp[m]>
and you're also prohibiting tx data and rx data sharing a register, like it's common for UART register interfaces to do
<jfng[m]>
zyp: they may share the same register, as different fields
<jfng[m]>
one would be connected to say, a FIFO sink, and the other to a FIFO source
<zyp[m]>
what I mean is that they're not only two fields sharing a register, they're overlapping
<cr1901>
You can't make an 16550 compat UART that way (2 8-bit fields laid out in parallel, rather than 2 8-bit fields multiplexed by R/W signal)
<jfng[m]>
this RFC prevents such a case, indeed
<jfng[m]>
unless
<jfng[m]>
you use an escape hatch, the GenericField class, and then anything goes
<jfng[m]>
you may have your way
<zyp[m]>
I think it could make sense to add a field type that doesn't have internal storage, but exposes read/write data signals and strobes
<zyp[m]>
ah, it already exists?
<jfng[m]>
yeah, the idea is that every field in csr.field.* inherits from GenericField
<cr1901>
I don't have a complaint if it's difficult to make muxed R/W regs. As long as it's possible
<zyp[m]>
agreed
<jfng[m]>
but if for some reason users need something that isn't covered here, they may simply define their own fields, inheriting from GenericField
<cr1901>
Also ignore the 17-bit register part... I was confusing myself w/ the "disable timer" example flag in the RFC
<jfng[m]>
but yeah, the key point is that the only case where this RFC allows shared ownership of a field, is for flag fields such as field.RW1C, field.RW1S
<jfng[m]>
such fields have very restrictive behaviors
<jfng[m]>
an RW1C can only be set by a CPU, and can only be cleared by a peripheral
<cr1901>
Maybe MuxedRW reg should be gated behind a marker class saying "I know I'm bypassing the API"?
<jfng[m]>
and the other way around for RW1S flag fields
<cr1901>
I need to go for a bit, but I've no showstopping complaints. I'm not exactly convinced the forward compat of ResRWL is worth having a RMW write all the time. If you're using Rust to interface to your peripheral, for instance, I doubt you'll be distributing a library.
<jfng[m]>
that marker would be... directly inheriting from GenericField ? it is easy for amaranth-soc to just recognize a custom field that it didn't provide itself
<cr1901>
fair
<zyp[m]>
on field offsets and reserved bits: I kinda would prefer to have the ability to specify the offset explicitly per field, rather than having to manually add up widths
<zyp[m]>
and then instead of padding with reserved fields, just set the default reserved type for the whole register
<zyp[m]>
having the offsets explicit would make it harder to accidentally unintentionally shift some of the bits by messing up the padding when adding fields
<jfng[m]>
i believe the contrary, as it forces the designer to exhaustively cover the register bits (minus, the case were upper bits aren't defined, that we discussed earlier)
<jfng[m]>
also, reserved fields may have different expected uses
<jfng[m]>
an important aspect of flag fields, is that writing 0 wil always be a no-op
<jfng[m]>
whether it is RW1C or RW1S
<jfng[m]>
a reserved field that is expected to hold flags in the future can use RW0, and software libraries can just write 0 without having to RMW
<zyp[m]>
yes, from a firmware perspective I'm very much in favor of avoiding the need for RMWs
<zyp[m]>
ResRW0 wouldn't only apply to RW1* fields, but also every potential RW field with a reset value of 0
<jfng[m]>
i understand the interest for using offsets, as it would e.g. be less error-prone when defining a register from the contents of a datasheet, where you would just give a width and an offset
<jfng[m]>
but i think the tradeoff is worth it
<jfng[m]>
zyp[m]: fair point, but i guess a register with a reserved `RW` field would probably already be accessed with an RMW cycle ?
<jfng[m]>
as in, it would have other RW fields
<zyp[m]>
you won't need a RMW if you're writing all the known fields at once
<zyp[m]>
which is typical for config registers
<jfng[m]>
yes, but that's not always the case
<jfng[m]>
hmm
<zyp[m]>
when I'm e.g. setting up a SPI peripheral, I don't care what it was configured to before, I'm typically setting the exact configuration I want now
<jfng[m]>
i'm not sold on ResRWL, actually, including the case where upper bits are non-existent, e.g. an 8-bit register aligned on a 32-bit boundary
<jfng[m]>
this may mean that a significant amount of registers in a cluster require an RMW to be accessed
<jfng[m]>
where they would otherwise be e.g. read-only
<jfng[m]>
s/read/write/
<jfng[m]>
but the meeting is running overtime
<zyp[m]>
I think `ResRWL` is good to have when you explicitly expect you might add a field with a reset value that you want to be preserved, not overwritten, but `ResRW0` is the more efficient default that probably covers >90% of cases
<zyp[m]>
also, on offsets, could it be possible to have an optional offset argument? it's useful even if it's only used for error checking
<jfng[m]>
having user-defined offset widens considerably the possible definitions of a csr.FieldMap, and would make its implementation more complicated
<jfng[m]>
error-checking is interesting, however
<jfng[m]>
that's not a bad idea
<whitequark>
zyp: I proposed ResRWL to be the default, because this lets you safely add new fields in a next chip revision, without having to rebuild software libraries
<zyp[m]>
yes, I get the point, my argument is that ResRW0 is safe if you add fields with a reset value of 0, which most fields tend to be
<whitequark>
not if your library inlines accessors from your CSP and resets these fields to 0 any time it wants to change some other field
<whitequark>
the difference between RW0 and RWL is really only important for this particular case
<whitequark>
well, aside from chicken bits and such
<zyp[m]>
I figure if firmware doesn't know about the field, it'll remain at its reset value in any case, and if part of the firmware is built to know about it, the whole ought to be
<zyp[m]>
hmm
<zyp[m]>
thinking about it, it could also be useful to define a «reserved, write whatever» type, for fields that are explicitly ignored
<zyp[m]>
e.g. for peripherals with optional functionality
<whitequark>
that doesn't win you anything in software I think
<zyp[m]>
(think e.g. stm32 timers)
<whitequark>
over RW0
<whitequark>
ohhh I se
<whitequark>
* ohhh I see
<whitequark>
yes, that is useful
<jfng[m]>
so, if you look at the reference implementation of this RFC, the idea is that for an RW0 field, whatever a bus initiator writes, it will be changed into a 0
<jfng[m]>
and whatever a bus initiator writes to an RWL field, it will preserve the value
<zyp[m]>
«it» being the field?
<whitequark>
RWA (any) is more relaxed than either of those
<jfng[m]>
but that's an implementation detail, software should write a 0, or the last value
<whitequark>
since it does not constrain either initiator or target
<zyp[m]>
I didn't expect a reserved field to have any storage at all
<jfng[m]>
"it" being the stored contents of a register field
<whitequark>
jfng: actually, yeah, I think you've got the RWL contract wrong?
<whitequark>
RW* covers the contract for the initiator
<jfng[m]>
zyp: they do, but i guess we'll go over them next time
<jfng[m]>
Catherine: ah ?
<whitequark>
what the initiator should and should not do
<whitequark>
it doesn't constrain the behavior of the target in any way since it's reserved
<zyp[m]>
also, so far we've only discussed writes, we should also discuss reads
<jfng[m]>
but, those fields may end up as RW, right ? then they will come with restrictions to the target (i.e. read-only)
<zyp[m]>
as in, there ought to be one that says «you can expect these bits to always read as 0»
<zyp[m]>
RW0 might imply that
<jfng[m]>
jfng[m]: that's why a `ResRWL` field isn't writable by the peripheral
<zyp[m]>
from a firmware developer perspective, when reading e.g. UART_DR, I don't want to have to always mask the upper bits when the register could just promise that the upper part always reads 0
<zyp[m]>
maybe at some point in the future, an 8-bit UART gets extended to support 9-bit frames or whatever, but that doesn't matter as long as the 9th bit remains 0 unless 9-bit mode is enabled
<jfng[m]>
the problem with "write any, read zero", is that future revisions of the silicon can make no assumptions about existing software drivers; they can't redefine the field, because a legacy library may write whatever to it
<zyp[m]>
I agree, write any should read undefined
<zyp[m]>
and write last should also read undefined
<jfng[m]>
my point is that a reserved field cannot be "write any"
<zyp[m]>
read what I wrote above, about optional fields
<zyp[m]>
stm32 timers is a good example, because there's a bunch of them with different feature levels and they share a common superset register interface
<zyp[m]>
e.g. some of them have a «master output enable» bit that needs to be set to e.g. output pwm
<zyp[m]>
you can simplify code by always setting the bit, and just relying on it to be ignored on the instances that doesn't implement it
<jfng[m]>
hmm, i guess "write-any, read whatever", would just be a read-only field here ?
<jfng[m]>
in timers that don't implement the feature
<zyp[m]>
yeah, I think it always reads 0 if not implemented
<jfng[m]>
i guess it would be a bit weird, but a designer may just use a field.R here
<jfng[m]>
actually, in my UART example in the RFC, the RxStatus field is a good example
<jfng[m]>
its a status reg, it is readonly; reserved fields would also be read-only
<jfng[m]>
s/field/register/
<zyp[m]>
sure, on the gateware side the storage for the unused bit gets optimized out, so that's fine
<zyp[m]>
but that goes for all reserved fields
<jfng[m]>
well no, as Catherine reminded me a few days ago, toolchains don't tend to optimize away registers
<zyp[m]>
the whole distinction between RW0, RWL and RWA is what the firmware is expected to do
<jfng[m]>
but the question of storage should be on topic for the next meeting, we are 40mins late
<jfng[m]>
alright, this wraps up the meeting i guess
<jfng[m]>
thanks !
<whitequark>
thanks everyone for this meeting! I missed some of it, esp. the part after the cutoff time, I'll catch up later
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 9f33cd4 - Deploying to main from @ amaranth-lang/amaranth@e6d8d5a3547747c1fdf9b955e8713b36e81f2fb0 🚀