| Message ID | 20200716130317.135841-1-gary.bisson@boundarydevices.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | package/freescale-imx/firmware-imx: fix sdma support for imx8m family | expand |
>In latest patch the SDMA installation was limited to platforms whose >name was mentioned in the binary. That would unfortunately be too simple >to manage, instead the i.MX 8M family uses the same binary as the i.MX 7 >processors [1]. Hi Gary, Sorry about this... indeed all platforms could not be tested before merging. Thank you for fixing this. > >Fixes: fad2df39b9 ("package/freescale-imx/firmware-imx: clarify >installation of firmware files") > >[1] >https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mq.dtsi?h=imx_5.4.24_2.1.0#n519 > >Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> >--- > package/freescale-imx/firmware-imx/Config.in | 3 +++ > package/freescale-imx/firmware-imx/firmware-imx.mk | 6 +++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > >diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in >index aae552177f..3859cc1d14 100644 >--- a/package/freescale-imx/firmware-imx/Config.in >+++ b/package/freescale-imx/firmware-imx/Config.in >@@ -20,6 +20,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 >+ 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 > > config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_VPU_FW > bool >diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk >index cd299aad5e..90d9a79860 100644 >--- a/package/freescale-imx/firmware-imx/firmware-imx.mk >+++ b/package/freescale-imx/firmware-imx/firmware-imx.mk >@@ -107,9 +107,13 @@ endif > # > > ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW),y) >+FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) The same concept is used for the VPU ; would it make sense to either use the same naming convention (i.e.: "FIRMWARE_IMX_SDMA_PLATFORM") or rename "FIRMWARE_IMX_VPU_PLATFORM" to "FIRMWARE_IMX_VPU_NAME"? Thanks, Stephane. >+ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) >+FIRMWARE_IMX_SDMA_NAME = imx7d >+endif > define FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW > mkdir -p $(TARGET_DIR)/lib/firmware/imx/sdma >- cp -r $(@D)/firmware/sdma/sdma-$(FIRMWARE_IMX_PLATFORM_LOWER)*.bin \ >+ cp -r $(@D)/firmware/sdma/sdma-$(FIRMWARE_IMX_SDMA_NAME)*.bin \ > $(TARGET_DIR)/lib/firmware/imx/sdma/ > endef > endif >-- >2.27.0 >
On Thu, 16 Jul 2020 15:03:17 +0200 Gary Bisson <gary.bisson@boundarydevices.com> wrote: > In latest patch the SDMA installation was limited to platforms whose > name was mentioned in the binary. That would unfortunately be too simple > to manage, instead the i.MX 8M family uses the same binary as the i.MX 7 > processors [1]. > > Fixes: fad2df39b9 ("package/freescale-imx/firmware-imx: clarify > installation of firmware files") > > [1] > https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mq.dtsi?h=imx_5.4.24_2.1.0#n519 > > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> > --- > package/freescale-imx/firmware-imx/Config.in | 3 +++ > package/freescale-imx/firmware-imx/firmware-imx.mk | 6 +++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in > index aae552177f..3859cc1d14 100644 > --- a/package/freescale-imx/firmware-imx/Config.in > +++ b/package/freescale-imx/firmware-imx/Config.in > @@ -20,6 +20,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > + 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 Thanks for looking into this. Another possible solution if we have more of this in the future (we already have it for vpu and sdma) would be to drop the NEEDS_XYZ_FW options and instead do this: config BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME string default "imx53" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN and then in the .mk file, we rely on BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME being empty or not to decide whether the VPU firmware should be installed. And because this variable is not just a boolean, it also tells us what is the firmware name. > > config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_VPU_FW > bool > diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk > index cd299aad5e..90d9a79860 100644 > --- a/package/freescale-imx/firmware-imx/firmware-imx.mk > +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk > @@ -107,9 +107,13 @@ endif > # > > ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW),y) > +FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) > +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) > +FIRMWARE_IMX_SDMA_NAME = imx7d > +endif I'd prefer: ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) FIRMWARE_IMX_SDMA_NAME = imx7d else FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) endif Also, now that I looked at this more closely, the SDMA firmware is sdma-imx7d.bin, but the BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 is named just "IMX7", so this will not work, so I'm not sure how the: default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 currently works for BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW. Thomas
Hi Thomas, Stéphane, On Thu, Jul 16, 2020 at 03:46:37PM +0200, Thomas Petazzoni wrote: > On Thu, 16 Jul 2020 15:03:17 +0200 > Gary Bisson <gary.bisson@boundarydevices.com> wrote: > > > In latest patch the SDMA installation was limited to platforms whose > > name was mentioned in the binary. That would unfortunately be too simple > > to manage, instead the i.MX 8M family uses the same binary as the i.MX 7 > > processors [1]. > > > > Fixes: fad2df39b9 ("package/freescale-imx/firmware-imx: clarify > > installation of firmware files") > > > > [1] > > https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mq.dtsi?h=imx_5.4.24_2.1.0#n519 > > > > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> > > --- > > package/freescale-imx/firmware-imx/Config.in | 3 +++ > > package/freescale-imx/firmware-imx/firmware-imx.mk | 6 +++++- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in > > index aae552177f..3859cc1d14 100644 > > --- a/package/freescale-imx/firmware-imx/Config.in > > +++ b/package/freescale-imx/firmware-imx/Config.in > > @@ -20,6 +20,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > > + 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 > > Thanks for looking into this. Another possible solution if we have more > of this in the future (we already have it for vpu and sdma) would be to > drop the NEEDS_XYZ_FW options and instead do this: > > config BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME > string > default "imx53" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > and then in the .mk file, we rely on > BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME being empty or not to decide > whether the VPU firmware should be installed. And because this variable > is not just a boolean, it also tells us what is the firmware name. Yes it looks like a better option, we could even do the same for VPU then. Are we all ok with such approach? > > config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_VPU_FW > > bool > > diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk > > index cd299aad5e..90d9a79860 100644 > > --- a/package/freescale-imx/firmware-imx/firmware-imx.mk > > +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk > > @@ -107,9 +107,13 @@ endif > > # > > > > ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW),y) > > +FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) > > +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) > > +FIRMWARE_IMX_SDMA_NAME = imx7d > > +endif > > I'd prefer: > > ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) > FIRMWARE_IMX_SDMA_NAME = imx7d > else > FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) > endif > > Also, now that I looked at this more closely, the SDMA firmware is > sdma-imx7d.bin, but the BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 is > named just "IMX7", so this will not work, so I'm not sure how the: > > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 > > currently works for BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW. Note the '*' which makes everything works ;) That is why imx7 will properly the imx7d binary and the imx27 will copy the imx27_TO2 one. Regards, Gary
On Thu, 16 Jul 2020 15:52:50 +0200 Gary Bisson <gary.bisson@boundarydevices.com> wrote: > > Thanks for looking into this. Another possible solution if we have more > > of this in the future (we already have it for vpu and sdma) would be to > > drop the NEEDS_XYZ_FW options and instead do this: > > > > config BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME > > string > > default "imx53" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > > > and then in the .mk file, we rely on > > BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME being empty or not to decide > > whether the VPU firmware should be installed. And because this variable > > is not just a boolean, it also tells us what is the firmware name. > > Yes it looks like a better option, we could even do the same for VPU > then. > Are we all ok with such approach? I think it doesn't add more lines, and will avoid the calculation of FIRMWARE_IMX_PLATFORM_LOWER. > Note the '*' which makes everything works ;) > That is why imx7 will properly the imx7d binary and the imx27 will copy > the imx27_TO2 one. Ah indeed, and that's another thing we could get rid of with the proposal above, as we would have the exact firmware name. Thanks! Thomas
On Thu, Jul 16, 2020 at 04:07:45PM +0200, Thomas Petazzoni wrote: > On Thu, 16 Jul 2020 15:52:50 +0200 > Gary Bisson <gary.bisson@boundarydevices.com> wrote: > > > > Thanks for looking into this. Another possible solution if we have more > > > of this in the future (we already have it for vpu and sdma) would be to > > > drop the NEEDS_XYZ_FW options and instead do this: > > > > > > config BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME > > > string > > > default "imx53" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 > > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M > > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM > > > default "imx7d" if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN > > > > > > and then in the .mk file, we rely on > > > BR2_PACKAGE_FIRMWARE_IMX_VPU_FW_NAME being empty or not to decide > > > whether the VPU firmware should be installed. And because this variable > > > is not just a boolean, it also tells us what is the firmware name. > > > > Yes it looks like a better option, we could even do the same for VPU > > then. > > Are we all ok with such approach? > > I think it doesn't add more lines, and will avoid the calculation of > FIRMWARE_IMX_PLATFORM_LOWER. Agreed, let's do it then. I'll offer another patch, not a v2 per se as it will also modify VPU copy. > > Note the '*' which makes everything works ;) > > That is why imx7 will properly the imx7d binary and the imx27 will copy > > the imx27_TO2 one. > > Ah indeed, and that's another thing we could get rid of with the > proposal above, as we would have the exact firmware name. Unfortunately no, I don't believe it will be the best option as imx31 for instance has 2 possible sdma firmware (to1 and to2). Same goes for imx35 sdma firmware. Same goes for imx27 vpu firmware... and so on. Still looks acceptable to me, but let me know if you think otherwise. Regards, Gary
diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in index aae552177f..3859cc1d14 100644 --- a/package/freescale-imx/firmware-imx/Config.in +++ b/package/freescale-imx/firmware-imx/Config.in @@ -20,6 +20,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53 default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 + 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 config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_VPU_FW bool diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk index cd299aad5e..90d9a79860 100644 --- a/package/freescale-imx/firmware-imx/firmware-imx.mk +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk @@ -107,9 +107,13 @@ endif # ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_NEEDS_SDMA_FW),y) +FIRMWARE_IMX_SDMA_NAME = $(FIRMWARE_IMX_PLATFORM_LOWER) +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM)$(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) +FIRMWARE_IMX_SDMA_NAME = imx7d +endif define FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW mkdir -p $(TARGET_DIR)/lib/firmware/imx/sdma - cp -r $(@D)/firmware/sdma/sdma-$(FIRMWARE_IMX_PLATFORM_LOWER)*.bin \ + cp -r $(@D)/firmware/sdma/sdma-$(FIRMWARE_IMX_SDMA_NAME)*.bin \ $(TARGET_DIR)/lib/firmware/imx/sdma/ endef endif
In latest patch the SDMA installation was limited to platforms whose name was mentioned in the binary. That would unfortunately be too simple to manage, instead the i.MX 8M family uses the same binary as the i.MX 7 processors [1]. Fixes: fad2df39b9 ("package/freescale-imx/firmware-imx: clarify installation of firmware files") [1] https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mq.dtsi?h=imx_5.4.24_2.1.0#n519 Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> --- package/freescale-imx/firmware-imx/Config.in | 3 +++ package/freescale-imx/firmware-imx/firmware-imx.mk | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-)