diff mbox series

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

Message ID 20201010135759.437903-14-luc@lmichel.fr (mailing list archive)
State New, archived
Headers show
Series raspi: add the bcm2835 cprman clock manager | expand

Commit Message

Luc Michel Oct. 10, 2020, 1:57 p.m. UTC 
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(+)

Comments

Philippe Mathieu-Daudé Oct. 10, 2020, 4:18 p.m. UTC | #1 
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.
Luc Michel Oct. 11, 2020, 6:26 p.m. UTC | #2 
On 18:18 Sat 10 Oct     , Philippe Mathieu-Daudé wrote:
> 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 ;)

Hi Phil,

I think there is a misunderstanding here:
  - set_xxx_init_info functions set (among others) register pointers
    to alias the common register array "regs" in BCM2835CprmanState.
    This is really an initialization step (in the sense of the QOM
    object). Those pointers won't move during the object's lifetime.
  - xxx_reset however (like e.g. xxx_update) _dereferences_ those
    pointers to access the registers data (in this case to set their
    reset values).

Doing so greatly decreases code complexity because:
  - read/write functions can directly access the common "regs" array
    without further decoding.
  - Each PLL shares a register with all its channels (A2W_CTRL). With
    this scheme, they simply all have a pointer aliasing the same data.
  - A lot a registers are unknown/unimplemented.

Thanks !
Philippe Mathieu-Daudé Oct. 16, 2020, 5:10 p.m. UTC | #3 
On 10/11/20 8:26 PM, Luc Michel wrote:
> On 18:18 Sat 10 Oct     , Philippe Mathieu-Daudé wrote:
>> 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 ;)
> 
> Hi Phil,
> 
> I think there is a misunderstanding here:
>    - set_xxx_init_info functions set (among others) register pointers
>      to alias the common register array "regs" in BCM2835CprmanState.
>      This is really an initialization step (in the sense of the QOM
>      object). Those pointers won't move during the object's lifetime.
>    - xxx_reset however (like e.g. xxx_update) _dereferences_ those
>      pointers to access the registers data (in this case to set their
>      reset values).
> 
> Doing so greatly decreases code complexity because:
>    - read/write functions can directly access the common "regs" array
>      without further decoding.
>    - Each PLL shares a register with all its channels (A2W_CTRL). With
>      this scheme, they simply all have a pointer aliasing the same data.
>    - A lot a registers are unknown/unimplemented.

OK, thanks for clarifying.
(I wanted to split the boilerplate code from the dumped constants).
Philippe Mathieu-Daudé Oct. 19, 2020, 3:44 p.m. UTC | #4 
On 10/16/20 7:10 PM, Philippe Mathieu-Daudé wrote:
> On 10/11/20 8:26 PM, Luc Michel wrote:
>> On 18:18 Sat 10 Oct     , Philippe Mathieu-Daudé wrote:
>>> 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(+)
[...]

I haven't verified the dumped values with real hardware,
but for the rest this patch shouldn't introduce regression,
and it helps to boot up to Linux kernel 5.7, so:

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

Patch

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
+    },
+
+    [CPRMAN_CLOCK_VPU] = {
+        .cm_ctl = 0x00000245,
+        .cm_div = 0x00003000,
+    },
+
+    [CPRMAN_CLOCK_SYS] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_PERIA] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_PERII] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_H264] = {
+        .cm_ctl = 0x00000244,
+        .cm_div = 0x00003000,
+    },
+
+    [CPRMAN_CLOCK_ISP] = {
+        .cm_ctl = 0x00000244,
+        .cm_div = 0x00003000,
+    },
+
+    [CPRMAN_CLOCK_V3D] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_CAM0] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_CAM1] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_CCP2] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_DSI0E] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_DSI0P] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_DPI] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_GP0] = {
+        .cm_ctl = 0x00000200,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_GP1] = {
+        .cm_ctl = 0x00000096,
+        .cm_div = 0x00014000,
+    },
+
+    [CPRMAN_CLOCK_GP2] = {
+        .cm_ctl = 0x00000291,
+        .cm_div = 0x00249f00,
+    },
+
+    [CPRMAN_CLOCK_HSM] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_OTP] = {
+        .cm_ctl = 0x00000091,
+        .cm_div = 0x00004000,
+    },
+
+    [CPRMAN_CLOCK_PCM] = {
+        .cm_ctl = 0x00000200,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_PWM] = {
+        .cm_ctl = 0x00000200,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_SLIM] = {
+        .cm_ctl = 0x00000200,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_SMI] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_TEC] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_TD0] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_TD1] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_TSENS] = {
+        .cm_ctl = 0x00000091,
+        .cm_div = 0x0000a000,
+    },
+
+    [CPRMAN_CLOCK_TIMER] = {
+        .cm_ctl = 0x00000291,
+        .cm_div = 0x00013333,
+    },
+
+    [CPRMAN_CLOCK_UART] = {
+        .cm_ctl = 0x00000296,
+        .cm_div = 0x0000a6ab,
+    },
+
+    [CPRMAN_CLOCK_VEC] = {
+        .cm_ctl = 0x00000097,
+        .cm_div = 0x00002000,
+    },
+
+    [CPRMAN_CLOCK_PULSE] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_SDC] = {
+        .cm_ctl = 0x00004006,
+        .cm_div = 0x00003000,
+    },
+
+    [CPRMAN_CLOCK_ARM] = {
+        .cm_ctl = 0, /* unknown */
+        .cm_div = 0
+    },
+
+    [CPRMAN_CLOCK_AVEO] = {
+        .cm_ctl = 0x00000000,
+        .cm_div = 0x00000000,
+    },
+
+    [CPRMAN_CLOCK_EMMC] = {
+        .cm_ctl = 0x00000295,
+        .cm_div = 0x00006000,
+    },
+
+    [CPRMAN_CLOCK_EMMC2] = {
+        .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];
+
+    *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;
+}
+
 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;
 }
 
 static const TypeInfo cprman_pll_info = {
     .name = TYPE_CPRMAN_PLL,
@@ -135,10 +147,18 @@  static const TypeInfo cprman_pll_info = {
 };
 
 
 /* PLL channel */
 
+static void pll_channel_reset(DeviceState *dev)
+{
+    CprmanPllChannelState *s = CPRMAN_PLL_CHANNEL(dev);
+    const PLLChannelResetInfo *info = &PLL_CHANNEL_RESET_INFO[s->id];
+
+    *s->reg_a2w_ctrl = info->a2w_ctrl;
+}
+
 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
@@ -215,10 +235,11 @@  static const VMStateDescription pll_channel_vmstate = {
 
 static void pll_channel_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pll_channel_reset;
     dc->vmsd = &pll_channel_vmstate;
 }
 
 static const TypeInfo cprman_pll_channel_info = {
     .name = TYPE_CPRMAN_PLL_CHANNEL,
@@ -293,10 +314,19 @@  static void clock_mux_src_update(void *opaque)
     }
 
     clock_mux_update(s);
 }
 
+static void clock_mux_reset(DeviceState *dev)
+{
+    CprmanClockMuxState *clock = CPRMAN_CLOCK_MUX(dev);
+    const ClockMuxResetInfo *info = &CLOCK_MUX_RESET_INFO[clock->id];
+
+    *clock->reg_ctl = info->cm_ctl;
+    *clock->reg_div = info->cm_div;
+}
+
 static void clock_mux_init(Object *obj)
 {
     CprmanClockMuxState *s = CPRMAN_CLOCK_MUX(obj);
     size_t i;
 
@@ -325,10 +355,11 @@  static const VMStateDescription clock_mux_vmstate = {
 
 static void clock_mux_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = clock_mux_reset;
     dc->vmsd = &clock_mux_vmstate;
 }
 
 static const TypeInfo cprman_clock_mux_info = {
     .name = TYPE_CPRMAN_CLOCK_MUX,