Re: [PATCH 1/2] ppc/pnv: Fix loss of LPC SERIRQ interrupts

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

Thanks,

Glenn

On Tue, 2024-05-28 at 16:20 +1000, Nicholas Piggin wrote:
> From: Glenn Miles <milesg@linux.vnet.ibm.com>
> 
> The LPC HC irq status register bits are set when an LPC IRQSER input
> is
> asserted. These irq status bits drive the PSI irq to the CPU
> interrupt
> controller. The LPC HC irq status bits are cleared by software
> writing
> to the register with 1's for the bits to clear.
> 
> Existing register write was clearing the irq status bits even when
> the
> input was asserted, this results in interrupts being lost.
> 
> This fix changes the behavior to keep track of the device IRQ status
> in internal state that is separate from the irq status register, and
> only allowing the irq status bits to be cleared if the associated
> input is not asserted.
> 
> [np: rebased before P9 PSI SERIRQ patch, adjust changelog/comments]
> Signed-off-by: Glenn Miles <milesg@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  include/hw/ppc/pnv_lpc.h |  3 +++
>  hw/ppc/pnv_lpc.c         | 22 +++++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 5d22c45570..97c6872c3f 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -73,6 +73,9 @@ struct PnvLpcController {
>      uint32_t opb_irq_pol;
>      uint32_t opb_irq_input;
>  
> +    /* LPC device IRQ state */
> +    uint32_t lpc_hc_irq_inputs;
> +
>      /* LPC HC registers */
>      uint32_t lpc_hc_fw_seg_idsel;
>      uint32_t lpc_hc_fw_rd_acc_size;
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index d692858bee..252690dcaa 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -505,7 +505,14 @@ static void lpc_hc_write(void *opaque, hwaddr
> addr, uint64_t val,
>          pnv_lpc_eval_irqs(lpc);
>          break;
>      case LPC_HC_IRQSTAT:
> -        lpc->lpc_hc_irqstat &= ~val;
> +        /*
> +         * This register is write-to-clear for the IRQSER (LPC
> device IRQ)
> +         * status. However if the device has not de-asserted its
> interrupt
> +         * that will just raise this IRQ status bit again. Model
> this by
> +         * keeping track of the inputs and only clearing if the
> inputs are
> +         * deasserted.
> +         */
> +        lpc->lpc_hc_irqstat &= ~(val & ~lpc->lpc_hc_irq_inputs);
>          pnv_lpc_eval_irqs(lpc);
>          break;
>      case LPC_HC_ERROR_ADDRESS:
> @@ -803,11 +810,20 @@ static void pnv_lpc_isa_irq_handler_cpld(void
> *opaque, int n, int level)
>  static void pnv_lpc_isa_irq_handler(void *opaque, int n, int level)
>  {
>      PnvLpcController *lpc = PNV_LPC(opaque);
> +    uint32_t irq_bit = LPC_HC_IRQ_SERIRQ0 >> n;
>  
> -    /* The Naples HW latches the 1 levels, clearing is done by SW */
>      if (level) {
> -        lpc->lpc_hc_irqstat |= LPC_HC_IRQ_SERIRQ0 >> n;
> +        lpc->lpc_hc_irq_inputs |= irq_bit;
> +
> +        /*
> +      * The LPC HC in Naples and later latches LPC IRQ into a bit
> field in
> +      * the IRQSTAT register, and that drives the PSI IRQ to the IC.
> +      * Software clears this bit manually (see LPC_HC_IRQSTAT
> handler).
> +         */
> +        lpc->lpc_hc_irqstat |= irq_bit;
>          pnv_lpc_eval_irqs(lpc);
> +    } else {
> +        lpc->lpc_hc_irq_inputs &= ~irq_bit;
>      }
>  }
>