On 7/17/20 6:59 PM, Havard Skinnemoen wrote:
> +Markus Armbruster
>
> On Fri, Jul 17, 2020 at 5:20 AM Cédric Le Goater <c...@kaod.org> wrote:
>>
>> On 7/17/20 8:02 AM, Havard Skinnemoen wrote:
>>> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
>>> Management Controllers in servers. While the family includes four SoCs,
>>> this patch implements limited support for two of them: NPCM730 (targeted
>>> for Data Center applications) and NPCM750 (targeted for Enterprise
>>> applications).
>>>
>>> This patch includes little more than the bare minimum needed to boot a
>>> Linux kernel built with NPCM7xx support in direct-kernel mode:
>>>
>>> - Two Cortex-A9 CPU cores with built-in periperhals.
>>> - Global Configuration Registers.
>>> - Clock Management.
>>> - 3 Timer Modules with 5 timers each.
>>> - 4 serial ports.
>>>
>>> The chips themselves have a lot more features, some of which will be
>>> added to the model at a later stage.
>>>
>>> Reviewed-by: Tyrone Ting <kft...@nuvoton.com>
>>> Reviewed-by: Joel Stanley <j...@jms.id.au>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>> Signed-off-by: Havard Skinnemoen <hskinnem...@google.com>
>>> ---
>>> include/hw/arm/npcm7xx.h | 85 ++++++++
>>> hw/arm/npcm7xx.c | 407 +++++++++++++++++++++++++++++++++++++++
>>> hw/arm/Kconfig | 5 +
>>> hw/arm/Makefile.objs | 1 +
>>> 4 files changed, 498 insertions(+)
>>> create mode 100644 include/hw/arm/npcm7xx.h
>>> create mode 100644 hw/arm/npcm7xx.c
...
>>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + NPCM7xxState *s = NPCM7XX(dev);
>>> + NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>>> + int i;
>>> +
>>> + if (memory_region_size(s->dram) > NPCM7XX_DRAM_SZ) {
>>> + error_setg(errp, "%s: NPCM7xx cannot address more than %" PRIu64
>>> + " MiB of DRAM", __func__, NPCM7XX_DRAM_SZ / MiB);
>>> + return;
>>> + }
>>> +
>>> + /* CPUs */
>>> + for (i = 0; i < nc->num_cpus; i++) {
>>> + object_property_set_int(OBJECT(&s->cpu[i]), "mp-affinity",
>>> + arm_cpu_mp_affinity(i,
>>> NPCM7XX_MAX_NUM_CPUS),
>>> + &error_abort);
>>> + object_property_set_int(OBJECT(&s->cpu[i]), "reset-cbar",
>>> + NPCM7XX_GIC_CPU_IF_ADDR, &error_abort);
>>> + object_property_set_bool(OBJECT(&s->cpu[i]), "reset-hivecs", true,
>>> + &error_abort);
>>> +
>>> + /* Disable security extensions. */
>>> + object_property_set_bool(OBJECT(&s->cpu[i]), "has_el3", false,
>>> + &error_abort);
>>> +
>>> + if (!qdev_realize(DEVICE(&s->cpu[i]), NULL, errp)) {
>>> + return;
>>> + }
>>> + }
>>> +
>>> + /* A9MPCORE peripherals. Can only fail if we pass bad parameters here.
>>> */
>>> + object_property_set_int(OBJECT(&s->a9mpcore), "num-cpu", nc->num_cpus,
>>> + &error_abort);
>>> + object_property_set_int(OBJECT(&s->a9mpcore), "num-irq",
>>> NPCM7XX_NUM_IRQ,
>>> + &error_abort);
[1]
>>> + sysbus_realize(SYS_BUS_DEVICE(&s->a9mpcore), &error_abort);
[2]
>>
>> shouldn't we test the return value and use errp ? I don't know what
>> was agreed upon.
Per https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html:
>> 1. Internal code failing to set simple properties to predefined
>> values is a programming error, so error_abort is appropriate.
>
> That would be my advice.
>
>> 2. qdev_realize() may fail due to user input, so errors should be
>> propagated.
>
> In general, yes. For a specific device, you may know it can't fail,
> and then &error_abort may be okay.
So it looks correct.
>
> I'm not sure if I got it 100% right, but what I tried to do was to see
> which submodules could possibly fail due to user input, and propagate
> errors from those modules only.
>
> For example, the GCR can fail if the user-provided memory size can't
> be encoded into registers, so that one clearly needs to be propagated.
>
> Other modules don't take any parameters at all, so they can only fail
> due to programming errors, hence error_abort.
>
> I wasn't able to find any way command line options could cause the
> a9mpcore module to fail, but that's one of the cases I'm very unsure
> about, so I'll be happy to propagate errors from that if you (or
> anyone else) think it's needed.
>
> I'm also not sure about the CPUs, but ended up going the other way
> since there's a -cpu option, and it's plausible that the user could
> cause it to fail even though I couldn't find any specific options to
> trigger an error.
