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

[Arnaud Minier]

Thank you for the review and for the tips ! It really helps.
I will address the problems you have highlighted and will send a new version 
later this week.

Arnaud

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Arnaud Minier" <arnaud.minier@telecom-paris.fr>
> Cc: "qemu-devel" <qemu-devel@nongnu.org>, "Thomas Huth" <thuth@redhat.com>, 
> "Laurent Vivier" <lvivier@redhat.com>, "Inès
> Varhol" <ines.varhol@telecom-paris.fr>, "Samuel Tardieu" 
> <samuel.tardieu@telecom-paris.fr>, "qemu-arm"
> <qemu-arm@nongnu.org>, "Alistair Francis" <alistair@alistair23.me>, "Paolo 
> Bonzini" <pbonzini@redhat.com>
> Sent: Friday, February 23, 2024 3:59:03 PM
> Subject: Re: [PATCH v5 6/8] Add write protections to CR register

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

Sorry. This will be done for the next version.

> 
>>
>> 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);

It seems really convenient ! Will use them !

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

This will be changed to use clock_update.

> 
> thanks
> -- PMM