Re: [Qemu-devel] [PATCH 13/17] e500: move PCI host bridge into CCSR
On 12/06/2017 05:11 AM, David Gibson wrote:
> On Tue, Dec 05, 2017 at 10:42:25PM -0500, Michael Davidsaver wrote:
>> 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
>>>
>>> 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.
>
> Right, but no actual devices, even small ones?
Well, there is the GPIO controller ("mpc8xxx_gpio") as I have frankly
have no idea where it comes from. Neither MPC8540 nor 8544 define
anything at CCSR offset 0xFF000. The registers modeled differ
from the GPIO controller which is defined at 0xE0030.
So I'm considering this to be specific to the existing boards.
>> 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?
>
> Sorry, I wasn't clear. the CCSR definitely *is* a device in the
> current scheme, but I'm wondering if that was a good idea.
I think I see what you're getting at. You're right that CCSR
isn't a "device" in the same sense as eg. a UART. In my mind
it's more like a PCI host bridge, having a few registers itself,
though serving mainly to route to the devices behind it.
I see the CCSR "device" as the bridge onto the system bus.
In some ways it might be considered to be the only device
on the system bus apart from the CPU(s). This "device"
handles first level routing of physical addresses to
RAM, PCI, or local bus via the LAWBAR* registers (unmodeled).
Or to the I/O devices in the CCSR range via CCSRBAR.
I don't have plans to model that LAWBAR* registers,
as it hasn't proved necessary for RTEMS or Linux guests.
I have considered how this could be done as Linux does
touch these registers, but doesn't readback/check the
values it has written. I think having a CCSR "device"
would make it simpler to model the local windows
should this become desirable. eg. if Linux starts
validating LAWBAR* or to run unmodified u-boot.
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 13/17] e500: move PCI host bridge into CCSR
On Tue, Dec 05, 2017 at 10:42:25PM -0500, Michael Davidsaver wrote:
> 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
> >
> > 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.
Right, but no actual devices, even small ones?
> 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?
Sorry, I wasn't clear. the CCSR definitely *is* a device in the
current scheme, but I'm wondering if that was a good idea.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 13/17] e500: move PCI host bridge into CCSR
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
>
> 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(0xE)
>> #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
Re: [Qemu-devel] [PATCH 13/17] e500: move PCI host bridge into CCSR
On Sun, Nov 26, 2017 at 03:59:11PM -0600, Michael Davidsaver wrote:
> Signed-off-by: Michael Davidsaver
Hmm. Is there anything you're *not* planning to move under the CCSR.
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.
> ---
> 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(0xE)
> #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,
