[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores
Andrew F. Davis
afd at ti.com
Thu Jan 23 18:05:00 CET 2020
On 1/23/20 11:44 AM, Keerthy wrote:
>
>
> 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.
Exactly what I said, and so size is not a huge deal.
>
>>
>>
>>> 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.
>
If we are early booting them from SPL then they don't really need to be
on the filesystem.
>>
>>
>>> 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?
>
Your patches do more than add DT aliases to add a firmware image. I
think you are responding to the wrong comment here, the ID part is below.
>>
>> 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.
>
Lokesh is not the whole U-Boot community, I get you two aligned
internally on this, but I'd be much more interested in Tom or Simon's
opinion here. I was doing the same thing when loading PMMC firmware for
HS and was pushed to make a FIT friendly version, I'm glad that I did,
it ended up much less hacky in the long run.
Andrew
> 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