[PATCHv3,2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts

From: Suman Anna <s-anna@ti.com>


The Programmable Real-Time Unit Subsystem (PRUSS) contains a local
interrupt controller (INTC) that can handle various system input events
and post interrupts back to the device-level initiators. The INTC can
support upto 64 input events with individual control configuration and
hardware prioritization. These events are mapped onto 10 output interrupt
lines through two levels of many-to-one mapping support. Different
interrupt lines are routed to the individual PRU cores or to the host
CPU, or to other devices on the SoC. Some of these events are sourced
from peripherals or other sub-modules within that PRUSS, while a few
others are sourced from SoC-level peripherals/devices.

The PRUSS INTC platform driver manages this PRUSS interrupt controller
and implements an irqchip driver to provide a Linux standard way for
the PRU client users to enable/disable/ack/re-trigger a PRUSS system
event. The system events to interrupt channels and output interrupts
relies on the mapping configuration provided either through the PRU
firmware blob or via the PRU application's device tree node. The
mappings will be programmed during the boot/shutdown of a PRU core.

The PRUSS INTC module is reference counted during the interrupt
setup phase through the irqchip's irq_request_resources() and
irq_release_resources() ops. This restricts the module from being
removed as long as there are active interrupt users.

The driver currently supports and can be built for OMAP architecture
based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based
66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs.
All of these SoCs support 64 system events, 10 interrupt channels and
10 output interrupt lines per PRUSS INTC with a few SoC integration
differences.

NOTE:
Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that
enables multiple external events to be routed to a specific number
of input interrupt events. Any non-default external interrupt event
directed towards PRUSS needs this crossbar to be setup properly.

Signed-off-by: Suman Anna <s-anna@ti.com>

Signed-off-by: Andrew F. Davis <afd@ti.com>

Signed-off-by: Roger Quadros <rogerq@ti.com>

Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>

Reviewed-by: Lee Jones <lee.jones@linaro.org>

---
v2->v3:
- use single irqchip description instead of separately allocating it for
  each pruss_intc
- get rid of unused mutex
- improve error handling
v1->v2:
- https://patchwork.kernel.org/patch/11069771/
---
 drivers/irqchip/Kconfig          |  10 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-pruss-intc.c | 307 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/irqchip/irq-pruss-intc.c

-- 
2.7.4

On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> From: Suman Anna <s-anna@ti.com>

> 

> The Programmable Real-Time Unit Subsystem (PRUSS) contains a local

> interrupt controller (INTC) that can handle various system input events

> and post interrupts back to the device-level initiators. The INTC can

> support upto 64 input events with individual control configuration and

> hardware prioritization. These events are mapped onto 10 output 

> interrupt

> lines through two levels of many-to-one mapping support. Different

> interrupt lines are routed to the individual PRU cores or to the host

> CPU, or to other devices on the SoC. Some of these events are sourced

> from peripherals or other sub-modules within that PRUSS, while a few

> others are sourced from SoC-level peripherals/devices.

> 

> The PRUSS INTC platform driver manages this PRUSS interrupt controller

> and implements an irqchip driver to provide a Linux standard way for

> the PRU client users to enable/disable/ack/re-trigger a PRUSS system

> event. The system events to interrupt channels and output interrupts

> relies on the mapping configuration provided either through the PRU

> firmware blob or via the PRU application's device tree node. The

> mappings will be programmed during the boot/shutdown of a PRU core.

> 

> The PRUSS INTC module is reference counted during the interrupt

> setup phase through the irqchip's irq_request_resources() and

> irq_release_resources() ops. This restricts the module from being

> removed as long as there are active interrupt users.

> 

> The driver currently supports and can be built for OMAP architecture

> based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based

> 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs.

> All of these SoCs support 64 system events, 10 interrupt channels and

> 10 output interrupt lines per PRUSS INTC with a few SoC integration

> differences.

> 

> NOTE:

> Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that

> enables multiple external events to be routed to a specific number

> of input interrupt events. Any non-default external interrupt event

> directed towards PRUSS needs this crossbar to be setup properly.

> 

> Signed-off-by: Suman Anna <s-anna@ti.com>

> Signed-off-by: Andrew F. Davis <afd@ti.com>

> Signed-off-by: Roger Quadros <rogerq@ti.com>

> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>

> Reviewed-by: Lee Jones <lee.jones@linaro.org>

> ---

> v2->v3:

> - use single irqchip description instead of separately allocating it 

> for

>   each pruss_intc

> - get rid of unused mutex

> - improve error handling

> v1->v2:

> - https://patchwork.kernel.org/patch/11069771/

> ---

>  drivers/irqchip/Kconfig          |  10 ++

>  drivers/irqchip/Makefile         |   1 +

>  drivers/irqchip/irq-pruss-intc.c | 307 

> +++++++++++++++++++++++++++++++++++++++

>  3 files changed, 318 insertions(+)

>  create mode 100644 drivers/irqchip/irq-pruss-intc.c

> 

> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig

> index 29fead2..733d7ec 100644

> --- a/drivers/irqchip/Kconfig

> +++ b/drivers/irqchip/Kconfig

> @@ -493,6 +493,16 @@ config TI_SCI_INTA_IRQCHIP

>  	  If you wish to use interrupt aggregator irq resources managed by 

> the

>  	  TI System Controller, say Y here. Otherwise, say N.

> 

> +config TI_PRUSS_INTC

> +	tristate "TI PRU-ICSS Interrupt Controller"

> +	depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX ||

> ARCH_KEYSTONE

> +	select IRQ_DOMAIN

> +	help

> +	   This enables support for the PRU-ICSS Local Interrupt Controller

> +	   present within a PRU-ICSS subsystem present on various TI SoCs.

> +	   The PRUSS INTC enables various interrupts to be routed to multiple

> +	   different processors within the SoC.

> +

>  config RISCV_INTC

>  	bool "RISC-V Local Interrupt Controller"

>  	depends on RISCV

> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile

> index 133f9c4..990a106 100644

> --- a/drivers/irqchip/Makefile

> +++ b/drivers/irqchip/Makefile

> @@ -106,6 +106,7 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o

>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o

>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o

>  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o

> +obj-$(CONFIG_TI_PRUSS_INTC)		+= irq-pruss-intc.o

>  obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o

>  obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o

>  obj-$(CONFIG_LOONGSON_HTVEC)		+= irq-loongson-htvec.o

> diff --git a/drivers/irqchip/irq-pruss-intc.c 

> b/drivers/irqchip/irq-pruss-intc.c

> new file mode 100644

> index 0000000..fb3dda3

> --- /dev/null

> +++ b/drivers/irqchip/irq-pruss-intc.c

> @@ -0,0 +1,307 @@

> +// SPDX-License-Identifier: GPL-2.0-only

> +/*

> + * PRU-ICSS INTC IRQChip driver for various TI SoCs

> + *

> + * Copyright (C) 2016-2020 Texas Instruments Incorporated - 

> http://www.ti.com/

> + *	Andrew F. Davis <afd@ti.com>

> + *	Suman Anna <s-anna@ti.com>

> + */

> +

> +#include <linux/irq.h>

> +#include <linux/irqchip/chained_irq.h>

> +#include <linux/irqdomain.h>

> +#include <linux/module.h>

> +#include <linux/of_device.h>

> +#include <linux/platform_device.h>

> +

> +/*

> + * Number of host interrupts reaching the main MPU sub-system. Note 

> that this

> + * is not the same as the total number of host interrupts supported

> by the PRUSS

> + * INTC instance

> + */

> +#define MAX_NUM_HOST_IRQS	8

> +

> +/* minimum starting host interrupt number for MPU */

> +#define MIN_PRU_HOST_INT	2

> +

> +/* maximum number of system events */

> +#define MAX_PRU_SYS_EVENTS	64

> +

> +/* PRU_ICSS_INTC registers */

> +#define PRU_INTC_REVID		0x0000

> +#define PRU_INTC_CR		0x0004

> +#define PRU_INTC_GER		0x0010

> +#define PRU_INTC_GNLR		0x001c

> +#define PRU_INTC_SISR		0x0020

> +#define PRU_INTC_SICR		0x0024

> +#define PRU_INTC_EISR		0x0028

> +#define PRU_INTC_EICR		0x002c

> +#define PRU_INTC_HIEISR		0x0034

> +#define PRU_INTC_HIDISR		0x0038

> +#define PRU_INTC_GPIR		0x0080

> +#define PRU_INTC_SRSR0		0x0200

> +#define PRU_INTC_SRSR1		0x0204

> +#define PRU_INTC_SECR0		0x0280

> +#define PRU_INTC_SECR1		0x0284

> +#define PRU_INTC_ESR0		0x0300

> +#define PRU_INTC_ESR1		0x0304

> +#define PRU_INTC_ECR0		0x0380

> +#define PRU_INTC_ECR1		0x0384

> +#define PRU_INTC_CMR(x)		(0x0400 + (x) * 4)

> +#define PRU_INTC_HMR(x)		(0x0800 + (x) * 4)

> +#define PRU_INTC_HIPIR(x)	(0x0900 + (x) * 4)

> +#define PRU_INTC_SIPR0		0x0d00

> +#define PRU_INTC_SIPR1		0x0d04

> +#define PRU_INTC_SITR0		0x0d80

> +#define PRU_INTC_SITR1		0x0d84

> +#define PRU_INTC_HINLR(x)	(0x1100 + (x) * 4)

> +#define PRU_INTC_HIER		0x1500

> +

> +/* HIPIR register bit-fields */

> +#define INTC_HIPIR_NONE_HINT	0x80000000

> +

> +/**

> + * struct pruss_intc - PRUSS interrupt controller structure

> + * @irqs: kernel irq numbers corresponding to PRUSS host interrupts

> + * @base: base virtual address of INTC register space

> + * @domain: irq domain for this interrupt controller

> + */

> +struct pruss_intc {

> +	unsigned int irqs[MAX_NUM_HOST_IRQS];

> +	void __iomem *base;

> +	struct irq_domain *domain;

> +};

> +

> +static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,

> unsigned int reg)

> +{

> +	return readl_relaxed(intc->base + reg);

> +}

> +

> +static inline void pruss_intc_write_reg(struct pruss_intc *intc,

> +					unsigned int reg, u32 val)

> +{

> +	writel_relaxed(val, intc->base + reg);

> +}

> +

> +static void pruss_intc_init(struct pruss_intc *intc)

> +{

> +	int i;

> +

> +	/* configure polarity to active high for all system interrupts */

> +	pruss_intc_write_reg(intc, PRU_INTC_SIPR0, 0xffffffff);

> +	pruss_intc_write_reg(intc, PRU_INTC_SIPR1, 0xffffffff);

> +

> +	/* configure type to pulse interrupt for all system interrupts */

> +	pruss_intc_write_reg(intc, PRU_INTC_SITR0, 0);

> +	pruss_intc_write_reg(intc, PRU_INTC_SITR1, 0);


So the default is to configure everything as edge...

> +

> +	/* clear all 16 interrupt channel map registers */

> +	for (i = 0; i < 16; i++)

> +		pruss_intc_write_reg(intc, PRU_INTC_CMR(i), 0);

> +

> +	/* clear all 3 host interrupt map registers */

> +	for (i = 0; i < 3; i++)

> +		pruss_intc_write_reg(intc, PRU_INTC_HMR(i), 0);

> +}

> +

> +static void pruss_intc_irq_ack(struct irq_data *data)

> +{

> +	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> +	unsigned int hwirq = data->hwirq;

> +

> +	pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);

> +}

> +

> +static void pruss_intc_irq_mask(struct irq_data *data)

> +{

> +	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> +	unsigned int hwirq = data->hwirq;

> +

> +	pruss_intc_write_reg(intc, PRU_INTC_EICR, hwirq);

> +}

> +

> +static void pruss_intc_irq_unmask(struct irq_data *data)

> +{

> +	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> +	unsigned int hwirq = data->hwirq;

> +

> +	pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);

> +}

> +

> +static int pruss_intc_irq_reqres(struct irq_data *data)

> +{

> +	if (!try_module_get(THIS_MODULE))

> +		return -ENODEV;

> +

> +	return 0;

> +}

> +

> +static void pruss_intc_irq_relres(struct irq_data *data)

> +{

> +	module_put(THIS_MODULE);

> +}

> +

> +static struct irq_chip pruss_irqchip = {

> +	.name = "pruss-intc",

> +	.irq_ack = pruss_intc_irq_ack,

> +	.irq_mask = pruss_intc_irq_mask,

> +	.irq_unmask = pruss_intc_irq_unmask,

> +	.irq_request_resources = pruss_intc_irq_reqres,

> +	.irq_release_resources = pruss_intc_irq_relres,

> +};

> +

> +static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned 

> int virq,

> +				     irq_hw_number_t hw)

> +{

> +	struct pruss_intc *intc = d->host_data;

> +

> +	irq_set_chip_data(virq, intc);

> +	irq_set_chip_and_handler(virq, &pruss_irqchip, handle_level_irq);


... and despite this edge-triggered default, you handle things as level.
This doesn't seem quite right.

> +

> +	return 0;

> +}

> +

> +static void pruss_intc_irq_domain_unmap(struct irq_domain *d,

> unsigned int virq)

> +{

> +	irq_set_chip_and_handler(virq, NULL, NULL);

> +	irq_set_chip_data(virq, NULL);

> +}

> +

> +static const struct irq_domain_ops pruss_intc_irq_domain_ops = {

> +	.xlate	= irq_domain_xlate_onecell,

> +	.map	= pruss_intc_irq_domain_map,

> +	.unmap	= pruss_intc_irq_domain_unmap,

> +};

> +

> +static void pruss_intc_irq_handler(struct irq_desc *desc)

> +{

> +	unsigned int irq = irq_desc_get_irq(desc);

> +	struct irq_chip *chip = irq_desc_get_chip(desc);

> +	struct pruss_intc *intc = irq_get_handler_data(irq);

> +	u32 hipir;

> +	unsigned int virq;

> +	int i, hwirq;

> +

> +	chained_irq_enter(chip, desc);

> +

> +	/* find our host irq number */

> +	for (i = 0; i < MAX_NUM_HOST_IRQS; i++)

> +		if (intc->irqs[i] == irq)

> +			break;


This loop is pretty ugly. The way to do it would normally to
associate the right data structure to the chained interrupt,
and only that one, directly associating the input signal
with the correct mux. Using the Linux irq as a discriminant is
at best clumsy.

But it feels to me that the base data structure is not
exactly the right one here, see below.

> +	if (i == MAX_NUM_HOST_IRQS)

> +		goto err;

> +

> +	i += MIN_PRU_HOST_INT;

> +

> +	/* get highest priority pending PRUSS system event */

> +	hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));

> +	while (!(hipir & INTC_HIPIR_NONE_HINT)) {


Please write this as a do { } while() loop, with a single instance
of the HW register read inside the loop (instead of one outside
and one inside.

> +		hwirq = hipir & GENMASK(9, 0);

> +		virq = irq_linear_revmap(intc->domain, hwirq);


And this is where I worry. You seems to have a single irqdomain
for all the muxes. Are you guaranteed that you will have no
overlap between muxes? And please use irq_find_mapping(), as
I have top-secret plans to kill irq_linear_revmap().

> +

> +		/*

> +		 * NOTE: manually ACK any system events that do not have a

> +		 * handler mapped yet

> +		 */

> +		if (WARN_ON(!virq))

> +			pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);


How can this happen? If you really need it, you probable want to
warn once only.

> +		else

> +			generic_handle_irq(virq);

> +

> +		/* get next system event */

> +		hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));

> +	}

> +err:

> +	chained_irq_exit(chip, desc);

> +}

> +

> +static int pruss_intc_probe(struct platform_device *pdev)

> +{

> +	static const char * const irq_names[MAX_NUM_HOST_IRQS] = {

> +		"host_intr0", "host_intr1", "host_intr2", "host_intr3",

> +		"host_intr4", "host_intr5", "host_intr6", "host_intr7", };

> +	struct device *dev = &pdev->dev;

> +	struct pruss_intc *intc;

> +	int i, irq;

> +

> +	intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);

> +	if (!intc)

> +		return -ENOMEM;

> +	platform_set_drvdata(pdev, intc);

> +

> +	intc->base = devm_platform_ioremap_resource(pdev, 0);

> +	if (IS_ERR(intc->base)) {

> +		dev_err(dev, "failed to parse and map intc memory resource\n");

> +		return PTR_ERR(intc->base);

> +	}

> +

> +	pruss_intc_init(intc);

> +

> +	/* always 64 events */

> +	intc->domain = irq_domain_add_linear(dev->of_node, 

> MAX_PRU_SYS_EVENTS,

> +					     &pruss_intc_irq_domain_ops, intc);

> +	if (!intc->domain)

> +		return -ENOMEM;

> +

> +	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {

> +		irq = platform_get_irq_byname(pdev, irq_names[i]);

> +		if (irq <= 0) {

> +			dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",

> +				irq_names[i], irq);

> +			goto fail_irq;

> +		}

> +

> +		intc->irqs[i] = irq;


Are the output IRQs guaranteed to be contiguous? If so, that'd be
a much nicer way to work out which mux has fired (just store the
base, and use it as an offset on handling the chained interrupt).

> +		irq_set_handler_data(irq, intc);

> +		irq_set_chained_handler(irq, pruss_intc_irq_handler);

> +	}

> +

> +	return 0;

> +

> +fail_irq:

> +	while (--i >= 0)

> +		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);

> +

> +	irq_domain_remove(intc->domain);

> +

> +	return irq;

> +}

> +

> +static int pruss_intc_remove(struct platform_device *pdev)

> +{

> +	struct pruss_intc *intc = platform_get_drvdata(pdev);

> +	unsigned int hwirq;

> +	int i;

> +

> +	for (i = 0; i < MAX_NUM_HOST_IRQS; i++)

> +		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);

> +

> +	for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)

> +		irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));

> +

> +	irq_domain_remove(intc->domain);

> +

> +	return 0;

> +}

> +

> +static const struct of_device_id pruss_intc_of_match[] = {

> +	{ .compatible = "ti,pruss-intc", },

> +	{ /* sentinel */ },

> +};

> +MODULE_DEVICE_TABLE(of, pruss_intc_of_match);

> +

> +static struct platform_driver pruss_intc_driver = {

> +	.driver = {

> +		.name = "pruss-intc",

> +		.of_match_table = pruss_intc_of_match,

> +		.suppress_bind_attrs = true,

> +	},

> +	.probe  = pruss_intc_probe,

> +	.remove = pruss_intc_remove,

> +};

> +module_platform_driver(pruss_intc_driver);

> +

> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");

> +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");

> +MODULE_DESCRIPTION("TI PRU-ICSS INTC Driver");

> +MODULE_LICENSE("GPL v2");


Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

On Thu, 2 Jul 2020 at 19:24, Marc Zyngier <maz@kernel.org> wrote:
>

> On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:

> > From: Suman Anna <s-anna@ti.com>

> >

> > The Programmable Real-Time Unit Subsystem (PRUSS) contains a local

> > interrupt controller (INTC) that can handle various system input events

> > and post interrupts back to the device-level initiators. The INTC can

> > support upto 64 input events with individual control configuration and

> > hardware prioritization. These events are mapped onto 10 output

> > interrupt

> > lines through two levels of many-to-one mapping support. Different

> > interrupt lines are routed to the individual PRU cores or to the host

> > CPU, or to other devices on the SoC. Some of these events are sourced

> > from peripherals or other sub-modules within that PRUSS, while a few

> > others are sourced from SoC-level peripherals/devices.

> >

> > The PRUSS INTC platform driver manages this PRUSS interrupt controller

> > and implements an irqchip driver to provide a Linux standard way for

> > the PRU client users to enable/disable/ack/re-trigger a PRUSS system

> > event. The system events to interrupt channels and output interrupts

> > relies on the mapping configuration provided either through the PRU

> > firmware blob or via the PRU application's device tree node. The

> > mappings will be programmed during the boot/shutdown of a PRU core.

> >

> > The PRUSS INTC module is reference counted during the interrupt

> > setup phase through the irqchip's irq_request_resources() and

> > irq_release_resources() ops. This restricts the module from being

> > removed as long as there are active interrupt users.

> >

> > The driver currently supports and can be built for OMAP architecture

> > based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based

> > 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs.

> > All of these SoCs support 64 system events, 10 interrupt channels and

> > 10 output interrupt lines per PRUSS INTC with a few SoC integration

> > differences.

> >

> > NOTE:

> > Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that

> > enables multiple external events to be routed to a specific number

> > of input interrupt events. Any non-default external interrupt event

> > directed towards PRUSS needs this crossbar to be setup properly.

> >

> > Signed-off-by: Suman Anna <s-anna@ti.com>

> > Signed-off-by: Andrew F. Davis <afd@ti.com>

> > Signed-off-by: Roger Quadros <rogerq@ti.com>

> > Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>

> > Reviewed-by: Lee Jones <lee.jones@linaro.org>

> > ---

> > v2->v3:

> > - use single irqchip description instead of separately allocating it

> > for

> >   each pruss_intc

> > - get rid of unused mutex

> > - improve error handling

> > v1->v2:

> > - https://patchwork.kernel.org/patch/11069771/

<snip>
> > +static void pruss_intc_init(struct pruss_intc *intc)

> > +{

> > +     int i;

> > +

> > +     /* configure polarity to active high for all system interrupts */

> > +     pruss_intc_write_reg(intc, PRU_INTC_SIPR0, 0xffffffff);

> > +     pruss_intc_write_reg(intc, PRU_INTC_SIPR1, 0xffffffff);

> > +

> > +     /* configure type to pulse interrupt for all system interrupts */

> > +     pruss_intc_write_reg(intc, PRU_INTC_SITR0, 0);

> > +     pruss_intc_write_reg(intc, PRU_INTC_SITR1, 0);

>

> So the default is to configure everything as edge...


Sorry, the description is wrong - '0' indicates level and '1' edge. So
the default configuration is level - I will fix the comment.

>

> > +

> > +     /* clear all 16 interrupt channel map registers */

> > +     for (i = 0; i < 16; i++)

> > +             pruss_intc_write_reg(intc, PRU_INTC_CMR(i), 0);

> > +

> > +     /* clear all 3 host interrupt map registers */

> > +     for (i = 0; i < 3; i++)

> > +             pruss_intc_write_reg(intc, PRU_INTC_HMR(i), 0);

> > +}

> > +

> > +static void pruss_intc_irq_ack(struct irq_data *data)

> > +{

> > +     struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> > +     unsigned int hwirq = data->hwirq;

> > +

> > +     pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);

> > +}

> > +

> > +static void pruss_intc_irq_mask(struct irq_data *data)

> > +{

> > +     struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> > +     unsigned int hwirq = data->hwirq;

> > +

> > +     pruss_intc_write_reg(intc, PRU_INTC_EICR, hwirq);

> > +}

> > +

> > +static void pruss_intc_irq_unmask(struct irq_data *data)

> > +{

> > +     struct pruss_intc *intc = irq_data_get_irq_chip_data(data);

> > +     unsigned int hwirq = data->hwirq;

> > +

> > +     pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);

> > +}

> > +

> > +static int pruss_intc_irq_reqres(struct irq_data *data)

> > +{

> > +     if (!try_module_get(THIS_MODULE))

> > +             return -ENODEV;

> > +

> > +     return 0;

> > +}

> > +

> > +static void pruss_intc_irq_relres(struct irq_data *data)

> > +{

> > +     module_put(THIS_MODULE);

> > +}

> > +

> > +static struct irq_chip pruss_irqchip = {

> > +     .name = "pruss-intc",

> > +     .irq_ack = pruss_intc_irq_ack,

> > +     .irq_mask = pruss_intc_irq_mask,

> > +     .irq_unmask = pruss_intc_irq_unmask,

> > +     .irq_request_resources = pruss_intc_irq_reqres,

> > +     .irq_release_resources = pruss_intc_irq_relres,

> > +};

> > +

> > +static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned

> > int virq,

> > +                                  irq_hw_number_t hw)

> > +{

> > +     struct pruss_intc *intc = d->host_data;

> > +

> > +     irq_set_chip_data(virq, intc);

> > +     irq_set_chip_and_handler(virq, &pruss_irqchip, handle_level_irq);

>

> ... and despite this edge-triggered default, you handle things as level.

> This doesn't seem quite right.


As above it is level. I will fix the comment

>

> > +

> > +     return 0;

> > +}

> > +

> > +static void pruss_intc_irq_domain_unmap(struct irq_domain *d,

> > unsigned int virq)

> > +{

> > +     irq_set_chip_and_handler(virq, NULL, NULL);

> > +     irq_set_chip_data(virq, NULL);

> > +}

> > +

> > +static const struct irq_domain_ops pruss_intc_irq_domain_ops = {

> > +     .xlate  = irq_domain_xlate_onecell,

> > +     .map    = pruss_intc_irq_domain_map,

> > +     .unmap  = pruss_intc_irq_domain_unmap,

> > +};

> > +

> > +static void pruss_intc_irq_handler(struct irq_desc *desc)

> > +{

> > +     unsigned int irq = irq_desc_get_irq(desc);

> > +     struct irq_chip *chip = irq_desc_get_chip(desc);

> > +     struct pruss_intc *intc = irq_get_handler_data(irq);

> > +     u32 hipir;

> > +     unsigned int virq;

> > +     int i, hwirq;

> > +

> > +     chained_irq_enter(chip, desc);

> > +

> > +     /* find our host irq number */

> > +     for (i = 0; i < MAX_NUM_HOST_IRQS; i++)

> > +             if (intc->irqs[i] == irq)

> > +                     break;

>

> This loop is pretty ugly. The way to do it would normally to

> associate the right data structure to the chained interrupt,

> and only that one, directly associating the input signal

> with the correct mux. Using the Linux irq as a discriminant is

> at best clumsy.

>

> But it feels to me that the base data structure is not

> exactly the right one here, see below.

>


Ok, you are right. I will introduce a new structure for host_irq data
which will be associated with chained interrupt and get rid of this
loop.

> > +     if (i == MAX_NUM_HOST_IRQS)

> > +             goto err;

> > +

> > +     i += MIN_PRU_HOST_INT;

> > +

> > +     /* get highest priority pending PRUSS system event */

> > +     hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));

> > +     while (!(hipir & INTC_HIPIR_NONE_HINT)) {

>

> Please write this as a do { } while() loop, with a single instance

> of the HW register read inside the loop (instead of one outside

> and one inside.


Ok, I will get rid of the outside HW register read, but I think it is
better to use bellow instead of do {} while () loop:
while (1) {
  /* get highest priority pending PRUSS system event */
  hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(host_irq));
  if (hipir & INTC_HIPIR_NONE_HINT)
    break;
...

Hope it works for you.

>

> > +             hwirq = hipir & GENMASK(9, 0);

> > +             virq = irq_linear_revmap(intc->domain, hwirq);

>

> And this is where I worry. You seems to have a single irqdomain

> for all the muxes. Are you guaranteed that you will have no

> overlap between muxes? And please use irq_find_mapping(), as

> I have top-secret plans to kill irq_linear_revmap().


Regarding irq_find_mapping - sure.

Regarding irqdomains:
It is a single irqdomain since the hwirq (system event) can be mapped
to different irq_host (muxes). Patch #6
https://lkml.org/lkml/2020/7/2/616 implements and describes how input
events can be mapped to some output host interrupts through 2 levels
of many-to-one mapping i.e. events to channel mapping and channels to
host interrupts. Mentioned implementation ensures that specific system
event (hwirq) can be mapped through PRUSS specific channel into a
single host interrupt.

>

> > +

> > +             /*

> > +              * NOTE: manually ACK any system events that do not have a

> > +              * handler mapped yet

> > +              */

> > +             if (WARN_ON(!virq))

> > +                     pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);

>

> How can this happen? If you really need it, you probable want to

> warn once only.


Ideally it shouldn't happen but I prefer to keep it to catch any
misuse. It is because the PRUSS INTC unit can be also accessed by PRU
cores which use the same registers to ack the internal events. The
current design is limited to only acking and triggering the interrupts
from PRU firmwares while the entire mapping is done by Linux (patch #6
https://lkml.org/lkml/2020/7/2/612).

I will convert it to WARN_ON_ONCE.

>

> > +             else

> > +                     generic_handle_irq(virq);

> > +

> > +             /* get next system event */

> > +             hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));

> > +     }

> > +err:

> > +     chained_irq_exit(chip, desc);

> > +}

> > +

> > +static int pruss_intc_probe(struct platform_device *pdev)

> > +{

> > +     static const char * const irq_names[MAX_NUM_HOST_IRQS] = {

> > +             "host_intr0", "host_intr1", "host_intr2", "host_intr3",

> > +             "host_intr4", "host_intr5", "host_intr6", "host_intr7", };

> > +     struct device *dev = &pdev->dev;

> > +     struct pruss_intc *intc;

> > +     int i, irq;

> > +

> > +     intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);

> > +     if (!intc)

> > +             return -ENOMEM;

> > +     platform_set_drvdata(pdev, intc);

> > +

> > +     intc->base = devm_platform_ioremap_resource(pdev, 0);

> > +     if (IS_ERR(intc->base)) {

> > +             dev_err(dev, "failed to parse and map intc memory resource\n");

> > +             return PTR_ERR(intc->base);

> > +     }

> > +

> > +     pruss_intc_init(intc);

> > +

> > +     /* always 64 events */

> > +     intc->domain = irq_domain_add_linear(dev->of_node,

> > MAX_PRU_SYS_EVENTS,

> > +                                          &pruss_intc_irq_domain_ops, intc);

> > +     if (!intc->domain)

> > +             return -ENOMEM;

> > +

> > +     for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {

> > +             irq = platform_get_irq_byname(pdev, irq_names[i]);

> > +             if (irq <= 0) {

> > +                     dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",

> > +                             irq_names[i], irq);

> > +                     goto fail_irq;

> > +             }

> > +

> > +             intc->irqs[i] = irq;

>

> Are the output IRQs guaranteed to be contiguous? If so, that'd be

> a much nicer way to work out which mux has fired (just store the

> base, and use it as an offset on handling the chained interrupt).


How about doing something like below in order to get rid of the for()
loop from the handler:

struct pruss_host_irq_data *host_data[]
...

host_data[i]->intc = intc;
host_data[i]->host_irq = i;

irq_set_handler_data(irq, host_data[i]);
irq_set_chained_handler(irq, pruss_intc_irq_handler);

>

> > +             irq_set_handler_data(irq, intc);

> > +             irq_set_chained_handler(irq, pruss_intc_irq_handler);

> > +     }

> > +

> > +     return 0;

> > +

> > +fail_irq:

> > +     while (--i >= 0)

> > +             irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);

> > +

> > +     irq_domain_remove(intc->domain);

> > +

> > +     return irq;

> > +}

> > +

> > +static int pruss_intc_remove(struct platform_device *pdev)

> > +{

> > +     struct pruss_intc *intc = platform_get_drvdata(pdev);

> > +     unsigned int hwirq;

> > +     int i;

> > +

> > +     for (i = 0; i < MAX_NUM_HOST_IRQS; i++)

> > +             irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);

> > +

> > +     for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)

> > +             irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));

> > +

> > +     irq_domain_remove(intc->domain);

> > +

> > +     return 0;

> > +}

> > +

> > +static const struct of_device_id pruss_intc_of_match[] = {

> > +     { .compatible = "ti,pruss-intc", },

> > +     { /* sentinel */ },

> > +};

> > +MODULE_DEVICE_TABLE(of, pruss_intc_of_match);

> > +

> > +static struct platform_driver pruss_intc_driver = {

> > +     .driver = {

> > +             .name = "pruss-intc",

> > +             .of_match_table = pruss_intc_of_match,

> > +             .suppress_bind_attrs = true,

> > +     },

> > +     .probe  = pruss_intc_probe,

> > +     .remove = pruss_intc_remove,

> > +};

> > +module_platform_driver(pruss_intc_driver);

> > +

> > +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");

> > +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");

> > +MODULE_DESCRIPTION("TI PRU-ICSS INTC Driver");

> > +MODULE_LICENSE("GPL v2");

>

> Thanks,

>

>          M.


Thank you for your feedback and suggestions,
Grzegorz

On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:
> On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

>> 

>> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:


[...]

>> It still begs the question: if the HW can support both edge and level

>> triggered interrupts, why isn't the driver supporting this diversity?

>> I appreciate that your HW may only have level interrupts so far, but

>> what guarantees that this will forever be true? It would imply a 

>> change

>> in the DT binding, which isn't desirable.

> 

> Ok, I've got your point. I will try to come up with something later

> on. Probably extending interrupt-cells by one and passing interrupt

> type will be enough for now. Extending this driver to actually support

> it can be handled later if needed. Hope it works for you.


Writing a set_type callback to deal with this should be pretty easy.
Don't delay doing the right thing.

[...]

>> >> > +             hwirq = hipir & GENMASK(9, 0);

>> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

>> >>

>> >> And this is where I worry. You seems to have a single irqdomain

>> >> for all the muxes. Are you guaranteed that you will have no

>> >> overlap between muxes? And please use irq_find_mapping(), as

>> >> I have top-secret plans to kill irq_linear_revmap().

>> >

>> > Regarding irq_find_mapping - sure.

>> >

>> > Regarding irqdomains:

>> > It is a single irqdomain since the hwirq (system event) can be mapped

>> > to different irq_host (muxes). Patch #6

>> > https://lkml.org/lkml/2020/7/2/616 implements and describes how input

>> > events can be mapped to some output host interrupts through 2 levels

>> > of many-to-one mapping i.e. events to channel mapping and channels to

>> > host interrupts. Mentioned implementation ensures that specific system

>> > event (hwirq) can be mapped through PRUSS specific channel into a

>> > single host interrupt.

>> 

>> Patch #6 is a nightmare of its own, and I haven't fully groked it yet.

>> Also, this driver seems to totally ignore the 2-level routing. Where

>> is it set up? map/unmap in this driver do exactly *nothing*, so

>> something somewhere must set it up.

> 

> The map/unmap is updated in patch #6 and it deals with those 2-level

> routing setup. Map is responsible for programming the Channel Map

> Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

> provided configuration from the one parsed in the xlate function.

> Unmap undo whatever was done on the map. More details can be found in

> patch #6.

> 

> Maybe it would be better to squash patch #6 with this one so it would

> be less confusing. What is your advice?


So am I right in understanding that without patch #6, this driver does
exactly nothing? If so, it has been a waste of review time.

Please split patch #6 so that this driver does something useful
for Linux, without any of the PRU interrupt routing stuff. I want
to see a Linux-only driver that works and doesn't rely on any other
exotic feature.

         M.
-- 
Jazz is not dead. It just smells funny...

On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:
>

> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

> >>

> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

>

> [...]

>

> >> It still begs the question: if the HW can support both edge and level

> >> triggered interrupts, why isn't the driver supporting this diversity?

> >> I appreciate that your HW may only have level interrupts so far, but

> >> what guarantees that this will forever be true? It would imply a

> >> change

> >> in the DT binding, which isn't desirable.

> >

> > Ok, I've got your point. I will try to come up with something later

> > on. Probably extending interrupt-cells by one and passing interrupt

> > type will be enough for now. Extending this driver to actually support

> > it can be handled later if needed. Hope it works for you.

>

> Writing a set_type callback to deal with this should be pretty easy.

> Don't delay doing the right thing.


Ok.

>

> [...]

>

> >> >> > +             hwirq = hipir & GENMASK(9, 0);

> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

> >> >>

> >> >> And this is where I worry. You seems to have a single irqdomain

> >> >> for all the muxes. Are you guaranteed that you will have no

> >> >> overlap between muxes? And please use irq_find_mapping(), as

> >> >> I have top-secret plans to kill irq_linear_revmap().

> >> >

> >> > Regarding irq_find_mapping - sure.

> >> >

> >> > Regarding irqdomains:

> >> > It is a single irqdomain since the hwirq (system event) can be mapped

> >> > to different irq_host (muxes). Patch #6

> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how input

> >> > events can be mapped to some output host interrupts through 2 levels

> >> > of many-to-one mapping i.e. events to channel mapping and channels to

> >> > host interrupts. Mentioned implementation ensures that specific system

> >> > event (hwirq) can be mapped through PRUSS specific channel into a

> >> > single host interrupt.

> >>

> >> Patch #6 is a nightmare of its own, and I haven't fully groked it yet.

> >> Also, this driver seems to totally ignore the 2-level routing. Where

> >> is it set up? map/unmap in this driver do exactly *nothing*, so

> >> something somewhere must set it up.

> >

> > The map/unmap is updated in patch #6 and it deals with those 2-level

> > routing setup. Map is responsible for programming the Channel Map

> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

> > provided configuration from the one parsed in the xlate function.

> > Unmap undo whatever was done on the map. More details can be found in

> > patch #6.

> >

> > Maybe it would be better to squash patch #6 with this one so it would

> > be less confusing. What is your advice?

>

> So am I right in understanding that without patch #6, this driver does

> exactly nothing? If so, it has been a waste of review time.

>

> Please split patch #6 so that this driver does something useful

> for Linux, without any of the PRU interrupt routing stuff. I want

> to see a Linux-only driver that works and doesn't rely on any other

> exotic feature.

>


Patch #6 provides PRU specific 2-level routing setup. This step is
required and it is part of the entire patch-set. Theoretically routing
setup could be done by other platform driver (not irq one) or e.g. by
PRU firmware. In such case this driver would be functional without
patch #6 but I do not think it would be proper. All this routing setup
is done via PRUSS INTC unit and uses PRUSS INTC register set,
therefore delegating it to another driver doesn't seem to be the best
option. Furthermore delegating this step to PRU firmware is also
problematic. First of all the PRU core tiny Instruction RAM space
makes it difficult to fit it together with the code that is required
for running a PRU specific application. Another issue that I see is
splitting management of the PRUSS INTC unit to Linux (main CPU) and
PRU firmware (PRU core).

I am also not sure if splitting patch #6 makes sense. Mentioned patch
allows to perform the entire 2-level routing setup. There is no
distinction between routing setup for main CPU and PRU core, both use
the same logic.The only difference between setting up the routing for
main CPU (Linux) and PRU core is choosing different, so called, "host
interrupt" in final level mapping.

Discussion about previous approach of handling this 2-level routing
setup can be found in v2 of this patch-set
(https://patchwork.kernel.org/patch/11069751/) - mentioned approach
wasn't good and was dropped but problem description made by Suman may
be useful.

I am open to any suggestion if there is a better way of handling
2-level routing. I will also appreciate if you could elaborate about
issues that you see with patch #6.

Best regards,
Grzegorz

On 2020-07-08 08:04, Grzegorz Jaszczyk wrote:
> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:

>> 

>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

>> >>

>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

>> 

>> [...]

>> 

>> >> It still begs the question: if the HW can support both edge and level

>> >> triggered interrupts, why isn't the driver supporting this diversity?

>> >> I appreciate that your HW may only have level interrupts so far, but

>> >> what guarantees that this will forever be true? It would imply a

>> >> change

>> >> in the DT binding, which isn't desirable.

>> >

>> > Ok, I've got your point. I will try to come up with something later

>> > on. Probably extending interrupt-cells by one and passing interrupt

>> > type will be enough for now. Extending this driver to actually support

>> > it can be handled later if needed. Hope it works for you.

>> 

>> Writing a set_type callback to deal with this should be pretty easy.

>> Don't delay doing the right thing.

> 

> Ok.

> 

>> 

>> [...]

>> 

>> >> >> > +             hwirq = hipir & GENMASK(9, 0);

>> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

>> >> >>

>> >> >> And this is where I worry. You seems to have a single irqdomain

>> >> >> for all the muxes. Are you guaranteed that you will have no

>> >> >> overlap between muxes? And please use irq_find_mapping(), as

>> >> >> I have top-secret plans to kill irq_linear_revmap().

>> >> >

>> >> > Regarding irq_find_mapping - sure.

>> >> >

>> >> > Regarding irqdomains:

>> >> > It is a single irqdomain since the hwirq (system event) can be mapped

>> >> > to different irq_host (muxes). Patch #6

>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how input

>> >> > events can be mapped to some output host interrupts through 2 levels

>> >> > of many-to-one mapping i.e. events to channel mapping and channels to

>> >> > host interrupts. Mentioned implementation ensures that specific system

>> >> > event (hwirq) can be mapped through PRUSS specific channel into a

>> >> > single host interrupt.

>> >>

>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it yet.

>> >> Also, this driver seems to totally ignore the 2-level routing. Where

>> >> is it set up? map/unmap in this driver do exactly *nothing*, so

>> >> something somewhere must set it up.

>> >

>> > The map/unmap is updated in patch #6 and it deals with those 2-level

>> > routing setup. Map is responsible for programming the Channel Map

>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

>> > provided configuration from the one parsed in the xlate function.

>> > Unmap undo whatever was done on the map. More details can be found in

>> > patch #6.

>> >

>> > Maybe it would be better to squash patch #6 with this one so it would

>> > be less confusing. What is your advice?

>> 

>> So am I right in understanding that without patch #6, this driver does

>> exactly nothing? If so, it has been a waste of review time.

>> 

>> Please split patch #6 so that this driver does something useful

>> for Linux, without any of the PRU interrupt routing stuff. I want

>> to see a Linux-only driver that works and doesn't rely on any other

>> exotic feature.

>> 

> 

> Patch #6 provides PRU specific 2-level routing setup. This step is

> required and it is part of the entire patch-set. Theoretically routing

> setup could be done by other platform driver (not irq one) or e.g. by

> PRU firmware. In such case this driver would be functional without

> patch #6 but I do not think it would be proper.


Then this whole driver is non-functional until the last patch that
comes with the PRU-specific "value-add".

[...]

> I am open to any suggestion if there is a better way of handling

> 2-level routing. I will also appreciate if you could elaborate about

> issues that you see with patch #6.


The two level routing has to be part of this (or another) irqchip
driver (specially given that it appears to me like another set of
crossbar). There should only be a *single* binding for all interrupts,
including those targeting the PRU (you seem to have two).

And the non-CPU interrupt code has to be in its own patch, because
it is pretty borderline anyway (I'm still not completely convinced
this is Linux's job).

         N,
-- 
Jazz is not dead. It just smells funny...

Hi Marc,

On 7/8/20 5:47 AM, Marc Zyngier wrote:
> On 2020-07-08 08:04, Grzegorz Jaszczyk wrote:

>> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:

>>>

>>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

>>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

>>> >>

>>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

>>>

>>> [...]

>>>

>>> >> It still begs the question: if the HW can support both edge and level

>>> >> triggered interrupts, why isn't the driver supporting this diversity?

>>> >> I appreciate that your HW may only have level interrupts so far, but

>>> >> what guarantees that this will forever be true? It would imply a

>>> >> change

>>> >> in the DT binding, which isn't desirable.

>>> >

>>> > Ok, I've got your point. I will try to come up with something later

>>> > on. Probably extending interrupt-cells by one and passing interrupt

>>> > type will be enough for now. Extending this driver to actually support

>>> > it can be handled later if needed. Hope it works for you.

>>>

>>> Writing a set_type callback to deal with this should be pretty easy.

>>> Don't delay doing the right thing.

>>

>> Ok.


Sorry for the typo in my comment causing this confusion.

The h/w actually doesn't support the edge-interrupts. Likewise, the 
polarity is always high. The individual register bit descriptions 
mention what the bit values 0 and 1 mean, but there is additional 
description in the TRMs on all the SoCs that says
"always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and
"always write 0 to the bits of this register" for PRUSS_INTC_SITR(x).
FWIW, these are also the reset values.

Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73
Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, 
4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC 
methodology.

>>

>>>

>>> [...]

>>>

>>> >> >> > +             hwirq = hipir & GENMASK(9, 0);

>>> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

>>> >> >>

>>> >> >> And this is where I worry. You seems to have a single irqdomain

>>> >> >> for all the muxes. Are you guaranteed that you will have no

>>> >> >> overlap between muxes? And please use irq_find_mapping(), as

>>> >> >> I have top-secret plans to kill irq_linear_revmap().

>>> >> >

>>> >> > Regarding irq_find_mapping - sure.

>>> >> >

>>> >> > Regarding irqdomains:

>>> >> > It is a single irqdomain since the hwirq (system event) can be 

>>> mapped

>>> >> > to different irq_host (muxes). Patch #6

>>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how 

>>> input

>>> >> > events can be mapped to some output host interrupts through 2 

>>> levels

>>> >> > of many-to-one mapping i.e. events to channel mapping and 

>>> channels to

>>> >> > host interrupts. Mentioned implementation ensures that specific 

>>> system

>>> >> > event (hwirq) can be mapped through PRUSS specific channel into a

>>> >> > single host interrupt.

>>> >>

>>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it 

>>> yet.

>>> >> Also, this driver seems to totally ignore the 2-level routing. Where

>>> >> is it set up? map/unmap in this driver do exactly *nothing*, so

>>> >> something somewhere must set it up.

>>> >

>>> > The map/unmap is updated in patch #6 and it deals with those 2-level

>>> > routing setup. Map is responsible for programming the Channel Map

>>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

>>> > provided configuration from the one parsed in the xlate function.

>>> > Unmap undo whatever was done on the map. More details can be found in

>>> > patch #6.

>>> >

>>> > Maybe it would be better to squash patch #6 with this one so it would

>>> > be less confusing. What is your advice?

>>>

>>> So am I right in understanding that without patch #6, this driver does

>>> exactly nothing? If so, it has been a waste of review time.

>>>

>>> Please split patch #6 so that this driver does something useful

>>> for Linux, without any of the PRU interrupt routing stuff. I want

>>> to see a Linux-only driver that works and doesn't rely on any other

>>> exotic feature.

>>>

>>

>> Patch #6 provides PRU specific 2-level routing setup. This step is

>> required and it is part of the entire patch-set. Theoretically routing

>> setup could be done by other platform driver (not irq one) or e.g. by

>> PRU firmware. In such case this driver would be functional without

>> patch #6 but I do not think it would be proper.

> 

> Then this whole driver is non-functional until the last patch that

> comes with the PRU-specific "value-add".


It is all moot actually and the interrupts work only when the PRU 
remoteproc/clients have invoked the irq_create_fwspec_mapping()
for all of the desired system events. It does not make much difference 
if it was a separate patch or squashed in, patch #6 is a replacement for 
the previous logic, and since it was complex, it was done in a separate 
patch to better explain the usage (same reason on v1 and v2 as well).

> 

> [...]

> 

>> I am open to any suggestion if there is a better way of handling

>> 2-level routing. I will also appreciate if you could elaborate about

>> issues that you see with patch #6.

> 

> The two level routing has to be part of this (or another) irqchip

> driver (specially given that it appears to me like another set of

> crossbar). There should only be a *single* binding for all interrupts,

> including those targeting the PRU (you seem to have two).

> 


Yeah, there hasn't been a clean way of doing this. Our previous attempt 
was to do this through custom exported functions so that the PRU 
remoteproc driver can set these up correctly, but that was shot down and 
this is the direction we are pointed to.

We do want to leverage the "interrupts" property in the PRU user nodes 
instead of inventing our own paradigm through a non-irqchip driver, and 
at the same time, be able to configure this at the run time only when 
that PRU driver is running, and remove the mappings once that driver is 
removed allowing another PRU application/driver. We treat PRUs as an 
exclusive resource, so everything needs to go along with an appropriate 
client user.

> And the non-CPU interrupt code has to be in its own patch, because

> it is pretty borderline anyway (I'm still not completely convinced

> this is Linux's job).


The logic for non-CPU interrupt code is exactly the same as the CPU 
interrupt code, as they are all setup through the 
irq_create_fwspec_mapping(). The CPU-specific pieces are primarily the 
chained interrupt handling.

We have already argued internally about the last part, but our firmware 
developers literally don't have any IRAM space (we have a lot of 
Industrial protocols working out of 4K/8K memory), and have pushed all 
one-time setup to the OS running (Linux or otherwise) on the main ARM 
core, and INTC is one among the other many such settings. Every word in 
Instruction RAM was crucial for them.

So, we are all ears if there is still an elegant way of doing this. Look 
forward to any suggestions you may have.

And thank you for all your review comments.

regards
Suman

Hi Marc,

> On 7/8/20 5:47 AM, Marc Zyngier wrote:

> > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote:

> >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:

> >>>

> >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

> >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

> >>> >>

> >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

> >>>

> >>> [...]

> >>>

> >>> >> It still begs the question: if the HW can support both edge and level

> >>> >> triggered interrupts, why isn't the driver supporting this diversity?

> >>> >> I appreciate that your HW may only have level interrupts so far, but

> >>> >> what guarantees that this will forever be true? It would imply a

> >>> >> change

> >>> >> in the DT binding, which isn't desirable.

> >>> >

> >>> > Ok, I've got your point. I will try to come up with something later

> >>> > on. Probably extending interrupt-cells by one and passing interrupt

> >>> > type will be enough for now. Extending this driver to actually support

> >>> > it can be handled later if needed. Hope it works for you.

> >>>

> >>> Writing a set_type callback to deal with this should be pretty easy.

> >>> Don't delay doing the right thing.

> >>

> >> Ok.

>

> Sorry for the typo in my comment causing this confusion.

>

> The h/w actually doesn't support the edge-interrupts. Likewise, the

> polarity is always high. The individual register bit descriptions

> mention what the bit values 0 and 1 mean, but there is additional

> description in the TRMs on all the SoCs that says

> "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and

> "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x).

> FWIW, these are also the reset values.

>

> Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73

> Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49,

> 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC

> methodology.

>

> >>

> >>>

> >>> [...]

> >>>

> >>> >> >> > +             hwirq = hipir & GENMASK(9, 0);

> >>> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

> >>> >> >>

> >>> >> >> And this is where I worry. You seems to have a single irqdomain

> >>> >> >> for all the muxes. Are you guaranteed that you will have no

> >>> >> >> overlap between muxes? And please use irq_find_mapping(), as

> >>> >> >> I have top-secret plans to kill irq_linear_revmap().

> >>> >> >

> >>> >> > Regarding irq_find_mapping - sure.

> >>> >> >

> >>> >> > Regarding irqdomains:

> >>> >> > It is a single irqdomain since the hwirq (system event) can be

> >>> mapped

> >>> >> > to different irq_host (muxes). Patch #6

> >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how

> >>> input

> >>> >> > events can be mapped to some output host interrupts through 2

> >>> levels

> >>> >> > of many-to-one mapping i.e. events to channel mapping and

> >>> channels to

> >>> >> > host interrupts. Mentioned implementation ensures that specific

> >>> system

> >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a

> >>> >> > single host interrupt.

> >>> >>

> >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it

> >>> yet.

> >>> >> Also, this driver seems to totally ignore the 2-level routing. Where

> >>> >> is it set up? map/unmap in this driver do exactly *nothing*, so

> >>> >> something somewhere must set it up.

> >>> >

> >>> > The map/unmap is updated in patch #6 and it deals with those 2-level

> >>> > routing setup. Map is responsible for programming the Channel Map

> >>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

> >>> > provided configuration from the one parsed in the xlate function.

> >>> > Unmap undo whatever was done on the map. More details can be found in

> >>> > patch #6.

> >>> >

> >>> > Maybe it would be better to squash patch #6 with this one so it would

> >>> > be less confusing. What is your advice?

> >>>

> >>> So am I right in understanding that without patch #6, this driver does

> >>> exactly nothing? If so, it has been a waste of review time.

> >>>

> >>> Please split patch #6 so that this driver does something useful

> >>> for Linux, without any of the PRU interrupt routing stuff. I want

> >>> to see a Linux-only driver that works and doesn't rely on any other

> >>> exotic feature.

> >>>

> >>

> >> Patch #6 provides PRU specific 2-level routing setup. This step is

> >> required and it is part of the entire patch-set. Theoretically routing

> >> setup could be done by other platform driver (not irq one) or e.g. by

> >> PRU firmware. In such case this driver would be functional without

> >> patch #6 but I do not think it would be proper.

> >

> > Then this whole driver is non-functional until the last patch that

> > comes with the PRU-specific "value-add".

>

> It is all moot actually and the interrupts work only when the PRU

> remoteproc/clients have invoked the irq_create_fwspec_mapping()

> for all of the desired system events. It does not make much difference

> if it was a separate patch or squashed in, patch #6 is a replacement for

> the previous logic, and since it was complex, it was done in a separate

> patch to better explain the usage (same reason on v1 and v2 as well).

>

> >

> > [...]

> >

> >> I am open to any suggestion if there is a better way of handling

> >> 2-level routing. I will also appreciate if you could elaborate about

> >> issues that you see with patch #6.

> >

> > The two level routing has to be part of this (or another) irqchip

> > driver (specially given that it appears to me like another set of

> > crossbar). There should only be a *single* binding for all interrupts,

> > including those targeting the PRU (you seem to have two).

> >

>

> Yeah, there hasn't been a clean way of doing this. Our previous attempt

> was to do this through custom exported functions so that the PRU

> remoteproc driver can set these up correctly, but that was shot down and

> this is the direction we are pointed to.

>

> We do want to leverage the "interrupts" property in the PRU user nodes

> instead of inventing our own paradigm through a non-irqchip driver, and

> at the same time, be able to configure this at the run time only when

> that PRU driver is running, and remove the mappings once that driver is

> removed allowing another PRU application/driver. We treat PRUs as an

> exclusive resource, so everything needs to go along with an appropriate

> client user.


I will just add an explanation about interrupt binding. So actually
there is one dt-binding defined in yaml (interrupt-cells = 1). The
reason why you see xlate allowing to proceed with 1 or 3 parameters is
because linux can change the PRU firmware at run-time (thorough linux
remoteproc framework) and different firmware may require different
kinds of interrupt mapping. Therefore during firmware load, the new
mapping is created through irq_create_fwspec_mapping() and in this
case 3 parameters are passed: system event, channel and host irq.
Similarly the mapping is disposed during remoteproc stop by invoking
irq_dispose_mapping. This allows to create new mapping, in the same
way, for next firmware loaded through Linux remote-proc at runtime
(depending on the needs of new remoteproc firmware).

On the other hand dt-bindings defines interrupt-cells = 1, so when the
interrupt is registered the xlate function (proceed with 1 parameter)
checks if this event already has valid mapping - if yes we are fine,
if not we return -EINVAL.

>

> > And the non-CPU interrupt code has to be in its own patch, because

> > it is pretty borderline anyway (I'm still not completely convinced

> > this is Linux's job).

>

> The logic for non-CPU interrupt code is exactly the same as the CPU

> interrupt code, as they are all setup through the

> irq_create_fwspec_mapping(). The CPU-specific pieces are primarily the

> chained interrupt handling.

>

> We have already argued internally about the last part, but our firmware

> developers literally don't have any IRAM space (we have a lot of

> Industrial protocols working out of 4K/8K memory), and have pushed all

> one-time setup to the OS running (Linux or otherwise) on the main ARM

> core, and INTC is one among the other many such settings. Every word in

> Instruction RAM was crucial for them.

>

> So, we are all ears if there is still an elegant way of doing this. Look

> forward to any suggestions you may have.


Yes, the non-CPU logic is exactly the same as the CPU interrupt code
as Suman described. There is no distinction between routing setup for
main CPU and PRU core, both use exactly the same logic, just different
numbers are passed through  irq_create_fwspec_mapping.

Looking forward to your feedback.

Best regards,
Grzegorz

Suman, Grzegorz,

On Wed, 15 Jul 2020 14:38:05 +0100,
Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> wrote:
> 

> Hi Marc,

> 

> > On 7/8/20 5:47 AM, Marc Zyngier wrote:

> > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote:

> > >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:

> > >>>

> > >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

> > >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

> > >>> >>

> > >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

> > >>>

> > >>> [...]

> > >>>

> > >>> >> It still begs the question: if the HW can support both edge and level

> > >>> >> triggered interrupts, why isn't the driver supporting this diversity?

> > >>> >> I appreciate that your HW may only have level interrupts so far, but

> > >>> >> what guarantees that this will forever be true? It would imply a

> > >>> >> change

> > >>> >> in the DT binding, which isn't desirable.

> > >>> >

> > >>> > Ok, I've got your point. I will try to come up with something later

> > >>> > on. Probably extending interrupt-cells by one and passing interrupt

> > >>> > type will be enough for now. Extending this driver to actually support

> > >>> > it can be handled later if needed. Hope it works for you.

> > >>>

> > >>> Writing a set_type callback to deal with this should be pretty easy.

> > >>> Don't delay doing the right thing.

> > >>

> > >> Ok.

> >

> > Sorry for the typo in my comment causing this confusion.

> >

> > The h/w actually doesn't support the edge-interrupts. Likewise, the

> > polarity is always high. The individual register bit descriptions

> > mention what the bit values 0 and 1 mean, but there is additional

> > description in the TRMs on all the SoCs that says

> > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and

> > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x).

> > FWIW, these are also the reset values.

> >

> > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73

> > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49,

> > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC

> > methodology.

> >

> > >>

> > >>>

> > >>> [...]

> > >>>

> > >>> >> >> > +             hwirq = hipir & GENMASK(9, 0);

> > >>> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

> > >>> >> >>

> > >>> >> >> And this is where I worry. You seems to have a single irqdomain

> > >>> >> >> for all the muxes. Are you guaranteed that you will have no

> > >>> >> >> overlap between muxes? And please use irq_find_mapping(), as

> > >>> >> >> I have top-secret plans to kill irq_linear_revmap().

> > >>> >> >

> > >>> >> > Regarding irq_find_mapping - sure.

> > >>> >> >

> > >>> >> > Regarding irqdomains:

> > >>> >> > It is a single irqdomain since the hwirq (system event) can be

> > >>> mapped

> > >>> >> > to different irq_host (muxes). Patch #6

> > >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how

> > >>> input

> > >>> >> > events can be mapped to some output host interrupts through 2

> > >>> levels

> > >>> >> > of many-to-one mapping i.e. events to channel mapping and

> > >>> channels to

> > >>> >> > host interrupts. Mentioned implementation ensures that specific

> > >>> system

> > >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a

> > >>> >> > single host interrupt.

> > >>> >>

> > >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it

> > >>> yet.

> > >>> >> Also, this driver seems to totally ignore the 2-level routing. Where

> > >>> >> is it set up? map/unmap in this driver do exactly *nothing*, so

> > >>> >> something somewhere must set it up.

> > >>> >

> > >>> > The map/unmap is updated in patch #6 and it deals with those 2-level

> > >>> > routing setup. Map is responsible for programming the Channel Map

> > >>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

> > >>> > provided configuration from the one parsed in the xlate function.

> > >>> > Unmap undo whatever was done on the map. More details can be found in

> > >>> > patch #6.

> > >>> >

> > >>> > Maybe it would be better to squash patch #6 with this one so it would

> > >>> > be less confusing. What is your advice?

> > >>>

> > >>> So am I right in understanding that without patch #6, this driver does

> > >>> exactly nothing? If so, it has been a waste of review time.

> > >>>

> > >>> Please split patch #6 so that this driver does something useful

> > >>> for Linux, without any of the PRU interrupt routing stuff. I want

> > >>> to see a Linux-only driver that works and doesn't rely on any other

> > >>> exotic feature.

> > >>>

> > >>

> > >> Patch #6 provides PRU specific 2-level routing setup. This step is

> > >> required and it is part of the entire patch-set. Theoretically routing

> > >> setup could be done by other platform driver (not irq one) or e.g. by

> > >> PRU firmware. In such case this driver would be functional without

> > >> patch #6 but I do not think it would be proper.

> > >

> > > Then this whole driver is non-functional until the last patch that

> > > comes with the PRU-specific "value-add".

> >

> > It is all moot actually and the interrupts work only when the PRU

> > remoteproc/clients have invoked the irq_create_fwspec_mapping()

> > for all of the desired system events. It does not make much difference

> > if it was a separate patch or squashed in, patch #6 is a replacement for

> > the previous logic, and since it was complex, it was done in a separate

> > patch to better explain the usage (same reason on v1 and v2 as

> > well).


It may make no difference to you, but it does for me, as I'm the lucky
idiot reviewing this code. So I am going to say it again: please keep
anything that only exists for the PRU subsystem benefit out of the
initial patches.

I want to see something that works for Linux, and only for Linux. Once
we have that working, we'll see to add more stuff. But stop throwing
the PRU business into the early patches, as all you are achieving is
to delay the whole thing.

> >

> > >

> > > [...]

> > >

> > >> I am open to any suggestion if there is a better way of handling

> > >> 2-level routing. I will also appreciate if you could elaborate about

> > >> issues that you see with patch #6.

> > >

> > > The two level routing has to be part of this (or another) irqchip

> > > driver (specially given that it appears to me like another set of

> > > crossbar). There should only be a *single* binding for all interrupts,

> > > including those targeting the PRU (you seem to have two).

> > >

> >

> > Yeah, there hasn't been a clean way of doing this. Our previous attempt

> > was to do this through custom exported functions so that the PRU

> > remoteproc driver can set these up correctly, but that was shot down and

> > this is the direction we are pointed to.

> >

> > We do want to leverage the "interrupts" property in the PRU user nodes

> > instead of inventing our own paradigm through a non-irqchip driver, and

> > at the same time, be able to configure this at the run time only when

> > that PRU driver is running, and remove the mappings once that driver is

> > removed allowing another PRU application/driver. We treat PRUs as an

> > exclusive resource, so everything needs to go along with an appropriate

> > client user.

> 

> I will just add an explanation about interrupt binding. So actually

> there is one dt-binding defined in yaml (interrupt-cells = 1). The

> reason why you see xlate allowing to proceed with 1 or 3 parameters is

> because linux can change the PRU firmware at run-time (thorough linux

> remoteproc framework) and different firmware may require different

> kinds of interrupt mapping. Therefore during firmware load, the new

> mapping is created through irq_create_fwspec_mapping() and in this

> case 3 parameters are passed: system event, channel and host irq.

> Similarly the mapping is disposed during remoteproc stop by invoking

> irq_dispose_mapping. This allows to create new mapping, in the same

> way, for next firmware loaded through Linux remote-proc at runtime

> (depending on the needs of new remoteproc firmware).

> 

> On the other hand dt-bindings defines interrupt-cells = 1, so when the

> interrupt is registered the xlate function (proceed with 1 parameter)

> checks if this event already has valid mapping - if yes we are fine,

> if not we return -EINVAL.


It means that interrupts declared in DT get their two-level routing
via the kernel driver, while PRU interrupts get their routing via some
external blob that Linux is not in control of?

If so, this looks broken. What if you get a resource allocation
conflict because the kernel and the blob are stepping into each
other's toes? Why should an end-point client decide on the routing of
the interrupt?

All the end-point should provide is the ID of the input signal, and to
which PRU this is routed. Interrupts described in DT should have the
exact same model (input signal, target). All the intermediate routing
logic should be handled by the Linux driver for *all* interrupts in
the system.

> 

> >

> > > And the non-CPU interrupt code has to be in its own patch, because

> > > it is pretty borderline anyway (I'm still not completely convinced

> > > this is Linux's job).

> >

> > The logic for non-CPU interrupt code is exactly the same as the CPU

> > interrupt code, as they are all setup through the

> > irq_create_fwspec_mapping(). The CPU-specific pieces are primarily the

> > chained interrupt handling.

> >

> > We have already argued internally about the last part, but our firmware

> > developers literally don't have any IRAM space (we have a lot of

> > Industrial protocols working out of 4K/8K memory), and have pushed all

> > one-time setup to the OS running (Linux or otherwise) on the main ARM

> > core, and INTC is one among the other many such settings. Every word in

> > Instruction RAM was crucial for them.


And that's fine. Just push *all* of it into Linux, and not just the
programming of the registers.

> >

> > So, we are all ears if there is still an elegant way of doing this. Look

> > forward to any suggestions you may have.

> 

> Yes, the non-CPU logic is exactly the same as the CPU interrupt code

> as Suman described. There is no distinction between routing setup for

> main CPU and PRU core, both use exactly the same logic, just different

> numbers are passed through  irq_create_fwspec_mapping.


It obviously isn't the same at the moment. You have two distinct code
paths, two ways to describe a mapping, and a potential resource
allocation issue.

	M.

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

Hi Marc,

Thank you again.

On Tue, 21 Jul 2020 at 12:10, Marc Zyngier <maz@kernel.org> wrote:
>

> On 2020-07-21 10:27, Grzegorz Jaszczyk wrote:

> > Hi Marc,

> >

> > First of all thank you very much for your review. I apologize in

> > advance if the description below is too verbose or not detailed

> > enough.

> >

> > On Fri, 17 Jul 2020 at 14:36, Marc Zyngier <maz@kernel.org> wrote:

> >>

> >> Suman, Grzegorz,

> >>

> >> On Wed, 15 Jul 2020 14:38:05 +0100,

> >> Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> wrote:

> >> >

> >> > Hi Marc,

> >> >

> >> > > On 7/8/20 5:47 AM, Marc Zyngier wrote:

> >> > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote:

> >> > > >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:

> >> > > >>>

> >> > > >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:

> >> > > >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.org> wrote:

> >> > > >>> >>

> >> > > >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:

> >> > > >>>

> >> > > >>> [...]

> >> > > >>>

> >> > > >>> >> It still begs the question: if the HW can support both edge and level

> >> > > >>> >> triggered interrupts, why isn't the driver supporting this diversity?

> >> > > >>> >> I appreciate that your HW may only have level interrupts so far, but

> >> > > >>> >> what guarantees that this will forever be true? It would imply a

> >> > > >>> >> change

> >> > > >>> >> in the DT binding, which isn't desirable.

> >> > > >>> >

> >> > > >>> > Ok, I've got your point. I will try to come up with something later

> >> > > >>> > on. Probably extending interrupt-cells by one and passing interrupt

> >> > > >>> > type will be enough for now. Extending this driver to actually support

> >> > > >>> > it can be handled later if needed. Hope it works for you.

> >> > > >>>

> >> > > >>> Writing a set_type callback to deal with this should be pretty easy.

> >> > > >>> Don't delay doing the right thing.

> >> > > >>

> >> > > >> Ok.

> >> > >

> >> > > Sorry for the typo in my comment causing this confusion.

> >> > >

> >> > > The h/w actually doesn't support the edge-interrupts. Likewise, the

> >> > > polarity is always high. The individual register bit descriptions

> >> > > mention what the bit values 0 and 1 mean, but there is additional

> >> > > description in the TRMs on all the SoCs that says

> >> > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and

> >> > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x).

> >> > > FWIW, these are also the reset values.

> >> > >

> >> > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73

> >> > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49,

> >> > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC

> >> > > methodology.

> >> > >

> >> > > >>

> >> > > >>>

> >> > > >>> [...]

> >> > > >>>

> >> > > >>> >> >> > +             hwirq = hipir & GENMASK(9, 0);

> >> > > >>> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);

> >> > > >>> >> >>

> >> > > >>> >> >> And this is where I worry. You seems to have a single irqdomain

> >> > > >>> >> >> for all the muxes. Are you guaranteed that you will have no

> >> > > >>> >> >> overlap between muxes? And please use irq_find_mapping(), as

> >> > > >>> >> >> I have top-secret plans to kill irq_linear_revmap().

> >> > > >>> >> >

> >> > > >>> >> > Regarding irq_find_mapping - sure.

> >> > > >>> >> >

> >> > > >>> >> > Regarding irqdomains:

> >> > > >>> >> > It is a single irqdomain since the hwirq (system event) can be

> >> > > >>> mapped

> >> > > >>> >> > to different irq_host (muxes). Patch #6

> >> > > >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how

> >> > > >>> input

> >> > > >>> >> > events can be mapped to some output host interrupts through 2

> >> > > >>> levels

> >> > > >>> >> > of many-to-one mapping i.e. events to channel mapping and

> >> > > >>> channels to

> >> > > >>> >> > host interrupts. Mentioned implementation ensures that specific

> >> > > >>> system

> >> > > >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a

> >> > > >>> >> > single host interrupt.

> >> > > >>> >>

> >> > > >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it

> >> > > >>> yet.

> >> > > >>> >> Also, this driver seems to totally ignore the 2-level routing. Where

> >> > > >>> >> is it set up? map/unmap in this driver do exactly *nothing*, so

> >> > > >>> >> something somewhere must set it up.

> >> > > >>> >

> >> > > >>> > The map/unmap is updated in patch #6 and it deals with those 2-level

> >> > > >>> > routing setup. Map is responsible for programming the Channel Map

> >> > > >>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on

> >> > > >>> > provided configuration from the one parsed in the xlate function.

> >> > > >>> > Unmap undo whatever was done on the map. More details can be found in

> >> > > >>> > patch #6.

> >> > > >>> >

> >> > > >>> > Maybe it would be better to squash patch #6 with this one so it would

> >> > > >>> > be less confusing. What is your advice?

> >> > > >>>

> >> > > >>> So am I right in understanding that without patch #6, this driver does

> >> > > >>> exactly nothing? If so, it has been a waste of review time.

> >> > > >>>

> >> > > >>> Please split patch #6 so that this driver does something useful

> >> > > >>> for Linux, without any of the PRU interrupt routing stuff. I want

> >> > > >>> to see a Linux-only driver that works and doesn't rely on any other

> >> > > >>> exotic feature.

> >> > > >>>

> >> > > >>

> >> > > >> Patch #6 provides PRU specific 2-level routing setup. This step is

> >> > > >> required and it is part of the entire patch-set. Theoretically routing

> >> > > >> setup could be done by other platform driver (not irq one) or e.g. by

> >> > > >> PRU firmware. In such case this driver would be functional without

> >> > > >> patch #6 but I do not think it would be proper.

> >> > > >

> >> > > > Then this whole driver is non-functional until the last patch that

> >> > > > comes with the PRU-specific "value-add".

> >> > >

> >> > > It is all moot actually and the interrupts work only when the PRU

> >> > > remoteproc/clients have invoked the irq_create_fwspec_mapping()

> >> > > for all of the desired system events. It does not make much difference

> >> > > if it was a separate patch or squashed in, patch #6 is a replacement for

> >> > > the previous logic, and since it was complex, it was done in a separate

> >> > > patch to better explain the usage (same reason on v1 and v2 as

> >> > > well).

> >>

> >> It may make no difference to you, but it does for me, as I'm the lucky

> >> idiot reviewing this code. So I am going to say it again: please keep

> >> anything that only exists for the PRU subsystem benefit out of the

> >> initial patches.

> >>

> >> I want to see something that works for Linux, and only for Linux. Once

> >> we have that working, we'll see to add more stuff. But stop throwing

> >> the PRU business into the early patches, as all you are achieving is

> >> to delay the whole thing.

> >>

> >> > >

> >> > > >

> >> > > > [...]

> >> > > >

> >> > > >> I am open to any suggestion if there is a better way of handling

> >> > > >> 2-level routing. I will also appreciate if you could elaborate about

> >> > > >> issues that you see with patch #6.

> >> > > >

> >> > > > The two level routing has to be part of this (or another) irqchip

> >> > > > driver (specially given that it appears to me like another set of

> >> > > > crossbar). There should only be a *single* binding for all interrupts,

> >> > > > including those targeting the PRU (you seem to have two).

> >> > > >

> >> > >

> >> > > Yeah, there hasn't been a clean way of doing this. Our previous attempt

> >> > > was to do this through custom exported functions so that the PRU

> >> > > remoteproc driver can set these up correctly, but that was shot down and

> >> > > this is the direction we are pointed to.

> >> > >

> >> > > We do want to leverage the "interrupts" property in the PRU user nodes

> >> > > instead of inventing our own paradigm through a non-irqchip driver, and

> >> > > at the same time, be able to configure this at the run time only when

> >> > > that PRU driver is running, and remove the mappings once that driver is

> >> > > removed allowing another PRU application/driver. We treat PRUs as an

> >> > > exclusive resource, so everything needs to go along with an appropriate

> >> > > client user.

> >> >

> >> > I will just add an explanation about interrupt binding. So actually

> >> > there is one dt-binding defined in yaml (interrupt-cells = 1). The

> >> > reason why you see xlate allowing to proceed with 1 or 3 parameters is

> >> > because linux can change the PRU firmware at run-time (thorough linux

> >> > remoteproc framework) and different firmware may require different

> >> > kinds of interrupt mapping. Therefore during firmware load, the new

> >> > mapping is created through irq_create_fwspec_mapping() and in this

> >> > case 3 parameters are passed: system event, channel and host irq.

> >> > Similarly the mapping is disposed during remoteproc stop by invoking

> >> > irq_dispose_mapping. This allows to create new mapping, in the same

> >> > way, for next firmware loaded through Linux remote-proc at runtime

> >> > (depending on the needs of new remoteproc firmware).

> >> >

> >> > On the other hand dt-bindings defines interrupt-cells = 1, so when the

> >> > interrupt is registered the xlate function (proceed with 1 parameter)

> >> > checks if this event already has valid mapping - if yes we are fine,

> >> > if not we return -EINVAL.

> >>

> >> It means that interrupts declared in DT get their two-level routing

> >> via the kernel driver, while PRU interrupts get their routing via some

> >> external blob that Linux is not in control of?

> >

> > Actually with the current approach all two-level routing goes through

> > this linux driver. The interrupts that should be routed to PRU are

> > described in remoteproc firmware resource table [1] and it is under

> > Linux remoteproc driver control. In general, the resource table

> > contains system resources that the remote processor requires before it

> > should be powered on. We treat the interrupt mapping (described in the

> > resource table, which is a dedicated elf section defined in [1]) as

> > one of system resources that linux has to provide before we power on

> > the PRU core. Therefore the remoteproce driver will parse the resource

> > table and trigger irq_create_fwspec_mapping() after validating

> > resource table content.

>

> Validating the resource table says nothing of a potential conflict

> with previous configured interrupts.


Yes, that's why we introduced the logic in pruss_intc_irq_domain_xlate
and pruss_intc_map triggered by irq_create_fwspec_mapping, which will
check potential conflicts with previous configured interrupts. I
understand that you do not like how it is done but I do not know how
to do it in a different way so it will cover all caveats, please see
below.

>

> >

> > [1] https://www.kernel.org/doc/Documentation/remoteproc.txt (Binary

> > Firmware Structure)

> >

> >>

> >> If so, this looks broken. What if you get a resource allocation

> >> conflict because the kernel and the blob are stepping into each

> >> other's toes? Why should an end-point client decide on the routing of

> >> the interrupt?

> >

> > The code in the pruss_intc_map function checks if there are no

> > allocation conflicts: e.g. if the sysevent is already assigned it will

> > throw -EBUSY. Similarly when some channel was already assigned to

> > host_irq and a different assignment is requested it will again throw

> > -EBUSY.

>

> But why should it? The allocation should take place based on constraints

> (source, target, and as you mentioned below, priority). Instead, you

> seem to be relying on static allocation coming from binary blobs,

> standardized or not.

>

> I claim that this static allocation is madness and should be eliminated.

> Instead, the Linux driver should perform the routing based on allocation

> requirements (the above constraints), and only fail if it cannot satisfy

> these constraints.


I am not sure if I understood. The allocation requirements are as
you've described: source (system event), target (host interrupt) and
priority (channel).
E.g.:
- routing system event 3 with priority (chanell) 2 to PRU core 0 will
be described as bellow: (3, 2, 0) (0 corresponds to PRU0)
- routing system event 10 with priority (chanell) 3 to PRU core 1: (10, 3, 1)
- routing system event 15 with priority (5) to MCPU interrupt 0*: (15, 5, 2)
* interrupts 2 through 9 (host_intr0 through host_intr7)

I am not sure but you probably refer to changing it to loosely dynamic
allocation but this will not work for any of them since:
- different system event is just a different sources (e.g. some of
them are tightly coupled with PRUSS industrial ethernet peripheral,
other with PRUSS UART, other are general purpose ones and so on).
- lower number channels have higher priority (10 different channels,
each with different priority).
- host interrupt 0 is for PRU core 0; host interrupt 1 is for PRU core
1; host interrupts 2 through 9 are for main CPU.

So the logic in patch #6 prevents mapping system events if it was
already assigned to a different channel or target (host interrupt) and
only fails if it cannot be satisfied. Moreover I do not see a way to
relax the static description since picking different numbers for each
individual: system event, channel and host interrupt will result with
something unintentional and wrong.

Sorry if I misunderstood you, if so could you please elaborate?

Thank you,
Grzegorz