Re: [Qemu-devel] [PATCH 03/21] ppc/pnv: Add support for POWER8+ LPC Controller

[Cédric Le Goater]

On 04/06/2017 04:02 AM, David Gibson wrote:
> On Wed, Apr 05, 2017 at 02:41:28PM +0200, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
>>
>> It adds the Naples chip which supports proper LPC interrupts via the
>> LPC controller rather than via an external CPLD.
>>
>> Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
>> [clg: - updated for qemu-2.9
>>       - ported on latest PowerNV patchset ]
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/pnv.c             | 13 ++++++++++++-
>>  hw/ppc/pnv_lpc.c         | 47 
>> +++++++++++++++++++++++++++++++++++++++++++++--
>>  include/hw/ppc/pnv_lpc.h |  9 +++++++++
>>  3 files changed, 66 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 24e523f554c6..78133e5d20e1 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -373,7 +373,14 @@ 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)
>>  {
>> -     /* XXX TODO */
>> +    PnvChip *chip = opaque;
>> +    PnvLpcController *lpc = &chip->lpc;
>> +
>> +    /* The Naples HW latches the 1 levels, clearing is done by SW */
>> +    if (level) {
>> +        lpc->lpc_hc_irqstat |= LPC_HC_IRQ_SERIRQ0 >> n;
>> +        pnv_lpc_eval_irqs(lpc);
>> +    }
>>  }
> 
> Now that you have a more complete LPC model, I think this function,
> and the allocation of the LPC irqs should move into pnv_lpc.c.
> 
> 
> Apart from that, looks fine.

While I am at changing things, we have a 'cpld_irqstate' under the 
machine to model the state of the CPLD chip. It is only used in : 

        static void pnv_lpc_isa_irq_handler_cpld(void *opaque, int n, int level)
        {
            PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
            uint32_t old_state = pnv->cpld_irqstate;
            PnvLpcController *lpc = opaque;

            if (level) {
                pnv->cpld_irqstate |= 1u << n;
            } else {
                pnv->cpld_irqstate &= ~(1u << n);
            }

            if (pnv->cpld_irqstate != old_state) {
                pnv_psi_irq_set(lpc->psi, PSIHB_IRQ_EXTERNAL, 
pnv->cpld_irqstate != 0);
            }
        }

May be we could move that under the LPC object even if it is not strictly
correct from a HW pov ? 

Thanks,

C.