Re: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Lokesh Vutla <lokeshvutla@ti.com>]

On Monday 08 October 2018 06:30 PM, Marc Zyngier wrote:
> Hi Lokesh,
> 
> On 08/10/18 10:48, Lokesh Vutla wrote:
>> Hi Marc,
>>
>> On 10/6/2018 3:25 PM, Marc Zyngier wrote:
>>> Hi Lokesh,
>>>
>>> On Sat, 06 Oct 2018 08:28:12 +0100,
>>> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>
>>>> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
>>>> that does allows for multiplexing of input interrupts to host
>>>> interrupt controller. Interrupt Router inputs are either from a
>>>> peripheral or from an Interrupt Aggregator which is another
>>>> interrupt controller.
>>>>
>>>> Configuration of the interrupt router registers can only be done by
>>>> a system co-processor and the driver needs to send a message to this
>>>> co processor over TISCI protocol.
>>>
>>> I assume that this co-processor only deals with the routing itself,
>>> and doesn't need to be talked to during interrupt processing, right?
>>
>> Yes, that's right.
>>
>>>
>>>>
>>>> Add support for Interrupt Router driver over TISCI protocol.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>>    MAINTAINERS                       |   1 +
>>>>    drivers/irqchip/Kconfig           |  11 +
>>>>    drivers/irqchip/Makefile          |   1 +
>>>>    drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
>>>>    4 files changed, 338 insertions(+)
>>>>    create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index a23778b68d74..cf3c834f8cee 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>>>    F:	drivers/clk/keystone/sci-clk.c
>>>>    F:	drivers/reset/reset-ti-sci.c
>>>>    F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>> +F:	drivers/irqchip/irq-ti-sci-intr.c
>>>>    
>>>>    THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>>>>    M:	Hans Verkuil <hverkuil@xs4all.nl>
>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>> index 96451b581452..9a965fe22043 100644
>>>> --- a/drivers/irqchip/Kconfig
>>>> +++ b/drivers/irqchip/Kconfig
>>>> @@ -374,6 +374,17 @@ config QCOM_PDC
>>>>    	  Power Domain Controller driver to manage and configure wakeup
>>>>    	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>>>>    
>>>> +config TI_SCI_INTR_IRQCHIP
>>>> +	tristate "TISCI based Interrupt Router irqchip driver"
>>>> +	depends on TI_SCI_PROTOCOL && ARCH_K3
>>>> +	select IRQ_DOMAIN
>>>> +	select IRQ_DOMAIN_HIERARCHY
>>>> +	help
>>>> +	  This enables the irqchip driver support for K3 Interrupt router
>>>> +	  over TI System Control Interface available on some new TI's SoCs.
>>>> +	  If you wish to use interrupt router irq resources managed by the
>>>> +	  TI System Controller, say Y here. Otherwise, say N.
>>>
>>> I don't really see the point of making this user-selectable. If you're
>>> compiling support for a given platform, this platform configuration
>>> fragment should itself select the necessary dependencies for the
>>> system to work as expected. Here, you are leaving the choice to the
>>> user, with a 50% chance of getting a system that doesn't boot...
>>
>> There are 2 reasons why I made it tristate:
>> - Not all interrupts go through this irqchip(At least in the AM6 SoC
>> using this). Most of the legacy peripherals still are directly connected
>> to GIC
>> - TI_SCI_PROTOCOL is defined as tristate.
> 
> But as you said, these are "legacy" interrupts, and most of the
> interesting stuff is routed through the system controller. We also try
> not to have core interrupt controllers as modules. As for having the
> firmware interface as a module, I wonder what the use-case is.
> 
>> If you still feel I should not make it user-selectable, I can drop it.
> 
> I really wonder what the added value is for the user.

okay, will not make it use configurable in v2.

> 
>>
>>>
>>>> +
>>>>    endmenu
>>>>    
>>>>    config SIFIVE_PLIC
>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>> index b822199445ff..44bf65606d60 100644
>>>> --- a/drivers/irqchip/Makefile
>>>> +++ b/drivers/irqchip/Makefile
>>>> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>>>>    obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>>>>    obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>>>>    obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>>>> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>>>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>>>> new file mode 100644
>>>> index 000000000000..f04fe6da1b09
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>>>> @@ -0,0 +1,325 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Texas Instruments' K3 Interrupt Router irqchip driver
>>>> + *
>>>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>>>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/irqchip.h>
>>>> +#include <linux/of_platform.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/moduleparam.h>
>>>> +#include <linux/irqdomain.h>
>>>> +#include <linux/soc/ti/ti_sci_protocol.h>
>>>> +
>>>> +#define TI_SCI_DEV_ID_MASK	0xffff
>>>> +#define TI_SCI_DEV_ID_SHIFT	16
>>>> +#define TI_SCI_IRQ_ID_MASK	0xffff
>>>> +#define TI_SCI_IRQ_ID_SHIFT	0
>>>> +#define TI_SCI_IS_EVENT_IRQ	BIT(31)
>>>> +
>>>> +#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
>>>> +				 (TI_SCI_DEV_ID_MASK))
>>>> +#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))
>>>
>>> nit: s/(HWIRQ)/(hwirq)/g
>>
>> okay.
>>
>>>
>>>> +
>>>> +/**
>>>> + * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
>>>> + *				   Interrupt Router IRQ domain.
>>>> + * @sci:	Pointer to TISCI handle
>>>> + * @dst_irq:	TISCI resource pointer representing destination irq controller.
>>>> + * @dst_id:	TISCI device ID of the destination irq controller.
>>>> + */
>>>> +struct ti_sci_intr_irq_domain {
>>>> +	const struct ti_sci_handle *sci;
>>>> +	struct ti_sci_resource *dst_irq;
>>>> +	u16 dst_id;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
>>>> + * @src_id:		TISCI device ID of the IRQ source
>>>> + * @src_index:		IRQ source index within the device.
>>>> + * @dst_irq:		Destination host IRQ.
>>>> + */
>>>> +struct ti_sci_intr_irq_desc {
>>>> +	u16 src_id;
>>>> +	u16 src_index;
>>>> +	u16 dst_irq;
>>>> +};
>>>
>>> Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(
>>>
>>> Now, this structure seems completely useless, see below.
>>>
>>>> +
>>>> +static struct irq_chip ti_sci_intr_irq_chip = {
>>>> +	.name			= "INTR",
>>>> +	.irq_eoi		= irq_chip_eoi_parent,
>>>> +	.irq_mask		= irq_chip_mask_parent,
>>>> +	.irq_unmask		= irq_chip_unmask_parent,
>>>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>>> +	.irq_set_type		= irq_chip_set_type_parent,
>>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>>> +};
>>>> +
>>>> +/**
>>>> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
>>>> + *					IRQ firmware specific handler.
>>>> + * @domain:	Pointer to IRQ domain
>>>> + * @fwspec:	Pointer to IRQ specific firmware structure
>>>> + * @hwirq:	IRQ number identified by hardware
>>>> + * @type:	IRQ type
>>>> + *
>>>> + * Return 0 if all went ok else appropriate error.
>>>> + */
>>>> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
>>>> +					    struct irq_fwspec *fwspec,
>>>> +					    unsigned long *hwirq,
>>>> +					    unsigned int *type)
>>>> +{
>>>> +	if (is_of_node(fwspec->fwnode)) {
>>>> +		if (fwspec->param_count != 3)
>>>> +			return -EINVAL;
>>>> +
>>>> +		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
>>>> +			  TI_SCI_DEV_ID_SHIFT) |
>>>> +			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);
>>>
>>> Maybe it would make sense to have a macro that hides this:
>>
>> okay.
>>
>>>
>>>         	       *hwirq = FWSPEC_TO_HWIRQ(fwspec);
>>>
>>>> +		*type = fwspec->param[2];
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
>>>> +					   struct ti_sci_intr_irq_desc *desc)
>>>> +{
>>>> +	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
>>>> +						  desc->src_index,
>>>> +						  intr->dst_id, desc->dst_irq);
>>>
>>> This looks horrible. Why doesn't your firmware interface have a helper
>>> functions that hides this? Something like:
>>>
>>> 	ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);
>>>
>>> and you could even add some error checking.
>>
>> All existing TISCI users follow the same convention, so I did not bother adding
>> any such wrapper. Will update TISCI with these wrappers and see what firmware
>> maintainer says.
> 
> Frankly, exposing all kind of data structures to the world is a pretty
> poor form of abstraction, which is what the firmware is supposed to
> provide.
> 
> I'd strongly suggest that include/linux/soc/ti/ti_sci_protocol.h gets
> cleaned up, and that the whole ti_sci_ops disappears from the that file.
> Nobody outside of the firmware *implementation* needs to know about its,
> and it would be much better served by a set of helpers.
> 
> Finally, please make the TISCI interrupt management part of this series,
> so that I can review it as part of the code that uses it.

Sure, my next version will include TISCI interrupt management as well.

Thanks and regards,
Lokesh