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 <[email protected]> wrote: >>> >>> On 05.07.21 17:29, Simon Glass wrote: >>>> Hi Jan, >>>> >>>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka <[email protected]> wrote: >>>>> >>>>> On 27.06.21 20:18, Simon Glass wrote: >>>>>> Hi Jan, >>>>>> >>>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka <[email protected]> wrote: >>>>>>> >>>>>>> On 26.06.21 20:29, Simon Glass wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini <[email protected]> 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 <[email protected]> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 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 <[email protected]> >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> 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/[email protected]/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 -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
