* [PATCH v3 0/3] dtc bus and unit address checks @ 2017-02-28 22:43 Rob Herring [not found] ` <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-02-28 22:43 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA This is a series of checks designed to check problems commonly found in binding reviews. The first patch adds checks for PCI bridges and devices. The 2nd patch is new in this version and adds checks for simple-bus. The 3rd patch is a default check if bus type is not set to check for '0x' or leading 0s in unit addresses. Rob Rob Herring (3): checks: Add bus checks for PCI buses checks: Add bus checks for simple-bus buses checks: Warn on node name unit-addresses with '0x' or leading 0s checks.c | 230 +++++++++++++++++++++++++++++++++++++++++ dtc.h | 5 + tests/run_tests.sh | 2 + tests/unit-addr-leading-0s.dts | 12 +++ tests/unit-addr-leading-0x.dts | 12 +++ 5 files changed, 261 insertions(+) create mode 100644 tests/unit-addr-leading-0s.dts create mode 100644 tests/unit-addr-leading-0x.dts -- 2.10.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]
* [PATCH v3 1/3] checks: Add bus checks for PCI buses [not found] ` <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> @ 2017-02-28 22:43 ` Rob Herring [not found] ` <20170228224310.14162-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-02-28 22:43 ` [PATCH v3 2/3] checks: Add bus checks for simple-bus buses Rob Herring 2017-02-28 22:43 ` [PATCH v3 3/3] checks: Warn on node name unit-addresses with '0x' or leading 0s Rob Herring 2 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-02-28 22:43 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Add PCI bridge and device node checks. We identify PCI bridges with 'device_type = "pci"' as only PCI bridges should set that property. For bridges, check that node name is pci or pcie, ranges and bus-range are present, and #address-cells and #size-cells are correct. For devices, check the reg property fields are correct for the first element (the config address). Check that the unit address is formatted corectly based on the reg property. Device unit addresses are in the form DD or DD,F where DD is the device 0-0x1f and F is the function 0-7. Also, check that the bus number is within the expected range defined by bridge's bus-ranges. Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- v3: - Use bus_type ptr for matching bus types - Add a name string to bus_type - Add a bus-range check - Improve the PCI device reg value checking - Use streq/strneq - fix FAIL call changes from current master v2: - Remove bus_type functions. Combine test for bus_type and bridge check into single check. - Add a check that PCI bridge node name is pci or pcie. checks.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ dtc.h | 5 +++ 2 files changed, 141 insertions(+) diff --git a/checks.c b/checks.c index 0e8b978c360c..5ed91ac50a10 100644 --- a/checks.c +++ b/checks.c @@ -685,6 +685,138 @@ static void check_ranges_format(struct check *c, struct dt_info *dti, } WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells); +static const struct bus_type pci_bus = { + .name = "PCI", +}; + +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + cell_t *cells; + + prop = get_property(node, "device_type"); + if (!prop || strcmp(prop->val.val, "pci")) + return; + + node->bus = &pci_bus; + + if (!strneq(node->name, "pci", node->basenamelen) && + !strneq(node->name, "pcie", node->basenamelen)) + FAIL(c, dti, "Node %s node name is not \"pci\" or \"pcie\"", + node->fullpath); + + prop = get_property(node, "ranges"); + if (!prop) + FAIL(c, dti, "Node %s missing ranges for PCI bridge (or not a bridge)", + node->fullpath); + + if (node_addr_cells(node) != 3) + FAIL(c, dti, "Node %s incorrect #address-cells for PCI bridge", + node->fullpath); + if (node_size_cells(node) != 2) + FAIL(c, dti, "Node %s incorrect #size-cells for PCI bridge", + node->fullpath); + + prop = get_property(node, "bus-range"); + if (!prop) { + FAIL(c, dti, "Node %s missing bus-range for PCI bridge", + node->fullpath); + return; + } + if (prop->val.len != (sizeof(cell_t) * 2)) { + FAIL(c, dti, "Node %s bus-range must be 2 cells", + node->fullpath); + return; + } + cells = (cell_t *)prop->val.val; + if (fdt32_to_cpu(cells[0]) > fdt32_to_cpu(cells[1])) + FAIL(c, dti, "Node %s bus-range 1st cell must be less than or equal to 2nd cell", + node->fullpath); + if (fdt32_to_cpu(cells[1]) > 0xff) + FAIL(c, dti, "Node %s bus-range maximum bus number must be less than 256", + node->fullpath); +} +WARNING(pci_bridge, check_pci_bridge, NULL, + &device_type_is_string, &addr_size_cells); + +static void check_pci_device_bus_num(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + unsigned int bus_num, min_bus, max_bus; + cell_t *cells; + + if (!node->parent || (node->parent->bus != &pci_bus)) + return; + + prop = get_property(node, "reg"); + if (!prop) + return; + + cells = (cell_t *)prop->val.val; + bus_num = (fdt32_to_cpu(cells[0]) & 0x00ff0000) >> 16; + + prop = get_property(node->parent, "bus-range"); + if (!prop || prop->val.len != (sizeof(cell_t) * 2)) { + min_bus = max_bus = 0; + } else { + cells = (cell_t *)prop->val.val; + min_bus = fdt32_to_cpu(cells[0]); + max_bus = fdt32_to_cpu(cells[0]); + } + if ((bus_num < min_bus) || (bus_num > max_bus)) + FAIL(c, dti, "Node %s PCI bus number %d out of range, expected (%d - %d)", + node->fullpath, bus_num, min_bus, max_bus); +} +WARNING(pci_device_bus_num, check_pci_device_bus_num, NULL, ®_format); + +static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + const char *unitname = get_unitname(node); + char unit_addr[5]; + unsigned int dev, func, reg; + cell_t *cells; + + if (!node->parent || (node->parent->bus != &pci_bus)) + return; + + prop = get_property(node, "reg"); + if (!prop) { + FAIL(c, dti, "Node %s missing PCI reg property", node->fullpath); + return; + } + + cells = (cell_t *)prop->val.val; + if (cells[1] || cells[2]) + FAIL(c, dti, "Node %s PCI reg config space address cells 2 and 3 must be 0", + node->fullpath); + + reg = fdt32_to_cpu(cells[0]); + dev = (reg & 0xf800) >> 11; + func = (reg & 0x700) >> 8; + + if (reg & 0xff000000) + FAIL(c, dti, "Node %s PCI reg address is not configuration space", + node->fullpath); + if (reg & 0x000000ff) + FAIL(c, dti, "Node %s PCI reg config space address register number must be 0", + node->fullpath); + + if (func == 0) { + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); + if (streq(unitname, unit_addr)) + return; + } + + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); + if (streq(unitname, unit_addr)) + return; + + FAIL(c, dti, "Node %s PCI unit address format error, expected \"%s\"", + node->fullpath, unit_addr); +} +WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); + /* * Style checks */ @@ -757,6 +889,10 @@ static struct check *check_table[] = { &unit_address_vs_reg, + &pci_bridge, + &pci_device_reg, + &pci_device_bus_num, + &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, diff --git a/dtc.h b/dtc.h index 1ac2a1e3a4a5..af67eef339b0 100644 --- a/dtc.h +++ b/dtc.h @@ -136,6 +136,10 @@ struct label { struct label *next; }; +struct bus_type { + const char *name; +}; + struct property { bool deleted; char *name; @@ -162,6 +166,7 @@ struct node { int addr_cells, size_cells; struct label *labels; + const struct bus_type *bus; }; #define for_each_label_withdel(l0, l) \ -- 2.10.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 12+ messages in thread
[parent not found: <20170228224310.14162-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH v3 1/3] checks: Add bus checks for PCI buses [not found] ` <20170228224310.14162-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> @ 2017-03-03 2:09 ` David Gibson 0 siblings, 0 replies; 12+ messages in thread From: David Gibson @ 2017-03-03 2:09 UTC (permalink / raw) To: Rob Herring Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 7356 bytes --] On Tue, Feb 28, 2017 at 04:43:08PM -0600, Rob Herring wrote: > Add PCI bridge and device node checks. We identify PCI bridges with > 'device_type = "pci"' as only PCI bridges should set that property. For > bridges, check that node name is pci or pcie, ranges and bus-range are > present, and #address-cells and #size-cells are correct. > > For devices, check the reg property fields are correct for the first > element (the config address). Check that the unit address is formatted > corectly based on the reg property. Device unit addresses are in the > form DD or DD,F where DD is the device 0-0x1f and F is the function 0-7. > Also, check that the bus number is within the expected range defined by > bridge's bus-ranges. > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Only some tiny details remaining. With the exception of the details mentioned below: Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> > --- > v3: > - Use bus_type ptr for matching bus types > - Add a name string to bus_type > - Add a bus-range check > - Improve the PCI device reg value checking > - Use streq/strneq > - fix FAIL call changes from current master > > v2: > - Remove bus_type functions. Combine test for bus_type and bridge check > into single check. > - Add a check that PCI bridge node name is pci or pcie. > > checks.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > dtc.h | 5 +++ > 2 files changed, 141 insertions(+) > > diff --git a/checks.c b/checks.c > index 0e8b978c360c..5ed91ac50a10 100644 > --- a/checks.c > +++ b/checks.c > @@ -685,6 +685,138 @@ static void check_ranges_format(struct check *c, struct dt_info *dti, > } > WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells); > > +static const struct bus_type pci_bus = { > + .name = "PCI", > +}; > + > +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + cell_t *cells; > + > + prop = get_property(node, "device_type"); > + if (!prop || strcmp(prop->val.val, "pci")) Use streq() above, please. > + return; > + > + node->bus = &pci_bus; > + > + if (!strneq(node->name, "pci", node->basenamelen) && > + !strneq(node->name, "pcie", node->basenamelen)) > + FAIL(c, dti, "Node %s node name is not \"pci\" or \"pcie\"", > + node->fullpath); > + > + prop = get_property(node, "ranges"); > + if (!prop) > + FAIL(c, dti, "Node %s missing ranges for PCI bridge (or not a bridge)", > + node->fullpath); > + > + if (node_addr_cells(node) != 3) > + FAIL(c, dti, "Node %s incorrect #address-cells for PCI bridge", > + node->fullpath); > + if (node_size_cells(node) != 2) > + FAIL(c, dti, "Node %s incorrect #size-cells for PCI bridge", > + node->fullpath); > + > + prop = get_property(node, "bus-range"); > + if (!prop) { > + FAIL(c, dti, "Node %s missing bus-range for PCI bridge", > + node->fullpath); > + return; > + } > + if (prop->val.len != (sizeof(cell_t) * 2)) { > + FAIL(c, dti, "Node %s bus-range must be 2 cells", > + node->fullpath); > + return; > + } > + cells = (cell_t *)prop->val.val; > + if (fdt32_to_cpu(cells[0]) > fdt32_to_cpu(cells[1])) > + FAIL(c, dti, "Node %s bus-range 1st cell must be less than or equal to 2nd cell", > + node->fullpath); > + if (fdt32_to_cpu(cells[1]) > 0xff) > + FAIL(c, dti, "Node %s bus-range maximum bus number must be less than 256", > + node->fullpath); > +} > +WARNING(pci_bridge, check_pci_bridge, NULL, > + &device_type_is_string, &addr_size_cells); > + > +static void check_pci_device_bus_num(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + unsigned int bus_num, min_bus, max_bus; > + cell_t *cells; > + > + if (!node->parent || (node->parent->bus != &pci_bus)) > + return; > + > + prop = get_property(node, "reg"); > + if (!prop) > + return; > + > + cells = (cell_t *)prop->val.val; > + bus_num = (fdt32_to_cpu(cells[0]) & 0x00ff0000) >> 16; > + > + prop = get_property(node->parent, "bus-range"); > + if (!prop || prop->val.len != (sizeof(cell_t) * 2)) { > + min_bus = max_bus = 0; You can make this check dependent on the bridge check, then you wouldn't need to handle the case of a bad bus-range property here. > + } else { > + cells = (cell_t *)prop->val.val; > + min_bus = fdt32_to_cpu(cells[0]); > + max_bus = fdt32_to_cpu(cells[0]); > + } > + if ((bus_num < min_bus) || (bus_num > max_bus)) > + FAIL(c, dti, "Node %s PCI bus number %d out of range, expected (%d - %d)", > + node->fullpath, bus_num, min_bus, max_bus); > +} > +WARNING(pci_device_bus_num, check_pci_device_bus_num, NULL, ®_format); > + > +static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + const char *unitname = get_unitname(node); > + char unit_addr[5]; > + unsigned int dev, func, reg; > + cell_t *cells; > + > + if (!node->parent || (node->parent->bus != &pci_bus)) > + return; > + > + prop = get_property(node, "reg"); > + if (!prop) { > + FAIL(c, dti, "Node %s missing PCI reg property", node->fullpath); > + return; > + } > + > + cells = (cell_t *)prop->val.val; > + if (cells[1] || cells[2]) > + FAIL(c, dti, "Node %s PCI reg config space address cells 2 and 3 must be 0", > + node->fullpath); > + > + reg = fdt32_to_cpu(cells[0]); > + dev = (reg & 0xf800) >> 11; > + func = (reg & 0x700) >> 8; > + > + if (reg & 0xff000000) > + FAIL(c, dti, "Node %s PCI reg address is not configuration space", > + node->fullpath); > + if (reg & 0x000000ff) > + FAIL(c, dti, "Node %s PCI reg config space address register number must be 0", > + node->fullpath); > + > + if (func == 0) { > + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); > + if (streq(unitname, unit_addr)) > + return; > + } > + > + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); > + if (streq(unitname, unit_addr)) > + return; > + > + FAIL(c, dti, "Node %s PCI unit address format error, expected \"%s\"", > + node->fullpath, unit_addr); > +} > +WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > + > /* > * Style checks > */ > @@ -757,6 +889,10 @@ static struct check *check_table[] = { > > &unit_address_vs_reg, > > + &pci_bridge, > + &pci_device_reg, > + &pci_device_bus_num, > + > &avoid_default_addr_size, > &obsolete_chosen_interrupt_controller, > > diff --git a/dtc.h b/dtc.h > index 1ac2a1e3a4a5..af67eef339b0 100644 > --- a/dtc.h > +++ b/dtc.h > @@ -136,6 +136,10 @@ struct label { > struct label *next; > }; > > +struct bus_type { > + const char *name; > +}; > + > struct property { > bool deleted; > char *name; > @@ -162,6 +166,7 @@ struct node { > int addr_cells, size_cells; > > struct label *labels; > + const struct bus_type *bus; > }; > > #define for_each_label_withdel(l0, l) \ -- David Gibson | 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-02-28 22:43 ` [PATCH v3 1/3] checks: Add bus checks for PCI buses Rob Herring @ 2017-02-28 22:43 ` Rob Herring [not found] ` <20170228224310.14162-3-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-02-28 22:43 ` [PATCH v3 3/3] checks: Warn on node name unit-addresses with '0x' or leading 0s Rob Herring 2 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-02-28 22:43 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Add checks to identify simple-bus bus types and checks for child devices. Simple-bus type is generally identified by "simple-bus" compatible string. We also treat the root as a simple-bus, but only for child nodes with reg property. Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- v2: - new patch checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/checks.c b/checks.c index 5ed91ac50a10..c4865b4c8da0 100644 --- a/checks.c +++ b/checks.c @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no } WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); +static const struct bus_type simple_bus = { + .name = "simple-bus", +}; + +static bool node_is_compatible(struct node *node, const char *compat) +{ + struct property *prop; + const char *str; + + prop = get_property(node, "compatible"); + if (!prop) + return false; + + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { + if (streq(str, compat)) + return true; + } + return false; +} + +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) +{ + if (node_is_compatible(node, "simple-bus") || !node->parent) + node->bus = &simple_bus; +} +WARNING(simple_bus_bridge, check_simple_bus_bridge, NULL, &addr_size_cells); + +static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + const char *unitname = get_unitname(node); + char unit_addr[17]; + unsigned int size; + uint64_t reg = 0; + cell_t *cells = NULL; + + if (!node->parent || (node->parent->bus != &simple_bus)) + return; + + prop = get_property(node, "reg"); + if (prop) + cells = (cell_t *)prop->val.val; + else { + prop = get_property(node, "ranges"); + if (prop && prop->val.len) + /* skip of child address */ + cells = ((cell_t *)prop->val.val) + node_addr_cells(node); + } + + if (!cells) { + if (node->parent->parent && !(node->bus == &simple_bus)) + FAIL(c, dti, "Node %s missing or empty reg/ranges property", node->fullpath); + return; + } + + size = node_addr_cells(node->parent); + while (size--) + reg = (reg << 32) | fdt32_to_cpu(*(cells++)); + + snprintf(unit_addr, sizeof(unit_addr), "%lx", reg); + if (!streq(unitname, unit_addr)) + FAIL(c, dti, "Node %s simple-bus unit address format error, expected \"%s\"", + node->fullpath, unit_addr); +} +WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); + /* * Style checks */ @@ -893,6 +959,9 @@ static struct check *check_table[] = { &pci_device_reg, &pci_device_bus_num, + &simple_bus_bridge, + &simple_bus_reg, + &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, -- 2.10.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 12+ messages in thread
[parent not found: <20170228224310.14162-3-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <20170228224310.14162-3-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> @ 2017-03-03 2:12 ` David Gibson [not found] ` <20170303021206.GD4067-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2017-03-03 2:12 UTC (permalink / raw) To: Rob Herring Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 3721 bytes --] On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > Add checks to identify simple-bus bus types and checks for child > devices. Simple-bus type is generally identified by "simple-bus" > compatible string. We also treat the root as a simple-bus, but only for > child nodes with reg property. > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > --- > v2: > - new patch > > checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/checks.c b/checks.c > index 5ed91ac50a10..c4865b4c8da0 100644 > --- a/checks.c > +++ b/checks.c > @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > } > WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > > +static const struct bus_type simple_bus = { > + .name = "simple-bus", > +}; > + > +static bool node_is_compatible(struct node *node, const char *compat) > +{ > + struct property *prop; > + const char *str; > + > + prop = get_property(node, "compatible"); > + if (!prop) > + return false; > + > + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { This isn't safe if the compatible property is filled with garbage (not '\0' terminated) - the strlen() could access beyond the end of the property value. > + if (streq(str, compat)) > + return true; > + } > + return false; > +} > + > +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > +{ > + if (node_is_compatible(node, "simple-bus") || !node->parent) > + node->bus = &simple_bus; I don't think it's correct to assume the root bus is always a simple-bus. If it is, it really should be listed explicitly in the root node's compatible property. > +} > +WARNING(simple_bus_bridge, check_simple_bus_bridge, NULL, &addr_size_cells); > + > +static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + const char *unitname = get_unitname(node); > + char unit_addr[17]; > + unsigned int size; > + uint64_t reg = 0; > + cell_t *cells = NULL; > + > + if (!node->parent || (node->parent->bus != &simple_bus)) > + return; > + > + prop = get_property(node, "reg"); > + if (prop) > + cells = (cell_t *)prop->val.val; > + else { > + prop = get_property(node, "ranges"); > + if (prop && prop->val.len) > + /* skip of child address */ > + cells = ((cell_t *)prop->val.val) + node_addr_cells(node); > + } > + > + if (!cells) { > + if (node->parent->parent && !(node->bus == &simple_bus)) > + FAIL(c, dti, "Node %s missing or empty reg/ranges property", node->fullpath); > + return; > + } > + > + size = node_addr_cells(node->parent); > + while (size--) > + reg = (reg << 32) | fdt32_to_cpu(*(cells++)); > + > + snprintf(unit_addr, sizeof(unit_addr), "%lx", reg); > + if (!streq(unitname, unit_addr)) > + FAIL(c, dti, "Node %s simple-bus unit address format error, expected \"%s\"", > + node->fullpath, unit_addr); > +} > +WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); > + > /* > * Style checks > */ > @@ -893,6 +959,9 @@ static struct check *check_table[] = { > &pci_device_reg, > &pci_device_bus_num, > > + &simple_bus_bridge, > + &simple_bus_reg, > + > &avoid_default_addr_size, > &obsolete_chosen_interrupt_controller, > -- David Gibson | 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20170303021206.GD4067-K0bRW+63XPQe6aEkudXLsA@public.gmane.org>]
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <20170303021206.GD4067-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> @ 2017-03-06 10:48 ` Rob Herring [not found] ` <CAL_JsqJzWAiBUZeRMx0i5Y_NAFH6_jrASWcFp_U4MwVf2+=h0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-03-06 10:48 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On Thu, Mar 2, 2017 at 8:12 PM, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> Add checks to identify simple-bus bus types and checks for child >> devices. Simple-bus type is generally identified by "simple-bus" >> compatible string. We also treat the root as a simple-bus, but only for >> child nodes with reg property. >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> >> --- >> v2: >> - new patch >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/checks.c b/checks.c >> index 5ed91ac50a10..c4865b4c8da0 100644 >> --- a/checks.c >> +++ b/checks.c >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> } >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> +static const struct bus_type simple_bus = { >> + .name = "simple-bus", >> +}; >> + >> +static bool node_is_compatible(struct node *node, const char *compat) >> +{ >> + struct property *prop; >> + const char *str; >> + >> + prop = get_property(node, "compatible"); >> + if (!prop) >> + return false; >> + >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > > This isn't safe if the compatible property is filled with garbage (not > '\0' terminated) - the strlen() could access beyond the end of the > property value. Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > >> + if (streq(str, compat)) >> + return true; >> + } >> + return false; >> +} >> + >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> +{ >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> + node->bus = &simple_bus; > > I don't think it's correct to assume the root bus is always a > simple-bus. If it is, it really should be listed explicitly in the > root node's compatible property. It is in the sense that Linux treats the root the same and creates devices for top level children and then descends for nodes with "simple-bus". And since unit addresses are a single address space for a given level, we can't have any other bus type. IOW, any other defined bus type like I2C is going to be a child of its controller. Rob ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <CAL_JsqJzWAiBUZeRMx0i5Y_NAFH6_jrASWcFp_U4MwVf2+=h0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <CAL_JsqJzWAiBUZeRMx0i5Y_NAFH6_jrASWcFp_U4MwVf2+=h0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-03-07 3:41 ` David Gibson [not found] ` <20170307034110.GC19967-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2017-03-07 3:41 UTC (permalink / raw) To: Rob Herring Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 3138 bytes --] On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: > On Thu, Mar 2, 2017 at 8:12 PM, David Gibson > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > >> Add checks to identify simple-bus bus types and checks for child > >> devices. Simple-bus type is generally identified by "simple-bus" > >> compatible string. We also treat the root as a simple-bus, but only for > >> child nodes with reg property. > >> > >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > >> --- > >> v2: > >> - new patch > >> > >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 69 insertions(+) > >> > >> diff --git a/checks.c b/checks.c > >> index 5ed91ac50a10..c4865b4c8da0 100644 > >> --- a/checks.c > >> +++ b/checks.c > >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > >> } > >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > >> > >> +static const struct bus_type simple_bus = { > >> + .name = "simple-bus", > >> +}; > >> + > >> +static bool node_is_compatible(struct node *node, const char *compat) > >> +{ > >> + struct property *prop; > >> + const char *str; > >> + > >> + prop = get_property(node, "compatible"); > >> + if (!prop) > >> + return false; > >> + > >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > > > > This isn't safe if the compatible property is filled with garbage (not > > '\0' terminated) - the strlen() could access beyond the end of the > > property value. > > Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. Sure. Or use strnlen. > > > > >> + if (streq(str, compat)) > >> + return true; > >> + } > >> + return false; > >> +} > >> + > >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > >> +{ > >> + if (node_is_compatible(node, "simple-bus") || !node->parent) > >> + node->bus = &simple_bus; > > > > I don't think it's correct to assume the root bus is always a > > simple-bus. If it is, it really should be listed explicitly in the > > root node's compatible property. > > It is in the sense that Linux treats the root the same and creates > devices for top level children and then descends for nodes with > "simple-bus". Hmm.. where in Linux is that? I think that's a bug, technically speaking, traversing the root node's children without regard to the type of the root node. > And since unit addresses are a single address space for > a given level, we can't have any other bus type. IOW, any other > defined bus type like I2C is going to be a child of its controller. > > Rob -- David Gibson | 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20170307034110.GC19967-K0bRW+63XPQe6aEkudXLsA@public.gmane.org>]
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <20170307034110.GC19967-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> @ 2017-03-07 13:39 ` Rob Herring 2017-03-07 13:51 ` Rob Herring 1 sibling, 0 replies; 12+ messages in thread From: Rob Herring @ 2017-03-07 13:39 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On Mon, Mar 6, 2017 at 9:41 PM, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> >> Add checks to identify simple-bus bus types and checks for child >> >> devices. Simple-bus type is generally identified by "simple-bus" >> >> compatible string. We also treat the root as a simple-bus, but only for >> >> child nodes with reg property. >> >> >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> >> >> --- >> >> v2: >> >> - new patch >> >> >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 69 insertions(+) >> >> >> >> diff --git a/checks.c b/checks.c >> >> index 5ed91ac50a10..c4865b4c8da0 100644 >> >> --- a/checks.c >> >> +++ b/checks.c >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> >> } >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> >> >> +static const struct bus_type simple_bus = { >> >> + .name = "simple-bus", >> >> +}; >> >> + >> >> +static bool node_is_compatible(struct node *node, const char *compat) >> >> +{ >> >> + struct property *prop; >> >> + const char *str; >> >> + >> >> + prop = get_property(node, "compatible"); >> >> + if (!prop) >> >> + return false; >> >> + >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { >> > >> > This isn't safe if the compatible property is filled with garbage (not >> > '\0' terminated) - the strlen() could access beyond the end of the >> > property value. >> >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > Sure. Or use strnlen. >> >> > >> >> + if (streq(str, compat)) >> >> + return true; >> >> + } >> >> + return false; >> >> +} >> >> + >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> >> +{ >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> >> + node->bus = &simple_bus; >> > >> > I don't think it's correct to assume the root bus is always a >> > simple-bus. If it is, it really should be listed explicitly in the >> > root node's compatible property. >> >> It is in the sense that Linux treats the root the same and creates >> devices for top level children and then descends for nodes with >> "simple-bus". > > Hmm.. where in Linux is that? I think that's a bug, technically > speaking, traversing the root node's children without regard to the > type of the root node. > >> And since unit addresses are a single address space for >> a given level, we can't have any other bus type. IOW, any other >> defined bus type like I2C is going to be a child of its controller. >> >> Rob > > -- > David Gibson | 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <20170307034110.GC19967-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> 2017-03-07 13:39 ` Rob Herring @ 2017-03-07 13:51 ` Rob Herring [not found] ` <CAL_Jsq+U4SqL2EDR6hRrE_JcONkfkZqacBQGz4Kkq=CxNbaYAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-03-07 13:51 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On Mon, Mar 6, 2017 at 9:41 PM, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> >> Add checks to identify simple-bus bus types and checks for child >> >> devices. Simple-bus type is generally identified by "simple-bus" >> >> compatible string. We also treat the root as a simple-bus, but only for >> >> child nodes with reg property. >> >> >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> >> >> --- >> >> v2: >> >> - new patch >> >> >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 69 insertions(+) >> >> >> >> diff --git a/checks.c b/checks.c >> >> index 5ed91ac50a10..c4865b4c8da0 100644 >> >> --- a/checks.c >> >> +++ b/checks.c >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> >> } >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> >> >> +static const struct bus_type simple_bus = { >> >> + .name = "simple-bus", >> >> +}; >> >> + >> >> +static bool node_is_compatible(struct node *node, const char *compat) >> >> +{ >> >> + struct property *prop; >> >> + const char *str; >> >> + >> >> + prop = get_property(node, "compatible"); >> >> + if (!prop) >> >> + return false; >> >> + >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { >> > >> > This isn't safe if the compatible property is filled with garbage (not >> > '\0' terminated) - the strlen() could access beyond the end of the >> > property value. >> >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > Sure. Or use strnlen. Duh... >> >> + if (streq(str, compat)) >> >> + return true; >> >> + } >> >> + return false; >> >> +} >> >> + >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> >> +{ >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> >> + node->bus = &simple_bus; >> > >> > I don't think it's correct to assume the root bus is always a >> > simple-bus. If it is, it really should be listed explicitly in the >> > root node's compatible property. >> >> It is in the sense that Linux treats the root the same and creates >> devices for top level children and then descends for nodes with >> "simple-bus". > > Hmm.. where in Linux is that? I think that's a bug, technically > speaking, traversing the root node's children without regard to the > type of the root node. drivers/of/platform.c:of_platform_populate() which is called on the root node with "simple-bus" in the match table. Plus I know we have some DT's like Tegra that didn't put all their devices under a bus (but should have). Maybe I should warn on that (i.e. warn on having unit-addresses without a bus type set on root). Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <CAL_Jsq+U4SqL2EDR6hRrE_JcONkfkZqacBQGz4Kkq=CxNbaYAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses [not found] ` <CAL_Jsq+U4SqL2EDR6hRrE_JcONkfkZqacBQGz4Kkq=CxNbaYAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-03-08 1:55 ` David Gibson 0 siblings, 0 replies; 12+ messages in thread From: David Gibson @ 2017-03-08 1:55 UTC (permalink / raw) To: Rob Herring Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 4121 bytes --] On Tue, Mar 07, 2017 at 07:51:15AM -0600, Rob Herring wrote: > On Mon, Mar 6, 2017 at 9:41 PM, David Gibson > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: > >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson > >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote: > >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > >> >> Add checks to identify simple-bus bus types and checks for child > >> >> devices. Simple-bus type is generally identified by "simple-bus" > >> >> compatible string. We also treat the root as a simple-bus, but only for > >> >> child nodes with reg property. > >> >> > >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > >> >> --- > >> >> v2: > >> >> - new patch > >> >> > >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> >> 1 file changed, 69 insertions(+) > >> >> > >> >> diff --git a/checks.c b/checks.c > >> >> index 5ed91ac50a10..c4865b4c8da0 100644 > >> >> --- a/checks.c > >> >> +++ b/checks.c > >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > >> >> } > >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > >> >> > >> >> +static const struct bus_type simple_bus = { > >> >> + .name = "simple-bus", > >> >> +}; > >> >> + > >> >> +static bool node_is_compatible(struct node *node, const char *compat) > >> >> +{ > >> >> + struct property *prop; > >> >> + const char *str; > >> >> + > >> >> + prop = get_property(node, "compatible"); > >> >> + if (!prop) > >> >> + return false; > >> >> + > >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > >> > > >> > This isn't safe if the compatible property is filled with garbage (not > >> > '\0' terminated) - the strlen() could access beyond the end of the > >> > property value. > >> > >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > > > Sure. Or use strnlen. > > Duh... > > >> >> + if (streq(str, compat)) > >> >> + return true; > >> >> + } > >> >> + return false; > >> >> +} > >> >> + > >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > >> >> +{ > >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) > >> >> + node->bus = &simple_bus; > >> > > >> > I don't think it's correct to assume the root bus is always a > >> > simple-bus. If it is, it really should be listed explicitly in the > >> > root node's compatible property. > >> > >> It is in the sense that Linux treats the root the same and creates > >> devices for top level children and then descends for nodes with > >> "simple-bus". > > > > Hmm.. where in Linux is that? I think that's a bug, technically > > speaking, traversing the root node's children without regard to the > > type of the root node. > > drivers/of/platform.c:of_platform_populate() which is called on the > root node with "simple-bus" in the match table. It's called on the root node *by platform code*. And the platform is selected on properties of the root node, so it already knows something about what format the root node should have on that particular platform. dtc does not know that. > Plus I know we have some DT's like Tegra that didn't put all their > devices under a bus (but should have). Maybe I should warn on that > (i.e. warn on having unit-addresses without a bus type set on root). There's nothing inherently wrong with having devices on the root bus. Really platforms that want this should be putting something like: compatible = "vendor,myboard", "simple-bus"; in their root node. -- David Gibson | 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/3] checks: Warn on node name unit-addresses with '0x' or leading 0s [not found] ` <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-02-28 22:43 ` [PATCH v3 1/3] checks: Add bus checks for PCI buses Rob Herring 2017-02-28 22:43 ` [PATCH v3 2/3] checks: Add bus checks for simple-bus buses Rob Herring @ 2017-02-28 22:43 ` Rob Herring [not found] ` <20170228224310.14162-4-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2017-02-28 22:43 UTC (permalink / raw) To: David Gibson Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Node name unit-addresses should generally never begin with 0x or leading 0s. Add warnings to check for these cases, but only for nodes without a known bus type as there should be better bus specific checks of the unit address in those cases. Any unit addresses that don't follow the general rule will need to add a new bus type. There aren't any known ones ATM. Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> --- v3: - Only run check if bus type is not set (moving to the end of the series as a result). The bus bridge checks also must be a dependency. v2: - Split into separate check from unit_address_vs_reg checks.c | 25 +++++++++++++++++++++++++ tests/run_tests.sh | 2 ++ tests/unit-addr-leading-0s.dts | 12 ++++++++++++ tests/unit-addr-leading-0x.dts | 12 ++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 tests/unit-addr-leading-0s.dts create mode 100644 tests/unit-addr-leading-0x.dts diff --git a/checks.c b/checks.c index c4865b4c8da0..537c27dc4082 100644 --- a/checks.c +++ b/checks.c @@ -883,6 +883,30 @@ static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct no } WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); +static void check_unit_address_format(struct check *c, struct dt_info *dti, + struct node *node) +{ + const char *unitname = get_unitname(node); + + if (node->parent && node->parent->bus) + return; + + if (!unitname[0]) + return; + + if (!strncmp(unitname, "0x", 2)) { + FAIL(c, dti, "Node %s unit name should not have leading \"0x\"", + node->fullpath); + /* skip over 0x for next test */ + unitname += 2; + } + if (unitname[0] == '0' && isxdigit(unitname[1])) + FAIL(c, dti, "Node %s unit name should not have leading 0s", + node->fullpath); +} +WARNING(unit_address_format, check_unit_address_format, NULL, + &node_name_format, &pci_bridge, &simple_bus_bridge); + /* * Style checks */ @@ -954,6 +978,7 @@ static struct check *check_table[] = { &addr_size_cells, ®_format, &ranges_format, &unit_address_vs_reg, + &unit_address_format, &pci_bridge, &pci_device_reg, diff --git a/tests/run_tests.sh b/tests/run_tests.sh index ed489dbdd269..0f5c3db79b80 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -540,6 +540,8 @@ dtc_tests () { check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller check_tests reg-without-unit-addr.dts unit_address_vs_reg check_tests unit-addr-without-reg.dts unit_address_vs_reg + check_tests unit-addr-leading-0x.dts unit_address_format + check_tests unit-addr-leading-0s.dts unit_address_format run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts new file mode 100644 index 000000000000..cc017e9431a2 --- /dev/null +++ b/tests/unit-addr-leading-0s.dts @@ -0,0 +1,12 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + bus { + node@001 { + reg = <1 0>; + }; + }; +}; diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts new file mode 100644 index 000000000000..74f19678c98c --- /dev/null +++ b/tests/unit-addr-leading-0x.dts @@ -0,0 +1,12 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + bus { + node@0x1 { + reg = <1 0>; + }; + }; +}; -- 2.10.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 12+ messages in thread
[parent not found: <20170228224310.14162-4-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH v3 3/3] checks: Warn on node name unit-addresses with '0x' or leading 0s [not found] ` <20170228224310.14162-4-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> @ 2017-03-03 2:12 ` David Gibson 0 siblings, 0 replies; 12+ messages in thread From: David Gibson @ 2017-03-03 2:12 UTC (permalink / raw) To: Rob Herring Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 4352 bytes --] On Tue, Feb 28, 2017 at 04:43:10PM -0600, Rob Herring wrote: > Node name unit-addresses should generally never begin with 0x or leading > 0s. Add warnings to check for these cases, but only for nodes without a > known bus type as there should be better bus specific checks of the > unit address in those cases. Any unit addresses that don't follow the > general rule will need to add a new bus type. There aren't any known > ones ATM. > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> In the context of the other two patches, Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> > --- > v3: > - Only run check if bus type is not set (moving to the end of the series > as a result). The bus bridge checks also must be a dependency. > > v2: > - Split into separate check from unit_address_vs_reg > > checks.c | 25 +++++++++++++++++++++++++ > tests/run_tests.sh | 2 ++ > tests/unit-addr-leading-0s.dts | 12 ++++++++++++ > tests/unit-addr-leading-0x.dts | 12 ++++++++++++ > 4 files changed, 51 insertions(+) > create mode 100644 tests/unit-addr-leading-0s.dts > create mode 100644 tests/unit-addr-leading-0x.dts > > diff --git a/checks.c b/checks.c > index c4865b4c8da0..537c27dc4082 100644 > --- a/checks.c > +++ b/checks.c > @@ -883,6 +883,30 @@ static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct no > } > WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); > > +static void check_unit_address_format(struct check *c, struct dt_info *dti, > + struct node *node) > +{ > + const char *unitname = get_unitname(node); > + > + if (node->parent && node->parent->bus) > + return; > + > + if (!unitname[0]) > + return; > + > + if (!strncmp(unitname, "0x", 2)) { > + FAIL(c, dti, "Node %s unit name should not have leading \"0x\"", > + node->fullpath); > + /* skip over 0x for next test */ > + unitname += 2; > + } > + if (unitname[0] == '0' && isxdigit(unitname[1])) > + FAIL(c, dti, "Node %s unit name should not have leading 0s", > + node->fullpath); > +} > +WARNING(unit_address_format, check_unit_address_format, NULL, > + &node_name_format, &pci_bridge, &simple_bus_bridge); > + > /* > * Style checks > */ > @@ -954,6 +978,7 @@ static struct check *check_table[] = { > &addr_size_cells, ®_format, &ranges_format, > > &unit_address_vs_reg, > + &unit_address_format, > > &pci_bridge, > &pci_device_reg, > diff --git a/tests/run_tests.sh b/tests/run_tests.sh > index ed489dbdd269..0f5c3db79b80 100755 > --- a/tests/run_tests.sh > +++ b/tests/run_tests.sh > @@ -540,6 +540,8 @@ dtc_tests () { > check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller > check_tests reg-without-unit-addr.dts unit_address_vs_reg > check_tests unit-addr-without-reg.dts unit_address_vs_reg > + check_tests unit-addr-leading-0x.dts unit_address_format > + check_tests unit-addr-leading-0s.dts unit_address_format > run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb > run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb > run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb > diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts > new file mode 100644 > index 000000000000..cc017e9431a2 > --- /dev/null > +++ b/tests/unit-addr-leading-0s.dts > @@ -0,0 +1,12 @@ > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + bus { > + node@001 { > + reg = <1 0>; > + }; > + }; > +}; > diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts > new file mode 100644 > index 000000000000..74f19678c98c > --- /dev/null > +++ b/tests/unit-addr-leading-0x.dts > @@ -0,0 +1,12 @@ > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + bus { > + node@0x1 { > + reg = <1 0>; > + }; > + }; > +}; -- David Gibson | 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-08 1:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-28 22:43 [PATCH v3 0/3] dtc bus and unit address checks Rob Herring [not found] ` <20170228224310.14162-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-02-28 22:43 ` [PATCH v3 1/3] checks: Add bus checks for PCI buses Rob Herring [not found] ` <20170228224310.14162-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-03-03 2:09 ` David Gibson 2017-02-28 22:43 ` [PATCH v3 2/3] checks: Add bus checks for simple-bus buses Rob Herring [not found] ` <20170228224310.14162-3-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-03-03 2:12 ` David Gibson [not found] ` <20170303021206.GD4067-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> 2017-03-06 10:48 ` Rob Herring [not found] ` <CAL_JsqJzWAiBUZeRMx0i5Y_NAFH6_jrASWcFp_U4MwVf2+=h0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-07 3:41 ` David Gibson [not found] ` <20170307034110.GC19967-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> 2017-03-07 13:39 ` Rob Herring 2017-03-07 13:51 ` Rob Herring [not found] ` <CAL_Jsq+U4SqL2EDR6hRrE_JcONkfkZqacBQGz4Kkq=CxNbaYAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-08 1:55 ` David Gibson 2017-02-28 22:43 ` [PATCH v3 3/3] checks: Warn on node name unit-addresses with '0x' or leading 0s Rob Herring [not found] ` <20170228224310.14162-4-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2017-03-03 2:12 ` David Gibson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.