Re: [RFC PATCH 2/5] remoteproc: q6v5: Extract common resource handling

[Sricharan R]

Hi Sibi,

On 6/1/2018 8:48 PM, Sibi S wrote:
> 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@xxxxxxxxxx>
>>> ---
>>> Â 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.
> 

 All right. My doubt was more because start_done completion  being
 moved to ready. I guess Rohit has already verified the functionality
 with the patch. So should be fine.

Regards,
 Sricharan
 
>>> +
>>> +/**
>>> + * 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 INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus