Re: [PATCH v4 05/19] irqchip: add nps Internal and external irqchips

[Vineet Gupta]

On Tuesday 12 January 2016 02:18 PM, Marc Zyngier wrote:
> Hi Vineet,
>
> Sorry I missed that one, it must have been caught in the mother of all
> "mark as read" I did on coming back from holiday.

That was expected - NP :-)

>
> On 12/01/16 07:00, Vineet Gupta wrote:
>> > Hi Marc,
>> > 
>> > On Wednesday 30 December 2015 05:05 PM, Vineet Gupta wrote:
>>> >> On Friday 18 December 2015 10:01 PM, Marc Zyngier wrote:
>>>> >>> On 18/12/15 14:29, Noam Camus wrote:
>>>>>> >>>>> From: Marc Zyngier [mailto:marc.zyngier@xxxxxxx] 
>>>>>> >>>>> Sent: Friday, December 18, 2015 1:21 PM
>>>>> >>>>
>>>>>>> >>>>>> I need this for my per CPU irqs such timer and IPI which do not come 
>>>>>>> >>>>>> from some external device but from CPUs. For these IRQs I am calling 
>>>>>>> >>>>>> to irq_create_mapping() from my platform at arch/arc and at that point 
>>>>>>> >>>>>> I got no irqdomain and using irq_find_host() is not good since I got 
>>>>>>> >>>>>> no device_node (at most I can have DT root).
>>>>> >>>>
>>>>>> >>>>> That's a problem. You should never do that for your timer (doing a request_irq will do the right thing, and that's what your timer driver already does).
>>>>> >>>>
>>>>> >>>> Please be more specific, from all that I wrote what is the problem?
>>>> >>>
>>>> >>> Calling irq_create_mapping out of the blue like you do it here:
>>>> >>>
>>>> >>> +static void eznps_init_per_cpu(int cpu)
>>>> >>> +{
>>>> >>> +	/* Create mapping for all per cpu IRQs */
>>>> >>> +	if (cpu == 0) {
>>>> >>> +		irq_create_mapping(NULL, TIMER0_IRQ);
>>>> >>> +		irq_create_mapping(NULL, IPI_IRQ);
>>>> >>> +	}
>>>> >>>
>>>> >>> is simply not acceptable.
>>>> >>>
>>>>> >>>> When I use request_irq() it fail without the mapping and mapping fail without a domain.
>>>> >>>
>>>> >>> Grmbl...
>>>> >>>
>>>> >>> That's not completely surprising:
>>>> >>>
>>>> >>> +		timer {
>>>> >>> +			compatible = "ezchip,nps400-timer";
>>>> >>> +			clocks = <&sysclk>;
>>>> >>> +			clock-names="sysclk";
>>>> >>> +		};
>>>> >>>
>>>> >>> Where is the interrupt?
>>>> >>>
>>>>> >>>> Never do what?
>>>>> >>>> What should I do then?
>>>> >>>
>>>> >>> Maybe you should start by looking how the other architectures have
>>>> >>> solved that exact problem at least half a dozen time.
>>>> >>>
>>>>> >>>>
>>>>>> >>>>> As for initializing your IPIs, they are usually outside of the IRQ
>>>>>> >>>>> space, so you should handle them separately (and get your irqchip
>>>>>> >>>>> to initialize them).
>>>>> >>>> I am handling all my IRQs within same irqchip, which is the only one 
>>>>> >>>> I have. So I am not sure what you expect here. Please be more 
>>>>> >>>> elaborate.
>>>> >>>
>>>> >>> Do not create a mapping for IPIs. Full stop. Handle them independently
>>>> >>> from your normal IRQs.
>>>> >>>
>>>>>>> >>>>>>
>>>>>>> >>>>>> Another thing I'm not seeing here is where is the interrupt actually taken. This code only contains the EOI part, but not the ACK side, as well as the reverse lookup hwirq -> irq). Where is that code?
>>>>>>> >>>>>>
>>>>>>> >>>>>> ACK is an optional handler and is not needed by my platform.
>>>>>>> >>>>>> I will add comment that since my IRQs are EOI based I do not need an ACK.
>>>>> >>>>
>>>>>> >>>>> What I'm talking about is not the irq_ack method that the irqchip can provide, but the action your perform on your interrupt controller to extract the hwirq number and find out what corresponding Linux interrupt has fired.
>>>>> >>>>
>>>>>>> >>>>>>
>>>>>>> >>>>>> I do not understand reverse lookup remark, where is it missing?
>>>>>>> >>>>>> Could you point me to an example for such reverse lookup? 
>>>>> >>>>
>>>>>> >>>>> See for example:
>>>>> >>>>
>>>>>> >>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sun4i.c#n136
>>>>> >>>>
>>>>>> >>>>> This is the function (sun4i_handle_irq) that is executed almost immediately after the IRQ is asserted. The CPU reads the hwirq from the interrupt controller, and use handle_domain_irq() to call the corresponding handler (by doing a lookup in the domain).
>>>>> >>>>
>>>>>> >>>>> I couldn't find out in your platform code where you are doing that.
>>>>> >>>>
>>>>> >>>> OK, this is seem much like exclusively ARM stuff.
>>>> >>>
>>>> >>> No, this is not. Can you please stop looking at the surface of things
>>>> >>> and start taking an interest in how things actually *work*? Almost
>>>> >>> *nothing* in the interrupt handling code is architecture specific.
>>>> >>>
>>>>> >>>> Note that I am working with ARC (seem alike) here and we do not
>>>>> >>>> define CONFIG_HANDLE_DOMAIN_IRQ and do not implement
>>>>> >>>> set_handle_irq().
>>>>> >>>>
>>>>> >>>> So for ARC this reverse mapping is something we can leave without
>>>>> >>>> (maybe because we are kind of a legacy domain).
>>>> >>>
>>>> >>> Yeah, I just located the crap: arch_do_IRQ() happily takes a hwirq (the
>>>> >>> vector number), and uses that as a Linux IRQ. This looks a lot like ARM
>>>> >>> pre-DT, about 10 years ago.
>>>> >>>
>>>> >>> Well, time to meet the 21st century. If you intend to use DT, please fix
>>>> >>> your arch port. Otherwise, just hardcode everything in your platform and
>>>> >>> don't pretend to support device tree.
>>> >>
>>> >> Hi Marc,
>>> >>
>>> >> Taking your rant in a positive stride and I'm all up for making this as
>>> >> nice/modern as possible. I don't have issues with enabling
>>> >> CONFIG_HANDLE_DOMAIN_IRQ for ARC (although it might add a few cycles o/h to each ISR)
>>> >>
>>> >> However currently (4.4-rcX) it is only enabled for arm/arm64/openrisc and from the
>>> >> looks of it in drivers/irqchip, only ARM based SoCs use the handle_domain_irq()
>>> >> calls by plugging into ARM top level handler.
>>> >>
>>> >> Why is that not a problem for other arches like PPC/MIPS which also use DT
>>> >> heavily. Or perhaps they are also subtly broken as ARC is !
>>> >>
>>> >> What am I missing ?
> HANDLE_DOMAIN_IRQ is not mandatory at all - a number of architectures
> had something open-coded in the past (with some drawbacks and/or bugs),
> and this config option is just one of the ways to get it right.
>
> MIPS/PPC perform the reverse lookup directly, without using this
> infrastructure, and that would be a valid approach for ARC too.

I'd rather use the generic infrastructure and improve it if needed !

>> > Marc, I hope you are back from holidays. When u get a chance could you please
>> > respond to above.
>> > 
>> > Also I tool a stab at it anyways.
>> > 1. Enabled HANDLE_DOMAIN_IRQ
>> > 2. arch_do_IRQ() calls handle_domain_irq(NULL, hwirq, regs) since that code
>> > doesn't know about domains.
>> > 
>> > It fails w/o irq_set_default_host() being called.
> Well, that's expected. unless you use the underlying primitive
> (__handle_domain_irq) and pass false as the parameter for lookup (see
> handle_IRQ() in arch/arm/kernel/irq.c). This is what we use for "legacy"
> platforms that do not support domains.

Right I saw that and that causes virq = hwirq - kind of defeats the purpose if we
were doing this on ARC.

>
>> > If we go back to sunxi example you quoted above, it relies on driver passing the
>> > domain to handle_domain_irq(). IMHO it is simpler if we had the default domain.
>> > 
>> > So long story short, ARC can be made to use handle_domain_irq() w/o the song and
>> > dance of registering another callback from irqchip code if we retained the default
>> > domain setting.
> This only works if you can guarantee that you will never have another
> irqchip calling irq_set_default_host()... If your system is always
> simple enough to guarantee that, why not.

ATM we can certainly assume that.

However with ARM approach two irqchips can still call set_handle_irq() and only
the first one succeeds (and others return silently). That seems wrong to me -
irq_xxx.c will still use the handler registered by say irq_sun41.c ?

Thx,
-Vineet