Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
On Tuesday 01 December 2015 06:59 PM, Marc Zyngier wrote:
>> +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.
>
> But using the Linux irq number is always wrong. You should only consider
> the hwirq.
The source is this incorrectness is ARC core intc code which also does the same
thing and we get away with it because of the legacy domain usage.
I'll fix that up.
Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
From: Marc Zyngier [mailto:marc.zyng...@arm.com] Sent: Thursday, December 03, 2015 8:34 PM >>> 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. Seem to me that ".inst" is ARM Machine directive. >Also, can you always tell the per-cpu property of your interrupt based on its >number? If you can, then it is fine. Yes I can, and I am using this knowledge. >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. Indeed when I moved from legacy to linear map I had to call irq_create_mapping() for my percpu IRQs and that's it. Thanks -Noam ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Hi Noam,
On 02/12/15 15:08, Noam Camus wrote:
>> From: Marc Zyngier [mailto:marc.zyng...@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_EN0x100 /* 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...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
>From: Marc Zyngier [mailto:marc.zyng...@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_BLK0x118 /* 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.
>> +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.
>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?
-Noam
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
On 01/12/15 13:02, Noam Camus wrote:
> From: Noam Camus
>
> Adding EZchip NPS400 support.
> NPS internal interrupts are internally handled at
> Multi Thread Manager (MTM) that is signaled for deactivating
> an interrupt.
> External interrupts is handled also at Global Interrupt
> Controller (GIC) e.g. serial and network devices.
>
> Signed-off-by: Noam Camus
> Cc: Thomas Gleixner
> Cc: Jason Cooper
> Cc: Marc Zyngier
> ---
> .../interrupt-controller/ezchip,nps400-ic.txt | 17 ++
> drivers/irqchip/Makefile |1 +
> drivers/irqchip/irq-eznps.c| 213
>
> 3 files changed, 231 insertions(+), 0 deletions(-)
> create mode 100644
> Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> create mode 100644 drivers/irqchip/irq-eznps.c
>
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> new file mode 100644
> index 000..888b2b9
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> @@ -0,0 +1,17 @@
> +EZchip NPS Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : should be "ezchip,nps400-ic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> + interrupt source. The value shall be 1.
So you never have to encode the interrupt trigger type? Do you only
support edge or level?
> +
> +
> +Example:
> +
> +intc: interrupt-controller {
> + compatible = "ezchip,nps400-ic";
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +};
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..b95b954 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC) +=
> irq-renesas-h8s.o
> obj-$(CONFIG_ARCH_SA1100)+= irq-sa11x0.o
> obj-$(CONFIG_INGENIC_IRQ)+= irq-ingenic.o
> obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o
> +obj-$(CONFIG_ARC_PLAT_EZNPS) += irq-eznps.o
> diff --git a/drivers/irqchip/irq-eznps.c b/drivers/irqchip/irq-eznps.c
> new file mode 100644
> index 000..bb8d547
> --- /dev/null
> +++ b/drivers/irqchip/irq-eznps.c
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright(c) 2015 EZchip Technologies.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define NPS_MSU_EN_CFG 0x80/* MSU Enable Configuration Register */
> +#define MSU_EN BIT(0) /* MSU block enable */
> +#define IPI_EN BIT(16) /* Enable service of incoming IPI
> Messages */
> +#define GIM0_EN BIT(17) /* Enable service of incoming GIM 0
> messages */
> +#define GIM1_EN BIT(18) /* Enable service of incoming GIM 1
> messages */
> +
> +#define NPS_GIM_P_POL0x110 /* Peripheral interrupts source
> polarity */
> +#define NPS_GIM_P_SENS 0x114 /* Peripheral interrupts sensitivity */
> +#define GIM_UART BIT(7)
> +#define GIM_LAN_TX (BIT(10) | BIT(25))
> +#define GIM_LAN_RX (BIT(11) | BIT(26))
> +#define GIM_PERIPH_ALL (GIM_UART | GIM_LAN_TX | GIM_LAN_RX)
> +
> +#define NPS_GIM_P_DST10 0x13A /* Peripheral Interrupt Destination
> (LAN RX) */
> +#define NPS_GIM_P_DST11 0x13B /* Peripheral Interrupt Destination
> (LAN TX) */
> +#define NPS_GIM_P_DST25 0x149 /* Peripheral Interrupt Destination
> (LAN RX) */
> +#define NPS_GIM_P_DST26 0x14A /* Peripheral Interrupt Destination
> (LAN TX) */
> +#define DST_IS BIT(26) /* Interrupt select for line 7 */
> +
> +#define NPS_GIM_P_EN 0x100 /* Peripheral interrupts source enable */
> +#define NPS_GIM_P_BLK0x118 /* Peripheral interrupts blocking for
> sources */
Are these the interrupts the peripherals are using? If yes, they really
have nothing to do here...
> +
> +/* Messaging and Scheduling Unit:
> + * Provides message management for a CPU cluster.
> + */
> +static void __init eznps_configure_msu(void)
> +{
> + int cpu;
> + u32 value = MSU_EN | IPI_EN | GIM0_EN | GIM1_EN;
> +
> + /* Enable IPI and GIM messages on all clusters */
> + for (cpu = 0 ; cp
