Re: [PATCH v2 2/2] irqchip/sifive-plic: Add support for Renesas RZ/Five SoC

[Marc Zyngier]

On Sun, 26 Jun 2022 01:43:26 +0100,
Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> 
> The Renesas RZ/Five SoC has a RISC-V AX45MP AndesCore with NCEPLIC100. The
> NCEPLIC100 supports both edge-triggered and level-triggered interrupts. In
> case of edge-triggered interrupts NCEPLIC100 ignores the next interrupt
> edge until the previous completion message has been received and
> NCEPLIC100 doesn't support pending interrupt counter, hence losing the
> interrupts if not acknowledged in time.
> 
> So the workaround for edge-triggered interrupts to be handled correctly
> and without losing is that it needs to be acknowledged first and then
> handler must be run so that we don't miss on the next edge-triggered
> interrupt.
> 
> This patch adds a new compatible string for Renesas RZ/Five SoC and adds
> support to change interrupt flow based on the interrupt type. It also
> implements irq_ack and irq_set_type callbacks.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v1->v2:
> * Implemented IRQ flow as suggested by Marc
> 
> RFC-->v1:
> * Fixed review comments pointed by Geert
> * Dropped handle_fasteoi_ack_irq support as for the PLIC we need to
> claim the interrupt by reading the register and then acknowledge it.
> * Add a new chained handler for RZ/Five SoC.
> ---
>  drivers/irqchip/Kconfig           |  1 +
>  drivers/irqchip/irq-sifive-plic.c | 73 ++++++++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 2 deletions(-)

[...]

>
> +static int plic_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +
> +	if (handler->priv->of_data != RENESAS_R9A07G043_PLIC)
> +		return 0;
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		irq_set_chip_handler_name_locked(d, &renesas_rzfive_edge_plic_chip,
> +						 handle_fasteoi_ack_irq,
> +						 "Edge");
> +		break;
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +		irq_set_chip_handler_name_locked(d, &plic_chip,
> +						 handle_fasteoi_irq,
> +						 "Level");
> +		break;

Really? Have you even tested this?

> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  			      irq_hw_number_t hwirq)
>  {
> @@ -198,6 +248,19 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  	return 0;
>  }
>  
> +static int plic_irq_domain_translate(struct irq_domain *d,
> +				     struct irq_fwspec *fwspec,
> +				     unsigned long *hwirq,
> +				     unsigned int *type)
> +{
> +	struct plic_priv *priv = d->host_data;
> +
> +	if (priv->of_data == RENESAS_R9A07G043_PLIC)
> +		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> +
> +	return irq_domain_translate_onecell(d, fwspec, hwirq, type);
> +}
> +
>  static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  				 unsigned int nr_irqs, void *arg)
>  {
> @@ -206,7 +269,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  	unsigned int type;
>  	struct irq_fwspec *fwspec = arg;
>  
> -	ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> +	ret = plic_irq_domain_translate(domain, fwspec, &hwirq, &type);
>  	if (ret)
>  		return ret;
>  
> @@ -220,7 +283,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  }
>  
>  static const struct irq_domain_ops plic_irqdomain_ops = {
> -	.translate	= irq_domain_translate_onecell,
> +	.translate	= plic_irq_domain_translate,
>  	.alloc		= plic_irq_domain_alloc,
>  	.free		= irq_domain_free_irqs_top,
>  };
> @@ -293,6 +356,11 @@ static int __init plic_init(struct device_node *node,
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	if (of_device_is_compatible(node, "renesas,r9a07g043-plic")) {
> +		priv->of_data = RENESAS_R9A07G043_PLIC;
> +		plic_chip.name = "Renesas RZ/Five PLIC";

NAK. The irq_chip structure isn't the place for platform marketing.
This is way too long anyway (and same for the edge version), and you
even sent me a patch to make that structure const...

	M.

-- 
Without deviation from the norm, progress is not possible.