| Message ID | 88d7d3e323c27417d7109b8a92bf53a08ad77654.1622626660.git.jan.kiszka@siemens.com |
|---|---|
| State | Superseded |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | Add SIMATIC IOT2050 board support | expand |
+Tom, Hi Tom, On 02/06/21 3:07 pm, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > 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 <jan.kiszka@siemens.com> > --- > 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? > + > +endif > + > config WDT_SANDBOX > bool "Enable Watchdog Timer support for Sandbox" > depends on SANDBOX && WDT > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 5c7ef593fe..5016ee4708 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -33,7 +33,12 @@ obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o > obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o > obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o > obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o > +obj-$(CONFIG_WDT_K3_RTI_LOAD_FW) += rti_wdt_fw.o > obj-$(CONFIG_WDT_SP805) += sp805_wdt.o > obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o > obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o > obj-$(CONFIG_WDT_XILINX) += xilinx_wwdt.o > + [...snip..] > timer_margin = timeout_ms * priv->clk_khz / 1000; > timer_margin >>= WDT_PRELOAD_SHIFT; > if (timer_margin > WDT_PRELOAD_MAX) > diff --git a/drivers/watchdog/rti_wdt_fw.S b/drivers/watchdog/rti_wdt_fw.S > new file mode 100644 > index 0000000000..78d99ff9f2 > --- /dev/null > +++ b/drivers/watchdog/rti_wdt_fw.S Isn't this usecase specific? IMHO, drivers might not be the right place. Did I misunderstand something? Thanks and regards, Lokesh > @@ -0,0 +1,20 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) Siemens AG, 2020 > + * > + * Authors: > + * Jan Kiszka <jan.kiszka@siemens.com> > + */ > + > +.section .rodata > + > +.global rti_wdt_fw > +.global rti_wdt_fw_size > + > +rti_wdt_fw: > +.align 4 > +.incbin CONFIG_WDT_K3_RTI_FW_FILE > +rti_wdt_fw_end: > + > +rti_wdt_fw_size: > +.int rti_wdt_fw_end - rti_wdt_fw >
On 07.06.21 12:03, Lokesh Vutla wrote: > +Tom, > > Hi Tom, > > On 02/06/21 3:07 pm, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> 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 <jan.kiszka@siemens.com> >> --- >> 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? > >> + >> +endif >> + >> config WDT_SANDBOX >> bool "Enable Watchdog Timer support for Sandbox" >> depends on SANDBOX && WDT >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> index 5c7ef593fe..5016ee4708 100644 >> --- a/drivers/watchdog/Makefile >> +++ b/drivers/watchdog/Makefile >> @@ -33,7 +33,12 @@ obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o >> obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o >> obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o >> obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o >> +obj-$(CONFIG_WDT_K3_RTI_LOAD_FW) += rti_wdt_fw.o >> obj-$(CONFIG_WDT_SP805) += sp805_wdt.o >> obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o >> obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o >> obj-$(CONFIG_WDT_XILINX) += xilinx_wwdt.o >> + > > [...snip..] > >> timer_margin = timeout_ms * priv->clk_khz / 1000; >> timer_margin >>= WDT_PRELOAD_SHIFT; >> if (timer_margin > WDT_PRELOAD_MAX) >> diff --git a/drivers/watchdog/rti_wdt_fw.S b/drivers/watchdog/rti_wdt_fw.S >> new file mode 100644 >> index 0000000000..78d99ff9f2 >> --- /dev/null >> +++ b/drivers/watchdog/rti_wdt_fw.S > > Isn't this usecase specific? IMHO, drivers might not be the right place. Did I > misunderstand something? > It's specific to this driver - on a subset of supported hardware platforms, namely AM65x SR1.0. Jan
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 <jan.kiszka@siemens.com> > > > > 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 <jan.kiszka@siemens.com> > > --- > > 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.
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 <jan.kiszka@siemens.com> >>> >>> 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 <jan.kiszka@siemens.com> >>> --- >>> 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
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 <jan.kiszka@siemens.com> >>>> >>>> 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 <jan.kiszka@siemens.com> >>>> --- >>>> 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. Thanks, Jan
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 <jan.kiszka@siemens.com> >>>>> >>>>> 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 <jan.kiszka@siemens.com> >>>>> --- >>>>> 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? Thanks and regards, Lokesh > > Thanks, > Jan >
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 <jan.kiszka@siemens.com> > >>>>> > >>>>> 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 <jan.kiszka@siemens.com> > >>>>> --- > >>>>> 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. Reviewed-by: Tom Rini <trini@konsulko.com>
Hi, On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > > >>>>> > > >>>>> 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 <jan.kiszka@siemens.com> > > >>>>> --- > > >>>>> 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. > > Reviewed-by: Tom Rini <trini@konsulko.com> > > -- > Tom Regards, Simon
On 26.06.21 20:29, Simon Glass wrote: > Hi, > > On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> >>>>>>>> >>>>>>>> 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 <jan.kiszka@siemens.com> >>>>>>>> --- >>>>>>>> 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. Thanks, Jan
Hi Jan, On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 26.06.21 20:29, Simon Glass wrote: > > Hi, > > > > On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > >>>>>>>> > >>>>>>>> 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 <jan.kiszka@siemens.com> > >>>>>>>> --- > >>>>>>>> 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? Regards, Simon
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 <jan.kiszka@siemens.com> wrote: > > > > On 26.06.21 20:29, Simon Glass wrote: > > > Hi, > > > > > > On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > > >>>>>>>> > > >>>>>>>> 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 <jan.kiszka@siemens.com> > > >>>>>>>> --- > > >>>>>>>> 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.
Hi Tom, On Sun, 27 Jun 2021 at 13:34, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> wrote: > > > > > > On 26.06.21 20:29, Simon Glass wrote: > > > > Hi, > > > > > > > > On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > > > >>>>>>>> > > > >>>>>>>> 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 <jan.kiszka@siemens.com> > > > >>>>>>>> --- > > > >>>>>>>> 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
On 27.06.21 20:18, Simon Glass wrote: > Hi Jan, > > On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> On 26.06.21 20:29, Simon Glass wrote: >>> Hi, >>> >>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> >>>>>>>>>> >>>>>>>>>> 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 <jan.kiszka@siemens.com> >>>>>>>>>> --- >>>>>>>>>> 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? Binman itself can stick things into binary images. But that is at most half of the tasks needed here. I would need concrete guidance how to - access that binary from u-boot proper in a reasonably simple way - make sure the binary can be signed and the signature is evaluated before using it Jan
Hi Jan, On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 27.06.21 20:18, Simon Glass wrote: > > Hi Jan, > > > > On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> > >> On 26.06.21 20:29, Simon Glass wrote: > >>> Hi, > >>> > >>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > >>>>>>>>>> > >>>>>>>>>> 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 <jan.kiszka@siemens.com> > >>>>>>>>>> --- > >>>>>>>>>> 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? > > Binman itself can stick things into binary images. But that is at most > half of the tasks needed here. I would need concrete guidance how to > > - access that binary from u-boot proper in a reasonably simple way I thought you wanted to access it from SPL. For that you would use linker symbols. See 'Access to binman entry offsets at run time (symbols)' in the binman docs for that. But for U-Boot proper, the section is 'Access to binman entry offsets at run time (fdt)'. There is no mention of the runtime library that now exists (binman.h) so I will send a patch for that. But basically you call binman_entry_find("name", &entry) and it returns the offset and size for you. > - make sure the binary can be signed and the signature is evaluated > before using it Do you mean signed or hashed? I think you mean hashed. If you trust the U-Boot binary then presumably you can put the firmware in a similar place do you can trust that as well. After all, you seem happy enough to link it into U-Boot. If not, you can ask binman to add a hash: my-firmware { type = "blob"; hash { algo = "sha256"; }; }; Then you can add support for that to some sort of helper function in binman.c, e.g.: ofnode node, hashnode; const u8 *hash; int size; node = binman_section_find_node("name"); if (!ofnode_valid(node)) ...return error hashnode = ofnode_read_prop(node, "hash"); if (!ofnode_valid(hashnode)) ...return error hash = ofnode_read_prop(hashnode, "value", &size); /* Hash the firmware...need to read it from flash into fwdata/fwlen using binman_entry_find() ...then: */ u8 digest[SHA256_SUM_LEN]; ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); if (ret) return log_msg_ret("hash", ret); /* compare the hashes */ if (size != sizeof(digest) || memcmp(hash, digest)) return log_msg_ret("cmp", ret); Regards, Simon
On 05.07.21 17:29, Simon Glass wrote: > Hi Jan, > > On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> On 27.06.21 20:18, Simon Glass wrote: >>> Hi Jan, >>> >>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> >>>> On 26.06.21 20:29, Simon Glass wrote: >>>>> Hi, >>>>> >>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>> >>>>>>>>>>>> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>> --- >>>>>>>>>>>> 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? >> >> Binman itself can stick things into binary images. But that is at most >> half of the tasks needed here. I would need concrete guidance how to >> >> - access that binary from u-boot proper in a reasonably simple way > > I thought you wanted to access it from SPL. For that you would use > linker symbols. See 'Access to binman entry offsets at run time > (symbols)' in the binman docs for that. > > But for U-Boot proper, the section is 'Access to binman entry offsets > at run time (fdt)'. There is no mention of the runtime library that > now exists (binman.h) so I will send a patch for that. But basically > you call binman_entry_find("name", &entry) and it returns the offset > and size for you. > >> - make sure the binary can be signed and the signature is evaluated >> before using it > > Do you mean signed or hashed? I think you mean hashed. If you trust > the U-Boot binary then presumably you can put the firmware in a > similar place do you can trust that as well. After all, you seem happy > enough to link it into U-Boot. > > If not, you can ask binman to add a hash: > > my-firmware { > type = "blob"; > hash { > algo = "sha256"; > }; > }; > > Then you can add support for that to some sort of helper function in > binman.c, e.g.: > > ofnode node, hashnode; > const u8 *hash; > int size; > > node = binman_section_find_node("name"); > if (!ofnode_valid(node)) > ...return error > hashnode = ofnode_read_prop(node, "hash"); > if (!ofnode_valid(hashnode)) > ...return error > hash = ofnode_read_prop(hashnode, "value", &size); > > /* Hash the firmware...need to read it from flash into fwdata/fwlen > using binman_entry_find() ...then: */ > u8 digest[SHA256_SUM_LEN]; > ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); > if (ret) > return log_msg_ret("hash", ret); > > /* compare the hashes */ > if (size != sizeof(digest) || memcmp(hash, digest)) > return log_msg_ret("cmp", ret); > Yes, it will likely be a hash that will be signed, and all that will be checked by SPL when loading proper. The infrastructure for that should be there, just not yet plugged for the way of loading things like we do (one of my todos). Obviously, what would be simplest for that is to have the watchdog blob embedded, rather than separately hashed, as that would Just Work. I didn't fully grasp yet what you propose, but it seems right now it will complicate things. I'm not saying it will make it impossible, just harder. Jan
Hi Jan, On Wed, 14 Jul 2021 at 03:53, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 05.07.21 17:29, Simon Glass wrote: > > Hi Jan, > > > > On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> > >> On 27.06.21 20:18, Simon Glass wrote: > >>> Hi Jan, > >>> > >>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>>> > >>>> On 26.06.21 20:29, Simon Glass wrote: > >>>>> Hi, > >>>>> > >>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > >>>>>>>>>>>> > >>>>>>>>>>>> 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 <jan.kiszka@siemens.com> > >>>>>>>>>>>> --- > >>>>>>>>>>>> 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? > >> > >> Binman itself can stick things into binary images. But that is at most > >> half of the tasks needed here. I would need concrete guidance how to > >> > >> - access that binary from u-boot proper in a reasonably simple way > > > > I thought you wanted to access it from SPL. For that you would use > > linker symbols. See 'Access to binman entry offsets at run time > > (symbols)' in the binman docs for that. > > > > But for U-Boot proper, the section is 'Access to binman entry offsets > > at run time (fdt)'. There is no mention of the runtime library that > > now exists (binman.h) so I will send a patch for that. But basically > > you call binman_entry_find("name", &entry) and it returns the offset > > and size for you. > > > >> - make sure the binary can be signed and the signature is evaluated > >> before using it > > > > Do you mean signed or hashed? I think you mean hashed. If you trust > > the U-Boot binary then presumably you can put the firmware in a > > similar place do you can trust that as well. After all, you seem happy > > enough to link it into U-Boot. > > > > If not, you can ask binman to add a hash: > > > > my-firmware { > > type = "blob"; > > hash { > > algo = "sha256"; > > }; > > }; > > > > Then you can add support for that to some sort of helper function in > > binman.c, e.g.: > > > > ofnode node, hashnode; > > const u8 *hash; > > int size; > > > > node = binman_section_find_node("name"); > > if (!ofnode_valid(node)) > > ...return error > > hashnode = ofnode_read_prop(node, "hash"); > > if (!ofnode_valid(hashnode)) > > ...return error > > hash = ofnode_read_prop(hashnode, "value", &size); > > > > /* Hash the firmware...need to read it from flash into fwdata/fwlen > > using binman_entry_find() ...then: */ > > u8 digest[SHA256_SUM_LEN]; > > ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); > > if (ret) > > return log_msg_ret("hash", ret); > > > > /* compare the hashes */ > > if (size != sizeof(digest) || memcmp(hash, digest)) > > return log_msg_ret("cmp", ret); > > > > Yes, it will likely be a hash that will be signed, and all that will be > checked by SPL when loading proper. The infrastructure for that should > be there, just not yet plugged for the way of loading things like we do > (one of my todos). > > Obviously, what would be simplest for that is to have the watchdog blob > embedded, rather than separately hashed, as that would Just Work. I > didn't fully grasp yet what you propose, but it seems right now it will > complicate things. I'm not saying it will make it impossible, just harder. I'm not even sure that is true. In binman, add the entry: watchdog-firmware { type = "blob"; filename = "..."; }; In the C code, declare a symbol that will get the image position of the entry: binman_sym_declare(unsigned long, watchdog_firmware, image_pos); then read that value when you need it: int check_it(..) { ulong pos = binman_sym(ulong, watchdog_firmware, image_pos); // read from flash offset @pos into a buffer // check the hash }; This is one of the key features of binman. It seems a shame to bring in linker magic instead, when it is already there. But my point was really that if you combine the watchdog firmware into the image with binman, you should be able to avoid verifying it, or at least rely on the same mechanism you use to verify U-Boot. Feel free to come along and discuss this at one of the contributor calls if that would be easier. Regards, Simon
On 14.07.21 16:15, Simon Glass wrote: > Hi Jan, > > On Wed, 14 Jul 2021 at 03:53, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> On 05.07.21 17:29, Simon Glass wrote: >>> Hi Jan, >>> >>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> >>>> On 27.06.21 20:18, Simon Glass wrote: >>>>> Hi Jan, >>>>> >>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>> >>>>>> On 26.06.21 20:29, Simon Glass wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>>>> >>>>>>>>>>>>>> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> 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? >>>> >>>> Binman itself can stick things into binary images. But that is at most >>>> half of the tasks needed here. I would need concrete guidance how to >>>> >>>> - access that binary from u-boot proper in a reasonably simple way >>> >>> I thought you wanted to access it from SPL. For that you would use >>> linker symbols. See 'Access to binman entry offsets at run time >>> (symbols)' in the binman docs for that. >>> >>> But for U-Boot proper, the section is 'Access to binman entry offsets >>> at run time (fdt)'. There is no mention of the runtime library that >>> now exists (binman.h) so I will send a patch for that. But basically >>> you call binman_entry_find("name", &entry) and it returns the offset >>> and size for you. >>> >>>> - make sure the binary can be signed and the signature is evaluated >>>> before using it >>> >>> Do you mean signed or hashed? I think you mean hashed. If you trust >>> the U-Boot binary then presumably you can put the firmware in a >>> similar place do you can trust that as well. After all, you seem happy >>> enough to link it into U-Boot. >>> >>> If not, you can ask binman to add a hash: >>> >>> my-firmware { >>> type = "blob"; >>> hash { >>> algo = "sha256"; >>> }; >>> }; >>> >>> Then you can add support for that to some sort of helper function in >>> binman.c, e.g.: >>> >>> ofnode node, hashnode; >>> const u8 *hash; >>> int size; >>> >>> node = binman_section_find_node("name"); >>> if (!ofnode_valid(node)) >>> ...return error >>> hashnode = ofnode_read_prop(node, "hash"); >>> if (!ofnode_valid(hashnode)) >>> ...return error >>> hash = ofnode_read_prop(hashnode, "value", &size); >>> >>> /* Hash the firmware...need to read it from flash into fwdata/fwlen >>> using binman_entry_find() ...then: */ >>> u8 digest[SHA256_SUM_LEN]; >>> ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); >>> if (ret) >>> return log_msg_ret("hash", ret); >>> >>> /* compare the hashes */ >>> if (size != sizeof(digest) || memcmp(hash, digest)) >>> return log_msg_ret("cmp", ret); >>> >> >> Yes, it will likely be a hash that will be signed, and all that will be >> checked by SPL when loading proper. The infrastructure for that should >> be there, just not yet plugged for the way of loading things like we do >> (one of my todos). >> >> Obviously, what would be simplest for that is to have the watchdog blob >> embedded, rather than separately hashed, as that would Just Work. I >> didn't fully grasp yet what you propose, but it seems right now it will >> complicate things. I'm not saying it will make it impossible, just harder. > > I'm not even sure that is true. In binman, add the entry: > > watchdog-firmware { > type = "blob"; > filename = "..."; > }; > > In the C code, declare a symbol that will get the image position of the entry: > > binman_sym_declare(unsigned long, watchdog_firmware, image_pos); > > then read that value when you need it: > > int check_it(..) > { > ulong pos = binman_sym(ulong, watchdog_firmware, image_pos); > > // read from flash offset @pos into a buffer > > // check the hash > }; This function is the extra boilerplate code that is not needed when the blob is simply part of the U-Boot binary that is loaded and checked by SPL. The worst part of this: It requires special handling of different storage media. We currently only have OSPI for out board, but you may also imagine versions that load U-Boot from filesystems (the TI EVM does that). So this is the extra complexity without extra value I'm always talking about. But I'm happy to take concrete suggestions where to add the firmware into our board and where to add the necessary code in a simple way. What we do so far: U-Boot proper and DTBs go into a fit image that SPL will load (and later also validate). It would be simple to do diff --git a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi index 96647719df..d3242af70c 100644 --- a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi +++ b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi @@ -60,6 +60,11 @@ filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb"; }; }; + + k3-rti-firmware { + type = "blob"; + filename = CONFIG_WDT_K3_RTI_FW_FILE; + }; }; configurations { in [1] (used by [2]). But now: How do I communicate that blob address from SPL to U-Boot proper so that I can skip the extra medium-specific loading part and also reuse the fit image validation of SPL? If there is a simple way for that, I could indeed switch the model. Jan [1] https://patchwork.ozlabs.org/project/uboot/patch/f7cf19b1fd2ce52d74c25e590aee452d30a6f1e4.1622626660.git.jan.kiszka@siemens.com/ [2] https://patchwork.ozlabs.org/project/uboot/patch/a83aa9023ea9c49cf1efd4a72d85bedb1e388478.1623440557.git.jan.kiszka@siemens.com/
On 20.07.21 14:57, Jan Kiszka wrote: > On 14.07.21 16:15, Simon Glass wrote: >> Hi Jan, >> >> On Wed, 14 Jul 2021 at 03:53, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> >>> On 05.07.21 17:29, Simon Glass wrote: >>>> Hi Jan, >>>> >>>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>> >>>>> On 27.06.21 20:18, Simon Glass wrote: >>>>>> Hi Jan, >>>>>> >>>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>>> >>>>>>> On 26.06.21 20:29, Simon Glass wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 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 <jan.kiszka@siemens.com> >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> 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? >>>>> >>>>> Binman itself can stick things into binary images. But that is at most >>>>> half of the tasks needed here. I would need concrete guidance how to >>>>> >>>>> - access that binary from u-boot proper in a reasonably simple way >>>> >>>> I thought you wanted to access it from SPL. For that you would use >>>> linker symbols. See 'Access to binman entry offsets at run time >>>> (symbols)' in the binman docs for that. >>>> >>>> But for U-Boot proper, the section is 'Access to binman entry offsets >>>> at run time (fdt)'. There is no mention of the runtime library that >>>> now exists (binman.h) so I will send a patch for that. But basically >>>> you call binman_entry_find("name", &entry) and it returns the offset >>>> and size for you. >>>> >>>>> - make sure the binary can be signed and the signature is evaluated >>>>> before using it >>>> >>>> Do you mean signed or hashed? I think you mean hashed. If you trust >>>> the U-Boot binary then presumably you can put the firmware in a >>>> similar place do you can trust that as well. After all, you seem happy >>>> enough to link it into U-Boot. >>>> >>>> If not, you can ask binman to add a hash: >>>> >>>> my-firmware { >>>> type = "blob"; >>>> hash { >>>> algo = "sha256"; >>>> }; >>>> }; >>>> >>>> Then you can add support for that to some sort of helper function in >>>> binman.c, e.g.: >>>> >>>> ofnode node, hashnode; >>>> const u8 *hash; >>>> int size; >>>> >>>> node = binman_section_find_node("name"); >>>> if (!ofnode_valid(node)) >>>> ...return error >>>> hashnode = ofnode_read_prop(node, "hash"); >>>> if (!ofnode_valid(hashnode)) >>>> ...return error >>>> hash = ofnode_read_prop(hashnode, "value", &size); >>>> >>>> /* Hash the firmware...need to read it from flash into fwdata/fwlen >>>> using binman_entry_find() ...then: */ >>>> u8 digest[SHA256_SUM_LEN]; >>>> ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); >>>> if (ret) >>>> return log_msg_ret("hash", ret); >>>> >>>> /* compare the hashes */ >>>> if (size != sizeof(digest) || memcmp(hash, digest)) >>>> return log_msg_ret("cmp", ret); >>>> >>> >>> Yes, it will likely be a hash that will be signed, and all that will be >>> checked by SPL when loading proper. The infrastructure for that should >>> be there, just not yet plugged for the way of loading things like we do >>> (one of my todos). >>> >>> Obviously, what would be simplest for that is to have the watchdog blob >>> embedded, rather than separately hashed, as that would Just Work. I >>> didn't fully grasp yet what you propose, but it seems right now it will >>> complicate things. I'm not saying it will make it impossible, just harder. >> >> I'm not even sure that is true. In binman, add the entry: >> >> watchdog-firmware { >> type = "blob"; >> filename = "..."; >> }; >> >> In the C code, declare a symbol that will get the image position of the entry: >> >> binman_sym_declare(unsigned long, watchdog_firmware, image_pos); >> >> then read that value when you need it: >> >> int check_it(..) >> { >> ulong pos = binman_sym(ulong, watchdog_firmware, image_pos); >> >> // read from flash offset @pos into a buffer >> >> // check the hash >> }; > > This function is the extra boilerplate code that is not needed when the > blob is simply part of the U-Boot binary that is loaded and checked by > SPL. The worst part of this: It requires special handling of different > storage media. We currently only have OSPI for out board, but you may > also imagine versions that load U-Boot from filesystems (the TI EVM does > that). So this is the extra complexity without extra value I'm always > talking about. > > But I'm happy to take concrete suggestions where to add the firmware > into our board and where to add the necessary code in a simple way. > > What we do so far: U-Boot proper and DTBs go into a fit image that SPL > will load (and later also validate). It would be simple to do > > diff --git a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > index 96647719df..d3242af70c 100644 > --- a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > +++ b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > @@ -60,6 +60,11 @@ > filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb"; > }; > }; > + > + k3-rti-firmware { > + type = "blob"; > + filename = CONFIG_WDT_K3_RTI_FW_FILE; > + }; > }; > > configurations { > > in [1] (used by [2]). > > But now: How do I communicate that blob address from SPL to U-Boot > proper so that I can skip the extra medium-specific loading part and > also reuse the fit image validation of SPL? If there is a simple way for > that, I could indeed switch the model. > OK, I think I found a trick (outside of binman): Making the firmware an additional loadable in the u-boot fit image, and then picking its location up from /fit-images/k3-rti-wdt-firmware in rti_wdt_load_fw(). That should be nicer then object embedded - without requiring the complexity of separate image loading and validating. Jan
Hi Jan, On Tue, 20 Jul 2021 at 06:58, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 14.07.21 16:15, Simon Glass wrote: > > Hi Jan, > > > > On Wed, 14 Jul 2021 at 03:53, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> > >> On 05.07.21 17:29, Simon Glass wrote: > >>> Hi Jan, > >>> > >>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>>> > >>>> On 27.06.21 20:18, Simon Glass wrote: > >>>>> Hi Jan, > >>>>> > >>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>>>>> > >>>>>> On 26.06.21 20:29, Simon Glass wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <trini@konsulko.com> 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 <jan.kiszka@siemens.com> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> 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 <jan.kiszka@siemens.com> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> 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? > >>>> > >>>> Binman itself can stick things into binary images. But that is at most > >>>> half of the tasks needed here. I would need concrete guidance how to > >>>> > >>>> - access that binary from u-boot proper in a reasonably simple way > >>> > >>> I thought you wanted to access it from SPL. For that you would use > >>> linker symbols. See 'Access to binman entry offsets at run time > >>> (symbols)' in the binman docs for that. > >>> > >>> But for U-Boot proper, the section is 'Access to binman entry offsets > >>> at run time (fdt)'. There is no mention of the runtime library that > >>> now exists (binman.h) so I will send a patch for that. But basically > >>> you call binman_entry_find("name", &entry) and it returns the offset > >>> and size for you. > >>> > >>>> - make sure the binary can be signed and the signature is evaluated > >>>> before using it > >>> > >>> Do you mean signed or hashed? I think you mean hashed. If you trust > >>> the U-Boot binary then presumably you can put the firmware in a > >>> similar place do you can trust that as well. After all, you seem happy > >>> enough to link it into U-Boot. > >>> > >>> If not, you can ask binman to add a hash: > >>> > >>> my-firmware { > >>> type = "blob"; > >>> hash { > >>> algo = "sha256"; > >>> }; > >>> }; > >>> > >>> Then you can add support for that to some sort of helper function in > >>> binman.c, e.g.: > >>> > >>> ofnode node, hashnode; > >>> const u8 *hash; > >>> int size; > >>> > >>> node = binman_section_find_node("name"); > >>> if (!ofnode_valid(node)) > >>> ...return error > >>> hashnode = ofnode_read_prop(node, "hash"); > >>> if (!ofnode_valid(hashnode)) > >>> ...return error > >>> hash = ofnode_read_prop(hashnode, "value", &size); > >>> > >>> /* Hash the firmware...need to read it from flash into fwdata/fwlen > >>> using binman_entry_find() ...then: */ > >>> u8 digest[SHA256_SUM_LEN]; > >>> ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); > >>> if (ret) > >>> return log_msg_ret("hash", ret); > >>> > >>> /* compare the hashes */ > >>> if (size != sizeof(digest) || memcmp(hash, digest)) > >>> return log_msg_ret("cmp", ret); > >>> > >> > >> Yes, it will likely be a hash that will be signed, and all that will be > >> checked by SPL when loading proper. The infrastructure for that should > >> be there, just not yet plugged for the way of loading things like we do > >> (one of my todos). > >> > >> Obviously, what would be simplest for that is to have the watchdog blob > >> embedded, rather than separately hashed, as that would Just Work. I > >> didn't fully grasp yet what you propose, but it seems right now it will > >> complicate things. I'm not saying it will make it impossible, just harder. > > > > I'm not even sure that is true. In binman, add the entry: > > > > watchdog-firmware { > > type = "blob"; > > filename = "..."; > > }; > > > > In the C code, declare a symbol that will get the image position of the entry: > > > > binman_sym_declare(unsigned long, watchdog_firmware, image_pos); > > > > then read that value when you need it: > > > > int check_it(..) > > { > > ulong pos = binman_sym(ulong, watchdog_firmware, image_pos); > > > > // read from flash offset @pos into a buffer > > > > // check the hash > > }; > > This function is the extra boilerplate code that is not needed when the > blob is simply part of the U-Boot binary that is loaded and checked by > SPL. The worst part of this: It requires special handling of different > storage media. We currently only have OSPI for out board, but you may > also imagine versions that load U-Boot from filesystems (the TI EVM does > that). So this is the extra complexity without extra value I'm always > talking about. Well you should load the whole thing (including U-Boot and the watchdog-firmware) in one go, i.e. put everything into a section called u-boot and SPL should load everything. My 'read from flash offset' could just be 'locate flash offset and determine where it has been loaded already'. But yes if you have to load it separately it is more complicated. Also that defeats the original goal I think, since the idea was to enable the watchdog fairly early. > > But I'm happy to take concrete suggestions where to add the firmware > into our board and where to add the necessary code in a simple way. > > What we do so far: U-Boot proper and DTBs go into a fit image that SPL > will load (and later also validate). It would be simple to do > > diff --git a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > index 96647719df..d3242af70c 100644 > --- a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > +++ b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > @@ -60,6 +60,11 @@ > filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb"; > }; > }; > + > + k3-rti-firmware { > + type = "blob"; > + filename = CONFIG_WDT_K3_RTI_FW_FILE; > + }; > }; > > configurations { > > in [1] (used by [2]). > > But now: How do I communicate that blob address from SPL to U-Boot > proper so that I can skip the extra medium-specific loading part and > also reuse the fit image validation of SPL? If there is a simple way for > that, I could indeed switch the model. The easiest way might be to put everything in a section: u-boot { // name this so that SPL locates the image_pos/size symbols type = "section"; u-boot { }; k3-rti-firmware { }; }; But I suppose you could also do things with a special SPL loader if necessary. Regards, Simon
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. + +endif + config WDT_SANDBOX bool "Enable Watchdog Timer support for Sandbox" depends on SANDBOX && WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c7ef593fe..5016ee4708 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -33,7 +33,12 @@ obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o +obj-$(CONFIG_WDT_K3_RTI_LOAD_FW) += rti_wdt_fw.o obj-$(CONFIG_WDT_SP805) += sp805_wdt.o obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o obj-$(CONFIG_WDT_XILINX) += xilinx_wwdt.o + +ifeq ($(CONFIG_WDT_K3_RTI_LOAD_FW),y) +$(obj)/rti_wdt_fw.o: $(shell readlink -f $(CONFIG_WDT_K3_RTI_FW_FILE)) FORCE +endif diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 8335b20ae8..97daf40145 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -11,9 +11,11 @@ #include <common.h> #include <clk.h> #include <dm.h> +#include <dm/device_compat.h> #include <power-domain.h> #include <wdt.h> #include <asm/io.h> +#include <remoteproc.h> /* Timer register set definition */ #define RTIDWDCTRL 0x90 @@ -42,15 +44,69 @@ struct rti_wdt_priv { unsigned int clk_khz; }; +extern const u32 rti_wdt_fw[]; +extern const int rti_wdt_fw_size; + static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) { +#ifdef CONFIG_WDT_K3_RTI_LOAD_FW + struct udevice *rproc_dev; + int primary_core, ret; + u32 cluster_mode; + ofnode node; +#endif struct rti_wdt_priv *priv = dev_get_priv(dev); u32 timer_margin; - int ret; if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) return -EBUSY; +#ifdef CONFIG_WDT_K3_RTI_LOAD_FW + node = ofnode_by_compatible(ofnode_null(), "ti,am654-r5fss"); + if (!ofnode_valid(node)) { + dt_error: + dev_err(dev, "No compatible firmware target processor found\n"); + return -ENODEV; + } + + ret = ofnode_read_u32(node, "ti,cluster-mode", &cluster_mode); + if (ret) + cluster_mode = 1; + + node = ofnode_by_compatible(node, "ti,am654-r5f"); + if (!ofnode_valid(node)) + goto dt_error; + + ret = uclass_get_device_by_ofnode(UCLASS_REMOTEPROC, node, &rproc_dev); + if (ret) + return ret; + + primary_core = dev_seq(rproc_dev); + + ret = rproc_dev_init(primary_core); + if (ret) { + fw_error: + dev_err(dev, "Failed to load watchdog firmware into remote processor %d\n", + primary_core); + return ret; + } + + if (cluster_mode == 1) { + ret = rproc_dev_init(primary_core + 1); + if (ret) + goto fw_error; + } + + ret = rproc_load(primary_core, (ulong)rti_wdt_fw, + rti_wdt_fw_size); + if (ret) + goto fw_error; + + ret = rproc_start(primary_core); + if (ret) + goto fw_error; +#endif + timer_margin = timeout_ms * priv->clk_khz / 1000; timer_margin >>= WDT_PRELOAD_SHIFT; if (timer_margin > WDT_PRELOAD_MAX) diff --git a/drivers/watchdog/rti_wdt_fw.S b/drivers/watchdog/rti_wdt_fw.S new file mode 100644 index 0000000000..78d99ff9f2 --- /dev/null +++ b/drivers/watchdog/rti_wdt_fw.S @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) Siemens AG, 2020 + * + * Authors: + * Jan Kiszka <jan.kiszka@siemens.com> + */ + +.section .rodata + +.global rti_wdt_fw +.global rti_wdt_fw_size + +rti_wdt_fw: +.align 4 +.incbin CONFIG_WDT_K3_RTI_FW_FILE +rti_wdt_fw_end: + +rti_wdt_fw_size: +.int rti_wdt_fw_end - rti_wdt_fw