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-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.
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.
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
