>> + s->pci = qemu_mallocz(sizeof(*s->pci));
>> + assert(s->pci != NULL);
>> + bonito_state = s;
>> +
>> + /* get the north bridge pci bus */
>> + s->pci->bus = pci_register_bus(NULL, "pci", pci_bonito_set_irq,
>> + pci_bonito_map_irq, pic, 0x28, 32);
>> +
>> + /* set the north bridge register mapping */
>> + s->bonito_reg_handle = cpu_register_io_memory(bonito_read,
>> bonito_write, s);
>> + s->bonito_reg_start = BONITO_INTERNAL_REG_BASE;
>
> Usually the devices don't specify their addresses, but these are
> passed from the board level.
I'm a bit confusing here, bonito internal registers are mapped to a
fixed physical address according to specification.
>> + /*add PCI io space */
>> + /*PCI IO Space 0x1fd0 0000 - 0x1fd1 0000 */
>> + if (s->bonito_pciio_length)
>> + {
>> + cpu_register_physical_memory(s->bonito_pciio_start,
>> + s->bonito_pciio_length,
>> IO_MEM_UNASSIGNED);
>
> Why would this be needed?
This is borrowed from gt64xxx.c
>> + d = pci_register_device(s->pci->bus, "Bonito PCI Bus",
>> sizeof(PCIDevice),
>> + 0, bonito_read_config, bonito_write_config);
>> +
>> + pci_config_set_vendor_id(d->config, 0xdf53); //Bonito North Bridge
>> + pci_config_set_device_id(d->config, 0x00d5);
>
> Please put the above constants to hw/pci_ids.h.
Bonito north bridge is built on FPGA now, VENDOR_ID/DEVICE_ID are
temporary value so I didn't put them in pci_ids.h
For your other comments I'll improve my code, thanks.
Best regards,
Huacai Chen
