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