Re: [Qemu-devel] [PATCH v3 10/10] ppc/pnv: add a ISA bus

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5275 bytes --]

On Thu, Sep 15, 2016 at 02:46:00PM +0200, Cédric Le Goater wrote:
> As Qemu only supports a single instance of the ISA bus, we use the LPC
> controller of chip 0 to create one and plug in a couple of useful
> devices, like an UART and RTC. An IPMI BT device, which is also an ISA
> device, can be defined on the command line to connect an external BMC.
> That is for later.
> 
> The PowerNV machine now has a console. Skiboot should load a kernel
> and jump into it but execution will stop quite early because we lack a
> model for the native XICS controller for the moment :
> 
>     [    0.000000] NR_IRQS:512 nr_irqs:512 16
>     [    0.000000] XICS: Cannot find a Presentation Controller !
>     [    0.000000] ------------[ cut here ]------------
>     [    0.000000] WARNING: at arch/powerpc/platforms/powernv/setup.c:81
>     ...
>     [    0.000000] NIP [c00000000079d65c] pnv_init_IRQ+0x30/0x44
> 
> You can still do a few things under xmon.
> 
> Based on previous work from :
>       Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

This looks pretty good, just a couple of minor queries below.

> ---
>  hw/ppc/pnv.c         | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h |  2 ++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1aa7b8ee8903..fd6e4917133b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -34,6 +34,10 @@
>  
>  #include "hw/ppc/pnv_xscom.h"
>  
> +#include "hw/isa/isa.h"
> +#include "hw/char/serial.h"
> +#include "hw/timer/mc146818rtc.h"
> +
>  #include <libfdt.h>
>  
>  #define FDT_MAX_SIZE            0x00100000
> @@ -301,6 +305,57 @@ static void ppc_powernv_reset(void)
>      cpu_physical_memory_write(POWERNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>  }
>  
> +/* If we don't use the built-in LPC interrupt deserializer, we need
> + * to provide a set of qirqs for the ISA bus or things will go bad.
> + *
> + * Most machines using pre-Naples chips (without said deserializer)
> + * have a CPLD that will collect the SerIRQ and shoot them as a
> + * single level interrupt to the P8 chip. So let's setup a hook
> + * for doing just that.
> + *
> + * Note: The actual interrupt input isn't emulated yet, this will
> + * come with the PSI bridge model.
> + */
> +static void pnv_lpc_isa_irq_handler_cpld(void *opaque, int n, int level)
> +{
> +    /* We don't yet emulate the PSI bridge which provides the external
> +     * interrupt, so just drop interrupts on the floor
> +     */
> +}
> +
> +static void pnv_lpc_isa_irq_handler(void *opaque, int n, int level)
> +{
> +     /* XXX TODO */
> +}
> +
> +static ISABus *pnv_isa_create(PnvChip *chip)
> +{
> +    PnvLpcController *lpc = &chip->lpc;
> +    ISABus *isa_bus;
> +    qemu_irq *irqs;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +
> +    /* Instanciate ISA bus. let isa_bus_new() create its own bridge on

Instantiate has 3 't's and no 'c's; English orthography strikes again.

> +     * sysbus otherwise devices speficied on the command line will
> +     * fail to create.
> +     */
> +    isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io,
> +                          &error_fatal);

It's not clear to me if this belongs in the chip code or on the lpc
code - the lpc does create a device node as 'isa@', although it also
does some other stuff.

> +
> +    /* Not all variants have a working serial irq decoder. If not,
> +     * handling of LPC interrupts becomes a platform issue (some
> +     * platforms have a CPLD to do it).
> +     */
> +    if (pcc->chip_type == PNV_CHIP_POWER8NVL) {
> +        irqs = qemu_allocate_irqs(pnv_lpc_isa_irq_handler, lpc, 16);
> +    } else {
> +        irqs = qemu_allocate_irqs(pnv_lpc_isa_irq_handler_cpld, NULL, 16);
> +    }
> +
> +    isa_bus_irqs(isa_bus, irqs);
> +    return isa_bus;
> +}
> +
>  static void ppc_powernv_init(MachineState *machine)
>  {
>      PnvMachineState *pnv = POWERNV_MACHINE(machine);
> @@ -389,6 +444,15 @@ static void ppc_powernv_init(MachineState *machine)
>          object_property_set_bool(chip, true, "realized", &error_fatal);
>      }
>      g_free(chip_typename);
> +
> +    /* Instanciate ISA bus on chip 0 */
> +    pnv->isa_bus = pnv_isa_create(pnv->chips[0]);
> +
> +    /* Create serial port */
> +    serial_hds_isa_init(pnv->isa_bus, MAX_SERIAL_PORTS);
> +
> +    /* Create an RTC ISA device too */
> +    rtc_init(pnv->isa_bus, 2000, NULL);
>  }
>  
>  static uint32_t pnv_chip_core_pir_p8(PnvChip *chip, uint32_t core_id)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index a30579a5817f..e75f937d40dd 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -123,6 +123,8 @@ typedef struct PnvMachineState {
>  
>      uint32_t  num_chips;
>      PnvChip   **chips;
> +
> +    ISABus *isa_bus;
>  } PnvMachineState;
>  
>  #define POWERNV_FDT_ADDR                0x01000000

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]