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.
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. > 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. > 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. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
