Message ID | 20240228120645.958316-1-danishanwar@ti.com |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
Series | [v6] remoteproc: uclass: Add methods to load firmware to rproc and boot rproc | expand |
On 28/02/2024 14:06, MD Danish Anwar wrote: > Add APIs to set a firmware_name to a rproc and boot the rproc with the > same firmware. > > Clients can call rproc_set_firmware() API to set firmware_name for a rproc > whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > a buffer by calling request_firmware_into_buf(). rproc_boot() will then > load the firmware file to the remote processor and start the remote > processor. > > Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > Kconfig so that we can call request_firmware_into_buf() from remoteproc > driver. > > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> Reviewed-by: Roger Quadros <rogerq@kernel.org>
On 29/02/24 3:43 pm, Roger Quadros wrote: > > > On 28/02/2024 14:06, MD Danish Anwar wrote: >> Add APIs to set a firmware_name to a rproc and boot the rproc with the >> same firmware. >> >> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >> load the firmware file to the remote processor and start the remote >> processor. >> >> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >> Kconfig so that we can call request_firmware_into_buf() from remoteproc >> driver. >> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > > Reviewed-by: Roger Quadros <rogerq@kernel.org> Hi Tom, Can you please pick this patch. There are no active comments on this patch and Roger and Ravi has reviewed / acked the patch.
On Tue, Mar 05, 2024 at 02:39:14PM +0530, MD Danish Anwar wrote: > On 29/02/24 3:43 pm, Roger Quadros wrote: > > > > > > On 28/02/2024 14:06, MD Danish Anwar wrote: > >> Add APIs to set a firmware_name to a rproc and boot the rproc with the > >> same firmware. > >> > >> Clients can call rproc_set_firmware() API to set firmware_name for a rproc > >> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > >> a buffer by calling request_firmware_into_buf(). rproc_boot() will then > >> load the firmware file to the remote processor and start the remote > >> processor. > >> > >> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > >> Kconfig so that we can call request_firmware_into_buf() from remoteproc > >> driver. > >> > >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > > > > Reviewed-by: Roger Quadros <rogerq@kernel.org> > > Hi Tom, > > Can you please pick this patch. There are no active comments on this > patch and Roger and Ravi has reviewed / acked the patch. Yes, I'm going to give people a little longer to comment / review and will pull this to -next soon.
On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: > Add APIs to set a firmware_name to a rproc and boot the rproc with the > same firmware. > > Clients can call rproc_set_firmware() API to set firmware_name for a rproc > whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > a buffer by calling request_firmware_into_buf(). rproc_boot() will then > load the firmware file to the remote processor and start the remote > processor. > > Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > Kconfig so that we can call request_firmware_into_buf() from remoteproc > driver. > > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > Reviewed-by: Roger Quadros <rogerq@kernel.org> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc in next currently, thanks.
On 3/7/2024 6:16 PM, Tom Rini wrote: > On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: >> Add APIs to set a firmware_name to a rproc and boot the rproc with the > >> same firmware. >> >> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >> load the firmware file to the remote processor and start the remote >> processor. >> >> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >> Kconfig so that we can call request_firmware_into_buf() from remoteproc >> driver. >> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> >> Reviewed-by: Roger Quadros <rogerq@kernel.org> > > This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu > am65x_evm_r5_usbmsc in next currently, thanks. > I will work on fixing this build error and re-spin the patch.
On 11/03/24 10:34 am, Anwar, Md Danish wrote: > > > On 3/7/2024 6:16 PM, Tom Rini wrote: >> On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: >>> Add APIs to set a firmware_name to a rproc and boot the rproc with the >> >>> same firmware. >>> >>> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >>> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >>> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >>> load the firmware file to the remote processor and start the remote >>> processor. >>> >>> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >>> Kconfig so that we can call request_firmware_into_buf() from remoteproc >>> driver. >>> >>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> >>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >> >> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu >> am65x_evm_r5_usbmsc in next currently, thanks. >> > I will work on fixing this build error and re-spin the patch. > Hi Tom, Roger, This patch adds "request_firmware_into_buf()" in the rproc driver. To use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled, FS_LOADER also gets enabled. Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c [2] has a "load_firmware()" API which calls fs-loader APIs and they have below if condition before calling fs-loader APIs. if (!IS_ENABLED(CONFIG_FS_LOADER)) return 0; Till now, CONFIG_FS_LOADER was not set as a result the load_firmware() API in above mentioned files, was returning 0. Now as this patch enables CONFIG_FS_LOADER, as a result the code after the if check starts getting executed and it tries to look for get_fs_loader() and other fs-loader APIs but this is done at SPL and at this time FS_LOADER is not built yet as a result we see below error. The if checks only checks for CONFIG_FS_LOADER but not for CONFIG_SPL_FS_LOADER. AR spl/boot/built-in.o LD spl/u-boot-spl arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function `load_firmware': /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined reference to `get_fs_loader' arm-none-linux-gnueabihf-ld.bfd: /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined reference to `request_firmware_into_buf' make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527: spl/u-boot-spl] Error 1 make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055: spl/u-boot-spl] Error 2 make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5' make: *** [Makefile:177: sub-make] Error 2 This bug has always been there but as CONFIG_FS_LOADER was never enabled, this build error was never seen as the load_firmware() API will return 0 without calling fs-loader APIs. Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and build error is seen. My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER) instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER) based on the build stage. I tested with the below diff and I don't see build errors with am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs. diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index f411366778..6792ff7467 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr) char *name = NULL; int size = 0; - if (!IS_ENABLED(CONFIG_FS_LOADER)) + if (!CONFIG_IS_ENABLED(FS_LOADER)) return 0; *loadaddr = 0; diff --git a/arch/arm/mach-omap2/boot-common.c b/arch/arm/mach-omap2/boot-common.c index 57917da25c..aa0ab13d5f 100644 --- a/arch/arm/mach-omap2/boot-common.c +++ b/arch/arm/mach-omap2/boot-common.c @@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr) struct udevice *fsdev; int size = 0; - if (!IS_ENABLED(CONFIG_FS_LOADER)) + if (!CONFIG_IS_ENABLED(FS_LOADER)) return 0; if (!*loadaddr) Tom, Roger, Please let me know if this looks ok. If it's ok, I will post this diff as a separate patch and once that is merged Tom can merge this patch or I can send a v7 if needed. [1] https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L159 [2] https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-omap2/boot-common.c#L188
On Tue, Mar 12, 2024 at 02:02:08PM +0530, MD Danish Anwar wrote: > > > On 11/03/24 10:34 am, Anwar, Md Danish wrote: > > > > > > On 3/7/2024 6:16 PM, Tom Rini wrote: > >> On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: > >>> Add APIs to set a firmware_name to a rproc and boot the rproc with the > >> > >>> same firmware. > >>> > >>> Clients can call rproc_set_firmware() API to set firmware_name for a rproc > >>> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > >>> a buffer by calling request_firmware_into_buf(). rproc_boot() will then > >>> load the firmware file to the remote processor and start the remote > >>> processor. > >>> > >>> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > >>> Kconfig so that we can call request_firmware_into_buf() from remoteproc > >>> driver. > >>> > >>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > >>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > >>> Reviewed-by: Roger Quadros <rogerq@kernel.org> > >> > >> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu > >> am65x_evm_r5_usbmsc in next currently, thanks. > >> > > I will work on fixing this build error and re-spin the patch. > > > > Hi Tom, Roger, > > This patch adds "request_firmware_into_buf()" in the rproc driver. To > use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in > REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled, > FS_LOADER also gets enabled. > > Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c > [2] has a "load_firmware()" API which calls fs-loader APIs and they have > below if condition before calling fs-loader APIs. > > if (!IS_ENABLED(CONFIG_FS_LOADER)) > return 0; > > Till now, CONFIG_FS_LOADER was not set as a result the load_firmware() > API in above mentioned files, was returning 0. > > Now as this patch enables CONFIG_FS_LOADER, as a result the code after > the if check starts getting executed and it tries to look for > get_fs_loader() and other fs-loader APIs but this is done at SPL and at > this time FS_LOADER is not built yet as a result we see below error. > The if checks only checks for CONFIG_FS_LOADER but not for > CONFIG_SPL_FS_LOADER. > > AR spl/boot/built-in.o > LD spl/u-boot-spl > arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function > `load_firmware': > /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined > reference to `get_fs_loader' > arm-none-linux-gnueabihf-ld.bfd: > /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined > reference to `request_firmware_into_buf' > make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527: > spl/u-boot-spl] Error 1 > make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055: > spl/u-boot-spl] Error 2 > make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5' > make: *** [Makefile:177: sub-make] Error 2 > > This bug has always been there but as CONFIG_FS_LOADER was never > enabled, this build error was never seen as the load_firmware() API will > return 0 without calling fs-loader APIs. > > Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and > build error is seen. > > My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER) > instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the > appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER) > based on the build stage. > > I tested with the below diff and I don't see build errors with > am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs. > > diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c > index f411366778..6792ff7467 100644 > --- a/arch/arm/mach-k3/common.c > +++ b/arch/arm/mach-k3/common.c > @@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char > *name_loadaddr, u32 *loadaddr) > char *name = NULL; > int size = 0; > > - if (!IS_ENABLED(CONFIG_FS_LOADER)) > + if (!CONFIG_IS_ENABLED(FS_LOADER)) > return 0; > > *loadaddr = 0; > diff --git a/arch/arm/mach-omap2/boot-common.c > b/arch/arm/mach-omap2/boot-common.c > index 57917da25c..aa0ab13d5f 100644 > --- a/arch/arm/mach-omap2/boot-common.c > +++ b/arch/arm/mach-omap2/boot-common.c > @@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr) > struct udevice *fsdev; > int size = 0; > > - if (!IS_ENABLED(CONFIG_FS_LOADER)) > + if (!CONFIG_IS_ENABLED(FS_LOADER)) > return 0; > > if (!*loadaddr) > > > Tom, Roger, Please let me know if this looks ok. > If it's ok, I will post this diff as a separate patch and once that is > merged Tom can merge this patch or I can send a v7 if needed. Yes, this seems like the right path, thanks.
On 3/14/2024 6:16 PM, Tom Rini wrote: > On Tue, Mar 12, 2024 at 02:02:08PM +0530, MD Danish Anwar wrote: >> >> >> On 11/03/24 10:34 am, Anwar, Md Danish wrote: >>> >>> >>> On 3/7/2024 6:16 PM, Tom Rini wrote: >>>> On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: >>>>> Add APIs to set a firmware_name to a rproc and boot the rproc with the >>>> >>>>> same firmware. >>>>> >>>>> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >>>>> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >>>>> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >>>>> load the firmware file to the remote processor and start the remote >>>>> processor. >>>>> >>>>> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >>>>> Kconfig so that we can call request_firmware_into_buf() from remoteproc >>>>> driver. >>>>> >>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> >>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>> >>>> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu >>>> am65x_evm_r5_usbmsc in next currently, thanks. >>>> >>> I will work on fixing this build error and re-spin the patch. >>> >> >> Hi Tom, Roger, >> >> This patch adds "request_firmware_into_buf()" in the rproc driver. To >> use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in >> REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled, >> FS_LOADER also gets enabled. >> >> Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c >> [2] has a "load_firmware()" API which calls fs-loader APIs and they have >> below if condition before calling fs-loader APIs. >> >> if (!IS_ENABLED(CONFIG_FS_LOADER)) >> return 0; >> >> Till now, CONFIG_FS_LOADER was not set as a result the load_firmware() >> API in above mentioned files, was returning 0. >> >> Now as this patch enables CONFIG_FS_LOADER, as a result the code after >> the if check starts getting executed and it tries to look for >> get_fs_loader() and other fs-loader APIs but this is done at SPL and at >> this time FS_LOADER is not built yet as a result we see below error. >> The if checks only checks for CONFIG_FS_LOADER but not for >> CONFIG_SPL_FS_LOADER. >> >> AR spl/boot/built-in.o >> LD spl/u-boot-spl >> arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function >> `load_firmware': >> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined >> reference to `get_fs_loader' >> arm-none-linux-gnueabihf-ld.bfd: >> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined >> reference to `request_firmware_into_buf' >> make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527: >> spl/u-boot-spl] Error 1 >> make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055: >> spl/u-boot-spl] Error 2 >> make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5' >> make: *** [Makefile:177: sub-make] Error 2 >> >> This bug has always been there but as CONFIG_FS_LOADER was never >> enabled, this build error was never seen as the load_firmware() API will >> return 0 without calling fs-loader APIs. >> >> Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and >> build error is seen. >> >> My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER) >> instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the >> appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER) >> based on the build stage. >> >> I tested with the below diff and I don't see build errors with >> am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs. >> >> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c >> index f411366778..6792ff7467 100644 >> --- a/arch/arm/mach-k3/common.c >> +++ b/arch/arm/mach-k3/common.c >> @@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char >> *name_loadaddr, u32 *loadaddr) >> char *name = NULL; >> int size = 0; >> >> - if (!IS_ENABLED(CONFIG_FS_LOADER)) >> + if (!CONFIG_IS_ENABLED(FS_LOADER)) >> return 0; >> >> *loadaddr = 0; >> diff --git a/arch/arm/mach-omap2/boot-common.c >> b/arch/arm/mach-omap2/boot-common.c >> index 57917da25c..aa0ab13d5f 100644 >> --- a/arch/arm/mach-omap2/boot-common.c >> +++ b/arch/arm/mach-omap2/boot-common.c >> @@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr) >> struct udevice *fsdev; >> int size = 0; >> >> - if (!IS_ENABLED(CONFIG_FS_LOADER)) >> + if (!CONFIG_IS_ENABLED(FS_LOADER)) >> return 0; >> >> if (!*loadaddr) >> >> >> Tom, Roger, Please let me know if this looks ok. >> If it's ok, I will post this diff as a separate patch and once that is >> merged Tom can merge this patch or I can send a v7 if needed. > > Yes, this seems like the right path, thanks. > Thanks Tom. Posted this diff as patch https://lore.kernel.org/all/20240314143311.259568-1-danishanwar@ti.com/
On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: > Add APIs to set a firmware_name to a rproc and boot the rproc with the > same firmware. > > Clients can call rproc_set_firmware() API to set firmware_name for a rproc > whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > a buffer by calling request_firmware_into_buf(). rproc_boot() will then > load the firmware file to the remote processor and start the remote > processor. > > Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > Kconfig so that we can call request_firmware_into_buf() from remoteproc > driver. > > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > Reviewed-by: Roger Quadros <rogerq@kernel.org> > --- > Changes from v5 to v6: > *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com> > *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org> > *) Added if condition to check if uc_pdata->fw_name exists and free it > before the strndup as suggested by Roger Quadros <rogerq@kernel.org> > > Changes from v4 to v5: > *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size > that can be loaded to a rproc. > *) Added freeing of address in rproc_boot() as pointed out by Ravi. > *) Allocating the address at a later point in rproc_boot() > *) Rebased on latest u-boot/master [commit > 9e00b6993f724da9699ef12573307afea8c19284] > > Changes from v3 to v4: > *) No functional change. Splitted the patch out of the series as suggested > by Nishant. > *) Droppped the RFC tag. > > v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/ > v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ > v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/ > > drivers/remoteproc/Kconfig | 8 +++ > drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++ > include/remoteproc.h | 34 ++++++++++ > 3 files changed, 144 insertions(+) > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index 781de530af..9f9877931c 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -10,6 +10,7 @@ menu "Remote Processor drivers" > # All users should depend on DM > config REMOTEPROC > bool > + select FS_LOADER > depends on DM > > # Please keep the configuration alphabetically sorted. Can we not make the FS_LOADER portion optional? I didn't realize how many non-TI platforms this impacted. And even then it's possible I assume that custom designs will load the firmwares in other manners.
Hi Tom, On 20/03/24 4:10 am, Tom Rini wrote: > On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: > >> Add APIs to set a firmware_name to a rproc and boot the rproc with the >> same firmware. >> >> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >> load the firmware file to the remote processor and start the remote >> processor. >> >> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >> Kconfig so that we can call request_firmware_into_buf() from remoteproc >> driver. >> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> >> Reviewed-by: Roger Quadros <rogerq@kernel.org> >> --- >> Changes from v5 to v6: >> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com> >> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org> >> *) Added if condition to check if uc_pdata->fw_name exists and free it >> before the strndup as suggested by Roger Quadros <rogerq@kernel.org> >> >> Changes from v4 to v5: >> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size >> that can be loaded to a rproc. >> *) Added freeing of address in rproc_boot() as pointed out by Ravi. >> *) Allocating the address at a later point in rproc_boot() >> *) Rebased on latest u-boot/master [commit >> 9e00b6993f724da9699ef12573307afea8c19284] >> >> Changes from v3 to v4: >> *) No functional change. Splitted the patch out of the series as suggested >> by Nishant. >> *) Droppped the RFC tag. >> >> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/ >> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ >> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/ >> >> drivers/remoteproc/Kconfig | 8 +++ >> drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++ >> include/remoteproc.h | 34 ++++++++++ >> 3 files changed, 144 insertions(+) >> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig >> index 781de530af..9f9877931c 100644 >> --- a/drivers/remoteproc/Kconfig >> +++ b/drivers/remoteproc/Kconfig >> @@ -10,6 +10,7 @@ menu "Remote Processor drivers" >> # All users should depend on DM >> config REMOTEPROC >> bool >> + select FS_LOADER >> depends on DM >> >> # Please keep the configuration alphabetically sorted. > > Can we not make the FS_LOADER portion optional? I didn't realize how > many non-TI platforms this impacted. And even then it's possible I > assume that custom designs will load the firmwares in other manners. > Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER, the clinet driver (ICSSG in this case) who is calling those remoteproc APIs will select FS_LOADER and enable it. This will make sure that other platforms (ti or non-ti) that doesn't support ICSSG but enables Remoteproc, will not enable FS_LOADER. This way we are not forcing other platforms using remoteproc to enable FS_LOADER. In this case the APIs will not get built. Now FS_LOADER will only be enabled when there is a client driver that uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER below is the diff, diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 9f9877931c..a49802c132 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -10,7 +10,6 @@ menu "Remote Processor drivers" # All users should depend on DM config REMOTEPROC bool - select FS_LOADER depends on DM # Please keep the configuration alphabetically sorted. diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index f4f22a3851..a6a8be5009 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) return 0; } +#ifdef CONFIG_FS_LOADER int rproc_boot(struct udevice *rproc_dev) { struct dm_rproc_uclass_pdata *uc_pdata; @@ -1063,3 +1064,4 @@ free_buffer: free(addr); return ret; } +#endif Let me know if this looks ok. If it's ok I will post v7 with this change.
On Wed, Mar 20, 2024 at 11:19:01AM +0530, MD Danish Anwar wrote: > Hi Tom, > > On 20/03/24 4:10 am, Tom Rini wrote: > > On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: > > > >> Add APIs to set a firmware_name to a rproc and boot the rproc with the > >> same firmware. > >> > >> Clients can call rproc_set_firmware() API to set firmware_name for a rproc > >> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to > >> a buffer by calling request_firmware_into_buf(). rproc_boot() will then > >> load the firmware file to the remote processor and start the remote > >> processor. > >> > >> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in > >> Kconfig so that we can call request_firmware_into_buf() from remoteproc > >> driver. > >> > >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > >> Reviewed-by: Roger Quadros <rogerq@kernel.org> > >> --- > >> Changes from v5 to v6: > >> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com> > >> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org> > >> *) Added if condition to check if uc_pdata->fw_name exists and free it > >> before the strndup as suggested by Roger Quadros <rogerq@kernel.org> > >> > >> Changes from v4 to v5: > >> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size > >> that can be loaded to a rproc. > >> *) Added freeing of address in rproc_boot() as pointed out by Ravi. > >> *) Allocating the address at a later point in rproc_boot() > >> *) Rebased on latest u-boot/master [commit > >> 9e00b6993f724da9699ef12573307afea8c19284] > >> > >> Changes from v3 to v4: > >> *) No functional change. Splitted the patch out of the series as suggested > >> by Nishant. > >> *) Droppped the RFC tag. > >> > >> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/ > >> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ > >> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/ > >> > >> drivers/remoteproc/Kconfig | 8 +++ > >> drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++ > >> include/remoteproc.h | 34 ++++++++++ > >> 3 files changed, 144 insertions(+) > >> > >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > >> index 781de530af..9f9877931c 100644 > >> --- a/drivers/remoteproc/Kconfig > >> +++ b/drivers/remoteproc/Kconfig > >> @@ -10,6 +10,7 @@ menu "Remote Processor drivers" > >> # All users should depend on DM > >> config REMOTEPROC > >> bool > >> + select FS_LOADER > >> depends on DM > >> > >> # Please keep the configuration alphabetically sorted. > > > > Can we not make the FS_LOADER portion optional? I didn't realize how > > many non-TI platforms this impacted. And even then it's possible I > > assume that custom designs will load the firmwares in other manners. > > > > Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef > CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER, > the clinet driver (ICSSG in this case) who is calling those remoteproc > APIs will select FS_LOADER and enable it. > > This will make sure that other platforms (ti or non-ti) that doesn't > support ICSSG but enables Remoteproc, will not enable FS_LOADER. This > way we are not forcing other platforms using remoteproc to enable > FS_LOADER. In this case the APIs will not get built. > > Now FS_LOADER will only be enabled when there is a client driver that > uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER > > below is the diff, > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index 9f9877931c..a49802c132 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -10,7 +10,6 @@ menu "Remote Processor drivers" > # All users should depend on DM > config REMOTEPROC > bool > - select FS_LOADER > depends on DM > > # Please keep the configuration alphabetically sorted. > diff --git a/drivers/remoteproc/rproc-uclass.c > b/drivers/remoteproc/rproc-uclass.c > index f4f22a3851..a6a8be5009 100644 > --- a/drivers/remoteproc/rproc-uclass.c > +++ b/drivers/remoteproc/rproc-uclass.c > @@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev, > const char *fw_name) > return 0; > } > > +#ifdef CONFIG_FS_LOADER > int rproc_boot(struct udevice *rproc_dev) > { > struct dm_rproc_uclass_pdata *uc_pdata; > @@ -1063,3 +1064,4 @@ free_buffer: > free(addr); > return ret; > } > +#endif > > Let me know if this looks ok. If it's ok I will post v7 with this change. Yes please, thanks.
On 20/03/24 6:08 pm, Tom Rini wrote: > On Wed, Mar 20, 2024 at 11:19:01AM +0530, MD Danish Anwar wrote: >> Hi Tom, >> >> On 20/03/24 4:10 am, Tom Rini wrote: >>> On Wed, Feb 28, 2024 at 05:36:45PM +0530, MD Danish Anwar wrote: >>> >>>> Add APIs to set a firmware_name to a rproc and boot the rproc with the >>>> same firmware. >>>> >>>> Clients can call rproc_set_firmware() API to set firmware_name for a rproc >>>> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to >>>> a buffer by calling request_firmware_into_buf(). rproc_boot() will then >>>> load the firmware file to the remote processor and start the remote >>>> processor. >>>> >>>> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in >>>> Kconfig so that we can call request_firmware_into_buf() from remoteproc >>>> driver. >>>> >>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>> --- >>>> Changes from v5 to v6: >>>> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com> >>>> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org> >>>> *) Added if condition to check if uc_pdata->fw_name exists and free it >>>> before the strndup as suggested by Roger Quadros <rogerq@kernel.org> >>>> >>>> Changes from v4 to v5: >>>> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size >>>> that can be loaded to a rproc. >>>> *) Added freeing of address in rproc_boot() as pointed out by Ravi. >>>> *) Allocating the address at a later point in rproc_boot() >>>> *) Rebased on latest u-boot/master [commit >>>> 9e00b6993f724da9699ef12573307afea8c19284] >>>> >>>> Changes from v3 to v4: >>>> *) No functional change. Splitted the patch out of the series as suggested >>>> by Nishant. >>>> *) Droppped the RFC tag. >>>> >>>> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/ >>>> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ >>>> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/ >>>> >>>> drivers/remoteproc/Kconfig | 8 +++ >>>> drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++ >>>> include/remoteproc.h | 34 ++++++++++ >>>> 3 files changed, 144 insertions(+) >>>> >>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig >>>> index 781de530af..9f9877931c 100644 >>>> --- a/drivers/remoteproc/Kconfig >>>> +++ b/drivers/remoteproc/Kconfig >>>> @@ -10,6 +10,7 @@ menu "Remote Processor drivers" >>>> # All users should depend on DM >>>> config REMOTEPROC >>>> bool >>>> + select FS_LOADER >>>> depends on DM >>>> >>>> # Please keep the configuration alphabetically sorted. >>> >>> Can we not make the FS_LOADER portion optional? I didn't realize how >>> many non-TI platforms this impacted. And even then it's possible I >>> assume that custom designs will load the firmwares in other manners. >>> >> >> Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef >> CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER, >> the clinet driver (ICSSG in this case) who is calling those remoteproc >> APIs will select FS_LOADER and enable it. >> >> This will make sure that other platforms (ti or non-ti) that doesn't >> support ICSSG but enables Remoteproc, will not enable FS_LOADER. This >> way we are not forcing other platforms using remoteproc to enable >> FS_LOADER. In this case the APIs will not get built. >> >> Now FS_LOADER will only be enabled when there is a client driver that >> uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER >> >> below is the diff, >> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig >> index 9f9877931c..a49802c132 100644 >> --- a/drivers/remoteproc/Kconfig >> +++ b/drivers/remoteproc/Kconfig >> @@ -10,7 +10,6 @@ menu "Remote Processor drivers" >> # All users should depend on DM >> config REMOTEPROC >> bool >> - select FS_LOADER >> depends on DM >> >> # Please keep the configuration alphabetically sorted. >> diff --git a/drivers/remoteproc/rproc-uclass.c >> b/drivers/remoteproc/rproc-uclass.c >> index f4f22a3851..a6a8be5009 100644 >> --- a/drivers/remoteproc/rproc-uclass.c >> +++ b/drivers/remoteproc/rproc-uclass.c >> @@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev, >> const char *fw_name) >> return 0; >> } >> >> +#ifdef CONFIG_FS_LOADER >> int rproc_boot(struct udevice *rproc_dev) >> { >> struct dm_rproc_uclass_pdata *uc_pdata; >> @@ -1063,3 +1064,4 @@ free_buffer: >> free(addr); >> return ret; >> } >> +#endif >> >> Let me know if this looks ok. If it's ok I will post v7 with this change. > > Yes please, thanks. > Posted v7 with the above changes https://lore.kernel.org/all/20240321102819.1011011-1-danishanwar@ti.com/ Please check.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..9f9877931c 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -10,6 +10,7 @@ menu "Remote Processor drivers" # All users should depend on DM config REMOTEPROC bool + select FS_LOADER depends on DM # Please keep the configuration alphabetically sorted. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver. +config REMOTEPROC_MAX_FW_SIZE + hex "Maximum size of firmware file that needs to be loaded to the remote processor" + default 0x10000 + help + Maximum size of the firmware file (elf, binary) that needs to be + loaded to the remote processor. + endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..f4f22a3851 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -13,6 +13,7 @@ #include <log.h> #include <malloc.h> #include <virtio_ring.h> +#include <fs_loader.h> #include <remoteproc.h> #include <asm/io.h> #include <dm/device-internal.h> @@ -961,3 +962,104 @@ unsigned long rproc_parse_resource_table(struct udevice *dev, struct rproc *cfg) return 1; } + +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) +{ + struct dm_rproc_uclass_pdata *uc_pdata; + int len; + char *p; + + if (!rproc_dev || !fw_name) + return -EINVAL; + + uc_pdata = dev_get_uclass_plat(rproc_dev); + if (!uc_pdata) + return -EINVAL; + + len = strcspn(fw_name, "\n"); + if (!len) { + debug("invalid firmware name\n"); + return -EINVAL; + } + + if (uc_pdata->fw_name) + free(uc_pdata->fw_name); + + p = strndup(fw_name, len); + if (!p) + return -ENOMEM; + + uc_pdata->fw_name = p; + + return 0; +} + +int rproc_boot(struct udevice *rproc_dev) +{ + struct dm_rproc_uclass_pdata *uc_pdata; + struct udevice *fs_loader; + int core_id, ret = 0; + char *firmware; + void *addr; + + if (!rproc_dev) + return -EINVAL; + + uc_pdata = dev_get_uclass_plat(rproc_dev); + if (!uc_pdata) + return -EINVAL; + + core_id = dev_seq(rproc_dev); + firmware = uc_pdata->fw_name; + if (!firmware) { + debug("No firmware name set for rproc core %d\n", core_id); + return -EINVAL; + } + + /* Initialize all rproc cores */ + if (!rproc_is_initialized()) { + ret = rproc_init(); + if (ret) { + debug("rproc_init() failed: %d\n", ret); + return ret; + } + } + + /* Loading firmware to a given address */ + ret = get_fs_loader(&fs_loader); + if (ret) { + debug("could not get fs loader: %d\n", ret); + return ret; + } + + if (CONFIG_REMOTEPROC_MAX_FW_SIZE) { + addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE); + if (!addr) + return -ENOMEM; + } else { + debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n"); + return -EINVAL; + } + + ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE, + 0); + if (ret < 0) { + debug("could not request %s: %d\n", firmware, ret); + goto free_buffer; + } + + ret = rproc_load(core_id, (ulong)addr, ret); + if (ret) { + debug("failed to load %s to rproc core %d from addr 0x%08lX err %d\n", + uc_pdata->fw_name, core_id, (ulong)addr, ret); + goto free_buffer; + } + + ret = rproc_start(core_id); + if (ret) + debug("failed to start rproc core %d\n", core_id); + +free_buffer: + free(addr); + return ret; +} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 100644 --- a/include/remoteproc.h +++ b/include/remoteproc.h @@ -403,6 +403,7 @@ enum rproc_mem_type { * @name: Platform-specific way of naming the Remote proc * @mem_type: one of 'enum rproc_mem_type' * @driver_plat_data: driver specific platform data that may be needed. + * @fw_name: firmware name * * This can be accessed with dev_get_uclass_plat() for any UCLASS_REMOTEPROC * device. @@ -412,6 +413,7 @@ struct dm_rproc_uclass_pdata { const char *name; enum rproc_mem_type mem_type; void *driver_plat_data; + char *fw_name; }; /** @@ -705,6 +707,34 @@ unsigned long rproc_parse_resource_table(struct udevice *dev, struct resource_table *rproc_find_resource_table(struct udevice *dev, unsigned int addr, int *tablesz); +/** + * rproc_set_firmware() - assign a new firmware name + * @rproc_dev: device for which new firmware name is being assigned + * @fw_name: new firmware name to be assigned + * + * This function allows remoteproc drivers or clients to configure a custom + * firmware name. The function does not trigger a remote processor boot, + * only sets the firmware name used for a subsequent boot. + * + * This function sets the fw_name field in uclass pdata of the Remote proc + * + * Return: 0 on success or a negative value upon failure + */ +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name); + +/** + * rproc_boot() - boot a remote processor + * @rproc_dev: rproc device to boot + * + * Boot a remote processor (i.e. load its firmware, power it on, ...). + * + * This function first loads the firmware set in the uclass pdata of Remote + * processor to a buffer and then loads firmware to the remote processor + * using rproc_load(). + * + * Return: 0 on success, and an appropriate error value otherwise + */ +int rproc_boot(struct udevice *rproc_dev); #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr, ulong fw_size, ulong *rsc_addr, ulong *rsc_size) { return -ENOSYS; } +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) +{ return -ENOSYS; } +static inline int rproc_boot(struct udevice *rproc_dev) +{ return -ENOSYS; } #endif #endif /* _RPROC_H_ */