On Sat, Aug 1, 2020 at 1:17 PM Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx> wrote: > > Hi Jim, here's some comments after testing your series against RPi4. > > On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote: > > The new field 'dma_range_map' in struct device is used to facilitate the > > use of single or multiple offsets between mapping regions of cpu addrs and > > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > > capable of holding a single uniform offset and had no region bounds > > checking. > > > > The function of_dma_get_range() has been modified so that it takes a single > > argument -- the device node -- and returns a map, NULL, or an error code. > > The map is an array that holds the information regarding the DMA regions. > > Each range entry contains the address offset, the cpu_start address, the > > dma_start address, and the size of the region. > > > > of_dma_configure() is the typical manner to set range offsets but there are > > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > > driver code. These cases now invoke the function > > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). > > > > Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx> > > --- > > [...] > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index 8eea3f6e29a4..4b718d199efe 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -918,33 +918,33 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index, > > } > > EXPORT_SYMBOL(of_io_request_and_map); > > > > +#ifdef CONFIG_HAS_DMA > > /** > > - * of_dma_get_range - Get DMA range info > > + * of_dma_get_range - Get DMA range info and put it into a map array > > * @np: device node to get DMA range info > > - * @dma_addr: pointer to store initial DMA address of DMA range > > - * @paddr: pointer to store initial CPU address of DMA range > > - * @size: pointer to store size of DMA range > > + * @map: dma range structure to return > > * > > * Look in bottom up direction for the first "dma-ranges" property > > - * and parse it. > > - * dma-ranges format: > > + * and parse it. Put the information into a DMA offset map array. > > + * > > + * dma-ranges format: > > * DMA addr (dma_addr) : naddr cells > > * CPU addr (phys_addr_t) : pna cells > > * size : nsize cells > > * > > - * It returns -ENODEV if "dma-ranges" property was not found > > - * for this device in DT. > > + * It returns -ENODEV if "dma-ranges" property was not found for this > > + * device in the DT. > > */ > > -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) > > +int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > > { > > struct device_node *node = of_node_get(np); > > const __be32 *ranges = NULL; > > - int len; > > - int ret = 0; > > bool found_dma_ranges = false; > > struct of_range_parser parser; > > struct of_range range; > > - u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0; > > + struct bus_dma_region *r; > > + int len, num_ranges = 0; > > + int ret; > > > > while (node) { > > ranges = of_get_property(node, "dma-ranges", &len); > > @@ -970,44 +970,35 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz > > } > > > > of_dma_range_parser_init(&parser, node); > > + for_each_of_range(&parser, &range) > > + num_ranges++; > > + > > + of_dma_range_parser_init(&parser, node); > > + > > + ret = -ENOMEM; > > + r = kcalloc(num_ranges + 1, sizeof(*r), GFP_KERNEL); > > + if (!r) > > + goto out; > > > > + /* > > + * Record all info in the generic DMA ranges array for struct device. > > + */ > > + *map = r; > > for_each_of_range(&parser, &range) { > > pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n", > > range.bus_addr, range.cpu_addr, range.size); > > - > > - if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset) { > > - pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node); > > - /* Don't error out as we'd break some existing DTs */ > > - continue; > > - } > > - dma_offset = range.cpu_addr - range.bus_addr; > > - > > - /* Take lower and upper limits */ > > - if (range.bus_addr < dma_start) > > - dma_start = range.bus_addr; > > - if (range.bus_addr + range.size > dma_end) > > - dma_end = range.bus_addr + range.size; > > - } > > - > > - if (dma_start >= dma_end) { > > - ret = -EINVAL; > > - pr_debug("Invalid DMA ranges configuration on node(%pOF)\n", > > - node); > > - goto out; > > + r->cpu_start = range.cpu_addr; > > + r->dma_start = range.bus_addr; > > + r->size = range.size; > > + r->offset = (u64)range.cpu_addr - (u64)range.bus_addr; > > + r++; > > } > > > > - *dma_addr = dma_start; > > - *size = dma_end - dma_start; > > - *paddr = dma_start + dma_offset; > > - > > - pr_debug("final: dma_addr(%llx) cpu_addr(%llx) size(%llx)\n", > > - *dma_addr, *paddr, *size); > > - > > I think you're missing here: > > ret = 0; Yes. It somehow passed my tests because it still sets dev->dma_range_map. Thanks again, Jiom > > > out: > > of_node_put(node); > > - > > return ret; > > } > > Regards, > Nicolas > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel