Re: [Qemu-devel] [PATCH target-arm v1 00/15] Next Generation Xilinx Zynq SoC

[Alistair Francis]

On Tue, Mar 3, 2015 at 9:05 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Mar 2, 2015 at 2:53 PM, Alistair Francis
> <address@hidden> wrote:
>> On Tue, Mar 3, 2015 at 6:06 AM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> On Thu, Feb 26, 2015 at 7:38 PM, Alistair Francis
>>> <address@hidden> wrote:
>>>> On Tue, Feb 24, 2015 at 9:04 AM, Peter Crosthwaite
>>>> <address@hidden> wrote:
>>>>> Hi Peter and all,
>>>>>
>>>>> Xilinx's next gen SoC has been announced. This series adds a SoC and
>>>>> machine model.
>>>>>
>>>>> Series start with addition of ARM cortex A53 support (P1 and P2). The
>>>>> Soc skeleton is then added with GIC, EMACs and UARTs are added. The
>>>>> pre-existing models for GEM and UART are not SoC friendly (no visible
>>>>> state struct), so those are refactored for SoC.
>>>>>
>>>>> Create a generic machine model that exposes just the RAW SoC itself. The
>>>>> only external device modelled is DDR RAM, as driven by the -m option.
>>>>> The standard bootloader and PSCI support is used.
>>>>>
>>>>> Regards,
>>>>> Peter
>>>>>
>>>>>
>>>>> Peter Crosthwaite (15):
>>>>>   target-arm: cpu64: Factor out ARM cortex init
>>>>>   target-arm: cpu64: Add support for cortex-a53
>>>>>   arm: Introduce Xilinx Zynq MPSoC
>>>>>   arm: xlnx-zynq-mp: Add GIC
>>>>>   arm: xlnx-zynq-mp: Connect CPU Timers to GIC
>>>>>   net: cadence_gem: Clean up variable names
>>>>>   net: cadence_gem: Split state struct and type into header
>>>>>   arm: xilinx-zynq-mp: Add GEM support
>>>>>   char: cadence_uart: Clean up variable names
>>>>>   char: cadence_uart: Split state struct and type into header
>>>>>   arm: xilinx-zynq-mp: Add UART support
>>>>>   arm: Add xilinx-zynq-mp-generic machine
>>>>>   arm: xilinx-zynq-mp-generic: Add external RAM
>>>>>   arm: xilinx-zynq-mp-generic: Add bootloading
>>>>>   arm: xlnx-zynq-mp: Add PSCI setup
>>>>>
>>>>>  default-configs/aarch64-softmmu.mak |   2 +-
>>>>>  hw/arm/Makefile.objs                |   1 +
>>>>>  hw/arm/xlnx-zynq-mp-generic.c       |  67 +++++++++++++++
>>>>>  hw/arm/xlnx-zynq-mp.c               | 167 
>>>>> ++++++++++++++++++++++++++++++++++++
>>>>
>>>> These two files need to be renamed. You can' tell what each one is
>>>> from the title.
>>
>> Just re-read this, sorry about the 'need to be'.
>>
>>>>
>>>
>>> So the unqualified "zynqmp" should imply the SoC. This currently
>>> applies to allwinner a10 and digic so Im trying to follow that
>>> convention. When we start using specific boards, the zynqmp will be
>>
>> I see your point, but Allwinner only have one file in the hw/arm/ folder
>> that starts with 'allwinner', making it much clearer what it is. Digic and
>> Exynos also have the '_boards' suffix on their boards, which separates
>> the SoC from the board/boards.
>>
>> With that logic the 'xlnx-zynq-mp-generic.c' probably should have a
>> 'board' in there somewhere.
>>
>>> dropped from the name completely. We could instead use the specific
>>> board name ep108 but I wanted that to be additive. The board would be
>>> called:
>>>
>>> hw/arm/xlnx-ep108.c
>>>
>>> by current conventions. One alternative would be just go out with
>>> ep108 board only and no generic machine if you think that works
>>> better?
>>
>> That naming is clearer, but only if the user knows what the EP108 is.
>> If they are just looking to 'boot Xilinx's ZynqMP chip' then it might be a
>> bit confusing. In saying that, I don't think it's a big issue and I would
>> be fine with having a 'xlnx-ep108.c' board
>>
>
> Thinking we just go with EP108? That is what Michal has done with the
> Linux kernel.

I think that is probably the best option. Especially if it is what we are doing
with the Linux kernel.

>
>>>
>>>> It should be something like 'xilinx_zynqmp_soc.c' and
>>>> 'xilinx_zynqmp_machine.c' (or maybe just 'xilinx_zynqmp.c').
>>>>
>>>
>>> I'm trying to start using the truncated vendor name xlnx instead of
>>> xilinx going forward (there is already in-tree precedent) as the 3
>>> saved chars does save on a few 80 char line wraps. This is also the
>>> convention used in device tree compatible strings.
>>
>> Ok, makes sense. Is it possible to change the current 'xilinx_zynq.c'
>> file then, that way everything matches? I guess that could break current
>> implementations though (epically if the actual machine name changes).
>>
>
> We could make this change to all the variable, fn and file names for
> consistency, but we would not be able to change the machine name (or
> any other user visible strings).

We should probably start to do (it probably can wait till once this set is
accepted). That way all it will be as consistent as possible.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> That way it also matches the same style as the current Zynq machine.
>>>>
>>>> Thanks,
>>>>
>>>> Alistair
>>>>
>>>>>  hw/char/cadence_uart.c              | 113 ++++++++++--------------
>>>>>  hw/net/cadence_gem.c                |  95 ++++++--------------
>>>>>  include/hw/arm/xlnx-zynq-mp.h       |  29 +++++++
>>>>>  include/hw/char/cadence_uart.h      |  35 ++++++++
>>>>>  include/hw/net/cadence_gem.h        |  49 +++++++++++
>>>>>  target-arm/cpu64.c                  |  47 +++++++---
>>>>>  10 files changed, 456 insertions(+), 149 deletions(-)
>>>>>  create mode 100644 hw/arm/xlnx-zynq-mp-generic.c
>>>>>  create mode 100644 hw/arm/xlnx-zynq-mp.c
>>>>>  create mode 100644 include/hw/arm/xlnx-zynq-mp.h
>>>>>  create mode 100644 include/hw/char/cadence_uart.h
>>>>>  create mode 100644 include/hw/net/cadence_gem.h
>>>>>
>>>>> --
>>>>> 2.3.0.1.g27a12f1
>>>>>
>>>>>
>>>>
>>>
>>
>