From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9YrP-00073q-Sl for qemu-devel@nongnu.org; Wed, 14 Aug 2013 07:03:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V9YrK-0004DF-A9 for qemu-devel@nongnu.org; Wed, 14 Aug 2013 07:03:27 -0400 Date: Wed, 14 Aug 2013 13:03:12 +0200 From: "Edgar E. Iglesias" Message-ID: <20130814110312.GH4011@smtp.vpn> References: <1376392181-30110-1-git-send-email-real@ispras.ru> <305EDCD1-E38D-4953-9935-9DAADFFEDD90@suse.de> <20130814095643.GG4011@smtp.vpn> <6BBCC8BD-4921-43FA-943A-325104B45F24@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6BBCC8BD-4921-43FA-943A-325104B45F24@suse.de> Subject: Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, Efimov Vasily , qemu-devel@nongnu.org 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 > >> > >> 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