| Message ID | 20170210164717.1234-5-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | dtc unit-address and character set checks | expand |
On Fri, Feb 10, 2017 at 10:47:17AM -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 ranges is present and #address-cells and > > For devices, the primary check is the reg property and the unit address. > 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. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > dtc.h | 7 ++++++ > 2 files changed, 90 insertions(+) > > diff --git a/checks.c b/checks.c > index 16d17d20caec..9ebb148f947a 100644 > --- a/checks.c > +++ b/checks.c > @@ -702,6 +702,86 @@ 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 = { > + .type = PCI_BUS_TYPE, Since you can use the struct pointer itself as a handle on the bus type, I don't think there's any value to having the enum-style type value. What _would_ be useful is a human readable bus type name. > +}; > + > +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + > + prop = get_property(node, "device_type"); > + if (!prop || strcmp(prop->val.val, "pci")) > + return; > + > + node->bus = &pci_bus; > + > + if (strncmp(node->name, "pci", node->basenamelen) && > + strncmp(node->name, "pcie", node->basenamelen)) > + FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"", > + node->fullpath); Please use the strneq() macro - I frequently get confused about whether strcmp()/strncmp() comparisons need an ! or not for equality testing. streq() / strneq() help me remember. > + > + prop = get_property(node, "ranges"); > + if (!prop) > + FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)", > + node->fullpath); > + > + if (node_addr_cells(node) != 3) > + FAIL(c, "Node %s incorrect #address-cells for PCI bridge", > + node->fullpath); > + if (node_size_cells(node) != 2) > + FAIL(c, "Node %s incorrect #size-cells for PCI bridge", > + node->fullpath); > +} > +WARNING(pci_bridge, check_pci_bridge, NULL, > + &device_type_is_string, &addr_size_cells); > + > +static void check_pci_device(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; > + > + if (!node->parent || !node->parent->bus || > + (node->parent->bus->type != PCI_BUS_TYPE)) You can just use node->parent->bus != &pci_bus here. > + return; > + > + prop = get_property(node, "reg"); > + if (!prop) > + return; > + > + reg = fdt32_to_cpu(*((cell_t *)prop->val.val)); > + > + dev = (reg & 0xf800) >> 11; > + func = (reg & 0x700) >> 8; > + > + if (reg & 0xff000000) > + FAIL(c, "Node %s PCI reg address is not configuration space", > + node->fullpath); > + > + if (dev > 0x1f) > + FAIL(c, "Node %s PCI device number out of range", > + node->fullpath); > + if (func > 7) > + FAIL(c, "Node %s PCI function number out of range", > + node->fullpath); > + > + if (func == 0) { > + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); > + if (!strcmp(unitname, unit_addr)) > + return; > + } > + > + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); > + if (!strcmp(unitname, unit_addr)) > + return; So as mentioned in my comments to 3/4, the test above, I would put back into unit_address_vs_reg, using a callback in the bus_type which formats a reg into the correct unit address. > + > + FAIL(c, "Node %s PCI unit address format error, expected \"%s\"", > + node->fullpath, unit_addr); > +} > +WARNING(pci_device, check_pci_device, NULL, ®_format); > + > /* > * Style checks > */ > @@ -775,6 +855,9 @@ static struct check *check_table[] = { > &unit_address_vs_reg, > &unit_address_format, > > + &pci_bridge, > + &pci_device, > + > &avoid_default_addr_size, > &obsolete_chosen_interrupt_controller, > > diff --git a/dtc.h b/dtc.h > index c6f125c68ba8..c538ef4afafb 100644 > --- a/dtc.h > +++ b/dtc.h > @@ -136,6 +136,12 @@ struct label { > struct label *next; > }; > > +#define PCI_BUS_TYPE 1 > + > +struct bus_type { > + int type; > +}; > + > struct property { > bool deleted; > char *name; > @@ -162,6 +168,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
On Sun, Feb 12, 2017 at 11:03 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Feb 10, 2017 at 10:47:17AM -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 ranges is present and #address-cells and >> >> For devices, the primary check is the reg property and the unit address. >> 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. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> dtc.h | 7 ++++++ >> 2 files changed, 90 insertions(+) >> >> diff --git a/checks.c b/checks.c >> index 16d17d20caec..9ebb148f947a 100644 >> --- a/checks.c >> +++ b/checks.c >> @@ -702,6 +702,86 @@ 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 = { >> + .type = PCI_BUS_TYPE, > > Since you can use the struct pointer itself as a handle on the bus > type, I don't think there's any value to having the enum-style type > value. What _would_ be useful is a human readable bus type name. Okay. >> +}; >> + >> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) >> +{ >> + struct property *prop; >> + >> + prop = get_property(node, "device_type"); >> + if (!prop || strcmp(prop->val.val, "pci")) >> + return; >> + >> + node->bus = &pci_bus; >> + >> + if (strncmp(node->name, "pci", node->basenamelen) && >> + strncmp(node->name, "pcie", node->basenamelen)) >> + FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"", >> + node->fullpath); > > Please use the strneq() macro - I frequently get confused about > whether strcmp()/strncmp() comparisons need an ! or not for equality > testing. streq() / strneq() help me remember. > >> + >> + prop = get_property(node, "ranges"); >> + if (!prop) >> + FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)", >> + node->fullpath); >> + >> + if (node_addr_cells(node) != 3) >> + FAIL(c, "Node %s incorrect #address-cells for PCI bridge", >> + node->fullpath); >> + if (node_size_cells(node) != 2) >> + FAIL(c, "Node %s incorrect #size-cells for PCI bridge", >> + node->fullpath); >> +} >> +WARNING(pci_bridge, check_pci_bridge, NULL, >> + &device_type_is_string, &addr_size_cells); >> + >> +static void check_pci_device(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; >> + >> + if (!node->parent || !node->parent->bus || >> + (node->parent->bus->type != PCI_BUS_TYPE)) > > You can just use node->parent->bus != &pci_bus here. > >> + return; >> + >> + prop = get_property(node, "reg"); >> + if (!prop) >> + return; >> + >> + reg = fdt32_to_cpu(*((cell_t *)prop->val.val)); >> + >> + dev = (reg & 0xf800) >> 11; >> + func = (reg & 0x700) >> 8; >> + >> + if (reg & 0xff000000) >> + FAIL(c, "Node %s PCI reg address is not configuration space", >> + node->fullpath); >> + >> + if (dev > 0x1f) >> + FAIL(c, "Node %s PCI device number out of range", >> + node->fullpath); >> + if (func > 7) >> + FAIL(c, "Node %s PCI function number out of range", >> + node->fullpath); BTW, I just noticed these 2 checks I can drop. They can never be true since I'm masking the values. >> + >> + if (func == 0) { >> + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); >> + if (!strcmp(unitname, unit_addr)) >> + return; >> + } >> + >> + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); >> + if (!strcmp(unitname, unit_addr)) >> + return; > > So as mentioned in my comments to 3/4, the test above, I would put > back into unit_address_vs_reg, using a callback in the bus_type which > formats a reg into the correct unit address. Humm, that doesn't really work. The unit address can be in 2 different forms when func# is 0. We can have either <dev> or <dev>,0. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 02:49:45PM -0600, Rob Herring wrote: > On Sun, Feb 12, 2017 at 11:03 PM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > On Fri, Feb 10, 2017 at 10:47:17AM -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 ranges is present and #address-cells and > >> > >> For devices, the primary check is the reg property and the unit address. > >> 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. > >> > >> Signed-off-by: Rob Herring <robh@kernel.org> > >> --- > >> 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> dtc.h | 7 ++++++ > >> 2 files changed, 90 insertions(+) > >> > >> diff --git a/checks.c b/checks.c > >> index 16d17d20caec..9ebb148f947a 100644 > >> --- a/checks.c > >> +++ b/checks.c > >> @@ -702,6 +702,86 @@ 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 = { > >> + .type = PCI_BUS_TYPE, > > > > Since you can use the struct pointer itself as a handle on the bus > > type, I don't think there's any value to having the enum-style type > > value. What _would_ be useful is a human readable bus type name. > > Okay. > > >> +}; > >> + > >> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) > >> +{ > >> + struct property *prop; > >> + > >> + prop = get_property(node, "device_type"); > >> + if (!prop || strcmp(prop->val.val, "pci")) > >> + return; > >> + > >> + node->bus = &pci_bus; > >> + > >> + if (strncmp(node->name, "pci", node->basenamelen) && > >> + strncmp(node->name, "pcie", node->basenamelen)) > >> + FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"", > >> + node->fullpath); > > > > Please use the strneq() macro - I frequently get confused about > > whether strcmp()/strncmp() comparisons need an ! or not for equality > > testing. streq() / strneq() help me remember. > > > >> + > >> + prop = get_property(node, "ranges"); > >> + if (!prop) > >> + FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)", > >> + node->fullpath); > >> + > >> + if (node_addr_cells(node) != 3) > >> + FAIL(c, "Node %s incorrect #address-cells for PCI bridge", > >> + node->fullpath); > >> + if (node_size_cells(node) != 2) > >> + FAIL(c, "Node %s incorrect #size-cells for PCI bridge", > >> + node->fullpath); > >> +} > >> +WARNING(pci_bridge, check_pci_bridge, NULL, > >> + &device_type_is_string, &addr_size_cells); > >> + > >> +static void check_pci_device(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; > >> + > >> + if (!node->parent || !node->parent->bus || > >> + (node->parent->bus->type != PCI_BUS_TYPE)) > > > > You can just use node->parent->bus != &pci_bus here. > > > >> + return; > >> + > >> + prop = get_property(node, "reg"); > >> + if (!prop) > >> + return; > >> + > >> + reg = fdt32_to_cpu(*((cell_t *)prop->val.val)); > >> + > >> + dev = (reg & 0xf800) >> 11; > >> + func = (reg & 0x700) >> 8; > >> + > >> + if (reg & 0xff000000) > >> + FAIL(c, "Node %s PCI reg address is not configuration space", > >> + node->fullpath); > >> + > >> + if (dev > 0x1f) > >> + FAIL(c, "Node %s PCI device number out of range", > >> + node->fullpath); > >> + if (func > 7) > >> + FAIL(c, "Node %s PCI function number out of range", > >> + node->fullpath); > > BTW, I just noticed these 2 checks I can drop. They can never be true > since I'm masking the values. Ah, good point. It looks like ther should be more to check though - dev and func are 8 bits, and you don't allow anything in the top 8 bits, but there's no checking of the remaining 16 bits. Or the other 2 address cells for that matter. > >> + > >> + if (func == 0) { > >> + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); > >> + if (!strcmp(unitname, unit_addr)) > >> + return; > >> + } > >> + > >> + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); > >> + if (!strcmp(unitname, unit_addr)) > >> + return; > > > > So as mentioned in my comments to 3/4, the test above, I would put > > back into unit_address_vs_reg, using a callback in the bus_type which > > formats a reg into the correct unit address. > > Humm, that doesn't really work. The unit address can be in 2 different > forms when func# is 0. We can have either <dev> or <dev>,0. Ah, good point. Alright leave it as is for now. When we get more bus types we can see if it makes sense to generalize something. -- 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
diff --git a/checks.c b/checks.c index 16d17d20caec..9ebb148f947a 100644 --- a/checks.c +++ b/checks.c @@ -702,6 +702,86 @@ 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 = { + .type = PCI_BUS_TYPE, +}; + +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + + prop = get_property(node, "device_type"); + if (!prop || strcmp(prop->val.val, "pci")) + return; + + node->bus = &pci_bus; + + if (strncmp(node->name, "pci", node->basenamelen) && + strncmp(node->name, "pcie", node->basenamelen)) + FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"", + node->fullpath); + + prop = get_property(node, "ranges"); + if (!prop) + FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)", + node->fullpath); + + if (node_addr_cells(node) != 3) + FAIL(c, "Node %s incorrect #address-cells for PCI bridge", + node->fullpath); + if (node_size_cells(node) != 2) + FAIL(c, "Node %s incorrect #size-cells for PCI bridge", + node->fullpath); +} +WARNING(pci_bridge, check_pci_bridge, NULL, + &device_type_is_string, &addr_size_cells); + +static void check_pci_device(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; + + if (!node->parent || !node->parent->bus || + (node->parent->bus->type != PCI_BUS_TYPE)) + return; + + prop = get_property(node, "reg"); + if (!prop) + return; + + reg = fdt32_to_cpu(*((cell_t *)prop->val.val)); + + dev = (reg & 0xf800) >> 11; + func = (reg & 0x700) >> 8; + + if (reg & 0xff000000) + FAIL(c, "Node %s PCI reg address is not configuration space", + node->fullpath); + + if (dev > 0x1f) + FAIL(c, "Node %s PCI device number out of range", + node->fullpath); + if (func > 7) + FAIL(c, "Node %s PCI function number out of range", + node->fullpath); + + if (func == 0) { + snprintf(unit_addr, sizeof(unit_addr), "%x", dev); + if (!strcmp(unitname, unit_addr)) + return; + } + + snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func); + if (!strcmp(unitname, unit_addr)) + return; + + FAIL(c, "Node %s PCI unit address format error, expected \"%s\"", + node->fullpath, unit_addr); +} +WARNING(pci_device, check_pci_device, NULL, ®_format); + /* * Style checks */ @@ -775,6 +855,9 @@ static struct check *check_table[] = { &unit_address_vs_reg, &unit_address_format, + &pci_bridge, + &pci_device, + &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, diff --git a/dtc.h b/dtc.h index c6f125c68ba8..c538ef4afafb 100644 --- a/dtc.h +++ b/dtc.h @@ -136,6 +136,12 @@ struct label { struct label *next; }; +#define PCI_BUS_TYPE 1 + +struct bus_type { + int type; +}; + struct property { bool deleted; char *name; @@ -162,6 +168,7 @@ struct node { int addr_cells, size_cells; struct label *labels; + const struct bus_type *bus; }; #define for_each_label_withdel(l0, l) \
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 ranges is present and #address-cells and For devices, the primary check is the reg property and the unit address. 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. Signed-off-by: Rob Herring <robh@kernel.org> --- 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ dtc.h | 7 ++++++ 2 files changed, 90 insertions(+) -- 2.10.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html