Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller

[Alexander Graf]

On 04.10.2012, at 17:54, Andreas Färber wrote:

> Am 03.10.2012 13:50, schrieb Bharat Bhushan:
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 197411d..c7ae2b6 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
>> 
>>     /* PCI */
>>     dev = qdev_create(NULL, "e500-pcihost");
>> +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
>>     qdev_init_nofail(dev);
>>     s = sysbus_from_qdev(dev);
>>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> 
> Please...
> 
>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>> index 92b1dc0..16e4af2 100644
>> --- a/hw/ppce500_pci.c
>> +++ b/hw/ppce500_pci.c
>> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>>     /* mmio maps */
>>     MemoryRegion container;
>>     MemoryRegion iomem;
>> +    void *bar0;
>> };
>> 
>> typedef struct PPCE500PCIState PPCE500PCIState;
> 
> ...do not do this. qdev_prop_set_ptr() is considered deprecated and we
> had long discussions how to solve this differently.
> 
> Was there anything wrong with using a SysBusDevice for the CCSR to
> encapsulate the MemoryRegion?

Not at all. This was meant as a first shot, so we can slowly move towards the 
CCSR-as-device model.
In fact, now that we have this code as far as it is, we can go over to tackle 
it that way.

Bharat, mind to model a new CCSR device now that contains all the CCSR devices?

That one creates a memory region then. The PCI host controller gets a reference 
to the CCSR device and from there can call a CCSR specific function to receive 
the memory region pointer. And suddenly we have all of this solved :).


Alex