Looks good to me, Peter
On Fri, Jun 29, 2012 at 3:56 PM, Jia Liu <pro...@gmail.com> wrote: > Hi Peter, > > On Fri, Jun 29, 2012 at 11:15 AM, Peter Crosthwaite > <peter.crosthwa...@petalogix.com> wrote: >> On Thu, Jun 28, 2012 at 11:56 PM, Jia Liu <pro...@gmail.com> wrote: >>> Hi, Peter, Andreas, and Peter, >>> >>> I've replace env with cpu, and rewrite the load_kernel. >>> Please review these new files: >>> >>> openrisc_pic.c >>> >>> #include "hw.h" >>> #include "cpu.h" >>> >>> /* OpenRISC pic handler */ >>> static void openrisc_pic_cpu_handler(void *opaque, int irq, int level) >>> { >>> OpenRISCCPU *cpu = (OpenRISCCPU *)opaque; >>> int i; >>> uint32_t irq_bit = 1 << irq; >>> >>> if (irq > 31 || irq < 0) { >>> return; >>> } >>> >>> if (level) { >>> cpu->env.picsr |= irq_bit; >>> } else { >>> cpu->env.picsr &= ~irq_bit; >>> } >>> >>> for (i = 0; i < 32; i++) { >>> if ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i))) { >>> cpu_interrupt(&cpu->env, CPU_INTERRUPT_HARD); >>> } else { >>> cpu_reset_interrupt(&cpu->env, CPU_INTERRUPT_HARD); >>> cpu->env.picsr &= ~(1 << i); >>> } >>> } >>> } >>> >>> void cpu_openrisc_pic_init(OpenRISCCPU *cpu) >>> { >>> int i; >>> qemu_irq *qi; >>> qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS); >>> >>> for (i = 0; i < NR_IRQS; i++) { >>> cpu->env.irq[i] = qi[i]; >>> } >>> } >>> -------------------------------------------------------------------- >>> >>> openrisc_timer.c >>> >>> #include "cpu.h" >>> #include "hw.h" >>> #include "qemu-timer.h" >>> >>> #define TIMER_FREQ (20 * 1000 * 1000) /* 20MHz */ >>> >>> /* The time when TTCR changes */ >>> static uint64_t last_clk; >>> static int is_counting; >>> >>> void cpu_openrisc_count_update(OpenRISCCPU *cpu) >>> { >>> uint64_t now, next; >>> uint32_t wait; >>> >>> now = qemu_get_clock_ns(vm_clock); >>> if (!is_counting) { >>> qemu_del_timer(cpu->env.timer); >>> last_clk = now; >>> return; >>> } >>> >>> cpu->env.ttcr += (uint32_t)muldiv64(now - last_clk, TIMER_FREQ, >>> get_ticks_per_sec()); >>> last_clk = now; >>> >>> if ((cpu->env.ttmr & TTMR_TP) <= (cpu->env.ttcr & TTMR_TP)) { >>> wait = TTMR_TP - (cpu->env.ttcr & TTMR_TP) + 1; >>> wait += cpu->env.ttmr & TTMR_TP; >>> } else { >>> wait = (cpu->env.ttmr & TTMR_TP) - (cpu->env.ttcr & TTMR_TP); >>> } >>> >>> next = now + muldiv64(wait, get_ticks_per_sec(), TIMER_FREQ); >>> qemu_mod_timer(cpu->env.timer, next); >>> } >>> >>> void cpu_openrisc_count_start(OpenRISCCPU *cpu) >>> { >>> is_counting = 1; >>> cpu_openrisc_count_update(cpu); >>> } >>> >>> void cpu_openrisc_count_stop(OpenRISCCPU *cpu) >>> { >>> is_counting = 0; >>> cpu_openrisc_count_update(cpu); >>> } >>> >>> static void openrisc_timer_cb(void *opaque) >>> { >>> OpenRISCCPU *cpu = opaque; >>> >>> if ((cpu->env.ttmr & TTMR_IE) && >>> qemu_timer_expired(cpu->env.timer, qemu_get_clock_ns(vm_clock))) { >>> cpu->env.ttmr |= TTMR_IP; >>> cpu->env.interrupt_request |= CPU_INTERRUPT_TIMER; >>> } >>> >>> switch (cpu->env.ttmr & TTMR_M) { >>> case TIMER_NONE: >>> break; >>> case TIMER_INTR: >>> cpu->env.ttcr = 0; >>> cpu_openrisc_count_start(cpu); >>> break; >>> case TIMER_SHOT: >>> cpu_openrisc_count_stop(cpu); >>> break; >>> case TIMER_CONT: >>> cpu_openrisc_count_start(cpu); >>> break; >>> } >>> } >>> >>> void cpu_openrisc_clock_init(OpenRISCCPU *cpu) >>> { >>> cpu->env.timer = qemu_new_timer_ns(vm_clock, &openrisc_timer_cb, cpu); >>> cpu->env.ttmr = 0x00000000; >>> cpu->env.ttcr = 0x00000000; >>> } >>> -------------------------------------------------------------------- >>> >>> openrisc_sim.c >>> >>> #include "hw.h" >>> #include "boards.h" >>> #include "elf.h" >>> #include "pc.h" >>> #include "loader.h" >>> #include "exec-memory.h" >>> #include "sysemu.h" >>> #include "sysbus.h" >>> #include "qtest.h" >>> >>> #define KERNEL_LOAD_ADDR 0x100 >>> >>> static void main_cpu_reset(void *opaque) >>> { >>> OpenRISCCPU *cpu = opaque; >>> >>> cpu_reset(CPU(cpu)); >>> } >>> >>> static void openrisc_sim_net_init(MemoryRegion *address_space, >>> target_phys_addr_t base, >>> target_phys_addr_t descriptors, >>> qemu_irq irq, NICInfo *nd) >>> { >>> DeviceState *dev; >>> SysBusDevice *s; >>> >>> dev = qdev_create(NULL, "open_eth"); >>> qdev_set_nic_properties(dev, nd); >>> qdev_init_nofail(dev); >>> >>> s = sysbus_from_qdev(dev); >>> sysbus_connect_irq(s, 0, irq); >>> memory_region_add_subregion(address_space, base, >>> sysbus_mmio_get_region(s, 0)); >>> memory_region_add_subregion(address_space, descriptors, >>> sysbus_mmio_get_region(s, 1)); >>> } >>> >>> static void openrisc_sim_init(ram_addr_t ram_size, >>> const char *boot_device, >>> const char *kernel_filename, >>> const char *kernel_cmdline, >>> const char *initrd_filename, >>> const char *cpu_model) >>> { >>> long kernel_size; >>> uint64_t elf_entry; >>> target_phys_addr_t entry; >>> OpenRISCCPU *cpu = NULL; >>> MemoryRegion *ram; >>> int n; >>> >>> if (!cpu_model) { >>> cpu_model = "or1200"; >>> } >>> >>> for (n = 0; n < smp_cpus; n++) { >>> cpu = cpu_openrisc_init(cpu_model); >>> if (cpu == NULL) { >>> qemu_log("Unable to find CPU defineition!\n"); >>> exit(1); >>> } >>> qemu_register_reset(main_cpu_reset, cpu); >>> main_cpu_reset(cpu); >>> } >>> >>> ram = g_malloc(sizeof(*ram)); >>> memory_region_init_ram(ram, "openrisc.ram", ram_size); >>> vmstate_register_ram_global(ram); >>> memory_region_add_subregion(get_system_memory(), 0, ram); >>> >>> /* load kernel */ >>> if (kernel_filename && !qtest_enabled()) { >>> kernel_size = load_elf(kernel_filename, NULL, NULL, >>> &elf_entry, NULL, NULL, 1, ELF_MACHINE, 1); >>> entry = elf_entry; >>> if (kernel_size < 0) { >>> kernel_size = load_uimage(kernel_filename, >>> &entry, NULL, NULL); >>> } >>> if (kernel_size < 0) { >>> kernel_size = load_image_targphys(kernel_filename, >>> KERNEL_LOAD_ADDR, >>> ram_size - KERNEL_LOAD_ADDR); >>> entry = KERNEL_LOAD_ADDR; >>> } >>> >>> if (kernel_size < 0) { >>> qemu_log("QEMU: couldn't load the kernel '%s'\n", >>> kernel_filename); >>> exit(1); >>> } >>> } >> >> I think it was cleaner to have the load_kernel() as its own function, >> just without all the dead struct args. Seperates bootloader from >> machine init and add a little bit of future proofing when we come to >> extract out bootloaders from machine models. But I wont block on it. >> Can you move this hunk (or the load_kernel() call) to the end of the >> function so bootloading happens after machine creation? >> >> Regards, >> Peter >> >>> >>> cpu_openrisc_pic_init(cpu); >>> cpu_openrisc_clock_init(cpu); >>> >>> serial_mm_init(get_system_memory(), 0x90000000, 0, cpu->env.irq[2], >>> 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN); >>> >>> if (nd_table[0].vlan) { >>> openrisc_sim_net_init(get_system_memory(), 0x92000000, >>> 0x92000400, cpu->env.irq[4], nd_table); >>> } >> >> So after this stuff. Bootload here. >> >>> >>> cpu->env.pc = entry; >> >> I would roll this into the load_kernel call too if you went for that >> approach. Part of loading a kernel is setting the entry point so it >> makes sense to have one BL function do everything. Again, I wont >> block, just a suggestion. >> > > Thanks for review. And, thank you very much for your suggestion. > > Is this new code OK? > > static void cpu_openrisc_load_kernel(ram_addr_t ram_size, > const char* kernel_filename, > OpenRISCCPU *cpu) > { > long kernel_size; > uint64_t elf_entry; > target_phys_addr_t entry; > > if (kernel_filename && !qtest_enabled()) { > kernel_size = load_elf(kernel_filename, NULL, NULL, > &elf_entry, NULL, NULL, 1, ELF_MACHINE, 1); > entry = elf_entry; > if (kernel_size < 0) { > kernel_size = load_uimage(kernel_filename, > &entry, NULL, NULL); > } > if (kernel_size < 0) { > kernel_size = load_image_targphys(kernel_filename, > KERNEL_LOAD_ADDR, > ram_size - KERNEL_LOAD_ADDR); > entry = KERNEL_LOAD_ADDR; > } > > if (kernel_size < 0) { > qemu_log("QEMU: couldn't load the kernel '%s'\n", > kernel_filename); > exit(1); > } > } > > cpu->env.pc = entry; > } > > static void openrisc_sim_init(ram_addr_t ram_size, > const char *boot_device, > const char *kernel_filename, > const char *kernel_cmdline, > const char *initrd_filename, > const char *cpu_model) > { > OpenRISCCPU *cpu = NULL; > MemoryRegion *ram; > int n; > > if (!cpu_model) { > cpu_model = "or1200"; > } > > for (n = 0; n < smp_cpus; n++) { > cpu = cpu_openrisc_init(cpu_model); > if (cpu == NULL) { > qemu_log("Unable to find CPU defineition!\n"); > exit(1); > } > qemu_register_reset(main_cpu_reset, cpu); > main_cpu_reset(cpu); > } > > ram = g_malloc(sizeof(*ram)); > memory_region_init_ram(ram, "openrisc.ram", ram_size); > vmstate_register_ram_global(ram); > memory_region_add_subregion(get_system_memory(), 0, ram); > > cpu_openrisc_pic_init(cpu); > cpu_openrisc_clock_init(cpu); > > serial_mm_init(get_system_memory(), 0x90000000, 0, cpu->env.irq[2], > 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN); > > if (nd_table[0].vlan) { > openrisc_sim_net_init(get_system_memory(), 0x92000000, > 0x92000400, cpu->env.irq[4], nd_table); > } > > cpu_openrisc_load_kernel(ram_size, kernel_filename, cpu); > } > >>> } >>> >>> static QEMUMachine openrisc_sim_machine = { >>> .name = "or32-sim", >>> .desc = "or32 simulation", >>> .init = openrisc_sim_init, >>> .max_cpus = 1, >>> .is_default = 1, >>> }; >>> >>> static void openrisc_sim_machine_init(void) >>> { >>> qemu_register_machine(&openrisc_sim_machine); >>> } >>> >>> machine_init(openrisc_sim_machine_init); >>> >>> >>> Please review and let me the new is OK or not, please. >>> Thank you, very much, all of you. >>> >>> >>> On Wed, Jun 27, 2012 at 9:10 PM, Peter Crosthwaite >>> <peter.crosthwa...@petalogix.com> wrote: >>>> On Wed, Jun 27, 2012 at 11:04 PM, Andreas Färber <afaer...@suse.de> wrote: >>>>> Am 27.06.2012 14:25, schrieb Peter Crosthwaite: >>>>>> Hi Jia, >>>>>> >>>>>> On Wed, Jun 27, 2012 at 7:54 PM, Jia Liu <pro...@gmail.com> wrote: >>>>>>> +static void openrisc_sim_init(ram_addr_t ram_size, >>>>>>> + const char *boot_device, >>>>>>> + const char *kernel_filename, >>>>>>> + const char *kernel_cmdline, >>>>>>> + const char *initrd_filename, >>>>>>> + const char *cpu_model) >>>>>>> +{ >>>>>>> + CPUOpenRISCState *env; >>>>>>> + MemoryRegion *ram = g_new(MemoryRegion, 1); >>>>>>> + >>>>>>> + if (!cpu_model) { >>>>>>> + cpu_model = "or1200"; >>>>>>> + } >>>>>>> + env = cpu_init(cpu_model); >>>>>>> + if (!env) { >>>>>>> + fprintf(stderr, "Unable to find CPU definition!\n"); >>>>>>> + exit(1); >>>>>>> + } >>>>>>> + >>>>>>> + qemu_register_reset(main_cpu_reset, env); >>>>>>> + main_cpu_reset(env); >>>>>>> + >>>>>> >>>>>> I think this needs rebasing. Andreas a while back abstracted back to >>>>>> the CPU level instead for resets. Andreas can you confirm? should this >>>>>> be changed to pass the CPU QOM object to the reset instead? cc >>>>>> andreas. >>>>> >>>>> Thought I had commented that already... maybe I'm starting to confuse >>>>> uc32 and or32? :) Yes please, cpu_or32_init() should be called and >>>>> return an OpenRISCCPU *cpu. main_cpu_reset() should be passed the cpu, >>>>> too. All new APIs (static helpers etc.) should use OpenRISCCPU, not >>>>> CPUOpenRISCState. >>>> >>>> That rule has significant impact on patches 9-10. Andreas, you may >>>> wish to check these out - they are psuedo device-models tightly >>>> coupled to the cpu env. >>>> >>>> Regards, >>>> Peter >>>> >>>> That will greatly simplify moving forward. >>>>> >>>>> Thanks for catching this, Peter. >>>>> >>>>> Andreas >>>>> >>>>> -- >>>>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >>>>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >>> >>> >>> Regards, >>> Jia. > > Regards, > Jia.