Re: [PATCH] firmware: fix missing firmware handling for external firmware

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Sascha,

sry. for the delayed answer.

On 24-01-17, Sascha Hauer wrote:
> On Tue, Jan 16, 2024 at 03:44:16PM +0100, Sascha Hauer wrote:
> > Hi Marco,
> > 
> > On Wed, Jan 10, 2024 at 03:59:08PM +0100, Marco Felsch wrote:
> > > Currently we depend on the external firmware file since we need it for
> > > the sha256sum. But this dependency break the 'optional missing firmware'
> > > feature.
> > > 
> > > Fix this by dropping the firmware file prerequisite and instead evaluate
> > > the FWNAME_EXISTS variable. If the firmware file does not exist a dummy
> > > file is created and the build continues till the linking process (like
> > > we do for the other firmware).
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  firmware/Makefile    | 6 +++---
> > >  images/Makefile      | 2 +-
> > >  scripts/Makefile.lib | 2 +-
> > >  3 files changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/firmware/Makefile b/firmware/Makefile
> > > index 51d98d54bf14..a07b31caec73 100644
> > > --- a/firmware/Makefile
> > > +++ b/firmware/Makefile
> > > @@ -41,7 +41,7 @@ fwobjdir := $(objtree)/firmware
> > >  pbl-y     := $(addsuffix .gen.o, $(pbl-firmware-y))
> > >  obj-pbl-y := $(addsuffix .gen.o, $(firmware-y))
> > >  
> > > -FWNAME    = $(patsubst $(obj)/%.extgen.S,%,$(patsubst $(obj)/%.gen.S,%,$@))
> > > +FWNAME    = $(patsubst $(obj)/%.sum,%,$(patsubst $(obj)/%.extgen.S,%,$(patsubst $(obj)/%.gen.S,%,$@)))
> > 
> > The purpose of this change is not clear to me. You replace the pattern
> > %.sum with %, but I don't see how this ever matches.
> 
> Ok, now I see why this is needed. FWNAME works on $@, the filename of
> the target of the rule...

yes.

> > Dropping this change has no visible effect for me.
> > 
> > >  FWSTR     = $(subst /,_,$(subst .,_,$(subst -,_,$(FWNAME))))
> > >  FWNAME_EXISTS = $(if $(wildcard $(fwdir)/$(FWNAME)),1,0)
> > >  
> > > @@ -91,8 +91,8 @@ $(obj)/%.extgen.S: $(obj)/%.sha.bin FORCE
> > >  $(obj)/%.sha.bin: $(obj)/%.sum FORCE
> > >  	$(call if_changed,sha256bin)
> > >  
> > > -$(obj)/%.sum: $(obj)/% FORCE
> > > -	$(call if_changed,sha256sum)
> > > +$(obj)/%.sum: FORCE
> > > +	$(if $(findstring 1,$(FWNAME_EXISTS)), $(call if_changed,sha256sum,$(obj)/$*), @touch $@)
> 
> ...and the target here indeed has the pattern %.sum

and yes.

> I think we should rather use the target stem which you are using
> already. Drop FWNAME_EXISTS so this becomes:
> 
> 	$(if $(wildcard $(fwdir)/$*), $(call if_changed,sha256sum,$(fwdir)/$*),@touch $@)

I wasn't considering this but it looks good to me. Now I see the issue
with $(obj)/% as well :/ I used the $(obj)/firmware path for my firmware
binaries and therefore had no issues with out-of-tree building.

Regards,
  Marco

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>