Re: [U-Boot] [PATCH v2 04/16] dm: core: Add a new api to get indexed device address

+Stephen

Hi,

On 4 November 2015 at 01:16, Mugunthan V N mugunthanvnm@ti.com wrote:

Add new api to get device address based on index.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/core/device.c | 16 ++++++++++++---- include/dm/device.h | 10 ++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-)

You are touching critical core code so I think we need to be careful here.

diff --git a/drivers/core/device.c b/drivers/core/device.c index 758f390..3cdcab5 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -581,18 +581,21 @@ const char *dev_get_uclass_name(struct udevice *dev) return dev->uclass->uc_drv->name; }

-fdt_addr_t dev_get_addr(struct udevice *dev) +fdt_addr_t dev_get_addr_index(struct udevice *dev, int index) { #if CONFIG_IS_ENABLED(OF_CONTROL) fdt_addr_t addr;

    if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
            const fdt32_t *reg;

•           int len = 0;
  
  

•           reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", NULL);
  

•           if (!reg)
  

•           reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", &len);
  

•           if (!reg || (len <= (index * sizeof(fdt32_t) * 2)))
                    return FDT_ADDR_T_NONE;
  
  

•           reg += index * 2;
  

This seems to be assuming that the address and size each occupy a single cell, but that is not the case in general (e.g. with 64-bit machines).

I think you need to call fdt_addr_cells() and fdt_size_cells(). See fdtdec_get_addr_size_auto_parent() as an example.

            /*
             * Use the full-fledged translate function for complex
             * bus setups.

@@ -608,7 +611,7 @@ fdt_addr_t dev_get_addr(struct udevice *dev) addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob, dev->parent->of_offset, dev->of_offset, "reg",

•                                           0, NULL);
  

•                                           index, NULL);
    if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
            if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
                    addr = simple_bus_translate(dev->parent, addr);
  

@@ -620,6 +623,11 @@ fdt_addr_t dev_get_addr(struct udevice *dev) #endif }

+fdt_addr_t dev_get_addr(struct udevice *dev) +{

•   return dev_get_addr_index(dev, 0);
  

+}

bool device_has_children(struct udevice *dev) { return !list_empty(&dev->child_head); diff --git a/include/dm/device.h b/include/dm/device.h index 28ba4ca..4de66d7 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -454,6 +454,16 @@ int device_find_next_child(struct udevice **devp); fdt_addr_t dev_get_addr(struct udevice *dev);

/**

• • dev_get_addr() - Get the reg property of a device

dev_get_addr_index

• • @dev: Pointer to a device
• • @index: reg address array index

I don't think this is sufficiently clear. Can you add a comment saying that the 'reg' property can hold a list of <addr, size> pairs and @index is used to select which one is used?

• • @return addr
• */

+fdt_addr_t dev_get_addr_index(struct udevice *dev, int index);

+/**

• device_has_children() - check if a device has any children
• @dev: Device to check

-- 2.6.2.280.g74301d6

Regards, Simon