Re: [Qemu-devel] [PATCH v11 3/5] hw/intc: add allwinner A10 interrupt controller

2013-12-12 Thread Peter Maydell 
On 11 December 2013 08:27, Peter Crosthwaite
peter.crosthwa...@xilinx.com wrote:
  Reduces reliance on qemu_set_irq implementation
 (ideally someone converts that API to accept bool).

I have a feeling we have some users of that API which use it
to pass an arbitrary integer value around, not just for a
true/false value.

Anyway, your suggestion here is correct.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v11 3/5] hw/intc: add allwinner A10 interrupt controller

2013-12-12 Thread Peter Maydell 
On 11 December 2013 08:08, liguang lig.f...@cn.fujitsu.com wrote:
 +static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
 +{
 +AwA10PICState *s = opaque;
 +
 +if (level) {
 +set_bit(irq%32, (void *)s-irq_pending[irq/32]);

The % and / operators here should have spaces round them.

 +}
 +aw_a10_pic_update(s);
 +}
 +
 +static uint64_t aw_a10_pic_read(void *opaque, hwaddr offset, unsigned size)
 +{
 +AwA10PICState *s = opaque;
 +uint8_t index = (offset  0xc)/4;

Spaces.

Otherwise
Reviewed-by: Peter Maydell peter.mayd...@linaro.org

-- PMM

Re: [Qemu-devel] [PATCH v11 3/5] hw/intc: add allwinner A10 interrupt controller

2013-12-12 Thread Li Guang 

Peter Maydell wrote:

On 11 December 2013 08:08, liguanglig.f...@cn.fujitsu.com  wrote:
   

+static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
+{
+AwA10PICState *s = opaque;
+
+if (level) {
+set_bit(irq%32, (void *)s-irq_pending[irq/32]);
 

The % and / operators here should have spaces round them.

   

+}
+aw_a10_pic_update(s);
+}
+
+static uint64_t aw_a10_pic_read(void *opaque, hwaddr offset, unsigned size)
+{
+AwA10PICState *s = opaque;
+uint8_t index = (offset  0xc)/4;
 

Spaces.
   


will fix,
thanks!


Otherwise
Reviewed-by: Peter Maydellpeter.mayd...@linaro.org

-- PMM

[Qemu-devel] [PATCH v11 3/5] hw/intc: add allwinner A10 interrupt controller

2013-12-11 Thread liguang 
Signed-off-by: liguang lig.f...@cn.fujitsu.com
---
 default-configs/arm-softmmu.mak |1 +
 hw/intc/Makefile.objs   |1 +
 hw/intc/allwinner-a10-pic.c |  200 +++
 include/hw/intc/allwinner-a10-pic.h |   40 +++
 4 files changed, 242 insertions(+), 0 deletions(-)
 create mode 100644 hw/intc/allwinner-a10-pic.c
 create mode 100644 include/hw/intc/allwinner-a10-pic.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 7858abf..e965068 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -83,3 +83,4 @@ CONFIG_SDHCI=y
 CONFIG_INTEGRATOR_DEBUG=y
 
 CONFIG_ALLWINNER_A10_PIT=y
+CONFIG_ALLWINNER_A10_PIC=y
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 47ac442..60eb936 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -24,3 +24,4 @@ obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
 obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
new file mode 100644
index 000..f39051a
--- /dev/null
+++ b/hw/intc/allwinner-a10-pic.c
@@ -0,0 +1,200 @@
+/*
+ * Allwinner A10 interrupt controller device emulation
+ *
+ * Copyright (C) 2013 Li Guang
+ * Written by Li Guang lig.f...@cn.fujitsu.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/sysbus.h
+#include hw/devices.h
+#include sysemu/sysemu.h
+#include hw/intc/allwinner-a10-pic.h
+
+static void aw_a10_pic_update(AwA10PICState *s)
+{
+uint8_t i;
+int irq = 0, fiq = 0;
+
+for (i = 0; i  AW_A10_PIC_REG_NUM; i++) {
+irq |= s-irq_pending[i]  ~s-mask[i];
+fiq |= s-select[i]  s-irq_pending[i]  ~s-mask[i];
+}
+
+qemu_set_irq(s-parent_irq, irq);
+qemu_set_irq(s-parent_fiq, fiq);
+}
+
+static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
+{
+AwA10PICState *s = opaque;
+
+if (level) {
+set_bit(irq%32, (void *)s-irq_pending[irq/32]);
+}
+aw_a10_pic_update(s);
+}
+
+static uint64_t aw_a10_pic_read(void *opaque, hwaddr offset, unsigned size)
+{
+AwA10PICState *s = opaque;
+uint8_t index = (offset  0xc)/4;
+
+switch (offset) {
+case AW_A10_PIC_VECTOR:
+return s-vector;
+case AW_A10_PIC_BASE_ADDR:
+return s-base_addr;
+case AW_A10_PIC_PROTECT:
+return s-protect;
+case AW_A10_PIC_NMI:
+return s-nmi;
+case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
+return s-irq_pending[index];
+case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
+return s-fiq_pending[index];
+case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
+return s-select[index];
+case AW_A10_PIC_ENABLE ... AW_A10_PIC_ENABLE + 8:
+return s-enable[index];
+case AW_A10_PIC_MASK ... AW_A10_PIC_MASK + 8:
+return s-mask[index];
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+  %s: Bad offset 0x%x\n,  __func__, (int)offset);
+break;
+}
+
+return 0;
+}
+
+static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+AwA10PICState *s = opaque;
+uint8_t index = (offset  0xc)/4;
+
+switch (offset) {
+case AW_A10_PIC_VECTOR:
+s-vector = value  ~0x3;
+break;
+case AW_A10_PIC_BASE_ADDR:
+s-base_addr = value  ~0x3;
+case AW_A10_PIC_PROTECT:
+s-protect = value;
+break;
+case AW_A10_PIC_NMI:
+s-nmi = value;
+break;
+case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
+s-irq_pending[index] = ~value;
+break;
+case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
+s-fiq_pending[index] = ~value;
+break;
+case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
+s-select[index] = value;
+break;
+case AW_A10_PIC_ENABLE ... AW_A10_PIC_ENABLE + 8:
+s-enable[index] = value;
+break;
+case AW_A10_PIC_MASK ... AW_A10_PIC_MASK + 8:
+s-mask[index] = value;
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+  %s: Bad offset 0x%x\n,  __func__, (int)offset);
+break;
+}
+
+aw_a10_pic_update(s);
+}
+
+static const MemoryRegionOps aw_a10_pic_ops = {
+.read = aw_a10_pic_read,
+.write = aw_a10_pic_write,
+

Re: [Qemu-devel] [PATCH v11 3/5] hw/intc: add allwinner A10 interrupt controller

2013-12-11 Thread Peter Crosthwaite 
On Wed, Dec 11, 2013 at 6:08 PM, liguang lig.f...@cn.fujitsu.com wrote:
 Signed-off-by: liguang lig.f...@cn.fujitsu.com
 ---
  default-configs/arm-softmmu.mak |1 +
  hw/intc/Makefile.objs   |1 +
  hw/intc/allwinner-a10-pic.c |  200 
 +++
  include/hw/intc/allwinner-a10-pic.h |   40 +++
  4 files changed, 242 insertions(+), 0 deletions(-)
  create mode 100644 hw/intc/allwinner-a10-pic.c
  create mode 100644 include/hw/intc/allwinner-a10-pic.h

 diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
 index 7858abf..e965068 100644
 --- a/default-configs/arm-softmmu.mak
 +++ b/default-configs/arm-softmmu.mak
 @@ -83,3 +83,4 @@ CONFIG_SDHCI=y
  CONFIG_INTEGRATOR_DEBUG=y

  CONFIG_ALLWINNER_A10_PIT=y
 +CONFIG_ALLWINNER_A10_PIC=y
 diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
 index 47ac442..60eb936 100644
 --- a/hw/intc/Makefile.objs
 +++ b/hw/intc/Makefile.objs
 @@ -24,3 +24,4 @@ obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
  obj-$(CONFIG_SH4) += sh_intc.o
  obj-$(CONFIG_XICS) += xics.o
  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
 +obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
 new file mode 100644
 index 000..f39051a
 --- /dev/null
 +++ b/hw/intc/allwinner-a10-pic.c
 @@ -0,0 +1,200 @@
 +/*
 + * Allwinner A10 interrupt controller device emulation
 + *
 + * Copyright (C) 2013 Li Guang
 + * Written by Li Guang lig.f...@cn.fujitsu.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/sysbus.h
 +#include hw/devices.h
 +#include sysemu/sysemu.h
 +#include hw/intc/allwinner-a10-pic.h
 +
 +static void aw_a10_pic_update(AwA10PICState *s)
 +{
 +uint8_t i;
 +int irq = 0, fiq = 0;
 +
 +for (i = 0; i  AW_A10_PIC_REG_NUM; i++) {
 +irq |= s-irq_pending[i]  ~s-mask[i];
 +fiq |= s-select[i]  s-irq_pending[i]  ~s-mask[i];
 +}
 +
 +qemu_set_irq(s-parent_irq, irq);
 +qemu_set_irq(s-parent_fiq, fiq);

!!irq or irq ? 1 : 0 to pass either 0 or 1 to qemu_set_irq, rather
than a random value. Reduces reliance on qemu_set_irq implementation
(ideally someone converts that API to accept bool).

otherwise,

Reviewed-by: Peter Crosthwaite peter.crosthwa...@xilinx.com

Please leave it a day or two to allow other reviewers a chance before respin.

Regards,
Peter

 +}
 +
 +static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
 +{
 +AwA10PICState *s = opaque;
 +
 +if (level) {
 +set_bit(irq%32, (void *)s-irq_pending[irq/32]);
 +}
 +aw_a10_pic_update(s);
 +}
 +
 +static uint64_t aw_a10_pic_read(void *opaque, hwaddr offset, unsigned size)
 +{
 +AwA10PICState *s = opaque;
 +uint8_t index = (offset  0xc)/4;
 +
 +switch (offset) {
 +case AW_A10_PIC_VECTOR:
 +return s-vector;
 +case AW_A10_PIC_BASE_ADDR:
 +return s-base_addr;
 +case AW_A10_PIC_PROTECT:
 +return s-protect;
 +case AW_A10_PIC_NMI:
 +return s-nmi;
 +case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
 +return s-irq_pending[index];
 +case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
 +return s-fiq_pending[index];
 +case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
 +return s-select[index];
 +case AW_A10_PIC_ENABLE ... AW_A10_PIC_ENABLE + 8:
 +return s-enable[index];
 +case AW_A10_PIC_MASK ... AW_A10_PIC_MASK + 8:
 +return s-mask[index];
 +default:
 +qemu_log_mask(LOG_GUEST_ERROR,
 +  %s: Bad offset 0x%x\n,  __func__, (int)offset);
 +break;
 +}
 +
 +return 0;
 +}
 +
 +static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
 + unsigned size)
 +{
 +AwA10PICState *s = opaque;
 +uint8_t index = (offset  0xc)/4;
 +
 +switch (offset) {
 +case AW_A10_PIC_VECTOR:
 +s-vector = value  ~0x3;
 +break;
 +case AW_A10_PIC_BASE_ADDR:
 +s-base_addr = value  ~0x3;
 +case AW_A10_PIC_PROTECT:
 +s-protect = value;
 +break;
 +case AW_A10_PIC_NMI:
 +s-nmi = value;
 +break;
 +case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
 +s-irq_pending[index] = ~value;
 +break;
 +case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
 +s-fiq_pending[index] = ~value;
 +break;
 +case AW_A10_PIC_SELECT ...