Re: [Qemu-devel] [PATCH target-arm v4 04/16] arm: Introduce Xilinx ZynqMP SoC

[Peter Maydell]

On 23 April 2015 at 20:21, Peter Crosthwaite
<address@hidden> wrote:
> On Thu, Apr 23, 2015 at 10:42 AM, Peter Maydell
> <address@hidden> wrote:
>> On 23 March 2015 at 11:05, Peter Crosthwaite
>> <address@hidden> wrote:
>>> --- a/hw/arm/Makefile.objs
>>> +++ b/hw/arm/Makefile.objs
>>> @@ -10,3 +10,4 @@ obj-$(CONFIG_DIGIC) += digic.o
>>>  obj-y += omap1.o omap2.o strongarm.o
>>>  obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
>>>  obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>>> +obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o
>>
>> Can this be a common-obj- ?
>>
>
> Seems to build fine. I'll make this change. It's inconsistent with
> surrounding code so I guess this is a new policy?

Historically this makefile's objects were only built for
ARM targets anyway, so it didn't make much difference.
Now we have aarch64 it avoids building a .o twice, so it
helps to reduce build time where we have source files which
don't actually care about which target CPU they're built for.
In other makefiles we make more effort to keep things in
common-obj. (The usual sticking point is use of functions
like "load/store to memory in target CPU endianness" or
caring about target CPU page size.)

If the topic hadn't happened to come to my attention elsewhere
already this week I'd probably not have noticed it in this
patch :-)

>> We don't abbreviate 'Xilinx' in existing type and function
>> names, so it's a bit inconsistent to do so here.
>>
>
> I want to fix this policy going forward as I am matching the device
> tree vendor code and the 2 less chars does save on a few 80 char wraps
> (particularly on lines with more than one "xilinx"). The existing code
> in tree is already self inconsistent between C variable names and
> literal string in this matter, e,g:
>
> hw/char/xilinx_uartlite.c:49:#define TYPE_XILINX_UARTLITE "xlnx.xps-uartlite"
> hw/char/xilinx_uartlite.c:221:
> "xlnx.xps-uartlite", R_MAX * 4);
> hw/dma/xilinx_axidma.c:36:#define TYPE_XILINX_AXI_DMA "xlnx.axi-dma"
> hw/dma/xilinx_axidma.c:601:                          "xlnx.axi-dma",
> R_MAX * 4 * 2);
> hw/intc/xilinx_intc.c:40:#define TYPE_XILINX_INTC "xlnx.xps-intc"
> hw/intc/xilinx_intc.c:171:    memory_region_init_io(&p->mmio, obj,
> &pic_ops, p, "xlnx.xps-intc",
>
> With a mix of XILINX and "xlnx". I think "xlnx' is universally correct
> going forward.

OK.

>> I don't think our QOM error handling is so ugly as to justify
>> hiding it behind a macro (particularly not one with hidden
>> control flow).
>>
>
> Fixing. This is discussed on list so I guess I can join that
> discussion. It is a little verbose having every 1 line API call have 4
> lines or error handling.

Mmm. However if we want to provide a better approach to this I
think it needs to be provided in common code so we can use it
consistently across the codebase, rather than as a local macro hack.

-- PMM