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

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@kaod.org>
---
 include/xive.h |  24 +++++++++
 hw/xive.c      | 133 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 144 insertions(+), 13 deletions(-)

[ ... ] 


> +void xive_cpu_callin(struct cpu_thread *cpu)
> +{


This is missing an extra test :

	if (!cpu->xstate)
		return;

to filter the xive call being done in  __secondary_cpu_entry 
on all platforms. 

I will wait for some more comments before sending a v3.

C.

> +	return xive_ops->cpu_callin(cpu);
> +}
> +
> +void *xive_get_trigger_port(uint32_t girq)
> +{
> +	return xive_ops->get_trigger_port(girq);
> +}
> +
> +void xive_register_hw_source(uint32_t base, uint32_t count, uint32_t shift,
> +			     void *mmio, uint32_t flags, void *data,
> +			     const struct irq_source_ops *ops)
> +{
> +	xive_ops->register_hw(base, count, shift, mmio, flags, data, ops);
> +}
> +
> +void xive_register_ipi_source(uint32_t base, uint32_t count, void *data,
> +			      const struct irq_source_ops *ops)
> +{
> +	xive_ops->register_ipi(base, count, data, ops);
> +}
> +
> +void xive_cpu_reset(void)
> +{
> +	xive_ops->cpu_reset();
> +}
> +
> +void xive_late_init(void)
> +{
> +	xive_ops->late_init();
> +}
> +
> +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,
> +};
> +
> +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;
> +}
> +
> +void init_xive(void)
> +{
> +	xive_ops = xive_ops_match();
> +	if (!xive_ops) {
> +		return;
> +	}
> +
> +	prlog(PR_INFO, "XIVE: found %s\n", xive_ops->name);
> +	xive_ops->init(xive_ops->compat);
> +}
>

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@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? 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)
		xive_gen1_blah()
	else
		xive_gen2_blah()

Which makes the code easier to browse. It's a bit more boilerplate, but
we can bury it in the accessors the generic code uses. 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 {}

> +
> +	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. Are you planning on sharing code
between generations?

> +}

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@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?
> 
>> +}
>