>>>>> "Andreas" == Andreas Färber <afaer...@suse.de> writes:
Andreas> Hello Peter, Am 06.06.2012 05:47, schrieb Peter Chubb: >> There are no major changes since last time, just rebased to current >> tip now that QEMU 1.2 is open. >> >> For those who have come into the story late, this is a series of >> patches to allow QEMU to emulate a Freescale i.MX31 on a Kyoto >> Microsystems evaluation board. It's pretty bare-bones, but runs >> Linux and seL4 nicely. Andreas> Something is wrong with the patch submission, they are shown Andreas> as attachments and Thunderbird doesn't let me comment Andreas> inline. Please use git-send-email to submit. This is a known issue with some thunderbird versions --- Mozilla/Mime support is a bit weird when it comes to emails with a Content-Disposition: inline with a filename. Andreas> Also the diffstat doesn't match the patch wrt file ordering, Andreas> so it would be advisable to use git-format-patch. Andreas> Our concept of topics in the subject seems to be troubling Andreas> you, you have added " support" since v7 (still readable, so Andreas> I'm okay) whereas what we usually use is a lower-case tag as Andreas> described here plus an active description of what it's doing Andreas> (e.g., "foo: Add/Introduce bar"): Andreas> https://live.gnome.org/Git/CommitMessages This series was broken in a couple of ways. Please ignore it! (my laptop got restored from a backup and in moving forward again I think some changes that were in v7 got lost, including this one). Andreas> On patch 5: Please use cpu_arm_init() in place of cpu_init() Andreas> and prefer ARMCPU. This is already in qemu.git and many more Andreas> conversions are in the QOM CPUState part 3 PULL and on Andreas> qom-next. I dunno what happened there. I put that in, then the result wouldn;t compile, so I reverted it. Andreas> Also note that arm_pic_init_cpu() and arm_load_kernel() are Andreas> being changed to take that ARMCPU, so this needs to be Andreas> coordinated. OK. Andreas> Please remove the semicolon after machine_init() and check Andreas> the indentation. After sysbus_create_varargs() for instance I Andreas> spot one space too few and below for imx_serial_create() Andreas> there's a double space. OK. Dunno why checkpatch didn't catch this. Andreas> Please also make all your TypeInfos static const, probably Andreas> same for QEMUMachine. OK. Andreas> The description sounds misleading: In qemu-system-arm all Andreas> boards are ARM architecture, and your wording may sound as if Andreas> the board were from ARM (Holdings plc) when it is from Kyoto Andreas> Micro and uses a Freescale SoC. Maybe also mention the exact Andreas> board name from the commit message and mention i.MX31? OK. Andreas> s/I.MX31/i.MX31/ in the header? Ok. Andreas> On patch 4: There's an empty line after type_init(), please Andreas> remove. OK. Andreas> On patch 3: In IMXTimerGState you're saving DeviceState Andreas> *ccm. That should probably become a QOM link<> property, Andreas> possibly after the initial submission. CC'ing Paolo. I'm not sure what this means. Andreas> What should be considered as a second step is factoring out Andreas> all the devices individually added to the board into an Andreas> i.MX31 SoC, which has implications on how the devices are Andreas> initialized (compare my recent prep_pci and Anthony's i440fx Andreas> patches). For a less sophisticated way using functions check Andreas> out exynos4210. The way it's done right now there's no Andreas> distinction of what is on the SoC and what is on the board, Andreas> so it needs to be done by you. Let's get the first round in first. Andreas> Missing is an entry to MAINTAINERS for this board and its Andreas> exclusive devices, naming who is to be cc'ed on patches. OK. -- Dr Peter Chubb peter.chubb AT nicta.com.au http://www.ssrg.nicta.com.au Software Systems Research Group/NICTA