Re: [Qemu-ppc] [PATCH 13/17] e500: move PCI host bridge into CCSR

[Michael Davidsaver]

On 12/05/2017 01:53 AM, David Gibson wrote:
> On Sun, Nov 26, 2017 at 03:59:11PM -0600, Michael Davidsaver wrote:
>> Signed-off-by: Michael Davidsaver <address@hidden>
> 
> Hmm.  Is there anything you're *not* planning to move under the CCSR.

Well, the decrementer/timebase initialization for one as this has
nothing to do with the CCSR registers.

I haven't added the TSEC/eTSEC instances either.
Partly this is because the existing boards, for reasons I don't understand,
use virtio NICs.

Further, the mpc8540 has TSEC instances 1 and 2, while the mpc8544
has instances 1 and 3.  So I decided to leave NIC setup to the Machine
rather then add the extra code to parameterize this under the CCSR device.

> If not, I'm really wondering if the CCSR ought to be a device in its
> own right, rather than just a container memory region used within the
> machine.

I don't think I follow what you mean by "device" in this context?
The CCSR object is a SysBusDevice in the qom tree ("/machine/e500-ccsr").
What device-like characteristics could it have?


>> ---
>>  hw/ppc/e500.c      | 13 ++++---------
>>  hw/ppc/e500_ccsr.c | 27 +++++++++++++++++++++++++++
>>  2 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index cfd5ed0152..b0c8495aef 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -769,6 +769,8 @@ void ppce500_init(MachineState *machine, PPCE500Params 
>> *params)
>>      qdev_prop_set_uint32(dev, "mpic-model", params->mpic_version);
>>      qdev_prop_set_uint32(dev, "base", params->ccsrbar_base);
>>      qdev_prop_set_uint32(dev, "ram-size", ram_size);
>> +    qdev_prop_set_uint32(dev, "pci_first_slot", params->pci_first_slot);
>> +    qdev_prop_set_uint32(dev, "pci_first_pin_irq", pci_irq_nrs[0]);
>>      qdev_init_nofail(dev);
>>      ccsr_addr_space = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>>  
>> @@ -778,20 +780,13 @@ void ppce500_init(MachineState *machine, PPCE500Params 
>> *params)
>>  
>>  
>>      /* PCI */
>> -    dev = qdev_create(NULL, "e500-pcihost");
>> -    object_property_add_child(qdev_get_machine(), "pci-host", OBJECT(dev),
>> -                              &error_abort);
>> -    qdev_prop_set_uint32(dev, "first_slot", params->pci_first_slot);
>> -    qdev_prop_set_uint32(dev, "first_pin_irq", pci_irq_nrs[0]);
>> -    qdev_init_nofail(dev);
>> +    dev = DEVICE(object_resolve_path("/machine/pci-host", 0));
>> +    assert(dev);
>>      s = SYS_BUS_DEVICE(dev);
>>      for (i = 0; i < PCI_NUM_PINS; i++) {
>>          sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, pci_irq_nrs[i]));
>>      }
>>  
>> -    memory_region_add_subregion(ccsr_addr_space, MPC8544_PCI_REGS_OFFSET,
>> -                                sysbus_mmio_get_region(s, 0));
>> -
>>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>>      if (!pci_bus)
>>          printf("couldn't create PCI controller!\n");
>> diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
>> index cd8216daaf..4ec8f7524d 100644
>> --- a/hw/ppc/e500_ccsr.c
>> +++ b/hw/ppc/e500_ccsr.c
>> @@ -50,6 +50,8 @@
>>  
>>  #define E500_DUART_OFFSET(N) (0x4500 + (N) * 0x100)
>>  
>> +#define E500_PCI_OFFSET  (0x8000ULL)
>> +
>>  #define E500_PORPLLSR    (0xE0000)
>>  #define E500_PVR         (0xE00A0)
>>  #define E500_SVR         (0xE00A4)
>> @@ -75,6 +77,7 @@ typedef struct {
>>  
>>      DeviceState *pic;
>>      DeviceState *i2c;
>> +    DeviceState *pcihost;
>>  } CCSRState;
>>  
>>  #define TYPE_E500_CCSR "e500-ccsr"
>> @@ -201,6 +204,7 @@ static void e500_ccsr_init(Object *obj)
>>      DeviceState *dev = DEVICE(obj);
>>      CCSRState *ccsr = E500_CCSR(dev);
>>  
>> +    /* prepare MPIC */
>>      assert(current_machine);
>>      if (kvm_enabled()) {
>>  
>> @@ -228,6 +232,18 @@ static void e500_ccsr_init(Object *obj)
>>      object_property_add_alias(obj, "mpic-model",
>>                                OBJECT(ccsr->pic), "model",
>>                                &error_fatal);
>> +
>> +    /* prepare PCI host bridge */
>> +    ccsr->pcihost = qdev_create(NULL, "e500-pcihost");
>> +    object_property_add_child(qdev_get_machine(), "pci-host", 
>> OBJECT(ccsr->pcihost),
>> +                              &error_abort);
>> +
>> +    object_property_add_alias(obj, "pci_first_slot",
>> +                              OBJECT(ccsr->pcihost), "first_slot",
>> +                              &error_fatal);
>> +    object_property_add_alias(obj, "pci_first_pin_irq",
>> +                              OBJECT(ccsr->pcihost), "first_pin_irq",
>> +                              &error_fatal);
>>  }
>>  
>>  static void e500_ccsr_realize(DeviceState *dev, Error **errp)
>> @@ -240,6 +256,7 @@ static void e500_ccsr_realize(DeviceState *dev, Error 
>> **errp)
>>                            ccsr, "e500-ccsr", 1024 * 1024);
>>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &ccsr->iomem);
>>  
>> +    /* realize MPIC */
>>      qdev_init_nofail(ccsr->pic);
>>      pic = SYS_BUS_DEVICE(ccsr->pic);
>>  
>> @@ -275,6 +292,13 @@ static void e500_ccsr_realize(DeviceState *dev, Error 
>> **errp)
>>                                  sysbus_mmio_get_region(pic, 0));
>>      /* Note: MPIC internal interrupts are offset by 16 */
>>  
>> +    /* realize PCI host bridge*/
>> +    qdev_init_nofail(ccsr->pcihost);
>> +
>> +    memory_region_add_subregion(&ccsr->iomem, E500_PCI_OFFSET,
>> +                                sysbus_mmio_get_region(
>> +                                    SYS_BUS_DEVICE(ccsr->pcihost), 0));
>> +
>>      /* attach I2C controller */
>>      ccsr->i2c = qdev_create(NULL, "mpc8540-i2c");
>>      object_property_add_child(qdev_get_machine(), "i2c[*]",
>> @@ -314,6 +338,9 @@ static Property e500_ccsr_props[] = {
>>      DEFINE_PROP_UINT32("porpllsr", CCSRState, porpllsr, 0),
>>      DEFINE_PROP_UINT32("ccb-freq", CCSRState, ccb_freq, 333333333u),
>>      /* "mpic-model" aliased from MPIC */
>> +    /* "pci_first_slot"
>> +     * "pci_first_pin_irq" aliased from PCI host bridge
>> +     */
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>  
>

signature.asc
Description: OpenPGP digital signature