Re: [PATCH v3 06/12] dm: Introduce DMA constraints into the core device model

Hi Nicolas,

On Tue, 15 Dec 2020 at 10:23, Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:

Calculating the DMA offset between a bus address space and CPU's every time we call phys_to_bus() and bus_to_phys() isn't ideal performance wise, as it implies traversing the device tree from the device's node up to the root. Since this information is static and available before the device's initialization, parse it before the probe call an provide the DMA offset in 'struct udevice' for the address translation code to use it.

Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de

---

Changes since v2:

• Return/Fail on error
• Add config option
• use ulong instead for u64 for dev->dma_offset

Changes since v1:

• Update commit message so as to explain better the reasoning behind this

drivers/core/Kconfig | 10 ++++++++++ drivers/core/device.c | 35 +++++++++++++++++++++++++++++++++++ include/dm/device.h | 3 +++ 3 files changed, 48 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

nits below

diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index ffae6f9795..153be25351 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -113,6 +113,16 @@ config SPL_DM_SEQ_ALIAS numbered devices (e.g. serial0 = &serial0). This feature can be disabled if it is not required, to save code space in SPL.

+config DM_DMA

•   bool "Support per device DMA constraints"
  

per-device

•   depends on DM
  

•   default n
  

•   help
  

•     Enable this to extract per-device DMA constraints, only supported on
  

•     device-tree systems for now. This is needed in order translate
  

•     addresses on systems where different buses have different views of
  

•     the physical address space.
  

config REGMAP bool "Support register maps" depends on DM diff --git a/drivers/core/device.c b/drivers/core/device.c index 4b3dcb3b37..21285432d8 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -421,6 +421,37 @@ fail: return ret; }

+#if CONFIG_IS_ENABLED(DM_DMA)

I feel you could use

if (CONFIG_IS_ENABLED)

in the function below (returning at the start) and avoid an #ifdef?

To do that you'll need access macros - you can see things like that in device.h and asm-generic/global_data.h

+static int device_get_dma_constraints(struct udevice *dev)

Please add a comment to describe this function.

+{

•   struct udevice *parent = dev->parent;
  

•   phys_addr_t cpu = 0;
  

•   dma_addr_t bus = 0;
  

•   u64 size = 0;
  

•   int ret;
  

•   if (!parent || !dev_of_valid(parent))
  

•           return 0;
  

•   /*
  

•    * We start parsing for dma-ranges from the device's bus node. This is
  

•    * specially important on nested buses.
  

•    */
  

•   ret = dev_get_dma_range(parent, &cpu, &bus, &size);
  

•   /* Don't return an error if no 'dma-ranges' were found */
  

•   if (ret && ret != -ENOENT) {
  

•           dm_warn("%s: failed to get DMA range, %d\n", dev->name, ret);
  

•           return ret;
  

•   }
  

•   dev->dma_offset = cpu - bus;
  

•   return 0;
  

+} +#else +static int device_get_dma_constraints(struct udevice *dev) { return 0; } +#endif

int device_probe(struct udevice *dev) { const struct driver *drv; @@ -482,6 +513,10 @@ int device_probe(struct udevice *dev) goto fail; }

•   ret = device_get_dma_constraints(dev);
  

•   if (ret)
  

•           goto fail;
  

•   ret = uclass_pre_probe_device(dev);
    if (ret)
            goto fail;
  

diff --git a/include/dm/device.h b/include/dm/device.h index 5bef484247..9126dc00fe 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -161,6 +161,9 @@ struct udevice { #ifdef CONFIG_DEVRES struct list_head devres_head; #endif +#ifdef CONFIG_DM_DMA

•   ulong dma_offset;
  

Needs a comment

+#endif };

/* Maximum sequence number supported */

2.29.2

Regards, Simon