Re: [PATCH v1] rust/pl011: Fix DeviceID reads

[Paolo Bonzini]

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?

>           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.

Thanks,

Paolo

>               Err(_bad_offset) => {
>                   eprintln!("write bad offset {offset} value {value}");
>               }
> diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
> index cd0a49acb9..1f305aa13f 100644
> --- a/rust/hw/char/pl011/src/lib.rs
> +++ b/rust/hw/char/pl011/src/lib.rs
> @@ -111,6 +111,22 @@ pub enum RegisterOffset {
>       /// DMA control Register
>       #[doc(alias = "UARTDMACR")]
>       DMACR = 0x048,
> +    #[doc(alias = "UARTPeriphID0")]
> +    PeriphID0 = 0xFE0,
> +    #[doc(alias = "UARTPeriphID1")]
> +    PeriphID1 = 0xFE4,
> +    #[doc(alias = "UARTPeriphID2")]
> +    PeriphID2 = 0xFE8,
> +    #[doc(alias = "UARTPeriphID3")]
> +    PeriphID3 = 0xFEC,
> +    #[doc(alias = "UARTPCellID0")]
> +    PCellID0 = 0xFF0,
> +    #[doc(alias = "UARTPCellID1")]
> +    PCellID1 = 0xFF4,
> +    #[doc(alias = "UARTPCellID2")]
> +    PCellID2 = 0xFF8,
> +    #[doc(alias = "UARTPCellID3")]
> +    PCellID3 = 0xFFC,
>       ///// Reserved, offsets `0x04C` to `0x07C`.
>       //Reserved = 0x04C,
>   }
> @@ -137,7 +153,11 @@ const fn _assert_exhaustive(val: RegisterOffset) {
>                   }
>               }
>           }
> -        case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, 
> ICR, DMACR }
> +        case! {
> +            DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, 
> ICR, DMACR,
> +            PeriphID0, PeriphID1, PeriphID2, PeriphID3,
> +            PCellID0, PCellID1, PCellID2, PCellID3,
> +        }
>       }
>   }
>   
>
> base-commit: 43f2def68476697deb0d119cbae51b20019c6c86