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

2020-02-05 Thread Lokesh Vutla



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 
>> Signed-off-by: Lokesh Vutla 
>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>> Signed-off-by: Andreas Dannenberg 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>    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.
>
>

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

2020-01-24 Thread Andrew F. Davis
On 1/24/20 3:42 AM, Tero Kristo wrote:
> On 23/01/2020 19:19, 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 
>> Signed-off-by: Lokesh Vutla 
>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>> Signed-off-by: Andreas Dannenberg 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>    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 

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

2020-01-24 Thread Keerthy




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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
 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 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
   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() and
rproc_load(). 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 

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

2020-01-24 Thread Tero Kristo

On 23/01/2020 19:19, 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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
 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 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
   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() and
rproc_load(). 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 

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

2020-01-23 Thread Keerthy




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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
     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 
     #include 
     #include 
+#include 
+#include 
+#include 
+#include 
       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() and
rproc_load(). 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 

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

2020-01-23 Thread Andrew F. Davis
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 
>>> Signed-off-by: Lokesh Vutla 
>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>> Signed-off-by: Andreas Dannenberg 
>>> ---
>>>     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 
>>>     #include 
>>>     #include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>>       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 

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

2020-01-23 Thread Keerthy




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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
    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 
    #include 
    #include 
+#include 
+#include 
+#include 
+#include 
      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() and
rproc_load(). 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, )) {
+    size = request_firmware_into_buf(fsdev, name, (void
*)*loadaddr,
+ 0, 0);
+    }
+
+    return size;
+}
+#else
+int 

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

2020-01-23 Thread Andrew F. Davis
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 
> Signed-off-by: Lokesh Vutla 
> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
> Signed-off-by: Andreas Dannenberg 
> ---
>    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 
>    #include 
>    #include 
> +#include 
> +#include 
> +#include 
> +#include 
>      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.


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


> 3) Changing firmware means building the tispl.bin again.
> 


FIT images can be disassembled and reassembled with a script around
tools/dumpimage.

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() and
rproc_load(). That all could be encoded in the FIT data.

Andrew


> - Keerthy
>>
>>


> +    return 0;
> +    }
> +#endif
> +    if (!*loadaddr)
> +    return 0;
> +
> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, )) {
> +    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;

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

2020-01-22 Thread Keerthy




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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
   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 
   #include 
   #include 
+#include 
+#include 
+#include 
+#include 
     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:


1) SPL size would bloat based on the size of the firmware.
2) There are multiple cores that need to be loaded and hence adding all 
the firmwares under a fit can be really painful.

3) Changing firmware means building the tispl.bin again.

The FIT solution can not scale well.

- Keerthy







+    return 0;
+    }
+#endif
+    if (!*loadaddr)
+    return 0;
+
+    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, )) {
+    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 

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

2020-01-22 Thread Andrew F. Davis
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 
>>> Signed-off-by: Lokesh Vutla 
>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>> Signed-off-by: Andreas Dannenberg 
>>> ---
>>>   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 
>>>   #include 
>>>   #include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>>     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?


>>
>>
>>> +    return 0;
>>> +    }
>>> +#endif
>>> +    if (!*loadaddr)
>>> +    return 0;
>>> +
>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, )) {
>>> +    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
>>> --- 

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

2020-01-21 Thread keerthy




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 
Signed-off-by: Lokesh Vutla 
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg 
---
  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 
  #include 
  #include 
+#include 
+#include 
+#include 
+#include 
  
  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.





+   return 0;
+   }
+#endif
+   if (!*loadaddr)
+   return 0;
+
+   if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, )) {
+   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.





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 

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

2020-01-21 Thread Andrew F. Davis
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 
> Signed-off-by: Lokesh Vutla 
> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
> Signed-off-by: Andreas Dannenberg 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  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..


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


> + return 0;
> + }
> +#endif
> + if (!*loadaddr)
> + return 0;
> +
> + if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, )) {
> + 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?


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