From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BDAB9ECE564 for ; Wed, 19 Sep 2018 17:55:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5B2F42150F for ; Wed, 19 Sep 2018 17:55:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="RqiMu3Ie"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="gc+MaZrk" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5B2F42150F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731877AbeISXeF (ORCPT ); Wed, 19 Sep 2018 19:34:05 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:53476 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726925AbeISXeF (ORCPT ); Wed, 19 Sep 2018 19:34:05 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id C961360BF5; Wed, 19 Sep 2018 17:55:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1537379702; bh=Ty5Le4td8hGpZLNS+S3EhWgPjwtNUY3w17H6HNWkkzI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=RqiMu3Iec7/GAJ+4WZMx1sSFxpfRvN6nltOFC5QH1QOGtk023kYV+g8Z/Z7WIxzkY 4bj6AzL79ll5mXOLlMGCkWYe5rYt5N80fItrDJndYqeGsEoSTMwX+PJPbXgBAW9Niq o2OcPrfbMkCuZ1w+dzHB8vzX3ON+JI9voG5kKps0= Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id BBA0E607BD; Wed, 19 Sep 2018 17:55:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1537379701; bh=Ty5Le4td8hGpZLNS+S3EhWgPjwtNUY3w17H6HNWkkzI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=gc+MaZrkVaiu6wk5ew86TZXd7wUGgx3JW0jGl3PZGwGQxUeUs5vHLMJtjZL+mdVSM 0M7fGbA6NvK/27+Wat1XmaG+aki19KeS7ZgoZz2SiekjJz9k6pRuS8UNTMEhNOdeY8 08bmxk1oikshP2ng6OxOaQyDJMXuE2uBXL07JxqI= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 19 Sep 2018 23:25:01 +0530 From: Vikash Garodia To: Stanimir Varbanov Cc: Alexandre Courbot , Hans Verkuil , Mauro Carvalho Chehab , robh@kernel.org, mark.rutland@arm.com, Andy Gross , Arnd Bergmann , bjorn.andersson@linaro.org, Linux Media Mailing List , LKML , linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-media-owner@vger.kernel.org Subject: Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9 In-Reply-To: <97b94b9b-f028-cb8b-a3db-67626dc517ab@linaro.org> References: <1537314192-26892-1-git-send-email-vgarodia@codeaurora.org> <1537314192-26892-2-git-send-email-vgarodia@codeaurora.org> <97b94b9b-f028-cb8b-a3db-67626dc517ab@linaro.org> Message-ID: <175fcecf3be715d2a20b71746c648f1e@codeaurora.org> X-Sender: vgarodia@codeaurora.org User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018-09-19 20:30, Stanimir Varbanov wrote: > Hi Alex, > > On 09/19/2018 10:32 AM, Alexandre Courbot wrote: >> On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia >> wrote: >>> >>> Add routine to reset the ARM9 and brings it out of reset. Also >>> abstract the Venus CPU state handling with a new function. This >>> is in preparation to add PIL functionality in venus driver. >>> >>> Signed-off-by: Vikash Garodia >>> --- >>> drivers/media/platform/qcom/venus/core.h | 2 ++ >>> drivers/media/platform/qcom/venus/firmware.c | 33 >>> ++++++++++++++++++++++++ >>> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ >>> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- >>> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ >>> 5 files changed, 57 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/core.h >>> b/drivers/media/platform/qcom/venus/core.h >>> index 2f02365..dfd5c10 100644 >>> --- a/drivers/media/platform/qcom/venus/core.h >>> +++ b/drivers/media/platform/qcom/venus/core.h >>> @@ -98,6 +98,7 @@ struct venus_caps { >>> * @dev: convenience struct device pointer >>> * @dev_dec: convenience struct device pointer for decoder device >>> * @dev_enc: convenience struct device pointer for encoder device >>> + * @no_tz: a flag that suggests presence of trustzone >> >> Looks like it suggests the absence of trustzone? >> >> Actually I would rename it as use_tz and set it if TrustZone is used. >> This would avoid double-negative statements like what we see below. > > I find this suggestion reasonable. Initially i planned to keep it as a positive flag. The reason behind keeping it as no_tz was to keep the default value of this flag to 0 indicating tz is present by default. I can switch it to use_tz though and set it in firmware_init after the presence of firmware node is checked. >> >>> * @lock: a lock for this strucure >>> * @instances: a list_head of all instances >>> * @insts_count: num of instances >>> @@ -129,6 +130,7 @@ struct venus_core { >>> struct device *dev; >>> struct device *dev_dec; >>> struct device *dev_enc; >>> + bool no_tz; >>> struct mutex lock; >>> struct list_head instances; >>> atomic_t insts_count; >>> diff --git a/drivers/media/platform/qcom/venus/firmware.c >>> b/drivers/media/platform/qcom/venus/firmware.c >>> index c4a5778..f2ae2f0 100644 >>> --- a/drivers/media/platform/qcom/venus/firmware.c >>> +++ b/drivers/media/platform/qcom/venus/firmware.c >>> @@ -22,10 +22,43 @@ >>> #include >>> #include >>> >>> +#include "core.h" >>> #include "firmware.h" >>> +#include "hfi_venus_io.h" >>> >>> #define VENUS_PAS_ID 9 >>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) >>> +#define VENUS_FW_START_ADDR 0x0 >>> + >>> +static void venus_reset_cpu(struct venus_core *core) >>> +{ >>> + void __iomem *base = core->base; >>> + >>> + writel(0, base + WRAPPER_FW_START_ADDR); >>> + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR); >>> + writel(0, base + WRAPPER_CPA_START_ADDR); >>> + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR); >>> + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR); >>> + writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR); >>> + writel(0x0, base + WRAPPER_CPU_CGC_DIS); >>> + writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG); >>> + >>> + /* Bring ARM9 out of reset */ >>> + writel(0, base + WRAPPER_A9SS_SW_RESET); >>> +} >>> + >>> +int venus_set_hw_state(struct venus_core *core, bool resume) >>> +{ >>> + if (!core->no_tz) >> >> This is the kind of double negative statement I was referring do >> above: "if we do not not have TrustZone". Turning it into >> >> if (core->use_tz) >> >> would save the reader a few neurons. :) >> >>> + return qcom_scm_set_remote_state(resume, 0); >>> + >>> + if (resume) >>> + venus_reset_cpu(core); >>> + else >>> + writel(1, core->base + WRAPPER_A9SS_SW_RESET); >>> + >>> + return 0; >>> +} >>> >>> int venus_boot(struct device *dev, const char *fwname) >>> { >>> diff --git a/drivers/media/platform/qcom/venus/firmware.h >>> b/drivers/media/platform/qcom/venus/firmware.h >>> index 428efb5..397570c 100644 >>> --- a/drivers/media/platform/qcom/venus/firmware.h >>> +++ b/drivers/media/platform/qcom/venus/firmware.h >>> @@ -18,5 +18,16 @@ >>> >>> int venus_boot(struct device *dev, const char *fwname); >>> int venus_shutdown(struct device *dev); >>> +int venus_set_hw_state(struct venus_core *core, bool suspend); >>> + >>> +static inline int venus_set_hw_state_suspend(struct venus_core >>> *core) >>> +{ >>> + return venus_set_hw_state(core, false); >>> +} >>> + >>> +static inline int venus_set_hw_state_resume(struct venus_core *core) >>> +{ >>> + return venus_set_hw_state(core, true); >>> +} >> >> I think these two venus_set_hw_state_suspend() and >> venus_set_hw_state_resume() are superfluous, if you want to make the >> state explicit you can also define an enum { SUSPEND, RESUME } to use >> as argument of venus_set_hw_state() and call it directly. > > Infact this was by my request, and I wanted to avoid enum and have the > type of the action in the function name and also avoid one extra > function argument. Of course it is a matter of taste.