Re: [PATCH 08/14] hw/misc/bcm2835_cprman: implement PLL channels behaviour

[Philippe Mathieu-Daudé]

On 9/25/20 12:17 PM, Luc Michel wrote:
> A PLL channel is able to further divide the generated PLL frequency. The

You can move the 'The' to the next line =)

> divider is given in the ctrl_a2w register. Some channels have a

s/a/an/

> additional fixed divider which is always applied to the signal.
> 
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>  hw/misc/bcm2835_cprman.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 2c70a3f317..e644aeb2b5 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -132,13 +132,44 @@ static const TypeInfo cprman_pll_info = {
>  };
>  
>  
>  /* PLL channel */
>  
> +static bool pll_channel_is_enabled(CprmanPLLChannelState *channel)
> +{
> +    /*
> +     * XXX I'm not sure of the purpose of the LOAD field. The Linux driver 
> does
> +     * not set it when enabling the channel, but does clear it when disabling
> +     * it.

Cc'ed firmware developers who might have a clue.

> +     */
> +    return !FIELD_EX32(*channel->reg_a2w_ctrl, A2W_PLLx_CHANNELy, DISABLE)
> +        && !(*channel->reg_cm & channel->hold_mask);
> +}
> +
>  static void pll_channel_update(CprmanPLLChannelState *channel)
>  {
> -    clock_update(channel->out, 0);
> +    uint64_t freq, div;
> +
> +    if (!pll_channel_is_enabled(channel)) {
> +        clock_update(channel->out, 0);
> +        return;
> +    }
> +
> +    div = FIELD_EX32(*channel->reg_a2w_ctrl, A2W_PLLx_CHANNELy, DIV);
> +
> +    if (!div) {
> +        /*
> +         * It seems that when the divider value is 0, it is considered as
> +         * being maximum by the hardware (see the Linux driver).
> +         */
> +        div = R_A2W_PLLx_CHANNELy_DIV_MASK;
> +    }
> +
> +    /* Some channels have an additional fixed divider */
> +    freq = clock_get_hz(channel->pll_in) / (div * channel->fixed_divider);
> +
> +    clock_update_hz(channel->out, freq);
>  }
>  
>  /* Update a PLL and all its channels */
>  static void pll_update_all_channels(BCM2835CprmanState *s,
>                                      CprmanPLLState *pll)
>