From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752259AbdJ0ILw (ORCPT ); Fri, 27 Oct 2017 04:11:52 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:35202 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751719AbdJ0ILj (ORCPT ); Fri, 27 Oct 2017 04:11:39 -0400 Subject: Re: [PATCH v2 2/9] irqchip: stm32: add multi-bank management To: Julien Thierry , Thomas Gleixner , Jason Cooper , Marc Zyngier , Rob Herring CC: Maxime Coquelin , Alexandre Torgue , , , References: <1508951465-17152-1-git-send-email-ludovic.Barre@st.com> <1508951465-17152-3-git-send-email-ludovic.Barre@st.com> <04dffa3d-d38d-2bf5-a2e9-a4b549419e67@arm.com> From: Ludovic BARRE Message-ID: <86841672-be73-7730-55b3-dffcd1bf3313@st.com> Date: Fri, 27 Oct 2017 10:10:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <04dffa3d-d38d-2bf5-a2e9-a4b549419e67@arm.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.75.127.49] X-ClientProxiedBy: SFHDAG5NODE2.st.com (10.75.127.14) To SFHDAG6NODE1.st.com (10.75.127.16) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-27_03:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Julien thank for your review On 10/26/2017 04:36 PM, Julien Thierry wrote: > Hi Ludovic, > > On 25/10/17 18:10, Ludovic Barre wrote: >> From: Ludovic Barre >> >> -Prepare to manage multi-bank of external interrupts >> (N banks of 32 inputs). >> -Prepare to manage registers offsets by compatible >> (registers offsets could be different follow per stm32 platform). >> >> Signed-off-by: Ludovic Barre >> --- >>   drivers/irqchip/irq-stm32-exti.c | 151 >> +++++++++++++++++++++++++++------------ >>   1 file changed, 105 insertions(+), 46 deletions(-) >> >> diff --git a/drivers/irqchip/irq-stm32-exti.c >> b/drivers/irqchip/irq-stm32-exti.c >> index 45363ff..6b4109b 100644 >> --- a/drivers/irqchip/irq-stm32-exti.c >> +++ b/drivers/irqchip/irq-stm32-exti.c >> @@ -14,27 +14,66 @@ >>   #include >>   #include >> -#define EXTI_IMR    0x0 >> -#define EXTI_EMR    0x4 >> -#define EXTI_RTSR    0x8 >> -#define EXTI_FTSR    0xc >> -#define EXTI_SWIER    0x10 >> -#define EXTI_PR        0x14 >> +#define IRQS_PER_BANK 32 >> + >> +struct stm32_exti_bank { >> +    u32 imr_ofst; >> +    u32 emr_ofst; >> +    u32 rtsr_ofst; >> +    u32 ftsr_ofst; >> +    u32 swier_ofst; >> +    u32 pr_ofst; >> +}; >> + >> +static const struct stm32_exti_bank stm32f4xx_exti_b1 = { >> +    .imr_ofst    = 0x00, >> +    .emr_ofst    = 0x04, >> +    .rtsr_ofst    = 0x08, >> +    .ftsr_ofst    = 0x0C, >> +    .swier_ofst    = 0x10, >> +    .pr_ofst    = 0x14, >> +}; >> + >> +static const struct stm32_exti_bank *stm32f4xx_exti_banks[] = { >> +    &stm32f4xx_exti_b1, >> +}; >> + >> +static unsigned long stm32_exti_pending(struct irq_chip_generic *gc) >> +{ >> +    const struct stm32_exti_bank *stm32_bank = gc->private; >> + >> +    return irq_reg_readl(gc, stm32_bank->pr_ofst); >> +} >> + >> +static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask) >> +{ >> +    const struct stm32_exti_bank *stm32_bank = gc->private; >> + >> +    irq_reg_writel(gc, mask, stm32_bank->pr_ofst); >> +} >>   static void stm32_irq_handler(struct irq_desc *desc) >>   { >>       struct irq_domain *domain = irq_desc_get_handler_data(desc); >> -    struct irq_chip_generic *gc = domain->gc->gc[0]; >>       struct irq_chip *chip = irq_desc_get_chip(desc); >> +    unsigned int virq, nbanks = domain->gc->num_chips; >> +    struct irq_chip_generic *gc; >> +    const struct stm32_exti_bank *stm32_bank; >>       unsigned long pending; >> -    int n; >> +    int n, i, irq_base = 0; >>       chained_irq_enter(chip, desc); >> -    while ((pending = irq_reg_readl(gc, EXTI_PR))) { >> -        for_each_set_bit(n, &pending, BITS_PER_LONG) { >> -            generic_handle_irq(irq_find_mapping(domain, n)); >> -            irq_reg_writel(gc, BIT(n), EXTI_PR); >> +    for (i = 0; i < nbanks; i++, irq_base += IRQS_PER_BANK) { >> +        gc = irq_get_domain_generic_chip(domain, irq_base); >> +        stm32_bank = gc->private; >> + >> +        while ((pending = stm32_exti_pending(gc))) { >> +            for_each_set_bit(n, &pending, IRQS_PER_BANK) { >> +                virq = irq_find_mapping(domain, irq_base + n); >> +                generic_handle_irq(virq); >> +                stm32_exti_irq_ack(gc, BIT(n)); >> +            } >>           } >>       } >> @@ -44,13 +83,14 @@ static void stm32_irq_handler(struct irq_desc *desc) >>   static int stm32_irq_set_type(struct irq_data *data, unsigned int type) >>   { >>       struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data); >> -    int pin = data->hwirq; >> +    const struct stm32_exti_bank *stm32_bank = gc->private; >> +    int pin = data->hwirq % IRQS_PER_BANK; >>       u32 rtsr, ftsr; >>       irq_gc_lock(gc); >> -    rtsr = irq_reg_readl(gc, EXTI_RTSR); >> -    ftsr = irq_reg_readl(gc, EXTI_FTSR); >> +    rtsr = irq_reg_readl(gc, stm32_bank->rtsr_ofst); >> +    ftsr = irq_reg_readl(gc, stm32_bank->ftsr_ofst); >>       switch (type) { >>       case IRQ_TYPE_EDGE_RISING: >> @@ -70,8 +110,8 @@ static int stm32_irq_set_type(struct irq_data >> *data, unsigned int type) >>           return -EINVAL; >>       } >> -    irq_reg_writel(gc, rtsr, EXTI_RTSR); >> -    irq_reg_writel(gc, ftsr, EXTI_FTSR); >> +    irq_reg_writel(gc, rtsr, stm32_bank->rtsr_ofst); >> +    irq_reg_writel(gc, ftsr, stm32_bank->ftsr_ofst); >>       irq_gc_unlock(gc); >> @@ -81,17 +121,18 @@ static int stm32_irq_set_type(struct irq_data >> *data, unsigned int type) >>   static int stm32_irq_set_wake(struct irq_data *data, unsigned int on) >>   { >>       struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data); >> -    int pin = data->hwirq; >> +    const struct stm32_exti_bank *stm32_bank = gc->private; >> +    int pin = data->hwirq % IRQS_PER_BANK; >>       u32 emr; >>       irq_gc_lock(gc); >> -    emr = irq_reg_readl(gc, EXTI_EMR); >> +    emr = irq_reg_readl(gc, stm32_bank->emr_ofst); >>       if (on) >>           emr |= BIT(pin); >>       else >>           emr &= ~BIT(pin); >> -    irq_reg_writel(gc, emr, EXTI_EMR); >> +    irq_reg_writel(gc, emr, stm32_bank->emr_ofst); >>       irq_gc_unlock(gc); >> @@ -101,11 +142,12 @@ static int stm32_irq_set_wake(struct irq_data >> *data, unsigned int on) >>   static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq, >>                   unsigned int nr_irqs, void *data) >>   { >> -    struct irq_chip_generic *gc = d->gc->gc[0]; >> +    struct irq_chip_generic *gc; >>       struct irq_fwspec *fwspec = data; >>       irq_hw_number_t hwirq; >>       hwirq = fwspec->param[0]; >> +    gc = irq_get_domain_generic_chip(d, hwirq); >>       irq_map_generic_chip(d, virq, hwirq); >>       irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc, >> @@ -129,8 +171,9 @@ struct irq_domain_ops irq_exti_domain_ops = { >>       .free    = stm32_exti_free, >>   }; >> -static int __init stm32_exti_init(struct device_node *node, >> -                  struct device_node *parent) >> +static int >> +__init stm32_exti_init(const struct stm32_exti_bank **stm32_exti_banks, >> +               int bank_nr, struct device_node *node) >>   { >>       unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN; >>       int nr_irqs, nr_exti, ret, i; >> @@ -144,23 +187,16 @@ static int __init stm32_exti_init(struct >> device_node *node, >>           return -ENOMEM; >>       } >> -    /* Determine number of irqs supported */ >> -    writel_relaxed(~0UL, base + EXTI_RTSR); >> -    nr_exti = fls(readl_relaxed(base + EXTI_RTSR)); >> -    writel_relaxed(0, base + EXTI_RTSR); >> - >> -    pr_info("%pOF: %d External IRQs detected\n", node, nr_exti); >> - >> -    domain = irq_domain_add_linear(node, nr_exti, >> +    domain = irq_domain_add_linear(node, bank_nr * IRQS_PER_BANK, >>                          &irq_exti_domain_ops, NULL); >>       if (!domain) { >>           pr_err("%s: Could not register interrupt domain.\n", >> -                node->name); >> +               node->name); >>           ret = -ENOMEM; >>           goto out_unmap; >>       } >> -    ret = irq_alloc_domain_generic_chips(domain, nr_exti, 1, "exti", >> +    ret = irq_alloc_domain_generic_chips(domain, IRQS_PER_BANK, 1, >> "exti", >>                            handle_edge_irq, clr, 0, 0); >>       if (ret) { >>           pr_err("%pOF: Could not allocate generic interrupt chip.\n", >> @@ -168,18 +204,34 @@ static int __init stm32_exti_init(struct >> device_node *node, >>           goto out_free_domain; >>       } >> -    gc = domain->gc->gc[0]; >> -    gc->reg_base                         = base; >> -    gc->chip_types->type               = IRQ_TYPE_EDGE_BOTH; >> -    gc->chip_types->chip.name          = gc->chip_types[0].chip.name; >> -    gc->chip_types->chip.irq_ack       = irq_gc_ack_set_bit; >> -    gc->chip_types->chip.irq_mask      = irq_gc_mask_clr_bit; >> -    gc->chip_types->chip.irq_unmask    = irq_gc_mask_set_bit; >> -    gc->chip_types->chip.irq_set_type  = stm32_irq_set_type; >> -    gc->chip_types->chip.irq_set_wake  = stm32_irq_set_wake; >> -    gc->chip_types->regs.ack           = EXTI_PR; >> -    gc->chip_types->regs.mask          = EXTI_IMR; >> -    gc->chip_types->handler            = handle_edge_irq; >> +    for (i = 0; i < bank_nr; i++) { >> +        const struct stm32_exti_bank *stm32_bank = stm32_exti_banks[i]; >> +        u32 irqs_mask; >> + >> +        gc = irq_get_domain_generic_chip(domain, i * IRQS_PER_BANK); >> + >> +        gc->reg_base = base; >> +        gc->chip_types->type = IRQ_TYPE_EDGE_BOTH; >> +        gc->chip_types->chip.name = gc->chip_types[0].chip.name; > > I might be missing something, but what is the point of this line? indeed these 2 lines: gc->chip_types->chip.name = gc->chip_types[0].chip.name gc->chip_types->handler = handle_edge_irq; are not needed, it's done in irq_alloc_domain_generic_chips > >> +        gc->chip_types->chip.irq_ack = irq_gc_ack_set_bit; >> +        gc->chip_types->chip.irq_mask = irq_gc_mask_clr_bit; >> +        gc->chip_types->chip.irq_unmask = irq_gc_mask_set_bit; >> +        gc->chip_types->chip.irq_set_type = stm32_irq_set_type; >> +        gc->chip_types->chip.irq_set_wake = stm32_irq_set_wake; >> +        gc->chip_types->regs.ack = stm32_bank->pr_ofst; >> +        gc->chip_types->regs.mask = stm32_bank->imr_ofst; >> +        gc->chip_types->handler = handle_edge_irq; > > I believe this is already done by irq_alloc_domain_generic_chips. > > Thanks, >