Messages in this thread | | | Subject | Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver | From | Lokesh Vutla <> | Date | Fri, 26 Oct 2018 12:09:37 +0530 |
| |
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@ti.com> 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. >
| |