Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934039AbbKSKxY (ORCPT ); Thu, 19 Nov 2015 05:53:24 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:27707 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757512AbbKSKxW (ORCPT ); Thu, 19 Nov 2015 05:53:22 -0500 Subject: Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation functions To: Marc Zyngier References: <1446798522-28000-1-git-send-email-majun258@huawei.com> <1446798522-28000-5-git-send-email-majun258@huawei.com> <20151119094127.7db3522c@arm.com> CC: , , , , , , , , , , , , , , , , , , , , , From: "majun (F)" Message-ID: <564DAA0D.80200@huawei.com> Date: Thu, 19 Nov 2015 18:53:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20151119094127.7db3522c@arm.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.235.245] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090201.564DAA1F.0014,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: d432f8616193a6d9cad1238323536f3b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5137 Lines: 179 Hi Marc: 在 2015/11/19 17:41, Marc Zyngier 写道: > On Fri, 6 Nov 2015 16:28:42 +0800 > MaJun wrote: > >> From: Ma Jun >> [...] >> struct mbigen_irq_data { >> void __iomem *base; >> + unsigned int pin_offset; >> unsigned int reg_vec; >> + unsigned int reg_type; >> + unsigned int reg_clear; >> + unsigned int type; >> }; > > I have the same comments here as for patch #3. You're storing > information that are just a pure function of hwirq, and essentially > free to compute at runtime. Please fix it in a similar way. > ok, I'll fix this. >> >> static inline int get_mbigen_vec_reg(u32 nid, u32 offset) >> @@ -77,8 +98,68 @@ static inline int get_mbigen_vec_reg(u32 nid, u32 offset) >> + REG_MBIGEN_VEC_OFFSET; >> } >> >> +static int get_mbigen_type_reg(u32 nid, u32 offset) >> +{ >> + int ofst; >> + >> + ofst = offset / 32 * 4; >> + return ofst + nid * MBIGEN_NODE_OFFSET >> + + REG_MBIGEN_TYPE_OFFSET; >> +} >> + >> +static int get_mbigen_clear_reg(u32 nid, u32 offset) >> +{ >> + int ofst; >> + >> + ofst = offset / 32 * 4; >> + return ofst + nid * MBIGEN_NODE_OFFSET >> + + REG_MBIGEN_CLEAR_OFFSET; >> +} >> + >> +static void mbigen_eoi_irq(struct irq_data *data) >> +{ >> + struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data); >> + u32 mask; >> + >> + /* only level triggered interrupt need to clear status */ >> + if (mgn_irq_data->type == IRQ_TYPE_LEVEL_HIGH) { >> + mask = 1 << (mgn_irq_data->pin_offset % 32); >> + writel_relaxed(mask, mgn_irq_data->reg_clear + mgn_irq_data->base); >> + } > > Does this have the effect of regenerating an edge if the level is still > active? > yes, it does. >> + >> + irq_chip_eoi_parent(data); >> +} >> + >> +static int mbigen_set_type(struct irq_data *d, unsigned int type) >> +{ >> + struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(d); >> + u32 mask; >> + int val; >> + >> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) >> + return -EINVAL; > > If these are the only two interrupt triggers you support, then you > should update the documentation (DT binding) to reflect this, as all it > says is "The 2nd cell is the interrupt trigger type" which is pretty > vague. > I'll specify this point in DT binding. Thanks Ma Jun >> + >> + mask = 1 << (mgn_irq_data->pin_offset % 32); >> + >> + val = readl_relaxed(mgn_irq_data->reg_type + mgn_irq_data->base); >> + >> + if (type == IRQ_TYPE_LEVEL_HIGH) >> + val |= mask; >> + else if (type == IRQ_TYPE_EDGE_RISING) >> + val &= ~mask; > > Given that you've excluded anything but LEVEL_HIGH and EDGE_RISING > already, the second if() is superfluous. > >> + >> + writel_relaxed(val, mgn_irq_data->reg_type + mgn_irq_data->base); >> + >> + return 0; >> +} >> + >> static struct irq_chip mbigen_irq_chip = { >> .name = "mbigen-v2", >> + .irq_mask = irq_chip_mask_parent, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_eoi = mbigen_eoi_irq, >> + .irq_set_type = mbigen_set_type, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> }; >> >> static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) >> @@ -94,10 +175,11 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) >> writel_relaxed(val, mgn_irq_data->reg_vec + mgn_irq_data->base); >> } >> >> -static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq) >> +static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq, >> + unsigned int type) >> { >> struct mbigen_irq_data *datap; >> - unsigned int nid, pin_offset; >> + unsigned int nid; >> >> datap = kzalloc(sizeof(*datap), GFP_KERNEL); >> if (!datap) >> @@ -106,10 +188,20 @@ static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq) >> /* get the mbigen node number */ >> nid = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) / IRQS_PER_MBIGEN_NODE + 1; >> >> - pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) >> + datap->pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) >> % IRQS_PER_MBIGEN_NODE; >> >> - datap->reg_vec = get_mbigen_vec_reg(nid, pin_offset); >> + datap->reg_vec = get_mbigen_vec_reg(nid, datap->pin_offset); >> + datap->reg_type = get_mbigen_type_reg(nid, datap->pin_offset); >> + >> + /* no clear register for edge triggered interrupt */ >> + if (type == IRQ_TYPE_EDGE_RISING) >> + datap->reg_clear = 0; >> + else >> + datap->reg_clear = get_mbigen_clear_reg(nid, >> + datap->pin_offset); >> + >> + datap->type = type; >> return datap; >> } > > That function can entirely go. > >> >> @@ -151,7 +243,7 @@ static int mbigen_irq_domain_alloc(struct irq_domain *domain, >> return err; >> >> /* set related information of this irq */ >> - mgn_irq_data = set_mbigen_irq_data(hwirq); >> + mgn_irq_data = set_mbigen_irq_data(hwirq, type); >> if (!mgn_irq_data) >> return err; >> > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/