Re: [Qemu-devel] [PATCH 1/2] hw/mips: add initial Cluster Power Controller support

[Leon Alrae <leon.alrae@imgtec.com>]

On 26/02/16 16:49, Peter Maydell wrote:
> On 26 February 2016 at 16:19, Leon Alrae <leon.alrae@imgtec.com> wrote:
>> Cluster Power Controller (CPC) is responsible for power management in
>> multiprocessing system. It provides registers to control the power and the
>> clock frequency of the individual elements in the system.
>>
>> This patch implements only three registers that are used to control the
>> power state of each VP on a single core:
>> * VP Run is a write-only register used to set each VP to the run state
>> * VP Stop is a write-only register used to set each VP to the suspend state
>> * VP Running is a read-only register indicating the run state of each VP
>>
>> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
>> ---
>>  default-configs/mips-softmmu.mak     |   1 +
>>  default-configs/mips64-softmmu.mak   |   1 +
>>  default-configs/mips64el-softmmu.mak |   1 +
>>  default-configs/mipsel-softmmu.mak   |   1 +
> 
> A separate thing, but maybe it would be worth having a mips-softmmu-common.mak
> that all the mips*-softmmu.mak include to avoid having to repeat CONFIG defines
> in four places like this.

Good idea. I'll prepare a separate patch for that.

And the comments below will be addressed in v2 (it'll also include few
extra changes).

Thanks,
Leon

> 
>> +static void mips_cpc_init(Object *obj)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    MIPSCPCState *s = MIPS_CPC(obj);
>> +
>> +    memory_region_init_io(&s->mr, OBJECT(s), &cpc_ops, s, "mips-cpc",
>> +                          CPC_ADDRSPACE_SZ);
>> +    sysbus_init_mmio(sbd, &s->mr);
>> +}
>> +
>> +static const TypeInfo mips_cpc_info = {
>> +    .name          = TYPE_MIPS_CPC,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(MIPSCPCState),
>> +    .instance_init = mips_cpc_init,
>> +};
> 
> I suspect you need a reset method.
> 
>> +
>> +static void mips_cpc_register_types(void)
>> +{
>> +    type_register_static(&mips_cpc_info);
>> +}
>> +
>> +type_init(mips_cpc_register_types)
> 
>> +typedef struct MIPSCPCState {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion mr;
>> +    uint64_t vp_running; /* Indicates which VP's are in the run state */
> 
> This is state, so you need a VMState structure to migrate it correctly.
> 
>> +} MIPSCPCState;
>> +
>> +#endif /* MIPS_CPC_H */
>> --
>> 2.1.0
> 
> thanks
> -- PMM
>