On Mon, Nov 04, 2024 at 02:32:54PM +0100, Philipp Zabel wrote: > On Mo, 2024-10-28 at 13:42 +1100, David Gibson wrote: > > On Fri, Oct 25, 2024 at 06:13:07PM +0200, Philipp Zabel wrote: > > > Do not fail the unnecessary #address-cells/#size-cells check if any > > > children of the node have a "ranges" property. > > > > I think this is correct, but I had to think abuot it for a while, > > because it's subtler than it looks. > > > > If there is no 'ranges' in the node itself, then the child devices' > > address space is not mapped into the parent bus. Of course, you can > > still establish a local address space for them that (e.g.) could be > > accessed indirectly via registers in this bridge device. > > > > Having a child device which acts as a bridge from this local address > > to another subordinate address space, but no children with any > > registers directly on the local bus seems odd... but it is logically > > possible. > > > > Given the subtlety, it would be pretty nice to add an explanatory > > comment about what this is check for and what some of the edge cases > > are. > > Thank you. Can I steal your wording and add the following comment > inside the for_each_child() loop: > > /* > * Even if the child devices' address space is not mapped into > * the parent bus (no 'ranges' property on node), children can > * still have registers on a local bus, or map local addresses > * to another subordinate address space. The properties on the > * child nodes then make #address-cells/#size-cells necessary: > */ Sure! > The specific reason for this patch is a PCI device tree overlay [1] > where the '#address-cells'/'#size-cells' properties are on the > __overlay__ node and a child simple-bus node contains the 'ranges' > property mapping between BAR space and the local bus. > > [1] > https://lore.kernel.org/all/20241014124636.24221-2-herve.codina@xxxxxxxxxxx/ Hrm... two things strike me as suspicious about that example. 1) The __overlay__ node is the PCI bus root... which should already have #address-cells and #size-cells in the base tree; it's PCI so those values are known and standard. 2) The PCI<->simple-bus bridge must surely have at least the normal configuration space registers, and so should have a 'reg' property as well as 'ranges'. > > > > Suggested-by: Rob Herring <robh@xxxxxxxxxx> > > > Link: https://lore.kernel.org/all/CAL_JsqKebRL454poAYZ9i=sCsHqGzmocLy0psQcng-79UWJB-A@xxxxxxxxxxxxxx/ > > > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > > --- > > > checks.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/checks.c b/checks.c > > > index 6e06aeab5503..76fdee2ed030 100644 > > > --- a/checks.c > > > +++ b/checks.c > > > @@ -1217,9 +1217,7 @@ WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL, > > > static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *dti, > > > struct node *node) > > > { > > > - struct property *prop; > > > struct node *child; > > > - bool has_reg = false; > > > > > > if (!node->parent || node->addr_cells < 0 || node->size_cells < 0) > > > return; > > > @@ -1228,13 +1226,11 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d > > > return; > > > > > > for_each_child(node, child) { > > > - prop = get_property(child, "reg"); > > > - if (prop) > > > - has_reg = true; > > > + if (get_property(child, "reg") || get_property(child, "ranges")) > > > + return; > > > } > > > > > > - if (!has_reg) > > > - FAIL(c, dti, node, "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" property"); > > > + FAIL(c, dti, node, "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" property"); > > > > ..also this message needs updating to reference child "ranges" as well. > > I'll change it to: > > "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" or \"ranges\" property" > > > regards > Philipp > -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
