Received: by 10.213.65.68 with SMTP id h4csp1521506imn; Mon, 19 Mar 2018 06:32:05 -0700 (PDT) X-Google-Smtp-Source: AG47ELvD4K9lHAjUmXjwsE0K9m5VaR740VlguCNWq8i0g1hTE+cVAw9uiNFsLLgwul1FR24Jv+lJ X-Received: by 10.101.65.134 with SMTP id a6mr9104507pgq.331.1521466325695; Mon, 19 Mar 2018 06:32:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1521466325; cv=none; d=google.com; s=arc-20160816; b=iZCGK8duX5vSQkA3s46hI9Ux80owlczx4PMdWERJK7VYEhgKYx88odEPKPM6rfosyx ++o0TEQaxRckDTRVfuoB+uJWjUBLbpORkM3iQSizKggj7gWPwB+QRdar2NbPA6nwlQTd gztNfFynVl9vdab6xt4S75DJJFo7LADIK0n+Hif/Rhfu2Yn2k7TJy0D7eobBkMfRghI4 gLgq/W7GsazWXzab+UPm5STl/RPaMKw0Bv19S86mW4A67jaoEMv+ASJTBOD3ud5RGgZJ QX6MdYG2DXXldTlsH/V/WZMNhS1yYvD3/mDIL3AXi+axuDdlCpl6pIqNGsr3kgobfUKR F+5w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:mime-version:user-agent:references :message-id:in-reply-to:subject:cc:to:from:date :arc-authentication-results; bh=BKwgFat9dhOTT2erQRAYQgg8U/2VL1yzyFdx8pK+RTA=; b=vnE+243X2n5/dN/FY45N24BnB08MVkx1ET/ofxUvRMyRoTkmxoNlDhGDvTGcWwrK3N nE3Pn+OlX6VLcK5tFRxXUw11Fg2BBRqE6vfymyL0dOVLjT0XDzc+v3ZFCkFBJTnbxOll 0Irz9hRizyPU06zReLXQlIeXUfKHHgn9JxHTQi2OeIqwcmxSn3GIdK6hVPpRzEGNvKab /c7/fC2ZwrjcveI6gx3iRWYJzAVupG+Yx5ewsF9WbtTjDZ0PiR7kaIrkNfd8cpQ5nQZa 1mC/rUE40F22ACW+no52Mu4NCU6rou1AR76giVPJacGME7CT/4X97+lT0ZN9fHN6jQ6d QNjw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t22si10832pgu.62.2018.03.19.06.31.49; Mon, 19 Mar 2018 06:32:05 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933186AbeCSNas (ORCPT + 99 others); Mon, 19 Mar 2018 09:30:48 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:60202 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933000AbeCSNap (ORCPT ); Mon, 19 Mar 2018 09:30:45 -0400 Received: from hsi-kbw-5-158-153-52.hsi19.kabel-badenwuerttemberg.de ([5.158.153.52] helo=nanos.tec.linutronix.de) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1exus6-00004t-IL; Mon, 19 Mar 2018 14:30:42 +0100 Date: Mon, 19 Mar 2018 14:30:42 +0100 (CET) From: Thomas Gleixner To: Guo Ren cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, daniel.lezcano@linaro.org, jason@lakedaemon.net, arnd@arndb.de, c-sky_gcc_upstream@c-sky.com, gnu-csky@mentor.com, thomas.petazzoni@bootlin.com, wbx@uclibc-ng.org Subject: Re: [PATCH 19/19] irqchip: add irq-nationalchip.c and irq-csky.c In-Reply-To: <62e687d3eb67505aa6f4d4d01ce268fd432bf58e.1521399976.git.ren_guo@c-sky.com> Message-ID: References: <62e687d3eb67505aa6f4d4d01ce268fd432bf58e.1521399976.git.ren_guo@c-sky.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 19 Mar 2018, Guo Ren wrote: > +static void ck_irq_mask(struct irq_data *d) > +{ > + unsigned int temp, irq; > + > + irq = d->irq; > + > + if (irq < 32) { > + temp = __raw_readl(CK_VA_INTC_NEN31_00); > + temp &= ~(1 << irq); > + __raw_writel(temp, CK_VA_INTC_NEN31_00); > + } else { > + temp = __raw_readl(CK_VA_INTC_NEN63_32); > + temp &= ~(1 << (irq -32)); > + __raw_writel(temp, CK_VA_INTC_NEN63_32); > + } > +} > + > +static void ck_irq_unmask(struct irq_data *d) > +{ > + unsigned int temp, irq; > + > + irq = d->irq; > + > + /* set IFR to support rising edge triggering */ > + if (irq < 32) { > + temp = __raw_readl(CK_VA_INTC_IFR31_00); > + temp &= ~(1 << irq); > + __raw_writel(temp, CK_VA_INTC_IFR31_00); > + } else { > + temp = __raw_readl(CK_VA_INTC_IFR63_32); > + temp &= ~(1 << (irq -32)); > + __raw_writel(temp, CK_VA_INTC_IFR63_32); > + } > + > + if (irq < 32) { > + temp = __raw_readl(CK_VA_INTC_NEN31_00); > + temp |= 1 << irq; > + __raw_writel(temp, CK_VA_INTC_NEN31_00); > + } else { > + temp = __raw_readl(CK_VA_INTC_NEN63_32); > + temp |= 1 << (irq -32); > + __raw_writel(temp, CK_VA_INTC_NEN63_32); > + } > +} > + > +static struct irq_chip ck_irq_chip = { > + .name = "csky_intc_v1", > + .irq_mask = ck_irq_mask, > + .irq_unmask = ck_irq_unmask, > +}; Please use the generic interrupt chip infrastructure for this. > +static unsigned int ck_get_irqno(void) > +{ > + unsigned int temp; newline between declaration and code. > + temp = __raw_readl(CK_VA_INTC_ISR); > + return temp & 0x3f; > +}; > + > +static int __init > +__intc_init(struct device_node *np, struct device_node *parent, bool ave) What is 'ave'? > +{ > + struct irq_domain *root_domain; > + int i; > + > + csky_get_auto_irqno = ck_get_irqno; > + > + if (parent) > + panic("pic not a root intc\n"); > + > + intc_reg = (unsigned int)of_iomap(np, 0); > + if (!intc_reg) > + panic("%s, of_iomap err.\n", __func__); > + > + __raw_writel(0, CK_VA_INTC_NEN31_00); > + __raw_writel(0, CK_VA_INTC_NEN63_32); > + > + if (ave == true) if (ave) > + __raw_writel( 0xc0000000, CK_VA_INTC_ICR); No magic numbers. Please use proper defines. > + else > + __raw_writel( 0x0, CK_VA_INTC_ICR); > + /* > + * csky irq ctrl has 64 sources. > + */ > + #define INTC_IRQS 64 No defines in code. > + for (i=0; i + __raw_writel((i+3)|((i+2)<<8)|((i+1)<<16)|(i<<24), Eew. Tons of magic numbers and a unreadable coding style. Please use an inline function with proper comments to calculate that value > + CK_VA_INTC_SOURCE + i); > + > + root_domain = irq_domain_add_legacy(np, INTC_IRQS, 0, 0, &ck_irq_ops, NULL); > + if (!root_domain) > + panic("root irq domain not available\n"); > + > + irq_set_default_host(root_domain); > + > + return 0; > +} > + > +static int __init > +intc_init(struct device_node *np, struct device_node *parent) > +{ > + Stray newline > + return __intc_init(np, parent, false); > +} > +IRQCHIP_DECLARE(csky_intc_v1, "csky,intc-v1", intc_init); > + > +/* > + * use auto vector exceptions 10 for interrupt. > + */ > +static int __init > +intc_init_ave(struct device_node *np, struct device_node *parent) > +{ > + return __intc_init(np, parent, true); Why is that 'ave' thing not a property of device tree? > +} > +IRQCHIP_DECLARE(csky_intc_v1_ave, "csky,intc-v1,ave", intc_init_ave); > + > +static unsigned int intc_reg; > + > +static void nc_irq_mask(struct irq_data *d) > +{ > + unsigned int mask, irq; > + > + irq = d->irq; > + > + if (irq < 32) { > + mask = __raw_readl(NC_VA_INTC_NMASK31_00); > + mask |= 1 << irq; > + __raw_writel(mask, NC_VA_INTC_NMASK31_00); > + } else { > + mask = __raw_readl(NC_VA_INTC_NMASK63_32); > + mask |= 1 << (irq - 32); > + __raw_writel(mask, NC_VA_INTC_NMASK63_32); > + } > +} > + > +static void nc_irq_unmask(struct irq_data *d) > +{ > + unsigned int mask, irq; > + > + irq = d->irq; > + > + if (irq < 32) { > + mask = __raw_readl(NC_VA_INTC_NMASK31_00); > + mask &= ~( 1 << irq); > + __raw_writel(mask, NC_VA_INTC_NMASK31_00); > + } else { > + mask = __raw_readl( NC_VA_INTC_NMASK63_32); > + mask &= ~(1 << (irq - 32)); > + __raw_writel(mask, NC_VA_INTC_NMASK63_32); > + } > +} > + > +static void nc_irq_en(struct irq_data *d) > +{ > + unsigned int mask, irq; > + > + irq = d->irq; > + > + if (irq < 32) { > + mask = 1 << irq; > + __raw_writel(mask, NC_VA_INTC_NENSET31_00); > + } else { > + mask = 1 << (irq - 32); > + __raw_writel(mask, NC_VA_INTC_NENSET63_32); > + } > + > + nc_irq_unmask(d); > +} > + > +static void nc_irq_dis(struct irq_data *d) > +{ > + unsigned int mask, irq; > + > + irq = d->irq; > + > + if (irq < 32) { > + mask = 1 << irq; > + __raw_writel(mask, NC_VA_INTC_NENCLR31_00); > + } else { > + mask = 1 << (irq - 32); > + __raw_writel(mask, NC_VA_INTC_NENCLR63_32); > + } > + > + nc_irq_mask(d); > +} > + > +struct irq_chip nc_irq_chip = { > + .name = "nationalchip_intc_v1", > + .irq_mask = nc_irq_mask, > + .irq_unmask = nc_irq_unmask, > + .irq_enable = nc_irq_en, > + .irq_disable = nc_irq_dis, > +}; Again. This all can use the generic interrupt chip. > + > +inline int ff1_64(unsigned int hi, unsigned int lo) What on earth means ff1_64? > +{ > + int result; > + asm volatile( > + "ff1 %0\n" > + :"=r"(hi) > + :"r"(hi) > + : > + ); > + > + asm volatile( > + "ff1 %0\n" > + :"=r"(lo) > + :"r"(lo) > + : > + ); So you want to decode the interrupt number from a bitfield. What's wrong with ffs()? > + if( lo != 32 ) > + result = 31-lo; Why is this subtracted? That code makes no sense w/o comments. > + else if( hi != 32 ) result = 31-hi + 32; > + else { > + printk("nc_get_irqno error hi:%x, lo:%x.\n", hi, lo); > + result = NR_IRQS; > + } Pleas use braces consistently. > + return result; > +} > + > +unsigned int nc_get_irqno(void) static? > +{ > + unsigned int nint64hi, nint64lo, irq_no; > + > + nint64lo = __raw_readl(NC_VA_INTC_NINT31_00); > + nint64hi = __raw_readl(NC_VA_INTC_NINT63_32); > + irq_no = ff1_64(nint64hi, nint64lo); > + > + return irq_no; > +} > +static int __init > +intc_init(struct device_node *intc, struct device_node *parent) > +{ > + struct irq_domain *root_domain; > + unsigned int i; > + > + if (parent) > + panic("DeviceTree incore intc not a root irq controller\n"); > + > + csky_get_auto_irqno = nc_get_irqno; > + > + intc_reg = (unsigned int) of_iomap(intc, 0); > + if (!intc_reg) > + panic("Nationalchip Intc Reg: %x.\n", intc_reg); > + > + __raw_writel(0xffffffff, NC_VA_INTC_NENCLR31_00); > + __raw_writel(0xffffffff, NC_VA_INTC_NENCLR63_32); > + __raw_writel(0xffffffff, NC_VA_INTC_NMASK31_00); > + __raw_writel(0xffffffff, NC_VA_INTC_NMASK63_32); > + > + /* > + * nationalchip irq ctrl has 64 sources. > + */ > + #define INTC_IRQS 64 > + for (i=0; i + __raw_writel(i|((i+1)<<8)|((i+2)<<16)|((i+3)<<24), > + NC_VA_INTC_SOURCE + i); Same comments as for the other variant. Thanks, tglx