Re: [PATCH v3 15/16] hw/riscv/atlantis: Extract an Atlantis SoC device

["Philippe Mathieu-Daudé" <philmd@oss.qualcomm.com>]

On 4/9/26 12:47, Philippe Mathieu-Daudé wrote:
> On 3/9/26 07:51, Joel Stanley wrote:
>> Introduce TYPE_TT_ATLANTIS_SOC and move the hart array, AIA, ACLINT,
>> boot ROM, UART and I2C controllers into the new TTAtlantisSoCState. The
>> machine keeps ownership of RAM, device tree generation, firmware
>> loading, and the board-level I2C slaves.
>>
>> This makes it easier to compose the SoC into a more complex machine in
>> the future.
>>
>> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   include/hw/riscv/tt_atlantis.h |  21 ++++-
>>   hw/riscv/tt_atlantis.c         | 144 +++++++++++++++++++++++----------
>>   2 files changed, 119 insertions(+), 46 deletions(-)
> 
> 
>> -struct TTAtlantisState {
>> +struct TTAtlantisSoCState {
>>       /*< private >*/
> 
> While here please drop these legacy private/public comments (QOM
> style doesn't want them anymore).
> 
>> -    MachineState parent;
>> +    DeviceState parent;
>>       /*< public >*/
>> -    Notifier machine_done;
>>       const MemMapEntry *memmap;
>>       MemoryRegion *memory;
>> @@ -39,6 +41,19 @@ struct TTAtlantisState {
>>       DesignWareI2CState i2c[TT_ATL_NUM_I2C];
>>       UnimplementedDeviceState uart1;
>>       MemoryRegion bootrom;
>> +
>> +    uint32_t num_harts;
>> +    char *cpu_type;
>> +};
>> +
>> +struct TTAtlantisState {
>> +    /*< private >*/
>> +    MachineState parent;
>> +
>> +    /*< public >*/
> 
> (Ditto).
> 
>> +    Notifier machine_done;
>> +
>> +    TTAtlantisSoCState soc;
>>   };
> 
> 
>> +static void tt_atlantis_soc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    TTAtlantisSoCState *s = TT_ATLANTIS_SOC(dev);
>>       ram_addr_t lo_ram_size, ram_size;
>> -    int hart_count = machine->smp.cpus;
>> +    int hart_count = s->num_harts;
>>       s->memory = get_system_memory();
> 
> For machine layer this was OK but now this is SOC layer and might be
> provided by machine layer via linked property. IOW this should be
> handled like s->dram below.

(Sorry I'm just noticing the next patch exactly addresses that).

>> -    s->dram = machine->ram;
>> +    if (!s->dram) {
>> +        error_setg(errp, "'dram' link is not set");
>> +        return;
>> +    }
>>       ram_size = memory_region_size(s->dram);
>>       s->memmap = tt_atlantis_memmap;
>> -    object_initialize_child(OBJECT(machine), "soc", &s->cpus,
>> -                            TYPE_RISCV_HART_ARRAY);
>> -    object_property_set_str(OBJECT(&s->cpus), "cpu-type", machine- 
>> >cpu_type,
>> +    /* CPUs */
>> +    object_property_set_str(OBJECT(&s->cpus), "cpu-type", s->cpu_type,
>>                               &error_abort);
>>       object_property_set_int(OBJECT(&s->cpus), "hartid-base", 0,
>>                               &error_abort);
>> @@ -505,7 +524,9 @@ static void tt_atlantis_machine_init(MachineState 
>> *machine)
>>       object_property_set_int(OBJECT(&s->cpus), "resetvec",
>>                               s->memmap[TT_ATL_BOOTROM].base,
>>                               &error_abort);
>> -    sysbus_realize(SYS_BUS_DEVICE(&s->cpus), &error_fatal);
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->cpus), errp)) {
>> +        return;
>> +    }
> 
> Patch LGTM otherwise, thanks!
>