diff mbox series

[4/6] irqchip/irq-pruss-intc: Add helper functions to configure internal mapping

Message ID 20190708035243.12170-5-s-anna@ti.com (mailing list archive)
State New, archived
Headers show
Series Add TI PRUSS Local Interrupt Controller IRQChip driver | expand

Commit Message

Suman Anna July 8, 2019, 3:52 a.m. UTC
The PRUSS INTC receives a number of system input interrupt source events
and supports individual control configuration and hardware prioritization.
These 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.

This mapping information is provided through the PRU firmware that is
loaded onto a PRU core/s or through the device tree node of the PRU
application. The mapping is configured by the PRU remoteproc driver, and
is setup before the PRU core is started and cleaned up after the PRU core
is stopped. This event mapping configuration logic is optimized to program
the Channel Map Registers (CMRx) and Host-Interrupt Map Registers (HMRx)
only when a new program is being loaded/started and simply disables the
same events and interrupt channels without zeroing out the corresponding
map registers when stopping a PRU.

Add two helper functions: pruss_intc_configure() & pruss_intc_unconfigure()
that the PRU remoteproc driver can use to configure the PRUSS INTC.

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>
---
 drivers/irqchip/irq-pruss-intc.c       | 258 ++++++++++++++++++++++++-
 include/linux/irqchip/irq-pruss-intc.h |  33 ++++
 2 files changed, 289 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/irqchip/irq-pruss-intc.h

Comments

David Lechner July 11, 2019, 3:10 a.m. UTC | #1
On 7/7/19 10:52 PM, Suman Anna wrote:
> The PRUSS INTC receives a number of system input interrupt source events
> and supports individual control configuration and hardware prioritization.
> These 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.
> 
> This mapping information is provided through the PRU firmware that is
> loaded onto a PRU core/s or through the device tree node of the PRU

What will the device tree bindings for this look like?

Looking back at Rob's comment on the initial series [1], I still think
that increasing the #interrupt-cells sounds like a reasonable solution.

[1]: https://patchwork.kernel.org/patch/10697705/#22375155



> application. The mapping is configured by the PRU remoteproc driver, and
> is setup before the PRU core is started and cleaned up after the PRU core
> is stopped. This event mapping configuration logic is optimized to program
> the Channel Map Registers (CMRx) and Host-Interrupt Map Registers (HMRx)
> only when a new program is being loaded/started and simply disables the
> same events and interrupt channels without zeroing out the corresponding
> map registers when stopping a PRU.
> 
> Add two helper functions: pruss_intc_configure() & pruss_intc_unconfigure()
> that the PRU remoteproc driver can use to configure the PRUSS INTC.
> 
> 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>
> ---
>   drivers/irqchip/irq-pruss-intc.c       | 258 ++++++++++++++++++++++++-
>   include/linux/irqchip/irq-pruss-intc.h |  33 ++++
>   2 files changed, 289 insertions(+), 2 deletions(-)
>   create mode 100644 include/linux/irqchip/irq-pruss-intc.h
> 
> diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
> index 142d01b434e0..8118c2a2ac43 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -9,6 +9,7 @@
>   
>   #include <linux/irq.h>
>   #include <linux/irqchip/chained_irq.h>
> +#include <linux/irqchip/irq-pruss-intc.h>
>   #include <linux/irqdomain.h>
>   #include <linux/module.h>
>   #include <linux/of_device.h>
> @@ -24,8 +25,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
> +/* maximum number of host interrupts */
> +#define MAX_PRU_HOST_INT	10
>   
>   /* PRU_ICSS_INTC registers */
>   #define PRU_INTC_REVID		0x0000
> @@ -57,15 +58,29 @@
>   #define PRU_INTC_HINLR(x)	(0x1100 + (x) * 4)
>   #define PRU_INTC_HIER		0x1500
>   
> +/* CMR register bit-field macros */
> +#define CMR_EVT_MAP_MASK	0xf
> +#define CMR_EVT_MAP_BITS	8
> +#define CMR_EVT_PER_REG		4
> +
> +/* HMR register bit-field macros */
> +#define HMR_CH_MAP_MASK		0xf
> +#define HMR_CH_MAP_BITS		8
> +#define HMR_CH_PER_REG		4
> +
>   /* HIPIR register bit-fields */
>   #define INTC_HIPIR_NONE_HINT	0x80000000
>   
> +/* use -1 to mark unassigned events and channels */
> +#define FREE			-1

It could be helpful to have this macro in the public header.

> +
>   /**
>    * struct pruss_intc - PRUSS interrupt controller structure
>    * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>    * @base: base virtual address of INTC register space
>    * @irqchip: irq chip for this interrupt controller
>    * @domain: irq domain for this interrupt controller
> + * @config_map: stored INTC configuration mapping data
>    * @lock: mutex to serialize access to INTC
>    * @host_mask: indicate which HOST IRQs are enabled
>    * @shared_intr: bit-map denoting if the MPU host interrupt is shared
> @@ -76,6 +91,7 @@ struct pruss_intc {
>   	void __iomem *base;
>   	struct irq_chip *irqchip;
>   	struct irq_domain *domain;
> +	struct pruss_intc_config config_map;
>   	struct mutex lock; /* PRUSS INTC lock */
>   	u32 host_mask;
>   	u16 shared_intr;
> @@ -107,6 +123,238 @@ static int pruss_intc_check_write(struct pruss_intc *intc, unsigned int reg,
>   	return 0;
>   }
>   
> +static struct pruss_intc *to_pruss_intc(struct device *pru_dev)
> +{
> +	struct device_node *np;
> +	struct platform_device *pdev;
> +	struct device *pruss_dev = pru_dev->parent;
> +	struct pruss_intc *intc = ERR_PTR(-ENODEV);
> +
> +	np = of_get_child_by_name(pruss_dev->of_node, "interrupt-controller");
> +	if (!np) {
> +		dev_err(pruss_dev, "pruss does not have an interrupt-controller node\n");
> +		return intc;
> +	}
> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev) {
> +		dev_err(pruss_dev, "no associated platform device\n");
> +		goto out;
> +	}
> +
> +	intc = platform_get_drvdata(pdev);
> +	if (!intc) {
> +		dev_err(pruss_dev, "pruss intc device probe failed?\n");
> +		intc = ERR_PTR(-EINVAL);
> +	}
> +
> +out:
> +	of_node_put(np);
> +	return intc;
> +}
> +
> +/**
> + * pruss_intc_configure() - configure the PRUSS INTC
> + * @dev: pru device pointer
> + * @intc_config: PRU core-specific INTC configuration
> + *
> + * Configures the PRUSS INTC with the provided configuration from
> + * a PRU core. Any existing event to channel mappings or channel to
> + * host interrupt mappings are checked to make sure there are no
> + * conflicting configuration between both the PRU cores. The function
> + * is intended to be used only by the PRU remoteproc driver.
> + *
> + * Returns 0 on success, or a suitable error code otherwise
> + */
> +int pruss_intc_configure(struct device *dev,

It seems like this would be easier to use if it took an IRQ number
or struct irq_data * as a parameter instead of struct device *. My
line of thinking is that callers of this function will already be
calling some variant of request_irq() so they will already have
this info. It would cut out the pointer acrobatics in to_pruss_intc.


> +			 struct pruss_intc_config *intc_config)
> +{
> +	struct pruss_intc *intc;
> +	int i, idx, ret;
> +	s8 ch, host;
> +	u64 sysevt_mask = 0;
> +	u32 ch_mask = 0;
> +	u32 host_mask = 0;
> +	u32 val;
> +
> +	intc = to_pruss_intc(dev);
> +	if (IS_ERR(intc))
> +		return PTR_ERR(intc);
> +
> +	mutex_lock(&intc->lock);
> +
> +	/*
> +	 * configure channel map registers - each register holds map info
> +	 * for 4 events, with each event occupying the lower nibble in
> +	 * a register byte address in little-endian fashion
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(intc_config->sysev_to_ch); i++) {
> +		ch = intc_config->sysev_to_ch[i];
> +		if (ch < 0)
> +			continue;
> +
> +		/* check if sysevent already assigned */
> +		if (intc->config_map.sysev_to_ch[i] != FREE) {
> +			dev_err(dev, "event %d (req. channel %d) already assigned to channel %d\n",
> +				i, ch, intc->config_map.sysev_to_ch[i]);
> +			ret = -EEXIST;
> +			goto unlock;

If we fail here, shouldn't we unwind any previous mappings made?
Otherwise, if we try to map the same event again, it will show as
in use, even though it is not in use.

> +		}
> +
> +		intc->config_map.sysev_to_ch[i] = ch;
> +
> +		idx = i / CMR_EVT_PER_REG;
> +		val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
> +		val &= ~(CMR_EVT_MAP_MASK <<
> +			 ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS));
> +		val |= ch << ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS);
> +		pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
> +		sysevt_mask |= BIT_ULL(i);
> +		ch_mask |= BIT(ch);
> +
> +		dev_dbg(dev, "SYSEV%d -> CH%d (CMR%d 0x%08x)\n", i, ch, idx,
> +			pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
> +	}
> +
> +	/*
> +	 * set host map registers - each register holds map info for
> +	 * 4 channels, with each channel occupying the lower nibble in
> +	 * a register byte address in little-endian fashion
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(intc_config->ch_to_host); i++) {
> +		host = intc_config->ch_to_host[i];
> +		if (host < 0)
> +			continue;
> +
> +		/* check if channel already assigned */
> +		if (intc->config_map.ch_to_host[i] != FREE) {
> +			dev_err(dev, "channel %d (req. intr_no %d) already assigned to intr_no %d\n",
> +				i, host, intc->config_map.ch_to_host[i]);
> +			ret = -EEXIST;
> +			goto unlock;

Same comment about unwinding here and below.

> +		}
> +
> +		/* check if host intr is already in use by other PRU */

It seems like there would be use cases where someone might want to map
multiple PRU system events, and therefore multiple channels, to a single
host interrupt.

> +		if (intc->host_mask & (1U << host)) {
> +			dev_err(dev, "%s: host intr %d already in use\n",
> +				__func__, host);
> +			ret = -EEXIST;
> +			goto unlock;
> +		}
> +

--snip--

> diff --git a/include/linux/irqchip/irq-pruss-intc.h b/include/linux/irqchip/irq-pruss-intc.h
> new file mode 100644
> index 000000000000..f1f1bb150100
> --- /dev/null
> +++ b/include/linux/irqchip/irq-pruss-intc.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PRU-ICSS sub-system private interfaces
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *	Suman Anna <s-anna@ti.com>
> + */
> +
> +#ifndef __LINUX_IRQ_PRUSS_INTC_H
> +#define __LINUX_IRQ_PRUSS_INTC_H
> +
> +/* maximum number of system events */
> +#define MAX_PRU_SYS_EVENTS	64
> +
> +/* maximum number of interrupt channels */
> +#define MAX_PRU_CHANNELS	10
> +
> +/**
> + * struct pruss_intc_config - INTC configuration info
> + * @sysev_to_ch: system events to channel mapping information
> + * @ch_to_host: interrupt channel to host interrupt information
> + */
> +struct pruss_intc_config {
> +	s8 sysev_to_ch[MAX_PRU_SYS_EVENTS];
> +	s8 ch_to_host[MAX_PRU_CHANNELS];
> +};
> +
> +int pruss_intc_configure(struct device *dev,
> +			 struct pruss_intc_config *intc_config);
> +int pruss_intc_unconfigure(struct device *dev,
> +			   struct pruss_intc_config *intc_config);
> +
> +#endif	/* __LINUX_IRQ_PRUSS_INTC_H */
> 

FYI, on AM18xx, events 0 to 31 can be muxed via CFGCHIP3[3].PRUSSEVTSEL
so an additional bit of information will be needed in this struct for
the mux selection. I don't see a probably with adding that later though.
David Lechner July 11, 2019, 10:09 p.m. UTC | #2
On 7/10/19 10:10 PM, David Lechner wrote:
> On 7/7/19 10:52 PM, Suman Anna wrote:
>> The PRUSS INTC receives a number of system input interrupt source events
>> and supports individual control configuration and hardware prioritization.
>> These 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.
>>
>> This mapping information is provided through the PRU firmware that is
>> loaded onto a PRU core/s or through the device tree node of the PRU
> 
> What will the device tree bindings for this look like?
> 
> Looking back at Rob's comment on the initial series [1], I still think
> that increasing the #interrupt-cells sounds like a reasonable solution.
> 
> [1]: https://patchwork.kernel.org/patch/10697705/#22375155
> 

I have come up with an alternative to this patch that addresses my
previous comments.

I propose that we change the device tree bindings to have:

     #interrupt-cells = <3>;

Where the parameters are the system event, the channel and the host
event.

In the case of AM18xx, we will need 4 cells, with the 4th one being
the EVTSEL value (the first 32 events are multiplexed so just giving
the system event is not enough info).

With the patch below, we don't need to introduce any new public APIs.
Instead, we implement the irqchip xlate callback. This parses the
device tree parameters and saves them for later.

For the case where the mapping comes from the PRU firmware instead
of the device tree, the remoteproc driver can just call
irq_create_fwspec_mapping() to register the mapping and the same
xlate function handle it just the same as if it came from a device
tree.

The channel mapping is now done in the irqchip map callback. Reference
counting is used to allow shared channels and host events and still
give an error when there are conflicting requests. This also simplifies
the code a bit since we aren't dealing with multiple mappings at one time.

There is a bit more work to do to get EVTSEL working on AM18xx, but
I've tested that both device tree and irq_create_fwspec_mapping()
work.

Complete set of my hacks can be found at [1].

[1]: https://github.com/dlech/linux/tree/pruss-2019-07-11

---
 From 199a13a2f224489bd95e03424c0ae98744dea364 Mon Sep 17 00:00:00 2001
From: Suman Anna <s-anna@ti.com>
Date: Sun, 7 Jul 2019 22:52:41 -0500
Subject: [PATCH] irqchip/irq-pruss-intc: Add event mapping support

The PRUSS INTC receives a number of system input interrupt source events
and supports individual control configuration and hardware prioritization.
These 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.

This mapping information is provided through the PRU firmware that is
loaded onto a PRU core/s or through the device tree node of the PRU
application. The mapping is configured when an IRQ is requested, and
cleaned up when the IRQ is freed. Reference counting is used to allow
multiple system events to share a single channel and to allow multiple
channels to share a single host event.

The remoteproc driver can register mappings read from a firmware blob
as shown below. The fwnode parameters must match the device tree
bindings.

	struct irq_fwspec fwspec;
	int irq;

	fwspec.fwnode = of_node_to_fwnode(dev->of_node);
	fwspec.param_count = 3;
	fwspec.param[0] = 63; // system event
	fwspec.param[1] = 5;  // channel
	fwspec.param[2] = 6;  // host event

	irq = irq_create_fwspec_mapping(&fwspec);
	if (irq < 0) {
		dev_err(dev, "failed to get irq\n");
		return irq;
	}

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: David Lechner <david@lechnology.com>
---
  drivers/irqchip/irq-pruss-intc.c | 302 ++++++++++++++++++++++++++++++-
  1 file changed, 293 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 142d01b434e0..dd14addfd0b4 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -13,6 +13,7 @@
  #include <linux/module.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
  
  /*
   * Number of host interrupts reaching the main MPU sub-system. Note that this
@@ -24,6 +25,12 @@
  /* minimum starting host interrupt number for MPU */
  #define MIN_PRU_HOST_INT	2
  
+/* maximum number of host interrupts */
+#define MAX_PRU_HOST_INT	10
+
+/* maximum number of interrupt channels */
+#define MAX_PRU_CHANNELS	10
+
  /* maximum number of system events */
  #define MAX_PRU_SYS_EVENTS	64
  
@@ -57,29 +64,71 @@
  #define PRU_INTC_HINLR(x)	(0x1100 + (x) * 4)
  #define PRU_INTC_HIER		0x1500
  
+/* CMR register bit-field macros */
+#define CMR_EVT_MAP_MASK	0xf
+#define CMR_EVT_MAP_BITS	8
+#define CMR_EVT_PER_REG		4
+
+/* HMR register bit-field macros */
+#define HMR_CH_MAP_MASK		0xf
+#define HMR_CH_MAP_BITS		8
+#define HMR_CH_PER_REG		4
+
  /* HIPIR register bit-fields */
  #define INTC_HIPIR_NONE_HINT	0x80000000
  
+/**
+ * struct pruss_intc_hwirq_data - additional metadata associated with a PRU
+ * system event
+ * @evtsel: The event select index (AM18xx only)
+ * @channel: The PRU INTC channel that the system event should be mapped to
+ * @host: The PRU INTC host that the channel should be mapped to
+ */
+struct pruss_intc_hwirq_data {
+	u8 evtsel;
+	u8 channel;
+	u8 host;
+};
+
+/**
+ * struct pruss_intc_map_record - keeps track of actual mapping state
+ * @value: The currently mapped value (evtsel, channel or host)
+ * @ref_count: Keeps track of number of current users of this resource
+ */
+struct pruss_intc_map_record {
+	u8 value;
+	u8 ref_count;
+};
+
  /**
   * struct pruss_intc - PRUSS interrupt controller structure
+ * @hwirq_data: Table of additional mapping data received from device tree
+ *	or PRU firmware
+ * @evtsel: Tracks the current state of CFGCHIP3[3].PRUSSEVTSEL (AM18xx only)
+ * @event_channel: Tracks the current state of system event to channel mappings
+ * @channel_host: Tracks the current state of channel to host mappings
   * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
   * @base: base virtual address of INTC register space
   * @irqchip: irq chip for this interrupt controller
   * @domain: irq domain for this interrupt controller
   * @lock: mutex to serialize access to INTC
- * @host_mask: indicate which HOST IRQs are enabled
   * @shared_intr: bit-map denoting if the MPU host interrupt is shared
   * @invalid_intr: bit-map denoting if host interrupt is connected to MPU
+ * @has_evtsel: indicates that the chip has an event select mux
   */
  struct pruss_intc {
+	struct pruss_intc_hwirq_data hwirq_data[MAX_PRU_SYS_EVENTS];
+	struct pruss_intc_map_record evtsel;
+	struct pruss_intc_map_record event_channel[MAX_PRU_SYS_EVENTS];
+	struct pruss_intc_map_record channel_host[MAX_PRU_CHANNELS];
  	unsigned int irqs[MAX_NUM_HOST_IRQS];
  	void __iomem *base;
  	struct irq_chip *irqchip;
  	struct irq_domain *domain;
  	struct mutex lock; /* PRUSS INTC lock */
-	u32 host_mask;
  	u16 shared_intr;
  	u16 invalid_intr;
+	bool has_evtsel;
  };
  
  static inline u32 pruss_intc_read_reg(struct pruss_intc *intc, unsigned int reg)
@@ -107,6 +156,170 @@ static int pruss_intc_check_write(struct pruss_intc *intc, unsigned int reg,
  	return 0;
  }
  
+/**
+ * pruss_intc_map() - configure the PRUSS INTC
+ * @intc: pru intc pointer
+ * @hwirq: the system event number
+ *
+ * Configures the PRUSS INTC with the provided configuration from the one
+ * parsed in the xlate function. Any existing event to channel mappings or
+ * channel to host interrupt mappings are checked to make sure there are no
+ * conflicting configuration between both the PRU cores.
+ *
+ * Returns 0 on success, or a suitable error code otherwise
+ */
+static int pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
+{
+	struct device* dev = intc->irqchip->parent_device;
+	u32 val;
+	int idx, ret;
+	u8 evtsel, ch, host;
+
+	if (hwirq >= MAX_PRU_SYS_EVENTS)
+		return -EINVAL;
+
+	mutex_lock(&intc->lock);
+
+	evtsel = intc->hwirq_data[hwirq].evtsel;
+	ch = intc->hwirq_data[hwirq].channel;
+	host = intc->hwirq_data[hwirq].host;
+
+	if (intc->has_evtsel && intc->evtsel.ref_count > 0 &&
+	    intc->evtsel.value != evtsel) {
+		dev_err(dev, "event %lu (req. evtsel %d) already assigned to evtsel %d\n",
+			hwirq, evtsel, intc->evtsel.value);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	/* check if sysevent already assigned */
+	if (intc->event_channel[hwirq].ref_count > 0 &&
+	    intc->event_channel[hwirq].value != ch) {
+		dev_err(dev, "event %lu (req. channel %d) already assigned to channel %d\n",
+			hwirq, ch, intc->event_channel[hwirq].value);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	/* check if channel already assigned */
+	if (intc->channel_host[ch].ref_count > 0 &&
+	    intc->channel_host[ch].value != host) {
+		dev_err(dev, "channel %d (req. intr_no %d) already assigned to intr_no %d\n",
+			ch, host, intc->channel_host[ch].value);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (++intc->evtsel.ref_count == 1) {
+		intc->evtsel.value = evtsel;
+
+		/* TODO: need to implement CFGCHIP3[3].PRUSSEVTSEL */
+	}
+
+	if (++intc->event_channel[hwirq].ref_count == 1) {
+		intc->event_channel[hwirq].value = ch;
+
+		/*
+		 * configure channel map registers - each register holds map
+		 * info for 4 events, with each event occupying the lower nibble
+		 * in a register byte address in little-endian fashion
+		 */
+		idx = hwirq / CMR_EVT_PER_REG;
+
+		val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
+		val &= ~(CMR_EVT_MAP_MASK <<
+				((hwirq % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS));
+		val |= ch << ((hwirq % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS);
+		pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
+
+		dev_dbg(dev, "SYSEV%lu -> CH%d (CMR%d 0x%08x)\n", hwirq, ch,
+			idx, pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
+
+		/* clear and enable system event */
+		pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
+		pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
+	}
+
+	if (++intc->channel_host[ch].ref_count == 1) {
+		intc->channel_host[ch].value = host;
+
+		/*
+		 * set host map registers - each register holds map info for
+		 * 4 channels, with each channel occupying the lower nibble in
+		 * a register byte address in little-endian fashion
+		 */
+		idx = ch / HMR_CH_PER_REG;
+
+		val = pruss_intc_read_reg(intc, PRU_INTC_HMR(idx));
+		val &= ~(HMR_CH_MAP_MASK <<
+				((ch % HMR_CH_PER_REG) * HMR_CH_MAP_BITS));
+		val |= host << ((ch % HMR_CH_PER_REG) * HMR_CH_MAP_BITS);
+		pruss_intc_write_reg(intc, PRU_INTC_HMR(idx), val);
+
+		dev_dbg(dev, "CH%d -> HOST%d (HMR%d 0x%08x)\n", ch, host, idx,
+			pruss_intc_read_reg(intc, PRU_INTC_HMR(idx)));
+
+		/* enable host interrupts */
+		pruss_intc_write_reg(intc, PRU_INTC_HIEISR, host);
+	}
+
+	dev_info(dev, "mapped system_event = %lu channel = %d host = %d\n",
+		 hwirq, ch, host);
+
+	/* global interrupt enable */
+	pruss_intc_write_reg(intc, PRU_INTC_GER, 1);
+
+	mutex_unlock(&intc->lock);
+	return 0;
+
+unlock:
+	mutex_unlock(&intc->lock);
+	return ret;
+}
+
+/**
+ * pruss_intc_unmap() - unconfigure the PRUSS INTC
+ * @intc: pru intc pointer
+ * @hwirq: the system event number
+ *
+ * Undo whatever was done in pruss_intc_map() for a PRU core.
+ * Mappings are reference counted, so resources are only disabled when there
+ * are no longer any users.
+ */
+static void pruss_intc_unmap(struct pruss_intc *intc, unsigned long hwirq)
+{
+	struct device* dev = intc->irqchip->parent_device;
+	u8 ch, host;
+
+	if (hwirq >= MAX_PRU_SYS_EVENTS)
+		return;
+
+	mutex_lock(&intc->lock);
+
+	ch = intc->event_channel[hwirq].value;
+	host = intc->channel_host[ch].value;
+
+	if (--intc->channel_host[ch].ref_count == 0) {
+		/* disable host interrupts */
+		pruss_intc_write_reg(intc, PRU_INTC_HIDISR, host);
+	}
+
+	if (--intc->event_channel[hwirq].ref_count == 0) {
+		/* disable system events */
+		pruss_intc_write_reg(intc, PRU_INTC_EICR, hwirq);
+		/* clear any pending status */
+		pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
+	}
+
+	if (intc->has_evtsel)
+		intc->evtsel.ref_count--;
+
+	dev_info(dev, "unmapped system_event = %lu channel = %d host = %d\n",
+		 hwirq, ch, host);
+
+	mutex_unlock(&intc->lock);
+}
+
  static void pruss_intc_init(struct pruss_intc *intc)
  {
  	int i;
@@ -173,10 +386,53 @@ static void pruss_intc_irq_relres(struct irq_data *data)
  	module_put(THIS_MODULE);
  }
  
+static int
+pruss_intc_irq_domain_xlate(struct irq_domain *d, struct device_node *node,
+			    const u32 *intspec, unsigned int intsize,
+			    unsigned long *out_hwirq, unsigned int *out_type)
+{
+	struct pruss_intc *intc = d->host_data;
+	int num_cells = intc->has_evtsel ? 4 : 3;
+	u32 sys_event, channel, host;
+	u32 evtsel = 0;
+
+	if (WARN_ON(intsize != num_cells))
+		return -EINVAL;
+
+	sys_event = intspec[0];
+	if (sys_event >= MAX_PRU_SYS_EVENTS)
+		return -EINVAL;
+
+	if (intc->has_evtsel)
+		evtsel = intspec[1];
+
+	channel = intspec[intsize - 2];
+	if (channel >= MAX_PRU_CHANNELS)
+		return -EINVAL;
+
+	host = intspec[intsize - 1];
+	if (host >= MAX_PRU_HOST_INT)
+		return -EINVAL;
+
+	intc->hwirq_data[sys_event].evtsel = evtsel;
+	intc->hwirq_data[sys_event].channel = channel;
+	intc->hwirq_data[sys_event].host = host;
+
+	*out_hwirq = sys_event;
+	*out_type = IRQ_TYPE_NONE;
+
+	return 0;
+}
+
  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;
+	int err;
+
+	err = pruss_intc_map(intc, hw);
+	if (err < 0)
+		return err;
  
  	irq_set_chip_data(virq, intc);
  	irq_set_chip_and_handler(virq, intc->irqchip, handle_level_irq);
@@ -186,12 +442,20 @@ static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned int virq,
  
  static void pruss_intc_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
  {
+	struct pruss_intc *intc = d->host_data;
+	int i;
+
+	for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
+		if (intc->irqs[i] == virq)
+			break;
+
  	irq_set_chip_and_handler(virq, NULL, NULL);
  	irq_set_chip_data(virq, NULL);
+	pruss_intc_unmap(intc, i);
  }
  
  static const struct irq_domain_ops pruss_intc_irq_domain_ops = {
-	.xlate	= irq_domain_xlate_onecell,
+	.xlate	= pruss_intc_irq_domain_xlate,
  	.map	= pruss_intc_irq_domain_map,
  	.unmap	= pruss_intc_irq_domain_unmap,
  };
@@ -247,7 +511,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
  	struct pruss_intc *intc;
  	struct resource *res;
  	struct irq_chip *irqchip;
-	int i, irq, count;
+	int i, err, irq, count;
  	u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
  
  	intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
@@ -298,13 +562,20 @@ static int pruss_intc_probe(struct platform_device *pdev)
  		}
  	}
  
+	/* TODO: get intc->has_evtsel from device tree */
+
  	mutex_init(&intc->lock);
  
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
  	pruss_intc_init(intc);
  
  	irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
-	if (!irqchip)
-		return -ENOMEM;
+	if (!irqchip) {
+		err = -ENOMEM;
+		goto fail_alloc;
+	}
  
  	irqchip->irq_ack = pruss_intc_irq_ack;
  	irqchip->irq_mask = pruss_intc_irq_mask;
@@ -312,14 +583,17 @@ static int pruss_intc_probe(struct platform_device *pdev)
  	irqchip->irq_retrigger = pruss_intc_irq_retrigger;
  	irqchip->irq_request_resources = pruss_intc_irq_reqres;
  	irqchip->irq_release_resources = pruss_intc_irq_relres;
+	irqchip->parent_device = dev;
  	irqchip->name = dev_name(dev);
  	intc->irqchip = irqchip;
  
  	/* 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;
+	if (!intc->domain) {
+		err = -ENOMEM;
+		goto fail_alloc;
+	}
  
  	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
  		irq = platform_get_irq_byname(pdev, irq_names[i]);
@@ -330,6 +604,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
  
  			dev_err(dev->parent, "platform_get_irq_byname failed for %s : %d\n",
  				irq_names[i], irq);
+			err = irq;
  			goto fail_irq;
  		}
  
@@ -347,12 +622,18 @@ static int pruss_intc_probe(struct platform_device *pdev)
  							 NULL);
  	}
  	irq_domain_remove(intc->domain);
-	return irq;
+
+fail_alloc:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
+	return err;
  }
  
  static int pruss_intc_remove(struct platform_device *pdev)
  {
  	struct pruss_intc *intc = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
  	unsigned int hwirq;
  	int i;
  
@@ -369,6 +650,9 @@ static int pruss_intc_remove(struct platform_device *pdev)
  		irq_domain_remove(intc->domain);
  	}
  
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
  	return 0;
  }
Suman Anna July 16, 2019, 11:29 p.m. UTC | #3
Hi David,

On 7/10/19 10:10 PM, David Lechner wrote:
> On 7/7/19 10:52 PM, Suman Anna wrote:
>> The PRUSS INTC receives a number of system input interrupt source events
>> and supports individual control configuration and hardware
>> prioritization.
>> These 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.
>>
>> This mapping information is provided through the PRU firmware that is
>> loaded onto a PRU core/s or through the device tree node of the PRU
> 

Thanks for the thorough review and alternate solutions/suggestions.

> What will the device tree bindings for this look like?

They would be as in the below patch you already figured.

> 
> Looking back at Rob's comment on the initial series [1], I still think
> that increasing the #interrupt-cells sounds like a reasonable solution.
> 
> [1]: https://patchwork.kernel.org/patch/10697705/#22375155

So, there are couple of reasons why I did not use an extended
#interrupt-cells:

1. There is only one irq descriptor associated with each event, and the
usage of events is typically per application. And the descriptor mapping
is done once. We can have two different applications use the same event
with different mappings. So we want this programming done at
application's usage of PRU (so done when a consumer driver acquires a
PRU processor(s) which are treated as an exclusive resource). All the
different application properties that you saw in [1] are configured at
the time of acquiring a PRU and reset when they release a PRU.

2. The configuration is performed by Linux for all host interrupts and
channels, and this was primarily done to save the very limited IRAM
space for those needed by the PRUs. From firmware's point of view, this
was offloaded to the ARM OS driver/infrastructure, but in general it is
a design by contract between a PRU client driver and its firmware. Also,
the DT binding semantics using interrupts property and request_irq()
typically limits these to interrupts only being requested by MPU, and so
will leave out those needed by PRUs.

> 
> 
> 
>> application. The mapping is configured by the PRU remoteproc driver, and
>> is setup before the PRU core is started and cleaned up after the PRU core
>> is stopped. This event mapping configuration logic is optimized to
>> program
>> the Channel Map Registers (CMRx) and Host-Interrupt Map Registers (HMRx)
>> only when a new program is being loaded/started and simply disables the
>> same events and interrupt channels without zeroing out the corresponding
>> map registers when stopping a PRU.
>>
>> Add two helper functions: pruss_intc_configure() &
>> pruss_intc_unconfigure()
>> that the PRU remoteproc driver can use to configure the PRUSS INTC.
>>
>> 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>
>> ---
>>   drivers/irqchip/irq-pruss-intc.c       | 258 ++++++++++++++++++++++++-
>>   include/linux/irqchip/irq-pruss-intc.h |  33 ++++
>>   2 files changed, 289 insertions(+), 2 deletions(-)
>>   create mode 100644 include/linux/irqchip/irq-pruss-intc.h
>>
>> diff --git a/drivers/irqchip/irq-pruss-intc.c
>> b/drivers/irqchip/irq-pruss-intc.c
>> index 142d01b434e0..8118c2a2ac43 100644
>> --- a/drivers/irqchip/irq-pruss-intc.c
>> +++ b/drivers/irqchip/irq-pruss-intc.c
>> @@ -9,6 +9,7 @@
>>     #include <linux/irq.h>
>>   #include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqchip/irq-pruss-intc.h>
>>   #include <linux/irqdomain.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> @@ -24,8 +25,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
>> +/* maximum number of host interrupts */
>> +#define MAX_PRU_HOST_INT    10
>>     /* PRU_ICSS_INTC registers */
>>   #define PRU_INTC_REVID        0x0000
>> @@ -57,15 +58,29 @@
>>   #define PRU_INTC_HINLR(x)    (0x1100 + (x) * 4)
>>   #define PRU_INTC_HIER        0x1500
>>   +/* CMR register bit-field macros */
>> +#define CMR_EVT_MAP_MASK    0xf
>> +#define CMR_EVT_MAP_BITS    8
>> +#define CMR_EVT_PER_REG        4
>> +
>> +/* HMR register bit-field macros */
>> +#define HMR_CH_MAP_MASK        0xf
>> +#define HMR_CH_MAP_BITS        8
>> +#define HMR_CH_PER_REG        4
>> +
>>   /* HIPIR register bit-fields */
>>   #define INTC_HIPIR_NONE_HINT    0x80000000
>>   +/* use -1 to mark unassigned events and channels */
>> +#define FREE            -1
> 
> It could be helpful to have this macro in the public header.

Yes, I can rename it and move it, and I can reuse it in the parsing
logic within the PRU remoteproc driver as well.

> 
>> +
>>   /**
>>    * struct pruss_intc - PRUSS interrupt controller structure
>>    * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>>    * @base: base virtual address of INTC register space
>>    * @irqchip: irq chip for this interrupt controller
>>    * @domain: irq domain for this interrupt controller
>> + * @config_map: stored INTC configuration mapping data
>>    * @lock: mutex to serialize access to INTC
>>    * @host_mask: indicate which HOST IRQs are enabled
>>    * @shared_intr: bit-map denoting if the MPU host interrupt is shared
>> @@ -76,6 +91,7 @@ struct pruss_intc {
>>       void __iomem *base;
>>       struct irq_chip *irqchip;
>>       struct irq_domain *domain;
>> +    struct pruss_intc_config config_map;
>>       struct mutex lock; /* PRUSS INTC lock */
>>       u32 host_mask;
>>       u16 shared_intr;
>> @@ -107,6 +123,238 @@ static int pruss_intc_check_write(struct
>> pruss_intc *intc, unsigned int reg,
>>       return 0;
>>   }
>>   +static struct pruss_intc *to_pruss_intc(struct device *pru_dev)
>> +{
>> +    struct device_node *np;
>> +    struct platform_device *pdev;
>> +    struct device *pruss_dev = pru_dev->parent;
>> +    struct pruss_intc *intc = ERR_PTR(-ENODEV);
>> +
>> +    np = of_get_child_by_name(pruss_dev->of_node,
>> "interrupt-controller");
>> +    if (!np) {
>> +        dev_err(pruss_dev, "pruss does not have an
>> interrupt-controller node\n");
>> +        return intc;
>> +    }
>> +
>> +    pdev = of_find_device_by_node(np);
>> +    if (!pdev) {
>> +        dev_err(pruss_dev, "no associated platform device\n");
>> +        goto out;
>> +    }
>> +
>> +    intc = platform_get_drvdata(pdev);
>> +    if (!intc) {
>> +        dev_err(pruss_dev, "pruss intc device probe failed?\n");
>> +        intc = ERR_PTR(-EINVAL);
>> +    }
>> +
>> +out:
>> +    of_node_put(np);
>> +    return intc;
>> +}
>> +
>> +/**
>> + * pruss_intc_configure() - configure the PRUSS INTC
>> + * @dev: pru device pointer
>> + * @intc_config: PRU core-specific INTC configuration
>> + *
>> + * Configures the PRUSS INTC with the provided configuration from
>> + * a PRU core. Any existing event to channel mappings or channel to
>> + * host interrupt mappings are checked to make sure there are no
>> + * conflicting configuration between both the PRU cores. The function
>> + * is intended to be used only by the PRU remoteproc driver.
>> + *
>> + * Returns 0 on success, or a suitable error code otherwise
>> + */
>> +int pruss_intc_configure(struct device *dev,
> 
> It seems like this would be easier to use if it took an IRQ number
> or struct irq_data * as a parameter instead of struct device *. My
> line of thinking is that callers of this function will already be
> calling some variant of request_irq() so they will already have
> this info. It would cut out the pointer acrobatics in to_pruss_intc.

These API are actually not seen by PRU client drivers, but is only
limited to the PRU remoteproc driver. The INTC configuration is managed
per PRU core and in sync with the life-cycle of the PRU load/start and stop.

As I mentioned above, we need to manage the configuration for events
generating interrupts to non Linux ARM host as well.

> 
> 
>> +             struct pruss_intc_config *intc_config)
>> +{
>> +    struct pruss_intc *intc;
>> +    int i, idx, ret;
>> +    s8 ch, host;
>> +    u64 sysevt_mask = 0;
>> +    u32 ch_mask = 0;
>> +    u32 host_mask = 0;
>> +    u32 val;
>> +
>> +    intc = to_pruss_intc(dev);
>> +    if (IS_ERR(intc))
>> +        return PTR_ERR(intc);
>> +
>> +    mutex_lock(&intc->lock);
>> +
>> +    /*
>> +     * configure channel map registers - each register holds map info
>> +     * for 4 events, with each event occupying the lower nibble in
>> +     * a register byte address in little-endian fashion
>> +     */
>> +    for (i = 0; i < ARRAY_SIZE(intc_config->sysev_to_ch); i++) {
>> +        ch = intc_config->sysev_to_ch[i];
>> +        if (ch < 0)
>> +            continue;
>> +
>> +        /* check if sysevent already assigned */
>> +        if (intc->config_map.sysev_to_ch[i] != FREE) {
>> +            dev_err(dev, "event %d (req. channel %d) already assigned
>> to channel %d\n",
>> +                i, ch, intc->config_map.sysev_to_ch[i]);
>> +            ret = -EEXIST;
>> +            goto unlock;
> 
> If we fail here, shouldn't we unwind any previous mappings made?
> Otherwise, if we try to map the same event again, it will show as
> in use, even though it is not in use.

Yeah, I will fix up the unwind logic. I intended for the callers to
invoke the unconfigure upon failures, but even that has some unneeded
operations, so it is better to unwind the operations here for a cleaner
style.

> 
>> +        }
>> +
>> +        intc->config_map.sysev_to_ch[i] = ch;
>> +
>> +        idx = i / CMR_EVT_PER_REG;
>> +        val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
>> +        val &= ~(CMR_EVT_MAP_MASK <<
>> +             ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS));
>> +        val |= ch << ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS);
>> +        pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
>> +        sysevt_mask |= BIT_ULL(i);
>> +        ch_mask |= BIT(ch);
>> +
>> +        dev_dbg(dev, "SYSEV%d -> CH%d (CMR%d 0x%08x)\n", i, ch, idx,
>> +            pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
>> +    }
>> +
>> +    /*
>> +     * set host map registers - each register holds map info for
>> +     * 4 channels, with each channel occupying the lower nibble in
>> +     * a register byte address in little-endian fashion
>> +     */
>> +    for (i = 0; i < ARRAY_SIZE(intc_config->ch_to_host); i++) {
>> +        host = intc_config->ch_to_host[i];
>> +        if (host < 0)
>> +            continue;
>> +
>> +        /* check if channel already assigned */
>> +        if (intc->config_map.ch_to_host[i] != FREE) {
>> +            dev_err(dev, "channel %d (req. intr_no %d) already
>> assigned to intr_no %d\n",
>> +                i, host, intc->config_map.ch_to_host[i]);
>> +            ret = -EEXIST;
>> +            goto unlock;
> 
> Same comment about unwinding here and below.

Yep, will fix this up as well in the next version.

> 
>> +        }
>> +
>> +        /* check if host intr is already in use by other PRU */
> 
> It seems like there would be use cases where someone might want to map
> multiple PRU system events, and therefore multiple channels, to a single
> host interrupt.

Yes, that is in general supported but for a given PRU. The idea here was
to partition the host events separately between two PRUs and this is
done to simplify the life-cycle per host event and their mappings
between two different PRUs potentially running two different unrelated
co-operative applications.

> 
>> +        if (intc->host_mask & (1U << host)) {
>> +            dev_err(dev, "%s: host intr %d already in use\n",
>> +                __func__, host);
>> +            ret = -EEXIST;
>> +            goto unlock;
>> +        }
>> +
> 
> --snip--
> 
>> diff --git a/include/linux/irqchip/irq-pruss-intc.h
>> b/include/linux/irqchip/irq-pruss-intc.h
>> new file mode 100644
>> index 000000000000..f1f1bb150100
>> --- /dev/null
>> +++ b/include/linux/irqchip/irq-pruss-intc.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * PRU-ICSS sub-system private interfaces
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + *    Suman Anna <s-anna@ti.com>
>> + */
>> +
>> +#ifndef __LINUX_IRQ_PRUSS_INTC_H
>> +#define __LINUX_IRQ_PRUSS_INTC_H
>> +
>> +/* maximum number of system events */
>> +#define MAX_PRU_SYS_EVENTS    64
>> +
>> +/* maximum number of interrupt channels */
>> +#define MAX_PRU_CHANNELS    10
>> +
>> +/**
>> + * struct pruss_intc_config - INTC configuration info
>> + * @sysev_to_ch: system events to channel mapping information
>> + * @ch_to_host: interrupt channel to host interrupt information
>> + */
>> +struct pruss_intc_config {
>> +    s8 sysev_to_ch[MAX_PRU_SYS_EVENTS];
>> +    s8 ch_to_host[MAX_PRU_CHANNELS];
>> +};
>> +
>> +int pruss_intc_configure(struct device *dev,
>> +             struct pruss_intc_config *intc_config);
>> +int pruss_intc_unconfigure(struct device *dev,
>> +               struct pruss_intc_config *intc_config);
>> +
>> +#endif    /* __LINUX_IRQ_PRUSS_INTC_H */
>>
> 
> FYI, on AM18xx, events 0 to 31 can be muxed via CFGCHIP3[3].PRUSSEVTSEL
> so an additional bit of information will be needed in this struct for
> the mux selection. I don't see a probably with adding that later though.

Yeah, there are different input pinmux'ing options controlling different
number of input events on different SoCs. On AM18xx it is a SoC-level
CHIPCFG register, and on other SoCs, it is a PRUSS CFG register
(Standard mode vs MII mode) both of which are registers outside of the
INTC module. I see these again as an application-level configuration,
and this is what the last bullet item in the feature list in my
cover-letter is about.

I did think about adding a separate property to INTC node to configure a
default value at INTC probe time, and then allow it to be overwritten as
per a PRU application need. The latter is going to be needed anyway, so
I dropped the idea of a default configuration, and leave it at POR values.

regards
Suman
David Lechner July 17, 2019, 5:57 p.m. UTC | #4
On 7/16/19 6:29 PM, Suman Anna wrote:
> Hi David,
> 
> On 7/10/19 10:10 PM, David Lechner wrote:
>> On 7/7/19 10:52 PM, Suman Anna wrote:
>>> The PRUSS INTC receives a number of system input interrupt source events
>>> and supports individual control configuration and hardware
>>> prioritization.
>>> These 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.
>>>
>>> This mapping information is provided through the PRU firmware that is
>>> loaded onto a PRU core/s or through the device tree node of the PRU
>>
> 
> Thanks for the thorough review and alternate solutions/suggestions.
> 
>> What will the device tree bindings for this look like?
> 
> They would be as in the below patch you already figured.

Ah, makes sense now: the mapping is defined in the remoteproc node
rather than in the interrupt controller node.

> 
>>
>> Looking back at Rob's comment on the initial series [1], I still think
>> that increasing the #interrupt-cells sounds like a reasonable solution.
>>
>> [1]: https://patchwork.kernel.org/patch/10697705/#22375155
> 
> So, there are couple of reasons why I did not use an extended
> #interrupt-cells:
> 
> 1. There is only one irq descriptor associated with each event, and the
> usage of events is typically per application. And the descriptor mapping
> is done once. We can have two different applications use the same event
> with different mappings. So we want this programming done at
> application's usage of PRU (so done when a consumer driver acquires a
> PRU processor(s) which are treated as an exclusive resource). All the
> different application properties that you saw in [1] are configured at
> the time of acquiring a PRU and reset when they release a PRU.
> 
> 2. The configuration is performed by Linux for all host interrupts and
> channels, and this was primarily done to save the very limited IRAM
> space for those needed by the PRUs. From firmware's point of view, this
> was offloaded to the ARM OS driver/infrastructure, but in general it is
> a design by contract between a PRU client driver and its firmware. Also,
> the DT binding semantics using interrupts property and request_irq()
> typically limits these to interrupts only being requested by MPU, and so
> will leave out those needed by PRUs.
> 

Hmm... case 1. is a tricky one indeed. If there are going to be times where
an event requires multiple mappings, I agree that this doesn't seem to fit
into any existing device tree bindings.
Suman Anna July 17, 2019, 7:04 p.m. UTC | #5
On 7/17/19 12:57 PM, David Lechner wrote:
> On 7/16/19 6:29 PM, Suman Anna wrote:
>> Hi David,
>>
>> On 7/10/19 10:10 PM, David Lechner wrote:
>>> On 7/7/19 10:52 PM, Suman Anna wrote:
>>>> The PRUSS INTC receives a number of system input interrupt source
>>>> events
>>>> and supports individual control configuration and hardware
>>>> prioritization.
>>>> These 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.
>>>>
>>>> This mapping information is provided through the PRU firmware that is
>>>> loaded onto a PRU core/s or through the device tree node of the PRU
>>>
>>
>> Thanks for the thorough review and alternate solutions/suggestions.
>>
>>> What will the device tree bindings for this look like?
>>
>> They would be as in the below patch you already figured.
> 
> Ah, makes sense now: the mapping is defined in the remoteproc node
> rather than in the interrupt controller node.

Actually in the PRU consumer/application node, but the client driver
need not deal with invoking any special API. The functions are called
transparently by the PRU remoteproc driver when the PRU client driver
acquires a PRU. The 4th cell was used to identify the PRU from the list
of prus in the client node.

regards
Suman

> 
>>
>>>
>>> Looking back at Rob's comment on the initial series [1], I still think
>>> that increasing the #interrupt-cells sounds like a reasonable solution.
>>>
>>> [1]: https://patchwork.kernel.org/patch/10697705/#22375155
>>
>> So, there are couple of reasons why I did not use an extended
>> #interrupt-cells:
>>
>> 1. There is only one irq descriptor associated with each event, and the
>> usage of events is typically per application. And the descriptor mapping
>> is done once. We can have two different applications use the same event
>> with different mappings. So we want this programming done at
>> application's usage of PRU (so done when a consumer driver acquires a
>> PRU processor(s) which are treated as an exclusive resource). All the
>> different application properties that you saw in [1] are configured at
>> the time of acquiring a PRU and reset when they release a PRU.
>>
>> 2. The configuration is performed by Linux for all host interrupts and
>> channels, and this was primarily done to save the very limited IRAM
>> space for those needed by the PRUs. From firmware's point of view, this
>> was offloaded to the ARM OS driver/infrastructure, but in general it is
>> a design by contract between a PRU client driver and its firmware. Also,
>> the DT binding semantics using interrupts property and request_irq()
>> typically limits these to interrupts only being requested by MPU, and so
>> will leave out those needed by PRUs.
>>
> 
> Hmm... case 1. is a tricky one indeed. If there are going to be times where
> an event requires multiple mappings, I agree that this doesn't seem to fit
> into any existing device tree bindings.
> 
>
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 142d01b434e0..8118c2a2ac43 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -9,6 +9,7 @@ 
 
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/irqchip/irq-pruss-intc.h>
 #include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -24,8 +25,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
+/* maximum number of host interrupts */
+#define MAX_PRU_HOST_INT	10
 
 /* PRU_ICSS_INTC registers */
 #define PRU_INTC_REVID		0x0000
@@ -57,15 +58,29 @@ 
 #define PRU_INTC_HINLR(x)	(0x1100 + (x) * 4)
 #define PRU_INTC_HIER		0x1500
 
+/* CMR register bit-field macros */
+#define CMR_EVT_MAP_MASK	0xf
+#define CMR_EVT_MAP_BITS	8
+#define CMR_EVT_PER_REG		4
+
+/* HMR register bit-field macros */
+#define HMR_CH_MAP_MASK		0xf
+#define HMR_CH_MAP_BITS		8
+#define HMR_CH_PER_REG		4
+
 /* HIPIR register bit-fields */
 #define INTC_HIPIR_NONE_HINT	0x80000000
 
+/* use -1 to mark unassigned events and channels */
+#define FREE			-1
+
 /**
  * struct pruss_intc - PRUSS interrupt controller structure
  * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
  * @base: base virtual address of INTC register space
  * @irqchip: irq chip for this interrupt controller
  * @domain: irq domain for this interrupt controller
+ * @config_map: stored INTC configuration mapping data
  * @lock: mutex to serialize access to INTC
  * @host_mask: indicate which HOST IRQs are enabled
  * @shared_intr: bit-map denoting if the MPU host interrupt is shared
@@ -76,6 +91,7 @@  struct pruss_intc {
 	void __iomem *base;
 	struct irq_chip *irqchip;
 	struct irq_domain *domain;
+	struct pruss_intc_config config_map;
 	struct mutex lock; /* PRUSS INTC lock */
 	u32 host_mask;
 	u16 shared_intr;
@@ -107,6 +123,238 @@  static int pruss_intc_check_write(struct pruss_intc *intc, unsigned int reg,
 	return 0;
 }
 
+static struct pruss_intc *to_pruss_intc(struct device *pru_dev)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+	struct device *pruss_dev = pru_dev->parent;
+	struct pruss_intc *intc = ERR_PTR(-ENODEV);
+
+	np = of_get_child_by_name(pruss_dev->of_node, "interrupt-controller");
+	if (!np) {
+		dev_err(pruss_dev, "pruss does not have an interrupt-controller node\n");
+		return intc;
+	}
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		dev_err(pruss_dev, "no associated platform device\n");
+		goto out;
+	}
+
+	intc = platform_get_drvdata(pdev);
+	if (!intc) {
+		dev_err(pruss_dev, "pruss intc device probe failed?\n");
+		intc = ERR_PTR(-EINVAL);
+	}
+
+out:
+	of_node_put(np);
+	return intc;
+}
+
+/**
+ * pruss_intc_configure() - configure the PRUSS INTC
+ * @dev: pru device pointer
+ * @intc_config: PRU core-specific INTC configuration
+ *
+ * Configures the PRUSS INTC with the provided configuration from
+ * a PRU core. Any existing event to channel mappings or channel to
+ * host interrupt mappings are checked to make sure there are no
+ * conflicting configuration between both the PRU cores. The function
+ * is intended to be used only by the PRU remoteproc driver.
+ *
+ * Returns 0 on success, or a suitable error code otherwise
+ */
+int pruss_intc_configure(struct device *dev,
+			 struct pruss_intc_config *intc_config)
+{
+	struct pruss_intc *intc;
+	int i, idx, ret;
+	s8 ch, host;
+	u64 sysevt_mask = 0;
+	u32 ch_mask = 0;
+	u32 host_mask = 0;
+	u32 val;
+
+	intc = to_pruss_intc(dev);
+	if (IS_ERR(intc))
+		return PTR_ERR(intc);
+
+	mutex_lock(&intc->lock);
+
+	/*
+	 * configure channel map registers - each register holds map info
+	 * for 4 events, with each event occupying the lower nibble in
+	 * a register byte address in little-endian fashion
+	 */
+	for (i = 0; i < ARRAY_SIZE(intc_config->sysev_to_ch); i++) {
+		ch = intc_config->sysev_to_ch[i];
+		if (ch < 0)
+			continue;
+
+		/* check if sysevent already assigned */
+		if (intc->config_map.sysev_to_ch[i] != FREE) {
+			dev_err(dev, "event %d (req. channel %d) already assigned to channel %d\n",
+				i, ch, intc->config_map.sysev_to_ch[i]);
+			ret = -EEXIST;
+			goto unlock;
+		}
+
+		intc->config_map.sysev_to_ch[i] = ch;
+
+		idx = i / CMR_EVT_PER_REG;
+		val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
+		val &= ~(CMR_EVT_MAP_MASK <<
+			 ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS));
+		val |= ch << ((i % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS);
+		pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
+		sysevt_mask |= BIT_ULL(i);
+		ch_mask |= BIT(ch);
+
+		dev_dbg(dev, "SYSEV%d -> CH%d (CMR%d 0x%08x)\n", i, ch, idx,
+			pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
+	}
+
+	/*
+	 * set host map registers - each register holds map info for
+	 * 4 channels, with each channel occupying the lower nibble in
+	 * a register byte address in little-endian fashion
+	 */
+	for (i = 0; i < ARRAY_SIZE(intc_config->ch_to_host); i++) {
+		host = intc_config->ch_to_host[i];
+		if (host < 0)
+			continue;
+
+		/* check if channel already assigned */
+		if (intc->config_map.ch_to_host[i] != FREE) {
+			dev_err(dev, "channel %d (req. intr_no %d) already assigned to intr_no %d\n",
+				i, host, intc->config_map.ch_to_host[i]);
+			ret = -EEXIST;
+			goto unlock;
+		}
+
+		/* check if host intr is already in use by other PRU */
+		if (intc->host_mask & (1U << host)) {
+			dev_err(dev, "%s: host intr %d already in use\n",
+				__func__, host);
+			ret = -EEXIST;
+			goto unlock;
+		}
+
+		intc->config_map.ch_to_host[i] = host;
+
+		idx = i / HMR_CH_PER_REG;
+
+		val = pruss_intc_read_reg(intc, PRU_INTC_HMR(idx));
+		val &= ~(HMR_CH_MAP_MASK <<
+			 ((i % HMR_CH_PER_REG) * HMR_CH_MAP_BITS));
+		val |= host << ((i % HMR_CH_PER_REG) * HMR_CH_MAP_BITS);
+		pruss_intc_write_reg(intc, PRU_INTC_HMR(idx), val);
+
+		ch_mask |= BIT(i);
+		host_mask |= BIT(host);
+
+		dev_dbg(dev, "CH%d -> HOST%d (HMR%d 0x%08x)\n", i, host, idx,
+			pruss_intc_read_reg(intc, PRU_INTC_HMR(idx)));
+	}
+
+	dev_info(dev, "configured system_events = 0x%016llx intr_channels = 0x%08x host_intr = 0x%08x\n",
+		 sysevt_mask, ch_mask, host_mask);
+
+	/* enable system events, writing 0 has no-effect */
+	pruss_intc_write_reg(intc, PRU_INTC_ESR0, lower_32_bits(sysevt_mask));
+	pruss_intc_write_reg(intc, PRU_INTC_SECR0, lower_32_bits(sysevt_mask));
+	pruss_intc_write_reg(intc, PRU_INTC_ESR1, upper_32_bits(sysevt_mask));
+	pruss_intc_write_reg(intc, PRU_INTC_SECR1, upper_32_bits(sysevt_mask));
+
+	/* enable host interrupts */
+	for (i = 0; i < MAX_PRU_HOST_INT; i++) {
+		if (host_mask & BIT(i))
+			pruss_intc_write_reg(intc, PRU_INTC_HIEISR, i);
+	}
+
+	/* global interrupt enable */
+	pruss_intc_write_reg(intc, PRU_INTC_GER, 1);
+
+	intc->host_mask |= host_mask;
+
+	mutex_unlock(&intc->lock);
+	return 0;
+
+unlock:
+	mutex_unlock(&intc->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pruss_intc_configure);
+
+/**
+ * pruss_intc_unconfigure() - unconfigure the PRUSS INTC
+ * @dev: pru device pointer
+ * @intc_config: PRU core specific INTC configuration
+ *
+ * Undo whatever was done in pruss_intc_configure() for a PRU core.
+ * It should be sufficient to just mark the resources free in the
+ * global map and disable the host interrupts and sysevents.
+ */
+int pruss_intc_unconfigure(struct device *dev,
+			   struct pruss_intc_config *intc_config)
+{
+	struct pruss_intc *intc;
+	int i;
+	s8 ch, host;
+	u64 sysevt_mask = 0;
+	u32 host_mask = 0;
+
+	intc = to_pruss_intc(dev);
+	if (IS_ERR(intc))
+		return PTR_ERR(intc);
+
+	mutex_lock(&intc->lock);
+
+	for (i = 0; i < ARRAY_SIZE(intc_config->sysev_to_ch); i++) {
+		ch = intc_config->sysev_to_ch[i];
+		if (ch < 0)
+			continue;
+
+		/* mark sysevent free in global map */
+		intc->config_map.sysev_to_ch[i] = FREE;
+		sysevt_mask |= BIT_ULL(i);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(intc_config->ch_to_host); i++) {
+		host = intc_config->ch_to_host[i];
+		if (host < 0)
+			continue;
+
+		/* mark channel free in global map */
+		intc->config_map.ch_to_host[i] = FREE;
+		host_mask |= BIT(host);
+	}
+
+	dev_info(dev, "unconfigured system_events = 0x%016llx host_intr = 0x%08x\n",
+		 sysevt_mask, host_mask);
+
+	/* disable system events, writing 0 has no-effect */
+	pruss_intc_write_reg(intc, PRU_INTC_ECR0, lower_32_bits(sysevt_mask));
+	pruss_intc_write_reg(intc, PRU_INTC_ECR1, upper_32_bits(sysevt_mask));
+	/* clear any pending status */
+	pruss_intc_write_reg(intc, PRU_INTC_SECR0, lower_32_bits(sysevt_mask));
+	pruss_intc_write_reg(intc, PRU_INTC_SECR1, upper_32_bits(sysevt_mask));
+
+	/* disable host interrupts */
+	for (i = 0; i < MAX_PRU_HOST_INT; i++) {
+		if (host_mask & BIT(i))
+			pruss_intc_write_reg(intc, PRU_INTC_HIDISR, i);
+	}
+
+	intc->host_mask &= ~host_mask;
+	mutex_unlock(&intc->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pruss_intc_unconfigure);
+
 static void pruss_intc_init(struct pruss_intc *intc)
 {
 	int i;
@@ -300,6 +548,12 @@  static int pruss_intc_probe(struct platform_device *pdev)
 
 	mutex_init(&intc->lock);
 
+	for (i = 0; i < ARRAY_SIZE(intc->config_map.sysev_to_ch); i++)
+		intc->config_map.sysev_to_ch[i] = FREE;
+
+	for (i = 0; i < ARRAY_SIZE(intc->config_map.ch_to_host); i++)
+		intc->config_map.ch_to_host[i] = FREE;
+
 	pruss_intc_init(intc);
 
 	irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
diff --git a/include/linux/irqchip/irq-pruss-intc.h b/include/linux/irqchip/irq-pruss-intc.h
new file mode 100644
index 000000000000..f1f1bb150100
--- /dev/null
+++ b/include/linux/irqchip/irq-pruss-intc.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PRU-ICSS sub-system private interfaces
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Suman Anna <s-anna@ti.com>
+ */
+
+#ifndef __LINUX_IRQ_PRUSS_INTC_H
+#define __LINUX_IRQ_PRUSS_INTC_H
+
+/* maximum number of system events */
+#define MAX_PRU_SYS_EVENTS	64
+
+/* maximum number of interrupt channels */
+#define MAX_PRU_CHANNELS	10
+
+/**
+ * struct pruss_intc_config - INTC configuration info
+ * @sysev_to_ch: system events to channel mapping information
+ * @ch_to_host: interrupt channel to host interrupt information
+ */
+struct pruss_intc_config {
+	s8 sysev_to_ch[MAX_PRU_SYS_EVENTS];
+	s8 ch_to_host[MAX_PRU_CHANNELS];
+};
+
+int pruss_intc_configure(struct device *dev,
+			 struct pruss_intc_config *intc_config);
+int pruss_intc_unconfigure(struct device *dev,
+			   struct pruss_intc_config *intc_config);
+
+#endif	/* __LINUX_IRQ_PRUSS_INTC_H */