On Wed, Aug 14, 2013 at 12:03:34PM +0200, Alexander Graf wrote:
>
> On 14.08.2013, at 11:56, Edgar E. Iglesias wrote:
>
> > On Wed, Aug 14, 2013 at 11:51:19AM +0200, Alexander Graf wrote:
> >>
> >> On 13.08.2013, at 13:09, Efimov Vasily wrote:
> >>
> >>>
> >>> Signed-off-by: Efimov Vasily <r...@ispras.ru>
> >>
> >> Please provide a patch description :).
> >>
> >>> ---
> >>> hw/ppc/virtex_ml507.c | 13 ++++++++++---
> >>> 1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> >>> index 08e77fb..a00f709 100644
> >>> --- a/hw/ppc/virtex_ml507.c
> >>> +++ b/hw/ppc/virtex_ml507.c
> >>> @@ -141,11 +141,18 @@ static int xilinx_load_device_tree(hwaddr addr,
> >>> {
> >>> char *path;
> >>> int fdt_size;
> >>> - void *fdt;
> >>> + void *fdt = 0;
> >>
> >> This should be NULL. NULL doesn't have to be 0 according to C IIRC.
> >>
> >>> int r;
> >>> + const char *dtb_filename;
> >>>
> >>> - /* Try the local "ppc.dtb" override. */
> >>> - fdt = load_device_tree("ppc.dtb", &fdt_size);
> >>> + dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> >>> + if (dtb_filename) {
> >>> + fdt = load_device_tree(dtb_filename, &fdt_size);
> >>> + }
> >>> + if (!fdt) {
> >>> + /* Try the local "ppc.dtb" override. */
> >>> + fdt = load_device_tree("ppc.dtb", &fdt_size);
> >>> + }
> >>
> >> Could you please just remove the ppc.dtb override option? It's superfluous
> >> once we have proper -dtb support.
> >>
> >> Edgar, any objections?
> >
> > Hi,
> >
> > No objections from my side, I tested the patch and it works fine.
> > I'd prefer to keep the ppc.dtb fallback for backwards compatibility,
> > for example the test image on the wiki relies on it.
>
> Ah, ok. Then let's make the logic work like this:
>
> if (user provided -dtb) {
> if (load_dtb(user provided dtb)) {
> abort();
> }
> } else {
> if (load_dtb("ppc.dtb")) {
> if (load_dtb(find_file(BINARY_DEVICE_TREE_FILE))) {
> abort();
> }
> }
> }
Hi Alex,
I think that we should only abort() on the -dtb arg case. The other
cases are for backwards compatibility.
The dtb used to be optional (useful when for example when running
kernels with a builtin dtb) hence the lack of aborts.
Except from that, your suggestion sounds good to me.
Best regards,
Edgar
