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

[Oliver O'Halloran]

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? 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?

> +}