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.mayd...@linaro.org>
> To: "Arnaud Minier" <arnaud.min...@telecom-paris.fr>
> Cc: "qemu-devel" <qemu-devel@nongnu.org>, "Thomas Huth" <th...@redhat.com>,
> "Laurent Vivier" <lviv...@redhat.com>, "Inès
> Varhol" <ines.var...@telecom-paris.fr>, "Samuel Tardieu"
> <samuel.tard...@telecom-paris.fr>, "qemu-arm"
> <qemu-...@nongnu.org>, "Alistair Francis" <alist...@alistair23.me>, "Paolo
> Bonzini" <pbonz...@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.min...@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.min...@telecom-paris.fr>
>> Signed-off-by: Inès Varhol <ines.var...@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
