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-keer...@ti.com> >>>>>>> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com> >>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER] >>>>>>> Signed-off-by: Andreas Dannenberg <dannenb...@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 >>>>>>>