Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver
From: Lokesh Vutla
Date: Fri Oct 26 2018 - 02:46:25 EST
Hi Marc,
On Tuesday 23 October 2018 07:20 PM, Marc Zyngier wrote:
Hi Lokesh,
On Mon, 22 Oct 2018 15:35:41 +0100,
Lokesh Vutla <lokeshvutla@xxxxxx> wrote:
Hi Marc,
On Friday 19 October 2018 08:52 PM, Marc Zyngier wrote:
Hi Lokesh,
On 18/10/18 16:40, Lokesh Vutla wrote:
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator
which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by
an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.
Aren't the last two points the same thing? Also, can you please define
what an "event" is? What is its semantic? If they look like interrupts,
can we just name them as such?
Event is actually a message sent by a master via an Event transport
lane. Based on the id within the message, each message is directed to
corresponding Interrupt Aggregator(IA). In turn IA raises a
corresponding interrupt as configured for this event.
Ergo, this is an interrupt, and there is nothing more to it. HW folks
may want to give it a sexy name, but as far as SW is concerned, it has
the properties of an interrupt and should be modelled as such.
[...]
+ for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+ val = 1 << bit;
+ __raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+ VINT_ENABLE_CLR_OFFSET);
+ }
If you need an ack callback, why is this part of the eoi? We have
interrupt flows that allow you to combine both, so why don't you use that?
Actually I started with ack_irq. But I did not see this callback being
triggered when interrupt is raised. Then I was suggested to use
irq_roi. Will see why ack_irq is not being triggered and update it in
next version.
It is probably because you're not using the right interrupt flow.
Also, the __raw_writeq call is probably wrong, as it assumes that both
the CPU and the INTA have the same endianness.
hmm.. May I know what is the right call to use here?
writeq_relaxed is most probably what you want. I assume this code will
never run on a 32bit platform, right?
[...]
+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain: Domain to which the irqs belong
+ * @virq: base linux virtual IRQ to be freed.
+ * @nr_irqs: Number of continuous irqs to be freed
+ */
+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct ti_sci_inta_irq_domain *inta = domain->host_data;
+ struct ti_sci_inta_vint_desc *vint_desc;
+ struct irq_data *data, *gic_data;
+ int event_index;
+
+ data = irq_domain_get_irq_data(domain, virq);
+ gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
That's absolutely horrid...
I agree. But I need to get GIC irq for sending TISCI message. Can you
suggest a better way of doing it?
I'd say "fix the firmware to have a layered approach". But I guess
that's not an option, right?
yeah, we cannot change the APIs now.
[...]
+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta: Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc: Virtual interrupt descriptor to which the event gets attached.
+ * @src_id: TISCI device id of the event source
+ * @src_index: Event index with in the device.
+ * @dst_irq: Destination host irq
+ * @vint: Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+ struct ti_sci_inta_vint_desc *vint_desc,
+ u16 src_id, u16 src_index, u16 dst_irq,
+ u16 vint)
+{
+ struct ti_sci_inta_event_desc *event;
+ unsigned long flags;
+ u32 free_bit;
+ int err;
+
+ raw_spin_lock_irqsave(&vint_desc->lock, flags);
+ free_bit = find_first_zero_bit(vint_desc->event_map,
+ MAX_EVENTS_PER_VINT);
+ if (free_bit != MAX_EVENTS_PER_VINT)
+ set_bit(free_bit, vint_desc->event_map);
+ raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
Why disabling the interrupts? Do you expect to take this lock
concurrently with an interrupt? Why isn't it enough to just have a mutex
instead?
I have thought about this while coding. We are attaching multiple
events to the same interrupt. Technically the events from different
IPs can be attached to the same interrupt or events from the same IP
can be registered at different times. So I thought it is possible that
when an event is being allocated to an interrupt, an event can be
raised that belongs to the same interrupt.
I strongly dispute this. Events are interrupts, and we're not
requesting an interrupt from an interrupt handler. That would be just
crazy.
okay, will use mutex instead.
[...]
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard
model to catter for your particular use case. But we don't define a new,
TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model
we use for MSIs would fit this thing. I also want to understand what
hmm..I haven't thought about using MSI. Will try to explore it. But
the "struct msi_msg" is not applicable in this case as device does not
write to a specific location.
It doesn't need to. You can perfectly ignore the address field and
only be concerned with the data. We already have MSI users that do not
need programming of the doorbell address, just the data.
Okay. I am reworking towards using MSI for this case. Will post the series once
it is done.
Once again, Thanks for the clear explanation.
Thanks and regards,
Lokesh
this event is, and how drivers get notified that such an event has fired.
As said above, Event is a message being sent by a device using a
hardware protocol. This message is sent over an Event Transport
Lane(ETL) that understands this protocol. Based on the message ETL re
directs the message to a specificed target(In our case it is interrupt
Aggregator).
From a client drivers(that generates this event) prespective, the
following needs to be done:
- Get an index that is free and allocate it to a particular task.
- Request INTA driver to assign an irq for this index.
- do a request_irq baseed on the return value from the above step.
All of that can be done in the using the current MSI framework. You
can either implement your own bus framework or use the platform MSI
stuff. You can then rewrite the INTA driver to be what it really is,
an interrupt multiplexer.
In case of grouping events, the client drivers has its own mechanism
to identify the index that caused an interrupt(at least that is the
case for the existing user).
This simply isn't acceptable. Each event must be the result of a
single interrupt allocation from the point of view of the driver. If
events are shared, they should be modelled as a shared interrupt.
Overall, I'm extremely concerned that you're reinventing the wheel and
coming up with a new "concept" that seems incredibly similar to what
we already have everywhere else, just offering an incompatible
API. This means that your drivers become specialised for your new API,
and this isn't going to fly.
I can only urge you to reconsider the way you provide these events,
and make sure that you use the existing API to its full potential. If
something is not up to the task, we can then fix it in core code.
Thanks,
M.