From: Marc Zyngier <marc.zyngier@arm.com> To: Noam Camus <noamc@ezchip.com>, "linux-snps-arc@lists.infradead.org" <linux-snps-arc@lists.infradead.org> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Chris Metcalf <cmetcalf@ezchip.com>, Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net> Subject: Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Date: Thu, 03 Dec 2015 18:33:35 +0000 [thread overview] Message-ID: <56608AFF.80302@arm.com> (raw) In-Reply-To: <DB5PR02MB114150510832BA29D46A8A4AD60E0@DB5PR02MB1141.eurprd02.prod.outlook.com> Hi Noam, On 02/12/15 15:08, Noam Camus wrote: >> From: Marc Zyngier [mailto:marc.zyngier@arm.com] >> Sent: Tuesday, December 01, 2015 3:29 PM > >> + interrupt source. The value shall be 1. > >> So you never have to encode the interrupt trigger type? Do you only support edge or level? > I Always use level sensitive. > >> + >> +#define NPS_GIM_P_EN 0x100 /* Peripheral interrupts source enable */ >> +#define NPS_GIM_P_BLK 0x118 /* Peripheral interrupts blocking for sources */ > >>> Are these the interrupts the peripherals are using? If yes, they really have nothing to do here... > I will move this from here >>> + __asm__ __volatile__ ( >>> + " .word %0\n" >>> + : >>> + : "i"(CTOP_INST_RSPI_GIC_0_R12) >>> + : "memory"); > >> Silly question: why cannot you just write the actual instruction >> instead of shoving the instruction like this? Also, .inst would be >> more appropriate... > [Noam Camus] Since this is instruction that yet is not part of > up-streamed binutils of ARC. Now ARC maintainer can build our kernel > with generic ARC toolchain. OK. If you decide to carry on using this, I'd still recommend using .inst instead of .word, so that you can get a proper disassembly. >>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq, >>> + irq_hw_number_t hw) >>> +{ >>> + switch (irq) { >>> + case TIMER0_IRQ: >>> +#if defined(CONFIG_SMP) >>> + case IPI_IRQ: >>> +#endif >>> + irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu, >>> + handle_percpu_irq); >>> + break; >>> + default: >>> + irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi, >>> + handle_fasteoi_irq); >>> + break; >>> + } > >> No. This is just wrong. Either you get per interrupt information >> from the device tree to configure the interrupt the right way, or >> you have different interrupt controllers for each device. > I am not sure how you want me to get it from DTB? Please refer to > some reference. Here, you are assuming that 'irq' is a hardware number, while there is no reason why it should be (it only works because you are using legacy domains, more on that later). Your switch/case statement should be based on the 'hw' parameter, because that is your HW IRQ number. the irq parameter can be completely random, and will eventually be once you fix the rest of the driver. Also, can you always tell the per-cpu property of your interrupt based on its number? If you can, then it is fine. >> But using the Linux irq number is always wrong. You should only consider the hwirq. > I will change > >> + >> + nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0, >> + &nps400_irq_ops, NULL); > >> And that's why you can get away with the above horror. Don't use >> legacy domains. This stuff is by no mean legacy. > So what is my alternative here? Your alternative is to use irq_domain_add_linear, for example, and to make sure that you always refer to the hw number when manipulating the HW. You will quickly notice that the Linux IRQ number has nothing to do with the HW one, and you'll be able to quickly iron out the bugs. Looking forward to reviewing your next version. Thanks, M. -- Jazz is not dead. It just smells funny...
WARNING: multiple messages have this Message-ID (diff)
From: marc.zyngier@arm.com (Marc Zyngier) To: linux-snps-arc@lists.infradead.org Subject: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Date: Thu, 03 Dec 2015 18:33:35 +0000 [thread overview] Message-ID: <56608AFF.80302@arm.com> (raw) In-Reply-To: <DB5PR02MB114150510832BA29D46A8A4AD60E0@DB5PR02MB1141.eurprd02.prod.outlook.com> Hi Noam, On 02/12/15 15:08, Noam Camus wrote: >> From: Marc Zyngier [mailto:marc.zyngier at arm.com] >> Sent: Tuesday, December 01, 2015 3:29 PM > >> + interrupt source. The value shall be 1. > >> So you never have to encode the interrupt trigger type? Do you only support edge or level? > I Always use level sensitive. > >> + >> +#define NPS_GIM_P_EN 0x100 /* Peripheral interrupts source enable */ >> +#define NPS_GIM_P_BLK 0x118 /* Peripheral interrupts blocking for sources */ > >>> Are these the interrupts the peripherals are using? If yes, they really have nothing to do here... > I will move this from here >>> + __asm__ __volatile__ ( >>> + " .word %0\n" >>> + : >>> + : "i"(CTOP_INST_RSPI_GIC_0_R12) >>> + : "memory"); > >> Silly question: why cannot you just write the actual instruction >> instead of shoving the instruction like this? Also, .inst would be >> more appropriate... > [Noam Camus] Since this is instruction that yet is not part of > up-streamed binutils of ARC. Now ARC maintainer can build our kernel > with generic ARC toolchain. OK. If you decide to carry on using this, I'd still recommend using .inst instead of .word, so that you can get a proper disassembly. >>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq, >>> + irq_hw_number_t hw) >>> +{ >>> + switch (irq) { >>> + case TIMER0_IRQ: >>> +#if defined(CONFIG_SMP) >>> + case IPI_IRQ: >>> +#endif >>> + irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu, >>> + handle_percpu_irq); >>> + break; >>> + default: >>> + irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi, >>> + handle_fasteoi_irq); >>> + break; >>> + } > >> No. This is just wrong. Either you get per interrupt information >> from the device tree to configure the interrupt the right way, or >> you have different interrupt controllers for each device. > I am not sure how you want me to get it from DTB? Please refer to > some reference. Here, you are assuming that 'irq' is a hardware number, while there is no reason why it should be (it only works because you are using legacy domains, more on that later). Your switch/case statement should be based on the 'hw' parameter, because that is your HW IRQ number. the irq parameter can be completely random, and will eventually be once you fix the rest of the driver. Also, can you always tell the per-cpu property of your interrupt based on its number? If you can, then it is fine. >> But using the Linux irq number is always wrong. You should only consider the hwirq. > I will change > >> + >> + nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0, >> + &nps400_irq_ops, NULL); > >> And that's why you can get away with the above horror. Don't use >> legacy domains. This stuff is by no mean legacy. > So what is my alternative here? Your alternative is to use irq_domain_add_linear, for example, and to make sure that you always refer to the hw number when manipulating the HW. You will quickly notice that the Linux IRQ number has nothing to do with the HW one, and you'll be able to quickly iron out the bugs. Looking forward to reviewing your next version. Thanks, M. -- Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2015-12-03 18:33 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-01 13:02 [PATCH v3 00/18] *** SUBJECT HERE *** Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-04 8:07 ` Daniel Lezcano 2015-12-04 8:07 ` Daniel Lezcano 2015-12-04 11:46 ` Noam Camus 2015-12-04 11:46 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 02/18] ARC: [plat-eznps] define IPI_IRQ Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 03/18] clocksource: Add NPS400 timers driver Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-04 9:13 ` Daniel Lezcano 2015-12-04 9:13 ` Daniel Lezcano 2015-12-04 12:26 ` Noam Camus 2015-12-04 12:26 ` Noam Camus 2015-12-04 12:52 ` Daniel Lezcano 2015-12-04 12:52 ` Daniel Lezcano 2015-12-08 13:00 ` Noam Camus 2015-12-08 13:00 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:29 ` Marc Zyngier 2015-12-01 13:29 ` Marc Zyngier 2015-12-02 15:08 ` Noam Camus 2015-12-02 15:08 ` Noam Camus 2015-12-03 18:33 ` Marc Zyngier [this message] 2015-12-03 18:33 ` Marc Zyngier 2015-12-07 11:19 ` Noam Camus 2015-12-07 11:19 ` Noam Camus 2015-12-11 7:58 ` Vineet Gupta 2015-12-11 7:58 ` Vineet Gupta 2015-12-01 13:02 ` [PATCH v3 05/18] ARC: Set vmalloc size from configuration Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 06/18] ARC: rwlock: disable interrupts in !LLSC variant Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 07/18] ARC: rename smp operation init_irq_cpu() to init_per_cpu() Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 08/18] ARC: Mark secondary cpu online only after all HW setup is done Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 09/18] ARC: add CONFIG_CLKSRC_OF support to time_init() Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 10/18] ARC: [plat-eznps] Add eznps board defconfig and dts Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 11/18] ARC: [plat-eznps] Add eznps platform Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:02 ` [PATCH v3 12/18] ARC: [plat-eznps] Use dedicated user stack top Noam Camus 2015-12-01 13:02 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 13/18] ARC: [plat-eznps] Use dedicated atomic/bitops/cmpxchg Noam Camus 2015-12-01 13:03 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 14/18] ARC: [plat-eznps] Use dedicated SMP barriers Noam Camus 2015-12-01 13:03 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 15/18] ARC: [plat-eznps] Use dedicated identity auxiliary register Noam Camus 2015-12-01 13:03 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 16/18] ARC: [plat-eznps] Use dedicated cpu_relax() Noam Camus 2015-12-01 13:03 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 17/18] ARC: [plat-eznps] Use dedicated COMMAND_LINE_SIZE Noam Camus 2015-12-01 13:03 ` Noam Camus 2015-12-01 13:03 ` [PATCH v3 18/18] ARC: Add eznps platform to Kconfig and Makefile Noam Camus 2015-12-01 13:03 ` Noam Camus
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=56608AFF.80302@arm.com \ --to=marc.zyngier@arm.com \ --cc=cmetcalf@ezchip.com \ --cc=jason@lakedaemon.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-snps-arc@lists.infradead.org \ --cc=noamc@ezchip.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.