[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

Lokesh Vutla lokeshvutla at ti.com
Wed Feb 5 13:07:59 CET 2020 


On 24/01/20 5:33 PM, Keerthy wrote:
> 
> 
> On 23/01/20 10:49 pm, Keerthy wrote:
>>
>>
>> On 23/01/20 10:35 pm, Andrew F. Davis wrote:
>>> 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.
>>
>> I would love to hear Tom & simon's opinions. Before we jump on to PMMC
>> fimrware example. I believe there is one such firmware that gets appended to
>> the FIT image. In case of remoteprocs there can be multiple such firmware.
>> File system is the most generic and scaleble place to access firmware.
>>
> 
> Andrew,
> 
> PMMC in K2G is a case where the firmware image is targeted for a specific core
> and in K2G U-Boot only takes care of loading that particular core. Where as this
> series targets multiple cores which can cater to multiple use cases resulting in
> different combinations. To give more details, below are the approaches
> evaluated(prototyped):
> 
> Approach 1: Pack all the firmwares into a fit Image along with A72 SPL
> ----------------------------------------------------------------------
> Pros:
> -----
> a) Boot media independent: No need to load firmware via file
> system present in different boot media.
> 
> Cons:
> -----
> a) Flash image: In case of raw flash devices. We need to upfront
> predict the partition that needs to be allocated for tispl.bin.
> Complex SoCs like J721e has ~9 remote cores and each firmware can be ~2-3MB
> resulting in ~20-30MB of tispl.bin. In case one has to add
> a bigger firmware that would mean flash partition resizing. Also not all boards
> has flashes > 32MB.
> 
> In fact a prototype of this approach was tried. It failed as soon as
> a larger firmware was included in the FIT image as the memory allocated
> was not sufficient.
> 
> b) Dependency on U-Boot repo for changing firmwares: The firmwares need to be
> built as part of the FIT Image that contains the SPL. Firmware change means
> repacking the FIT image. Any customer/developer who intends to change the remote
> core firmware needs to be using the u-boot repository to pack the image into the
> FIT Image.
> 
> c) Mapping the firmware to remote cores: J7 has around 9 remote
> cores(Mostly R5Fs & DSPs). Each firmware is position dependent.
> There is no good way to map which firmware
> corresponds to which remote core other than hard coding.
> 
> d) Use case support: Different use cases result in different combination of
> remote cores. Every different combination will result in a new binary.
> 
> e) Example build command would be:
> make ARCH=arm R50=<> R51=<> R52=<>.........C7x=<> C6x0=<>
> 
> You can see it :)
> 
> Approach 2: Loading the firmware using fs_loader from file system
> 
> Pros:
> a) No restriction on the firmware image sizes.
> b) Need not re build u-boot for changing firmware
> c) U-Boot build is un-touched for enabling various combination of use cases.
> 
> Cons:
> 
> 1) Boot media dependent: fs_loader as of today supports loading firmware from
> the most common boot media and can be extended to any boot media.
> 
> 2) Hard coding of remote core numbers to the firmware. This cannot be avoided
> any case with the current remoteproc framework in u-boot.
> 
> Rationale on choosing fs_loader approach:
> 
> Considering these options, and to scale the development process as well,
> fs_loader approach has been preferred.
> 


Its been more than a week and there are no responses for the above
justification. I assume there are no objections for the current approach.

Thanks and regards,
Lokesh


> Regards,
> Keerthy
>>
>> Thanks,
>> Keerthy
>>>
>>> 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