Path: csiph.com!eternal-september.org!feeder.eternal-september.org!aioe.org!bofh.it!news.nic.it!robomod
From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Newsgroups: linux.kernel
Subject: Re: [PATCH v2 2/4] remoteproc: Introduce Qualcomm ADSP PIL
Date: Tue, 23 Aug 2016 17:20:01 +0200
Message-ID: <s9lEB-5Js-25@gated-at.bofh.it>
References: <s9cUF-8g8-9@gated-at.bofh.it> <s9cUG-8g8-15@gated-at.bofh.it>
Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=UL4RXLi9og+OB+6uXJuCvlabPN5K8+bw8eS3oMYPNZ4=; b=PECegV+dPRossaJKbzFOIrpbTrVXbGmAWfBts7a+XQBjEbBB7jgTr/y3Js2o2AmEFx auIwqMSITUceaDFu/BBl0yaAd2cAPW6meP6tJiOPoUAc164K52GONgaZNHVHIZZ0od0Y ib23bPOVLk685VfCp958etbPXawtMVSc6RHF8=
X-Google-Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=UL4RXLi9og+OB+6uXJuCvlabPN5K8+bw8eS3oMYPNZ4=; b=THsEYeJIpOe2uFCxdhfJDyXAN3VvVxv7WBqQuIKeDUyr/VwTgMJ/GW1yjgwccYeueL 9oigPyHxls56Pj+q8I4n30RhUDjMxVBZdOVBGoCT7nBGZnx501iTNggnjo2oXnLfBGjf GAez40H+oUhQ10FvkmHv04msMz17G9/cnD+AEO0XzNsMr6son7V7GWGnpu6VogXeamSB 6kK4AXsGZP7qrBQYlied5/X+2uP0ezAr2Ms4qyXkOwLthUacrKUvumuaHIfxt0KVCELu IXhYqWGIXnfHQpVxCcNmpk1mPvnhVwYc6k61SpBpAF2TtuvEFAwcE1kbgTIpu6D/f1lE VzCA==
X-Gm-Message-State: AEkoousFjw8A8wGiSb6hsw10Yf8S1emtdLMNeVw8j33YLgwFZToEaN5NGA8QQnfdQQnXDOfa
X-Received: by 10.28.95.67 with SMTP id t64mr21764297wmb.99.1471965264869; Tue, 23 Aug 2016 08:14:24 -0700 (PDT)
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: robomod@news.nic.it
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Approved: robomod@news.nic.it
Lines: 62
Organization: linux.* mail to news gateway
X-Original-Cc: Andy Gross <andy.gross@linaro.org>, Mark Rutland <mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-soc@vger.kernel.org, Bjorn Andersson <bjorn.andersson@sonymobile.com>
X-Original-Date: Tue, 23 Aug 2016 18:14:22 +0300
X-Original-Message-ID: <ae9f06c0-6e5e-8c33-f239-2fd60e2e6682@linaro.org>
X-Original-References: <1471931866-3107-1-git-send-email-bjorn.andersson@linaro.org> <1471931866-3107-2-git-send-email-bjorn.andersson@linaro.org>
X-Original-Sender: linux-kernel-owner@vger.kernel.org
Xref: csiph.com linux.kernel:1468631

Hi Bjorn,

On 08/23/2016 08:57 AM, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 
> 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 <bjorn.andersson@sonymobile.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Added platform names to compatibility
> - Removed some incorrect quirks that tried to handle older platforms
> 

<cut>

> +
> +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.

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

> +	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,
Stan