linux-kernel - Re: [PATCH 1/3] ARM: at91/aic: add device tree support for AIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ED4E4A7.80603@atmel.com>
Date:	Tue, 29 Nov 2011 14:56:55 +0100
From:	Nicolas Ferre <nicolas.ferre@...el.com>
To:	Jamie Iles <jamie@...ieiles.com>
CC:	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
Subject: Re: [PATCH 1/3] ARM: at91/aic: add device tree support for AIC

On 11/29/2011 02:04 PM, Nicolas Ferre :
> On 11/25/2011 04:28 PM, Jamie Iles :
>> On Fri, Nov 25, 2011 at 02:51:06PM +0100, Jean-Christophe
>> PLAGNIOL-VILLARD wrote:
>>> On 22:26 Thu 24 Nov , Jamie Iles wrote:
>>>> Hi Nicolas,
>>>> On Thu, Nov 24, 2011 at 10:56:27PM +0100, Nicolas Ferre wrote:
>> [...]
>>>>> +#if defined(CONFIG_OF)
>>>>> +static struct of_device_id aic_ids[] = {
>>>>> + { .compatible = "atmel,at91rm9200-aic" },
>>>>> + { /*sentinel*/ }
>>>>> +};
>>>>> +
>>>>> +static int __init at91_aic_of_init(void)
>>>>> +{
>>>>> + struct device_node *np;
>>>>> +
>>>>> + np = of_find_matching_node(NULL, aic_ids);
>>>>> + if (np == NULL)
>>>>> + return -ENODEV;
>>>>> +
>>>>> + at91_aic_base = of_iomap(np, 0);
>>>>> + at91_aic_domain.of_node = np;
>>>>
>>>> I think this needs to be:
>>>>
>>>> at91_aic_domain.of_node = of_node_get(np);
>>>>
>>>> to keep the reference count.
>
> Well, in fact the of_find_matching_node() function already indent the
> ref. count...
>
>>>>> + /* Keep refcount of the node */
>
> ... That is why I added this comment ^^
>
> But maybe for sake of clarity, I may have used what you propose anyway.
> What it your opinion?
>
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +#else
>>>>> +static int __init at91_aic_of_init(void)
>>>>> +{
>>>>> + return -ENOSYS;
>>>>> +}
>>>>> +#endif
>>>>
>>>> I think it's preferred if you use of_irq_init() here as it can handle
>>>> the ordering of IRQ controllers. There are GIC and VIC bindings in
>>>> -next that use this and provide a way for non-DT platforms to still use
>>>> the drivers.
>>> which is the case here as if the of_init fail we failback to the
>>> non-dt init
>>>
>>> and this IP is AT91 only
>>
>> Right, but it's not using the of_irq_init() interface which is the
>> standard way of registering interrupt controllers and will correctly
>> dependencies for you.
>>
>> So if you could have something like:
>>
>> void __init __at91_aic_init(unsigned int priority[NR_AIC_IRQS],
>> void __iomem *regs,
>> struct device_node *np)
>> {
>> /*
>> * Do all of the writes to the AIC itself and configure
>> * the IRQ domain.
>> */
>> }
>>
>> void __init at91_aic_init(unsigned int priority[NR_AIC_IRQS])
>> {
>> void __iomem *base = ioremap(AT91_AIC, 512);
>>
>> __at91_aic_init(priority, base, NULL);
>> }
>>
>> int __init at91_aic_of_init(struct device_node *node,
>> struct device_node *parent)
>> {
>> void __iomem *regs = of_iomap(node, 0);
>>
>> /*
>> * Get priorities from the DT. If this was an array of cells
>> * then that should be okay.
>> */
>> __at91_aic_init(dt_priorities, regs, node);
>> }
>>
>> Then the DT based board initialisation can do:
>>
>> static const struct of_device_id at91_irq_of_match[] __initconst = {
>> { .compatible = "atmel,at91-aic", .data = at91_aic_of_init },
>> {}
>> };
>>
>> static void __init at91_of_irq_init(void)
>> {
>> of_irq_init(at91_of_irq_init);
>> }
>
> That looks nice. I will try to implement this. I will try to figure out
> when of_irq_init() is called compared to the other init_IRQ() function.
>
>> Which is consistent with other platforms. However this does require
>> that the priorities are encoded in the device-tree, but I guess that's a
>> good thing anyway isn't it?
>
> That is a annoying point: I do not want to add all this "default"
> priority stuff in the DT. It is kind of useless until we use the
> threaded interrupts everywhere and may bloat the DT...
>
> I will try to find a way to pass the default priority table to the DT
> called function.

Well, I have better understood the way that all this functions are 
called. And I tend to find it not so nice: I prefer to have all this irq 
controller related details included in the driver itself.
So, I keep the basic structure of my first attempt but will correct it 
using the of_irq_init() API.

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/