From: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
To: Suman Anna <s-anna@ti.com>
Cc: tglx@linutronix.de, jason@lakedaemon.net,
Marc Zyngier <maz@kernel.org>,
robh+dt@kernel.org, Lee Jones <lee.jones@linaro.org>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
david@lechnology.com, "Mills, William" <wmills@ti.com>
Subject: Re: [PATCHv3 6/6] irqchip/irq-pruss-intc: Add event mapping support
Date: Sun, 5 Jul 2020 15:39:36 +0200 [thread overview]
Message-ID: <CAMxfBF52X+USBYt4MGs_z3T+SciqoJrGW61ZC39P3Xo8rjqPsg@mail.gmail.com> (raw)
In-Reply-To: <46b98790-a393-d66c-1e42-bf2055fc24ff@ti.com>
Hi Suman,
On Thu, 2 Jul 2020 at 18:25, Suman Anna <s-anna@ti.com> wrote:
>
> Hi Greg,
>
> On 7/2/20 9:17 AM, Grzegorz Jaszczyk 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
> > application. The mapping configuration is triggered 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 programs the Channel Map Registers (CMRx) and
> > Host-Interrupt Map Registers (HMRx) only when a new program is being
> > loaded/started and the same events and interrupt channels are reset to
> > zero when stopping a PRU.
> >
> > 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.
> >
> > 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;
> > }
> >
> > Suggested-by: David Lechner <david@lechnology.com>
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> > ---
> > v3:
> > - This patch replaces https://patchwork.kernel.org/patch/11069753/
> > according to received feedback. Instead of exporting custom functions
> > from interrupt driver, the xlate and irq domain map is used for
> > interrupt parsing and mapping.
>
> Thanks for reworking this. Only have couple of very minor comments below.
>
> > ---
> > drivers/irqchip/irq-pruss-intc.c | 265 ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 264 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
> > index 362aa01..cf40a97 100644
> > --- a/drivers/irqchip/irq-pruss-intc.c
> > +++ b/drivers/irqchip/irq-pruss-intc.c
> > @@ -51,14 +51,42 @@
> > #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
> >
> > +#define MAX_PRU_SYS_EVENTS 160
> > +#define MAX_PRU_CHANNELS 20
> > +
> > +/**
> > + * struct pruss_intc_hwirq_data - additional metadata associated with a PRU
> > + * system event
> > + * @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 channel;
> > + u8 host;
> > +};
> > +
> > +/**
> > + * struct pruss_intc_map_record - keeps track of actual mapping state
> > + * @value: The currently mapped value (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_match_data - match data to handle SoC variations
> > * @num_system_events: number of input system events handled by the PRUSS INTC
> > @@ -71,18 +99,29 @@ struct pruss_intc_match_data {
> >
> > /**
> > * struct pruss_intc - PRUSS interrupt controller structure
> > + * @hwirq_data: table of additional mapping data received from device tree
> > + * or PRU firmware
> > + * @event_channel: current state of system event to channel mappings
> > + * @channel_host: current state of channel to host mappings
> > * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
> > * @base: base virtual address of INTC register space
> > * @domain: irq domain for this interrupt controller
> > * @soc_config: cached PRUSS INTC IP configuration data
> > + * @lock: mutex to serialize access to INTC
> > + * @dev: PRUSS INTC device pointer
> > * @shared_intr: bit-map denoting if the MPU host interrupt is shared
> > * @invalid_intr: bit-map denoting if host interrupt is not connected to MPU
> > */
> > struct pruss_intc {
> > + struct pruss_intc_hwirq_data hwirq_data[MAX_PRU_SYS_EVENTS];
> > + 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_domain *domain;
> > const struct pruss_intc_match_data *soc_config;
> > + struct mutex lock; /* PRUSS INTC lock */
> > + struct device *dev;
> > u16 shared_intr;
> > u16 invalid_intr;
> > };
> > @@ -98,6 +137,165 @@ static inline void pruss_intc_write_reg(struct pruss_intc *intc,
> > writel_relaxed(val, intc->base + reg);
> > }
> >
> > +static void pruss_intc_update_cmr(struct pruss_intc *intc, int evt, s8 ch)
> > +{
> > + u32 idx, offset, val;
> > +
> > + idx = evt / CMR_EVT_PER_REG;
> > + offset = (evt % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS;
> > +
> > + val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
> > + val &= ~(CMR_EVT_MAP_MASK << offset);
> > + val |= ch << offset;
> > + pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
> > +
> > + dev_dbg(intc->dev, "SYSEV%u -> CH%d (CMR%d 0x%08x)\n", evt, ch,
> > + idx, pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
> > +}
> > +
> > +static void pruss_intc_update_hmr(struct pruss_intc *intc, int ch, s8 host)
> > +{
> > + u32 idx, offset, val;
> > +
> > + idx = ch / HMR_CH_PER_REG;
> > + offset = (ch % HMR_CH_PER_REG) * HMR_CH_MAP_BITS;
> > +
> > + val = pruss_intc_read_reg(intc, PRU_INTC_HMR(idx));
> > + val &= ~(HMR_CH_MAP_MASK << offset);
> > + val |= host << offset;
> > + pruss_intc_write_reg(intc, PRU_INTC_HMR(idx), val);
> > +
> > + dev_dbg(intc->dev, "CH%d -> HOST%d (HMR%d 0x%08x)\n", ch, host, idx,
> > + pruss_intc_read_reg(intc, PRU_INTC_HMR(idx)));
> > +}
> > +
> > +/**
> > + * pruss_intc_map() - configure the PRUSS INTC
> > + * @intc: PRUSS interrupt controller 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->dev;
> > + int ret = 0;
> > + u8 ch, host, reg_idx;
> > + u32 val;
> > +
> > + if (hwirq >= intc->soc_config->num_system_events)
> > + return -EINVAL;
> > +
> > + mutex_lock(&intc->lock);
> > +
> > + ch = intc->hwirq_data[hwirq].channel;
> > + host = intc->hwirq_data[hwirq].host;
> > +
> > + /* 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. host %d) already assigned to host %d\n",
> > + ch, host, intc->channel_host[ch].value);
> > + ret = -EBUSY;
> > + goto unlock;
> > + }
> > +
> > + if (++intc->event_channel[hwirq].ref_count == 1) {
> > + intc->event_channel[hwirq].value = ch;
> > +
> > + pruss_intc_update_cmr(intc, hwirq, ch);
> > +
> > + reg_idx = hwirq / 32;
> > + val = BIT(hwirq % 32);
> > +
> > + /* clear and enable system event */
> > + pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
> > + pruss_intc_write_reg(intc, PRU_INTC_SECR(reg_idx), val);
> > + }
> > +
> > + if (++intc->channel_host[ch].ref_count == 1) {
> > + intc->channel_host[ch].value = host;
> > +
> > + pruss_intc_update_hmr(intc, ch, host);
> > +
> > + /* enable host interrupts */
> > + pruss_intc_write_reg(intc, PRU_INTC_HIEISR, host);
> > + }
> > +
> > + dev_dbg(dev, "mapped system_event = %lu channel = %d host = %d",
> > + hwirq, ch, host);
> > +
> > + /* global interrupt enable */
> > + pruss_intc_write_reg(intc, PRU_INTC_GER, 1);
> > +
> > +unlock:
> > + mutex_unlock(&intc->lock);
> > + return ret;
> > +}
> > +
> > +/**
> > + * pruss_intc_unmap() - unconfigure the PRUSS INTC
> > + * @intc: PRUSS interrupt controller 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)
> > +{
> > + u8 ch, host, reg_idx;
> > + u32 val;
> > +
> > + if (hwirq >= intc->soc_config->num_system_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);
> > +
> > + /* clear the map using reset value 0 */
> > + pruss_intc_update_hmr(intc, ch, 0);
> > + }
> > +
> > + if (--intc->event_channel[hwirq].ref_count == 0) {
> > + reg_idx = hwirq / 32;
> > + val = BIT(hwirq % 32);
> > +
> > + /* disable system events */
> > + pruss_intc_write_reg(intc, PRU_INTC_ECR(reg_idx), val);
> > + /* clear any pending status */
> > + pruss_intc_write_reg(intc, PRU_INTC_SECR(reg_idx), val);
> > +
> > + /* clear the map using reset value 0 */
> > + pruss_intc_update_cmr(intc, hwirq, 0);
> > + }
> > +
> > + dev_dbg(intc->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)
> > {
> > const struct pruss_intc_match_data *soc_config = intc->soc_config;
> > @@ -212,10 +410,67 @@ static struct irq_chip pruss_irqchip = {
> > .irq_set_irqchip_state = pruss_intc_irq_set_irqchip_state,
> > };
> >
> > +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;
> > + struct device *dev = intc->dev;
> > + int sys_event, channel, host;
> > +
> > + if (intsize == 1) {
> > + /*
> > + * In case of short version (intsize == 1) verify if sysevent
> > + * already mapped to channel/host irq if not return error
> > + */
> > + sys_event = intspec[0];
> > + if (intc->event_channel[sys_event].ref_count)
> > + goto already_mapped;
> > + else
> > + return -EINVAL;
>
> Perhaps revise this to check the error condition first and skipping the
> else, similar to the change you have done in Patch 3.
Ok. I will do that for v4.
>
> > + }
> > +
> > + if (intsize < 3)
> > + return -EINVAL;
> > +
> > + sys_event = intspec[0];
> > + if (sys_event < 0 || sys_event >= intc->soc_config->num_system_events) {
> > + dev_err(dev, "not valid event number\n");
>
> Would be useful to print the invalid event numbers here, and each of
> channel and host below.
Yes, good point.
Thank you,
Grzegorz
prev parent reply other threads:[~2020-07-05 13:39 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 14:17 [PATCHv3 0/6] Add TI PRUSS Local Interrupt Controller IRQChip driver Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 1/6] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings Grzegorz Jaszczyk
2020-07-13 21:25 ` Rob Herring
2020-07-16 9:25 ` Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts Grzegorz Jaszczyk
2020-07-02 17:24 ` Marc Zyngier
2020-07-03 14:28 ` Grzegorz Jaszczyk
2020-07-04 9:39 ` Marc Zyngier
2020-07-05 13:26 ` Grzegorz Jaszczyk
2020-07-05 20:45 ` Marc Zyngier
2020-07-08 7:04 ` Grzegorz Jaszczyk
2020-07-08 10:47 ` Marc Zyngier
2020-07-10 23:03 ` Suman Anna
2020-07-15 13:38 ` Grzegorz Jaszczyk
2020-07-17 12:36 ` Marc Zyngier
2020-07-21 9:27 ` Grzegorz Jaszczyk
2020-07-21 10:10 ` Marc Zyngier
2020-07-21 13:59 ` Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts Grzegorz Jaszczyk
2020-07-02 17:44 ` Marc Zyngier
2020-07-10 20:59 ` Suman Anna
2020-07-17 11:02 ` Marc Zyngier
2020-07-25 15:57 ` Suman Anna
2020-07-25 16:27 ` Marc Zyngier
2020-07-25 16:39 ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Grzegorz Jaszczyk
2020-07-02 17:54 ` Marc Zyngier
2020-07-03 17:04 ` Grzegorz Jaszczyk
2020-07-10 21:04 ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 5/6] irqchip/irq-pruss-intc: Add support for ICSSG INTC on K3 SoCs Grzegorz Jaszczyk
2020-07-02 17:59 ` Marc Zyngier
2020-07-03 17:05 ` Grzegorz Jaszczyk
2020-07-10 21:13 ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 6/6] irqchip/irq-pruss-intc: Add event mapping support Grzegorz Jaszczyk
2020-07-02 16:24 ` Suman Anna
2020-07-05 13:39 ` Grzegorz Jaszczyk [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMxfBF52X+USBYt4MGs_z3T+SciqoJrGW61ZC39P3Xo8rjqPsg@mail.gmail.com \
--to=grzegorz.jaszczyk@linaro.org \
--cc=david@lechnology.com \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=maz@kernel.org \
--cc=robh+dt@kernel.org \
--cc=s-anna@ti.com \
--cc=tglx@linutronix.de \
--cc=wmills@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).