mbox series

[00/14] raspi: add the bcm2835 cprman clock manager

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

Message

Luc Michel Sept. 25, 2020, 10:17 a.m. UTC
Hi,

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

Patches 1-3 are preliminary changes, patches 4-12 are the actual
implementation.

The two last patches add a clock input to the PL011 and
connect it to the cprman and are RFC.

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 (14):
  hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro
  hw/core/clock: trace clock values in Hz instead of ns
  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                         |    5 +
 include/hw/misc/bcm2835_cprman.h           |  209 ++++
 include/hw/misc/bcm2835_cprman_internals.h | 1018 ++++++++++++++++++++
 hw/arm/bcm2835_peripherals.c               |   15 +-
 hw/char/pl011.c                            |   45 +
 hw/core/clock.c                            |    6 +-
 hw/misc/bcm2835_cprman.c                   |  810 ++++++++++++++++
 hw/char/trace-events                       |    1 +
 hw/core/trace-events                       |    4 +-
 hw/misc/meson.build                        |    1 +
 hw/misc/trace-events                       |    5 +
 14 files changed, 2118 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

no-reply@patchew.org Sept. 25, 2020, 11:56 a.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20200925101731.2159827-1-luc@lmichel.fr/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
Running test qtest-aarch64: qmp-test
Broken pipe
../src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
ERROR qtest-aarch64: device-introspect-test - too few tests run (expected 6, got 5)
Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory
qemu-system-x86_64: falling back to tcg
make: *** [run-test-166] Error 1
make: *** Waiting for unfinished jobs....

Looking for expected file 'tests/data/acpi/q35/FACP.ipmibt'
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=4e540e8b62f746b082ad47693e2521fa', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-d4ues5y_/src/docker-src.2020-09-25-07.47.48.18126:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=4e540e8b62f746b082ad47693e2521fa
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-d4ues5y_/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    8m13.133s
user    0m20.580s


The full log is available at
http://patchew.org/logs/20200925101731.2159827-1-luc@lmichel.fr/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Philippe Mathieu-Daudé Sept. 25, 2020, 12:55 p.m. UTC | #2
Cc'ing Paul/Niek/Havard.

On 9/25/20 12:17 PM, Luc Michel wrote:
> Hi,
> 
> This series add the BCM2835 cprman clock manager peripheral to the
> Raspberry Pi machine.
> 
> Patches 1-3 are preliminary changes, patches 4-12 are the actual
> implementation.
> 
> The two last patches add a clock input to the PL011 and
> connect it to the cprman and are RFC.
> 
> 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 (14):
>   hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro
>   hw/core/clock: trace clock values in Hz instead of ns
>   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
> 

>  14 files changed, 2118 insertions(+), 12 deletions(-)
ouch... o_O huge device...

Tested on top of my raspi branch with raspi[0123]:
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé Sept. 26, 2020, 8:36 p.m. UTC | #3
On 9/25/20 12:17 PM, Luc Michel wrote:
> Signed-off-by: Luc Michel <luc@lmichel.fr>

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

> ---
>  include/hw/clock.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index d357594df9..c93e6113cd 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -79,10 +79,15 @@ struct Clock {
>  extern const VMStateDescription vmstate_clock;
>  #define VMSTATE_CLOCK(field, state) \
>      VMSTATE_CLOCK_V(field, state, 0)
>  #define VMSTATE_CLOCK_V(field, state, version) \
>      VMSTATE_STRUCT_POINTER_V(field, state, version, vmstate_clock, Clock)
> +#define VMSTATE_ARRAY_CLOCK(field, state, num) \
> +    VMSTATE_ARRAY_CLOCK_V(field, state, num, 0)
> +#define VMSTATE_ARRAY_CLOCK_V(field, state, num, version)          \
> +    VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(field, state, num, version, \
> +                                       vmstate_clock, Clock)
>  
>  /**
>   * clock_setup_canonical_path:
>   * @clk: clock
>   *
>
Philippe Mathieu-Daudé Sept. 26, 2020, 9:40 p.m. UTC | #4
On 9/25/20 12:17 PM, Luc Michel wrote:
> A clock mux can be configured to select one of its 10 sources through
> the cm_ctl register. It also embeds yet another clock divider, composed
> of an integer part and a fractionnal part. The number of bits of each

Typo "fractional".

> part is mux dependant.

Typo "dependent"?

> 
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>  hw/misc/bcm2835_cprman.c | 43 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 8df2db0fd9..75bc11939b 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -229,19 +229,60 @@ static const TypeInfo cprman_pll_channel_info = {
>  };
>  
>  
>  /* clock mux */
>  
> +static bool clock_mux_is_enabled(CprmanClockMuxState *mux)
> +{
> +    return FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, ENABLE);
> +}
> +
>  static void clock_mux_update(CprmanClockMuxState *mux)
>  {
> -    clock_update(mux->out, 0);
> +    uint64_t freq, div;
> +    uint32_t src = FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, SRC);
> +    bool enabled = clock_mux_is_enabled(mux);
> +
> +    *mux->reg_cm = FIELD_DP32(*mux->reg_cm, CM_CLOCKx_CTL, BUSY, enabled);
> +
> +    if (!enabled) {
> +        clock_update(mux->out, 0);
> +        return;
> +    }
> +
> +    freq = clock_get_hz(mux->srcs[src]);
> +
> +    if (mux->int_bits == 0 && mux->frac_bits == 0) {
> +        clock_update_hz(mux->out, freq);
> +        return;
> +    }
> +
> +    /*
> +     * The divider has an integer and a fractional part. The size of each part
> +     * varies with the muxes (int_bits and frac_bits). Both parts are
> +     * concatenated, with the integer part always starting at bit 12.
> +     */
> +    div = mux->reg_cm[1] >> (R_CM_CLOCKx_DIV_FRAC_LENGTH - mux->frac_bits);
> +    div &= (1 << (mux->int_bits + mux->frac_bits)) - 1;
> +
> +    freq = (freq << mux->frac_bits) / div;
> +
> +    clock_update_hz(mux->out, freq);
>  }
>  
>  static void clock_mux_src_update(void *opaque)
>  {
>      CprmanClockMuxState **backref = opaque;
>      CprmanClockMuxState *s = *backref;
> +    CprmanClockMuxSource src = backref - s->backref;
> +    uint32_t current_src;
> +
> +    current_src = FIELD_EX32(*s->reg_cm, CM_CLOCKx_CTL, SRC);
> +
> +    if (current_src != src) {
> +        return;
> +    }
>  
>      clock_mux_update(s);
>  }
>  
>  static void clock_mux_init(Object *obj)
>
Philippe Mathieu-Daudé Oct. 2, 2020, 2:51 p.m. UTC | #5
On 9/26/20 11:40 PM, Philippe Mathieu-Daudé wrote:
> On 9/25/20 12:17 PM, Luc Michel wrote:
>> A clock mux can be configured to select one of its 10 sources through
>> the cm_ctl register. It also embeds yet another clock divider, composed
>> of an integer part and a fractionnal part. The number of bits of each
> 
> Typo "fractional".
> 
>> part is mux dependant.
> 
> Typo "dependent"?
> 
>>
>> Signed-off-by: Luc Michel <luc@lmichel.fr>
>> ---
>>  hw/misc/bcm2835_cprman.c | 43 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>> index 8df2db0fd9..75bc11939b 100644
>> --- a/hw/misc/bcm2835_cprman.c
>> +++ b/hw/misc/bcm2835_cprman.c
>> @@ -229,19 +229,60 @@ static const TypeInfo cprman_pll_channel_info = {
>>  };
>>  
>>  
>>  /* clock mux */
>>  
>> +static bool clock_mux_is_enabled(CprmanClockMuxState *mux)
>> +{
>> +    return FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, ENABLE);
>> +}
>> +
>>  static void clock_mux_update(CprmanClockMuxState *mux)
>>  {
>> -    clock_update(mux->out, 0);
>> +    uint64_t freq, div;
>> +    uint32_t src = FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, SRC);
>> +    bool enabled = clock_mux_is_enabled(mux);
>> +
>> +    *mux->reg_cm = FIELD_DP32(*mux->reg_cm, CM_CLOCKx_CTL, BUSY, enabled);
>> +
>> +    if (!enabled) {
>> +        clock_update(mux->out, 0);
>> +        return;
>> +    }
>> +
>> +    freq = clock_get_hz(mux->srcs[src]);
>> +
>> +    if (mux->int_bits == 0 && mux->frac_bits == 0) {
>> +        clock_update_hz(mux->out, freq);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * The divider has an integer and a fractional part. The size of each part
>> +     * varies with the muxes (int_bits and frac_bits). Both parts are
>> +     * concatenated, with the integer part always starting at bit 12.
>> +     */
>> +    div = mux->reg_cm[1] >> (R_CM_CLOCKx_DIV_FRAC_LENGTH - mux->frac_bits);
>> +    div &= (1 << (mux->int_bits + mux->frac_bits)) - 1;

Eventually:

    div &= MAKE_64BIT_MASK(mux->int_bits + mux->frac_bits, 64);

>> +
>> +    freq = (freq << mux->frac_bits) / div;

Maybe:

    freq = muldiv64(freq, 1 << mux->frac_bits, div);

>> +
>> +    clock_update_hz(mux->out, freq);
>>  }
>>  
>>  static void clock_mux_src_update(void *opaque)
>>  {
>>      CprmanClockMuxState **backref = opaque;
>>      CprmanClockMuxState *s = *backref;
>> +    CprmanClockMuxSource src = backref - s->backref;
>> +    uint32_t current_src;

Maybe avoid current_src and use in place.

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

>> +
>> +    current_src = FIELD_EX32(*s->reg_cm, CM_CLOCKx_CTL, SRC);
>> +
>> +    if (current_src != src) {
>> +        return;
>> +    }
>>  
>>      clock_mux_update(s);
>>  }
>>  
>>  static void clock_mux_init(Object *obj)
>>
>
Philippe Mathieu-Daudé Oct. 2, 2020, 3:34 p.m. UTC | #6
On 10/2/20 4:42 PM, Philippe Mathieu-Daudé wrote:
> On 9/25/20 12:17 PM, Luc Michel wrote:
>> The clock multiplexers are the last clock stage in the cprman. Each mux
>> outputs one clock signal that goes out of the cprman to the SoC
>> peripherals.
>>
>> Each mux has at most 10 sources. The sources 0 to 3 are common to all
>> muxes. They are:
>>    0. ground (no clock signal)
>>    1. the main oscillator (xosc)
>>    2. "test debug 0" clock
>>    3. "test debug 1" clock
>>
>> Test debug 0 and 1 are actual clock muxes that can be used as sources to
>> other muxes (for debug purpose).
>>
>> Sources 4 to 9 are mux specific and can be unpopulated (grounded). Those
>> sources are fed by the PLL channels outputs.
>>
>> One corner case exists for DSI0E and DSI0P muxes. They have their source
>> number 4 connected to an intermediate multiplexer that can select
>> between PLLA-DSI0 and PLLD-DSI0 channel. This multiplexer is called
>> DSI0HSCK and is not a clock mux as such. It is really a simple mux from
>> the hardware point of view (see https://elinux.org/The_Undocumented_Pi).
>> This mux is not implemented in this commit.
>>
>> Note that there is some muxes for which sources are unknown (because of
>> a lack of documentation). For those cases all the sources are connected
>> to ground in this implementation.
>>
>> Each clock mux output is exported by the cprman at the qdev level,
>> adding the suffix '-out' to the mux name to form the output clock name.
>> (E.g. the 'uart' mux sees its output exported as 'uart-out' at the
>> cprman level.)
>>
>> Signed-off-by: Luc Michel <luc@lmichel.fr>
>> ---
[...]
>>  struct BCM2835CprmanState {
>>      /*< private >*/
>>      SysBusDevice parent_obj;
>>  
>>      /*< public >*/
>>      MemoryRegion iomem;
>>  
>>      CprmanPLLState plls[CPRMAN_NUM_PLL];
>>      CprmanPLLChannelState channels[CPRMAN_NUM_PLL_CHANNEL];
>> +    CprmanClockMuxState clock_muxes[CPRMAN_NUM_CLOCK_MUX];
>>  
>>      uint32_t regs[CPRMAN_NUM_REGS];
>>      uint32_t xosc_freq;
>>  
>>      Clock *xosc;
>> +    Clock *gnd;
> 
> This one seems to belong to MachineState in "hw/boards.h".

Although it might be easier to have a singleton in hw/core/clock.c...
Luc Michel Oct. 4, 2020, 6:37 p.m. UTC | #7
On 16:51 Fri 02 Oct     , Philippe Mathieu-Daudé wrote:
> On 9/26/20 11:40 PM, Philippe Mathieu-Daudé wrote:

> > On 9/25/20 12:17 PM, Luc Michel wrote:

> >> A clock mux can be configured to select one of its 10 sources through

> >> the cm_ctl register. It also embeds yet another clock divider, composed

> >> of an integer part and a fractionnal part. The number of bits of each

> > 

> > Typo "fractional".

> > 

> >> part is mux dependant.

> > 

> > Typo "dependent"?

> > 

> >>

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

> >> ---

> >>  hw/misc/bcm2835_cprman.c | 43 +++++++++++++++++++++++++++++++++++++++-

> >>  1 file changed, 42 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c

> >> index 8df2db0fd9..75bc11939b 100644

> >> --- a/hw/misc/bcm2835_cprman.c

> >> +++ b/hw/misc/bcm2835_cprman.c

> >> @@ -229,19 +229,60 @@ static const TypeInfo cprman_pll_channel_info = {

> >>  };

> >>  

> >>  

> >>  /* clock mux */

> >>  

> >> +static bool clock_mux_is_enabled(CprmanClockMuxState *mux)

> >> +{

> >> +    return FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, ENABLE);

> >> +}

> >> +

> >>  static void clock_mux_update(CprmanClockMuxState *mux)

> >>  {

> >> -    clock_update(mux->out, 0);

> >> +    uint64_t freq, div;

> >> +    uint32_t src = FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, SRC);

> >> +    bool enabled = clock_mux_is_enabled(mux);

> >> +

> >> +    *mux->reg_cm = FIELD_DP32(*mux->reg_cm, CM_CLOCKx_CTL, BUSY, enabled);

> >> +

> >> +    if (!enabled) {

> >> +        clock_update(mux->out, 0);

> >> +        return;

> >> +    }

> >> +

> >> +    freq = clock_get_hz(mux->srcs[src]);

> >> +

> >> +    if (mux->int_bits == 0 && mux->frac_bits == 0) {

> >> +        clock_update_hz(mux->out, freq);

> >> +        return;

> >> +    }

> >> +

> >> +    /*

> >> +     * The divider has an integer and a fractional part. The size of each part

> >> +     * varies with the muxes (int_bits and frac_bits). Both parts are

> >> +     * concatenated, with the integer part always starting at bit 12.

> >> +     */

> >> +    div = mux->reg_cm[1] >> (R_CM_CLOCKx_DIV_FRAC_LENGTH - mux->frac_bits);

> >> +    div &= (1 << (mux->int_bits + mux->frac_bits)) - 1;

> 

> Eventually:

> 

>     div &= MAKE_64BIT_MASK(mux->int_bits + mux->frac_bits, 64);

I think this is MAKE_64BIT_MASK(0, mux->int_bits + mux->frac_bits)
(The shift macro parameter is 0 to have the ones positioned at the
mask's LSBs.
I'll use this macro in my next re-roll.

> 

> >> +

> >> +    freq = (freq << mux->frac_bits) / div;

> 

> Maybe:

> 

>     freq = muldiv64(freq, 1 << mux->frac_bits, div);

OK

> 

> >> +

> >> +    clock_update_hz(mux->out, freq);

> >>  }

> >>  

> >>  static void clock_mux_src_update(void *opaque)

> >>  {

> >>      CprmanClockMuxState **backref = opaque;

> >>      CprmanClockMuxState *s = *backref;

> >> +    CprmanClockMuxSource src = backref - s->backref;

> >> +    uint32_t current_src;

> 

> Maybe avoid current_src and use in place.

OK

> 

> Otherwise:

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

Thanks !

-- 
Luc

> 

> >> +

> >> +    current_src = FIELD_EX32(*s->reg_cm, CM_CLOCKx_CTL, SRC);

> >> +

> >> +    if (current_src != src) {

> >> +        return;

> >> +    }

> >>  

> >>      clock_mux_update(s);

> >>  }

> >>  

> >>  static void clock_mux_init(Object *obj)

> >>

> > 


--
Luc Michel Oct. 5, 2020, 7:50 p.m. UTC | #8
On 20:37 Sun 04 Oct     , Luc Michel wrote:
> On 16:51 Fri 02 Oct     , Philippe Mathieu-Daudé wrote:
> > On 9/26/20 11:40 PM, Philippe Mathieu-Daudé wrote:
> > > On 9/25/20 12:17 PM, Luc Michel wrote:
> > >> A clock mux can be configured to select one of its 10 sources through
> > >> the cm_ctl register. It also embeds yet another clock divider, composed
> > >> of an integer part and a fractionnal part. The number of bits of each
> > > 
> > > Typo "fractional".
> > > 
> > >> part is mux dependant.
> > > 
> > > Typo "dependent"?
> > > 
> > >>
> > >> Signed-off-by: Luc Michel <luc@lmichel.fr>
> > >> ---
> > >>  hw/misc/bcm2835_cprman.c | 43 +++++++++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 42 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> > >> index 8df2db0fd9..75bc11939b 100644
> > >> --- a/hw/misc/bcm2835_cprman.c
> > >> +++ b/hw/misc/bcm2835_cprman.c
> > >> @@ -229,19 +229,60 @@ static const TypeInfo cprman_pll_channel_info = {
> > >>  };
> > >>  
> > >>  
> > >>  /* clock mux */
> > >>  
> > >> +static bool clock_mux_is_enabled(CprmanClockMuxState *mux)
> > >> +{
> > >> +    return FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, ENABLE);
> > >> +}
> > >> +
> > >>  static void clock_mux_update(CprmanClockMuxState *mux)
> > >>  {
> > >> -    clock_update(mux->out, 0);
> > >> +    uint64_t freq, div;
> > >> +    uint32_t src = FIELD_EX32(*mux->reg_cm, CM_CLOCKx_CTL, SRC);
> > >> +    bool enabled = clock_mux_is_enabled(mux);
> > >> +
> > >> +    *mux->reg_cm = FIELD_DP32(*mux->reg_cm, CM_CLOCKx_CTL, BUSY, enabled);
> > >> +
> > >> +    if (!enabled) {
> > >> +        clock_update(mux->out, 0);
> > >> +        return;
> > >> +    }
> > >> +
> > >> +    freq = clock_get_hz(mux->srcs[src]);
> > >> +
> > >> +    if (mux->int_bits == 0 && mux->frac_bits == 0) {
> > >> +        clock_update_hz(mux->out, freq);
> > >> +        return;
> > >> +    }
> > >> +
> > >> +    /*
> > >> +     * The divider has an integer and a fractional part. The size of each part
> > >> +     * varies with the muxes (int_bits and frac_bits). Both parts are
> > >> +     * concatenated, with the integer part always starting at bit 12.
> > >> +     */
> > >> +    div = mux->reg_cm[1] >> (R_CM_CLOCKx_DIV_FRAC_LENGTH - mux->frac_bits);
> > >> +    div &= (1 << (mux->int_bits + mux->frac_bits)) - 1;
> > 
> > Eventually:
> > 
> >     div &= MAKE_64BIT_MASK(mux->int_bits + mux->frac_bits, 64);
> I think this is MAKE_64BIT_MASK(0, mux->int_bits + mux->frac_bits)
> (The shift macro parameter is 0 to have the ones positioned at the
> mask's LSBs.
> I'll use this macro in my next re-roll.
Actually, I won't, because switching to muldiv64 implied some
modifications. The muldiv64 divisor parameter is uint32_t. Since there
is no MAKE_32BIT_MASK, and I don't use all the "features" of this macro
anyway (shift = 0 in my case), I'll keep this form (but I'll switch to
uin32_t for div).