| Message ID | 1593012228-18959-2-git-send-email-stephane.viau@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | package/freescale-imx: clean-up proposal | expand |
Hi Stephane, On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau <stephane.viau@oss.nxp.com> wrote: > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S > bool "imx6sl/imx6sx" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY There is no VPU on i.MX6SL/i.MX6SX. > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL > bool "imx6ul/imx6ull" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY There is no VPU on i.MX6UL/i.MX6ULL. > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > bool "imx7d/imx7ulp" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY There is no VPU on i.MX7.
Hi Stephane, Forgot to add one more comment: On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau <stephane.viau@oss.nxp.com> wrote: > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY Should we name this after the VPU IP name instead? Something like this: BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_CODA > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_MALONE
Fabio, Stéphane, All, On 2020-06-24 17:11 -0300, Fabio Estevam spake thusly: > Forgot to add one more comment: > > On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau > <stephane.viau@oss.nxp.com> wrote: > > > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > Should we name this after the VPU IP name instead? Something like this: > BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_CODA > > > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_MALONE If the VPU code names are pretty commonly known, then yes, that looks better, indeed. Regards, Yann E. MORIN.
Hello, Thanks for doing this work! It's definitely bringing some good sanity into this firmware-imx mess! See below some comments. On Wed, 24 Jun 2020 17:23:47 +0200 Stephane Viau <stephane.viau@oss.nxp.com> wrote: > diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in > index 0be37ce..2cac650 100644 > --- a/package/freescale-imx/Config.in > +++ b/package/freescale-imx/Config.in > @@ -12,40 +12,63 @@ choice > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK > bool "imx25-3stack" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS > bool "imx27ads" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK > bool "imx37-3stack" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 > bool "imx50" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 > bool "imx51" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > bool "imx53" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > bool "imx6q/imx6dl" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S > bool "imx6sl/imx6sx" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL > bool "imx6ul/imx6ull" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > bool "imx7d/imx7ulp" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 > bool "imx8" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > bool "imx8m" > select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > bool "imx8mm" > @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X > bool "imx8x" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > endchoice > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM > @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU > config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > bool > > +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > + bool It would be nicer to use "NEEDS" instead of "NEED". That would require a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW. However, I am wondering if package/freescale-imx/Config.in is the right place for all this logic. After all, this is only related to the firmware-imx package. Shouldn't we instead move that to package/freescale-imx/firmware-imx/Config.in, with the following form: config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW bool default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN and ditto the other options ? Here as well, it would require a preparation patch to take care of the NEEDS_DDR_FW case, and then your patch that handles all other FW files. Best regards, Thomas
>Hello, Hello Thomas, Yann/Gary, > >Thanks for doing this work! It's definitely bringing some good sanity >into this firmware-imx mess! See below some comments. Glad you like it & thank you for your comments. > >> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in >> index 0be37ce..2cac650 100644 >> --- a/package/freescale-imx/Config.in >> +++ b/package/freescale-imx/Config.in >> @@ -12,40 +12,63 @@ choice >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK >> bool "imx25-3stack" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS >> bool "imx27ads" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK >> bool "imx37-3stack" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 >> bool "imx50" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 >> bool "imx51" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 >> bool "imx53" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q >> bool "imx6q/imx6dl" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S >> bool "imx6sl/imx6sx" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL >> bool "imx6ul/imx6ull" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 >> bool "imx7d/imx7ulp" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 >> bool "imx8" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M >> bool "imx8m" >> select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM >> bool "imx8mm" >> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X >> bool "imx8x" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> endchoice >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM >> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU >> config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW >> bool >> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> + bool > >It would be nicer to use "NEEDS" instead of "NEED". That would require >a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW >to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW. Agreed. > >However, I am wondering if package/freescale-imx/Config.in is the right >place for all this logic. After all, this is only related to the >firmware-imx package. > >Shouldn't we instead move that to >package/freescale-imx/firmware-imx/Config.in, with the following form: > >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW > bool > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > >and ditto the other options ? Here as well, it would require a >preparation patch to take care of the NEEDS_DDR_FW case, and then your >patch that handles all other FW files. I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment: " As Yann mentioned on IRC: "Usually, when we introduce such option, it does not 'default y' based on some other options. Instead, the other options 'select' it." " from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html BR, Stephane. > >Best regards, > >Thomas >-- >Thomas Petazzoni, CTO, Bootlin >Embedded Linux and Kernel engineering >https://bootlin.com
Stephane, All, On 2020-06-25 06:33 +0000, Stephane Viau (OSS) spake thusly: > Thomas wrote: > >However, I am wondering if package/freescale-imx/Config.in is the right > >place for all this logic. After all, this is only related to the > >firmware-imx package. > > > >Shouldn't we instead move that to > >package/freescale-imx/firmware-imx/Config.in, with the following form: > > > >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW > > bool > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > > >and ditto the other options ? Here as well, it would require a > >preparation patch to take care of the NEEDS_DDR_FW case, and then your > >patch that handles all other FW files. > > I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment: > " > As Yann mentioned on IRC: > "Usually, when we introduce such option, it does not 'default y' based > on some other options. Instead, the other options 'select' it." > " > from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html True, that's what I said. That was based on the assumption that the options would be in package/freescale-imx/Config.in. But now, seeing the reasonning by Thomas, I agree with him: those are better suited to live in package/freescale-imx/firmware-imx/Config.in/ And thus it is less clean that the variant selection in the generic choice would have to catter with options specific to a package. Hence, the "default y if ..." is indeed better. Sorry I did not have that insight to begin with... Regards, Yann E. MORIN.
Yann, Sebastien, all, >> Thomas wrote: >> >However, I am wondering if package/freescale-imx/Config.in is the right >> >place for all this logic. After all, this is only related to the >> >firmware-imx package. >> > >> >Shouldn't we instead move that to >> >package/freescale-imx/firmware-imx/Config.in, with the following form: >> > >> >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW >> > bool >> > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M >> > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM >> > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN >> > >> >and ditto the other options ? Here as well, it would require a >> >preparation patch to take care of the NEEDS_DDR_FW case, and then your >> >patch that handles all other FW files. >> >> I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment: >> " >> As Yann mentioned on IRC: >> "Usually, when we introduce such option, it does not 'default y' based >> on some other options. Instead, the other options 'select' it." >> " >> from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html > >True, that's what I said. That was based on the assumption that the >options would be in package/freescale-imx/Config.in. > >But now, seeing the reasonning by Thomas, I agree with him: those are >better suited to live in package/freescale-imx/firmware-imx/Config.in/ > >And thus it is less clean that the variant selection in the generic >choice would have to catter with options specific to a package. > >Hence, the "default y if ..." is indeed better. > >Sorry I did not have that insight to begin with... No worries ; thank you all for your comments! Will send out a v2 soon.. BTW, I'm also thinking of embedding the 'install path' fix proposed by Sebastien here: http://lists.busybox.net/pipermail/buildroot/2020-June/284875.html BR, Stephane. > >Regards, >Yann E. MORIN. > >-- >.-----------------.--------------------.------------------.--------------------. >| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | >| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | >| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | >| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | >'------------------------------^-------^------------------^--------------------'
Hi Stephane, Sorry for the late reply, apart for the comments already made by otherse I have one suggestion below. On Wed, Jun 24, 2020 at 05:23:47PM +0200, Stephane Viau wrote: > Some SoC need a HDMI FW for their bootloader, some other require EPDC, > SDMA and/or VPU. > Instead of trying to "guess" what firmware images need to be installed > in firmware-imx.mk, let the Config framework do the job and allow each > SoC to actually 'select' what firmware they need. > > Note that this patch should also help introducing an eventual DP FW, as > Gary mentioned in a separate thread [1]. > > [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html > > Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr> > Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com> > --- > package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in > index 0be37ce..2cac650 100644 > --- a/package/freescale-imx/Config.in > +++ b/package/freescale-imx/Config.in > @@ -12,40 +12,63 @@ choice > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK > bool "imx25-3stack" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS > bool "imx27ads" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK > bool "imx37-3stack" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 > bool "imx50" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 > bool "imx51" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > bool "imx53" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > bool "imx6q/imx6dl" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S > bool "imx6sl/imx6sx" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL > bool "imx6ul/imx6ull" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > bool "imx7d/imx7ulp" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 > bool "imx8" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > bool "imx8m" > select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > bool "imx8mm" > @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X > bool "imx8x" > + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > endchoice > > config BR2_PACKAGE_FREESCALE_IMX_PLATFORM > @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU > config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > bool > > +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > + bool > + > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > + bool So here you would copy all VPU FW for "LEGACY" but the one for IMX8/IMX8X? I'd rather have a generic IMX_NEED_VPU_FW for all platforms that require a VPU FW and then in firmware-imx.mk only copy the files relevant to the platform selected (imx35,51,6,8x), what do you think? Regards, Gary
> >________________________________________ >From: Gary Bisson <gary.bisson@boundarydevices.com> >Sent: Monday, June 29, 2020 8:08 AM >To: Stephane Viau (OSS) >Cc: buildroot@buildroot.org; Gary Bisson; Refik Tuzakli; Yann E . MORIN >Subject: Re: [Buildroot] [PATCH 1/2] package/freescale-imx: Add option for all i.MX FW needs > >Hi Stephane, Hi Gary, > >Sorry for the late reply, apart for the comments already made by otherse >I have one suggestion below. > >On Wed, Jun 24, 2020 at 05:23:47PM +0200, Stephane Viau wrote: >> Some SoC need a HDMI FW for their bootloader, some other require EPDC, >> SDMA and/or VPU. >> Instead of trying to "guess" what firmware images need to be installed >> in firmware-imx.mk, let the Config framework do the job and allow each >> SoC to actually 'select' what firmware they need. >> >> Note that this patch should also help introducing an eventual DP FW, as >> Gary mentioned in a separate thread [1]. >> >> [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html >> >> Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr> >> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com> >> --- >> package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in >> index 0be37ce..2cac650 100644 >> --- a/package/freescale-imx/Config.in >> +++ b/package/freescale-imx/Config.in >> @@ -12,40 +12,63 @@ choice >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK >> bool "imx25-3stack" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS >> bool "imx27ads" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK >> bool "imx37-3stack" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 >> bool "imx50" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 >> bool "imx51" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 >> bool "imx53" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q >> bool "imx6q/imx6dl" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S >> bool "imx6sl/imx6sx" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL >> bool "imx6ul/imx6ull" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 >> bool "imx7d/imx7ulp" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 >> bool "imx8" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M >> bool "imx8m" >> select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM >> bool "imx8mm" >> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X >> bool "imx8x" >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> endchoice >> >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM >> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU >> config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW >> bool >> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY >> + bool >> + >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X >> + bool > >So here you would copy all VPU FW for "LEGACY" but the one for >IMX8/IMX8X? >I'd rather have a generic IMX_NEED_VPU_FW for all platforms that require >a VPU FW and then in firmware-imx.mk only copy the files relevant to the >platform selected (imx35,51,6,8x), what do you think? This whole section has been reworked in v2 ; can you please review and let me know if you still want some improvement there? BR, Stephane. > >Regards, >Gary >
On do, 25 jun 2020 06:33:59 +0000, Stephane Viau (OSS) wrote: > Date: Thu, 25 Jun 2020 06:33:59 +0000 > From: "Stephane Viau (OSS)" <stephane.viau@oss.nxp.com> > To: Thomas Petazzoni <thomas.petazzoni@bootlin.com>, "Yann E. MORIN" > <yann.morin.1998@free.fr>, Gary Bisson <bisson.gary@gmail.com> > Cc: Refik Tuzakli <tuzakli.refik@gmail.com>, "buildroot@buildroot.org" > <buildroot@buildroot.org> > Subject: Re: [Buildroot] [PATCH 1/2] package/freescale-imx: Add option for > all i.MX FW needs > > > >Hello, > > Hello Thomas, Yann/Gary, > > > > >Thanks for doing this work! It's definitely bringing some good sanity > >into this firmware-imx mess! See below some comments. > > Glad you like it & thank you for your comments. > > > > >> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in > >> index 0be37ce..2cac650 100644 > >> --- a/package/freescale-imx/Config.in > >> +++ b/package/freescale-imx/Config.in > >> @@ -12,40 +12,63 @@ choice > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK > >> bool "imx25-3stack" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS > >> bool "imx27ads" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK > >> bool "imx37-3stack" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 > >> bool "imx50" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 > >> bool "imx51" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > >> bool "imx53" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > >> bool "imx6q/imx6dl" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S > >> bool "imx6sl/imx6sx" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL > >> bool "imx6ul/imx6ull" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > >> bool "imx7d/imx7ulp" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 > >> bool "imx8" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > >> bool "imx8m" > >> select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > >> bool "imx8mm" > >> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X > >> bool "imx8x" > >> + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > >> endchoice > >> > >> config BR2_PACKAGE_FREESCALE_IMX_PLATFORM > >> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU > >> config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > >> bool > >> > >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW > >> + bool > >> + > >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW > >> + bool > >> + > >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW > >> + bool > >> + > >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY > >> + bool > >> + > >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X > >> + bool > > > >It would be nicer to use "NEEDS" instead of "NEED". That would require > >a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW > >to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW. > > Agreed. > > > > >However, I am wondering if package/freescale-imx/Config.in is the right > >place for all this logic. After all, this is only related to the > >firmware-imx package. > > > >Shouldn't we instead move that to > >package/freescale-imx/firmware-imx/Config.in, with the following form: > > > >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW > > bool > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > > >and ditto the other options ? Here as well, it would require a > >preparation patch to take care of the NEEDS_DDR_FW case, and then your > >patch that handles all other FW files. > > I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment: > " > As Yann mentioned on IRC: > "Usually, when we introduce such option, it does not 'default y' based > on some other options. Instead, the other options 'select' it." > " > from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html If you insert a prompt statement, then that allows to NOT include the firmware if explicitely turned off. I'm currently working on an imx8 platform without VPU needs. I don't like shipping firmware that I don't need. Kurt
diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in index 0be37ce..2cac650 100644 --- a/package/freescale-imx/Config.in +++ b/package/freescale-imx/Config.in @@ -12,40 +12,63 @@ choice config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK bool "imx25-3stack" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS bool "imx27ads" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK bool "imx37-3stack" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50 bool "imx50" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51 bool "imx51" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 bool "imx53" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q bool "imx6q/imx6dl" + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S bool "imx6sl/imx6sx" + select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL bool "imx6ul/imx6ull" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 bool "imx7d/imx7ulp" + select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 bool "imx8" + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M bool "imx8m" select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW + select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM bool "imx8mm" @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X bool "imx8x" + select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X endchoice config BR2_PACKAGE_FREESCALE_IMX_PLATFORM @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW bool +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW + bool + +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW + bool + +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW + bool + +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY + bool + +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X + bool + source "package/freescale-imx/imx-alsa-plugins/Config.in" source "package/freescale-imx/imx-codec/Config.in" source "package/freescale-imx/imx-kobs/Config.in"
Some SoC need a HDMI FW for their bootloader, some other require EPDC, SDMA and/or VPU. Instead of trying to "guess" what firmware images need to be installed in firmware-imx.mk, let the Config framework do the job and allow each SoC to actually 'select' what firmware they need. Note that this patch should also help introducing an eventual DP FW, as Gary mentioned in a separate thread [1]. [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com> --- package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)