Re: [PATCH v2 10/15] hw/misc/bcm2835_cprman: add a clock mux skeleton implementation

[Luc Michel]

On 10:40 Tue 06 Oct     , Philippe Mathieu-Daudé wrote:
> 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.
I'll add a comment to clarify this. The +4 check is indeed necessary.
This comes from the fact that for all muxes, CM_CTL and CM_DIV registers are
contiguous. So I treat CprmanClockMuxState.reg_cm as a 2 cells array (as
I do for PLLs and the A2W_ANA regs). I'll clarify this point in the next
re-roll.

> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Thanks!
> 
> > +            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;
> >      }
> >  }
> [...]

--