Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753120AbbDFKp5 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 6 Apr 2015 06:45:57 -0400
Received: from inca-roads.misterjones.org ([213.251.177.50]:59515 "EHLO
	inca-roads.misterjones.org" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1752654AbbDFKpz (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 6 Apr 2015 06:45:55 -0400
X-Greylist: delayed 1345 seconds by postgrey-1.27 at vger.kernel.org; Mon, 06 Apr 2015 06:45:54 EDT
To: Geert Uytterhoeven <geert+renesas@glider.be>
Subject: Re: [PATCH/RFC 3/6] staging: board: Add support for translating  hwirq to virq numbers
X-PHP-Originating-Script: 0:main.inc
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Mon, 06 Apr 2015 11:45:44 +0100
From: Marc Zyngier <marc.zyngier@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Magnus Damm <damm+renesas@opensource.se>,
        Arnd Bergmann <arnd@arndb.de>,
        Simon Horman <horms+renesas@verge.net.au>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        <devel@driverdev.osuosl.org>, <devicetree@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <linux-sh@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>
Organization: ARM Ltd
In-Reply-To: <1428064923-24950-4-git-send-email-geert+renesas@glider.be>
References: <1428064923-24950-1-git-send-email-geert+renesas@glider.be>
 <1428064923-24950-4-git-send-email-geert+renesas@glider.be>
Message-ID: <7a82f5d435aaaf8d121233769a77943c@www.loen.fr>
User-Agent: Roundcube Webmail/0.7.2
X-SA-Exim-Connect-IP: <locally generated>
X-SA-Exim-Rcpt-To: geert+renesas@glider.be, gregkh@linuxfoundation.org, damm+renesas@opensource.se, arnd@arndb.de, horms+renesas@verge.net.au, laurent.pinchart@ideasonboard.com, kuninori.morimoto.gx@renesas.com, devel@driverdev.osuosl.org, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
X-SA-Exim-Mail-From: marc.zyngier@arm.com
X-SA-Exim-Scanned: No (on cheepnis.misterjones.org); SAEximRunCond expanded to false
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4767
Lines: 160

On 2015-04-03 13:42, Geert Uytterhoeven wrote:
> As of commit 9a1091ef0017c40a ("irqchip: gic: Support hierarchy irq
> domain."), GIC IRQ numbers are virtual, breaking hardcoded hardware 
> IRQ
> numbers in platform device resources.
>
> Add support for translating hardware IRQ numbers to virtual IRQ 
> numbers,
> and fixing up platform device resources with hardcoded IRQ numbers.
>
> Add a copyright header, including the original author.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/staging/board/board.c | 66
> +++++++++++++++++++++++++++++++++++++++++++
>  drivers/staging/board/board.h |  5 ++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/drivers/staging/board/board.c 
> b/drivers/staging/board/board.c
> index d5a6abc845191c93..b84ac2837a20bf06 100644
> --- a/drivers/staging/board/board.c
> +++ b/drivers/staging/board/board.c
> @@ -1,10 +1,27 @@
> +/*
> + * Copyright (C) 2014 Magnus Damm
> + * Copyright (C) 2015 Glider bvba
> + *
> + * This file is subject to the terms and conditions of the GNU
> General Public
> + * License.  See the file "COPYING" in the main directory of this 
> archive
> + * for more details.
> + */
> +
> +#define pr_fmt(fmt)	"board_staging: "  fmt
> +
>  #include <linux/init.h>
> +#include <linux/irq.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
>  #include "board.h"
>
> +static struct device_node *irqc_node __initdata;
> +static unsigned int irqc_base __initdata;
> +
>  static bool find_by_address(u64 base_address)
>  {
>  	struct device_node *dn = of_find_all_nodes(NULL);
> @@ -38,3 +55,52 @@ bool __init board_staging_dt_node_available(const
> struct resource *resource,
>
>  	return false; /* Nothing found */
>  }
> +
> +int __init board_staging_setup_hwirq_xlate(const char *irqc_match,
> +					   unsigned int base)
> +{
> +	irqc_node = of_find_compatible_node(NULL, NULL, irqc_match);
> +
> +	WARN_ON(!irqc_node);
> +	if (!irqc_node)
> +		return -ENOENT;
> +
> +	irqc_base = base;
> +	return 0;
> +}

And what happens when you have multiple (cascaded) interrupt 
controllers, each wanting to register with this translation service? You 
should at least check that nobody has been here before.

> +static unsigned int __init xlate_hwirq(unsigned int hwirq)
> +{
> +	struct of_phandle_args irq_data;
> +	unsigned int irq;
> +
> +	if (!irqc_node)
> +		return hwirq;
> +
> +	irq_data.np = irqc_node;
> +	irq_data.args_count = 3;
> +	irq_data.args[0] = 0;
> +	irq_data.args[1] = hwirq - irqc_base;
> +	irq_data.args[2] = IRQ_TYPE_LEVEL_HIGH;

And what happens for edge interrupts? Or LEVEL_LOW? This is very much 
GIC specific (3 args...). How does it work with non-GIC interrupt 
controllers?

> +
> +	irq = irq_create_of_mapping(&irq_data);
> +	if (WARN_ON(!irq))
> +		irq = hwirq;
> +
> +	return irq;
> +}
> +
> +void __init board_staging_fixup_irq_resources(struct resource *res,
> +					      unsigned int nres)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < nres; i++)
> +		if (res[i].flags == IORESOURCE_IRQ) {
> +			unsigned int hwirq = res[i].start;
> +			unsigned int virq = xlate_hwirq(hwirq);
> +
> +			pr_debug("hwirq %u -> virq %u\n", hwirq, virq);
> +			res[i].start = virq;
> +		}
> +}
> diff --git a/drivers/staging/board/board.h 
> b/drivers/staging/board/board.h
> index e9c914985d4acb36..4cedc3c46e287eb7 100644
> --- a/drivers/staging/board/board.h
> +++ b/drivers/staging/board/board.h
> @@ -1,10 +1,15 @@
>  #ifndef __BOARD_H__
>  #define __BOARD_H__
> +
>  #include <linux/init.h>
>  #include <linux/of.h>
>
> +struct resource;
> +
>  bool board_staging_dt_node_available(const struct resource 
> *resource,
>  				     unsigned int num_resources);
> +int board_staging_setup_hwirq_xlate(const char *irqc_match, unsigned
> int base);
> +void board_staging_fixup_irq_resources(struct resource *res,
> unsigned int nres);
>
>  #define board_staging(str, fn)			\
>  static int __init runtime_board_check(void)	\

It won't surprise you that I don't really like this approach. It is 
controller-specific, restrictive, and allows platform code to continue 
doing something that is essentially wrong. Should this code ever move 
out of staging, it should depend on CONFIG_BROKEN, because this is 
essentially what it is - support code for broken systems. I'd also 
welcome moving parts of the OMAP4/5 code to such CONFIG_BROKEN 
dependency.

Thanks,

         M.
-- 
Fast, cheap, reliable. Pick two.
--
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/