Re: [PATCH] ppc/pnv: Implement POWER9 LPC PSI serirq outputs and auto-clear function

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon May 13, 2024 at 9:49 PM AEST, Cédric Le Goater wrote:
> Hello,
>
> On 5/10/24 16:30, Nicholas Piggin wrote:
> > The POWER8 LPC ISA device irqs all get combined and reported to the line
> > connected the PSI LPCHC irq. POWER9 changed this so only internal LPC
> > host controller irqs use that line, and the device irqs get routed to
> > 4 new lines connected to PSI SERIRQ0-3.
> > 
> > POWER9 also introduced a new feature that automatically clears the irq
> > status in the LPC host controller when EOI'ed, so software does not have
> > to.
> > 
> > The powernv OPAL (skiboot) firmware managed to work because the LPCHC
> > irq handler scanned all LPC irqs and handled those including clearing
> > status even on POWER9 systems. So LPC irqs worked despite OPAL thinking
> > it was running in POWER9 mode. After this change, UART interrupts show
> > up on serirq1 which is where OPAL routes them to:
> > 
> >   cat /proc/interrupts
> >   ...
> >   20:          0  XIVE-IRQ 1048563 Level     opal-psi#0:lpchc
> >   ...
> >   25:         34  XIVE-IRQ 1048568 Level     opal-psi#0:lpc_serirq_mux1
> > 
> > Whereas they previously turn up on lpchc.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   include/hw/ppc/pnv_lpc.h |  12 ++++-
> >   hw/ppc/pnv.c             |  38 +++++++++++++--
> >   hw/ppc/pnv_lpc.c         | 100 +++++++++++++++++++++++++++++++++++----
> >   3 files changed, 136 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> > index 5d22c45570..57e324b4dc 100644
> > --- a/include/hw/ppc/pnv_lpc.h
> > +++ b/include/hw/ppc/pnv_lpc.h
> > @@ -84,8 +84,18 @@ struct PnvLpcController {
> >       /* XSCOM registers */
> >       MemoryRegion xscom_regs;
> >   
> > +    /*
> > +     * In P8, ISA irqs are combined with internal sources to drive the
> > +     * LPCHC interrupt output. P9 ISA irqs raise one of 4 lines that
> > +     * drive PSI SERIRQ irqs, routing according to OPB routing registers.
> > +     */
> > +    bool psi_serirq;
> > +
> >       /* PSI to generate interrupts */
> > -    qemu_irq psi_irq;
> > +    qemu_irq psi_irq_lpchc;
> > +
> > +    /* P9 introduced a serirq mode */
> > +    qemu_irq psi_irq_serirq[4];
> >   };
> >   
> >   struct PnvLpcClass {
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 6e3a5ccdec..3b1c05a1d8 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -744,18 +744,48 @@ static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
> >   static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
> >   {
> >       Pnv9Chip *chip9 = PNV9_CHIP(chip);
> > -    qemu_irq irq = qdev_get_gpio_in(DEVICE(&chip9->psi), PSIHB9_IRQ_LPCHC);
> >   
> > -    qdev_connect_gpio_out(DEVICE(&chip9->lpc), 0, irq);
>
> The pnv_chip_power8*_isa_create() routines also need an update.

Good catch, thank you.

> > +    qdev_connect_gpio_out_named(DEVICE(&chip9->lpc), "LPCHC", 0,
> > +                                qdev_get_gpio_in(DEVICE(&chip9->psi),
> > +                                PSIHB9_IRQ_LPCHC));
> > +
> > +    qdev_connect_gpio_out_named(DEVICE(&chip9->lpc), "SERIRQ", 0,
> > +                                qdev_get_gpio_in(DEVICE(&chip9->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ0));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip9->lpc), "SERIRQ", 1,
> > +                                qdev_get_gpio_in(DEVICE(&chip9->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ1));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip9->lpc), "SERIRQ", 2,
> > +                                qdev_get_gpio_in(DEVICE(&chip9->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ2));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip9->lpc), "SERIRQ", 3,
> > +                                qdev_get_gpio_in(DEVICE(&chip9->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ3));
> > +
> >       return pnv_lpc_isa_create(&chip9->lpc, false, errp);
> >   }
> >   
> >   static ISABus *pnv_chip_power10_isa_create(PnvChip *chip, Error **errp)
> >   {
> >       Pnv10Chip *chip10 = PNV10_CHIP(chip);
> > -    qemu_irq irq = qdev_get_gpio_in(DEVICE(&chip10->psi), PSIHB9_IRQ_LPCHC);
> >   
> > -    qdev_connect_gpio_out(DEVICE(&chip10->lpc), 0, irq);
> > +    qdev_connect_gpio_out_named(DEVICE(&chip10->lpc), "LPCHC", 0,
> > +                                qdev_get_gpio_in(DEVICE(&chip10->psi),
> > +                                PSIHB9_IRQ_LPCHC));
> > +
> > +    qdev_connect_gpio_out_named(DEVICE(&chip10->lpc), "SERIRQ", 0,
> > +                                qdev_get_gpio_in(DEVICE(&chip10->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ0));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip10->lpc), "SERIRQ", 1,
> > +                                qdev_get_gpio_in(DEVICE(&chip10->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ1));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip10->lpc), "SERIRQ", 2,
> > +                                qdev_get_gpio_in(DEVICE(&chip10->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ2));
> > +    qdev_connect_gpio_out_named(DEVICE(&chip10->lpc), "SERIRQ", 3,
> > +                                qdev_get_gpio_in(DEVICE(&chip10->psi),
> > +                                PSIHB9_IRQ_LPC_SIRQ3));
> > +
> >       return pnv_lpc_isa_create(&chip10->lpc, false, errp);
> >   }
> >   
> > diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> > index d692858bee..e28eae672f 100644
> > --- a/hw/ppc/pnv_lpc.c
> > +++ b/hw/ppc/pnv_lpc.c
> > @@ -64,6 +64,7 @@ enum {
> >   #define   LPC_HC_IRQSER_START_4CLK      0x00000000
> >   #define   LPC_HC_IRQSER_START_6CLK      0x01000000
> >   #define   LPC_HC_IRQSER_START_8CLK      0x02000000
> > +#define   LPC_HC_IRQSER_AUTO_CLEAR      0x00800000
> >   #define LPC_HC_IRQMASK          0x34    /* same bit defs as LPC_HC_IRQSTAT */
> >   #define LPC_HC_IRQSTAT          0x38
> >   #define   LPC_HC_IRQ_SERIRQ0            0x80000000 /* all bits down to ... */
> > @@ -420,6 +421,34 @@ static const MemoryRegionOps pnv_lpc_mmio_ops = {
> >       .endianness = DEVICE_BIG_ENDIAN,
> >   };
> >   
> > +/* POWER9 serirq routing, see below */
> > +static int irq_to_serirq_route[ISA_NUM_IRQS];
>
> This static is not friendly for multichip machines. Could we avoid it and
> move the IRQ routes under PnvLpcController ?

Ah yes I don't know what I was thinking. It's trivial to move under
the controller. I will post a v2 in a bit.

Thanks,
Nick