Re: [Qemu-ppc] [PATCH for-2.10 6/8] ppc/pnv: Add cut down PSI bridge model and hookup external interrupt

[Cédric Le Goater]

[ ... ]

>>> +void pnv_psi_irq_set(PnvPsi *psi, PnvPsiIrq irq, bool state)
>>> +{
>>> +    ICSState *ics = &psi->ics;
>>> +    uint32_t xivr_reg;
>>> +    uint32_t stat_reg;
>>> +    uint64_t stat_bit;
>>> +    uint32_t src;
>>> +    bool masked;
>>> +
>>> +    if (!pnv_psi_irq_bits(psi, irq, &xivr_reg, &stat_reg, &stat_bit)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "PSI: Unsupported irq %d\n", irq);
>>> +        return;
>>> +    }
>>> +
>>> +    src = (psi->regs[xivr_reg] & PSIHB_XIVR_SRC_MSK) >> PSIHB_XIVR_SRC_SH;
>>> +    masked = (psi->regs[xivr_reg] & PSIHB_XIVR_PRIO_MSK) == 
>>> PSIHB_XIVR_PRIO_MSK;
>>> +    if (state) {
>>> +        psi->regs[stat_reg] |= stat_bit;
>>> +        /* XXX optimization: check mask here. That means re-evaluating
>>> +         * when unmasking, thus TODO
>>> +         */
>>> +        qemu_irq_raise(ics->qirqs[src]);
>>> +    } else {
>>> +        psi->regs[stat_reg] &= ~stat_bit;
>>> +
>>> +        /* FSP and PSI are muxed so don't lower if either still set */
>>> +        if (stat_reg != PSIHB_XSCOM_CR ||
>>> +            !(psi->regs[stat_reg] & (PSIHB_CR_PSI_IRQ | 
>>> PSIHB_CR_FSP_IRQ))) {
>>> +            qemu_irq_lower(ics->qirqs[src]);
>>> +        } else {
>>> +            state = true;

ugly.

>>> +        }
>>> +    }
>>
>> It might be cleaner to just revaluate the irq level from scratch here,
>> and set the level, rather than doing this complicated dance to work
>> out if it has changed.
> 
> OK. I need to take a closer look at that.

So I took a closer a look and some parts are not clear, even if 
correct. The FSP and PSI interrupts are muxed and the above code 
tries to re-conciliate the IRQ triggering in a single routine. 
But we ended up (I think) using some hacks. See below PnvPsiIrq.

[ ... ]

>>> +static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    PnvPsi *psi = PNV_PSI(dev);
>>> +    ICSState *ics = &psi->ics;
>>> +    Object *obj;
>>> +    Error *err = NULL, *local_err = NULL;
>>> +    unsigned int i;
>>> +
>>> +    /* Initialize MMIO region */
>>> +    memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>>> +                          "psihb", PNV_PSIHB_BAR_SIZE);
>>> +
>>> +    /* Default BAR. Use object properties ? */
>>> +    pnv_psi_set_bar(psi, PNV_PSIHB_BAR | PSIHB_BAR_EN);
>>> +
>>> +    /* Default sources in XIVR */
>>> +    psi->regs[PSIHB_XSCOM_XIVR_PSI] = PSIHB_XIVR_PRIO_MSK |
>>> +            (0ull << PSIHB_XIVR_SRC_SH);
>>> +    psi->regs[PSIHB_XSCOM_XIVR_OCC] = PSIHB_XIVR_PRIO_MSK |
>>> +            (1ull << PSIHB_XIVR_SRC_SH);
>>> +    psi->regs[PSIHB_XSCOM_XIVR_FSI] = PSIHB_XIVR_PRIO_MSK |
>>> +            (2ull << PSIHB_XIVR_SRC_SH);
>>> +    psi->regs[PSIHB_XSCOM_XIVR_LPCI2C] = PSIHB_XIVR_PRIO_MSK |
>>> +            (3ull << PSIHB_XIVR_SRC_SH);
>>> +    psi->regs[PSIHB_XSCOM_XIVR_LOCERR] = PSIHB_XIVR_PRIO_MSK |
>>> +            (4ull << PSIHB_XIVR_SRC_SH);
>>> +    psi->regs[PSIHB_XSCOM_XIVR_EXT] = PSIHB_XIVR_PRIO_MSK |
>>> +            (5ull << PSIHB_XIVR_SRC_SH);
>>> +

The above is not using a loop on PnvPsiIrq because the numbers
do not match the PSI IRQ definitions.

[ ... ]

>>> +
>>> +typedef enum PnvPsiIrq {
>>> +    PSIHB_IRQ_PSI, /* internal use only */
>>> +    PSIHB_IRQ_FSP, /* internal use only */
>>> +    PSIHB_IRQ_OCC,
>>> +    PSIHB_IRQ_FSI,
>>> +    PSIHB_IRQ_LPC_I2C,
>>> +    PSIHB_IRQ_LOCAL_ERR,
>>> +    PSIHB_IRQ_EXTERNAL,
>>> +} PnvPsiIrq;
>>> +
>>> +#define PSI_NUM_INTERRUPTS 6

a maximum of 6 interrupts for an enum containing 7 entries. It is a
little ugly.

I am going to rewrite this part in a more straight forward way.

C.