On Thu, May 13, 2021 at 05:26:37PM +0200, Paolo Bonzini wrote:
> On 13/05/21 05:46, David Gibson wrote:
> > > The patch makes sense in general.  The file is only needed for pseries and
> > > powernv, not for e.g. e500 which does need fdt.
> > 
> > Yes, agreed.
> > 
> > > I would get rid of FDT_PPC completely.  First, before patch 3, you can move
> > > fdt.c to PSERIES and POWERNV (it's too small to need its own Kconfig symbol)
> > > and only leave
> > > 
> > >     ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: fdt)
> > 
> > Uh... why do we need even this?
> 
> To tell Meson that a board requires QEMU to be linked with libfdt.  This
> symbol is then renamed to CONFIG_LIBFDT once it can be used with all targets
> (rather than just hw/ppc).

Oh, I thought CONFIG_LIBFDT already did that.

> > > Since you are at it, remove the silly #ifdef TARGET_PPC64 in the
> > > hw/ppc/fdt.c file.
> > > 
> > > Then in patch 3:
> > > 
> > > - add to Kconfig.host
> > > 
> > >      config FDT
> > >         bool
> > > 
> > >      config LIBFDT
> > >         bool
> > >         depends on FDT
> > 
> > Um.. I'm not sure what semantic difference you're envisaging between
> > FDT and LIBFDT.
> 
> "FDT" is set by meson.build if the library is available, LIBFDT is set by
> the board to link with the library.  In other words CONFIG_FDT is per-build,
> while CONFIG_LIBFDT is per-target.

Oof... that's highly non-obvious.  Could we call it HAVE_LIBFDT and
USE_LIBFDT instead?

> If a board selects LIBFDT but the library is not available, minikconf will
> report a contradiction due to "CONFIG_PSERIES=y" -> "config PSERIES select
> LIBFDT" -> "config LIBFDT depends on FDT" ->  "CONFIG_FDT=n".
> 
> > > - for all the boards I listed in my review, add "select LIBFDT" in addition
> > > to "depends on FDT".
> 
> This is actually unnecessary---"depends on FDT" is not needed in the boards
> because LIBFDT already has the dependency.
> 
> Paolo
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson