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

[Luc Michel]

On 13:33 Sat 10 Oct     , Luc Michel wrote:
> 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.
But the +4 is actually on the wrong side of the comparison :) I'll fix
that.
> 
> > 
> > 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;
> > >      }
> > >  }
> > [...]
> 
> -- 

--