Message ID | 20200925101731.2159827-1-luc@lmichel.fr |
---|---|
Headers | show |
Series | raspi: add the bcm2835 cprman clock manager | expand |
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
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>
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 > * >
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) >
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) >> >
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...
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) > >> > > --
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).