Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Philippe Mathieu-Daudé]

On 2/10/20 10:50 AM, Igor Mammedov wrote:
> On Sat,  8 Feb 2020 17:56:40 +0100
> Philippe Mathieu-Daudé <address@hidden> wrote:
>
> > We want to have a common class_init(). The only value that
> > matters (and changes) is the board revision.
> > Pass the board_rev as class_data to class_init().
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > ---
> >   hw/arm/raspi.c | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > index 62b8df3c2e..fbfcd29732 100644
> > --- a/hw/arm/raspi.c
> > +++ b/hw/arm/raspi.c
> > @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
> >       /*< private >*/
> >       MachineClass parent_obj;
> >       /*< public >*/
> > +    uint32_t board_rev;
> >   } RaspiMachineClass;
> >   
> >   #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
> > @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, 
> > size_t ram_size)
> >       arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
> >   }
> >   
> > -static void raspi_init(MachineState *machine, uint32_t board_rev)
> > +static void raspi_init(MachineState *machine)
> >   {
> > +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
> >       RaspiMachineState *s = RASPI_MACHINE(machine);
> > +    uint32_t board_rev = mc->board_rev;
> >       int version = board_version(board_rev);
> >       uint64_t ram_size = board_ram_size(board_rev);
> >       uint32_t vcram_size;
> > @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t 
> > board_rev)
> >   
> >   static void raspi2_init(MachineState *machine)
> >   {
> > -    raspi_init(machine, 0xa21041);
> > +    raspi_init(machine);
> >   }
> >   
> >   static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> > +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> > +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
> >   
> > +    rmc->board_rev = board_rev;
>
> instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
> using it here, I'd just do
>
>         rmc->board_rev = 0xa21041;
>
> using value specific for each leaf class

Leaf classes are removed in patch #12 "Use a unique 
raspi_machine_class_init() method", see more uses of .class_data from v2:
https://www.mail-archive.com/address@hidden/msg677164.html
https://www.mail-archive.com/address@hidden/msg677166.html

Are you disagreeing with them? Then we should document .class_data as 
deprecated and show example of good code.

> with this change
>   Reviewed-by: Igor Mammedov <address@hidden>
>
> >       mc->desc = "Raspberry Pi 2B";
> >       mc->init = raspi2_init;
> >       mc->block_default_type = IF_SD;
> > @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, 
> > void *data)
> >   #ifdef TARGET_AARCH64
> >   static void raspi3_init(MachineState *machine)
> >   {
> > -    raspi_init(machine, 0xa02082);
> > +    raspi_init(machine);
> >   }
> >   
> >   static void raspi3_machine_class_init(ObjectClass *oc, void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> > +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> > +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
> >   
> > +    rmc->board_rev = board_rev;
> >       mc->desc = "Raspberry Pi 3B";
> >       mc->init = raspi3_init;
> >       mc->block_default_type = IF_SD;
> > @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
> >           .name           = MACHINE_TYPE_NAME("raspi2"),
> >           .parent         = TYPE_RASPI_MACHINE,
> >           .class_init     = raspi2_machine_class_init,
> > +        .class_data     = (void *)0xa21041,
> >   #ifdef TARGET_AARCH64
> >       }, {
> >           .name           = MACHINE_TYPE_NAME("raspi3"),
> >           .parent         = TYPE_RASPI_MACHINE,
> >           .class_init     = raspi3_machine_class_init,
> > +        .class_data     = (void *)0xa02082,
> >   #endif
> >       }, {
> >           .name           = TYPE_RASPI_MACHINE,