Re: [PATCH v3 08/15] hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation

[Philippe Mathieu-Daudé]

On 10/10/20 6:05 PM, Philippe Mathieu-Daudé wrote:
> On 10/10/20 3:57 PM, Luc Michel wrote:
> > PLLs are composed of multiple channels. Each channel outputs one clock
> > signal. They are modeled as one device taking the PLL generated clock as
> > input, and outputting a new clock.
> >
> > A channel shares the CM register with its parent PLL, and has its own
> > A2W_CTRL register. A write to the CM register will trigger an update of
> > the PLL and all its channels, while a write to an A2W_CTRL channel
> > register will update the required channel only.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Luc Michel <luc@lmichel.fr>
> > ---
> >   include/hw/misc/bcm2835_cprman.h           |  44 ++++++
> >   include/hw/misc/bcm2835_cprman_internals.h | 146 +++++++++++++++++++
> >   hw/misc/bcm2835_cprman.c                   | 155 +++++++++++++++++++--
> >   3 files changed, 337 insertions(+), 8 deletions(-)
[...]

> > +#define FILL_PLL_CHANNEL_INIT_INFO_common(pll_, channel_)            \
> > +    .parent = CPRMAN_ ## pll_,                                       \
> > +    .cm_offset = R_CM_ ## pll_,                                      \
> > +    .cm_load_mask = R_CM_ ## pll_ ## _ ## LOAD ## channel_ ## _MASK, \
> > +    .a2w_ctrl_offset = R_A2W_ ## pll_ ## _ ## channel_
> > +
> > +#define FILL_PLL_CHANNEL_INIT_INFO(pll_, channel_)                   \
> > +    FILL_PLL_CHANNEL_INIT_INFO_common(pll_, channel_),               \
> > +    .cm_hold_mask = R_CM_ ## pll_ ## _ ## HOLD ## channel_ ## _MASK, \
> > +    .fixed_divider = 1
> > +
> > +#define FILL_PLL_CHANNEL_INIT_INFO_nohold(pll_, channel_) \
> > +    FILL_PLL_CHANNEL_INIT_INFO_common(pll_, channel_),    \
> > +    .cm_hold_mask = 0
> > +
> > +static PLLChannelInitInfo PLL_CHANNEL_INIT_INFO[] = {
>
> Hmm I missed this static definition in an header.
> As it is local and only include once, I'd prefer match the
> rest of the source tree style and name it .c.inc:
>
> -- >8 --
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 7e415a017c9..9d4c0ee6c73 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -48,7 +48,7 @@
>   #include "migration/vmstate.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/misc/bcm2835_cprman.h"
> -#include "hw/misc/bcm2835_cprman_internals.h"
> +#include "bcm2835_cprman_internals.c.inc"
>   #include "trace.h"
>
>   /* PLL */
> diff --git a/include/hw/misc/bcm2835_cprman_internals.h 
> b/hw/misc/bcm2835_cprman_internals.c.inc
> similarity index 100%
> rename from include/hw/misc/bcm2835_cprman_internals.h
> rename to hw/misc/bcm2835_cprman_internals.c.inc
> ---

This can be applied directly by Peter, or can
be cleaned later. This is not a blocker to get
this series merged.