Re: [PATCH v2 10/15] hw/misc/bcm2835_cprman: add a clock mux skeleton implementation - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Luc Michel <luc@lmichel.fr>, qemu-devel@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Havard Skinnemoen <hskinnemoen@google.com>,
	Andrew Baumann <Andrew.Baumann@microsoft.com>,
	Paul Zimmerman <pauldzim@gmail.com>,
	Niek Linnenbank <nieklinnenbank@gmail.com>,
	qemu-arm@nongnu.org
Subject: Re: [PATCH v2 10/15] hw/misc/bcm2835_cprman: add a clock mux skeleton implementation
Date: Tue, 6 Oct 2020 10:40:17 +0200	[thread overview]
Message-ID: <ea432580-114b-3f85-b4f1-0012bfb008c0@amsat.org> (raw)
In-Reply-To: <20201005195612.1999165-11-luc@lmichel.fr>

On 10/5/20 9:56 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.)
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>  include/hw/misc/bcm2835_cprman.h           |  84 ++++
>  include/hw/misc/bcm2835_cprman_internals.h | 421 +++++++++++++++++++++
>  hw/misc/bcm2835_cprman.c                   | 150 ++++++++
>  3 files changed, 655 insertions(+)
[...]

> +#define FILL_CLOCK_MUX_INIT_INFO(clock_, kind_) \
> +    .cm_offset = R_CM_ ## clock_ ## CTL,        \
> +    FILL_CLOCK_MUX_SRC_MAPPING_INIT_INFO(kind_)
> +
> +static ClockMuxInitInfo CLOCK_MUX_INIT_INFO[] = {
> +    [CPRMAN_CLOCK_GNRIC] = {
> +        .name = "gnric",
> +        FILL_CLOCK_MUX_INIT_INFO(GNRIC, unknown),
> +    },
[...]

> +static inline void update_mux_from_cm(BCM2835CprmanState *s, size_t idx)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < CPRMAN_NUM_CLOCK_MUX; i++) {
> +        if ((CLOCK_MUX_INIT_INFO[i].cm_offset == idx) ||
> +            (CLOCK_MUX_INIT_INFO[i].cm_offset == idx + 4)) {

Maybe add a /* Matches DIV or CTL */ comment. Anyway
FILL_CLOCK_MUX_INIT_INFO() only uses CTL, not DIV, so
+4 check is not necessary.

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

> +            clock_mux_update(&s->clock_muxes[i]);
> +            return;
> +        }
> +    }
> +}
> +
>  #define CASE_PLL_A2W_REGS(pll_) \
>      case R_A2W_ ## pll_ ## _CTRL: \
>      case R_A2W_ ## pll_ ## _ANA0: \
>      case R_A2W_ ## pll_ ## _ANA1: \
>      case R_A2W_ ## pll_ ## _ANA2: \
> @@ -363,10 +438,19 @@ static void cprman_write(void *opaque, hwaddr offset,
>      case R_A2W_PLLH_RCAL:
>      case R_A2W_PLLH_PIX:
>      case R_A2W_PLLB_ARM:
>          update_channel_from_a2w(s, idx);
>          break;
> +
> +    case R_CM_GNRICCTL ... R_CM_SMIDIV:
> +    case R_CM_TCNTCNT ... R_CM_VECDIV:
> +    case R_CM_PULSECTL ... R_CM_PULSEDIV:
> +    case R_CM_SDCCTL ... R_CM_ARMCTL:
> +    case R_CM_AVEOCTL ... R_CM_EMMCDIV:
> +    case R_CM_EMMC2CTL ... R_CM_EMMC2DIV:
> +        update_mux_from_cm(s, idx);
> +        break;
>      }
>  }
[...]