Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[<Conor.Dooley@microchip.com>]

On 08/08/2022 22:28, Jessica Clarke wrote:
> On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
>>
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> The subnodes of the syscon have been added to the incorrect paths.
>> Rather than add them as subnodes, they were originally added to "/foo"
>> and a later patch moved them to "/soc/foo". Both are incorrect & they
>> should have been added as "/soc/test@###/foo" as "/soc/test" is the
>> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
>> such as:
>>
>> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> Reported-by: Rob Herring <robh@kernel.org>
>> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
>> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
>> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> is written to handle syscon-poweroff/reboot existing as a child of a
> simplebus not a syscon.

I know next to nothing about FreeBSD unfortunately or how it handles
buses. My understanding of simple-bus was that it is supposed to
represent a bus with "things" mapped to addresses, relying on the "reg"
property. And then syscon is used when there is some multifunction
register region that controls multiple features of the hardware.
Since simple-bus defines a reg property and the function nodes do not
define one, I'd like to know how FreeBSD's driver handles that.

> Moreover, what is the point of regmap in this
> case? Its existence suggests the point is for them to *not* be children
> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> just look at the parent. Moving the nodes whilst keeping the property
> doesn’t make sense to me.

That's how syscon bindings are constructed, makes it easier to follow
I suppose if they functions are children of the syscon node. Strictly
I think they don't need to be under the syscon itself, I think they can
also go at the top level - they just aren't valid under the /soc node
as it has been defined as a "simple-bus".

It would appear that the original patch 0e404da007 ("riscv/virt: Add
syscon reboot and poweroff DT nodes") that added them put them at the
top level and it was in the refactor that they got moved to the soc bus.*
Maybe the solution would be to put them back at the top level?

dt-validate will consider it valid, but what does the FreeBSD driver
think of that?

Thanks for your input Jess,
Conor.

* On reflection it looks like my fixes tags are not correct and that
0e404da007 was actually correct but the refactor broke things.

> 
> Jess
> 
>> ---
>> hw/riscv/virt.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 8b2978076e..a98b054545 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/reboot");
>> +    name = g_strdup_printf("/soc/test@%lx/reboot",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/poweroff");
>> +    name = g_strdup_printf("/soc/test@%lx/poweroff",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> -- 
>> 2.37.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>