Hi Simon,
On Sun, Dec 28, 2014 at 9:55 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 27 December 2014 at 05:10, Bin Meng bmeng.cn@gmail.com wrote:
This commit adds several APIs to decode PCI device node according to the Open Firmware PCI bus bindings, including:
- fdtdec_get_pci_addr() for encoded pci address
- fdtdec_get_pci_vendev() for vendor id and device id
- fdtdec_get_pci_bdf() for pci device bdf triplet
- fdtdec_get_pci_bar32() for pci device register bar
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Looks good - some minor comments below.
Changes in v2:
- New patch to add several apis to decode pci device node
include/fdtdec.h | 108 ++++++++++++++++++++++++++++++++++---- lib/fdtdec.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 240 insertions(+), 25 deletions(-)
diff --git a/include/fdtdec.h b/include/fdtdec.h index d2b665c..2b2652f 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -50,6 +50,49 @@ struct fdt_resource { fdt_addr_t end; };
+enum fdt_pci_space {
FDT_PCI_SPACE_CONFIG = 0,FDT_PCI_SPACE_IO = 0x01000000,FDT_PCI_SPACE_MEM32 = 0x02000000,FDT_PCI_SPACE_MEM64 = 0x03000000,FDT_PCI_SPACE_MEM32_PREF = 0x42000000,FDT_PCI_SPACE_MEM64_PREF = 0x43000000,+};
+#define FDT_PCI_ADDR_CELLS 3 +#define FDT_PCI_SIZE_CELLS 2 +#define FDT_PCI_REG_SIZE \
((FDT_PCI_ADDR_CELLS + FDT_PCI_SIZE_CELLS) * sizeof(u32))+/*
- The Open Firmware spec defines PCI physical address as follows:
bits# 31 .... 24 23 .... 16 15 .... 08 07 .... 00
- phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
- phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
- phys.lo cell: llllllll llllllll llllllll llllllll
- where:
- n: is 0 if the address is relocatable, 1 otherwise
- p: is 1 if addressable region is prefetchable, 0 otherwise
- t: is 1 if the address is aliased (for non-relocatable I/O) below 1MB
(for Memory), or below 64KB (for relocatable I/O)
- ss: is the space code, denoting the address space
- bbbbbbbb: is the 8-bit Bus Number
- ddddd: is the 5-bit Device Number
- fff: is the 3-bit Function Number
- rrrrrrrr: is the 8-bit Register Number
- hhhhhhhh: is a 32-bit unsigned number
- llllllll: is a 32-bit unsigned number
- */
+struct fdt_pci_addr {
u32 phys_hi;u32 phys_mid;u32 phys_lo;+};
/**
- Compute the size of a resource.
@@ -252,6 +295,60 @@ fdt_addr_t fdtdec_get_addr_size(const void *blob, int node, const char *prop_name, fdt_size_t *sizep);
/**
- Look at an address property in a node and return the pci address which
- corresponds to the given type in the form of fdt_pci_addr.
- The property must hold one fdt_pci_addr with a lengh.
- @param blob FDT blob
- @param node node to examine
- @param type pci address type (FDT_PCI_SPACE_xxx)
- @param prop_name name of property to find
- @param addr returns pci address in the form of fdt_pci_addr
- @return 0 if ok, negative on error
- */
+int fdtdec_get_pci_addr(const void *blob, int node, enum fdt_pci_space type,
const char *prop_name, struct fdt_pci_addr *addr);+/**
- Look at the compatible property of a device node that represents a PCI
- device and extract pci vendor id and device id from it.
- @param blob FDT blob
- @param node node to examine
- @param vendor vendor id of the pci device
- @param device device id of the pci device
- @return 0 if ok, negative on error
- */
+int fdtdec_get_pci_vendev(const void *blob, int node,
u16 *vendor, u16 *device);+/**
- Look at the pci address of a device node that represents a PCI device
- and parse the bus, device and function number from it.
- @param blob FDT blob
- @param node node to examine
- @param addr pci address in the form of fdt_pci_addr
- @param bdf returns bus, device, function triplet
- @return 0 if ok, negative on error
- */
+int fdtdec_get_pci_bdf(const void *blob, int node,
struct fdt_pci_addr *addr, pci_dev_t *bdf);+/**
- Look at the pci address of a device node that represents a PCI device
- and return base address of the pci device's registers.
- @param blob FDT blob
- @param node node to examine
- @param addr pci address in the form of fdt_pci_addr
- @param bar returns base address of the pci device's registers
- @return 0 if ok, negative on error
- */
+int fdtdec_get_pci_bar32(const void *blob, int node,
struct fdt_pci_addr *addr, u32 *bar);+/**
- Look up a 32-bit integer property in a node and return it. The property
- must have at least 4 bytes of data. The value of the first cell is
- returned.
@@ -677,17 +774,6 @@ int fdt_get_named_resource(const void *fdt, int node, const char *property, struct fdt_resource *res);
/**
- Look at the reg property of a device node that represents a PCI device
- and parse the bus, device and function number from it.
- @param fdt FDT blob
- @param node node to examine
- @param bdf returns bus, device, function triplet
- @return 0 if ok, negative on error
- */
-int fdtdec_pci_get_bdf(const void *fdt, int node, int *bdf);
-/**
- Decode a named region within a memory bank of a given type.
- This function handles selection of a memory region. The region is
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 9d86dba..e40430e 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -121,6 +121,149 @@ fdt_addr_t fdtdec_get_addr(const void *blob, int node, return fdtdec_get_addr_size(blob, node, prop_name, NULL); }
+#ifdef CONFIG_PCI +int fdtdec_get_pci_addr(const void *blob, int node, enum fdt_pci_space type,
const char *prop_name, struct fdt_pci_addr *addr)+{
const u32 *cell;int len;debug("%s: %s: ", __func__, prop_name);/** If we follow the pci bus bindings strictly, we should check* the value of the node's parant node's #address-cells andparent
Fixed in v3.
* #size-cells. They need to be 3 and 2 accordingly. However,* for simplicity we skip the check here.*/cell = fdt_getprop(blob, node, prop_name, &len);I think it would be better to return -ENOENT if cell is NULL and -EINVAL in the case where the number of cells is not a multiple of 5.
Will do in v3.
if (cell && ((len % FDT_PCI_REG_SIZE) == 0)) {int num = len / FDT_PCI_REG_SIZE;int i;for (i = 0; i < num; i++) {if ((fdt_addr_to_cpu(*cell) & type) == type) {addr->phys_hi = fdt_addr_to_cpu(cell[0]);addr->phys_mid = fdt_addr_to_cpu(cell[1]);addr->phys_lo = fdt_addr_to_cpu(cell[2]);break;} else {cell += (FDT_PCI_ADDR_CELLS +FDT_PCI_SIZE_CELLS);}You need a debug() in here to print stuff out, and a way of getting a \n at the end.
Will do in v3.
}if (i == num)goto fail;return 0;}+fail:
debug("(not found)\n");return -ENOENT;+}
+int fdtdec_get_pci_vendev(const void *blob, int node, u16 *vendor, u16 *device) +{
const char *list, *end;int len;list = fdt_getprop(blob, node, "compatible", &len);if (!list)return -ENOENT;end = list + len;while (list < end) {int l = strlen(list);s/l/len/
Will do in v3.
char *s, v[5], d[5];s = strstr(list, "pci");/* check if the string is something like pciVVVV,DDDD */if (s && s[7] == ',') {Should check that end - list > 7 otherwise s[7] might not exist. Also how about checking for the '.'?
Will do in v3.
s += 3;strlcpy(v, s, 5);s += 5;strlcpy(d, s, 5);*vendor = simple_strtol(v, NULL, 16);*device = simple_strtol(d, NULL, 16);I suspect you can avoid this - just use simple_strtol() directly on the correct places in the string. The function will automatically stop when it sees no more hex digits.
Yes, you are right. Will fix this.
return 0;} else {list += (l + 1);}}return -ENOENT;+}
+int fdtdec_get_pci_bdf(const void *blob, int node,
struct fdt_pci_addr *addr, pci_dev_t *bdf)+{
u16 dt_vendor, dt_device, vendor, device;int ret;/* get vendor id & device id from the compatible string */ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);if (ret)return ret;/* extract the bdf from fdt_pci_addr */*bdf = addr->phys_hi & 0xffff00;/* read vendor id & device id based on bdf */pci_read_config_word(*bdf, PCI_VENDOR_ID, &vendor);pci_read_config_word(*bdf, PCI_DEVICE_ID, &device);Check return values
I think there is no need to check return values, as if there is anything wrong with the pci_read_config_word() call, the vendor/device will be set to 0xffff, which are invalid values and will fail at the codes below.
/** Note there are two places in the device tree to fully describe* a pci device: one is via compatible string with a format of* "pciVVVV,DDDD" and the other one is the bdf numbers encoded in* the device node's reg address property. We read the vendor id* and device id based on bdf and compare the values with the* "VVVV,DDDD". If they are the same, then we are good to use bdf* to read device's bar. But if they are different, we have to rely* on the vendor id and device id extracted from the compatible* string and locate the real bdf by pci_find_device(). This is* because normally we may only know device's device number and* function number when writing device tree. The bus number is* dynamically assigned during the pci enumeration process.*/if ((dt_vendor != vendor) || (dt_device != device)) {*bdf = pci_find_device(dt_vendor, dt_device, 0);if (*bdf == -1)return -ENODEV;}return 0;+}
+int fdtdec_get_pci_bar32(const void *blob, int node,
struct fdt_pci_addr *addr, u32 *bar)+{
pci_dev_t bdf;int bn;s/bn/barnum/ since it is self-documenting.
Will do in v3.
[snip]
Regards, Bin