Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models

[Havard Skinnemoen]

On Wed, Jul 8, 2020 at 10:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/9/20 2:06 AM, Havard Skinnemoen wrote:
> > On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
> > <hskinnemoen@google.com> wrote:
> >> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> >> wrote:
> >>> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> >>>> +typedef struct NPCM7xxClass {
> >>>> +    DeviceClass         parent;
> >>>
> >>> Similar comment that elsewhere on this series, if NPCM7xxClass not used
> >>> outside of npcm7xx.c, keep it local.
> >>
> >> OK, will do.
> >
> > Turns out it is used in npcm7xx_boards.c, so it has to stay where it is.
>
> Indeed:
>
> static void npcm7xx_load_kernel(MachineState *machine,
>                                 NPCM7xxState *soc)
> {
>     NPCM7xxClass *sc = NPCM7XX_GET_CLASS(soc);
>
>     npcm7xx_binfo.ram_size = machine->ram_size;
>     npcm7xx_binfo.nb_cpus = sc->num_cpus;
>
>     arm_load_kernel(&soc->cpu[0], machine, &npcm7xx_binfo);
> }
>
> This is fine.

It's also used here:

static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
{
    NPCM7xxClass *sc = NPCM7XX_CLASS(object_class_by_name(type));
    MachineClass *mc = MACHINE_CLASS(nmc);

    nmc->soc_type = type;
    mc->default_cpus = mc->min_cpus = mc->max_cpus = sc->num_cpus;
}

>
> Just thinking loudly, we traditionally add the load_kernel() code
> in the machine, because it is often specific to Linux guest, and
> the SoC doesn't need to know about the guest OS.
>
> hw/arm/boot.c contains helpers also useful for firmwares.
>
> The SoC has a link to the DRAM so can get its size.
> All the arm_boot_info fields are specific to this SoC.
> So we could move a lot of code to npcm7xx.c, only declaring:
>
>   void npcm7xx_load_kernel(MachineState *machine,
>                            NPCM7xxState *soc);

I can do that, but it doesn't completely get rid of all references to
NPCM7xxClass. I'm afraid there will always be some amount of coupling
between the machines and corresponding SoCs.

Havard