On Sun, Feb 12, 2017 at 11:03 PM, David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
