mbox series

[v3,00/15] raspi: add the bcm2835 cprman clock manager

Message ID 20201010135759.437903-1-luc@lmichel.fr
Headers show
Series raspi: add the bcm2835 cprman clock manager | expand

Message

Luc Michel Oct. 10, 2020, 1:57 p.m. UTC
v2 -> v3:
  - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
  - patch 03: commit message typo [Clement]
  - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
              reg_cm replaced with reg_ctl and reg_div. Add some
              comments for clarity. [Phil]
  - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
              correctly. [Phil]
  - patch 11: replaced manual bitfield extraction with extract32 [Phil]
  - patch 11: added a visual representation of CM_DIV for clarity [Phil]
  - patch 11: added a missing return in clock_mux_update.

v1 -> v2:
  - patch 05: Added a comment about MMIO .valid constraints [Phil]
  - patch 05: Added MMIO .impl [Phil]
  - patch 05: Moved init_internal_clock to the public clock API, renamed
    clock_new (new patch 03) [Phil]
  - patch 11: use muldiv64 for clock mux frequency output computation [Phil]
  - patch 11: add a check for null divisor (Phil: I dropped your r-b)
  - Typos, formatting, naming, style [Phil]

Patches without review: 03, 11, 13

Hi,

This series add the BCM2835 CPRMAN clock manager peripheral to the
Raspberry Pi machine.

Patches 1-4 are preliminary changes, patches 5-13 are the actual
implementation.

The two last patches add a clock input to the PL011 and
connect it to the CPRMAN.

This series has been tested with Linux 5.4.61 (the current raspios
version). It fixes the kernel Oops at boot time due to invalid UART
clock value, and other warnings/errors here and there because of bad
clocks or lack of CPRMAN.

Here is the clock tree as seen by Linux when booted in QEMU:
(/sys/kernel/debug/clk/clk_summary with some columns removed)

                        enable  prepare              
   clock                 count    count          rate
-----------------------------------------------------
 otg                         0        0     480000000
 osc                         5        5      19200000
    gp2                      1        1         32768
    tsens                    0        0       1920000
    otp                      0        0       4800000
    timer                    0        0       1000002
    pllh                     4        4     864000000
       pllh_pix_prediv       1        1       3375000
          pllh_pix           0        0        337500
       pllh_aux              1        1     216000000
          vec                0        0     108000000
       pllh_rcal_prediv      1        1       3375000
          pllh_rcal          0        0        337500
    plld                     3        3    2000000024
       plld_dsi1             0        0       7812501
       plld_dsi0             0        0       7812501
       plld_per              3        3     500000006
          gp1                1        1      25000000
          uart               1        2      47999625
       plld_core             2        2     500000006
          sdram              0        0     166666668
    pllc                     3        3    2400000000
       pllc_per              1        1    1200000000
          emmc               0        0     200000000
       pllc_core2            0        0       9375000
       pllc_core1            0        0       9375000
       pllc_core0            2        2    1200000000
          vpu                1        1     700000000
             aux_spi2        0        0     700000000
             aux_spi1        0        0     700000000
             aux_uart        0        0     700000000
             peri_image      0        0     700000000
    plla                     2        2    2250000000
       plla_ccp2             0        0       8789063
       plla_dsi0             0        0       8789063
       plla_core             1        1     750000000
          h264               0        0     250000000
          isp                0        0     250000000
 dsi1p                       0        0             0
 dsi0p                       0        0             0
 dsi1e                       0        0             0
 dsi0e                       0        0             0
 cam1                        0        0             0
 cam0                        0        0             0
 dpi                         0        0             0
 tec                         0        0             0
 smi                         0        0             0
 slim                        0        0             0
 gp0                         0        0             0
 dft                         0        0             0
 aveo                        0        0             0
 pcm                         0        0             0
 pwm                         0        0             0
 hsm                         0        0             0

It shows small differences with real hardware due other missing
peripherals for which the driver turn the clock off (like tsens).

Luc Michel (15):
  hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro
  hw/core/clock: trace clock values in Hz instead of ns
  hw/core/clock: add the clock_new helper function
  hw/arm/raspi: fix CPRMAN base address
  hw/arm/raspi: add a skeleton implementation of the CPRMAN
  hw/misc/bcm2835_cprman: add a PLL skeleton implementation
  hw/misc/bcm2835_cprman: implement PLLs behaviour
  hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation
  hw/misc/bcm2835_cprman: implement PLL channels behaviour
  hw/misc/bcm2835_cprman: add a clock mux skeleton implementation
  hw/misc/bcm2835_cprman: implement clock mux behaviour
  hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer
  hw/misc/bcm2835_cprman: add sane reset values to the registers
  hw/char/pl011: add a clock input
  hw/arm/bcm2835_peripherals: connect the UART clock

 include/hw/arm/bcm2835_peripherals.h       |    5 +-
 include/hw/arm/raspi_platform.h            |    5 +-
 include/hw/char/pl011.h                    |    1 +
 include/hw/clock.h                         |   18 +
 include/hw/misc/bcm2835_cprman.h           |  210 ++++
 include/hw/misc/bcm2835_cprman_internals.h | 1019 ++++++++++++++++++++
 hw/arm/bcm2835_peripherals.c               |   15 +-
 hw/char/pl011.c                            |   45 +
 hw/core/clock.c                            |   21 +-
 hw/misc/bcm2835_cprman.c                   |  808 ++++++++++++++++
 hw/char/trace-events                       |    1 +
 hw/core/trace-events                       |    4 +-
 hw/misc/meson.build                        |    1 +
 hw/misc/trace-events                       |    5 +
 14 files changed, 2146 insertions(+), 12 deletions(-)
 create mode 100644 include/hw/misc/bcm2835_cprman.h
 create mode 100644 include/hw/misc/bcm2835_cprman_internals.h
 create mode 100644 hw/misc/bcm2835_cprman.c

Comments

Philippe Mathieu-Daudé Oct. 10, 2020, 3:17 p.m. UTC | #1
On 10/10/20 3:57 PM, Luc Michel wrote:
> This function creates a clock and parents it to another object with a given
> name. It calls clock_setup_canonical_path before returning the new
> clock.
> 
> This function is useful to create clocks in devices when one doesn't
> want to expose it at the qdev level (as an input or an output).
> 
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>   include/hw/clock.h | 13 +++++++++++++
>   hw/core/clock.c    | 15 +++++++++++++++
>   2 files changed, 28 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé Oct. 10, 2020, 3:44 p.m. UTC | #2
On 10/10/20 5:17 PM, Philippe Mathieu-Daudé wrote:
> On 10/10/20 3:57 PM, Luc Michel wrote:

>> This function creates a clock and parents it to another object with a 

>> given

>> name. It calls clock_setup_canonical_path before returning the new

>> clock.

>>

>> This function is useful to create clocks in devices when one doesn't

>> want to expose it at the qdev level (as an input or an output).

>>

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

>> Signed-off-by: Luc Michel <luc@lmichel.fr>

>> ---

>>   include/hw/clock.h | 13 +++++++++++++

>>   hw/core/clock.c    | 15 +++++++++++++++

>>   2 files changed, 28 insertions(+)

> 

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


Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé Oct. 10, 2020, 4:18 p.m. UTC | #3
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 | #4
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 | #5
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. 16, 2020, 5:11 p.m. UTC | #6
On 10/10/20 3:57 PM, Luc Michel wrote:
[...]
> Hi,
> 
> This series add the BCM2835 CPRMAN clock manager peripheral to the
> Raspberry Pi machine.

Series:
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé Oct. 19, 2020, 3:44 p.m. UTC | #7
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>
Peter Maydell Oct. 19, 2020, 3:45 p.m. UTC | #8
On Sat, 10 Oct 2020 at 14:57, Luc Michel <luc@lmichel.fr> wrote:
>
> v2 -> v3:
>   - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
>   - patch 03: commit message typo [Clement]
>   - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
>               reg_cm replaced with reg_ctl and reg_div. Add some
>               comments for clarity. [Phil]
>   - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
>               correctly. [Phil]
>   - patch 11: replaced manual bitfield extraction with extract32 [Phil]
>   - patch 11: added a visual representation of CM_DIV for clarity [Phil]
>   - patch 11: added a missing return in clock_mux_update.



Applied to target-arm.next, thanks.

-- PMM
Peter Maydell Oct. 19, 2020, 7:31 p.m. UTC | #9
On Mon, 19 Oct 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 10 Oct 2020 at 14:57, Luc Michel <luc@lmichel.fr> wrote:
> >
> > v2 -> v3:
> >   - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
> >   - patch 03: commit message typo [Clement]
> >   - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
> >               reg_cm replaced with reg_ctl and reg_div. Add some
> >               comments for clarity. [Phil]
> >   - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
> >               correctly. [Phil]
> >   - patch 11: replaced manual bitfield extraction with extract32 [Phil]
> >   - patch 11: added a visual representation of CM_DIV for clarity [Phil]
> >   - patch 11: added a missing return in clock_mux_update.
>
>
>
> Applied to target-arm.next, thanks.

Dropped again due to segv in 'make check' when running with
clang sanitizer, which I gather from irc that you're looking
into.

thanks
-- PMM
Philippe Mathieu-Daudé Oct. 22, 2020, 10:06 p.m. UTC | #10
Cc'ing Guenter who had a similar patch and might be interested
to test :)

patch 16/15 fixup:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg752113.html

On 10/10/20 3:57 PM, Luc Michel wrote:
> v2 -> v3:
>    - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
>    - patch 03: commit message typo [Clement]
>    - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
>                reg_cm replaced with reg_ctl and reg_div. Add some
>                comments for clarity. [Phil]
>    - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
>                correctly. [Phil]
>    - patch 11: replaced manual bitfield extraction with extract32 [Phil]
>    - patch 11: added a visual representation of CM_DIV for clarity [Phil]
>    - patch 11: added a missing return in clock_mux_update.
> 
> v1 -> v2:
>    - patch 05: Added a comment about MMIO .valid constraints [Phil]
>    - patch 05: Added MMIO .impl [Phil]
>    - patch 05: Moved init_internal_clock to the public clock API, renamed
>      clock_new (new patch 03) [Phil]
>    - patch 11: use muldiv64 for clock mux frequency output computation [Phil]
>    - patch 11: add a check for null divisor (Phil: I dropped your r-b)
>    - Typos, formatting, naming, style [Phil]
> 
> Patches without review: 03, 11, 13
> 
> Hi,
> 
> This series add the BCM2835 CPRMAN clock manager peripheral to the
> Raspberry Pi machine.
> 
> Patches 1-4 are preliminary changes, patches 5-13 are the actual
> implementation.
> 
> The two last patches add a clock input to the PL011 and
> connect it to the CPRMAN.
> 
> This series has been tested with Linux 5.4.61 (the current raspios
> version). It fixes the kernel Oops at boot time due to invalid UART
> clock value, and other warnings/errors here and there because of bad
> clocks or lack of CPRMAN.
> 
> Here is the clock tree as seen by Linux when booted in QEMU:
> (/sys/kernel/debug/clk/clk_summary with some columns removed)
> 
>                          enable  prepare
>     clock                 count    count          rate
> -----------------------------------------------------
>   otg                         0        0     480000000
>   osc                         5        5      19200000
>      gp2                      1        1         32768
>      tsens                    0        0       1920000
>      otp                      0        0       4800000
>      timer                    0        0       1000002
>      pllh                     4        4     864000000
>         pllh_pix_prediv       1        1       3375000
>            pllh_pix           0        0        337500
>         pllh_aux              1        1     216000000
>            vec                0        0     108000000
>         pllh_rcal_prediv      1        1       3375000
>            pllh_rcal          0        0        337500
>      plld                     3        3    2000000024
>         plld_dsi1             0        0       7812501
>         plld_dsi0             0        0       7812501
>         plld_per              3        3     500000006
>            gp1                1        1      25000000
>            uart               1        2      47999625
>         plld_core             2        2     500000006
>            sdram              0        0     166666668
>      pllc                     3        3    2400000000
>         pllc_per              1        1    1200000000
>            emmc               0        0     200000000
>         pllc_core2            0        0       9375000
>         pllc_core1            0        0       9375000
>         pllc_core0            2        2    1200000000
>            vpu                1        1     700000000
>               aux_spi2        0        0     700000000
>               aux_spi1        0        0     700000000
>               aux_uart        0        0     700000000
>               peri_image      0        0     700000000
>      plla                     2        2    2250000000
>         plla_ccp2             0        0       8789063
>         plla_dsi0             0        0       8789063
>         plla_core             1        1     750000000
>            h264               0        0     250000000
>            isp                0        0     250000000
>   dsi1p                       0        0             0
>   dsi0p                       0        0             0
>   dsi1e                       0        0             0
>   dsi0e                       0        0             0
>   cam1                        0        0             0
>   cam0                        0        0             0
>   dpi                         0        0             0
>   tec                         0        0             0
>   smi                         0        0             0
>   slim                        0        0             0
>   gp0                         0        0             0
>   dft                         0        0             0
>   aveo                        0        0             0
>   pcm                         0        0             0
>   pwm                         0        0             0
>   hsm                         0        0             0
> 
> It shows small differences with real hardware due other missing
> peripherals for which the driver turn the clock off (like tsens).
> 
> Luc Michel (15):
>    hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro
>    hw/core/clock: trace clock values in Hz instead of ns
>    hw/core/clock: add the clock_new helper function
>    hw/arm/raspi: fix CPRMAN base address
>    hw/arm/raspi: add a skeleton implementation of the CPRMAN
>    hw/misc/bcm2835_cprman: add a PLL skeleton implementation
>    hw/misc/bcm2835_cprman: implement PLLs behaviour
>    hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation
>    hw/misc/bcm2835_cprman: implement PLL channels behaviour
>    hw/misc/bcm2835_cprman: add a clock mux skeleton implementation
>    hw/misc/bcm2835_cprman: implement clock mux behaviour
>    hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer
>    hw/misc/bcm2835_cprman: add sane reset values to the registers
>    hw/char/pl011: add a clock input
>    hw/arm/bcm2835_peripherals: connect the UART clock
> 
>   include/hw/arm/bcm2835_peripherals.h       |    5 +-
>   include/hw/arm/raspi_platform.h            |    5 +-
>   include/hw/char/pl011.h                    |    1 +
>   include/hw/clock.h                         |   18 +
>   include/hw/misc/bcm2835_cprman.h           |  210 ++++
>   include/hw/misc/bcm2835_cprman_internals.h | 1019 ++++++++++++++++++++
>   hw/arm/bcm2835_peripherals.c               |   15 +-
>   hw/char/pl011.c                            |   45 +
>   hw/core/clock.c                            |   21 +-
>   hw/misc/bcm2835_cprman.c                   |  808 ++++++++++++++++
>   hw/char/trace-events                       |    1 +
>   hw/core/trace-events                       |    4 +-
>   hw/misc/meson.build                        |    1 +
>   hw/misc/trace-events                       |    5 +
>   14 files changed, 2146 insertions(+), 12 deletions(-)
>   create mode 100644 include/hw/misc/bcm2835_cprman.h
>   create mode 100644 include/hw/misc/bcm2835_cprman_internals.h
>   create mode 100644 hw/misc/bcm2835_cprman.c
>
Guenter Roeck Oct. 22, 2020, 10:48 p.m. UTC | #11
Hi,

On 10/22/20 3:06 PM, Philippe Mathieu-Daudé wrote:
> Cc'ing Guenter who had a similar patch and might be interested
> to test :)
> 

great. I think my patch doesn't work anymore since qemu 5.0 (at least not
for raspi3), and it was pretty hackish anyway. I'll give the series a try.

Guenter

> patch 16/15 fixup:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg752113.html
> 
> On 10/10/20 3:57 PM, Luc Michel wrote:
>> v2 -> v3:
>>    - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
>>    - patch 03: commit message typo [Clement]
>>    - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
>>                reg_cm replaced with reg_ctl and reg_div. Add some
>>                comments for clarity. [Phil]
>>    - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
>>                correctly. [Phil]
>>    - patch 11: replaced manual bitfield extraction with extract32 [Phil]
>>    - patch 11: added a visual representation of CM_DIV for clarity [Phil]
>>    - patch 11: added a missing return in clock_mux_update.
>>
>> v1 -> v2:
>>    - patch 05: Added a comment about MMIO .valid constraints [Phil]
>>    - patch 05: Added MMIO .impl [Phil]
>>    - patch 05: Moved init_internal_clock to the public clock API, renamed
>>      clock_new (new patch 03) [Phil]
>>    - patch 11: use muldiv64 for clock mux frequency output computation [Phil]
>>    - patch 11: add a check for null divisor (Phil: I dropped your r-b)
>>    - Typos, formatting, naming, style [Phil]
>>
>> Patches without review: 03, 11, 13
>>
>> Hi,
>>
>> This series add the BCM2835 CPRMAN clock manager peripheral to the
>> Raspberry Pi machine.
>>
>> Patches 1-4 are preliminary changes, patches 5-13 are the actual
>> implementation.
>>
>> The two last patches add a clock input to the PL011 and
>> connect it to the CPRMAN.
>>
>> This series has been tested with Linux 5.4.61 (the current raspios
>> version). It fixes the kernel Oops at boot time due to invalid UART
>> clock value, and other warnings/errors here and there because of bad
>> clocks or lack of CPRMAN.
>>
>> Here is the clock tree as seen by Linux when booted in QEMU:
>> (/sys/kernel/debug/clk/clk_summary with some columns removed)
>>
>>                          enable  prepare
>>     clock                 count    count          rate
>> -----------------------------------------------------
>>   otg                         0        0     480000000
>>   osc                         5        5      19200000
>>      gp2                      1        1         32768
>>      tsens                    0        0       1920000
>>      otp                      0        0       4800000
>>      timer                    0        0       1000002
>>      pllh                     4        4     864000000
>>         pllh_pix_prediv       1        1       3375000
>>            pllh_pix           0        0        337500
>>         pllh_aux              1        1     216000000
>>            vec                0        0     108000000
>>         pllh_rcal_prediv      1        1       3375000
>>            pllh_rcal          0        0        337500
>>      plld                     3        3    2000000024
>>         plld_dsi1             0        0       7812501
>>         plld_dsi0             0        0       7812501
>>         plld_per              3        3     500000006
>>            gp1                1        1      25000000
>>            uart               1        2      47999625
>>         plld_core             2        2     500000006
>>            sdram              0        0     166666668
>>      pllc                     3        3    2400000000
>>         pllc_per              1        1    1200000000
>>            emmc               0        0     200000000
>>         pllc_core2            0        0       9375000
>>         pllc_core1            0        0       9375000
>>         pllc_core0            2        2    1200000000
>>            vpu                1        1     700000000
>>               aux_spi2        0        0     700000000
>>               aux_spi1        0        0     700000000
>>               aux_uart        0        0     700000000
>>               peri_image      0        0     700000000
>>      plla                     2        2    2250000000
>>         plla_ccp2             0        0       8789063
>>         plla_dsi0             0        0       8789063
>>         plla_core             1        1     750000000
>>            h264               0        0     250000000
>>            isp                0        0     250000000
>>   dsi1p                       0        0             0
>>   dsi0p                       0        0             0
>>   dsi1e                       0        0             0
>>   dsi0e                       0        0             0
>>   cam1                        0        0             0
>>   cam0                        0        0             0
>>   dpi                         0        0             0
>>   tec                         0        0             0
>>   smi                         0        0             0
>>   slim                        0        0             0
>>   gp0                         0        0             0
>>   dft                         0        0             0
>>   aveo                        0        0             0
>>   pcm                         0        0             0
>>   pwm                         0        0             0
>>   hsm                         0        0             0
>>
>> It shows small differences with real hardware due other missing
>> peripherals for which the driver turn the clock off (like tsens).
>>
>> Luc Michel (15):
>>    hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro
>>    hw/core/clock: trace clock values in Hz instead of ns
>>    hw/core/clock: add the clock_new helper function
>>    hw/arm/raspi: fix CPRMAN base address
>>    hw/arm/raspi: add a skeleton implementation of the CPRMAN
>>    hw/misc/bcm2835_cprman: add a PLL skeleton implementation
>>    hw/misc/bcm2835_cprman: implement PLLs behaviour
>>    hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation
>>    hw/misc/bcm2835_cprman: implement PLL channels behaviour
>>    hw/misc/bcm2835_cprman: add a clock mux skeleton implementation
>>    hw/misc/bcm2835_cprman: implement clock mux behaviour
>>    hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer
>>    hw/misc/bcm2835_cprman: add sane reset values to the registers
>>    hw/char/pl011: add a clock input
>>    hw/arm/bcm2835_peripherals: connect the UART clock
>>
>>   include/hw/arm/bcm2835_peripherals.h       |    5 +-
>>   include/hw/arm/raspi_platform.h            |    5 +-
>>   include/hw/char/pl011.h                    |    1 +
>>   include/hw/clock.h                         |   18 +
>>   include/hw/misc/bcm2835_cprman.h           |  210 ++++
>>   include/hw/misc/bcm2835_cprman_internals.h | 1019 ++++++++++++++++++++
>>   hw/arm/bcm2835_peripherals.c               |   15 +-
>>   hw/char/pl011.c                            |   45 +
>>   hw/core/clock.c                            |   21 +-
>>   hw/misc/bcm2835_cprman.c                   |  808 ++++++++++++++++
>>   hw/char/trace-events                       |    1 +
>>   hw/core/trace-events                       |    4 +-
>>   hw/misc/meson.build                        |    1 +
>>   hw/misc/trace-events                       |    5 +
>>   14 files changed, 2146 insertions(+), 12 deletions(-)
>>   create mode 100644 include/hw/misc/bcm2835_cprman.h
>>   create mode 100644 include/hw/misc/bcm2835_cprman_internals.h
>>   create mode 100644 hw/misc/bcm2835_cprman.c
>>
Guenter Roeck Oct. 23, 2020, 3:55 a.m. UTC | #12
On 10/22/20 3:06 PM, Philippe Mathieu-Daudé wrote:
> Cc'ing Guenter who had a similar patch and might be interested

> to test :)

> 

I applied the series on top of qemu mainline and ran all my test with it
(raspi2 with qemu-system-arm as well as qemu-system-aarch64, and raspi3
in both big endian and little endian mode with qemu-system-aarch64.
All tests passed without error or kernel warning.

For the series:

Tested-by: Guenter Roeck <linux@roeck-us.net>


Guenter

> patch 16/15 fixup:

> https://www.mail-archive.com/qemu-devel@nongnu.org/msg752113.html

> 

> On 10/10/20 3:57 PM, Luc Michel wrote:

>> v2 -> v3:

>>    - patch 03: moved clock_new definition to hw/core/clock.c [Phil]

>>    - patch 03: commit message typo [Clement]

>>    - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.

>>                reg_cm replaced with reg_ctl and reg_div. Add some

>>                comments for clarity. [Phil]

>>    - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset

>>                correctly. [Phil]

>>    - patch 11: replaced manual bitfield extraction with extract32 [Phil]

>>    - patch 11: added a visual representation of CM_DIV for clarity [Phil]

>>    - patch 11: added a missing return in clock_mux_update.

>>

>> v1 -> v2:

>>    - patch 05: Added a comment about MMIO .valid constraints [Phil]

>>    - patch 05: Added MMIO .impl [Phil]

>>    - patch 05: Moved init_internal_clock to the public clock API, renamed

>>      clock_new (new patch 03) [Phil]

>>    - patch 11: use muldiv64 for clock mux frequency output computation [Phil]

>>    - patch 11: add a check for null divisor (Phil: I dropped your r-b)

>>    - Typos, formatting, naming, style [Phil]

>>

>> Patches without review: 03, 11, 13

>>

>> Hi,

>>

>> This series add the BCM2835 CPRMAN clock manager peripheral to the

>> Raspberry Pi machine.

>>

>> Patches 1-4 are preliminary changes, patches 5-13 are the actual

>> implementation.

>>

>> The two last patches add a clock input to the PL011 and

>> connect it to the CPRMAN.

>>

>> This series has been tested with Linux 5.4.61 (the current raspios

>> version). It fixes the kernel Oops at boot time due to invalid UART

>> clock value, and other warnings/errors here and there because of bad

>> clocks or lack of CPRMAN.

>>

>> Here is the clock tree as seen by Linux when booted in QEMU:

>> (/sys/kernel/debug/clk/clk_summary with some columns removed)

>>

>>                          enable  prepare

>>     clock                 count    count          rate

>> -----------------------------------------------------

>>   otg                         0        0     480000000

>>   osc                         5        5      19200000

>>      gp2                      1        1         32768

>>      tsens                    0        0       1920000

>>      otp                      0        0       4800000

>>      timer                    0        0       1000002

>>      pllh                     4        4     864000000

>>         pllh_pix_prediv       1        1       3375000

>>            pllh_pix           0        0        337500

>>         pllh_aux              1        1     216000000

>>            vec                0        0     108000000

>>         pllh_rcal_prediv      1        1       3375000

>>            pllh_rcal          0        0        337500

>>      plld                     3        3    2000000024

>>         plld_dsi1             0        0       7812501

>>         plld_dsi0             0        0       7812501

>>         plld_per              3        3     500000006

>>            gp1                1        1      25000000

>>            uart               1        2      47999625

>>         plld_core             2        2     500000006

>>            sdram              0        0     166666668

>>      pllc                     3        3    2400000000

>>         pllc_per              1        1    1200000000

>>            emmc               0        0     200000000

>>         pllc_core2            0        0       9375000

>>         pllc_core1            0        0       9375000

>>         pllc_core0            2        2    1200000000

>>            vpu                1        1     700000000

>>               aux_spi2        0        0     700000000

>>               aux_spi1        0        0     700000000

>>               aux_uart        0        0     700000000

>>               peri_image      0        0     700000000

>>      plla                     2        2    2250000000

>>         plla_ccp2             0        0       8789063

>>         plla_dsi0             0        0       8789063

>>         plla_core             1        1     750000000

>>            h264               0        0     250000000

>>            isp                0        0     250000000

>>   dsi1p                       0        0             0

>>   dsi0p                       0        0             0

>>   dsi1e                       0        0             0

>>   dsi0e                       0        0             0

>>   cam1                        0        0             0

>>   cam0                        0        0             0

>>   dpi                         0        0             0

>>   tec                         0        0             0

>>   smi                         0        0             0

>>   slim                        0        0             0

>>   gp0                         0        0             0

>>   dft                         0        0             0

>>   aveo                        0        0             0

>>   pcm                         0        0             0

>>   pwm                         0        0             0

>>   hsm                         0        0             0

>>

>> It shows small differences with real hardware due other missing

>> peripherals for which the driver turn the clock off (like tsens).

>>

>> Luc Michel (15):

>>    hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro

>>    hw/core/clock: trace clock values in Hz instead of ns

>>    hw/core/clock: add the clock_new helper function

>>    hw/arm/raspi: fix CPRMAN base address

>>    hw/arm/raspi: add a skeleton implementation of the CPRMAN

>>    hw/misc/bcm2835_cprman: add a PLL skeleton implementation

>>    hw/misc/bcm2835_cprman: implement PLLs behaviour

>>    hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation

>>    hw/misc/bcm2835_cprman: implement PLL channels behaviour

>>    hw/misc/bcm2835_cprman: add a clock mux skeleton implementation

>>    hw/misc/bcm2835_cprman: implement clock mux behaviour

>>    hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer

>>    hw/misc/bcm2835_cprman: add sane reset values to the registers

>>    hw/char/pl011: add a clock input

>>    hw/arm/bcm2835_peripherals: connect the UART clock

>>

>>   include/hw/arm/bcm2835_peripherals.h       |    5 +-

>>   include/hw/arm/raspi_platform.h            |    5 +-

>>   include/hw/char/pl011.h                    |    1 +

>>   include/hw/clock.h                         |   18 +

>>   include/hw/misc/bcm2835_cprman.h           |  210 ++++

>>   include/hw/misc/bcm2835_cprman_internals.h | 1019 ++++++++++++++++++++

>>   hw/arm/bcm2835_peripherals.c               |   15 +-

>>   hw/char/pl011.c                            |   45 +

>>   hw/core/clock.c                            |   21 +-

>>   hw/misc/bcm2835_cprman.c                   |  808 ++++++++++++++++

>>   hw/char/trace-events                       |    1 +

>>   hw/core/trace-events                       |    4 +-

>>   hw/misc/meson.build                        |    1 +

>>   hw/misc/trace-events                       |    5 +

>>   14 files changed, 2146 insertions(+), 12 deletions(-)

>>   create mode 100644 include/hw/misc/bcm2835_cprman.h

>>   create mode 100644 include/hw/misc/bcm2835_cprman_internals.h

>>   create mode 100644 hw/misc/bcm2835_cprman.c

>>
Philippe Mathieu-Daudé Oct. 23, 2020, 11:13 a.m. UTC | #13
On 10/23/20 5:55 AM, Guenter Roeck wrote:
> On 10/22/20 3:06 PM, Philippe Mathieu-Daudé wrote:
>> Cc'ing Guenter who had a similar patch and might be interested
>> to test :)
>>
> I applied the series on top of qemu mainline and ran all my test with it
> (raspi2 with qemu-system-arm as well as qemu-system-aarch64, and raspi3
> in both big endian and little endian mode with qemu-system-aarch64.
> All tests passed without error or kernel warning.

Awesome :) I'm glad we finally get this complex part implemented.

Regards,

Phil.

> 
> For the series:
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Guenter
Philippe Mathieu-Daudé Oct. 27, 2020, 8:55 a.m. UTC | #14
Hi Peter,

On 10/19/20 9:31 PM, Peter Maydell wrote:
> On Mon, 19 Oct 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sat, 10 Oct 2020 at 14:57, Luc Michel <luc@lmichel.fr> wrote:
>>>
>>> v2 -> v3:
>>>   - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
>>>   - patch 03: commit message typo [Clement]
>>>   - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
>>>               reg_cm replaced with reg_ctl and reg_div. Add some
>>>               comments for clarity. [Phil]
>>>   - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
>>>               correctly. [Phil]
>>>   - patch 11: replaced manual bitfield extraction with extract32 [Phil]
>>>   - patch 11: added a visual representation of CM_DIV for clarity [Phil]
>>>   - patch 11: added a missing return in clock_mux_update.
>>
>>
>>
>> Applied to target-arm.next, thanks.
> 
> Dropped again due to segv in 'make check' when running with
> clang sanitizer, which I gather from irc that you're looking
> into.

The fix has been merged as commit a6e9b9123e7
("hw/core/qdev-clock: add a reference on aliased clocks")
and the series also got:
Tested-by: Guenter Roeck <linux@roeck-us.net>

Hopefully it will make it for 5.2 :)

Thanks,

Phil.
Peter Maydell Oct. 27, 2020, 11:11 a.m. UTC | #15
On Tue, 27 Oct 2020 at 08:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 10/19/20 9:31 PM, Peter Maydell wrote:
> > On Mon, 19 Oct 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Sat, 10 Oct 2020 at 14:57, Luc Michel <luc@lmichel.fr> wrote:
> >>>
> >>> v2 -> v3:
> >>>   - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
> >>>   - patch 03: commit message typo [Clement]
> >>>   - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
> >>>               reg_cm replaced with reg_ctl and reg_div. Add some
> >>>               comments for clarity. [Phil]
> >>>   - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
> >>>               correctly. [Phil]
> >>>   - patch 11: replaced manual bitfield extraction with extract32 [Phil]
> >>>   - patch 11: added a visual representation of CM_DIV for clarity [Phil]
> >>>   - patch 11: added a missing return in clock_mux_update.
> >>
> >>
> >>
> >> Applied to target-arm.next, thanks.
> >
> > Dropped again due to segv in 'make check' when running with
> > clang sanitizer, which I gather from irc that you're looking
> > into.
>
> The fix has been merged as commit a6e9b9123e7
> ("hw/core/qdev-clock: add a reference on aliased clocks")
> and the series also got:
> Tested-by: Guenter Roeck <linux@roeck-us.net>
>
> Hopefully it will make it for 5.2 :)



Applied to target-arm.next, thanks.

-- PMM