| Message ID | 20220808210643.2192602-4-mail@conchuod.ie (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU: Fix RISC-V virt & spike machines' dtbs | expand |
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. 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. 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
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 >
On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote: > > 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. It probably breaks Linux, too. > 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? Perhaps. The other option is adding 'simple-mfd' to the 'test' node compatible. That would work for Linux. Not sure for FreeBSD. Rob
On 09/08/2022 00:10, Rob Herring wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote: >> >> On 08/08/2022 22:28, Jessica Clarke wrote: >>> 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? > > Perhaps. > > The other option is adding 'simple-mfd' to the 'test' node compatible. > That would work for Linux. Not sure for FreeBSD. Right, of course I was missing something in my understanding. The probe flow on Linux came back to me on the bike this morning after reading this and I felt like an idiot for missing that in the devicetrees I looked at! @Jess, which does FreeBSD prefer? top level or add the extra compatible? Thanks, Conor.
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);