On Wed, Jun 3, 2020 at 5:21 AM André Przywara <andre.przywara@xxxxxxx> wrote: > > On 02/06/2020 00:12, Rob Herring wrote: > > Hi, > > > On Mon, Jun 1, 2020 at 4:15 AM André Przywara <andre.przywara@xxxxxxx> wrote: > >> > >> On 28/05/2020 14:30, André Przywara wrote: > >> > >> Hi, > >> > >>> On 28/05/2020 03:48, Guenter Roeck wrote: > >>> > >>> Hi Guenter, > >>> > >>>> On Wed, May 13, 2020 at 11:30:00AM +0100, Andre Przywara wrote: > >>>>> The devicetree compiler complains when DT nodes without a reg property > >>>>> live inside a (simple) bus node: > >>>>> Warning (simple_bus_reg): Node /bus@8000000/motherboard-bus/refclk32khz > >>>>> missing or empty reg/ranges property > >>>>> > >>>>> Move the fixed clocks, the fixed regulator, the leds and the config bus > >>>>> subtree to the root node, since they do not depend on any busses. > >>>>> > >>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> > >>>> > >>>> This patch results in tracebacks when booting the vexpress-a15 machine > >>>> with vexpress-v2p-ca15-tc1 devicetree file in qemu. Reverting it as well > >>>> as the subsequent patches affecting the same file (to avoid revert > >>>> conflicts) fixes the problem. > >>> > >>> Many thanks for the heads up! I was able to reproduce it here. On the > >>> first glance it looks like the UART is probed before the clocks now, > >>> because the traversal of the changed DT leads to a different probe > >>> order. I will look into how to fix this. > >> > >> Turned out to be a bit more complicated: > >> The arm,vexpress,config-bus driver walks up the device tree to find a > >> arm,vexpress,site property [1]. With this patch the first parent node > >> with that property it finds is now the root node, with the wrong site ID > >> (0xf instead of 0x0). So it queries the wrong clocks (those IDs are > >> actually reserved there), and QEMU reports back "0", consequently [2]. > >> Finding a clock frequency in the range of [0, 0] won't get very far. > >> > >> Possible solutions are: > >> 1) Just keep the mcc and its children at where it is in mainline right > >> now, so *partly* reverting this patch. This has the problem of still > >> producing a dtc warning, so kind of defeats the purpose of this patch. > >> > >> 2) Add a "arm,vexpress,site = <0>;" line to the "mcc" node itself. > >> Works, but looks somewhat dodgy, as the mcc node should really be a > >> child of the motherboard node, and we should not hack around this. > >> > >> 3) Dig deeper and fix the DT in a way that makes dtc happy. Might > >> involve (dummy?) ranges or reg properties. My gut feeling is that > >> arm,vexpress-sysreg,func should really have been "reg" in the first > >> place, but that's too late to change now, anyway. > >> > >> I will post 2) as a fix if 3) turns out to be not feasible. > > > > I would just do 1). > > > > To some extent, the warnings are for avoiding poor design on new > > bindings. We need a way to distinguish between existing boards and new > > ones. Maybe dts needs to learn some warning disable annotations or we > > need per target warning settings (DTC_FLAGS_foo.dtb ?). Or maybe this > > check is just too strict. > > So I was always wondering about this check, actually. A simple-bus > describes a bus which is mapped into the CPU address space (in contrast > to say an I2C bus, for instance). So children of this bus node typically > have a reg property. > > Now also those simple-bus nodes seem to be used to logically group > hardware in a DT (see this "motherboard" node here). *If* we go with > this, we should also allow other subnodes, for instance fixed-clocks: > after all there is probably an actual fixed crystal oscillator on the > motherboard, so it would also belong in there. Yes, that's probably right. We'd want things grouped if this was something applied as an overlay. > I see that (ab)using simple-bus for *just* grouping nodes is probably > not a good design, but I don't see why *every* child must be mapped into > the address space. > > Maybe dtc's simple-bus check should indeed be relaxed, to just require > *at least one* child with a reg or ranges property, but also allow other > nodes? It's made you think about the proper location, so it's accomplished its goal. Maybe this is one that's not without false positives. It would be good to distinguish between what's for sure a warning and what's maybe a warning as just blindly fixing the warning is not the answer. Rob
