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=-21.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS 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 32CFFC48BC2 for ; Sun, 27 Jun 2021 20:38:07 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 527EF61C34 for ; Sun, 27 Jun 2021 20:38:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 527EF61C34 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id EDC3182C6F; Sun, 27 Jun 2021 22:38:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="nKrCKdrT"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2C42082C6F; Sun, 27 Jun 2021 22:38:01 +0200 (CEST) Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id B581882C72 for ; Sun, 27 Jun 2021 22:37:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-wm1-x333.google.com with SMTP id r3so5355731wmq.1 for ; Sun, 27 Jun 2021 13:37:57 -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=RxbFEV7zzUwXd+2E3JU056gUgx8tNKpJUrzF5DVEX08=; b=nKrCKdrTFu4j+Cydps70lBVVev7lOwsIVRIQW/V+uOosyJj9iZWH04hrsPfuD0s6ea Mrr1ETroq6WT8Hk9atgTkqY7xmCtdT+3EqUpdn6TNaLk3OyIePDCm73wcryVKHihZUlg QRKD8tuE63XvbMbkGJTXcKSArjIlVe1TgTvOA= 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=RxbFEV7zzUwXd+2E3JU056gUgx8tNKpJUrzF5DVEX08=; b=Bv5KewGEK0gzm+DkhpkZ4L/s+74qlNvPG7FqC13k7BNFOv3CFVdYc2giOPPqGqsiJt E7/3aq8UYJLt8eZtmKWg8un5rNMJ86Cz0wJjwBbaBvzZh7zbMTggDuDEkasKHquMmzS4 LUVG0uWDYAVfxt1UinJSR1J+SLmhOKL+Y14a5gbB9XhGdyRnGsVyLDu9Vri7rz+X+wKt v2nZ1KnooZk1e52Y7naqOYDLIQa66CagUxQZz0XkIYRDiS+UfFZ6UelXICRXEXmFBWkf CRRjH4+gDzWDNXrU/CZGlt2KystJR6CAhaDnmt3lxhUZxqBvGCaNuHJz7/zZnk/4jfqM Fndw== X-Gm-Message-State: AOAM5304oecHddddgzrl/2CCnHX7UfMpFO4H1W3E5k71bHfefbWMZ6/E vfqzOUvkPo3/iJ6IuCZ64VZhT//3JH6RBhkeRz4prg== X-Google-Smtp-Source: ABdhPJwAV0ERyV6d9Q6Fk00uaZSc5XCZzkapCjBQpaVe0ELF3eMTpYlUA7g03PBARhMItOMRUdpkrxpt3cL50PzBhQQ= X-Received: by 2002:a1c:7515:: with SMTP id o21mr8270255wmc.65.1624826276948; Sun, 27 Jun 2021 13:37:56 -0700 (PDT) MIME-Version: 1.0 References: <88d7d3e323c27417d7109b8a92bf53a08ad77654.1622626660.git.jan.kiszka@siemens.com> <96039724-9a5f-dbb8-d46b-b268a0d9a8c2@ti.com> <20210607114007.GD9516@bill-the-cat> <20210611140831.GU9516@bill-the-cat> <54a75091-0499-8902-b7f5-b1f75ae0091c@siemens.com> <20210627193420.GV9516@bill-the-cat> In-Reply-To: <20210627193420.GV9516@bill-the-cat> From: Simon Glass Date: Sun, 27 Jun 2021 14:37:45 -0600 Message-ID: Subject: Re: [PATCH v2 4/5] watchdog: rti_wdt: Add support for loading firmware To: Tom Rini Cc: Jan Kiszka , Lokesh Vutla , U-Boot Mailing List , Le Jin , Bao Cheng Su , Nian Gao , Chao Zeng Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hi Tom, On Sun, 27 Jun 2021 at 13:34, Tom Rini wrote: > > On Sun, Jun 27, 2021 at 12:18:09PM -0600, Simon Glass wrote: > > Hi Jan, > > > > On Sun, 27 Jun 2021 at 12:01, Jan Kiszka wrote: > > > > > > On 26.06.21 20:29, Simon Glass wrote: > > > > Hi, > > > > > > > > On Fri, 11 Jun 2021 at 08:08, Tom Rini wrote: > > > >> > > > >> On Fri, Jun 11, 2021 at 07:14:21PM +0530, Lokesh Vutla wrote: > > > >>> Hi Tom, > > > >>> > > > >>> On 09/06/21 6:47 pm, Jan Kiszka wrote: > > > >>>> On 07.06.21 13:44, Jan Kiszka wrote: > > > >>>>> On 07.06.21 13:40, Tom Rini wrote: > > > >>>>>> On Mon, Jun 07, 2021 at 03:33:52PM +0530, Lokesh Vutla wrote: > > > >>>>>>> +Tom, > > > >>>>>>> > > > >>>>>>> Hi Tom, > > > >>>>>>> > > > >>>>>>> On 02/06/21 3:07 pm, Jan Kiszka wrote: > > > >>>>>>>> From: Jan Kiszka > > > >>>>>>>> > > > >>>>>>>> To avoid the need of extra boot scripting on AM65x for loading a > > > >>>>>>>> watchdog firmware, add the required rproc init and loading logic for the > > > >>>>>>>> first R5F core to the watchdog start handler. In case the R5F cluster is > > > >>>>>>>> in lock-step mode, also initialize the second core. The firmware itself > > > >>>>>>>> is embedded into U-Boot binary to ease access to it and ensure it is > > > >>>>>>>> properly hashed in case of secure boot. > > > >>>>>>>> > > > >>>>>>>> One possible firmware source is https://github.com/siemens/k3-rti-wdt. > > > >>>>>>>> > > > >>>>>>>> Signed-off-by: Jan Kiszka > > > >>>>>>>> --- > > > >>>>>>>> drivers/watchdog/Kconfig | 20 ++++++++++++ > > > >>>>>>>> drivers/watchdog/Makefile | 5 +++ > > > >>>>>>>> drivers/watchdog/rti_wdt.c | 58 ++++++++++++++++++++++++++++++++++- > > > >>>>>>>> drivers/watchdog/rti_wdt_fw.S | 20 ++++++++++++ > > > >>>>>>>> 4 files changed, 102 insertions(+), 1 deletion(-) > > > >>>>>>>> create mode 100644 drivers/watchdog/rti_wdt_fw.S > > > >>>>>>>> > > > >>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > > >>>>>>>> index f0ff2612a6..1a1fddfe9f 100644 > > > >>>>>>>> --- a/drivers/watchdog/Kconfig > > > >>>>>>>> +++ b/drivers/watchdog/Kconfig > > > >>>>>>>> @@ -209,6 +209,26 @@ config WDT_K3_RTI > > > >>>>>>>> Say Y here if you want to include support for the K3 watchdog > > > >>>>>>>> timer (RTI module) available in the K3 generation of processors. > > > >>>>>>>> > > > >>>>>>>> +if WDT_K3_RTI > > > >>>>>>>> + > > > >>>>>>>> +config WDT_K3_RTI_LOAD_FW > > > >>>>>>>> + bool "Load watchdog firmware" > > > >>>>>>>> + depends on REMOTEPROC > > > >>>>>>>> + help > > > >>>>>>>> + Automatically load the specified firmware image into the MCU R5F > > > >>>>>>>> + core 0. On the AM65x, this firmware is supposed to handle the expiry > > > >>>>>>>> + of the watchdog timer, typically by resetting the system. > > > >>>>>>>> + > > > >>>>>>>> +config WDT_K3_RTI_FW_FILE > > > >>>>>>>> + string "Watchdog firmware image file" > > > >>>>>>>> + default "k3-rti-wdt.fw" > > > >>>>>>>> + depends on WDT_K3_RTI_LOAD_FW > > > >>>>>>>> + help > > > >>>>>>>> + Firmware image to be embedded into U-Boot and loaded on watchdog > > > >>>>>>>> + start. > > > >>>>>>> > > > >>>>>>> I need your input on this proach. Is it okay to include the linker file unders > > > >>>>>>> drivers? > > > >>>>>> > > > >>>>>> Maybe? I suppose the first thing that springs to mind is why aren't we > > > >>>>>> using binman and including this blob (which I happily see is GPLv2) > > > >>>>>> similar to how we do things with x86 for one example. > > > >>>>>> > > > >>>>> > > > >>>>> See https://www.mail-archive.com/u-boot@lists.denx.de/msg377894.html > > > >>>>> > > > >>>>> Jan > > > >>>>> > > > >>>> > > > >>>> Did this help to answer open questions? Otherwise, please let me know. > > > >>>> > > > >>>> I'd also like to avoid that his patch alone blocks 1-3 of the series > > > >>>> needless - but I would also not mind getting everything in at once. > > > >>> > > > >>> Can you provide your reviewed-by if you are okay with this approach? > > > >> > > > >> I was kind of hoping Simon would chime in here on binman usage. So, > > > >> re-re-reading the above URL, yes, fsloader wouldn't be the right choice > > > >> for watchdog firmware. But I think binman_entry_find() and related > > > >> could work, in general, for this case of "need firmware blob embedded in > > > >> to image". That said, this isn't just any firmware blob, it's the > > > >> watchdog firmware. The less reliance on other things the safer it is. > > > >> That means this would be an exception to the general firmware blob > > > >> loading rule and yeah, OK, we can do it this way. Sorry for the delay. > > > > > > > > Yes I've been a little tied up recently. But I think this should use > > > > binman. We really don't want to be building binary firmware into > > > > U-Boot itself! > > > > > > > > Also Tom says, see x86 for a load of binaries of different types and > > > > how they are accessed at runttime. This is what binman is for. > > > > > > > > > > Please take the time and study my arguments. I'm open for better > > > proposals, but they need to be concrete and addressing my points. > > > > Do you mean 'properly hashed' and 'easy access', or something else? > > What can binman not do? > > Well, as I said when ack'ing this, we're also talking about making sure > the system watchdog works. It needs to be as dead simple as possible. Another option would be for the system to fail to boot if the watchdog could not be enabled. Anyway, I'll leave it to you. Regards, Simon