[Skiboot] [PATCH v2 04/17] xive/p9: introduce an operation backend for current and new drivers

[Cédric Le Goater]

On 24/09/2019 07:23, Oliver O'Halloran wrote:
> On Thu, 2019-09-12 at 19:22 +0200, Cédric Le Goater wrote:
>> These operations define the high level interface of the XIVE driver
>> with other OPAL drivers using interrupts : PHBs, PSI, NPU. Each XIVE
>> driver will need to define its custom set to operate. Driver probing
>> is done as before using the "compatible" property.
>>
>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>> ---
>>  include/xive.h |  24 +++++++++
>>  hw/xive.c      | 133 ++++++++++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 144 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/xive.h b/include/xive.h
>> index b57f1441d6a9..85d6e80d753c 100644
>> --- a/include/xive.h
>> +++ b/include/xive.h
>> @@ -59,4 +59,28 @@ void xive_source_mask(struct irq_source *is, uint32_t isn);
>> *snip*
>> +struct xive_ops xive_p9_ops = {
>> +	.name		    = "POWER9 Interrupt Controller (XIVE)",
>> +	.compat		    = "ibm,power9-xive-x",
>> +
>> +	.init		    = xive_p9_init,
>> +	.late_init	    = xive_p9_late_init,
>> +	.reset		    = xive_p9_reset,
>> +	.alloc_hw_irqs	    = xive_p9_alloc_hw_irqs,
>> +	.alloc_ipi_irqs	    = xive_p9_alloc_ipi_irqs,
>> +	.get_trigger_port   = xive_p9_get_trigger_port,
>> +	.get_notify_port    = xive_p9_get_notify_port,
>> +	.get_notify_base    = xive_p9_get_notify_base,
>> +	.register_hw	    = xive_p9_register_hw_source,
>> +	.register_ipi	    = xive_p9_register_ipi_source,
>> +	.cpu_reset	    = xive_p9_cpu_reset,
>> +	.cpu_callin	    = xive_p9_cpu_callin,
>> +};
> 
> Do we have to use an ops struct? 

It makes the transition easier for the other drivers.

> The layer of indirection in the PHB
> code is annoying at the best of times, but it's necessary there since
> we have to deal with multiple PHB types (PHB4, OpenCAPI, NPU) existing
> on a system simultaneously. For XIVE we can assume there's only one
> flavour of XIVE at once so we could use:
> 
> 	if (xive_gen == xive_gen_1)

let's use 'proc_gen' then. There is no need of a 'xive_gen' toggle I think.

  P9    is XIVE1
  Pblah is XIVE2

XIVE2 has some configuration bits changing the TIMA layout to a compatible 
P9 layout. This is referred as XIVE2 gen1 but OPAL is still handling a 
XIVE2 controller and it only impacts the OS interrupt management in PowerNV 
and sPAPR. 

New XIVE2 features are sometime referred as Gen2 because they are disabled
when the P9 compat bit (Gen1) is on.  

> 		xive_gen1_blah()
> 	else
> 		xive_gen2_blah()
> 
> Which makes the code easier to browse. 

In terms of XIVE API, we have IRQ allocation :

hw/phb4.c:	irq_base = xive_alloc_hw_irqs(p->chip_id, p->num_irqs, ...
hw/npu3.c:	base = xive_alloc_ipi_irqs(npu->chip_id, NPU3_IRQ_LEVELS, ...
hw/npu2-common.c:	p->base_lsi = xive_alloc_ipi_irqs(p->chip_id, ...
hw/psi.c:	psi->interrupt = xive_alloc_hw_irqs(chip->id, P9_PSI_NUM_IRQS,

I agree that the phb5 won't be using a p9 XIVE API for instance, nor will 
the npu4 (unless npu3 is used for Pblah). So it makes sense. I will give  
it a try and duplicate some code in PSI.


These XIVE APIs are more generic : 

  core/init.c:	xive_cpu_callin(cpu);
  core/fast-reboot.c:		xive_cpu_reset();
  core/fast-reboot.c:		xive_reset();

We might want to handle the differences below the generic code.


Finally, we have the configuration of the trigger address 

hw/phb4.c:		 xive_get_notify_base(p->base_msi));
hw/phb4.c:	p->irq_port = xive_get_notify_port(p->chip_id,
hw/npu3.c:		       (uint64_t)xive_get_trigger_port(base) >> 12);
hw/npu2-common.c:	tp = xive_get_trigger_port(p->base_lsi);
hw/psi.c:	val = xive_get_notify_port(psi->chip_id, XIVE_HW_SRC_PSI);
hw/psi.c:	val = xive_get_notify_base(psi->interrupt);

That might change in Pblah if we use the IC ESB pages. PSI will still use
the old method though. 

> It's a bit more boilerplate, but
> we can bury it in the accessors the generic code uses. 

yes. 

> I'll leave
> deciding what to do up to you though since I expect you'll be staring
> at xive.c more than I will :)
> 
>> +static struct xive_ops *xive_ops_table[] = {
>> +	&xive_p9_ops,
>> +};
>> +
> 
>> +static struct xive_ops *xive_ops_match(void)
>> +{
>> +	struct dt_node *np;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(xive_ops_table); i++) {
>> +		dt_for_each_compatible(dt_root, np, xive_ops_table[i]->compat)
>> +			return xive_ops_table[i];
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> You can probably fold this into xive_init().
> 
>> +
>> +void init_xive(void)
>> +{
>> +	xive_ops = xive_ops_match();
>> +	if (!xive_ops) {
>> +		return;
>> +	}
> 
> ditch the {}

OK. 

> 
>> +
>> +	prlog(PR_INFO, "XIVE: found %s\n", xive_ops->name);
>> +	xive_ops->init(xive_ops->compat);
> 
> I don't really get why we're passing the compat to the init function
> since we already matched on it. 

because the init function is looping on the DT to find all XIVE nodes.

I can rework that part with the API rework.

Thanks,

C.


> Are you planning on sharing code
> between generations?
> 
>> +}
>