Re: [PATCH 06/14] hw/misc/bcm2835_cprman: implement PLLs behaviour

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 9/25/20 12:17 PM, Luc Michel wrote:
> The cprman PLLs generate a clock based on a prescaler, a multiplier and
> a divider. The prescaler doubles the parent (xosc) frequency, then the
> multiplier/divider are applied. The multiplier has an integer and a
> fractionnal part.

Typo "fractional".

> 
> This commit also implements the cprman CM_LOCK register. This register
> reports which PLL is currently locked. We consider a PLL has being
> locked as soon as it is enabled (on real hardware, there is a delay
> after turning a PLL on, for it to stabilise).

Typo "stabilize".

> 
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>  include/hw/misc/bcm2835_cprman_internals.h |  8 +++
>  hw/misc/bcm2835_cprman.c                   | 64 +++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/bcm2835_cprman_internals.h b/include/hw/misc/bcm2835_cprman_internals.h
> index 5cfa849492..22a2500ab0 100644
> --- a/include/hw/misc/bcm2835_cprman_internals.h
> +++ b/include/hw/misc/bcm2835_cprman_internals.h
> @@ -98,10 +98,18 @@ REG32(A2W_PLLA_FRAC, 0x1200)
>  REG32(A2W_PLLC_FRAC, 0x1220)
>  REG32(A2W_PLLD_FRAC, 0x1240)
>  REG32(A2W_PLLH_FRAC, 0x1260)
>  REG32(A2W_PLLB_FRAC, 0x12e0)
>  
> +/* misc registers */
> +REG32(CM_LOCK, 0x114)
> +    FIELD(CM_LOCK, FLOCKH, 12, 1)
> +    FIELD(CM_LOCK, FLOCKD, 11, 1)
> +    FIELD(CM_LOCK, FLOCKC, 10, 1)
> +    FIELD(CM_LOCK, FLOCKB, 9, 1)
> +    FIELD(CM_LOCK, FLOCKA, 8, 1)
> +
>  /*
>   * This field is common to all registers. Each register write value must match
>   * the CPRMAN_PASSWORD magic value in its 8 MSB.
>   */
>  FIELD(CPRMAN, PASSWORD, 24, 8)
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index ad71d30a86..ba82522eb1 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -48,13 +48,51 @@
>  #include "hw/misc/bcm2835_cprman_internals.h"
>  #include "trace.h"
>  
>  /* PLL */
>  
> +static bool pll_is_locked(const CprmanPLLState *pll)
> +{
> +    return !FIELD_EX32(*pll->reg_a2w_ctrl, A2W_PLLx_CTRL, PWRDN)
> +        && !FIELD_EX32(*pll->reg_cm, CM_PLLx, ANARST);
> +}
> +
>  static void pll_update(CprmanPLLState *pll)
>  {
> -    clock_update(pll->out, 0);
> +    uint64_t freq, ndiv, fdiv, pdiv;
> +
> +    if (!pll_is_locked(pll)) {
> +        clock_update(pll->out, 0);
> +        return;
> +    }
> +
> +    pdiv = FIELD_EX32(*pll->reg_a2w_ctrl, A2W_PLLx_CTRL, PDIV);
> +
> +    if (!pdiv) {
> +        clock_update(pll->out, 0);
> +        return;
> +    }
> +
> +    ndiv = FIELD_EX32(*pll->reg_a2w_ctrl, A2W_PLLx_CTRL, NDIV);
> +    fdiv = FIELD_EX32(*pll->reg_a2w_frac, A2W_PLLx_FRAC, FRAC);
> +
> +    if (pll->reg_a2w_ana[1] & pll->prediv_mask) {
> +        /* The prescaler doubles the parent frequency */
> +        ndiv *= 2;
> +        fdiv *= 2;
> +    }
> +
> +    /*
> +     * We have a multiplier with an integer part (ndiv) and a fractional part
> +     * (fdiv), and a divider (pdiv).
> +     */
> +    freq = clock_get_hz(pll->xosc_in) *
> +        ((ndiv << R_A2W_PLLx_FRAC_FRAC_LENGTH) + fdiv);
> +    freq /= pdiv;
> +    freq >>= R_A2W_PLLx_FRAC_FRAC_LENGTH;

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +
> +    clock_update_hz(pll->out, freq);
>  }
>  
>  static void pll_xosc_update(void *opaque)
>  {
>      pll_update(CPRMAN_PLL(opaque));
> @@ -94,18 +132,42 @@ static const TypeInfo cprman_pll_info = {
>  };
>  
>  
>  /* CPRMAN "top level" model */
>  
> +static uint32_t get_cm_lock(const BCM2835CprmanState *s)
> +{
> +    static const int CM_LOCK_MAPPING[] = {

Maybe CM_LOCK_MAPPING[CPRMAN_NUM_PLL].

> +        [CPRMAN_PLLA] = R_CM_LOCK_FLOCKA_SHIFT,
> +        [CPRMAN_PLLC] = R_CM_LOCK_FLOCKC_SHIFT,
> +        [CPRMAN_PLLD] = R_CM_LOCK_FLOCKD_SHIFT,
> +        [CPRMAN_PLLH] = R_CM_LOCK_FLOCKH_SHIFT,
> +        [CPRMAN_PLLB] = R_CM_LOCK_FLOCKB_SHIFT,
> +    };
> +
> +    uint32_t r = 0;
> +    size_t i;
> +
> +    for (i = 0; i < CPRMAN_NUM_PLL; i++) {
> +        r |= pll_is_locked(&s->plls[i]) << CM_LOCK_MAPPING[i];
> +    }
> +
> +    return r;
> +}
> +
>  static uint64_t cprman_read(void *opaque, hwaddr offset,
>                                      unsigned size)
>  {
>      BCM2835CprmanState *s = CPRMAN(opaque);
>      uint64_t r = 0;
>      size_t idx = offset / sizeof(uint32_t);
>  
>      switch (idx) {
> +    case R_CM_LOCK:
> +        r = get_cm_lock(s);
> +        break;
> +
>      default:
>          r = s->regs[idx];
>      }
>  
>      trace_bcm2835_cprman_read(offset, r);
>