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=-0.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, T_DKIMWL_WL_HIGH,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 939E1C433F4 for ; Tue, 28 Aug 2018 05:50:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4318420659 for ; Tue, 28 Aug 2018 05:50:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="NngVZWMc" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4318420659 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.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 S1727087AbeH1Jkk (ORCPT ); Tue, 28 Aug 2018 05:40:40 -0400 Received: from mail-io0-f195.google.com ([209.85.223.195]:35603 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726181AbeH1Jkk (ORCPT ); Tue, 28 Aug 2018 05:40:40 -0400 Received: by mail-io0-f195.google.com with SMTP id w11-v6so370334iob.2 for ; Mon, 27 Aug 2018 22:50:40 -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=TS6e59ioU0vAnpUBjRPSIEHk7nsmswXO+XTQ6up7kh0=; b=NngVZWMcqO+hvfaRUA7WBZH65YzGP0G0uPBvOTv4F1ZfJwMEOonGJSZF+UHbsBWFjO BiZN5dnlRwSv9uXEwQyrsPe4c1Umgi+opS4TFuzG4eOpgTeyaaB+h9+12mkRpBtsGqOt myDAe9K2iU0n4ySTqXGet4QpcJ6ICSoEwHoKo= 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=TS6e59ioU0vAnpUBjRPSIEHk7nsmswXO+XTQ6up7kh0=; b=kINRuzGFbZQH5podbR6BR4raaGeMy12iVrkj68Mnf++5pBXGkvR1Eiv1sn3yD9sTtk Eko7SGwNzOxGavF2A1HUXbrXRJcefIyWfQ5uD7PpuURGhX6QvveRz4bYS8loM7OD4keM UzErZC99Tco2Uxmqr6JBAkAddg70cO8DVvse/cxiCQXn3gLXgkyPqpZro7ISkF5yYqey E+SGtUkQTU+PM6y7h7qMHUrNWWclmRHn0WtOMmKnvhI2oz9OlvmVSRGaZbFR202ZtK/4 FHIyA5EFcVsPAfAMflbLn2h1gom//5vuN7aK/8BK652FSQdzfgXH0+YOFGm3pkG/UnDT e2jg== X-Gm-Message-State: APzg51D4oA2gxuccidmpNoDOfPpvAojdbAG4t51X1sRzZ0EU3qruYJXV G87Tjs/kVgExl5ValNvaiUY+lxwmYm8= X-Google-Smtp-Source: ANB0VdYfPHvJVvbKWgstTlFuhi1/lfOmxlTk8yjaN5VOD4sBUEr/umdvNeNoB3JKlyjqWoNo92mt/w== X-Received: by 2002:a5e:db07:: with SMTP id q7-v6mr16009iop.81.1535435022066; Mon, 27 Aug 2018 22:43:42 -0700 (PDT) Received: from mail-it0-f54.google.com (mail-it0-f54.google.com. [209.85.214.54]) by smtp.gmail.com with ESMTPSA id h2-v6sm59562iok.29.2018.08.27.22.43.41 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 27 Aug 2018 22:43:42 -0700 (PDT) Received: by mail-it0-f54.google.com with SMTP id x79-v6so1213727ita.1 for ; Mon, 27 Aug 2018 22:43:41 -0700 (PDT) X-Received: by 2002:a24:dfc6:: with SMTP id r189-v6mr267323itg.36.1535435021449; Mon, 27 Aug 2018 22:43:41 -0700 (PDT) MIME-Version: 1.0 References: <1535034528-11590-1-git-send-email-vgarodia@codeaurora.org> <1535034528-11590-2-git-send-email-vgarodia@codeaurora.org> <51cc9d6b-0483-76a6-d413-3f5cc63f3f56@linaro.org> <85ffba9c-8b29-4c7a-7aec-999af94aeda0@linaro.org> In-Reply-To: <85ffba9c-8b29-4c7a-7aec-999af94aeda0@linaro.org> From: Alexandre Courbot Date: Tue, 28 Aug 2018 14:43:29 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v6 1/4] venus: firmware: add routine to reset ARM9 To: Stanimir Varbanov Cc: vgarodia@codeaurora.org, 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 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 Mon, Aug 27, 2018 at 7:56 PM Stanimir Varbanov wrote: > > > > On 08/27/2018 06:04 AM, Alexandre Courbot wrote: > > On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov > > wrote: > >> > >> Hi Alex, > >> > >> On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > >>> On Thu, Aug 23, 2018 at 11:29 PM 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 > >>>> * @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..a9d042e 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) > >>> > >>> This is making a strong assumption about the size of the FW memory > >>> region, which in practice is not always true (I had to reduce it to > >>> 5MB). How about having this as a member of venus_core, which is > >> > >> Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > >> waste reserved memory? > > > > The DT layout of our board only has 5MB reserved for Venus. > > > >>> initialized in venus_load_fw() from the actual size of the memory > >>> region? You could do this as an extra patch that comes before this > >>> one. > >>> > >> > >> The size is 6MB by historical reasons and they are no more valid, so I > >> think we could safely decrease to 5MB. I could prepare a patch for that. > > > > Whether we settle with 6MB or 5MB, that size remains arbitrary and not > > based on the actual firmware size. And __qcom_mdt_load() does check > > If we go with 5MB it will not be arbitrary, cause all firmware we have > support to are expecting that size of memory. > > > that the firmware fits the memory area. So I don't understand what > > extra safety is added by ensuring the memory region is larger than a > > given number of megabytes? > > > > OK, is that fine for you? Drop size and check does memory region size is > big enough to contain the firmware. > > diff --git a/drivers/media/platform/qcom/venus/firmware.c > b/driver/media/platform/qcom/venus/firmware.c > index c4a577848dd7..9bf0d21e02d4 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -25,7 +25,6 @@ > #include "firmware.h" > > #define VENUS_PAS_ID 9 > -#define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > int venus_boot(struct device *dev, const char *fwname) > { > @@ -54,25 +53,28 @@ int venus_boot(struct device *dev, const char *fwname) > mem_phys = r.start; > mem_size = resource_size(&r); > > - if (mem_size < VENUS_FW_MEM_SIZE) > - return -EINVAL; > - > - mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > - if (!mem_va) { > - dev_err(dev, "unable to map memory region: %pa+%zx\n", > - &r.start, mem_size); > - return -ENOMEM; > - } > - > ret = request_firmware(&mdt, fwname, dev); > if (ret < 0) > - goto err_unmap; > + return ret; > > fw_size = qcom_mdt_get_size(mdt); > if (fw_size < 0) { > ret = fw_size; > release_firmware(mdt); > - goto err_unmap; > + return ret; > + } > + > + if (mem_size < fw_size) { > + release_firmware(mdt); > + return -EINVAL; > + } > + > + mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > + if (!mem_va) { > + release_firmware(mdt); > + dev_err(dev, "unable to map memory region: %pa+%zx\n", > + &r.start, mem_size); > + return -ENOMEM; > } That looks reasonable to me, at least if the firmware does not require extra memory beyond its reported size. But you know the firmware better, so your call! :)