Re: [PATCH v5 6/8] Add write protections to CR register

[Peter Maydell]

On Mon, 19 Feb 2024 at 20:16, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> Add write protections for the fields in the CR register.
> PLL configuration write protections (among others) have not
> been handled yet. This is planned in a future patch set.

Can you make sure you include a suitable prefix (eg
"hw/misc/stm32l4x5_rcc: ") at the front of patch subjects, please?

>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  hw/misc/stm32l4x5_rcc.c | 164 ++++++++++++++++++++++++++++------------
>  1 file changed, 114 insertions(+), 50 deletions(-)
>
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index a3b192e61b..198c6238b6 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -346,9 +346,47 @@ static void rcc_update_irq(Stm32l4x5RccState *s)
>      }
>  }
>
> -static void rcc_update_cr_register(Stm32l4x5RccState *s)
> +static void rcc_update_msi(Stm32l4x5RccState *s, uint32_t previous_value)
> +{
> +    uint32_t val;
> +
> +    static const uint32_t msirange[] = {
> +        100000, 200000, 400000, 800000, 1000000, 2000000,
> +        4000000, 8000000, 16000000, 24000000, 32000000, 48000000
> +    };
> +    /* MSIRANGE and MSIRGSEL */
> +    val = extract32(s->cr, R_CR_MSIRGSEL_SHIFT, R_CR_MSIRGSEL_LENGTH);

registerfields.h provides macros for "extract a named field", so you
can write this
    val = FIELD_EX32(s->cr, CR, MSIRGSEL);

> +    if (val) {
> +        /* MSIRGSEL is set, use the MSIRANGE field */
> +        val = extract32(s->cr, R_CR_MSIRANGE_SHIFT, R_CR_MSIRANGE_LENGTH);

and these as val = extract32(s->cr, CR, MSIRANGE)
and so on.

> +    } else {
> +        /* MSIRGSEL is not set, use the MSISRANGE field */
> +        val = extract32(s->csr, R_CSR_MSISRANGE_SHIFT, 
> R_CSR_MSISRANGE_LENGTH);
> +    }
> +
> +    if (val < ARRAY_SIZE(msirange)) {
> +        clock_update_hz(s->msi_rc, msirange[val]);
> +    } else {
> +        /*
> +         * There is a hardware write protection if the value is out of bound.
> +         * Restore the previous value.
> +         */
> +        s->cr = (s->cr & ~R_CSR_MSISRANGE_MASK) |
> +                (previous_value & R_CSR_MSISRANGE_MASK);
> +    }
> +}
> +

> -    /* HSEON and update HSERDY */
> +    /*
> +     * HSEON and update HSERDY.
> +     * HSEON cannot be reset if the HSE oscillator is used directly or
> +     * indirectly as the system clock.
> +     */
>      val = extract32(s->cr, R_CR_HSEON_SHIFT, R_CR_HSEON_LENGTH);
> -    s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
> -            (val << R_CR_HSERDY_SHIFT);
> -    if (val) {
> -        clock_update_hz(s->hse, s->hse_frequency);
> -        if (s->cier & R_CIER_HSERDYIE_MASK) {
> -            s->cifr |= R_CIFR_HSERDYF_MASK;
> +    if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) != 0b10 &&
> +        current_pll_src != RCC_CLOCK_MUX_SRC_HSE) {
> +        s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
> +                (val << R_CR_HSERDY_SHIFT);
> +        if (val) {
> +            clock_update_hz(s->hse, s->hse_frequency);
> +            if (s->cier & R_CIER_HSERDYIE_MASK) {
> +                s->cifr |= R_CIFR_HSERDYF_MASK;
> +            }
> +        } else {
> +            clock_update_hz(s->hse, 0);

As I mentioned earlier, please avoid clock_update_hz() for
clock calculations if possible.

thanks
-- PMM