On Tue 23 Aug 08:14 PDT 2016, Stanimir Varbanov wrote:

Hi Bjorn,

On 08/23/2016 08:57 AM, Bjorn Andersson wrote:

quoted

From: Bjorn Andersson <redacted>

The Qualcomm ADSP Peripheral Image Loader is used on a variety of
different Qualcomm platforms for loading firmware into and controlling
the Hexagon based ADSP.

Signed-off-by: Bjorn Andersson <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---

Changes since v1:
- Added platform names to compatibility
- Removed some incorrect quirks that tried to handle older platforms

<cut>

quoted

+
+static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
+{
+	struct device_node *node;
+	struct resource r;
+	int ret;
+
+	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(adsp->dev, "no memory-region specified\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(node, 0, &r);
+	if (ret)
+		return ret;
+

I think that the parsing of memory-region and subsequent
address_to_resource is not so good. I had the same issue with Venus
remoteproc driver and come to the more standard way by using
of_reserved_mem_device_init() and dma_alloc_coherent.

I tried this in an earlier version. But on 8974 the adsp is supposed to
be loaded at 0xdc00000, where we have a chunk that's 0x1900000 (25.6MB).

The problem is that dma_alloc_coherent(25.6MB) will fail, because there
is not 32MB free in that 25.6MB region. This stems from how
dma_alloc_coherent() is implemented, so some work would need to be done
there.


Further more, in remoteproc you are expected to provide a resource table
detailing carveout regions like this - a table that's supposed to come
from the ELF. We are currently looking into ways of injecting individual
resource requests from a remoteproc driver, so I hope to replace this
with a "rproc_add_carveout()".

But then this shows another issue with using dma_alloc_coherent(), it
suddenly requires us to create dummy devices for each memory region; to
be able to control which dma_mem should be used for each operation.


So, I intend to move most of this handling into the remoteproc core, but
I'm still uncertain if I we will not just have the same logic, but
shared in the core...

Could you review "[PATCH 0/4] Venus remoteproc driver" patchset in this
regard?

I will, sorry for the delays.

quoted

+	adsp->mem_phys = adsp->mem_reloc = r.start;
+	adsp->mem_size = resource_size(&r);
+	adsp->mem_region = devm_ioremap_wc(adsp->dev, adsp->mem_phys, adsp->mem_size);
+	if (!adsp->mem_region) {
+		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
+			&r.start, adsp->mem_size);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+

Regards,
Bjorn