Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch01.xlnx.xilinx.com;
Envelope-to: git@xilinx.com,
 maz@kernel.org,
 linux-kernel@vger.kernel.org,
 monstr@monstr.eu,
 robert.hancock@calian.com,
 tglx@linutronix.de,
 linux-arm-kernel@lists.infradead.org
Message-ID: <88f39791-2154-8df4-c95e-f416f83c20fb@xilinx.com>
Date:   Fri, 4 Mar 2022 08:51:57 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH] irqchip: xilinx: Enable generic irq multi handler
Content-Language: en-US
To:     Marc Zyngier <maz@kernel.org>,
        Michal Simek <michal.simek@xilinx.com>
CC:     <linux-kernel@vger.kernel.org>, <monstr@monstr.eu>,
        <git@xilinx.com>, Robert Hancock <robert.hancock@calian.com>,
        Stefan Asserhall <stefan.asserhall@xilinx.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        <linux-arm-kernel@lists.infradead.org>
References: <2a08d6c33e95d5da5d564ed3fbddc835983ef355.1646323896.git.michal.simek@xilinx.com>
 <87sfrz0x1p.wl-maz@kernel.org>
From:   Michal Simek <michal.simek@xilinx.com>
In-Reply-To: <87sfrz0x1p.wl-maz@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2022 07:52:01.7295
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 86091d1b-1ebf-4695-05ac-08d9fdb3de2f
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch01.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: DM3NAM02FT007.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR02MB8594
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/3/22 18:24, Marc Zyngier wrote:
> On Thu, 03 Mar 2022 16:11:39 +0000,
> Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Register default arch handler via driver instead of directly pointing to
>> xilinx intc controller. This patch makes architecture code more generic.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-by: Stefan Asserhall <stefan.asserhall@xilinx.com>
>> ---
>>
>>   arch/microblaze/Kconfig           |  2 ++
>>   arch/microblaze/include/asm/irq.h |  3 ---
>>   arch/microblaze/kernel/irq.c      | 16 +---------------
>>   drivers/irqchip/irq-xilinx-intc.c | 22 +++++++++++++++++++++-
>>   4 files changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
>> index 59798e43cdb0..da568e981604 100644
>> --- a/arch/microblaze/Kconfig
>> +++ b/arch/microblaze/Kconfig
>> @@ -45,6 +45,8 @@ config MICROBLAZE
>>   	select SET_FS
>>   	select ZONE_DMA
>>   	select TRACE_IRQFLAGS_SUPPORT
>> +	select GENERIC_IRQ_MULTI_HANDLER
>> +	select HANDLE_DOMAIN_IRQ
>>   
>>   # Endianness selection
>>   choice
>> diff --git a/arch/microblaze/include/asm/irq.h b/arch/microblaze/include/asm/irq.h
>> index 0a28e80bbab0..cb6ab55d1d01 100644
>> --- a/arch/microblaze/include/asm/irq.h
>> +++ b/arch/microblaze/include/asm/irq.h
>> @@ -11,7 +11,4 @@
>>   struct pt_regs;
>>   extern void do_IRQ(struct pt_regs *regs);
>>   
>> -/* should be defined in each interrupt controller driver */
>> -extern unsigned int xintc_get_irq(void);
>> -
>>   #endif /* _ASM_MICROBLAZE_IRQ_H */
>> diff --git a/arch/microblaze/kernel/irq.c b/arch/microblaze/kernel/irq.c
>> index 903dad822fad..1f8cb4c4f74f 100644
>> --- a/arch/microblaze/kernel/irq.c
>> +++ b/arch/microblaze/kernel/irq.c
>> @@ -20,27 +20,13 @@
>>   #include <linux/irqchip.h>
>>   #include <linux/of_irq.h>
>>   
>> -static u32 concurrent_irq;
>> -
>>   void __irq_entry do_IRQ(struct pt_regs *regs)
>>   {
>> -	unsigned int irq;
>>   	struct pt_regs *old_regs = set_irq_regs(regs);
>>   	trace_hardirqs_off();
>>   
>>   	irq_enter();
>> -	irq = xintc_get_irq();
>> -next_irq:
>> -	BUG_ON(!irq);
>> -	generic_handle_irq(irq);
>> -
>> -	irq = xintc_get_irq();
>> -	if (irq != -1U) {
>> -		pr_debug("next irq: %d\n", irq);
>> -		++concurrent_irq;
>> -		goto next_irq;
>> -	}
>> -
>> +	handle_arch_irq(regs);
>>   	irq_exit();
>>   	set_irq_regs(old_regs);
>>   	trace_hardirqs_on();
>> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
>> index 356a59755d63..c6710190c152 100644
>> --- a/drivers/irqchip/irq-xilinx-intc.c
>> +++ b/drivers/irqchip/irq-xilinx-intc.c
>> @@ -110,7 +110,7 @@ static struct irq_chip intc_dev = {
>>   	.irq_mask_ack = intc_mask_ack,
>>   };
>>   
>> -unsigned int xintc_get_irq(void)
>> +static unsigned int xintc_get_irq(void)
>>   {
>>   	unsigned int irq = -1;
>>   	u32 hwirq;
>> @@ -164,6 +164,25 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
>>   	chained_irq_exit(chip, desc);
>>   }
>>   
>> +static u32 concurrent_irq;
> 
> Please kill this. It serves no purpose at all.
> 
>> +
>> +static void xil_intc_handle_irq(struct pt_regs *regs)
>> +{
>> +	unsigned int irq;
>> +
>> +	irq = xintc_get_irq();
>> +next_irq:
>> +	BUG_ON(!irq);
>> +	generic_handle_irq(irq);
>> +
>> +	irq = xintc_get_irq();
>> +	if (irq != -1U) {
>> +		pr_debug("next irq: %d\n", irq);
>> +		++concurrent_irq;
>> +		goto next_irq;
>> +	}
> 
> How about writing this in (basic) C code, and use the exiting APIs?
> 
> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> index 356a59755d63..18125bb841b2 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -110,18 +110,19 @@ static struct irq_chip intc_dev = {
>   	.irq_mask_ack = intc_mask_ack,
>   };
>   
> -unsigned int xintc_get_irq(void)
> +#define SPURIOUS_IRQ	(-1U)
> +
> +static void xil_intc_handle_irq(struct pt_regs *regs)
>   {
> -	unsigned int irq = -1;
>   	u32 hwirq;
>   
> -	hwirq = xintc_read(primary_intc, IVR);
> -	if (hwirq != -1U)
> -		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> -
> -	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +	do {
> +		hwirq = xintc_read(primary_intc, IVR);
> +		if (unlikely(hwirq == SPURIOUS_IRQ))
> +			break;
>   
> -	return irq;
> +		generic_handle_domain_irq(primary_intc->root_domain, hwirq);
> +	} while (true);
>   }
>   
>   static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> 
> 	M.
> 

That works. Sent v2.

Thanks,
Michal