Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp1519334imm; Wed, 19 Sep 2018 21:16:23 -0700 (PDT) X-Google-Smtp-Source: ANB0VdYRKm25TjrAnCc0ju+zLakRWMWiXfwjfEPJwk+LSAqfRhCaHchffbOlRlw8r1ijx4KjhZ7M X-Received: by 2002:a63:144b:: with SMTP id 11-v6mr34072452pgu.219.1537416983427; Wed, 19 Sep 2018 21:16:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1537416983; cv=none; d=google.com; s=arc-20160816; b=BpM/62bmg1tmd8b1tBTBXohVAU/9kWsopldCd3gi+Gvbi/yWgW3NeSmjwBh3OT6q3L bXIdZdBxlIuKS04mBC+/mStbFlgRjLYnUFiEcRDNjsVOe+NZh97D5TzMPC972gzWxYUQ 9xZ0lDedRXvEvyPpt/47hJ9ZQtF2M4iDnLbm3MrhGK95W05NSJc0P8z7quHlbRtNPute fN5JJJZCtsIbriPgZQ6O0QplXYe4Xp5jOF87cjB1FmvCJFF3OZuEjn7obX7Wsz4KllJh WM+h7m8P2fVTB4CgiraJKHU47DJ+A3aH7s2I69SbPO+xhCE8hJ1Il61tnTpoiItc2hg7 YgTQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=0UI1UJ3VGKb0pjcJ1KCiEileSfBeYUk/GSyLEbrPm7Q=; b=EYYXG6Tjex1m38Zmo7WkwZPYkKBb96MRvJlXW2JH0cX++jzWBo1CWkyW33re1HG2x5 V3RfiYhh61xbRir1bVE20manGAqUzGTvzXfJM99tNVky8lghNfWAZJ2eKA8BJBRB1c4O C9zKU1xenOj1TAnwUGbfQkaNQ70Lf/dHZaUsA5FqA5hBoWGbdfxqftb3WAFRgAXJc4IX P4njF/UjJl6/o/iImPm12Ec8vfuz8XpJ0eNq+MoW6iWEHnUmGQtitR4I8ZTMO91jyi+R BegzOm6WoEsDXt0d0QpOdrMTFy5vW3OwEBM6v5sWFPvPN7ss7JSBKlcQmnY1lCe5PZ/w Ec8A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=DJjIroN1; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 31-v6si21504277pgm.551.2018.09.19.21.16.07; Wed, 19 Sep 2018 21:16:23 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=DJjIroN1; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732607AbeITJNF (ORCPT + 99 others); Thu, 20 Sep 2018 05:13:05 -0400 Received: from mail-it0-f65.google.com ([209.85.214.65]:53191 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731479AbeITJNF (ORCPT ); Thu, 20 Sep 2018 05:13:05 -0400 Received: by mail-it0-f65.google.com with SMTP id h3-v6so11305744ita.2 for ; Wed, 19 Sep 2018 20:31:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0UI1UJ3VGKb0pjcJ1KCiEileSfBeYUk/GSyLEbrPm7Q=; b=DJjIroN1AeZX9HgqtK7PO7zwI6r+Jxd/7nrbB9vDUc/WiPkO+tn1ow0f4SHSra+ZPQ Ly2QEcDtNVP+NEKpvnKA5wtik4mam4r+jz1me4e2G+jk5a0P3rRQ4vGhquN9YsIgdfW1 kKEzsl2esCkt9Jh3kqhrmvGvfUnTHImnG3YgU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0UI1UJ3VGKb0pjcJ1KCiEileSfBeYUk/GSyLEbrPm7Q=; b=osTP6uyPwkdRt807vmNt5LTj6tKRMjRkIXt9Cbl098BjzYNpL+9wgkTBci4WG68gNT 1qYqdwWWVeEGjVlQzqlrWlmbMMHYAlcYGWUWjB+6zC7hv6f3MJ/oukExyqu3a1Q0XSVf TDDgMFsyzWWruRU0o9DoWWA6kPaCdNzV/y0vvRf1VteqUZwzsrLL7lk1/G6aEsRsT+bV p6LzkgfsNgyfphvBLuyYqza2LdUMt26z9dx3/e4JBklX5H1e5tcJ3mI4ljKJzftN5wZs AJN/DwhTDyT1ksezYXbEeM7IBVUMkmIMwJpk4sVEXJ+GST4vIsVFv54KlytBu4StMfXw NcyQ== X-Gm-Message-State: APzg51BCee4xZtonAMo8MiUIYAyYEVWqgx53EcSkmYqR9WFCELw0JdYW bVyjg1csQoxgPMU4Lq0yn58wy0qn8CXDkA== X-Received: by 2002:a24:7941:: with SMTP id z62-v6mr665833itc.20.1537414313569; Wed, 19 Sep 2018 20:31:53 -0700 (PDT) Received: from mail-it0-f46.google.com (mail-it0-f46.google.com. [209.85.214.46]) by smtp.gmail.com with ESMTPSA id f74-v6sm358488itf.20.2018.09.19.20.31.52 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 19 Sep 2018 20:31:52 -0700 (PDT) Received: by mail-it0-f46.google.com with SMTP id j198-v6so451477ita.0 for ; Wed, 19 Sep 2018 20:31:52 -0700 (PDT) X-Received: by 2002:a24:f54a:: with SMTP id k71-v6mr647746ith.36.1537414311816; Wed, 19 Sep 2018 20:31:51 -0700 (PDT) MIME-Version: 1.0 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> <175fcecf3be715d2a20b71746c648f1e@codeaurora.org> In-Reply-To: <175fcecf3be715d2a20b71746c648f1e@codeaurora.org> From: Alexandre Courbot Date: Thu, 20 Sep 2018 12:31:39 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9 To: vgarodia@codeaurora.org Cc: Stanimir Varbanov , 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 20, 2018 at 2:55 AM Vikash Garodia wrote: > > 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. Making sure the flag is explicitly initialized instead of relying on default initialization is another good reason to have that change IMHO. :) > > >> > >>> * @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. That's reasonable as well. I really don't feel strongly about this, so please feel free to keep it as it is.