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.zyng...@arm.com] >>>>>> >>>>> 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 _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
