On Wed, May 12, 2010 at 3:52 AM, Blue Swirl <blauwir...@gmail.com> wrote:
> 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?
>
I think I haven't represent things clear, 0xdf53/0x00d5 is actually
stable and won't change any more, but we couldn't say 0xdf53 stands
for which vendor (may be stand for Lemote Inc.). In Linux kernel the
two values are used, and also not get defined in pci_ids.h
--
Huacai Chen
