On 5/11/10, chen huacai <zltjiang...@gmail.com> wrote:
> >> + 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.
With qdev system, the board should map the device. In this old way,
you can just pass the (fixed) addresses from board level. It probably
makes conversion to qdev easier.
> >> + /*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
IO_MEM_UNASSIGNED is the default MMIO type, there shouldn't be any
need to map it (except to remove a previous mapping).
> >> + 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
In that case, perhaps the code should be committed after the design
has stabilized a bit more?
> For your other comments I'll improve my code, thanks.
>
> Best regards,
>
>
> Huacai Chen
>
