Re: [PATCH 11/14] hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer

[Philippe Mathieu-Daudé]

On 9/25/20 12:17 PM, Luc Michel wrote:
> This simple mux sits between the PLL channels and the DSI0E and DSI0P
> clock muxes. This mux selects between PLLA-DSI0 and PLLD-DSI0 channel
> and outputs the selected signal to source number 4 of DSI0E/P clock
> muxes. It is controlled by the cm_dsi0hsck register.
> 
> Signed-off-by: Luc Michel <luc@lmichel.fr>
> ---
>  include/hw/misc/bcm2835_cprman.h           | 15 +++++
>  include/hw/misc/bcm2835_cprman_internals.h |  6 ++
>  hw/misc/bcm2835_cprman.c                   | 78 +++++++++++++++++++++-
>  3 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/bcm2835_cprman.h 
> b/include/hw/misc/bcm2835_cprman.h
> index c2a89e8e90..5555a299fc 100644
> --- a/include/hw/misc/bcm2835_cprman.h
> +++ b/include/hw/misc/bcm2835_cprman.h
> @@ -171,20 +171,35 @@ typedef struct CprmanClockMuxState {
>       * source number.
>       */
>      struct CprmanClockMuxState *backref[CPRMAN_NUM_CLOCK_MUX_SRC];
>  } CprmanClockMuxState;
>  
> +typedef struct CprmanDsi0HsckMuxState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    CprmanClockMux id;
> +
> +    uint32_t *reg_cm;
> +
> +    Clock *plla_in;
> +    Clock *plld_in;
> +    Clock *out;
> +} CprmanDsi0HsckMuxState;
> +
>  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];
> +    CprmanDsi0HsckMuxState dsi0hsck_mux;
>  
>      uint32_t regs[CPRMAN_NUM_REGS];
>      uint32_t xosc_freq;
>  
>      Clock *xosc;
> diff --git a/include/hw/misc/bcm2835_cprman_internals.h 
> b/include/hw/misc/bcm2835_cprman_internals.h
> index a2b5a1aa50..ad07cf5276 100644
> --- a/include/hw/misc/bcm2835_cprman_internals.h
> +++ b/include/hw/misc/bcm2835_cprman_internals.h
> @@ -13,17 +13,20 @@
>  #include "hw/misc/bcm2835_cprman.h"
>  
>  #define TYPE_CPRMAN_PLL "bcm2835-cprman-pll"
>  #define TYPE_CPRMAN_PLL_CHANNEL "bcm2835-cprman-pll-channel"
>  #define TYPE_CPRMAN_CLOCK_MUX "bcm2835-cprman-clock-mux"
> +#define TYPE_CPRMAN_DSI0HSCK_MUX "bcm2835-cprman-dsi0hsck-mux"
>  
>  DECLARE_INSTANCE_CHECKER(CprmanPLLState, CPRMAN_PLL,
>                           TYPE_CPRMAN_PLL)
>  DECLARE_INSTANCE_CHECKER(CprmanPLLChannelState, CPRMAN_PLL_CHANNEL,
>                           TYPE_CPRMAN_PLL_CHANNEL)
>  DECLARE_INSTANCE_CHECKER(CprmanClockMuxState, CPRMAN_CLOCK_MUX,
>                           TYPE_CPRMAN_CLOCK_MUX)
> +DECLARE_INSTANCE_CHECKER(CprmanDsi0HsckMuxState, CPRMAN_DSI0HSCK_MUX,
> +                         TYPE_CPRMAN_DSI0HSCK_MUX)
>  
>  /* Register map */
>  
>  /* PLLs */
>  REG32(CM_PLLA, 0x104)
> @@ -221,10 +224,13 @@ REG32(CM_LOCK, 0x114)
>      FIELD(CM_LOCK, FLOCKD, 11, 1)
>      FIELD(CM_LOCK, FLOCKC, 10, 1)
>      FIELD(CM_LOCK, FLOCKB, 9, 1)
>      FIELD(CM_LOCK, FLOCKA, 8, 1)
>  
> +REG32(CM_DSI0HSCK, 0x120)
> +    FIELD(CM_DSI0HSCK, SELPLLD, 0, 1)
> +
>  /*
>   * This field is common to all registers. Each register write value must 
> match
>   * the CPRMAN_PASSWORD magic value in its 8 MSB.
>   */
>  FIELD(CPRMAN, PASSWORD, 24, 8)
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 75bc11939b..e576ab2642 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -327,10 +327,62 @@ static const TypeInfo cprman_clock_mux_info = {
>      .class_init = clock_mux_class_init,
>      .instance_init = clock_mux_init,
>  };
>  
>  
> +/* DSI0HSCK mux */
> +
> +static void dsi0hsck_mux_update(CprmanDsi0HsckMuxState *s)
> +{
> +    bool src_is_plld = FIELD_EX32(*s->reg_cm, CM_DSI0HSCK, SELPLLD);
> +    Clock *src = src_is_plld ? s->plld_in : s->plla_in;
> +
> +    clock_update(s->out, clock_get(src));
> +}
> +
> +static void dsi0hsck_mux_in_update(void *opaque)
> +{
> +    dsi0hsck_mux_update(CPRMAN_DSI0HSCK_MUX(opaque));
> +}
> +
> +static void dsi0hsck_mux_init(Object *obj)
> +{
> +    CprmanDsi0HsckMuxState *s = CPRMAN_DSI0HSCK_MUX(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    s->plla_in = qdev_init_clock_in(dev, "plla-in", dsi0hsck_mux_in_update, 
> s);
> +    s->plld_in = qdev_init_clock_in(dev, "plld-in", dsi0hsck_mux_in_update, 
> s);
> +    s->out = qdev_init_clock_out(DEVICE(s), "out");
> +}
> +
> +static const VMStateDescription dsi0hsck_mux_vmstate = {
> +    .name = TYPE_CPRMAN_DSI0HSCK_MUX,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_CLOCK(plla_in, CprmanDsi0HsckMuxState),
> +        VMSTATE_CLOCK(plld_in, CprmanDsi0HsckMuxState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void dsi0hsck_mux_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &dsi0hsck_mux_vmstate;
> +}
> +
> +static const TypeInfo cprman_dsi0hsck_mux_info = {
> +    .name = TYPE_CPRMAN_DSI0HSCK_MUX,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(CprmanDsi0HsckMuxState),
> +    .class_init = dsi0hsck_mux_class_init,
> +    .instance_init = dsi0hsck_mux_init,
> +};
> +
> +
>  /* CPRMAN "top level" model */
>  
>  static uint32_t get_cm_lock(const BCM2835CprmanState *s)
>  {
>      static const int CM_LOCK_MAPPING[] = {
> @@ -488,10 +540,14 @@ static void cprman_write(void *opaque, hwaddr offset,
>      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;
> +
> +    case R_CM_DSI0HSCK:
> +        dsi0hsck_mux_update(&s->dsi0hsck_mux);
> +        break;
>      }
>  }
>  
>  #undef CASE_PLL_A2W_REGS
>  
> @@ -519,10 +575,12 @@ static void cprman_reset(DeviceState *dev)
>  
>      for (i = 0; i < CPRMAN_NUM_PLL_CHANNEL; i++) {
>          device_cold_reset(DEVICE(&s->channels[i]));
>      }
>  
> +    device_cold_reset(DEVICE(&s->dsi0hsck_mux));
> +
>      for (i = 0; i < CPRMAN_NUM_CLOCK_MUX; i++) {
>          device_cold_reset(DEVICE(&s->clock_muxes[i]));
>      }
>  
>      clock_update_hz(s->xosc, s->xosc_freq);
> @@ -560,10 +618,14 @@ static void cprman_init(Object *obj)
>                                  &s->channels[i],
>                                  TYPE_CPRMAN_PLL_CHANNEL);
>          set_pll_channel_init_info(s, &s->channels[i], i);
>      }
>  
> +    object_initialize_child(obj, "dsi0hsck-mux",
> +                            &s->dsi0hsck_mux, TYPE_CPRMAN_DSI0HSCK_MUX);
> +    s->dsi0hsck_mux.reg_cm = &s->regs[R_CM_DSI0HSCK];
> +
>      for (i = 0; i < CPRMAN_NUM_CLOCK_MUX; i++) {
>          char *alias;
>  
>          object_initialize_child(obj, CLOCK_MUX_INIT_INFO[i].name,
>                                  &s->clock_muxes[i],
> @@ -608,11 +670,11 @@ static void connect_mux_sources(BCM2835CprmanState *s,
>          Clock *src;
>  
>          if (mapping == CPRMAN_CLOCK_SRC_FORCE_GROUND) {
>              src = s->gnd;
>          } else if (mapping == CPRMAN_CLOCK_SRC_DSI0HSCK) {
> -            src = s->gnd; /* TODO */
> +            src = s->dsi0hsck_mux.out;
>          } else if (i < CPRMAN_CLOCK_SRC_PLLA) {
>              src = CLK_SRC_MAPPING[i];
>          } else {
>              src = s->channels[mapping].out;
>          }
> @@ -646,10 +708,23 @@ static void cprman_realize(DeviceState *dev, Error 
> **errp)
>          if (!qdev_realize(DEVICE(channel), NULL, errp)) {
>              return;
>          }
>      }
>  
> +    {
> +        CprmanDsi0HsckMuxState *dsi0hsck_mux = &s->dsi0hsck_mux;

Why use a new block?

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

> +
> +        clock_set_source(dsi0hsck_mux->plla_in,
> +                         s->channels[CPRMAN_PLLA_CHANNEL_DSI0].out);
> +        clock_set_source(dsi0hsck_mux->plld_in,
> +                         s->channels[CPRMAN_PLLD_CHANNEL_DSI0].out);
> +
> +        if (!qdev_realize(DEVICE(dsi0hsck_mux), NULL, errp)) {
> +            return;
> +        }
> +    }
> +
>      for (i = 0; i < CPRMAN_NUM_CLOCK_MUX; i++) {
>          CprmanClockMuxState *clock_mux = &s->clock_muxes[i];
>  
>          connect_mux_sources(s, clock_mux, 
> CLOCK_MUX_INIT_INFO[i].src_mapping);
>  
> @@ -696,8 +771,9 @@ static void cprman_register_types(void)
>  {
>      type_register_static(&cprman_info);
>      type_register_static(&cprman_pll_info);
>      type_register_static(&cprman_pll_channel_info);
>      type_register_static(&cprman_clock_mux_info);
> +    type_register_static(&cprman_dsi0hsck_mux_info);
>  }
>  
>  type_init(cprman_register_types);
>