[boot-wrapper PATCH v2 9/9] avoid dtc warnings on re-compiling DTB

[Mark Rutland]

On Fri, Jan 14, 2022 at 12:09:31PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:44:55 +0000
> Mark Rutland <mark.rutland at arm.com> wrote:
> 
> > On Fri, Jan 14, 2022 at 08:35:06AM +0000, Vladimir Murzin wrote:
> > > Hi Andre,
> > > 
> > > On 1/13/22 7:50 PM, Andre Przywara wrote:  
> > > > On Thu, 13 Jan 2022 18:42:50 +0000
> > > > Vladimir Murzin <vladimir.murzin at arm.com> wrote:
> > > > 
> > > > Hi Vladimir,
> > > >   
> > > >> On 12/22/21 6:16 PM, Andre Przywara wrote:  
> > > >>> When we add the PSCI nodes to the provided DTB, we use dtc to de-compile
> > > >>> the blob first, then re-compile it with our nodes and properties added.
> > > >>>
> > > >>> In our input DTB the proper phandle references have already been lost,
> > > >>> all we see in the DTB is phandle properties in the target node, and some
> > > >>> numbers in the clocks and gpios properties:
> > > >>> ===========
> > > >>> 	clk24mhz {
> > > >>> 		compatible = "fixed-clock";
> > > >>> 		#clock-cells = <0x00>;
> > > >>> 		clock-frequency = <0x16e3600>;
> > > >>> 		clock-output-names = "v2m:clk24mhz";    
> > > >>> ->		phandle = <0x05>;    
> > > >>> 	};
> > > >>> 	...
> > > >>> 	serial at 90000 {
> > > >>> 		compatible = "arm,pl011", "arm,primecell";
> > > >>> 		reg = <0x90000 0x1000>;
> > > >>> 		interrupts = <0x05>;    
> > > >>> ->		clocks = <0x05 0x05>;    
> > > >>> 		clock-names = "uartclk", "apb_pclk";
> > > >>> 	};
> > > >>> ===========
> > > >>> dtc warns that those numbers might be wrong:
> > > >>> =========
> > > >>> <stdin>:177.6-27: Warning (clocks_property):
> > > >>>  /bus at 8000000/motherboard-bus at 8000000/iofpga-bus at 300000000/serial at 90000:
> > > >>>    clocks: cell 0 is not a phandle reference
> > > >>> ....
> > > >>> =========
> > > >>> The proper solution would be to use references (&v2m_clk24mhz) instead,
> > > >>> as there are in the source .dts file, but we don't have that information
> > > >>> anymore, and cannot easily recover it.
> > > >>>
> > > >>> To avoid the lengthy list of warnings, just drop those checks from the
> > > >>> dtc compilation run. This disables more checks than we want or need, but
> > > >>> we somewhat trust in the original DTB to be sane, so that should be
> > > >>> fine.
> > > >>>
> > > >>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > > >>> ---
> > > >>>  Makefile.am | 2 +-
> > > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/Makefile.am b/Makefile.am
> > > >>> index 3d8128f..430b4a9 100644
> > > >>> --- a/Makefile.am
> > > >>> +++ b/Makefile.am
> > > >>> @@ -160,7 +160,7 @@ model.lds: $(LD_SCRIPT) Makefile
> > > >>>  	$(CPP) $(CPPFLAGS) -ansi -DPHYS_OFFSET=$(PHYS_OFFSET) -DMBOX_OFFSET=$(MBOX_OFFSET) -DKERNEL_OFFSET=$(KERNEL_OFFSET) -DFDT_OFFSET=$(FDT_OFFSET) -DFS_OFFSET=$(FS_OFFSET) $(XEN) -DXEN_OFFSET=$(XEN_OFFSET) -DKERNEL=$(KERNEL_IMAGE) -DFILESYSTEM=$(FILESYSTEM) -DTEXT_LIMIT=$(TEXT_LIMIT) -P -C -o $@ $<
> > > >>>  
> > > >>>  fdt.dtb: $(KERNEL_DTB) Makefile
> > > >>> -	( $(DTC) -O dts -I dtb $(KERNEL_DTB) ; echo "/ { $(CHOSEN_NODE) $(PSCI_NODE) }; $(CPU_NODES)" ) | $(DTC) -O dtb -o $@ -
> > > >>> +	( $(DTC) -O dts -I dtb $(KERNEL_DTB) ; echo "/ { $(CHOSEN_NODE) $(PSCI_NODE) }; $(CPU_NODES)" ) | $(DTC) -O dtb -o $@ -Wno-clocks_property -Wno-gpios_property -
> > > >>>  
> > > >>>  # The filesystem archive might not exist if INITRD is not being used
> > > >>>  .PHONY: all clean $(FILESYSTEM)
> > > >>>     
> > > >>
> > > >> dtc 1.4.1 complains  
> > > > 
> > > > Which distro ships this version? (distrowatch doesn't list dtc)
> > > > 
> > > > tag v1.4.1
> > > > Tagger: David Gibson <david at gibson.dropbear.id.au>
> > > > Date:   Wed Nov 12 14:31:44 2014 +1100
> > > > 
> > > > Any chance it's just you and you can update this? It looks like the
> > > > first version to support it is 1.4.5, as shipped for instance with
> > > > Ubuntu 18.04.  
> > > 
> > > It is shipped as LSF module. I can try to ask for an update, but I thought
> > > that other people may run into it as well...
> > >   
> > > >   
> > > >> FATAL ERROR: Unrecognized check name "clocks_property"  
> > > > 
> > > > Sigh, thanks for the heads up. I don't know if we want to blow up the
> > > > Makefile with a feature test?  
> > > 
> > > I dunno, TBH. It look like warning used to be less evil than error...  
> 
> Yeah, that's what I meant: Either revert it or extend the Makefile.
> 
> > I agree.
> > 
> > My preference would be to revert that for now, and consider the problem afresh.
> > Andre, are you ok with that?
> 
> Sure, I don't want to break the build for people.
> I think kvmtool has some lightweight feature tests in its Makefile, I can
> try to steal some of it, and see how evil it looks. Or wait for half a
> year to see those older dtcs flushed out and try it again ;-)

FWIW, the feature test idea doesn't sound bad, though I beleive that for
licensing reasons we cannot take that from kvmtool and would have to grow our
own.

Another option would be to extend FDT.pm to do the manipulation and drop the
dtc dependency entirely. That would require more work, but might make it easier
to do other DTB manipulation in future.

For now I've applied a revert, with a link to this thread and some wording that
we can reconsider this in future.

Thanks,
Mark.