[Qemu-devel] [PATCH target-arm v4 00/16] Next Generation Xilinx Zynq SoC

[Qemu-devel] [PATCH target-arm v4 00/16] Next Generation Xilinx Zynq SoC

[Peter Crosthwaite]

Hi Peter and all,

Xilinx's next gen SoC has been announced. This series adds a SoC and
board.

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 model of the EP108 board. Currently this doesn't have any
EP108 specific features but is a usable board exposing the user visible
features of the raw SoC.

changed since v3:
Included CPU thread kick fix
Addressed Alistair review

changed since v2:
Fix CPU child prop adder
Add DTS compat string

changed since v1:
Addressed Alistair review (individual changes on resp. patches)
Changed board name to EP108
Changed naming scheme to "zynqmp" / "ZYNQMP" (Michal review)

Regards,
Peter


Peter Crosthwaite (16):
  cpus: Don't kick un-realized cpus.
  target-arm: cpu64: Factor out ARM cortex init
  target-arm: cpu64: Add support for cortex-a53
  arm: Introduce Xilinx ZynqMP SoC
  arm: xlnx-zynqmp: Add GIC
  arm: xlnx-zynqmp: Connect CPU Timers to GIC
  net: cadence_gem: Clean up variable names
  net: cadence_gem: Split state struct and type into header
  arm: xilinx-zynqmp: Add GEM support
  char: cadence_uart: Clean up variable names
  char: cadence_uart: Split state struct and type into header
  arm: xilinx-zynqmp: Add UART support
  arm: Add xlnx-ep108 machine
  arm: xilinx-ep108: Add external RAM
  arm: xilinx-ep108: Add bootloading
  arm: xlnx-zynqmp: Add PSCI setup

 cpus.c                              |   2 +-
 default-configs/aarch64-softmmu.mak |   2 +-
 hw/arm/Makefile.objs                |   1 +
 hw/arm/xlnx-ep108.c                 |  82 ++++++++++++++++++
 hw/arm/xlnx-zynqmp.c                | 168 ++++++++++++++++++++++++++++++++++++
 hw/char/cadence_uart.c              | 115 ++++++++++--------------
 hw/net/cadence_gem.c                |  95 ++++++--------------
 include/hw/arm/xlnx-zynqmp.h        |  29 +++++++
 include/hw/char/cadence_uart.h      |  35 ++++++++
 include/hw/net/cadence_gem.h        |  49 +++++++++++
 target-arm/cpu64.c                  |  50 ++++++++---
 11 files changed, 476 insertions(+), 152 deletions(-)
 create mode 100644 hw/arm/xlnx-ep108.c
 create mode 100644 hw/arm/xlnx-zynqmp.c
 create mode 100644 include/hw/arm/xlnx-zynqmp.h
 create mode 100644 include/hw/char/cadence_uart.h
 create mode 100644 include/hw/net/cadence_gem.h

-- 
2.3.1.2.g90df61e.dirty

[Qemu-devel] [PATCH target-arm v4 01/16] cpus: Don't kick un-realized cpus.

[Peter Crosthwaite]

following a464982499b2f637f6699e3d03e0a9d2e0b5288b, it's now possible for
there to be attempts to take the BQL before CPUs have been realized in
cases where a machine model inits peripherals before the first CPU.

BQL lock aquisition kicks the first_cpu, leading to a segfault if this
happens pre-realize. Guard the CPU kick routine to perform no action for
a CPU that doesn't exist or doesn't have a thread yet.

There was a fix to this with commit
6b49809c597331803ea941eadda813e5bb4e8fe2, but the check there misses
the case where the CPU has been inited and not realized. Strengthen the
check to make sure that the first_cpu has a thread (i.e. it is
realized) before allowing the kick.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 314df16..e6dcae3 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1119,7 +1119,7 @@ bool qemu_in_vcpu_thread(void)
 void qemu_mutex_lock_iothread(void)
 {
     atomic_inc(&iothread_requesting_mutex);
-    if (!tcg_enabled() || !first_cpu) {
+    if (!tcg_enabled() || !first_cpu || !first_cpu->thread) {
         qemu_mutex_lock(&qemu_global_mutex);
         atomic_dec(&iothread_requesting_mutex);
     } else {
-- 
2.3.1.2.g90df61e.dirty

[Qemu-devel] [PATCH target-arm v4 03/16] target-arm: cpu64: Add support for cortex-a53

[Peter Crosthwaite]

Similar to a53, but with different L1 I cache policy, phys addr size and
different cache geometries. The cache sizes is implementation
configurable, but use these values (from Xilinx MPSoC) as a default
until cache size configurability is added.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed since v2:
Added dtb compatible string

 target-arm/cpu64.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 3eb58c6..728d9a7 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -149,6 +149,21 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->ccsidr[2] = 0x70ffe07a; /* 2048KB L2 cache */
 }
 
+static void aarch64_a53_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    aarch64_axx_initfn(cpu);
+
+    cpu->dtb_compatible = "arm,cortex-a53";
+    cpu->midr = 0x410fd034;
+    cpu->ctr = 0x84448004; /* L1Ip = VIPT */
+    cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
+    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
+    cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
+    cpu->ccsidr[2] = 0x707fe07a; /* 1024KB L2 cache */
+}
+
 #ifdef CONFIG_USER_ONLY
 static void aarch64_any_initfn(Object *obj)
 {
@@ -176,6 +191,7 @@ typedef struct ARMCPUInfo {
 
 static const ARMCPUInfo aarch64_cpus[] = {
     { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
+    { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = aarch64_any_initfn },
 #endif
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 03/16] target-arm: cpu64: Add support for cortex-a53

[Ryota Ozaki]

Hi,

On Mon, Mar 23, 2015 at 8:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Similar to a53, but with different L1 I cache policy, phys addr size and
             ^^^
I guess      a57 :)

  ozaki-r

> different cache geometries. The cache sizes is implementation
> configurable, but use these values (from Xilinx MPSoC) as a default
> until cache size configurability is added.
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Changed since v2:
> Added dtb compatible string
>
>  target-arm/cpu64.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index 3eb58c6..728d9a7 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -149,6 +149,21 @@ static void aarch64_a57_initfn(Object *obj)
>      cpu->ccsidr[2] = 0x70ffe07a; /* 2048KB L2 cache */
>  }
>
> +static void aarch64_a53_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    aarch64_axx_initfn(cpu);
> +
> +    cpu->dtb_compatible = "arm,cortex-a53";
> +    cpu->midr = 0x410fd034;
> +    cpu->ctr = 0x84448004; /* L1Ip = VIPT */
> +    cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
> +    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
> +    cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
> +    cpu->ccsidr[2] = 0x707fe07a; /* 1024KB L2 cache */
> +}
> +
>  #ifdef CONFIG_USER_ONLY
>  static void aarch64_any_initfn(Object *obj)
>  {
> @@ -176,6 +191,7 @@ typedef struct ARMCPUInfo {
>
>  static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
> +    { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
>  #ifdef CONFIG_USER_ONLY
>      { .name = "any",         .initfn = aarch64_any_initfn },
>  #endif
> --
> 2.3.1.2.g90df61e.dirty
>

Re: [Qemu-devel] [PATCH target-arm v4 03/16] target-arm: cpu64: Add support for cortex-a53

[Peter Crosthwaite]

On Mon, Mar 23, 2015 at 6:17 AM, Ryota Ozaki <ozaki.ryota@gmail.com> wrote:
> Hi,
>
> On Mon, Mar 23, 2015 at 8:05 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Similar to a53, but with different L1 I cache policy, phys addr size and
>              ^^^
> I guess      a57 :)
>

Fixed. Thanks.

Regards,
Peter

[Qemu-devel] [PATCH target-arm v4 02/16] target-arm: cpu64: Factor out ARM cortex init

[Peter Crosthwaite]

In preparation for support for Cortex a53. Use "axx" to describe the
shareable features. Some of the CP15 registers (such as ACTLR) are
specific to implementation, but we currently just RAZ them so continue
with that as the policy for all cortex A processors under a shared
definition.

The cache sizes and geometeries, the L1 I-cache policy and the physical
address range differ between A53 and A57 so those particulars are left
as A57 specific. The rest are moved to the generalisation.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu64.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 270bc2f..3eb58c6 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -38,22 +38,22 @@ static inline void unset_feature(CPUARMState *env, int feature)
 }
 
 #ifndef CONFIG_USER_ONLY
-static uint64_t a57_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+static uint64_t axx_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     /* Number of processors is in [25:24]; otherwise we RAZ */
     return (smp_cpus - 1) << 24;
 }
 #endif
 
-static const ARMCPRegInfo cortexa57_cp_reginfo[] = {
+static const ARMCPRegInfo cortexaxx_cp_reginfo[] = {
 #ifndef CONFIG_USER_ONLY
     { .name = "L2CTLR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 1, .crn = 11, .crm = 0, .opc2 = 2,
-      .access = PL1_RW, .readfn = a57_l2ctlr_read,
+      .access = PL1_RW, .readfn = axx_l2ctlr_read,
       .writefn = arm_cp_write_ignore },
     { .name = "L2CTLR",
       .cp = 15, .opc1 = 1, .crn = 9, .crm = 0, .opc2 = 2,
-      .access = PL1_RW, .readfn = a57_l2ctlr_read,
+      .access = PL1_RW, .readfn = axx_l2ctlr_read,
       .writefn = arm_cp_write_ignore },
 #endif
     { .name = "L2ECTLR_EL1", .state = ARM_CP_STATE_AA64,
@@ -92,11 +92,8 @@ static const ARMCPRegInfo cortexa57_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
-static void aarch64_a57_initfn(Object *obj)
+static void aarch64_axx_initfn(ARMCPU *cpu)
 {
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    cpu->dtb_compatible = "arm,cortex-a57";
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
@@ -108,13 +105,10 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
     set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
     set_feature(&cpu->env, ARM_FEATURE_CRC);
-    cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
-    cpu->midr = 0x411fd070;
     cpu->reset_fpsid = 0x41034070;
     cpu->mvfr0 = 0x10110222;
     cpu->mvfr1 = 0x12111111;
     cpu->mvfr2 = 0x00000043;
-    cpu->ctr = 0x8444c004;
     cpu->reset_sctlr = 0x00c50838;
     cpu->id_pfr0 = 0x00000131;
     cpu->id_pfr1 = 0x00011011;
@@ -133,14 +127,26 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
     cpu->id_aa64isar0 = 0x00011120;
-    cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
+    cpu->dcz_blocksize = 4; /* 64 bytes */
+    define_arm_cp_regs(cpu, cortexaxx_cp_reginfo);
+}
+
+static void aarch64_a57_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    aarch64_axx_initfn(cpu);
+
+    cpu->dtb_compatible = "arm,cortex-a57";
+    cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
+    cpu->midr = 0x411fd070;
+    cpu->ctr = 0x8444c004; /* L1Ip = PIPT */
+    cpu->id_aa64mmfr0 = 0x00001124; /* 44 bit physical addr */
     cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
     cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
     cpu->ccsidr[2] = 0x70ffe07a; /* 2048KB L2 cache */
-    cpu->dcz_blocksize = 4; /* 64 bytes */
-    define_arm_cp_regs(cpu, cortexa57_cp_reginfo);
 }
 
 #ifdef CONFIG_USER_ONLY
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 02/16] target-arm: cpu64: Factor out ARM cortex init

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> In preparation for support for Cortex a53. Use "axx" to describe the
> shareable features. Some of the CP15 registers (such as ACTLR) are
> specific to implementation, but we currently just RAZ them so continue
> with that as the policy for all cortex A processors under a shared
> definition.
>
> The cache sizes and geometeries, the L1 I-cache policy and the physical
> address range differ between A53 and A57 so those particulars are left
> as A57 specific. The rest are moved to the generalisation.

I'm not a huge fan of this as it seems unlikely that every CPU
with a "Cortex-Axx" is going to look so similar to the A57.

Call the reginfo table cortex_a53_a57_cp_reginfo if you want
to share it, but I would leave the initfns completely separate
personally.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 02/16] target-arm: cpu64: Factor out ARM cortex init

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:35 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> In preparation for support for Cortex a53. Use "axx" to describe the
>> shareable features. Some of the CP15 registers (such as ACTLR) are
>> specific to implementation, but we currently just RAZ them so continue
>> with that as the policy for all cortex A processors under a shared
>> definition.
>>
>> The cache sizes and geometeries, the L1 I-cache policy and the physical
>> address range differ between A53 and A57 so those particulars are left
>> as A57 specific. The rest are moved to the generalisation.
>
> I'm not a huge fan of this as it seems unlikely that every CPU
> with a "Cortex-Axx" is going to look so similar to the A57.
>
> Call the reginfo table cortex_a53_a57_cp_reginfo if you want
> to share it, but I would leave the initfns completely separate
> personally.
>

Both fixed.

Regards,
Peter

> -- PMM
>

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

[Peter Crosthwaite]

With quad Cortex-A53 CPUs.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v2:
Added [*] to cpu child property name.
changed since v1:
Add &error_abort to CPU child adder call.

 default-configs/aarch64-softmmu.mak |  2 +-
 hw/arm/Makefile.objs                |  1 +
 hw/arm/xlnx-zynqmp.c                | 72 +++++++++++++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h        | 21 +++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/xlnx-zynqmp.c
 create mode 100644 include/hw/arm/xlnx-zynqmp.h

diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 6d3b5c7..96dd994 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -3,4 +3,4 @@
 # We support all the 32 bit boards so need all their config
 include arm-softmmu.mak
 
-# Currently no 64-bit specific config requirements
+CONFIG_XLNX_ZYNQMP=y
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 2577f68..d7cd5f4 100644
--- 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
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
new file mode 100644
index 0000000..41c207a
--- /dev/null
+++ b/hw/arm/xlnx-zynqmp.c
@@ -0,0 +1,72 @@
+/*
+ * Xilinx Zynq MPSoC emulation
+ *
+ * Copyright (C) 2015 Xilinx Inc
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "hw/arm/xlnx-zynqmp.h"
+
+static void xlnx_zynqmp_init(Object *obj)
+{
+    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
+    int i;
+
+    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
+        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
+                          "cortex-a53-" TYPE_ARM_CPU);
+        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
+                                  &error_abort);
+    }
+}
+
+#define ERR_PROP_CHECK_RETURN(err, errp) do { \
+    if (err) { \
+        error_propagate((errp), (err)); \
+        return; \
+    } \
+} while (0)
+
+static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
+{
+    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
+    uint8_t i;
+    Error *err = NULL;
+
+    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
+        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
+        ERR_PROP_CHECK_RETURN(err, errp);
+    }
+}
+
+static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = xlnx_zynqmp_realize;
+}
+
+static const TypeInfo xlnx_zynqmp_type_info = {
+    .name = TYPE_XLNX_ZYNQMP,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(XlnxZynqMPState),
+    .instance_init = xlnx_zynqmp_init,
+    .class_init = xlnx_zynqmp_class_init,
+};
+
+static void xlnx_zynqmp_register_types(void)
+{
+    type_register_static(&xlnx_zynqmp_type_info);
+}
+
+type_init(xlnx_zynqmp_register_types)
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
new file mode 100644
index 0000000..d6b3b92
--- /dev/null
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -0,0 +1,21 @@
+#ifndef XLNX_ZYNQMP_H_
+
+#include "qemu-common.h"
+#include "hw/arm/arm.h"
+
+#define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
+#define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
+                                       TYPE_XLNX_ZYNQMP)
+
+#define XLNX_ZYNQMP_NUM_CPUS 4
+
+typedef struct XlnxZynqMPState {
+    /*< private >*/
+    DeviceState parent_obj;
+    /*< public >*/
+
+    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
+}  XlnxZynqMPState;
+
+#define XLNX_ZYNQMP_H_
+#endif
-- 
2.3.1.2.g90df61e.dirty

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

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> With quad Cortex-A53 CPUs.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v2:
> Added [*] to cpu child property name.
> changed since v1:
> Add &error_abort to CPU child adder call.
>
>  default-configs/aarch64-softmmu.mak |  2 +-
>  hw/arm/Makefile.objs                |  1 +
>  hw/arm/xlnx-zynqmp.c                | 72 +++++++++++++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h        | 21 +++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/xlnx-zynqmp.c
>  create mode 100644 include/hw/arm/xlnx-zynqmp.h
>
> diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> index 6d3b5c7..96dd994 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -3,4 +3,4 @@
>  # We support all the 32 bit boards so need all their config
>  include arm-softmmu.mak
>
> -# Currently no 64-bit specific config requirements
> +CONFIG_XLNX_ZYNQMP=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2577f68..d7cd5f4 100644
> --- 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
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> new file mode 100644
> index 0000000..41c207a
> --- /dev/null
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -0,0 +1,72 @@
> +/*
> + * Xilinx Zynq MPSoC emulation
> + *
> + * Copyright (C) 2015 Xilinx Inc
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/arm/xlnx-zynqmp.h"
> +
> +static void xlnx_zynqmp_init(Object *obj)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> +    int i;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a53-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
> +                                  &error_abort);
> +    }
> +}
> +
> +#define ERR_PROP_CHECK_RETURN(err, errp) do { \
> +    if (err) { \
> +        error_propagate((errp), (err)); \
> +        return; \
> +    } \
> +} while (0)
> +
> +static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
> +    uint8_t i;
> +    Error *err = NULL;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
> +        ERR_PROP_CHECK_RETURN(err, errp);
> +    }
> +}
> +
> +static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = xlnx_zynqmp_realize;
> +}
> +
> +static const TypeInfo xlnx_zynqmp_type_info = {
> +    .name = TYPE_XLNX_ZYNQMP,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(XlnxZynqMPState),
> +    .instance_init = xlnx_zynqmp_init,
> +    .class_init = xlnx_zynqmp_class_init,
> +};
> +
> +static void xlnx_zynqmp_register_types(void)
> +{
> +    type_register_static(&xlnx_zynqmp_type_info);
> +}
> +
> +type_init(xlnx_zynqmp_register_types)
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> new file mode 100644
> index 0000000..d6b3b92
> --- /dev/null
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -0,0 +1,21 @@
> +#ifndef XLNX_ZYNQMP_H_
> +
> +#include "qemu-common.h"
> +#include "hw/arm/arm.h"
> +
> +#define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
> +#define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
> +                                       TYPE_XLNX_ZYNQMP)
> +
> +#define XLNX_ZYNQMP_NUM_CPUS 4
> +
> +typedef struct XlnxZynqMPState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/

I have the same nit pick here as the others, I would just
space it out differently. That's not important though.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> +
> +    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
> +}  XlnxZynqMPState;
> +
> +#define XLNX_ZYNQMP_H_
> +#endif
> --
> 2.3.1.2.g90df61e.dirty
>
>

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

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> With quad Cortex-A53 CPUs.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v2:
> Added [*] to cpu child property name.
> changed since v1:
> Add &error_abort to CPU child adder call.
>
>  default-configs/aarch64-softmmu.mak |  2 +-
>  hw/arm/Makefile.objs                |  1 +
>  hw/arm/xlnx-zynqmp.c                | 72 +++++++++++++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h        | 21 +++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/xlnx-zynqmp.c
>  create mode 100644 include/hw/arm/xlnx-zynqmp.h
>
> diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> index 6d3b5c7..96dd994 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -3,4 +3,4 @@
>  # We support all the 32 bit boards so need all their config
>  include arm-softmmu.mak
>
> -# Currently no 64-bit specific config requirements
> +CONFIG_XLNX_ZYNQMP=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2577f68..d7cd5f4 100644
> --- 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- ?

> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> new file mode 100644
> index 0000000..41c207a
> --- /dev/null
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -0,0 +1,72 @@
> +/*
> + * Xilinx Zynq MPSoC emulation
> + *
> + * Copyright (C) 2015 Xilinx Inc
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/arm/xlnx-zynqmp.h"
> +
> +static void xlnx_zynqmp_init(Object *obj)

We don't abbreviate 'Xilinx' in existing type and function
names, so it's a bit inconsistent to do so here.

> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> +    int i;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a53-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
> +                                  &error_abort);
> +    }
> +}
> +
> +#define ERR_PROP_CHECK_RETURN(err, errp) do { \
> +    if (err) { \
> +        error_propagate((errp), (err)); \
> +        return; \
> +    } \
> +} while (0)

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).

> +static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
> +    uint8_t i;
> +    Error *err = NULL;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
> +        ERR_PROP_CHECK_RETURN(err, errp);
> +    }
> +}
> +
> +static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = xlnx_zynqmp_realize;
> +}
> +
> +static const TypeInfo xlnx_zynqmp_type_info = {
> +    .name = TYPE_XLNX_ZYNQMP,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(XlnxZynqMPState),
> +    .instance_init = xlnx_zynqmp_init,
> +    .class_init = xlnx_zynqmp_class_init,
> +};
> +
> +static void xlnx_zynqmp_register_types(void)
> +{
> +    type_register_static(&xlnx_zynqmp_type_info);
> +}
> +
> +type_init(xlnx_zynqmp_register_types)
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> new file mode 100644
> index 0000000..d6b3b92
> --- /dev/null
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -0,0 +1,21 @@

This file needs a copyright-and-license comment.

> +#ifndef XLNX_ZYNQMP_H_

No trailing underscore, please.

> +
> +#include "qemu-common.h"
> +#include "hw/arm/arm.h"
> +
> +#define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
> +#define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
> +                                       TYPE_XLNX_ZYNQMP)
> +
> +#define XLNX_ZYNQMP_NUM_CPUS 4
> +
> +typedef struct XlnxZynqMPState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/
> +
> +    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
> +}  XlnxZynqMPState;
> +
> +#define XLNX_ZYNQMP_H_
> +#endif
> --
> 2.3.1.2.g90df61e.dirty
>

thanks
-- PMM

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

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:42 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> With quad Cortex-A53 CPUs.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed since v2:
>> Added [*] to cpu child property name.
>> changed since v1:
>> Add &error_abort to CPU child adder call.
>>
>>  default-configs/aarch64-softmmu.mak |  2 +-
>>  hw/arm/Makefile.objs                |  1 +
>>  hw/arm/xlnx-zynqmp.c                | 72 +++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/xlnx-zynqmp.h        | 21 +++++++++++
>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/arm/xlnx-zynqmp.c
>>  create mode 100644 include/hw/arm/xlnx-zynqmp.h
>>
>> diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
>> index 6d3b5c7..96dd994 100644
>> --- a/default-configs/aarch64-softmmu.mak
>> +++ b/default-configs/aarch64-softmmu.mak
>> @@ -3,4 +3,4 @@
>>  # We support all the 32 bit boards so need all their config
>>  include arm-softmmu.mak
>>
>> -# Currently no 64-bit specific config requirements
>> +CONFIG_XLNX_ZYNQMP=y
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 2577f68..d7cd5f4 100644
>> --- 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?

>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> new file mode 100644
>> index 0000000..41c207a
>> --- /dev/null
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * Xilinx Zynq MPSoC emulation
>> + *
>> + * Copyright (C) 2015 Xilinx Inc
>> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> + * for more details.
>> + */
>> +
>> +#include "hw/arm/xlnx-zynqmp.h"
>> +
>> +static void xlnx_zynqmp_init(Object *obj)
>
> 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.

>> +{
>> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>> +    int i;
>> +
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>> +                          "cortex-a53-" TYPE_ARM_CPU);
>> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>> +                                  &error_abort);
>> +    }
>> +}
>> +
>> +#define ERR_PROP_CHECK_RETURN(err, errp) do { \
>> +    if (err) { \
>> +        error_propagate((errp), (err)); \
>> +        return; \
>> +    } \
>> +} while (0)
>
> 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.

>> +static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
>> +    uint8_t i;
>> +    Error *err = NULL;
>> +
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>> +        ERR_PROP_CHECK_RETURN(err, errp);
>> +    }
>> +}
>> +
>> +static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = xlnx_zynqmp_realize;
>> +}
>> +
>> +static const TypeInfo xlnx_zynqmp_type_info = {
>> +    .name = TYPE_XLNX_ZYNQMP,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(XlnxZynqMPState),
>> +    .instance_init = xlnx_zynqmp_init,
>> +    .class_init = xlnx_zynqmp_class_init,
>> +};
>> +
>> +static void xlnx_zynqmp_register_types(void)
>> +{
>> +    type_register_static(&xlnx_zynqmp_type_info);
>> +}
>> +
>> +type_init(xlnx_zynqmp_register_types)
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> new file mode 100644
>> index 0000000..d6b3b92
>> --- /dev/null
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -0,0 +1,21 @@
>
> This file needs a copyright-and-license comment.
>

Fixed.

>> +#ifndef XLNX_ZYNQMP_H_
>
> No trailing underscore, please.
>

Fixed.

Regards,
Peter

>> +
>> +#include "qemu-common.h"
>> +#include "hw/arm/arm.h"
>> +
>> +#define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>> +#define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>> +                                       TYPE_XLNX_ZYNQMP)
>> +
>> +#define XLNX_ZYNQMP_NUM_CPUS 4
>> +
>> +typedef struct XlnxZynqMPState {
>> +    /*< private >*/
>> +    DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
>> +}  XlnxZynqMPState;
>> +
>> +#define XLNX_ZYNQMP_H_
>> +#endif
>> --
>> 2.3.1.2.g90df61e.dirty
>>
>
> thanks
> -- PMM
>

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
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Apr 23, 2015 at 10:42 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 23 March 2015 at 11:05, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> 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

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

[Peter Maydell]

On 23 April 2015 at 22:38, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 April 2015 at 20:21, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, Apr 23, 2015 at 10:42 AM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 23 March 2015 at 11:05, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> 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

I looked a bit more closely at this, and hw/arm is treated
specially (the hw/$(TARGET_BASE_ARCH) directories are
pulled in directly by Makefile.target and only for obj-y),
so for now new object files in this directory should just be
in obj-. I might see if we can add support for common-obj-y,
but that's a separate patch.

thanks
-- PMM

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

[Peter Crosthwaite]

On Fri, Apr 24, 2015 at 8:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 April 2015 at 22:38, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 23 April 2015 at 20:21, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Thu, Apr 23, 2015 at 10:42 AM, Peter Maydell
>>> <peter.maydell@linaro.org> wrote:
>>>> On 23 March 2015 at 11:05, Peter Crosthwaite
>>>> <peter.crosthwaite@xilinx.com> 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
>
> I looked a bit more closely at this, and hw/arm is treated
> specially (the hw/$(TARGET_BASE_ARCH) directories are
> pulled in directly by Makefile.target and only for obj-y),

Yes, I discovered the same.

> so for now new object files in this directory should just be
> in obj-. I might see if we can add support for common-obj-y,
> but that's a separate patch.
>

Will keep as-is.

Regards,
Peter


> thanks
> -- PMM
>

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

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> +static void xlnx_zynqmp_init(Object *obj)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> +    int i;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a53-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
> +                                  &error_abort);
> +    }
> +}

> +static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
> +    uint8_t i;
> +    Error *err = NULL;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
> +        ERR_PROP_CHECK_RETURN(err, errp);
> +    }
> +}

This seems to be a touch short on CPU property setting.
I would expect you want most or all of:
 * has_el3
 * psci-conduit
 * start-powered-off for the secondaries
 * reset-cbar

(Some of those may be new in master since this series was posted.)

thanks
-- PMM

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

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:47 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> +static void xlnx_zynqmp_init(Object *obj)
>> +{
>> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>> +    int i;
>> +
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>> +                          "cortex-a53-" TYPE_ARM_CPU);
>> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>> +                                  &error_abort);
>> +    }
>> +}
>
>> +static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XlnxZynqMPState *s = XLNX_ZYNQMP(dev);
>> +    uint8_t i;
>> +    Error *err = NULL;
>> +
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>> +        ERR_PROP_CHECK_RETURN(err, errp);
>> +    }
>> +}
>
> This seems to be a touch short on CPU property setting.
> I would expect you want most or all of:
>  * has_el3

has_el3 defines to true by default and the SoC does support EL3.

DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);

>  * psci-conduit
>  * start-powered-off for the secondaries

Squashed in P16.

>  * reset-cbar

Adding to GIC patch.

Regards,
Peter

>
> (Some of those may be new in master since this series was posted.)
>
> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 05/16] arm: xlnx-zynqmp: Add GIC

[Peter Crosthwaite]

And connect IRQ outputs to the CPUs.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c         | 19 +++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 41c207a..9465185 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -17,6 +17,11 @@
 
 #include "hw/arm/xlnx-zynqmp.h"
 
+#define GIC_NUM_SPI_INTR 128
+
+#define GIC_DIST_ADDR       0xf9010000
+#define GIC_CPU_ADDR        0xf9020000
+
 static void xlnx_zynqmp_init(Object *obj)
 {
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
@@ -28,6 +33,9 @@ static void xlnx_zynqmp_init(Object *obj)
         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
                                   &error_abort);
     }
+
+    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
+    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 }
 
 #define ERR_PROP_CHECK_RETURN(err, errp) do { \
@@ -43,9 +51,20 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     uint8_t i;
     Error *err = NULL;
 
+    qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
+    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
+    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
+    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
+    ERR_PROP_CHECK_RETURN(err, errp);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 0, GIC_DIST_ADDR);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
+
     for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
         object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
         ERR_PROP_CHECK_RETURN(err, errp);
+
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
+                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
     }
 }
 
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index d6b3b92..d29c7de 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -2,6 +2,7 @@
 
 #include "qemu-common.h"
 #include "hw/arm/arm.h"
+#include "hw/intc/arm_gic.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
@@ -15,6 +16,7 @@ typedef struct XlnxZynqMPState {
     /*< public >*/
 
     ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
+    GICState gic;
 }  XlnxZynqMPState;
 
 #define XLNX_ZYNQMP_H_
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 05/16] arm: xlnx-zynqmp: Add GIC

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> And connect IRQ outputs to the CPUs.

Can you make sure the body of the commit message
makes sense when read as a standalone text without
the subject line, please?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 05/16] arm: xlnx-zynqmp: Add GIC

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:45 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> And connect IRQ outputs to the CPUs.
>
> Can you make sure the body of the commit message
> makes sense when read as a standalone text without
> the subject line, please?
>

Fixed.

Regards,
Peter

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 07/16] net: cadence_gem: Clean up variable names

[Peter Crosthwaite]

In preparation for migrating the state struct and type cast macro to a public
header. The acronym "GEM" on it's own is not specific enough to be used in a
more global namespace so preface with "cadence". Fix the capitalisation of
"gem" in the state type while touching the typename. Also preface the
GEM_MAXREG macro as this will need to migrate to public header.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/net/cadence_gem.c | 70 ++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 55b6293..5994306 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -141,7 +141,7 @@
 #define GEM_DESCONF6      (0x00000294/4)
 #define GEM_DESCONF7      (0x00000298/4)
 
-#define GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
+#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
 
 /*****************************************/
 #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
@@ -350,9 +350,9 @@ static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
 }
 
 #define TYPE_CADENCE_GEM "cadence_gem"
-#define GEM(obj) OBJECT_CHECK(GemState, (obj), TYPE_CADENCE_GEM)
+#define CADENCE_GEM(obj) OBJECT_CHECK(CadenceGEMState, (obj), TYPE_CADENCE_GEM)
 
-typedef struct GemState {
+typedef struct CadenceGEMState {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
@@ -361,15 +361,15 @@ typedef struct GemState {
     qemu_irq irq;
 
     /* GEM registers backing store */
-    uint32_t regs[GEM_MAXREG];
+    uint32_t regs[CADENCE_GEM_MAXREG];
     /* Mask of register bits which are write only */
-    uint32_t regs_wo[GEM_MAXREG];
+    uint32_t regs_wo[CADENCE_GEM_MAXREG];
     /* Mask of register bits which are read only */
-    uint32_t regs_ro[GEM_MAXREG];
+    uint32_t regs_ro[CADENCE_GEM_MAXREG];
     /* Mask of register bits which are clear on read */
-    uint32_t regs_rtc[GEM_MAXREG];
+    uint32_t regs_rtc[CADENCE_GEM_MAXREG];
     /* Mask of register bits which are write 1 to clear */
-    uint32_t regs_w1c[GEM_MAXREG];
+    uint32_t regs_w1c[CADENCE_GEM_MAXREG];
 
     /* PHY registers backing store */
     uint16_t phy_regs[32];
@@ -385,7 +385,7 @@ typedef struct GemState {
     unsigned rx_desc[2];
 
     bool sar_active[4];
-} GemState;
+} CadenceGEMState;
 
 /* The broadcast MAC address: 0xFFFFFFFFFFFF */
 static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
@@ -395,7 +395,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
  * One time initialization.
  * Set masks to identify which register bits have magical clear properties
  */
-static void gem_init_register_masks(GemState *s)
+static void gem_init_register_masks(CadenceGEMState *s)
 {
     /* Mask of register bits which are read only */
     memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
@@ -430,7 +430,7 @@ static void gem_init_register_masks(GemState *s)
  * phy_update_link:
  * Make the emulated PHY link state match the QEMU "interface" state.
  */
-static void phy_update_link(GemState *s)
+static void phy_update_link(CadenceGEMState *s)
 {
     DB_PRINT("down %d\n", qemu_get_queue(s->nic)->link_down);
 
@@ -450,7 +450,7 @@ static void phy_update_link(GemState *s)
 
 static int gem_can_receive(NetClientState *nc)
 {
-    GemState *s;
+    CadenceGEMState *s;
 
     s = qemu_get_nic_opaque(nc);
 
@@ -483,7 +483,7 @@ static int gem_can_receive(NetClientState *nc)
  * gem_update_int_status:
  * Raise or lower interrupt based on current status.
  */
-static void gem_update_int_status(GemState *s)
+static void gem_update_int_status(CadenceGEMState *s)
 {
     if (s->regs[GEM_ISR]) {
         DB_PRINT("asserting int. (0x%08x)\n", s->regs[GEM_ISR]);
@@ -495,7 +495,7 @@ static void gem_update_int_status(GemState *s)
  * gem_receive_updatestats:
  * Increment receive statistics.
  */
-static void gem_receive_updatestats(GemState *s, const uint8_t *packet,
+static void gem_receive_updatestats(CadenceGEMState *s, const uint8_t *packet,
                                     unsigned bytes)
 {
     uint64_t octets;
@@ -586,7 +586,7 @@ static unsigned calc_mac_hash(const uint8_t *mac)
  * GEM_RM_PROMISCUOUS_ACCEPT, GEM_RX_BROADCAST_ACCEPT,
  * GEM_RX_MULTICAST_HASH_ACCEPT or GEM_RX_UNICAST_HASH_ACCEPT
  */
-static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
+static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
 {
     uint8_t *gem_spaddr;
     int i;
@@ -636,7 +636,7 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet)
     return GEM_RX_REJECT;
 }
 
-static void gem_get_rx_desc(GemState *s)
+static void gem_get_rx_desc(CadenceGEMState *s)
 {
     DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
     /* read current descriptor */
@@ -660,7 +660,7 @@ static void gem_get_rx_desc(GemState *s)
  */
 static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
-    GemState *s;
+    CadenceGEMState *s;
     unsigned   rxbufsize, bytes_to_copy;
     unsigned   rxbuf_offset;
     uint8_t    rxbuf[2048];
@@ -810,7 +810,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
  * gem_transmit_updatestats:
  * Increment transmit statistics.
  */
-static void gem_transmit_updatestats(GemState *s, const uint8_t *packet,
+static void gem_transmit_updatestats(CadenceGEMState *s, const uint8_t *packet,
                                      unsigned bytes)
 {
     uint64_t octets;
@@ -856,7 +856,7 @@ static void gem_transmit_updatestats(GemState *s, const uint8_t *packet,
  * gem_transmit:
  * Fish packets out of the descriptor ring and feed them to QEMU
  */
-static void gem_transmit(GemState *s)
+static void gem_transmit(CadenceGEMState *s)
 {
     unsigned    desc[2];
     hwaddr packet_desc_addr;
@@ -976,7 +976,7 @@ static void gem_transmit(GemState *s)
     }
 }
 
-static void gem_phy_reset(GemState *s)
+static void gem_phy_reset(CadenceGEMState *s)
 {
     memset(&s->phy_regs[0], 0, sizeof(s->phy_regs));
     s->phy_regs[PHY_REG_CONTROL] = 0x1140;
@@ -1004,7 +1004,7 @@ static void gem_phy_reset(GemState *s)
 static void gem_reset(DeviceState *d)
 {
     int i;
-    GemState *s = GEM(d);
+    CadenceGEMState *s = CADENCE_GEM(d);
 
     DB_PRINT("\n");
 
@@ -1032,13 +1032,13 @@ static void gem_reset(DeviceState *d)
     gem_update_int_status(s);
 }
 
-static uint16_t gem_phy_read(GemState *s, unsigned reg_num)
+static uint16_t gem_phy_read(CadenceGEMState *s, unsigned reg_num)
 {
     DB_PRINT("reg: %d value: 0x%04x\n", reg_num, s->phy_regs[reg_num]);
     return s->phy_regs[reg_num];
 }
 
-static void gem_phy_write(GemState *s, unsigned reg_num, uint16_t val)
+static void gem_phy_write(CadenceGEMState *s, unsigned reg_num, uint16_t val)
 {
     DB_PRINT("reg: %d value: 0x%04x\n", reg_num, val);
 
@@ -1072,10 +1072,10 @@ static void gem_phy_write(GemState *s, unsigned reg_num, uint16_t val)
  */
 static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
 {
-    GemState *s;
+    CadenceGEMState *s;
     uint32_t retval;
 
-    s = (GemState *)opaque;
+    s = (CadenceGEMState *)opaque;
 
     offset >>= 2;
     retval = s->regs[offset];
@@ -1120,7 +1120,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
 static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         unsigned size)
 {
-    GemState *s = (GemState *)opaque;
+    CadenceGEMState *s = (CadenceGEMState *)opaque;
     uint32_t readonly;
 
     DB_PRINT("offset: 0x%04x write: 0x%08x ", (unsigned)offset, (unsigned)val);
@@ -1226,7 +1226,7 @@ static NetClientInfo net_gem_info = {
 static int gem_init(SysBusDevice *sbd)
 {
     DeviceState *dev = DEVICE(sbd);
-    GemState *s = GEM(dev);
+    CadenceGEMState *s = CADENCE_GEM(dev);
 
     DB_PRINT("\n");
 
@@ -1248,18 +1248,18 @@ static const VMStateDescription vmstate_cadence_gem = {
     .version_id = 2,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(regs, GemState, GEM_MAXREG),
-        VMSTATE_UINT16_ARRAY(phy_regs, GemState, 32),
-        VMSTATE_UINT8(phy_loop, GemState),
-        VMSTATE_UINT32(rx_desc_addr, GemState),
-        VMSTATE_UINT32(tx_desc_addr, GemState),
-        VMSTATE_BOOL_ARRAY(sar_active, GemState, 4),
+        VMSTATE_UINT32_ARRAY(regs, CadenceGEMState, CADENCE_GEM_MAXREG),
+        VMSTATE_UINT16_ARRAY(phy_regs, CadenceGEMState, 32),
+        VMSTATE_UINT8(phy_loop, CadenceGEMState),
+        VMSTATE_UINT32(rx_desc_addr, CadenceGEMState),
+        VMSTATE_UINT32(tx_desc_addr, CadenceGEMState),
+        VMSTATE_BOOL_ARRAY(sar_active, CadenceGEMState, 4),
         VMSTATE_END_OF_LIST(),
     }
 };
 
 static Property gem_properties[] = {
-    DEFINE_NIC_PROPERTIES(GemState, conf),
+    DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1277,7 +1277,7 @@ static void gem_class_init(ObjectClass *klass, void *data)
 static const TypeInfo gem_info = {
     .name  = TYPE_CADENCE_GEM,
     .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size  = sizeof(GemState),
+    .instance_size  = sizeof(CadenceGEMState),
     .class_init = gem_class_init,
 };
 
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 07/16] net: cadence_gem: Clean up variable names

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> In preparation for migrating the state struct and type cast macro to a public
> header. The acronym "GEM" on it's own is not specific enough to be used in a
> more global namespace so preface with "cadence". Fix the capitalisation of
> "gem" in the state type while touching the typename. Also preface the
> GEM_MAXREG macro as this will need to migrate to public header.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 07/16] net: cadence_gem: Clean up variable names

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:50 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> In preparation for migrating the state struct and type cast macro to a public

I have also made this commit message more standalone.

>> header. The acronym "GEM" on it's own is not specific enough to be used in a
>> more global namespace so preface with "cadence". Fix the capitalisation of
>> "gem" in the state type while touching the typename. Also preface the
>> GEM_MAXREG macro as this will need to migrate to public header.
>>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>

Thanks.

Regards,
Peter

> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 06/16] arm: xlnx-zynqmp: Connect CPU Timers to GIC

[Peter Crosthwaite]

Connect the GPIO outputs from the individual CPUs for the timers to the
GIC.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 9465185..29954f5 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -19,9 +19,17 @@
 
 #define GIC_NUM_SPI_INTR 128
 
+#define ARM_PHYS_TIMER_PPI  30
+#define ARM_VIRT_TIMER_PPI  27
+
 #define GIC_DIST_ADDR       0xf9010000
 #define GIC_CPU_ADDR        0xf9020000
 
+static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
+{
+    return GIC_NUM_SPI_INTR + cpu_nr * 32 + ppi_index;
+}
+
 static void xlnx_zynqmp_init(Object *obj)
 {
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
@@ -60,11 +68,19 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
+        qemu_irq irq;
+
         object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
         ERR_PROP_CHECK_RETURN(err, errp);
 
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
                            qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
+        irq = qdev_get_gpio_in(DEVICE(&s->gic),
+                               arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
+        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
+        irq = qdev_get_gpio_in(DEVICE(&s->gic),
+                               arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
+        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
     }
 }
 
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 06/16] arm: xlnx-zynqmp: Connect CPU Timers to GIC

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Connect the GPIO outputs from the individual CPUs for the timers to the
> GIC.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 9465185..29954f5 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -19,9 +19,17 @@
>
>  #define GIC_NUM_SPI_INTR 128
>
> +#define ARM_PHYS_TIMER_PPI  30
> +#define ARM_VIRT_TIMER_PPI  27
> +
>  #define GIC_DIST_ADDR       0xf9010000
>  #define GIC_CPU_ADDR        0xf9020000

Hey Peter,

I'm wondering if the #define's should be in the header file?

>
> +static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
> +{
> +    return GIC_NUM_SPI_INTR + cpu_nr * 32 + ppi_index;

Should the 32 also be a #define? Everything else is.

Thanks,

Alistair

> +}
> +
>  static void xlnx_zynqmp_init(Object *obj)
>  {
>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> @@ -60,11 +68,19 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
>
>      for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +        qemu_irq irq;
> +
>          object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>          ERR_PROP_CHECK_RETURN(err, errp);
>
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>                             qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
> +        irq = qdev_get_gpio_in(DEVICE(&s->gic),
> +                               arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
> +        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
> +        irq = qdev_get_gpio_in(DEVICE(&s->gic),
> +                               arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
> +        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
>      }
>  }
>
> --
> 2.3.1.2.g90df61e.dirty
>
>

Re: [Qemu-devel] [PATCH target-arm v4 06/16] arm: xlnx-zynqmp: Connect CPU Timers to GIC

[Peter Crosthwaite]

On Sun, Mar 29, 2015 at 6:29 PM, Alistair Francis <alistair23@gmail.com> wrote:
> On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Connect the GPIO outputs from the individual CPUs for the timers to the
>> GIC.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  hw/arm/xlnx-zynqmp.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 9465185..29954f5 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -19,9 +19,17 @@
>>
>>  #define GIC_NUM_SPI_INTR 128
>>
>> +#define ARM_PHYS_TIMER_PPI  30
>> +#define ARM_VIRT_TIMER_PPI  27
>> +
>>  #define GIC_DIST_ADDR       0xf9010000
>>  #define GIC_CPU_ADDR        0xf9020000
>
> Hey Peter,
>
> I'm wondering if the #define's should be in the header file?
>

I was thinking not, as they are private to the SoC Configuration.

>>
>> +static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
>> +{
>> +    return GIC_NUM_SPI_INTR + cpu_nr * 32 + ppi_index;
>
> Should the 32 also be a #define? Everything else is.
>

include/hw/intc/arm_gic_common.h:29:#define GIC_INTERNAL 32

I'll use it.

Regards,
Peter


> Thanks,
>
> Alistair
>
>> +}
>> +
>>  static void xlnx_zynqmp_init(Object *obj)
>>  {
>>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>> @@ -60,11 +68,19 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
>>
>>      for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +        qemu_irq irq;
>> +
>>          object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>>          ERR_PROP_CHECK_RETURN(err, errp);
>>
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>>                             qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
>> +        irq = qdev_get_gpio_in(DEVICE(&s->gic),
>> +                               arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
>> +        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
>> +        irq = qdev_get_gpio_in(DEVICE(&s->gic),
>> +                               arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
>> +        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
>>      }
>>  }
>>
>> --
>> 2.3.1.2.g90df61e.dirty
>>
>>
>

[Qemu-devel] [PATCH target-arm v4 08/16] net: cadence_gem: Split state struct and type into header

[Peter Crosthwaite]

To allow using the device with modern SoC programming conventions. The
state struct needs to be visible to embed the device in SoC containers.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1:
Fix /* Public */ comment spacing (Alistair review)

 hw/net/cadence_gem.c         | 43 +-------------------------------------
 include/hw/net/cadence_gem.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 42 deletions(-)
 create mode 100644 include/hw/net/cadence_gem.h

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 5994306..dafe914 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -24,8 +24,7 @@
 
 #include <zlib.h> /* For crc32 */
 
-#include "hw/sysbus.h"
-#include "net/net.h"
+#include "hw/net/cadence_gem.h"
 #include "net/checksum.h"
 
 #ifdef CADENCE_GEM_ERR_DEBUG
@@ -141,8 +140,6 @@
 #define GEM_DESCONF6      (0x00000294/4)
 #define GEM_DESCONF7      (0x00000298/4)
 
-#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
-
 /*****************************************/
 #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
 #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
@@ -349,44 +346,6 @@ static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
     desc[1] |= R_DESC_1_RX_SAR_MATCH;
 }
 
-#define TYPE_CADENCE_GEM "cadence_gem"
-#define CADENCE_GEM(obj) OBJECT_CHECK(CadenceGEMState, (obj), TYPE_CADENCE_GEM)
-
-typedef struct CadenceGEMState {
-    SysBusDevice parent_obj;
-
-    MemoryRegion iomem;
-    NICState *nic;
-    NICConf conf;
-    qemu_irq irq;
-
-    /* GEM registers backing store */
-    uint32_t regs[CADENCE_GEM_MAXREG];
-    /* Mask of register bits which are write only */
-    uint32_t regs_wo[CADENCE_GEM_MAXREG];
-    /* Mask of register bits which are read only */
-    uint32_t regs_ro[CADENCE_GEM_MAXREG];
-    /* Mask of register bits which are clear on read */
-    uint32_t regs_rtc[CADENCE_GEM_MAXREG];
-    /* Mask of register bits which are write 1 to clear */
-    uint32_t regs_w1c[CADENCE_GEM_MAXREG];
-
-    /* PHY registers backing store */
-    uint16_t phy_regs[32];
-
-    uint8_t phy_loop; /* Are we in phy loopback? */
-
-    /* The current DMA descriptor pointers */
-    uint32_t rx_desc_addr;
-    uint32_t tx_desc_addr;
-
-    uint8_t can_rx_state; /* Debug only */
-
-    unsigned rx_desc[2];
-
-    bool sar_active[4];
-} CadenceGEMState;
-
 /* The broadcast MAC address: 0xFFFFFFFFFFFF */
 static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
 
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
new file mode 100644
index 0000000..12de820
--- /dev/null
+++ b/include/hw/net/cadence_gem.h
@@ -0,0 +1,49 @@
+#ifndef CADENCE_GEM_H_
+
+#define TYPE_CADENCE_GEM "cadence_gem"
+#define CADENCE_GEM(obj) OBJECT_CHECK(CadenceGEMState, (obj), TYPE_CADENCE_GEM)
+
+#include "net/net.h"
+#include "hw/sysbus.h"
+
+#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
+
+typedef struct CadenceGEMState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion iomem;
+    NICState *nic;
+    NICConf conf;
+    qemu_irq irq;
+
+    /* GEM registers backing store */
+    uint32_t regs[CADENCE_GEM_MAXREG];
+    /* Mask of register bits which are write only */
+    uint32_t regs_wo[CADENCE_GEM_MAXREG];
+    /* Mask of register bits which are read only */
+    uint32_t regs_ro[CADENCE_GEM_MAXREG];
+    /* Mask of register bits which are clear on read */
+    uint32_t regs_rtc[CADENCE_GEM_MAXREG];
+    /* Mask of register bits which are write 1 to clear */
+    uint32_t regs_w1c[CADENCE_GEM_MAXREG];
+
+    /* PHY registers backing store */
+    uint16_t phy_regs[32];
+
+    uint8_t phy_loop; /* Are we in phy loopback? */
+
+    /* The current DMA descriptor pointers */
+    uint32_t rx_desc_addr;
+    uint32_t tx_desc_addr;
+
+    uint8_t can_rx_state; /* Debug only */
+
+    unsigned rx_desc[2];
+
+    bool sar_active[4];
+} CadenceGEMState;
+
+#define CADENCE_GEM_H_
+#endif
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 08/16] net: cadence_gem: Split state struct and type into header

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> To allow using the device with modern SoC programming conventions. The
> state struct needs to be visible to embed the device in SoC containers.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1:
> Fix /* Public */ comment spacing (Alistair review)
>
>  hw/net/cadence_gem.c         | 43 +-------------------------------------
>  include/hw/net/cadence_gem.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 42 deletions(-)
>  create mode 100644 include/hw/net/cadence_gem.h
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 5994306..dafe914 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -24,8 +24,7 @@
>
>  #include <zlib.h> /* For crc32 */
>
> -#include "hw/sysbus.h"
> -#include "net/net.h"
> +#include "hw/net/cadence_gem.h"
>  #include "net/checksum.h"
>
>  #ifdef CADENCE_GEM_ERR_DEBUG
> @@ -141,8 +140,6 @@
>  #define GEM_DESCONF6      (0x00000294/4)
>  #define GEM_DESCONF7      (0x00000298/4)
>
> -#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
> -
>  /*****************************************/
>  #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
>  #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
> @@ -349,44 +346,6 @@ static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
>      desc[1] |= R_DESC_1_RX_SAR_MATCH;
>  }
>
> -#define TYPE_CADENCE_GEM "cadence_gem"
> -#define CADENCE_GEM(obj) OBJECT_CHECK(CadenceGEMState, (obj), TYPE_CADENCE_GEM)
> -
> -typedef struct CadenceGEMState {
> -    SysBusDevice parent_obj;
> -
> -    MemoryRegion iomem;
> -    NICState *nic;
> -    NICConf conf;
> -    qemu_irq irq;
> -
> -    /* GEM registers backing store */
> -    uint32_t regs[CADENCE_GEM_MAXREG];
> -    /* Mask of register bits which are write only */
> -    uint32_t regs_wo[CADENCE_GEM_MAXREG];
> -    /* Mask of register bits which are read only */
> -    uint32_t regs_ro[CADENCE_GEM_MAXREG];
> -    /* Mask of register bits which are clear on read */
> -    uint32_t regs_rtc[CADENCE_GEM_MAXREG];
> -    /* Mask of register bits which are write 1 to clear */
> -    uint32_t regs_w1c[CADENCE_GEM_MAXREG];
> -
> -    /* PHY registers backing store */
> -    uint16_t phy_regs[32];
> -
> -    uint8_t phy_loop; /* Are we in phy loopback? */
> -
> -    /* The current DMA descriptor pointers */
> -    uint32_t rx_desc_addr;
> -    uint32_t tx_desc_addr;
> -
> -    uint8_t can_rx_state; /* Debug only */
> -
> -    unsigned rx_desc[2];
> -
> -    bool sar_active[4];
> -} CadenceGEMState;
> -
>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
>  static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
>
> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> new file mode 100644
> index 0000000..12de820
> --- /dev/null
> +++ b/include/hw/net/cadence_gem.h
> @@ -0,0 +1,49 @@
> +#ifndef CADENCE_GEM_H_

This new file needs a copyright-and-license comment, and the
include guard should lose the trailing '_', but otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 08/16] net: cadence_gem: Split state struct and type into header

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:51 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> To allow using the device with modern SoC programming conventions. The
>> state struct needs to be visible to embed the device in SoC containers.
>>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed since v1:
>> Fix /* Public */ comment spacing (Alistair review)
>>
>>  hw/net/cadence_gem.c         | 43 +-------------------------------------
>>  include/hw/net/cadence_gem.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+), 42 deletions(-)
>>  create mode 100644 include/hw/net/cadence_gem.h
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 5994306..dafe914 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -24,8 +24,7 @@
>>
>>  #include <zlib.h> /* For crc32 */
>>
>> -#include "hw/sysbus.h"
>> -#include "net/net.h"
>> +#include "hw/net/cadence_gem.h"
>>  #include "net/checksum.h"
>>
>>  #ifdef CADENCE_GEM_ERR_DEBUG
>> @@ -141,8 +140,6 @@
>>  #define GEM_DESCONF6      (0x00000294/4)
>>  #define GEM_DESCONF7      (0x00000298/4)
>>
>> -#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
>> -
>>  /*****************************************/
>>  #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
>>  #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
>> @@ -349,44 +346,6 @@ static inline void rx_desc_set_sar(unsigned *desc, int sar_idx)
>>      desc[1] |= R_DESC_1_RX_SAR_MATCH;
>>  }
>>
>> -#define TYPE_CADENCE_GEM "cadence_gem"
>> -#define CADENCE_GEM(obj) OBJECT_CHECK(CadenceGEMState, (obj), TYPE_CADENCE_GEM)
>> -
>> -typedef struct CadenceGEMState {
>> -    SysBusDevice parent_obj;
>> -
>> -    MemoryRegion iomem;
>> -    NICState *nic;
>> -    NICConf conf;
>> -    qemu_irq irq;
>> -
>> -    /* GEM registers backing store */
>> -    uint32_t regs[CADENCE_GEM_MAXREG];
>> -    /* Mask of register bits which are write only */
>> -    uint32_t regs_wo[CADENCE_GEM_MAXREG];
>> -    /* Mask of register bits which are read only */
>> -    uint32_t regs_ro[CADENCE_GEM_MAXREG];
>> -    /* Mask of register bits which are clear on read */
>> -    uint32_t regs_rtc[CADENCE_GEM_MAXREG];
>> -    /* Mask of register bits which are write 1 to clear */
>> -    uint32_t regs_w1c[CADENCE_GEM_MAXREG];
>> -
>> -    /* PHY registers backing store */
>> -    uint16_t phy_regs[32];
>> -
>> -    uint8_t phy_loop; /* Are we in phy loopback? */
>> -
>> -    /* The current DMA descriptor pointers */
>> -    uint32_t rx_desc_addr;
>> -    uint32_t tx_desc_addr;
>> -
>> -    uint8_t can_rx_state; /* Debug only */
>> -
>> -    unsigned rx_desc[2];
>> -
>> -    bool sar_active[4];
>> -} CadenceGEMState;
>> -
>>  /* The broadcast MAC address: 0xFFFFFFFFFFFF */
>>  static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
>>
>> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
>> new file mode 100644
>> index 0000000..12de820
>> --- /dev/null
>> +++ b/include/hw/net/cadence_gem.h
>> @@ -0,0 +1,49 @@
>> +#ifndef CADENCE_GEM_H_
>
> This new file needs a copyright-and-license comment, and the
> include guard should lose the trailing '_', but otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>

Fixed. Thanks.

Regards,
Peter

> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 11/16] char: cadence_uart: Split state struct and type into header

[Peter Crosthwaite]

To allow using the device with modern SoC programming conventions. The
state struct needs to be visible to embed the device in SoC containers.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1:
Fix /* Public */ comment spacing (Alistair review)

 hw/char/cadence_uart.c         | 29 +----------------------------
 include/hw/char/cadence_uart.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 28 deletions(-)
 create mode 100644 include/hw/char/cadence_uart.h

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index efc6576..b7c4c2d 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -16,9 +16,7 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "hw/sysbus.h"
-#include "sysemu/char.h"
-#include "qemu/timer.h"
+#include "hw/char/cadence_uart.h"
 
 #ifdef CADENCE_UART_ERR_DEBUG
 #define DB_PRINT(...) do { \
@@ -85,8 +83,6 @@
 #define LOCAL_LOOPBACK         (0x2 << UART_MR_CHMODE_SH)
 #define REMOTE_LOOPBACK        (0x3 << UART_MR_CHMODE_SH)
 
-#define CADENCE_UART_RX_FIFO_SIZE           16
-#define CADENCE_UART_TX_FIFO_SIZE           16
 #define UART_INPUT_CLK         50000000
 
 #define R_CR       (0x00/4)
@@ -108,29 +104,6 @@
 #define R_PWID     (0x40/4)
 #define R_TTRIG    (0x44/4)
 
-#define CADENCE_UART_R_MAX (0x48/4)
-
-#define TYPE_CADENCE_UART "cadence_uart"
-#define CADENCE_UART(obj) OBJECT_CHECK(CadenceUARTState, (obj), \
-                                       TYPE_CADENCE_UART)
-
-typedef struct {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    MemoryRegion iomem;
-    uint32_t r[CADENCE_UART_R_MAX];
-    uint8_t rx_fifo[CADENCE_UART_RX_FIFO_SIZE];
-    uint8_t tx_fifo[CADENCE_UART_TX_FIFO_SIZE];
-    uint32_t rx_wpos;
-    uint32_t rx_count;
-    uint32_t tx_count;
-    uint64_t char_tx_time;
-    CharDriverState *chr;
-    qemu_irq irq;
-    QEMUTimer *fifo_trigger_handle;
-} CadenceUARTState;
 
 static void uart_update_status(CadenceUARTState *s)
 {
diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
new file mode 100644
index 0000000..3456d4c
--- /dev/null
+++ b/include/hw/char/cadence_uart.h
@@ -0,0 +1,35 @@
+#ifndef CADENCE_UART_H_
+
+#include "hw/sysbus.h"
+#include "sysemu/char.h"
+#include "qemu/timer.h"
+
+#define CADENCE_UART_RX_FIFO_SIZE           16
+#define CADENCE_UART_TX_FIFO_SIZE           16
+
+#define CADENCE_UART_R_MAX (0x48/4)
+
+#define TYPE_CADENCE_UART "cadence_uart"
+#define CADENCE_UART(obj) OBJECT_CHECK(CadenceUARTState, (obj), \
+                                       TYPE_CADENCE_UART)
+
+typedef struct {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion iomem;
+    uint32_t r[CADENCE_UART_R_MAX];
+    uint8_t rx_fifo[CADENCE_UART_RX_FIFO_SIZE];
+    uint8_t tx_fifo[CADENCE_UART_TX_FIFO_SIZE];
+    uint32_t rx_wpos;
+    uint32_t rx_count;
+    uint32_t tx_count;
+    uint64_t char_tx_time;
+    CharDriverState *chr;
+    qemu_irq irq;
+    QEMUTimer *fifo_trigger_handle;
+} CadenceUARTState;
+
+#define CADENCE_UART_H_
+#endif
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 11/16] char: cadence_uart: Split state struct and type into header

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> To allow using the device with modern SoC programming conventions. The
> state struct needs to be visible to embed the device in SoC containers.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Non-standalone commit message body, header file needs copyright/license
comment, include guard shouldn't have trailing _. Otherwise:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 11/16] char: cadence_uart: Split state struct and type into header

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 11:00 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> To allow using the device with modern SoC programming conventions. The
>> state struct needs to be visible to embed the device in SoC containers.
>>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Non-standalone commit message body, header file needs copyright/license
> comment, include guard shouldn't have trailing _. Otherwise:
>

All fixed.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>

Thanks,

Regards,
Peter

> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 10/16] char: cadence_uart: Clean up variable names

[Peter Crosthwaite]

In preparation for migrating the state struct and type cast macro to a public
header. The acronym "UART" on it's own is not specific enough to be used in a
more global namespace so preface with "cadence". Fix the capitalisation of
"uart" in the state type while touching the typename. Preface macros
used by the state struct itself with CADENCE_UART so they don't conflict
in namespace either.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/char/cadence_uart.c | 102 ++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 48 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index a5dc2a4..efc6576 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -85,8 +85,8 @@
 #define LOCAL_LOOPBACK         (0x2 << UART_MR_CHMODE_SH)
 #define REMOTE_LOOPBACK        (0x3 << UART_MR_CHMODE_SH)
 
-#define RX_FIFO_SIZE           16
-#define TX_FIFO_SIZE           16
+#define CADENCE_UART_RX_FIFO_SIZE           16
+#define CADENCE_UART_TX_FIFO_SIZE           16
 #define UART_INPUT_CLK         50000000
 
 #define R_CR       (0x00/4)
@@ -108,10 +108,11 @@
 #define R_PWID     (0x40/4)
 #define R_TTRIG    (0x44/4)
 
-#define R_MAX (R_TTRIG + 1)
+#define CADENCE_UART_R_MAX (0x48/4)
 
 #define TYPE_CADENCE_UART "cadence_uart"
-#define CADENCE_UART(obj) OBJECT_CHECK(UartState, (obj), TYPE_CADENCE_UART)
+#define CADENCE_UART(obj) OBJECT_CHECK(CadenceUARTState, (obj), \
+                                       TYPE_CADENCE_UART)
 
 typedef struct {
     /*< private >*/
@@ -119,9 +120,9 @@ typedef struct {
     /*< public >*/
 
     MemoryRegion iomem;
-    uint32_t r[R_MAX];
-    uint8_t rx_fifo[RX_FIFO_SIZE];
-    uint8_t tx_fifo[TX_FIFO_SIZE];
+    uint32_t r[CADENCE_UART_R_MAX];
+    uint8_t rx_fifo[CADENCE_UART_RX_FIFO_SIZE];
+    uint8_t tx_fifo[CADENCE_UART_TX_FIFO_SIZE];
     uint32_t rx_wpos;
     uint32_t rx_count;
     uint32_t tx_count;
@@ -129,17 +130,19 @@ typedef struct {
     CharDriverState *chr;
     qemu_irq irq;
     QEMUTimer *fifo_trigger_handle;
-} UartState;
+} CadenceUARTState;
 
-static void uart_update_status(UartState *s)
+static void uart_update_status(CadenceUARTState *s)
 {
     s->r[R_SR] = 0;
 
-    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
+    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
+                                                           : 0;
     s->r[R_SR] |= !s->rx_count ? UART_SR_INTR_REMPTY : 0;
     s->r[R_SR] |= s->rx_count >= s->r[R_RTRIG] ? UART_SR_INTR_RTRIG : 0;
 
-    s->r[R_SR] |= s->tx_count == TX_FIFO_SIZE ? UART_SR_INTR_TFUL : 0;
+    s->r[R_SR] |= s->tx_count == CADENCE_UART_TX_FIFO_SIZE ? UART_SR_INTR_TFUL
+                                                           : 0;
     s->r[R_SR] |= !s->tx_count ? UART_SR_INTR_TEMPTY : 0;
     s->r[R_SR] |= s->tx_count >= s->r[R_TTRIG] ? UART_SR_TTRIG : 0;
 
@@ -150,14 +153,14 @@ static void uart_update_status(UartState *s)
 
 static void fifo_trigger_update(void *opaque)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
 
     s->r[R_CISR] |= UART_INTR_TIMEOUT;
 
     uart_update_status(s);
 }
 
-static void uart_rx_reset(UartState *s)
+static void uart_rx_reset(CadenceUARTState *s)
 {
     s->rx_wpos = 0;
     s->rx_count = 0;
@@ -166,12 +169,12 @@ static void uart_rx_reset(UartState *s)
     }
 }
 
-static void uart_tx_reset(UartState *s)
+static void uart_tx_reset(CadenceUARTState *s)
 {
     s->tx_count = 0;
 }
 
-static void uart_send_breaks(UartState *s)
+static void uart_send_breaks(CadenceUARTState *s)
 {
     int break_enabled = 1;
 
@@ -181,7 +184,7 @@ static void uart_send_breaks(UartState *s)
     }
 }
 
-static void uart_parameters_setup(UartState *s)
+static void uart_parameters_setup(CadenceUARTState *s)
 {
     QEMUSerialSetParams ssp;
     unsigned int baud_rate, packet_size;
@@ -236,20 +239,20 @@ static void uart_parameters_setup(UartState *s)
 
 static int uart_can_receive(void *opaque)
 {
-    UartState *s = (UartState *)opaque;
-    int ret = MAX(RX_FIFO_SIZE, TX_FIFO_SIZE);
+    CadenceUARTState *s = opaque;
+    int ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
     uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
     if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
-        ret = MIN(ret, RX_FIFO_SIZE - s->rx_count);
+        ret = MIN(ret, CADENCE_UART_RX_FIFO_SIZE - s->rx_count);
     }
     if (ch_mode == REMOTE_LOOPBACK || ch_mode == ECHO_MODE) {
-        ret = MIN(ret, TX_FIFO_SIZE - s->tx_count);
+        ret = MIN(ret, CADENCE_UART_TX_FIFO_SIZE - s->tx_count);
     }
     return ret;
 }
 
-static void uart_ctrl_update(UartState *s)
+static void uart_ctrl_update(CadenceUARTState *s)
 {
     if (s->r[R_CR] & UART_CR_TXRST) {
         uart_tx_reset(s);
@@ -268,7 +271,7 @@ static void uart_ctrl_update(UartState *s)
 
 static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint64_t new_rx_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     int i;
 
@@ -276,12 +279,12 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    if (s->rx_count == RX_FIFO_SIZE) {
+    if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
         s->r[R_CISR] |= UART_INTR_ROVR;
     } else {
         for (i = 0; i < size; i++) {
             s->rx_fifo[s->rx_wpos] = buf[i];
-            s->rx_wpos = (s->rx_wpos + 1) % RX_FIFO_SIZE;
+            s->rx_wpos = (s->rx_wpos + 1) % CADENCE_UART_RX_FIFO_SIZE;
             s->rx_count++;
         }
         timer_mod(s->fifo_trigger_handle, new_rx_time +
@@ -293,7 +296,7 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
 static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
                                   void *opaque)
 {
-    UartState *s = opaque;
+    CadenceUARTState *s = opaque;
     int ret;
 
     /* instant drain the fifo when there's no back-end */
@@ -320,14 +323,15 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     return FALSE;
 }
 
-static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
+static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
+                               int size)
 {
     if ((s->r[R_CR] & UART_CR_TX_DIS) || !(s->r[R_CR] & UART_CR_TX_EN)) {
         return;
     }
 
-    if (size > TX_FIFO_SIZE - s->tx_count) {
-        size = TX_FIFO_SIZE - s->tx_count;
+    if (size > CADENCE_UART_TX_FIFO_SIZE - s->tx_count) {
+        size = CADENCE_UART_TX_FIFO_SIZE - s->tx_count;
         /*
          * This can only be a guest error via a bad tx fifo register push,
          * as can_receive() should stop remote loop and echo modes ever getting
@@ -345,7 +349,7 @@ static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
 
 static void uart_receive(void *opaque, const uint8_t *buf, int size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
     if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
@@ -358,7 +362,7 @@ static void uart_receive(void *opaque, const uint8_t *buf, int size)
 
 static void uart_event(void *opaque, int event)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint8_t buf = '\0';
 
     if (event == CHR_EVENT_BREAK) {
@@ -368,15 +372,15 @@ static void uart_event(void *opaque, int event)
     uart_update_status(s);
 }
 
-static void uart_read_rx_fifo(UartState *s, uint32_t *c)
+static void uart_read_rx_fifo(CadenceUARTState *s, uint32_t *c)
 {
     if ((s->r[R_CR] & UART_CR_RX_DIS) || !(s->r[R_CR] & UART_CR_RX_EN)) {
         return;
     }
 
     if (s->rx_count) {
-        uint32_t rx_rpos =
-                (RX_FIFO_SIZE + s->rx_wpos - s->rx_count) % RX_FIFO_SIZE;
+        uint32_t rx_rpos = (CADENCE_UART_RX_FIFO_SIZE + s->rx_wpos -
+                            s->rx_count) % CADENCE_UART_RX_FIFO_SIZE;
         *c = s->rx_fifo[rx_rpos];
         s->rx_count--;
 
@@ -393,7 +397,7 @@ static void uart_read_rx_fifo(UartState *s, uint32_t *c)
 static void uart_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
 
     DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
     offset >>= 2;
@@ -437,11 +441,11 @@ static void uart_write(void *opaque, hwaddr offset,
 static uint64_t uart_read(void *opaque, hwaddr offset,
         unsigned size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint32_t c = 0;
 
     offset >>= 2;
-    if (offset >= R_MAX) {
+    if (offset >= CADENCE_UART_R_MAX) {
         c = 0;
     } else if (offset == R_TX_RX) {
         uart_read_rx_fifo(s, &c);
@@ -461,7 +465,7 @@ static const MemoryRegionOps uart_ops = {
 
 static void cadence_uart_reset(DeviceState *dev)
 {
-    UartState *s = CADENCE_UART(dev);
+    CadenceUARTState *s = CADENCE_UART(dev);
 
     s->r[R_CR] = 0x00000128;
     s->r[R_IMR] = 0;
@@ -478,7 +482,7 @@ static void cadence_uart_reset(DeviceState *dev)
 
 static void cadence_uart_realize(DeviceState *dev, Error **errp)
 {
-    UartState *s = CADENCE_UART(dev);
+    CadenceUARTState *s = CADENCE_UART(dev);
 
     s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                           fifo_trigger_update, s);
@@ -494,7 +498,7 @@ static void cadence_uart_realize(DeviceState *dev, Error **errp)
 static void cadence_uart_init(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    UartState *s = CADENCE_UART(obj);
+    CadenceUARTState *s = CADENCE_UART(obj);
 
     memory_region_init_io(&s->iomem, obj, &uart_ops, s, "uart", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
@@ -505,7 +509,7 @@ static void cadence_uart_init(Object *obj)
 
 static int cadence_uart_post_load(void *opaque, int version_id)
 {
-    UartState *s = opaque;
+    CadenceUARTState *s = opaque;
 
     uart_parameters_setup(s);
     uart_update_status(s);
@@ -518,13 +522,15 @@ static const VMStateDescription vmstate_cadence_uart = {
     .minimum_version_id = 2,
     .post_load = cadence_uart_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(r, UartState, R_MAX),
-        VMSTATE_UINT8_ARRAY(rx_fifo, UartState, RX_FIFO_SIZE),
-        VMSTATE_UINT8_ARRAY(tx_fifo, UartState, RX_FIFO_SIZE),
-        VMSTATE_UINT32(rx_count, UartState),
-        VMSTATE_UINT32(tx_count, UartState),
-        VMSTATE_UINT32(rx_wpos, UartState),
-        VMSTATE_TIMER_PTR(fifo_trigger_handle, UartState),
+        VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
+        VMSTATE_UINT8_ARRAY(rx_fifo, CadenceUARTState,
+                            CADENCE_UART_RX_FIFO_SIZE),
+        VMSTATE_UINT8_ARRAY(tx_fifo, CadenceUARTState,
+                            CADENCE_UART_TX_FIFO_SIZE),
+        VMSTATE_UINT32(rx_count, CadenceUARTState),
+        VMSTATE_UINT32(tx_count, CadenceUARTState),
+        VMSTATE_UINT32(rx_wpos, CadenceUARTState),
+        VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -541,7 +547,7 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
 static const TypeInfo cadence_uart_info = {
     .name          = TYPE_CADENCE_UART,
     .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(UartState),
+    .instance_size = sizeof(CadenceUARTState),
     .instance_init = cadence_uart_init,
     .class_init    = cadence_uart_class_init,
 };
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 10/16] char: cadence_uart: Clean up variable names

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> In preparation for migrating the state struct and type cast macro to a public
> header. The acronym "UART" on it's own is not specific enough to be used in a
> more global namespace so preface with "cadence". Fix the capitalisation of
> "uart" in the state type while touching the typename. Preface macros
> used by the state struct itself with CADENCE_UART so they don't conflict
> in namespace either.

This is another non-standalone commit message. Other than that:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> -    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
> +    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
> +                                                           : 0;

These look kind of ugly as the line length got long enough to wrap.
    if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
        s->r[R_SR] |= UART_SR_INTR_RFUL;
    }
looks nicer to me (and makes it clearer that the bit only updates
if the condition is true, rather than leaving you to figure it out
from the combination of the OR and the ternary operator with a zero
operand), but that kind of change doesn't belong in this
no-content-change patch. You could do it in a later patch, or
not do it at all if you don't think it's important enough (I
don't care strongly either way).

-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 10/16] char: cadence_uart: Clean up variable names

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:59 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> In preparation for migrating the state struct and type cast macro to a public
>> header. The acronym "UART" on it's own is not specific enough to be used in a
>> more global namespace so preface with "cadence". Fix the capitalisation of
>> "uart" in the state type while touching the typename. Preface macros
>> used by the state struct itself with CADENCE_UART so they don't conflict
>> in namespace either.
>
> This is another non-standalone commit message. Other than that:

Fixed.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks.

>
>> -    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
>> +    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
>> +                                                           : 0;
>
> These look kind of ugly as the line length got long enough to wrap.
>     if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
>         s->r[R_SR] |= UART_SR_INTR_RFUL;
>     }
> looks nicer to me (and makes it clearer that the bit only updates
> if the condition is true, rather than leaving you to figure it out
> from the combination of the OR and the ternary operator with a zero
> operand), but that kind of change doesn't belong in this
> no-content-change patch. You could do it in a later patch, or
> not do it at all if you don't think it's important enough (I
> don't care strongly either way).
>

Thinking we do this later to minimize scope of the series.

Regards,
Peter

> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 13/16] arm: Add xlnx-ep108 machine

[Peter Crosthwaite]

Add a machine model for the Xilinx ZynqMP SoC EP108 board.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Chaned since v1:
Change board name to ep108

 hw/arm/Makefile.objs |  2 +-
 hw/arm/xlnx-ep108.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/xlnx-ep108.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index d7cd5f4..a75a182 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -10,4 +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
+obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
new file mode 100644
index 0000000..81704bb
--- /dev/null
+++ b/hw/arm/xlnx-ep108.c
@@ -0,0 +1,53 @@
+/*
+ * Xilinx ZynqMP EP108 board
+ *
+ * Copyright (C) 2015 Xilinx Inc
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "hw/arm/xlnx-zynqmp.h"
+#include "hw/boards.h"
+#include "qemu/error-report.h"
+
+typedef struct XlnxEP108 {
+    XlnxZynqMPState soc;
+} XlnxEP108;
+
+static void xlnx_ep108_init(MachineState *machine)
+{
+    XlnxEP108 *s = g_new0(XlnxEP108, 1);
+    Error *err = NULL;
+
+    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+                              &error_abort);
+
+    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
+    if (err) {
+        error_report("%s", error_get_pretty(err));
+        exit(1);
+    }
+}
+
+static QEMUMachine xlnx_ep108_machine = {
+    .name = "xlnx-ep108",
+    .desc = "Xilinx ZynqMP EP108 board",
+    .init = xlnx_ep108_init,
+};
+
+static void xlnx_ep108_machine_init(void)
+{
+    qemu_register_machine(&xlnx_ep108_machine);
+}
+
+machine_init(xlnx_ep108_machine_init);
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 13/16] arm: Add xlnx-ep108 machine

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Add a machine model for the Xilinx ZynqMP SoC EP108 board.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Looks good

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
> Chaned since v1:
> Change board name to ep108
>
>  hw/arm/Makefile.objs |  2 +-
>  hw/arm/xlnx-ep108.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/xlnx-ep108.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index d7cd5f4..a75a182 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -10,4 +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
> +obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> new file mode 100644
> index 0000000..81704bb
> --- /dev/null
> +++ b/hw/arm/xlnx-ep108.c
> @@ -0,0 +1,53 @@
> +/*
> + * Xilinx ZynqMP EP108 board
> + *
> + * Copyright (C) 2015 Xilinx Inc
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/arm/xlnx-zynqmp.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +
> +typedef struct XlnxEP108 {
> +    XlnxZynqMPState soc;
> +} XlnxEP108;
> +
> +static void xlnx_ep108_init(MachineState *machine)
> +{
> +    XlnxEP108 *s = g_new0(XlnxEP108, 1);
> +    Error *err = NULL;
> +
> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> +                              &error_abort);
> +
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        exit(1);
> +    }
> +}
> +
> +static QEMUMachine xlnx_ep108_machine = {
> +    .name = "xlnx-ep108",
> +    .desc = "Xilinx ZynqMP EP108 board",
> +    .init = xlnx_ep108_init,
> +};
> +
> +static void xlnx_ep108_machine_init(void)
> +{
> +    qemu_register_machine(&xlnx_ep108_machine);
> +}
> +
> +machine_init(xlnx_ep108_machine_init);
> --
> 2.3.1.2.g90df61e.dirty
>
>

Re: [Qemu-devel] [PATCH target-arm v4 13/16] arm: Add xlnx-ep108 machine

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Add a machine model for the Xilinx ZynqMP SoC EP108 board.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH target-arm v4 14/16] arm: xilinx-ep108: Add external RAM

[Peter Crosthwaite]

Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1:
Add ram size clamps and warnings

 hw/arm/xlnx-ep108.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 81704bb..6e89456 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -18,11 +18,16 @@
 #include "hw/arm/xlnx-zynqmp.h"
 #include "hw/boards.h"
 #include "qemu/error-report.h"
+#include "exec/address-spaces.h"
 
 typedef struct XlnxEP108 {
     XlnxZynqMPState soc;
+    MemoryRegion ddr_ram;
 } XlnxEP108;
 
+/* Max 2GB RAM */
+#define EP108_MAX_RAM_SIZE 0x80000000ull
+
 static void xlnx_ep108_init(MachineState *machine)
 {
     XlnxEP108 *s = g_new0(XlnxEP108, 1);
@@ -37,6 +42,22 @@ static void xlnx_ep108_init(MachineState *machine)
         error_report("%s", error_get_pretty(err));
         exit(1);
     }
+
+    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
+        error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
+                     "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
+        machine->ram_size = EP108_MAX_RAM_SIZE;
+    }
+
+    if (machine->ram_size <= 0x08000000) {
+        error_report("WARNING: RAM size " RAM_ADDR_FMT " is small for EP108\n",
+                     machine->ram_size);
+    }
+
+    memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", machine->ram_size,
+                           &error_abort);
+    vmstate_register_ram_global(&s->ddr_ram);
+    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
 }
 
 static QEMUMachine xlnx_ep108_machine = {
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 14/16] arm: xilinx-ep108: Add external RAM

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
> changed since v1:
> Add ram size clamps and warnings
>
>  hw/arm/xlnx-ep108.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 81704bb..6e89456 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -18,11 +18,16 @@
>  #include "hw/arm/xlnx-zynqmp.h"
>  #include "hw/boards.h"
>  #include "qemu/error-report.h"
> +#include "exec/address-spaces.h"
>
>  typedef struct XlnxEP108 {
>      XlnxZynqMPState soc;
> +    MemoryRegion ddr_ram;
>  } XlnxEP108;
>
> +/* Max 2GB RAM */
> +#define EP108_MAX_RAM_SIZE 0x80000000ull
> +
>  static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> @@ -37,6 +42,22 @@ static void xlnx_ep108_init(MachineState *machine)
>          error_report("%s", error_get_pretty(err));
>          exit(1);
>      }
> +
> +    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
> +                     "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
> +        machine->ram_size = EP108_MAX_RAM_SIZE;
> +    }
> +
> +    if (machine->ram_size <= 0x08000000) {
> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " is small for EP108\n",
> +                     machine->ram_size);
> +    }
> +
> +    memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", machine->ram_size,
> +                           &error_abort);
> +    vmstate_register_ram_global(&s->ddr_ram);
> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>  }
>
>  static QEMUMachine xlnx_ep108_machine = {
> --
> 2.3.1.2.g90df61e.dirty
>
>

Re: [Qemu-devel] [PATCH target-arm v4 14/16] arm: xilinx-ep108: Add external RAM

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1:
> Add ram size clamps and warnings
>
>  hw/arm/xlnx-ep108.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 81704bb..6e89456 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -18,11 +18,16 @@
>  #include "hw/arm/xlnx-zynqmp.h"
>  #include "hw/boards.h"
>  #include "qemu/error-report.h"
> +#include "exec/address-spaces.h"
>
>  typedef struct XlnxEP108 {
>      XlnxZynqMPState soc;
> +    MemoryRegion ddr_ram;
>  } XlnxEP108;
>
> +/* Max 2GB RAM */
> +#define EP108_MAX_RAM_SIZE 0x80000000ull
> +
>  static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> @@ -37,6 +42,22 @@ static void xlnx_ep108_init(MachineState *machine)
>          error_report("%s", error_get_pretty(err));
>          exit(1);
>      }
> +
> +    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
> +                     "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
> +        machine->ram_size = EP108_MAX_RAM_SIZE;
> +    }
> +
> +    if (machine->ram_size <= 0x08000000) {
> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " is small for EP108\n",
> +                     machine->ram_size);

error_report() strings don't need the trailing \n.

> +    }
> +
> +    memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", machine->ram_size,
> +                           &error_abort);
> +    vmstate_register_ram_global(&s->ddr_ram);

This should probably be using memory_region_allocate_system_memory().

> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>  }
>
>  static QEMUMachine xlnx_ep108_machine = {

-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 14/16] arm: xilinx-ep108: Add external RAM

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 11:12 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed since v1:
>> Add ram size clamps and warnings
>>
>>  hw/arm/xlnx-ep108.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>> index 81704bb..6e89456 100644
>> --- a/hw/arm/xlnx-ep108.c
>> +++ b/hw/arm/xlnx-ep108.c
>> @@ -18,11 +18,16 @@
>>  #include "hw/arm/xlnx-zynqmp.h"
>>  #include "hw/boards.h"
>>  #include "qemu/error-report.h"
>> +#include "exec/address-spaces.h"
>>
>>  typedef struct XlnxEP108 {
>>      XlnxZynqMPState soc;
>> +    MemoryRegion ddr_ram;
>>  } XlnxEP108;
>>
>> +/* Max 2GB RAM */
>> +#define EP108_MAX_RAM_SIZE 0x80000000ull
>> +
>>  static void xlnx_ep108_init(MachineState *machine)
>>  {
>>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>> @@ -37,6 +42,22 @@ static void xlnx_ep108_init(MachineState *machine)
>>          error_report("%s", error_get_pretty(err));
>>          exit(1);
>>      }
>> +
>> +    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
>> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
>> +                     "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
>> +        machine->ram_size = EP108_MAX_RAM_SIZE;
>> +    }
>> +
>> +    if (machine->ram_size <= 0x08000000) {
>> +        error_report("WARNING: RAM size " RAM_ADDR_FMT " is small for EP108\n",
>> +                     machine->ram_size);
>
> error_report() strings don't need the trailing \n.
>

Actually error_report is too noisy for this, so changing to qemu_log.
As-is, this comes up in make check.

>> +    }
>> +
>> +    memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", machine->ram_size,
>> +                           &error_abort);
>> +    vmstate_register_ram_global(&s->ddr_ram);
>
> This should probably be using memory_region_allocate_system_memory().
>

Fixed.

Regards,
Peter

>> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>>  }
>>
>>  static QEMUMachine xlnx_ep108_machine = {
>
> -- PMM
>

[Qemu-devel] [PATCH target-arm v4 15/16] arm: xilinx-ep108: Add bootloading

[Peter Crosthwaite]

Using standard ARM bootloader.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/xlnx-ep108.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 6e89456..a86f595 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -28,6 +28,8 @@ typedef struct XlnxEP108 {
 /* Max 2GB RAM */
 #define EP108_MAX_RAM_SIZE 0x80000000ull
 
+static struct arm_boot_info xlnx_ep108_binfo;
+
 static void xlnx_ep108_init(MachineState *machine)
 {
     XlnxEP108 *s = g_new0(XlnxEP108, 1);
@@ -58,6 +60,12 @@ static void xlnx_ep108_init(MachineState *machine)
                            &error_abort);
     vmstate_register_ram_global(&s->ddr_ram);
     memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
+
+    xlnx_ep108_binfo.ram_size = machine->ram_size;
+    xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
+    xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
+    xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
+    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
 }
 
 static QEMUMachine xlnx_ep108_machine = {
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 15/16] arm: xilinx-ep108: Add bootloading

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Using standard ARM bootloader.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/arm/xlnx-ep108.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 6e89456..a86f595 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -28,6 +28,8 @@ typedef struct XlnxEP108 {
>  /* Max 2GB RAM */
>  #define EP108_MAX_RAM_SIZE 0x80000000ull
>
> +static struct arm_boot_info xlnx_ep108_binfo;
> +
>  static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> @@ -58,6 +60,12 @@ static void xlnx_ep108_init(MachineState *machine)
>                             &error_abort);
>      vmstate_register_ram_global(&s->ddr_ram);
>      memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
> +
> +    xlnx_ep108_binfo.ram_size = machine->ram_size;
> +    xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
> +    xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
> +    xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
> +    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>  }
>
>  static QEMUMachine xlnx_ep108_machine = {
> --
> 2.3.1.2.g90df61e.dirty
>
>

Re: [Qemu-devel] [PATCH target-arm v4 15/16] arm: xilinx-ep108: Add bootloading

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Using standard ARM bootloader.

Commit msg, etc.

> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/arm/xlnx-ep108.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 6e89456..a86f595 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -28,6 +28,8 @@ typedef struct XlnxEP108 {
>  /* Max 2GB RAM */
>  #define EP108_MAX_RAM_SIZE 0x80000000ull
>
> +static struct arm_boot_info xlnx_ep108_binfo;
> +
>  static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> @@ -58,6 +60,12 @@ static void xlnx_ep108_init(MachineState *machine)
>                             &error_abort);
>      vmstate_register_ram_global(&s->ddr_ram);
>      memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
> +
> +    xlnx_ep108_binfo.ram_size = machine->ram_size;
> +    xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
> +    xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
> +    xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;

nb_cpus, loader_start ?

> +    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>  }
>
>  static QEMUMachine xlnx_ep108_machine = {
> --
> 2.3.1.2.g90df61e.dirty

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 15/16] arm: xilinx-ep108: Add bootloading

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 11:15 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Using standard ARM bootloader.
>
> Commit msg, etc.
>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  hw/arm/xlnx-ep108.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>> index 6e89456..a86f595 100644
>> --- a/hw/arm/xlnx-ep108.c
>> +++ b/hw/arm/xlnx-ep108.c
>> @@ -28,6 +28,8 @@ typedef struct XlnxEP108 {
>>  /* Max 2GB RAM */
>>  #define EP108_MAX_RAM_SIZE 0x80000000ull
>>
>> +static struct arm_boot_info xlnx_ep108_binfo;
>> +
>>  static void xlnx_ep108_init(MachineState *machine)
>>  {
>>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>> @@ -58,6 +60,12 @@ static void xlnx_ep108_init(MachineState *machine)
>>                             &error_abort);
>>      vmstate_register_ram_global(&s->ddr_ram);
>>      memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>> +
>> +    xlnx_ep108_binfo.ram_size = machine->ram_size;
>> +    xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
>> +    xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>> +    xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
>
> nb_cpus, loader_start ?
>

I don't see any valid use for non-0 nb_cpus for aarch64 bootloading. I
think PSCI just handles it all? default_write_secondary will write in
smpboot contents which is AA32.

Loader start is 0. I'll make it explicit.

Regards,
Peter

>> +    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>>  }
>>
>>  static QEMUMachine xlnx_ep108_machine = {
>> --
>> 2.3.1.2.g90df61e.dirty
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v4 15/16] arm: xilinx-ep108: Add bootloading

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 5:43 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Apr 23, 2015 at 11:15 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 23 March 2015 at 11:05, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> Using standard ARM bootloader.
>>
>> Commit msg, etc.
>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>  hw/arm/xlnx-ep108.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>>> index 6e89456..a86f595 100644
>>> --- a/hw/arm/xlnx-ep108.c
>>> +++ b/hw/arm/xlnx-ep108.c
>>> @@ -28,6 +28,8 @@ typedef struct XlnxEP108 {
>>>  /* Max 2GB RAM */
>>>  #define EP108_MAX_RAM_SIZE 0x80000000ull
>>>
>>> +static struct arm_boot_info xlnx_ep108_binfo;
>>> +
>>>  static void xlnx_ep108_init(MachineState *machine)
>>>  {
>>>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>>> @@ -58,6 +60,12 @@ static void xlnx_ep108_init(MachineState *machine)
>>>                             &error_abort);
>>>      vmstate_register_ram_global(&s->ddr_ram);
>>>      memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>>> +
>>> +    xlnx_ep108_binfo.ram_size = machine->ram_size;
>>> +    xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
>>> +    xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>>> +    xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
>>
>> nb_cpus, loader_start ?
>>
>
> I don't see any valid use for non-0 nb_cpus for aarch64 bootloading. I
> think PSCI just handles it all? default_write_secondary will write in
> smpboot contents which is AA32.
>

I just did another sanity check on this. SMP works fine via PSCI with
nb_cpus unset:

CPU1: Booted secondary processor
Detected VIPT I-cache on CPU1
CPU2: Booted secondary processor
Detected VIPT I-cache on CPU2
CPU3: Booted secondary processor
Detected VIPT I-cache on CPU3
Brought up 4 CPUs
SMP: Total of 4 processors activated.
devtmpfs: initialized

 The real hardware on POR holds all the SMP CPUs in reset and PSCI
would layer onto of that. For Linux boots, the correct solution is
going to be PSCI. For other boots you will need your own firmware
which will then have to unreset via the reset controller (device mode
TBA). So I don't see the SMP secondary loop having a place in this
machine model and nb_cpus should default to 0 ( == 1).

Regards,
Peter

> Loader start is 0. I'll make it explicit.
>
> Regards,
> Peter
>
>>> +    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>>>  }
>>>
>>>  static QEMUMachine xlnx_ep108_machine = {
>>> --
>>> 2.3.1.2.g90df61e.dirty
>>
>> thanks
>> -- PMM
>>

[Qemu-devel] [PATCH target-arm v4 16/16] arm: xlnx-zynqmp: Add PSCI setup

[Peter Crosthwaite]

Use SMC PSCI, with the standard policy of secondaries starting in
power-off.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1:
Add &error_abort to property setter calls

 hw/arm/xlnx-zynqmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index e015025..0265fba 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -97,6 +97,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
         qemu_irq irq;
 
+        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
+                                "psci-conduit", &error_abort);
+        if (i > 0) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            object_property_set_bool(OBJECT(&s->cpu[i]), true,
+                                     "start-powered-off", &error_abort);
+        }
+
         object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
         ERR_PROP_CHECK_RETURN(err, errp);
 
-- 
2.3.1.2.g90df61e.dirty

Re: [Qemu-devel] [PATCH target-arm v4 16/16] arm: xlnx-zynqmp: Add PSCI setup

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Use SMC PSCI, with the standard policy of secondaries starting in
> power-off.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1:
> Add &error_abort to property setter calls
>
>  hw/arm/xlnx-zynqmp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index e015025..0265fba 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -97,6 +97,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>          qemu_irq irq;
>
> +        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
> +                                "psci-conduit", &error_abort);
> +        if (i > 0) {
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            object_property_set_bool(OBJECT(&s->cpu[i]), true,
> +                                     "start-powered-off", &error_abort);
> +        }

...oh, here's the PSCI config. I think you might as well have
rolled this into the initial "create the CPUs" patch.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 16/16] arm: xlnx-zynqmp: Add PSCI setup

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 11:16 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Use SMC PSCI, with the standard policy of secondaries starting in
>> power-off.
>>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed since v1:
>> Add &error_abort to property setter calls
>>
>>  hw/arm/xlnx-zynqmp.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index e015025..0265fba 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -97,6 +97,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>      for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>>          qemu_irq irq;
>>
>> +        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
>> +                                "psci-conduit", &error_abort);
>> +        if (i > 0) {
>> +            /* Secondary CPUs start in PSCI powered-down state */
>> +            object_property_set_bool(OBJECT(&s->cpu[i]), true,
>> +                                     "start-powered-off", &error_abort);
>> +        }
>
> ...oh, here's the PSCI config. I think you might as well have
> rolled this into the initial "create the CPUs" patch.
>

Squashed.

Regards,
Peter

> -- PMM
>

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

[Alistair Francis]

On Mon, Mar 23, 2015 at 9:05 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Peter and all,
>
> Xilinx's next gen SoC has been announced. This series adds a SoC and
> board.
>
> 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 model of the EP108 board. Currently this doesn't have any
> EP108 specific features but is a usable board exposing the user visible
> features of the raw SoC.

I'm able to boot u-boot using this command:
./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -nographic -serial
mon:stdio -kernel ./u-boot.elf -m 0x800

Tested-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair


>
> changed since v3:
> Included CPU thread kick fix
> Addressed Alistair review
>
> changed since v2:
> Fix CPU child prop adder
> Add DTS compat string
>
> changed since v1:
> Addressed Alistair review (individual changes on resp. patches)
> Changed board name to EP108
> Changed naming scheme to "zynqmp" / "ZYNQMP" (Michal review)
>
> Regards,
> Peter
>
>
> Peter Crosthwaite (16):
>   cpus: Don't kick un-realized cpus.
>   target-arm: cpu64: Factor out ARM cortex init
>   target-arm: cpu64: Add support for cortex-a53
>   arm: Introduce Xilinx ZynqMP SoC
>   arm: xlnx-zynqmp: Add GIC
>   arm: xlnx-zynqmp: Connect CPU Timers to GIC
>   net: cadence_gem: Clean up variable names
>   net: cadence_gem: Split state struct and type into header
>   arm: xilinx-zynqmp: Add GEM support
>   char: cadence_uart: Clean up variable names
>   char: cadence_uart: Split state struct and type into header
>   arm: xilinx-zynqmp: Add UART support
>   arm: Add xlnx-ep108 machine
>   arm: xilinx-ep108: Add external RAM
>   arm: xilinx-ep108: Add bootloading
>   arm: xlnx-zynqmp: Add PSCI setup
>
>  cpus.c                              |   2 +-
>  default-configs/aarch64-softmmu.mak |   2 +-
>  hw/arm/Makefile.objs                |   1 +
>  hw/arm/xlnx-ep108.c                 |  82 ++++++++++++++++++
>  hw/arm/xlnx-zynqmp.c                | 168 ++++++++++++++++++++++++++++++++++++
>  hw/char/cadence_uart.c              | 115 ++++++++++--------------
>  hw/net/cadence_gem.c                |  95 ++++++--------------
>  include/hw/arm/xlnx-zynqmp.h        |  29 +++++++
>  include/hw/char/cadence_uart.h      |  35 ++++++++
>  include/hw/net/cadence_gem.h        |  49 +++++++++++
>  target-arm/cpu64.c                  |  50 ++++++++---
>  11 files changed, 476 insertions(+), 152 deletions(-)
>  create mode 100644 hw/arm/xlnx-ep108.c
>  create mode 100644 hw/arm/xlnx-zynqmp.c
>  create mode 100644 include/hw/arm/xlnx-zynqmp.h
>  create mode 100644 include/hw/char/cadence_uart.h
>  create mode 100644 include/hw/net/cadence_gem.h
>
> --
> 2.3.1.2.g90df61e.dirty
>
>

Re: [Qemu-devel] [PATCH target-arm v4 09/16] arm: xilinx-zynqmp: Add GEM support

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> There are 4x Cadence GEMs in ZynqMP. Add them.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
(other than the use of that error-check macro...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v4 09/16] arm: xilinx-zynqmp: Add GEM support

[Peter Crosthwaite]

On Thu, Apr 23, 2015 at 10:52 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> There are 4x Cadence GEMs in ZynqMP. Add them.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks.

> (other than the use of that error-check macro...)
>

Fixed.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v4 12/16] arm: xilinx-zynqmp: Add UART support

[Peter Maydell]

On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> There are 2x Cadence UARTs in Zynq MP. Add them.
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM