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

[Rob Herring]

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