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

[Suman Anna]

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@xxxxxx>
>> Signed-off-by: Andrew F. Davis <afd@xxxxxx>
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>> ---
>> Â 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@xxxxxx>
>> + */
>> +
>> +#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