[PATCH v2 2/3] watchdog: rti_wdt: Add support for loading firmware

[Jan Kiszka <jan.kiszka@siemens.com>]

On 21.07.20 07:27, Lokesh Vutla wrote:
> Hi Jan,
> 
> On 20/07/20 3:06 pm, Jan Kiszka wrote:
>> On 29.06.20 20:44, Jan Kiszka wrote:
>>> On 29.06.20 14:37, Tom Rini wrote:
>>>> On Mon, Jun 29, 2020 at 07:56:53AM +0530, Lokesh Vutla wrote:
>>>>> +Tom
>>>>>
>>>>> On 23/06/20 8:11 pm, Jan Kiszka wrote:
>>>>>> On 23.06.20 14:37, Jan Kiszka wrote:
>>>>>>> On 23.06.20 13:50, Lokesh Vutla wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23/06/20 4:45 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. The firmware itself is
>>>>>>>>> embedded into U-Boot binary.
>>>>>>>>>
>>>>>>>>> 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???? |? 3 +++
>>>>>>>>>  ?? drivers/watchdog/rti_wdt.c??? | 24 ++++++++++++++++++++++++
>>>>>>>>>  ?? drivers/watchdog/rti_wdt_fw.S | 20 ++++++++++++++++++++
>>>>>>>>>  ?? 4 files changed, 67 insertions(+)
>>>>>>>>>  ?? create mode 100644 drivers/watchdog/rti_wdt_fw.S
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>>>>> index ee99bd2af1..fd6ab9a85b 100644
>>>>>>>>> --- a/drivers/watchdog/Kconfig
>>>>>>>>> +++ b/drivers/watchdog/Kconfig
>>>>>>>>> @@ -162,6 +162,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 16bdbf4970..bf58684875 100644
>>>>>>>>> --- a/drivers/watchdog/Makefile
>>>>>>>>> +++ b/drivers/watchdog/Makefile
>>>>>>>>> @@ -28,7 +28,10 @@ obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
>>>>>>>>>  ?? obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
>>>>>>>>>  ?? obj-$(CONFIG_WDT_OMAP3) += omap_wdt.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
>>>>>>>>> +
>>>>>>>>> +$(obj)/rti_wdt_fw.o: $(shell readlink -f $(CONFIG_WDT_K3_RTI_FW_FILE))
>>>>>>>>> FORCE
>>>>>>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>>>>>>>> index ebe29c7409..38e82a6b6b 100644
>>>>>>>>> --- a/drivers/watchdog/rti_wdt.c
>>>>>>>>> +++ b/drivers/watchdog/rti_wdt.c
>>>>>>>>> @@ -14,6 +14,7 @@
>>>>>>>>>  ?? #include <power-domain.h>
>>>>>>>>>  ?? #include <wdt.h>
>>>>>>>>>  ?? #include <asm/io.h>
>>>>>>>>> +#include <remoteproc.h>
>>>>>>>>>  ?? /* Timer register set definition */
>>>>>>>>>  ?? #define RTIDWDCTRL??????? 0x90
>>>>>>>>> @@ -37,11 +38,16 @@
>>>>>>>>>  ?? #define WDT_PRELOAD_MAX??????? 0xfff
>>>>>>>>> +#define RTI_PROC_ID??????? 0
>>>>>>>>
>>>>>>>> Can we get the rproc id from DT?
>>>>>>>
>>>>>>> You mean, resolve "mcu_r5fss0_core0" to the ID? Doable.
>>>>>>>
>>>>>>
>>>>>> I'm now probing for the first instance of "ti,am654-r5f" compatible. That
>>>>>> excludes usage for the J721E for now, but that one is fine without firmware -
>>>>>> provided there is way to prevent power-down for RTI watchdog otherwise...
>>>>>
>>>>> I was more of thinking to have a DT property to reference R5-core. But anyways,
>>>>> the property is not present in kernel. I am okay with this for now.
>>>>
>>>> Note that not all properties in a DT in Linux need to be used in Linux,
>>>> this is something I _think_ there is growing understanding of.
>>>>
>>>>>>>>> +
>>>>>>>>>  ?? struct rti_wdt_priv {
>>>>>>>>>  ?????? phys_addr_t regs;
>>>>>>>>>  ?????? unsigned int clk_khz;
>>>>>>>>>  ?? };
>>>>>>>>> +extern const u32 rti_wdt_fw[];
>>>>>>>>> +extern const int rti_wdt_fw_size;
>>>>>>>>
>>>>>>>> FIT is the preferred way of packing images in U-Boot. Can you try using it?
>>>>>>>
>>>>>>> How does that work? Some example for me?
>>>>>>>
>>>>>>
>>>>>> If you happen to refer to fs-loader: That does not target OSPI, our primary
>>>>>> use
>>>>>> case.
>>>>>
>>>>> No. I was thinking to pack the image in FIT along with ATF and OPTEE like in
>>>>> k3_fit_atf.sh and register a U_BOOT_FIT_LOADABLE_HANDLER for installing the
>>>>> firmware.
>>>>>
>>>>>>
>>>>>>> What benefit would that bring? There are other users of this pattern, e.g.
>>>>>>> board/xilinx/zynqmp/pm_cfg_obj.S.
>>>>>
>>>>> I didn't know U-Boot is accepting this. Tom, is this the preferred way for
>>>>> including firmware images?
>>>>
>>>> Adding in Michal.? Why is this area doing what it's doing and not one of
>>>> the other generic ways to handle this problem?? Thanks!
>>>>
>>>
>>> So, what are the recommended "generic ways" for these use cases?
>>>
>>> Is there a way to stick own stuff into u-boot.itb and get that provided to
>>> some handler of U-Boot proper? Seems unlikely. The pattern I found pushes the
>>> firmware into the kernel fit image - which is a clear no-go for the watchdog
>>> use case because it shall monitor the booting of that very same image
>>> (software update case).
>>>
>>> Jan
>>>
>>
>> Ping on this. I'd like to move forward with this series.
> 
> If you want remotecore to be loaded early, then my preference would be to pack
> Firmware inside FIT image(along with A72 SPL, ATF or A72 U-Boot) as loadable and
> register a handle to load and start the rproc in SPL. If it can be delayed, I
> prefer to do at U-Boot prompt.
> 

So far, we do not need it in SPL as we have no procedure to update 
U-Boot proper safely. Moving things to SPL would imply carrying the 
remoteproc infrastructure twice.

For loading from the prompt, we are still lacking a handy way to bundle 
the firmware with the integrity-checked U-Boot proper. Yes, technically 
I could push those 676 bytes into a fitimage, sign it, reserve a block 
in SPI flash or some other storage, load it from there and validate it. 
But that's overkill IMHO.

> 
> However, the driver can be merged separately. If you can re post the driver part
> I can pull it in soon.

You could cherry-pick patch 1 and 3 for that already, they still apply.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux