Re: [SPAM] [PATCH v1 02/21] hw/arm/ast27x0: Move SSP coprocessor initialization from machine to SoC leve

[Markus Armbruster <armbru@redhat.com>]

Cédric Le Goater <clg@kaod.org> writes:

> On 7/17/25 05:40, Jamin Lin wrote:
>> In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was initialized
>> and realized at the machine level (e.g., AST2700FC). However, to make sure the
>> coprocessors can work together properly—such as using the same SRAM, sharing
>> the SCU, and having consistent memory remapping—we need to change how these
>> devices are set up.
>>
>> This commit moves the SSP coprocessor initialization and realization into the
>> AST2700 SoC (aspeed_soc_ast2700_init() and aspeed_soc_ast2700_realize()).
>> By doing so, the SSP becomes a proper child of the SoC device, rather than
>> the machine.
>>
>> This is a preparation step for future commits that will support shared SCU,
>> SRAM, and memory remap logic—specifically enabling PSP DRAM remap for SSP SDRAM
>> access.
>>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

[...]

>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>> index 6aa3841b69..ffbc32fef2 100644
>> --- a/hw/arm/aspeed_ast27x0.c
>> +++ b/hw/arm/aspeed_ast27x0.c

[...]

>> @@ -610,9 +619,35 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
>>       return true;
>>   }
>>   +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error **errp)
>
> I would pass 'Aspeed27x0SoCState *' instead.
>
>> +{
>> +    Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
>> +    AspeedSoCState *s = ASPEED_SOC(dev);
>> +    Clock *sysclk;
>> +
>> +    sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
>> +    clock_set_hz(sysclk, 200000000ULL);
>> +    qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
>> +
>> +    memory_region_init(&a->ssp.memory, OBJECT(&a->ssp), "ssp-memory",
>> +                       UINT64_MAX);
>> +    if (!object_property_set_link(OBJECT(&a->ssp), "memory",
>> +                                  OBJECT(&a->ssp.memory), &error_abort)) {
>
> please use errp instead.
>
>> +        return false;
>> +    }

object_property_set_link() can return false only when it fails, and it
sets an error when it fails.  Since you pass &error_abort, it cannot
fail (it aborts instead).  Therefore the return value is always true,
and the return statement is dead code.

If object_property_set_link() is not expected to fail, i.e. failure
would be a programming error, use

        object_property_set_link(..., &error_abort);

If failure is not a programming error, passing &error_abort is wrong,
and you need to pass errp instead.

>> +
>> +    if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
>
> same here.

Same argument.

[...]