On Sat, Nov 16, 2024 at 10:24 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On 11/16/24 19:15, Manos Pitsidianakis wrote:
> > impl DeviceId {
> > - const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d,
> > 0xf0, 0x05, 0xb1];
> > - const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d,
> > 0xf0, 0x05, 0xb1];
> > + /// Value of `UARTPeriphID0` register, which contains the
> > `PartNumber0` value.
> > + const fn uart_periph_id0(self) -> u64 {
> > + 0x11
> > + }
> > +
> > + /// Value of `UARTPeriphID1` register, which contains the `Designer0`
> > and `PartNumber1` values.
> > + const fn uart_periph_id1(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x10,
> > + Self::Luminary => 0x00,
> > + }) as u64
> > + }
> >
> > +
> > + /// Value of `UARTPeriphID2` register, which contains the `Revision`
> > and `Designer1` values.
> > + const fn uart_periph_id2(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x14,
> > + Self::Luminary => 0x18,
> > + }) as u64
> > + }
> > +
> > + /// Value of `UARTPeriphID3` register, which contains the
> > `Configuration` value.
> > + const fn uart_periph_id3(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x0,
> > + Self::Luminary => 0x1,
> > + }) as u64
> > + }
>
> If I understand correctly, these are two reasons why you did not go with
> the simple adjustment to the Err(v) pattern:
>
> * the separate registers match the datasheet more
>
> * given the bug that you are fixing in the write function, you want to
> avoid duplicating "Err(v) if (0xfe0..=0xffc).contains(&v)" between read
> and write; instead, you rely on exhaustive enums for error checking.
>
> I wonder if we can keep these improvements while making the
> implementation a bit more concise... The eight const getter functions
> are quite verbose, and having the device type match inside each function
> is repetitive and hard to verify. Maybe something like
>
> const UART_PCELL_ID: [u8; 4] = [0x0d, 0xf0, 0x05, 0xb1];
> const ARM_UART_PERIPH_ID: [u8; 4] = [0x11, 0x10, 0x14, 0x00];
> const LUMINARY_UART_PERIPH_ID: [u8; 4] = [0x11, 0x00, 0x18, 0x01];
>
> /// Value of `UARTPeriphID0` through `UARTPeriphID3` registers
> const fn uart_periph_id(&self, idx: usize) -> u8 {
> match self {
> Self::Arm => ARM_UART_PERIPH_ID,
> Self::Luminary => LUMINARY_UART_PERIPH_ID,
> }[idx]
> }
>
> /// Value of `UARTPCellID0` through `UARTPCellID3` registers
> const fn uart_pcell_id(idx: usize) -> u8 {
> Self::UART_PCELL_ID[idx]
> }
>
> could be the best of both worlds?
Eh, there's no real reason to do that though. I prefer verbosity and
static checking with symbols rather than indexing; we're not writing C
here.
>
> > match RegisterOffset::try_from(offset) {
> > + Ok(PeriphID0) | Ok(PeriphID1) | Ok(PeriphID2) | Ok(PeriphID3)
> > | Ok(PCellID0)
> > + | Ok(PCellID1) | Ok(PCellID2) | Ok(PCellID3) => {
> > + // Ignore writes to read-only registers.
> > + }
>
> This is indeed an improvement over the patches that Junjie and I had
> sent, because the writes would have gone down the eprintln! path.
I will send a v2 to print them anyway as guest errors like Peter requested.
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
