All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hanjun Guo <hanjun.guo@linaro.org>
To: Graeme Gregory <gg@slimlogic.co.uk>, Marc Zyngier <marc.zyngier@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Jason Cooper <jason@lakedaemon.net>,
	Will Deacon <Will.Deacon@arm.com>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jiang Liu <jiang.liu@linux.intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Tomasz Nowicki <tomasz.nowicki@linaro.org>,
	"grant.likely@linaro.org" <grant.likely@linaro.org>,
	Olof Johansson <olof@lixom.net>, Wei Huang <wei@redhat.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linaro-acpi@lists.linaro.org" <linaro-acpi@lists.linaro.org>
Subject: Re: [PATCH v2 6/9] irqchip / gic: Add stacked irqdomain support for ACPI based GICv2 init
Date: Fri, 03 Jul 2015 16:47:44 +0800	[thread overview]
Message-ID: <55964C30.7020208@linaro.org> (raw)
In-Reply-To: <20150630150719.GC12142@xora-haswell.xora.org.uk>

On 06/30/2015 11:07 PM, Graeme Gregory wrote:
> On Tue, Jun 30, 2015 at 01:17:14PM +0100, Marc Zyngier wrote:
>> On 30/06/15 12:50, Hanjun Guo wrote:
>>> Hi Marc,
>>>
>>> On 06/29/2015 04:39 PM, Marc Zyngier wrote:
>>>> On 27/06/15 04:52, Hanjun Guo wrote:
>>>>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote:
>>>>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote:
>>> [...]
>>>>>>
>>>>>>>>>      	for (i = 0; i < nr_irqs; i++)
>>>>>>>>>      		gic_irq_domain_map(domain, virq + i, hwirq + i);
>>>>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>>>>>>>      		gic_irqs = 1020;
>>>>>>>>>      	gic->gic_irqs = gic_irqs;
>>>>>>>>>
>>>>>>>>> -	if (node) {		/* DT case */
>>>>>>>>> +	if (node || !acpi_disabled) {		/* DT or ACPI case */
>>>>>>>>>      		gic->domain = irq_domain_add_linear(node, gic_irqs,
>>>>>>>>>      						    &gic_irq_domain_hierarchy_ops,
>>>>>>>>>      						    gic);
>>>>>>
>>>>>> I think this is a bit more worrying, I mean passing a NULL node pointer to
>>>>>> the irqdomain layer which basically means you are booting out of ACPI
>>>>>
>>>>> I'm little confused here, would you mind explaining more for your
>>>>> worrying? To me, node pointer is optional and it's ok for ACPI
>>>>> case.
>>>>>
>>>>>> (for you, if that's true for the irq_domain_add_linear implementation
>>>>>> that's another story), the node pointer should be optional but you
>>>>>> need feedback from IRQ layer maintainers here.
>>>>>
>>>>> Sure.
>>>>
>>>> Frankly, I'd really like to see ACPI using the "node" parameter for
>>>> something useful. This would save having to cache pointers all over the
>>>> place, will make find_irq_host() work as expected... etc.
>>>>
>>>> See the comment at the top of linux/irqdomain.h :
>>>>
>>>> "... This code could thus be used on other architectures by replacing
>>>> those two by some sort of arch-specific void * "token" used to identify
>>>> interrupt controllers."
>>>
>>> To init GIC in ACPI, we can only use the table entry pointer as
>>> the token, but the ACPI static tables are early mem/io remapped
>>> memory at boot stage, and it will be not available after boot,
>>> also we need muti types of MADT enties to init GIC (GICC and GICD
>>> for GICv2, GICC or GICR and GICD for GICv3), not as DT, just
>>> one single node to include all the information needed to init
>>> the GIC.
>>
>> A single pointer would be enough, you don't need all of them.
>>
>>> We use ACPI handle for devices as node for DT when the namespace
>>> is available, but that's pretty late in the boot stage which GIC,
>>> SMP and timers were already initialized, so ACPI handle can not
>>> use as the token too.
>>>
>>> I see multi places just pass NULL as the pointer directly for
>>> irq_domain_add_linear() which works fine, and for ACPI, we tested
>>> this patch and also it works.
>>
>> Yes it works. But you're reinventing the wheel by keeping references
>> outside of the normal framework, which is simply going to make the code
>> more difficult to maintain in the long run.
>>
>> Putting NULL as the device_node parameter really means "this is a domain
>> I don't need to look up later". In your case, you will have to lookup
>> that domain, all the time. You're just doing it in your own little
>> corner, which is what bothers me.
>>
> Hanjun, I think it should be possible that instead of looking up the
> domains in our own bit of code. We can instead use a ptr to the
> appropriate information as the token instead.
>
> I don't think we have to replicate the behaviour of node, in the DT
> case, but just do what is sensible for ACPI in this case.

I'm trying to introduce a pointer which point to the GICD when GIC
is initialized, this pointer will be used to match the irqdomain.
In driver code, I will find this pointer with the gsi, then find the
irqdomain match the pointer, then I hope I can remove acpi_irq_domain
in the end.

Marc, beyond this issue, do you have any more comments about this
patch set? the framework of init GIC? and the code to init GICV3?
if yes, I can address it along with this one together.

Thanks
Hanjun

WARNING: multiple messages have this Message-ID (diff)
From: hanjun.guo@linaro.org (Hanjun Guo)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 6/9] irqchip / gic: Add stacked irqdomain support for ACPI based GICv2 init
Date: Fri, 03 Jul 2015 16:47:44 +0800	[thread overview]
Message-ID: <55964C30.7020208@linaro.org> (raw)
In-Reply-To: <20150630150719.GC12142@xora-haswell.xora.org.uk>

On 06/30/2015 11:07 PM, Graeme Gregory wrote:
> On Tue, Jun 30, 2015 at 01:17:14PM +0100, Marc Zyngier wrote:
>> On 30/06/15 12:50, Hanjun Guo wrote:
>>> Hi Marc,
>>>
>>> On 06/29/2015 04:39 PM, Marc Zyngier wrote:
>>>> On 27/06/15 04:52, Hanjun Guo wrote:
>>>>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote:
>>>>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote:
>>> [...]
>>>>>>
>>>>>>>>>      	for (i = 0; i < nr_irqs; i++)
>>>>>>>>>      		gic_irq_domain_map(domain, virq + i, hwirq + i);
>>>>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>>>>>>>      		gic_irqs = 1020;
>>>>>>>>>      	gic->gic_irqs = gic_irqs;
>>>>>>>>>
>>>>>>>>> -	if (node) {		/* DT case */
>>>>>>>>> +	if (node || !acpi_disabled) {		/* DT or ACPI case */
>>>>>>>>>      		gic->domain = irq_domain_add_linear(node, gic_irqs,
>>>>>>>>>      						    &gic_irq_domain_hierarchy_ops,
>>>>>>>>>      						    gic);
>>>>>>
>>>>>> I think this is a bit more worrying, I mean passing a NULL node pointer to
>>>>>> the irqdomain layer which basically means you are booting out of ACPI
>>>>>
>>>>> I'm little confused here, would you mind explaining more for your
>>>>> worrying? To me, node pointer is optional and it's ok for ACPI
>>>>> case.
>>>>>
>>>>>> (for you, if that's true for the irq_domain_add_linear implementation
>>>>>> that's another story), the node pointer should be optional but you
>>>>>> need feedback from IRQ layer maintainers here.
>>>>>
>>>>> Sure.
>>>>
>>>> Frankly, I'd really like to see ACPI using the "node" parameter for
>>>> something useful. This would save having to cache pointers all over the
>>>> place, will make find_irq_host() work as expected... etc.
>>>>
>>>> See the comment at the top of linux/irqdomain.h :
>>>>
>>>> "... This code could thus be used on other architectures by replacing
>>>> those two by some sort of arch-specific void * "token" used to identify
>>>> interrupt controllers."
>>>
>>> To init GIC in ACPI, we can only use the table entry pointer as
>>> the token, but the ACPI static tables are early mem/io remapped
>>> memory at boot stage, and it will be not available after boot,
>>> also we need muti types of MADT enties to init GIC (GICC and GICD
>>> for GICv2, GICC or GICR and GICD for GICv3), not as DT, just
>>> one single node to include all the information needed to init
>>> the GIC.
>>
>> A single pointer would be enough, you don't need all of them.
>>
>>> We use ACPI handle for devices as node for DT when the namespace
>>> is available, but that's pretty late in the boot stage which GIC,
>>> SMP and timers were already initialized, so ACPI handle can not
>>> use as the token too.
>>>
>>> I see multi places just pass NULL as the pointer directly for
>>> irq_domain_add_linear() which works fine, and for ACPI, we tested
>>> this patch and also it works.
>>
>> Yes it works. But you're reinventing the wheel by keeping references
>> outside of the normal framework, which is simply going to make the code
>> more difficult to maintain in the long run.
>>
>> Putting NULL as the device_node parameter really means "this is a domain
>> I don't need to look up later". In your case, you will have to lookup
>> that domain, all the time. You're just doing it in your own little
>> corner, which is what bothers me.
>>
> Hanjun, I think it should be possible that instead of looking up the
> domains in our own bit of code. We can instead use a ptr to the
> appropriate information as the token instead.
>
> I don't think we have to replicate the behaviour of node, in the DT
> case, but just do what is sensible for ACPI in this case.

I'm trying to introduce a pointer which point to the GICD when GIC
is initialized, this pointer will be used to match the irqdomain.
In driver code, I will find this pointer with the gsi, then find the
irqdomain match the pointer, then I hope I can remove acpi_irq_domain
in the end.

Marc, beyond this issue, do you have any more comments about this
patch set? the framework of init GIC? and the code to init GICV3?
if yes, I can address it along with this one together.

Thanks
Hanjun

  reply	other threads:[~2015-07-03  8:47 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-19  8:46 [PATCH v2 0/9] Add self-probe infrastructure and stacked irqdomain support for ACPI based GICv2/3 init Hanjun Guo
2015-06-19  8:46 ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 1/9] ACPICA: ACPI 6.0: Add changes for MADT table Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 2/9] ACPICA: ACPI 6.0: Add values for MADT GIC version field Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 3/9] irqchip / GIC: Add GIC version support in ACPI MADT Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-22 16:45   ` Lorenzo Pieralisi
2015-06-22 16:45     ` Lorenzo Pieralisi
2015-06-22 16:45     ` Lorenzo Pieralisi
2015-06-23 11:18     ` Hanjun Guo
2015-06-23 11:18       ` Hanjun Guo
2015-06-23 11:18       ` Hanjun Guo
2015-06-27  6:07     ` Hanjun Guo
2015-06-27  6:07       ` Hanjun Guo
2015-06-27  6:07       ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 4/9] ACPI / irqchip: Add self-probe infrastructure to initialize IRQ controller Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 5/9] irqchip / GIC / ACPI: Use IRQCHIP_ACPI_DECLARE to simplify GICv2 init code Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 6/9] irqchip / gic: Add stacked irqdomain support for ACPI based GICv2 init Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-22 17:20   ` Lorenzo Pieralisi
2015-06-22 17:20     ` Lorenzo Pieralisi
2015-06-22 17:20     ` Lorenzo Pieralisi
2015-06-23 15:11     ` Hanjun Guo
2015-06-23 15:11       ` Hanjun Guo
2015-06-23 15:11       ` Hanjun Guo
2015-06-23 17:38       ` Lorenzo Pieralisi
2015-06-23 17:38         ` Lorenzo Pieralisi
2015-06-23 17:38         ` Lorenzo Pieralisi
2015-06-27  3:52         ` Hanjun Guo
2015-06-27  3:52           ` Hanjun Guo
2015-06-27  3:52           ` Hanjun Guo
2015-06-29  8:39           ` Marc Zyngier
2015-06-29  8:39             ` Marc Zyngier
2015-06-29  8:39             ` Marc Zyngier
2015-06-30 11:50             ` Hanjun Guo
2015-06-30 11:50               ` Hanjun Guo
2015-06-30 11:50               ` Hanjun Guo
2015-06-30 12:17               ` Marc Zyngier
2015-06-30 12:17                 ` Marc Zyngier
2015-06-30 12:17                 ` Marc Zyngier
2015-06-30 15:07                 ` Graeme Gregory
2015-06-30 15:07                   ` Graeme Gregory
2015-06-30 15:07                   ` Graeme Gregory
2015-07-03  8:47                   ` Hanjun Guo [this message]
2015-07-03  8:47                     ` Hanjun Guo
2015-07-03  8:47                     ` Hanjun Guo
2015-07-08  3:40                 ` Hanjun Guo
2015-07-08  3:40                   ` Hanjun Guo
2015-07-08  3:40                   ` Hanjun Guo
2015-07-10  9:41                   ` Marc Zyngier
2015-07-10  9:41                     ` Marc Zyngier
2015-07-10  9:41                     ` Marc Zyngier
2015-06-19  8:46 ` [PATCH v2 7/9] irqchip / GICv3: Refactor gic_of_init() for GICv3 driver Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 8/9] irqchip / GICv3: Add ACPI support for GICv3+ initialization Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo
2015-06-19  8:46 ` [PATCH v2 9/9] irqchip / GICv3: Add stacked irqdomain support for ACPI based init Hanjun Guo
2015-06-19  8:46   ` Hanjun Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55964C30.7020208@linaro.org \
    --to=hanjun.guo@linaro.org \
    --cc=Catalin.Marinas@arm.com \
    --cc=Lorenzo.Pieralisi@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=arnd@arndb.de \
    --cc=gg@slimlogic.co.uk \
    --cc=grant.likely@linaro.org \
    --cc=jason@lakedaemon.net \
    --cc=jiang.liu@linux.intel.com \
    --cc=linaro-acpi@lists.linaro.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=olof@lixom.net \
    --cc=rjw@rjwysocki.net \
    --cc=tglx@linutronix.de \
    --cc=tomasz.nowicki@linaro.org \
    --cc=wei@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.