From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932699AbbLBPI5 (ORCPT ); Wed, 2 Dec 2015 10:08:57 -0500 Received: from mail-am1on0056.outbound.protection.outlook.com ([157.56.112.56]:42339 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932512AbbLBPIz convert rfc822-to-8bit (ORCPT ); Wed, 2 Dec 2015 10:08:55 -0500 From: Noam Camus To: Marc Zyngier , "linux-snps-arc@lists.infradead.org" CC: "linux-kernel@vger.kernel.org" , "Chris Metcalf" , Thomas Gleixner , "Jason Cooper" Subject: RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Thread-Topic: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Thread-Index: AQHRLDj4dTCLMG5tRUC3s/7br+9FDp62ICYAgAGqn9A= Date: Wed, 2 Dec 2015 15:08:51 +0000 Message-ID: References: <1448974985-11487-1-git-send-email-noamc@ezchip.com> <1448974985-11487-5-git-send-email-noamc@ezchip.com> <565DA0B6.5080903@arm.com> In-Reply-To: <565DA0B6.5080903@arm.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=noamc@ezchip.com; x-originating-ip: [212.179.42.66] x-microsoft-exchange-diagnostics: 1;AMSPR02MB120;5:+zR32uDidCT9MDd3B4OdsJXA7spyAJt9Awji7negptpVBgeFo0ZFIoe7eIY/pMn7VFWje85r6xTeOjy031olgHcCw5C8acFC1DYA4Jv/Q6bNo7Soc8UfFvw62bGYv00xrxYEX3t2cSpMvRB5JtUs9A==;24:vWgMCVnpltiw8xcHKDeWPIRz21KjO3cqY8TX+GkmJrBe4uPyxPkcg+CtaDfYw+sisqyyV8HGHOVElh7tDbpdld5SP30YXOiYve4iIXDx7Hw= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AMSPR02MB120; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(520078)(8121501046)(3002001)(10201501046);SRVR:AMSPR02MB120;BCL:0;PCL:0;RULEID:;SRVR:AMSPR02MB120; x-forefront-prvs: 077884B8B5 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(199003)(377454003)(189002)(5008740100001)(50986999)(1096002)(76176999)(33656002)(586003)(97736004)(122556002)(105586002)(1220700001)(3846002)(5002640100001)(102836003)(101416001)(5003600100002)(74316001)(40100003)(106356001)(87936001)(92566002)(6116002)(106116001)(19580395003)(2900100001)(2950100001)(81156007)(19580405001)(189998001)(10400500002)(76576001)(5001770100001)(54356999)(575784001)(5004730100002)(2501003)(66066001)(5001960100002)(86362001)(77096005);DIR:OUT;SFP:1101;SCL:1;SRVR:AMSPR02MB120;H:DB5PR02MB1141.eurprd02.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: ezchip.com X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Dec 2015 15:08:51.8183 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 0fc16e0a-3cd3-4092-8b2f-0a42cff122c3 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AMSPR02MB120 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >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. >> +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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: noamc@ezchip.com (Noam Camus) Date: Wed, 2 Dec 2015 15:08:51 +0000 Subject: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips In-Reply-To: <565DA0B6.5080903@arm.com> References: <1448974985-11487-1-git-send-email-noamc@ezchip.com> <1448974985-11487-5-git-send-email-noamc@ezchip.com> <565DA0B6.5080903@arm.com> List-ID: Message-ID: To: linux-snps-arc@lists.infradead.org >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. >> +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