Re: [Qemu-devel] [PATCH v5 2/5] hw/riscv/virt: Connect the gpex PCIe

2018-10-30 Thread Alistair Francis
On Thu, Oct 25, 2018 at 11:47 AM Peter Maydell  wrote:
>
> On 4 October 2018 at 21:06, Alistair Francis  wrote:
> > Connect the gpex PCIe device based on the device tree included in the
> > HiFive Unleashed ROM.
> >
> > Signed-off-by: Alistair Francis 
> > ---
> >  default-configs/riscv32-softmmu.mak |  6 ++-
> >  default-configs/riscv64-softmmu.mak |  6 ++-
> >  hw/riscv/virt.c | 58 +
> >  include/hw/riscv/virt.h |  4 +-
> >  4 files changed, 71 insertions(+), 3 deletions(-)
>
> > +static inline DeviceState *
> > +gpex_pcie_init(MemoryRegion *sys_mem, uint32_t bus_nr,
> > + hwaddr cfg_base, uint64_t cfg_size,
> > + hwaddr mmio_base, uint64_t mmio_size,
> > + qemu_irq irq, bool link_up)
> > +{
> > +DeviceState *dev;
> > +MemoryRegion *cfg, *mmio;
> > +
> > +dev = qdev_create(NULL, TYPE_GPEX_HOST);
> > +
> > +qdev_init_nofail(dev);
> > +
> > +cfg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> > +memory_region_add_subregion_overlap(sys_mem, cfg_base, cfg, 0);
> > +
> > +mmio = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> > +memory_region_add_subregion_overlap(sys_mem, 0, mmio, 0);
>
> You probably also want to map the gpex's mmio region 2,
> which is the PCI IO BAR window. Otherwise PCI devices
> with IO BARs won't work right.

Ah, I missed this one. Fixed in the next version.

>
> > +
> > +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
>
> GPEX has 4 interrupt lines, corresponding to classic PCI
> IRQs A, B, C, D. You need to wire them all up, or only the
> first PCI device plugged in will have a working interrupt.
> You also don't look to have the bit of device-tree generation
> that sets up the interrupt-map, which tells the guest about
> the mapping from PCI devfns (ie, which slot the card is in) to
> IRQ lines.

Yep, I based this on the Xilinx PCIe as that's what I started with. I
have updated to use an interrupt-map similar to the ARM virt board.
Interrupts look like they are working now.

>
> > +
> > +return dev;
> > +}
> > +
> >  static void riscv_virt_board_init(MachineState *machine)
> >  {
> >  const struct MemmapEntry *memmap = virt_memmap;
> > @@ -382,6 +436,10 @@ static void riscv_virt_board_init(MachineState 
> > *machine)
> >  qdev_get_gpio_in(DEVICE(s->plic), VIRTIO_IRQ + i));
> >  }
> >
> > +gpex_pcie_init(system_memory, 0, memmap[VIRT_PCIE].base,
> > +   memmap[VIRT_PCIE].size, 0x4000, 0x2000,
>
> You could put the MMIO BAR window base and size in your memmap[] too...

Fixed in v6.

Alistair

>
> > +   qdev_get_gpio_in(DEVICE(s->plic), PCIE_IRQ), 
> > true);
>
> thanks
> -- PMM



Re: [Qemu-devel] [PATCH v5 2/5] hw/riscv/virt: Connect the gpex PCIe

2018-10-25 Thread Peter Maydell
On 4 October 2018 at 21:06, Alistair Francis  wrote:
> Connect the gpex PCIe device based on the device tree included in the
> HiFive Unleashed ROM.
>
> Signed-off-by: Alistair Francis 
> ---
>  default-configs/riscv32-softmmu.mak |  6 ++-
>  default-configs/riscv64-softmmu.mak |  6 ++-
>  hw/riscv/virt.c | 58 +
>  include/hw/riscv/virt.h |  4 +-
>  4 files changed, 71 insertions(+), 3 deletions(-)

> +static inline DeviceState *
> +gpex_pcie_init(MemoryRegion *sys_mem, uint32_t bus_nr,
> + hwaddr cfg_base, uint64_t cfg_size,
> + hwaddr mmio_base, uint64_t mmio_size,
> + qemu_irq irq, bool link_up)
> +{
> +DeviceState *dev;
> +MemoryRegion *cfg, *mmio;
> +
> +dev = qdev_create(NULL, TYPE_GPEX_HOST);
> +
> +qdev_init_nofail(dev);
> +
> +cfg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +memory_region_add_subregion_overlap(sys_mem, cfg_base, cfg, 0);
> +
> +mmio = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> +memory_region_add_subregion_overlap(sys_mem, 0, mmio, 0);

You probably also want to map the gpex's mmio region 2,
which is the PCI IO BAR window. Otherwise PCI devices
with IO BARs won't work right.

> +
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);

GPEX has 4 interrupt lines, corresponding to classic PCI
IRQs A, B, C, D. You need to wire them all up, or only the
first PCI device plugged in will have a working interrupt.
You also don't look to have the bit of device-tree generation
that sets up the interrupt-map, which tells the guest about
the mapping from PCI devfns (ie, which slot the card is in) to
IRQ lines.

> +
> +return dev;
> +}
> +
>  static void riscv_virt_board_init(MachineState *machine)
>  {
>  const struct MemmapEntry *memmap = virt_memmap;
> @@ -382,6 +436,10 @@ static void riscv_virt_board_init(MachineState *machine)
>  qdev_get_gpio_in(DEVICE(s->plic), VIRTIO_IRQ + i));
>  }
>
> +gpex_pcie_init(system_memory, 0, memmap[VIRT_PCIE].base,
> +   memmap[VIRT_PCIE].size, 0x4000, 0x2000,

You could put the MMIO BAR window base and size in your memmap[] too...

> +   qdev_get_gpio_in(DEVICE(s->plic), PCIE_IRQ), 
> true);

thanks
-- PMM