On Tue, Sep 10, 2019 at 01:00:54PM +0200, Cédric Le Goater wrote:
> >>> +
> >>> + object_initialize_child(obj, "homer", &chip9->homer,
> >>> sizeof(chip9->homer),
> >>> + TYPE_PNV9_HOMER, &error_abort, NULL);
> >>> + object_property_add_const_link(OBJECT(&chip9->homer), "chip", obj,
> >>> + &error_abort);
> >>
> >> Does HOMER need the chip ? It is not used but we might want to in the
> >> core_max_array() ?
> >
> > sorry, no it is not required, I will remove it.
>
> It seems you will need the chip in core_max_array(). I would keep it.
> See below,
Thanks Cedric, I was not aware of how to use it,
>
> [ ... ]
>
> >>> +static bool core_max_array(void *opaque, hwaddr addr)
>
> Please change the 'void *opaque' function parameter in 'PnvHOMER *homer'
>
> >>> +{
> >>> + PnvHOMER *homer = PNV_HOMER(opaque);
> >>> + PnvHOMERClass *homer_class = PNV_HOMER_GET_CLASS(homer);
> >>> +
> >>> + MachineState *ms = MACHINE(qdev_get_machine());
> >>
> >> Do you need the whole machine or only the chip ?
> >
> > yes, I see it is for active cores in the chip, I can use `nr_cores`
> > defined in PnvChip.
>
>
> If you keep the QOM link above, you can grab it in the realize handler of
> the HOMER model with :
>
> Object *obj;
> Error *local_err = NULL;
>
> obj = object_property_get_link(OBJECT(dev), "chip", &local_err);
> if (!obj) {
> error_propagate(errp, local_err);
> error_prepend(errp, "required link 'chip' not found: ");
> return;
> }
>
> homer->chip = PNV_CHIP(obj);
sure, :+1:
>
> [ ... ]
>
> >>> +
> >>> +/* P9 Pstate table */
> >>> +
> >>
> >> no version ?
> >
> > PNV9_OCC_PSTATE_MAJOR_VERSION is the P9 pstate version.
>
> why isn't it in the switch statement below ?
it is there in the switch statement,
::
+ case PNV9_OCC_PSTATE_MAJOR_VERSION:
+ return 0x90;
::
>
> [ ... ]
>
> >>> +typedef struct PnvHOMER {
> >>> + DeviceState parent;
> >>> +
> >>> + MemoryRegion homer_regs;
> >>
> >> the homer_ prefix is not useful.
> >
> > okay, I will change it to `hregs`.
>
> I would just remove the prefix
will make the change as suggested by removing the prefix.
-- Bala
>
> Thanks,
>
> C.
>
