[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores
Keerthy
j-keerthy at ti.com
Thu Jan 23 17:44:17 CET 2020
On 23/01/20 6:54 pm, Andrew F. Davis wrote:
> On 1/22/20 11:10 PM, Keerthy wrote:
>>
>>
>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>
>>>>
>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>
>>>>>> In order to start the core, there should be a file with path
>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>> of respective boot mode.
>>>>>>
>>>>>> Signed-off-by: Keerthy <j-keerthy at ti.com>
>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg at ti.com>
>>>>>> ---
>>>>>> arch/arm/mach-k3/common.c | 84
>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>> arch/arm/mach-k3/common.h | 2 +
>>>>>> arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>> 3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>> @@ -16,6 +16,10 @@
>>>>>> #include <asm/arch/sys_proto.h>
>>>>>> #include <asm/hardware.h>
>>>>>> #include <asm/io.h>
>>>>>> +#include <fs_loader.h>
>>>>>> +#include <fs.h>
>>>>>> +#include <env.h>
>>>>>> +#include <elf.h>
>>>>>> struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>> {
>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>> #endif
>>>>>> #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>> +
>>>>>> +void init_env(void)
>>>>>> +{
>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>> + char *part;
>>>>>> +
>>>>>> + env_init();
>>>>>> + env_load();
>>>>>> + switch (spl_boot_device()) {
>>>>>> + case BOOT_DEVICE_MMC2:
>>>>>> + part = env_get("bootpart");
>>>>>> + env_set("storage_interface", "mmc");
>>>>>> + env_set("fw_dev_part", part);
>>>>>> + break;
>>>>>> + case BOOT_DEVICE_SPI:
>>>>>> + env_set("storage_interface", "ubi");
>>>>>> + env_set("fw_ubi_mtdpart", "UBI");
>>>>>> + env_set("fw_ubi_volume", "UBI0");
>>>>>> + break;
>>>>>> + default:
>>>>>> + printf("%s from device %u not supported!\n",
>>>>>> + __func__, spl_boot_device());
>>>>>
>>>>>
>>>>> This will print for almost every boot mode..
>>>>
>>>> I can keep this under debug.
>>>>
>>>>>
>>>>>
>>>>>> + return;
>>>>>> + }
>>>>>> +#endif
>>>>>> +}
>>>>>> +
>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>>> +{
>>>>>> + struct udevice *fsdev;
>>>>>> + char *name = NULL;
>>>>>> + int size = 0;
>>>>>> +
>>>>>> + *loadaddr = 0;
>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>> + switch (spl_boot_device()) {
>>>>>> + case BOOT_DEVICE_MMC2:
>>>>>> + name = env_get(name_fw);
>>>>>> + *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>> + break;
>>>>>> + default:
>>>>>> + printf("Loading rproc fw image from device %u not
>>>>>> supported!\n",
>>>>>> + spl_boot_device());
>>>>>
>>>>>
>>>>> This whole thing seems very MMC specific, if early firmware loading is
>>>>> important it should work for all boot modes. Find a way to include
>>>>> it in
>>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>>
>>>> That was not NAKd. We are going with fs_loader approach.
>>>>
>>>
>>>
>>> When, where, link?
>>
>> I had implemented that way internally. That was rejected for multiple
>> right reasons:
>>
>
>
> I must have missed the internal reviews for this, anyway this is posted
> upstream so lets discus it here.
>
>
>> 1) SPL size would bloat based on the size of the firmware.
>
>
> SPL size would remain constant, the combined FIT (tispl.bin) would grow,
> but that is okay as DRAM is enabled at this point so we have no hard
> memory constraints.
I meant the FIT image containing the SPL will bloat.
>
>
>> 2) There are multiple cores that need to be loaded and hence adding all
>> the firmwares under a fit can be really painful.
>
>
> Bundling images is what FIT is for, are you saying the better solution
> is to hard-code each firmware starting like done here?
How many firmwares will you go on bundling. Firmwares are already kept
in file system. It is a matter of reading them from there.
>
>
>> 3) Changing firmware means building the tispl.bin again.
>>
>
>
> FIT images can be disassembled and reassembled with a script around
> tools/dumpimage.
And you expect everyone to master that instead of looking at couple of
aliases in DT to figure out which core corresponds to which ID?
>
> SPL should be simple and load the one next stage.
>
>
>> The FIT solution can not scale well.
>>
>
>
> How does this current series scale at all? At least with FIT you can add
> more images without adding code for
> request_firmware(<hard-coded-firmware-name>) and
> rproc_load(<hard-coded-number>). That all could be encoded in the FIT data.
I understand and as explained earlier i have even implemented that once
before. fs_loader was meant to address the exact use case we are
discussing about.
Even in u-boot remotecores are started/loaded by indices. Users need to
know them. This is no different than that.
I am not convinced about FIT approach. I would let Lokesh take a call on
this.
Thanks,
Keerthy
>
> Andrew
>
>
>> - Keerthy
>>>
>>>
>>>>>
>>>>>
>>>>>> + return 0;
>>>>>> + }
>>>>>> +#endif
>>>>>> + if (!*loadaddr)
>>>>>> + return 0;
>>>>>> +
>>>>>> + if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>>> + size = request_firmware_into_buf(fsdev, name, (void
>>>>>> *)*loadaddr,
>>>>>> + 0, 0);
>>>>>> + }
>>>>>> +
>>>>>> + return size;
>>>>>> +}
>>>>>> +#else
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>> +{
>>>>>> +}
>>>>>> +
>>>>>> void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>> *spl_image)
>>>>>> {
>>>>>> struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>> spl_image_info *spl_image)
>>>>>> /* Release all the exclusive devices held by SPL before
>>>>>> starting ATF */
>>>>>> ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>> + ret = rproc_init();
>>>>>> + if (ret)
>>>>>> + panic("rproc failed to be initialized (%d)\n", ret);
>>>>>> +
>>>>>> + init_env();
>>>>>> + start_non_linux_remote_cores();
>>>>>> +
>>>>>> /*
>>>>>> * It is assumed that remoteproc device 1 is the corresponding
>>>>>> * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>>> same.
>>>>>> */
>>>>>> - ret = rproc_dev_init(1);
>>>>>> - if (ret)
>>>>>> - panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>> __func__,
>>>>>> - ret);
>>>>>> -
>>>>>
>>>>>
>>>>> Where did this code go?
>>>>
>>>> rproc_init takes care of that.
>>>>
>>>
>>>
>>> Is that new behavior then? It should be it's own patch with a commit
>>> message about that.
>>>
>>>
>>>>>
>>>>>
>>>>>> ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>> if (ret)
>>>>>> panic("%s: ATF failed to load on rproc (%d)\n", __func__,
>>>>>> ret);
>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>> int early_console_init(void);
>>>>>> void disable_linefill_optimization(void);
>>>>>> void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>> fwl_data_size);
>>>>>> +void start_non_linux_remote_cores(void);
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>> @@ -18,6 +18,7 @@
>>>>>> #include <dm.h>
>>>>>> #include <dm/uclass-internal.h>
>>>>>> #include <dm/pinctrl.h>
>>>>>> +#include <remoteproc.h>
>>>>>> #ifdef CONFIG_SPL_BUILD
>>>>>> #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>> }
>>>>>> }
>>>>>> #endif
>>>>>> +
>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>> +void start_non_linux_remote_cores(void)
>>>>>> +{
>>>>>> + int size = 0, ret;
>>>>>> + u32 loadaddr = 0;
>>>>>> +
>>>>>> + size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>>> + &loadaddr);
>>>>>> + if (size <= 0)
>>>>>> + goto err_load;
>>>>>> +
>>>>>> + /* remoteproc 2 is aliased for the needed remotecore */
>>>>>
>>>>>
>>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>>> there needs to be a better what than assuming the number for every
>>>>> other
>>>>> remote core.
>>>>>
>>>>>
>>>>>> + ret = rproc_load(2, loadaddr, size);
>>>>>> + if (ret) {
>>>>>> + printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>> + goto err_load;
>>>>>> + }
>>>>>> +
>>>>>> + ret = rproc_start(2);
>>>>>> + if (ret) {
>>>>>> + printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>> + goto err_load;
>>>>>> + }
>>>>>> +
>>>>>> + printf("Remoteproc 2 started successfully\n");
>>>>>
>>>>>
>>>>> That's useful..
>>>>
>>>> That is a print that tells everything went well with rproc 2. Otherwise
>>>> one has to really find other ways to see if it succeeded or not.
>>>>
>>>
>>>
>>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>>> 2" is. I'm saying make the message describe what has happened.
>>>
>>> Andrew
>>>
>>>
>>>>>
>>>>> Andrew
>>>>>
>>>>>
>>>>>> +
>>>>>> + return;
>>>>>> +
>>>>>> +err_load:
>>>>>> + rproc_reset(2);
>>>>>> +}
>>>>>> +#endif
>>>>>>
More information about the U-Boot
mailing list