Re: [U-Boot] [PATCH v2 4/7] fdt: Add several apis to decode pci device node

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 and
  

parent

•    * #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.

•   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.

•           }
  

•           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/

•           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 '.'?

•                   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.

•                   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

•   /*
  

•    * 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.

•   int ret;
  

•   /* get pci devices's bdf */
  

•   ret = fdtdec_get_pci_bdf(blob, node, addr, &bdf);
  

•   if (ret)
  

•           return ret;
  

•   /* extract the bar number from fdt_pci_addr */
  

•   bn = addr->phys_hi & 0xff;
  

•   if ((bn < PCI_BASE_ADDRESS_0) || (bn > PCI_CARDBUS_CIS))
  

•           return -EINVAL;
  

•   bn = (bn - PCI_BASE_ADDRESS_0) / 4;
  

•   *bar = pci_read_bar32(pci_bus_to_hose(PCI_BUS(bdf)), bdf, bn);
  

•   return 0;
  

+} +#endif

uint64_t fdtdec_get_uint64(const void *blob, int node, const char *prop_name, uint64_t default_val) { @@ -790,20 +933,6 @@ int fdt_get_named_resource(const void *fdt, int node, const char *property, return fdt_get_resource(fdt, node, property, index, res); }

-int fdtdec_pci_get_bdf(const void *fdt, int node, int *bdf) -{

•   const fdt32_t *prop;
  

•   int len;
  

•   prop = fdt_getprop(fdt, node, "reg", &len);
  

•   if (!prop)
  

•           return len;
  

•   *bdf = fdt32_to_cpu(*prop) & 0xffffff;
  

•   return 0;
  

-}

int fdtdec_decode_memory_region(const void *blob, int config_node, const char *mem_type, const char *suffix, fdt_addr_t *basep, fdt_size_t *sizep) -- 1.8.2.1

Regards, Simon