Re: [PATCH 5/7] hw/char/stm32l4x5_usart: Add options for serial parameters setting

[Peter Maydell]

On Sun, 17 Mar 2024 at 10:41, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> Add a function to change the settings of the
> serial connection.
>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  hw/char/stm32l4x5_usart.c | 97 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
> index 958d05a56d..95e792d09d 100644
> --- a/hw/char/stm32l4x5_usart.c
> +++ b/hw/char/stm32l4x5_usart.c
> @@ -165,6 +165,91 @@ static int stm32l4x5_usart_base_can_receive(void *opaque)
>      return 0;
>  }
>
> +static void stm32l4x5_update_params(Stm32l4x5UsartBaseState *s)
> +{
> +    int speed, parity, data_bits, stop_bits;
> +    uint32_t value, usart_div;
> +    QEMUSerialSetParams ssp;
> +
> +    /* Select the parity type */
> +    if (s->cr1 & R_CR1_PCE_MASK) {
> +        if (s->cr1 & R_CR1_PS_MASK) {
> +            parity = 'O';
> +        } else {
> +            parity = 'E';
> +        }
> +    } else {
> +        parity = 'N';
> +    }
> +
> +    /* Select the number of stop bits */
> +    value = FIELD_EX32(s->cr2, CR2, STOP);
> +    if (value == 0b00) {
> +        stop_bits = 1;
> +    } else if (value == 0b10) {
> +        stop_bits = 2;
> +    } else {

I think this would read a little more clearly as

     switch (FIELD_EX32(s->cr2, CR2, STOP)) {
     case 0:
          stop_bits = 1;
          break;
     case 2:
          stop_bits = 2;
          break;
     default:
          [error case code]
     }

rather than using an if-else ladder. Similarly below.

> +        /* TODO: raise an error here */
> +        stop_bits = 1;
> +        error_report(
> +            "UNIMPLEMENTED: fractionnal stop bits; CR2[13:12] = %x",
> +            value);

We generally use qemu_log_mask(LOG_UNIMP, ...) for
"this was a valid thing for the guest to do but our implementation
doesn't handle it", and qemu_log_mask(LOG_GUEST_ERROR, ...) for
"this was an invalid thing for the guest to do" (eg programming register
fields to reserved/undefined values), rather than using error_report().

> +        return;
> +    }
> +
> +    /* Select the length of the word */
> +    value = (FIELD_EX32(s->cr1, CR1, M1) << 1) | FIELD_EX32(s->cr1, CR1, M0);
> +    if (value == 0b00) {
> +        data_bits = 8;
> +    } else if (value == 0b01) {
> +        data_bits = 9;
> +    } else if (value == 0b01) {
> +        data_bits = 7;

These two arms both check against the same value, so one of them
must be wrong...

> +    } else {
> +        /* TODO: Raise an error here */
> +        data_bits = 8;
> +        error_report("UNDEFINED: invalid word length, CR1.M = 0b11");
> +        return;
> +    }
> +
> +    /* Select the baud rate */
> +    value = FIELD_EX32(s->brr, BRR, BRR);
> +    if (value < 16) {
> +        /* TODO: Raise an error here */
> +        error_report("UNDEFINED: BRR lesser than 16: %u", value);
> +        return;
> +    }
> +
> +    if (FIELD_EX32(s->cr1, CR1, OVER8) == 0) {
> +        /*
> +         * Oversampling by 16
> +         * BRR = USARTDIV
> +         */
> +        usart_div = value;
> +    } else {
> +        /*
> +         * Oversampling by 8
> +         * - BRR[2:0] = USARTDIV[3:0] shifted 1 bit to the right.
> +         * - BRR[3] must be kept cleared.
> +         * - BRR[15:4] = USARTDIV[15:4]
> +         * - The frequency is multiplied by 2
> +         */
> +        usart_div = ((value & 0xFFF0) | ((value & 0x0007) << 1)) / 2;
> +    }
> +
> +    speed = clock_get_hz(s->clk) / usart_div;
> +
> +    ssp.speed     = speed;
> +    ssp.parity    = parity;
> +    ssp.data_bits = data_bits;
> +    ssp.stop_bits = stop_bits;
> +
> +    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
> +
> +    trace_stm32l4x5_usart_update_params(
> +        speed, parity, data_bits, stop_bits, 0);

This is slightly weird indentation.

> +}
> +
>  static void stm32l4x5_update_irq(Stm32l4x5UsartBaseState *s)
>  {
>      if (((s->isr & R_ISR_WUF_MASK) && (s->cr3 & R_CR3_WUFIE_MASK))        ||
> @@ -318,16 +403,19 @@ static void stm32l4x5_usart_base_write(void *opaque, 
> hwaddr addr,
>      switch (addr) {
>      case A_CR1:
>          s->cr1 = value;
> +        stm32l4x5_update_params(s);
>          stm32l4x5_update_irq(s);
>          return;
>      case A_CR2:
>          s->cr2 = value;
> +        stm32l4x5_update_params(s);
>          return;
>      case A_CR3:
>          s->cr3 = value;
>          return;
>      case A_BRR:
>          s->brr = value;
> +        stm32l4x5_update_params(s);
>          return;
>      case A_GTPR:
>          s->gtpr = value;
> @@ -409,10 +497,19 @@ static void stm32l4x5_usart_base_init(Object *obj)
>      s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
>  }
>
> +static int stm32l4x5_usart_base_post_load(void *opaque, int version_id)
> +{
> +    Stm32l4x5UsartBaseState *s = (Stm32l4x5UsartBaseState *)opaque;
> +
> +    stm32l4x5_update_params(s);
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_stm32l4x5_usart_base = {
>      .name = TYPE_STM32L4X5_USART_BASE,
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = stm32l4x5_usart_base_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(cr1, Stm32l4x5UsartBaseState),
>          VMSTATE_UINT32(cr2, Stm32l4x5UsartBaseState),
> --

thanks
-- PMM