Re: [PATCH v3 13/15] hw/misc/bcm2835_cprman: add sane reset values to the registers

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

On 10/10/20 3:57 PM, Luc Michel wrote:
> Those reset values have been extracted from a Raspberry Pi 3 model B
> v1.2, using the 2020-08-20 version of raspios. The dump was done using
> the debugfs interface of the CPRMAN driver in Linux (under
> '/sys/kernel/debug/clk'). Each exposed clock tree stage (PLLs, channels
> and muxes) can be observed by reading the 'regdump' file (e.g.
> 'plla/regdump').
> 
> Those values are set by the Raspberry Pi firmware at boot time (Linux
> expects them to be set when it boots up).
> 
> Some stages are not exposed by the Linux driver (e.g. the PLL B). For
> those, the reset values are unknown and left to 0 which implies a
> disabled output.
> 
> Once booted in QEMU, the final clock tree is very similar to the one
> visible on real hardware. The differences come from some unimplemented
> devices for which the driver simply disable the corresponding clock.
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>   include/hw/misc/bcm2835_cprman_internals.h | 269 +++++++++++++++++++++
>   hw/misc/bcm2835_cprman.c                   |  31 +++
>   2 files changed, 300 insertions(+)
> 
> diff --git a/include/hw/misc/bcm2835_cprman_internals.h b/include/hw/misc/bcm2835_cprman_internals.h
> index a6e799075f..339759b307 100644
> --- a/include/hw/misc/bcm2835_cprman_internals.h
> +++ b/include/hw/misc/bcm2835_cprman_internals.h
> @@ -745,6 +745,275 @@ static inline void set_clock_mux_init_info(BCM2835CprmanState *s,
>       mux->reg_div = &s->regs[CLOCK_MUX_INIT_INFO[id].cm_offset + 1];
>       mux->int_bits = CLOCK_MUX_INIT_INFO[id].int_bits;
>       mux->frac_bits = CLOCK_MUX_INIT_INFO[id].frac_bits;
>   }
>   
> +
> +/*
> + * Object reset info
> + * Those values have been dumped from a Raspberry Pi 3 Model B v1.2 using the
> + * clk debugfs interface in Linux.
> + */
> +typedef struct PLLResetInfo {
> +    uint32_t cm;
> +    uint32_t a2w_ctrl;
> +    uint32_t a2w_ana[4];
> +    uint32_t a2w_frac;
> +} PLLResetInfo;
> +
> +static const PLLResetInfo PLL_RESET_INFO[] = {
> +    [CPRMAN_PLLA] = {
> +        .cm = 0x0000008a,
> +        .a2w_ctrl = 0x0002103a,
> +        .a2w_frac = 0x00098000,
> +        .a2w_ana = { 0x00000000, 0x00144000, 0x00000000, 0x00000100 }
> +    },
> +
> +    [CPRMAN_PLLC] = {
> +        .cm = 0x00000228,
> +        .a2w_ctrl = 0x0002103e,
> +        .a2w_frac = 0x00080000,
> +        .a2w_ana = { 0x00000000, 0x00144000, 0x00000000, 0x00000100 }
> +    },
> +
> +    [CPRMAN_PLLD] = {
> +        .cm = 0x0000020a,
> +        .a2w_ctrl = 0x00021034,
> +        .a2w_frac = 0x00015556,
> +        .a2w_ana = { 0x00000000, 0x00144000, 0x00000000, 0x00000100 }
> +    },
> +
> +    [CPRMAN_PLLH] = {
> +        .cm = 0x00000000,
> +        .a2w_ctrl = 0x0002102d,
> +        .a2w_frac = 0x00000000,
> +        .a2w_ana = { 0x00900000, 0x0000000c, 0x00000000, 0x00000000 }
> +    },
> +
> +    [CPRMAN_PLLB] = {
> +        /* unknown */
> +        .cm = 0x00000000,
> +        .a2w_ctrl = 0x00000000,
> +        .a2w_frac = 0x00000000,
> +        .a2w_ana = { 0x00000000, 0x00000000, 0x00000000, 0x00000000 }
> +    }
> +};
> +
> +typedef struct PLLChannelResetInfo {
> +    /*
> +     * Even though a PLL channel has a CM register, it shares it with its
> +     * parent PLL. The parent already takes care of the reset value.
> +     */
> +    uint32_t a2w_ctrl;
> +} PLLChannelResetInfo;
> +
> +static const PLLChannelResetInfo PLL_CHANNEL_RESET_INFO[] = {
> +    [CPRMAN_PLLA_CHANNEL_DSI0] = { .a2w_ctrl = 0x00000100 },
> +    [CPRMAN_PLLA_CHANNEL_CORE] = { .a2w_ctrl = 0x00000003 },
> +    [CPRMAN_PLLA_CHANNEL_PER] = { .a2w_ctrl = 0x00000000 }, /* unknown */
> +    [CPRMAN_PLLA_CHANNEL_CCP2] = { .a2w_ctrl = 0x00000100 },
> +
> +    [CPRMAN_PLLC_CHANNEL_CORE2] = { .a2w_ctrl = 0x00000100 },
> +    [CPRMAN_PLLC_CHANNEL_CORE1] = { .a2w_ctrl = 0x00000100 },
> +    [CPRMAN_PLLC_CHANNEL_PER] = { .a2w_ctrl = 0x00000002 },
> +    [CPRMAN_PLLC_CHANNEL_CORE0] = { .a2w_ctrl = 0x00000002 },
> +
> +    [CPRMAN_PLLD_CHANNEL_DSI0] = { .a2w_ctrl = 0x00000100 },
> +    [CPRMAN_PLLD_CHANNEL_CORE] = { .a2w_ctrl = 0x00000004 },
> +    [CPRMAN_PLLD_CHANNEL_PER] = { .a2w_ctrl = 0x00000004 },
> +    [CPRMAN_PLLD_CHANNEL_DSI1] = { .a2w_ctrl = 0x00000100 },
> +
> +    [CPRMAN_PLLH_CHANNEL_AUX] = { .a2w_ctrl = 0x00000004 },
> +    [CPRMAN_PLLH_CHANNEL_RCAL] = { .a2w_ctrl = 0x00000000 },
> +    [CPRMAN_PLLH_CHANNEL_PIX] = { .a2w_ctrl = 0x00000000 },
> +
> +    [CPRMAN_PLLB_CHANNEL_ARM] = { .a2w_ctrl = 0x00000000 }, /* unknown */
> +};
> +
> +typedef struct ClockMuxResetInfo {
> +    uint32_t cm_ctl;
> +    uint32_t cm_div;
> +} ClockMuxResetInfo;
> +
> +static const ClockMuxResetInfo CLOCK_MUX_RESET_INFO[] = {
> +    [CPRMAN_CLOCK_GNRIC] = {
> +        .cm_ctl = 0, /* unknown */
> +        .cm_div = 0
> +    },
> +
[...]
> +};
> +
>   #endif
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 7a7401963d..7e415a017c 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -51,10 +51,21 @@
>   #include "hw/misc/bcm2835_cprman_internals.h"
>   #include "trace.h"
>   
>   /* PLL */
>   
> +static void pll_reset(DeviceState *dev)
> +{
> +    CprmanPllState *s = CPRMAN_PLL(dev);
> +    const PLLResetInfo *info = &PLL_RESET_INFO[s->id];

Hmm so we overwrite various values from PLL_INIT_INFO.
> +
> +    *s->reg_cm = info->cm;
> +    *s->reg_a2w_ctrl = info->a2w_ctrl;
> +    memcpy(s->reg_a2w_ana, info->a2w_ana, sizeof(info->a2w_ana));
> +    *s->reg_a2w_frac = info->a2w_frac;

set_pll_init_info() can be simplified as:

     pll->id = id;
     pll->prediv_mask = PLL_INIT_INFO[id].prediv_mask;

Or directly in cprman_init():

     &s->plls[i]->id = i;
     &s->plls[i]->prediv_mask = PLL_INIT_INFO[i].prediv_mask;

And the rest directly implemented in pll_reset().

Maybe not, but having pll_reset() added in patch #8/15
"bcm2835_cprman: add a PLL channel skeleton implementation"
would make this patch review easier ;)

> +}
> +
>   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);
>   }
> @@ -121,10 +132,11 @@ static const VMStateDescription pll_vmstate = {
>   
>   static void pll_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
> +    dc->reset = pll_reset;
>       dc->vmsd = &pll_vmstate;
>   }
[...]
Similarly, implement clock_mux_reset() in patch #10/15
"bcm2835_cprman: add a clock mux skeleton implementation".

Regards,

Phil.