Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Mitsyanko Igor

On 01/11/2012 08:31 PM, Mark Langsdorf wrote:


 Removed the automatic detection and resetting of ram_size. Image MUST
be loaded with -m 4089 or it will crash


I don't know what is maintainer's politics on this, but as a user of 
your board I don't really like that I will have to remember this strange 
number when I launch Highbank emulation. Have you considered adding an 
automatic upper limitation to user-defined ram_size?




+sysram = g_new(MemoryRegion, 1);
+memory_region_init_ram(sysram, highbank.sysram, 0x8000);
+memory_region_add_subregion(sysmem, 0xfff88000, sysram);
+if (bios_name != NULL) {
+sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+if (sysboot_filename != NULL) {
+uint32_t filesize = get_image_size(sysboot_filename);
+if (load_image_targphys(sysram.bin, 0xfff88000, filesize)  0) {
+hw_error(Unable to load %s\n, bios_name);
+}


Probably should be
if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000)  0) {
and then you don't need uint32_t filesize at all.


+dev = qdev_create(NULL, l2x0);
+qdev_init_nofail(dev);
+busdev = sysbus_from_qdev(dev);
+sysbus_mmio_map(busdev, 0, 0xfff12000);


 +dev = qdev_create(NULL, highbank-regs);
 +qdev_init_nofail(dev);
 +busdev = sysbus_from_qdev(dev);
 +sysbus_mmio_map(busdev, 0, 0xfff3c000);
 +

You can use sysbus_create_simple() here (of course, if you didn't avoid 
it intentionally for some reason).




--
Mitsyanko Igor
ASWG, Moscow RD center, Samsung Electronics
email: i.mitsya...@samsung.com



Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Andreas Färber
Am 12.01.2012 13:47, schrieb Mitsyanko Igor:
 On 01/11/2012 08:31 PM, Mark Langsdorf wrote:
 +sysram = g_new(MemoryRegion, 1);
 +memory_region_init_ram(sysram, highbank.sysram, 0x8000);
 +memory_region_add_subregion(sysmem, 0xfff88000, sysram);
 +if (bios_name != NULL) {
 +sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
 bios_name);
 +if (sysboot_filename != NULL) {
 +uint32_t filesize = get_image_size(sysboot_filename);
 +if (load_image_targphys(sysram.bin, 0xfff88000,
 filesize)  0) {
 +hw_error(Unable to load %s\n, bios_name);
 +}
 
 Probably should be
 if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000)  0) {
 and then you don't need uint32_t filesize at all.

You need it either way; if you use 0x8000 there, you need to check if
filesize is actually 0x8000. Doing it this way allows to load smaller
files; a check for larger files should be added though. Thanks for
making me aware.

 +dev = qdev_create(NULL, l2x0);
 +qdev_init_nofail(dev);
 +busdev = sysbus_from_qdev(dev);
 +sysbus_mmio_map(busdev, 0, 0xfff12000);
 
 +dev = qdev_create(NULL, highbank-regs);
 +qdev_init_nofail(dev);
 +busdev = sysbus_from_qdev(dev);
 +sysbus_mmio_map(busdev, 0, 0xfff3c000);
 +
 
 You can use sysbus_create_simple() here (of course, if you didn't avoid
 it intentionally for some reason).

Depends on how you read this:

/* Legacy helper function for creating devices.  */
DeviceState *sysbus_create_varargs(const char *name,
 target_phys_addr_t addr, ...);
DeviceState *sysbus_try_create_varargs(const char *name,
   target_phys_addr_t addr, ...);
static inline DeviceState *sysbus_create_simple(const char *name,
  target_phys_addr_t addr,
  qemu_irq irq)
{
return sysbus_create_varargs(name, addr, irq, NULL);
}

I interpret it as sysbus_create_simple() using deprecated
sysbus_create_varargs() and therefore being deprecated, too.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg



Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Mitsyanko Igor

On 01/12/2012 05:09 PM, Andreas Färber wrote:

Am 12.01.2012 13:47, schrieb Mitsyanko Igor:

On 01/11/2012 08:31 PM, Mark Langsdorf wrote:

+sysram = g_new(MemoryRegion, 1);
+memory_region_init_ram(sysram, highbank.sysram, 0x8000);
+memory_region_add_subregion(sysmem, 0xfff88000, sysram);
+if (bios_name != NULL) {
+sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
bios_name);
+if (sysboot_filename != NULL) {
+uint32_t filesize = get_image_size(sysboot_filename);
+if (load_image_targphys(sysram.bin, 0xfff88000,
filesize)   0) {
+hw_error(Unable to load %s\n, bios_name);
+}


Probably should be
if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000)   0) {
and then you don't need uint32_t filesize at all.


You need it either way; if you use 0x8000 there, you need to check if
filesize is actually 0x8000. Doing it this way allows to load smaller
files; a check for larger files should be added though. Thanks for
making me aware.



Why do we need to check if filesize is 0x8000? load_image_targphys() 
will call get_image_size() and check that size is not more then 0x8000 
automatically, so it would operate on any file with size=0x8000 and 
return error if size0x8000, just like we need it to. Well, OK, I know 
load_image_targphys() is currently broken and doesn't use max_sz 
argument, but recently I saw a patch in mailing list which fixes this.



+dev = qdev_create(NULL, l2x0);
+qdev_init_nofail(dev);
+busdev = sysbus_from_qdev(dev);
+sysbus_mmio_map(busdev, 0, 0xfff12000);



+dev = qdev_create(NULL, highbank-regs);
+qdev_init_nofail(dev);
+busdev = sysbus_from_qdev(dev);
+sysbus_mmio_map(busdev, 0, 0xfff3c000);
+


You can use sysbus_create_simple() here (of course, if you didn't avoid
it intentionally for some reason).


Depends on how you read this:

/* Legacy helper function for creating devices.  */
DeviceState *sysbus_create_varargs(const char *name,
  target_phys_addr_t addr, ...);
DeviceState *sysbus_try_create_varargs(const char *name,
target_phys_addr_t addr, ...);
static inline DeviceState *sysbus_create_simple(const char *name,
   target_phys_addr_t addr,
   qemu_irq irq)
{
 return sysbus_create_varargs(name, addr, irq, NULL);
}

I interpret it as sysbus_create_simple() using deprecated
sysbus_create_varargs() and therefore being deprecated, too.

Andreas



Sorry, never paid attention that these functions are deprecated.
--
Mitsyanko Igor
ASWG, Moscow RD center, Samsung Electronics
email: i.mitsya...@samsung.com




Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Peter Maydell
On 12 January 2012 13:42, Mitsyanko Igor i.mitsya...@samsung.com wrote:
 On 01/12/2012 05:09 PM, Andreas Färber wrote:
 Depends on how you read this:

 /* Legacy helper function for creating devices.  */
 DeviceState *sysbus_create_varargs(const char *name,
                                  target_phys_addr_t addr, ...);
 DeviceState *sysbus_try_create_varargs(const char *name,
                                        target_phys_addr_t addr, ...);
 static inline DeviceState *sysbus_create_simple(const char *name,
                                               target_phys_addr_t addr,
                                               qemu_irq irq)
 {
     return sysbus_create_varargs(name, addr, irq, NULL);
 }

 I interpret it as sysbus_create_simple() using deprecated
 sysbus_create_varargs() and therefore being deprecated, too.

 Sorry, never paid attention that these functions are deprecated.

Personally I don't think we should deprecate either
sysbus_create_simple() or sysbus_create_varargs() until QOM has
advanced to the point where we can throw out sysbus devices altogether.
These functions are a straightforward way of instantiating simple
sysbus devices, they're widely used, and I don't see anything
particularly wrong with them.

-- PMM



Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Mitsyanko Igor

On 01/11/2012 08:31 PM, Mark Langsdorf wrote:

+sysram = g_new(MemoryRegion, 1);
+memory_region_init_ram(sysram, highbank.sysram, 0x8000);
+memory_region_add_subregion(sysmem, 0xfff88000, sysram);
+if (bios_name != NULL) {
+sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+if (sysboot_filename != NULL) {
+uint32_t filesize = get_image_size(sysboot_filename);
+if (load_image_targphys(sysram.bin, 0xfff88000, filesize)  0) {
+hw_error(Unable to load %s\n, bios_name);
+}
+} else {
+   hw_error(Unable to find %s\n, bios_name);
+}
+}


I'm sorry, I forgot to ask in my previous message, how does control is 
passed to uboot image at 0xfff88000 address? Shouldn't highbank_binfo be 
modified somehow if bios_name supplied?



--
Mitsyanko Igor
ASWG, Moscow RD center, Samsung Electronics
email: i.mitsya...@samsung.com



Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-12 Thread Peter Maydell
On 11 January 2012 16:31, Mark Langsdorf mark.langsd...@calxeda.com wrote:
 +    highbank_binfo.ram_size = ram_size;
 +    highbank_binfo.kernel_filename = kernel_filename;
 +    highbank_binfo.kernel_cmdline = kernel_cmdline;
 +    highbank_binfo.initrd_filename = initrd_filename;
 +    highbank_binfo.board_id = -1; /* provided by deviceTree */
 +    highbank_binfo.nb_cpus = smp_cpus;
 +    highbank_binfo.loader_start = 0;
 +    highbank_binfo.smp_loader_start = SMP_BOOT_ADDR;
 +    arm_load_kernel(env, highbank_binfo);

So at the moment this will use address 0x1030 as the location
that the bootloader for secondary CPUs polls to find out whether
it can release the secondary CPUs. This is right for realview
and vexpress, because it's the sysreg SYS_FLAGS (implemented in
arm_sysctl.c). Is this really the right location to poll for
Highbank?

(There's a patch in the Samsung Exynos4 patch series which
addresses this by allowing boards to specify a polling location.
So I'm wondering what that location ought to be for Highbank...)

thanks
-- PMM



[Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

2012-01-11 Thread Mark Langsdorf
From: Rob Herring rob.herr...@calxeda.com

Adds support for Calxeda's Highbank SoC.

Signed-off-by: Rob Herring rob.herr...@calxeda.com
Signed-off-by: Mark Langsdorf mark.langsd...@calxeda.com
Reviewed-by: Peter Maydell peter.mayd...@linaro.org
---
Changes from v7, v8
None
Changes from v3, v4, v5, v6
Skipped
Changes from v2
Created a reset function for highbank_regs
Handled creation of regs i/o memory region in a sensible manner
Added code to boot secondary CPUs properly
Changes from v1
Restructed the loading of sysram.bin and made it more clearly optional
Made the regs structure into a proper qdev/sysbus object
Removed some unnecessary include files
Clarified the GPL version
Simplified the reset function
Removed the automatic detection and resetting of ram_size. Image MUST
be loaded with -m 4089 or it will crash
Added a guard for xgmac creation
Added a fuller description in the QEMUMachine .desc field

 Makefile.target |1 +
 hw/highbank.c   |  277 +++
 2 files changed, 278 insertions(+), 0 deletions(-)
 create mode 100644 hw/highbank.c

diff --git a/Makefile.target b/Makefile.target
index 9bc0248..1c86522 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -342,6 +342,7 @@ obj-arm-y += realview_gic.o realview.o arm_sysctl.o 
arm11mpcore.o a9mpcore.o
 obj-arm-y += arm_l2x0.o
 obj-arm-y += arm_mptimer.o
 obj-arm-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
+obj-arm-y += highbank.o
 obj-arm-y += pl061.o
 obj-arm-y += xgmac.o
 obj-arm-y += arm-semi.o
diff --git a/hw/highbank.c b/hw/highbank.c
new file mode 100644
index 000..0e1fe66
--- /dev/null
+++ b/hw/highbank.c
@@ -0,0 +1,277 @@
+/*
+ * Calxeda Highbank SoC emulation
+ *
+ * Copyright (c) 2010-2012 Calxeda
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.
+ *
+ */
+
+#include sysbus.h
+#include arm-misc.h
+#include primecell.h
+#include devices.h
+#include loader.h
+#include net.h
+#include sysemu.h
+#include boards.h
+#include sysbus.h
+#include blockdev.h
+#include exec-memory.h
+
+#define SMP_BOOT_ADDR 0x100
+#define NIRQ_GIC  160
+
+/* Board init.  */
+static void highbank_cpu_reset(void *opaque)
+{
+CPUState *env = opaque;
+
+env-cp15.c15_config_base_address = 0xfff1;
+}
+
+#define NUM_REGS  0x200
+static void hb_regs_write(void *opaque, target_phys_addr_t offset,
+  uint64_t value, unsigned size)
+{
+uint32_t *regs = opaque;
+
+if (offset == 0xf00) {
+if (value == 1 || value == 2) {
+qemu_system_reset_request();
+} else if (value == 3) {
+qemu_system_shutdown_request();
+}
+}
+
+regs[offset/4] = value;
+}
+
+static uint64_t hb_regs_read(void *opaque, target_phys_addr_t offset,
+ unsigned size)
+{
+uint32_t *regs = opaque;
+uint32_t value = regs[offset/4];
+
+if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {
+value |= 0x3000;
+}
+
+return value;
+}
+
+static const MemoryRegionOps hb_mem_ops = {
+.read = hb_regs_read,
+.write = hb_regs_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+typedef struct {
+SysBusDevice busdev;
+MemoryRegion *iomem;
+uint32_t regs[NUM_REGS];
+} highbank_regs_state;
+
+static VMStateDescription vmstate_highbank_regs = {
+.name = highbank-regs,
+.version_id = 0,
+.minimum_version_id = 0,
+.minimum_version_id_old = 0,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32_ARRAY(regs, highbank_regs_state, NUM_REGS),
+VMSTATE_END_OF_LIST(),
+},
+};
+
+static void highbank_regs_reset(DeviceState *dev)
+{
+SysBusDevice *sys_dev = sysbus_from_qdev(dev);
+highbank_regs_state *s = FROM_SYSBUS(highbank_regs_state, sys_dev);
+
+s-regs[0x40] = 0x05F20121;
+s-regs[0x41] = 0x2;
+s-regs[0x42] = 0x05F30121;
+s-regs[0x43] = 0x05F40121;
+}
+
+static int highbank_regs_init(SysBusDevice *dev)
+{
+highbank_regs_state *s = FROM_SYSBUS(highbank_regs_state, dev);
+
+s-iomem = g_new(MemoryRegion, 1);
+memory_region_init_io(s-iomem, hb_mem_ops, s-regs, highbank_regs,
+  0x1000);
+sysbus_init_mmio(dev, s-iomem);
+
+return 0;
+}
+
+static SysBusDeviceInfo highbank_regs_info = {
+.init   =