LKML Archive on lore.kernel.org
 help / Atom feed
From: Sibi S <sibis@codeaurora.org>
To: Sricharan R <sricharan@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Rohit kumar <rohitkr@codeaurora.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [RFC PATCH 2/5] remoteproc: q6v5: Extract common resource handling
Date: Fri, 1 Jun 2018 20:48:37 +0530
Message-ID: <5a1ecc9e-34e8-63be-3502-5d2927fbc97c@codeaurora.org> (raw)
In-Reply-To: <8c2acd62-0f3b-9167-b028-1f5149bd7937@codeaurora.org>

Hi Sricharan,

On 06/01/2018 11:46 AM, Sricharan R wrote:
> Hi Bjorn,
>    Thanks for this much needed consolidation.
> 
> On 5/23/2018 10:50 AM, Bjorn Andersson wrote:
>> Shared between all Hexagon V5 based remoteprocs is the handling of the 5
>> interrupts and the SMP2P stop request, so break this out into a separate
>> function in order to allow these drivers to be cleaned up.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>   drivers/remoteproc/Kconfig     |   5 +
>>   drivers/remoteproc/Makefile    |   1 +
>>   drivers/remoteproc/qcom_q6v5.c | 243 +++++++++++++++++++++++++++++++++
>>   drivers/remoteproc/qcom_q6v5.h |  46 +++++++
>>   4 files changed, 295 insertions(+)
>>   create mode 100644 drivers/remoteproc/qcom_q6v5.c
>>   create mode 100644 drivers/remoteproc/qcom_q6v5.h
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index cd1c168fd188..63b79ea91a21 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -102,6 +102,11 @@ config QCOM_ADSP_PIL
>>   config QCOM_RPROC_COMMON
>>   	tristate
>>   
>> +config QCOM_Q6V5_COMMON
>> +	tristate
>> +	depends on ARCH_QCOM
>> +	depends on QCOM_SMEM
>> +
>>   config QCOM_Q6V5_PIL
>>   	tristate "Qualcomm Hexagon V5 Peripherial Image Loader"
>>   	depends on OF && ARCH_QCOM
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index 02627ede8d4a..5dd0249cf76a 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>>   obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
>>   obj-$(CONFIG_QCOM_ADSP_PIL)		+= qcom_adsp_pil.o
>>   obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
>> +obj-$(CONFIG_QCOM_Q6V5_COMMON)		+= qcom_q6v5.o
>>   obj-$(CONFIG_QCOM_Q6V5_PIL)		+= qcom_q6v5_pil.o
>>   obj-$(CONFIG_QCOM_SYSMON)		+= qcom_sysmon.o
>>   obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss_pil.o
>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
>> new file mode 100644
>> index 000000000000..9076537a1671
>> --- /dev/null
>> +++ b/drivers/remoteproc/qcom_q6v5.c
>> @@ -0,0 +1,243 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Qualcomm Peripheral Image Loader for Q6V5 WCSS
>> + *
> 
>    Probably just Q6V5, QCSS not needed.
> 
>> + * Copyright (C) 2016-2018 Linaro Ltd.
>> + * Copyright (C) 2014 Sony Mobile Communications AB
>> + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/soc/qcom/smem.h>
>> +#include <linux/soc/qcom/smem_state.h>
>> +#include <linux/remoteproc.h>
>> +#include "qcom_q6v5.h"
>> +
>> +/**
>> + * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before start
>> + * @q6v5:	reference to qcom_q6v5 context to be reinitialized
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5)
>> +{
>> +	reinit_completion(&q6v5->start_done);
>> +	reinit_completion(&q6v5->stop_done);
>> +
>> +	q6v5->running = true;
>> +	q6v5->handover_issued = false;
>> +
>> +	enable_irq(q6v5->handover_irq);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qcom_q6v5_unprepare() - unprepare the qcom_q6v5 context after stop
>> + * @q6v5:	reference to qcom_q6v5 context to be unprepared
>> + *
>> + * Return: 0 on success, 1 if handover hasn't yet been called
>> + */
>> +int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5)
>> +{
>> +	disable_irq(q6v5->handover_irq);
>> +
>> +	return !q6v5->handover_issued;
>> +}
>> +
>> +static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
>> +{
>> +	struct qcom_q6v5 *q6v5 = data;
>> +	size_t len;
>> +	char *msg;
>> +
>> +	/* Sometimes the stop triggers a watchdog rather than a stop-ack */
>> +	if (!q6v5->running) {
>> +		complete(&q6v5->stop_done);
>> +		return IRQ_HANDLED;
>> +	}
>> +
> 
>     Does this change the behavior for adsp pil, which was unconditionally
>     doing a rproc_report_crash before, but now checks for the running flag
>     or probably this is the correct sequence ?
> 
>> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, q6v5->crash_reason, &len);
>> +	if (!IS_ERR(msg) && len > 0 && msg[0])
>> +		dev_err(q6v5->dev, "watchdog received: %s\n", msg);
>> +	else
>> +		dev_err(q6v5->dev, "watchdog without message\n");
>> +
>> +	rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
>> +
> 
>     Should be rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
> 
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_fatal_interrupt(int irq, void *data)
>> +{
>> +	struct qcom_q6v5 *q6v5 = data;
>> +	size_t len;
>> +	char *msg;
>> +
>> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, q6v5->crash_reason, &len);
>> +	if (!IS_ERR(msg) && len > 0 && msg[0])
>> +		dev_err(q6v5->dev, "fatal error received: %s\n", msg);
>> +	else
>> +		dev_err(q6v5->dev, "fatal error without message\n");
>> +
>> +	rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_ready_interrupt(int irq, void *data)
>> +{
>> +	struct qcom_q6v5 *q6v5 = data;
>> +
>> +	complete(&q6v5->start_done);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>    For adsp, previously start_done completion was done as a part of
>    handover interrupt, now its done in ready. Does it mean that the
>    entries in DT should be changed etc ?

ready interrupt has always been a part dt entry however a corresponding
interrupt handler was never registered. The trigger condition for
the handover interrupt in the remote processor seem to vary across SoCs
but we can always rely on the ready interrupt to declare the rproc
device is running.

>> +
>> +/**
>> + * qcom_q6v5_wait_for_start() - wait for remote processor start signal
>> + * @q6v5:	reference to qcom_q6v5 context
>> + * @timeout:	timeout to wait for the event, in jiffies
>> + *
>> + * qcom_q6v5_unprepare() should not be called when this function fails.
>> + *
>> + * Return: 0 on success, -ETIMEDOUT on timeout
>> + */
>> +int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout)
>> +{
>> +	int ret;
>> +
>> +	ret = wait_for_completion_timeout(&q6v5->start_done, timeout);
>> +	if (!ret)
>> +		disable_irq(q6v5->handover_irq);
>> +
>> +	return !ret ? -ETIMEDOUT : 0;
>> +}
>> +
>> +static irqreturn_t q6v5_handover_interrupt(int irq, void *data)
>> +{
>> +	struct qcom_q6v5 *q6v5 = data;
>> +
>> +	if (q6v5->handover)
>> +		q6v5->handover(q6v5);
>> +
>> +	q6v5->handover_issued = true;
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_stop_interrupt(int irq, void *data)
>> +{
>> +	struct qcom_q6v5 *q6v5 = data;
>> +
>> +	complete(&q6v5->stop_done);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * qcom_q6v5_request_stop() - request the remote processor to stop
>> + * @q6v5:	reference to qcom_q6v5 context
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
>> +{
>> +	int ret;
>> +
>> +	q6v5->running = false;
>> +
>> +	qcom_smem_state_update_bits(q6v5->state,
>> +				    BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));
>> +
>> +	ret = wait_for_completion_timeout(&q6v5->stop_done, 5 * HZ);
>> +
>> +	qcom_smem_state_update_bits(q6v5->state, BIT(q6v5->stop_bit), 0);
>> +
>> +	return ret == 0 ? -ETIMEDOUT : 0;
>> +}
>> +
>> +/**
>> + * qcom_q6v5_init() - initializer of the q6v5 common struct
>> + * @q6v5:	handle to be initialized
>> + * @pdev:	platform_device reference for acquiring resources
>> + * @rproc:	associated remoteproc instance
>> + * @crash_reason: SMEM id for crash reason string, or 0 if none
>> + * @handover:	function to be called when proxy resources should be released
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
>> +		   struct rproc *rproc, int crash_reason,
>> +		   void (*handover)(struct qcom_q6v5 *q6v5))
>> +{
>> +	int ret;
>> +
>> +	q6v5->rproc = rproc;
>> +	q6v5->dev = &pdev->dev;
>> +	q6v5->crash_reason = crash_reason;
>> +	q6v5->handover = handover;
>> +
>> +	init_completion(&q6v5->start_done);
>> +	init_completion(&q6v5->stop_done);
>> +
>> +	q6v5->wdog_irq = platform_get_irq_byname(pdev, "wdog");
>> +	ret = devm_request_threaded_irq(&pdev->dev, q6v5->wdog_irq,
>> +					NULL, q6v5_wdog_interrupt,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5 wdog", q6v5);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to acquire wdog IRQ\n");
>> +		return ret;
>> +	}
>> +
>> +	q6v5->fatal_irq = platform_get_irq_byname(pdev, "fatal");
>> +	ret = devm_request_threaded_irq(&pdev->dev, q6v5->fatal_irq,
>> +					NULL, q6v5_fatal_interrupt,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5 fatal", q6v5);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to acquire fatal IRQ\n");
>> +		return ret;
>> +	}
>> +
>> +	q6v5->ready_irq = platform_get_irq_byname(pdev, "ready");
>> +	ret = devm_request_threaded_irq(&pdev->dev, q6v5->ready_irq,
>> +					NULL, q6v5_ready_interrupt,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5 ready", q6v5);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to acquire ready IRQ\n");
>> +		return ret;
>> +	}
>> +
>> +	q6v5->handover_irq = platform_get_irq_byname(pdev, "handover");
>> +	ret = devm_request_threaded_irq(&pdev->dev, q6v5->handover_irq,
>> +					NULL, q6v5_handover_interrupt,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5 handover", q6v5);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to acquire handover IRQ\n");
>> +		return ret;
>> +	}
>> +	disable_irq(q6v5->handover_irq);
>> +
>> +	q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack");
>> +	ret = devm_request_threaded_irq(&pdev->dev, q6v5->stop_irq,
>> +					NULL, q6v5_stop_interrupt,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5 stop", q6v5);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to acquire stop-ack IRQ\n");
>> +		return ret;
>> +	}
>> +
>> +	q6v5->state = qcom_smem_state_get(&pdev->dev, "stop", &q6v5->stop_bit);
>> +	if (IS_ERR(q6v5->state)) {
>> +		dev_err(&pdev->dev, "failed to acquire stop state\n");
>> +		return PTR_ERR(q6v5->state);
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
>> new file mode 100644
>> index 000000000000..7ac92c1e0f49
>> --- /dev/null
>> +++ b/drivers/remoteproc/qcom_q6v5.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __QCOM_Q6V5_H__
>> +#define __QCOM_Q6V5_H__
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/completion.h>
>> +
>> +struct rproc;
>> +struct qcom_smem_state;
>> +
>> +struct qcom_q6v5 {
>> +	struct device *dev;
>> +	struct rproc *rproc;
>> +
>> +	struct qcom_smem_state *state;
>> +	unsigned stop_bit;
>> +
>> +	int wdog_irq;
>> +	int fatal_irq;
>> +	int ready_irq;
>> +	int handover_irq;
>> +	int stop_irq;
>> +
>> +	bool handover_issued;
>> +
>> +	struct completion start_done;
>> +	struct completion stop_done;
>> +
>> +	int crash_reason;
>> +
>> +	bool running;
>> +
>> +	void (*handover)(struct qcom_q6v5 *q6v5);
>> +};
>> +
>> +int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
>> +		   struct rproc *rproc, int crash_reason,
>> +		   void (*handover)(struct qcom_q6v5 *q6v5));
>> +
>> +int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
>> +int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
>> +int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
>> +int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
>> +
>> +#endif
>>
> 
> Regards,
>   Sricharan
> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-23  5:20 [RFC PATCH 0/5] Hexagon remoteproc spring cleaning Bjorn Andersson
2018-05-23  5:20 ` [RFC PATCH 1/5] remoteproc: qcom: mdt_loader: Make the firmware authentication optional Bjorn Andersson
2018-05-23  5:20 ` [RFC PATCH 2/5] remoteproc: q6v5: Extract common resource handling Bjorn Andersson
2018-06-01  6:16   ` Sricharan R
2018-06-01 15:18     ` Sibi S [this message]
2018-06-01 16:39       ` Sricharan R
2018-05-23  5:20 ` [RFC PATCH 3/5] remoteproc: qcom: adsp: Use common q6v5 helpers Bjorn Andersson
2018-06-01  6:25   ` Sricharan R
2018-06-01 13:35   ` Rohit Kumar
2018-05-23  5:20 ` [RFC PATCH 4/5] remoteproc: qcom: q6v5-pil: " Bjorn Andersson
2018-06-01  6:42   ` Sricharan R
2018-05-23  5:20 ` [RFC PATCH 5/5] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver Bjorn Andersson
2018-05-23  6:05   ` Vinod
2018-05-23  6:58     ` Bjorn Andersson
2018-05-23  7:37       ` Vinod
2018-05-23 14:48         ` Sricharan R
2018-05-29  4:07           ` Bjorn Andersson
2018-05-29  8:32             ` Sricharan R
2018-06-01 13:33 ` [RFC PATCH 0/5] Hexagon remoteproc spring cleaning Rohit Kumar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a1ecc9e-34e8-63be-3502-5d2927fbc97c@codeaurora.org \
    --to=sibis@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=rohitkr@codeaurora.org \
    --cc=sricharan@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox