Re: [PATCH 1/8] ethosu: Add Arm Ethos-U driver

From: Robin Murphy
Date: Fri Jun 16 2023 - 11:49:24 EST


On 2023-06-16 06:59, Alison Wang wrote:
[...]
+/*
+ * The 'dma-ranges' device tree property for shared dma memory does not seem
+ * to be fully supported for coherent memory. Therefor we apply the DMA range
+ * offset ourselves.
+ */

NAK - if there's a bug in the core code, that wants to be fixed, not bodged around by individual drivers. However from the look of the code here, the driver appears to be misusing the property in an incorrect manner anyway. But of course there's no devicetree binding here, so we don't even really know what it thinks it expects... :/

I'd also agree with Greg that this is definitely not a firmware driver. IIUC it's not so much a driver for the Ethos-U NPU itself, but one for this particular subsystem configuration where requests to the NPU are proxied through a dedicated Cortex-M core. As such, if you don't think it belongs in drivers/accel, then drivers/remoteproc might be the next most relevant choice. Also, is there a more specific name for this particular subsystem, or is there a general expectation that this is the only way an Ethos-U should ever be exposed to Linux, and it should never have direct access to the hardware (as it would with an Ethos-N), and thus there's no chance of ending up with multiple different "Ethos-U" drivers in future?

Thanks,
Robin.

+static dma_addr_t ethosu_buffer_dma_ranges(struct device *dev,
+ dma_addr_t dma_addr,
+ size_t dma_buf_size)
+{
+ struct device_node *node = dev->of_node;
+ const __be32 *ranges;
+ int len;
+ int naddr;
+ int nsize;
+ int inc;
+ int i;
+
+ if (!node)
+ return dma_addr;
+
+ /* Get the #address-cells and #size-cells properties */
+ naddr = of_n_addr_cells(node);
+ nsize = of_n_size_cells(node);
+
+ /* Read the 'dma-ranges' property */
+ ranges = of_get_property(node, "dma-ranges", &len);
+ if (!ranges || len <= 0)
+ return dma_addr;
+
+ dev_dbg(dev, "ranges=%p, len=%d, naddr=%d, nsize=%d\n",
+ ranges, len, naddr, nsize);
+
+ len /= sizeof(*ranges);
+ inc = naddr + naddr + nsize;
+
+ for (i = 0; (i + inc) <= len; i += inc) {
+ dma_addr_t daddr;
+ dma_addr_t paddr;
+ dma_addr_t size;
+
+ daddr = of_read_number(&ranges[i], naddr);
+ paddr = of_read_number(&ranges[i + naddr], naddr);
+ size = of_read_number(&ranges[i + naddr + naddr], nsize);
+
+ dev_dbg(dev, "daddr=0x%llx, paddr=0x%llx, size=0x%llx\n",
+ daddr, paddr, size);
+
+ if (dma_addr >= paddr &&
+ (dma_addr + dma_buf_size) < (paddr + size))
+ return dma_addr + daddr - paddr;
+ }
+
+ return dma_addr;
+}