From mboxrd@z Thu Jan  1 00:00:00 1970
From: guohanjun@huawei.com (Hanjun Guo)
Date: Mon, 16 Jan 2017 22:23:16 +0800
Subject: [PATCH v7 15/15] irqchip: mbigen: Add ACPI support
In-Reply-To: <20170116113804.GB23703@red-moon>
References: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
 <1484147199-4267-16-git-send-email-hanjun.guo@linaro.org>
 <20170113102104.GB20837@red-moon> <58799376.6040302@huawei.com>
 <20170116113804.GB23703@red-moon>
Message-ID: <587CD754.1050808@huawei.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

Hi Lorenzo,

On 2017/1/16 19:38, Lorenzo Pieralisi wrote:
> On Sat, Jan 14, 2017 at 10:56:54AM +0800, Hanjun Guo wrote:
>> Hi Lorenzo,
>>
>> On 2017/1/13 18:21, Lorenzo Pieralisi wrote:
>>> On Wed, Jan 11, 2017 at 11:06:39PM +0800, Hanjun Guo wrote:
>>>> With the preparation of platform msi support and interrupt producer
>>>> in DSDT, we can add mbigen ACPI support now.
>>>>
>>>> We are using _PRS methd to indicate number of irq pins instead
>>>> of num_pins in DT to avoid _DSD usage in this case.
>>>>
>>>> For mbi-gen,
>>>>     Device(MBI0) {
>>>>           Name(_HID, "HISI0152")
>>>>           Name(_UID, Zero)
>>>>           Name(_CRS, ResourceTemplate() {
>>>>                   Memory32Fixed(ReadWrite, 0xa0080000, 0x10000)
>>>>           })
>>>>
>>>>           Name (_PRS, ResourceTemplate() {
>>>> 		  Interrupt(ResourceProducer,...) {12,14,....}
>>> I still do not understand why you are using _PRS for this, I think
>>> the MBIgen configuration is static and if it is so the Interrupt
>>> resource should be part of the _CRS unless there is something I am
>>> missing here.
>> Sorry for not clear in the commit message. MBIgen is an interrupt producer
>> which produces irq resource to devices connecting to it, and MBIgen itself
>> don't consume wired interrupts.
> That's why you mark it as ResourceProducer, but that's not a reason to
> put it in the _PRS instead of _CRS.

If using _CRS for the interrupt resource, the irq number represented will be mapped
(i.e acpi_register_gsi()), then will conflict with the irq number of devices consuming
it (mbigen is producing the interrupts), but I agree with you that let's ask Rafael's
point of view.

>
> IIUC _PRS is there to provide a way to define the possible resource
> settings of a _configurable_ device (ie programmable) so that the actual
> resource value you would programme with a call to its _SRS is sane (ie
> the OS has a way, through the _PRS, to detect what possible resource
> settings are available for the device).
>
> I think Rafael has more insights into how the _PRS is used on x86
> systems so I would ask his point of view here before merrily merging
> this code.

OK, Rafael is traveling now, hope he will have time to take a look.

How about updating this patch set then sending a new version for review
with this patch unchanged? if Rafael have comments on this one, I will
send a single updated one for this patch (if no other changes).

>
>> Also devices connecting MBIgen may not consume all the interrupts produced
>> by MBIgen, for example, MBIgen may produce 128 interrupts but only half of
>> them are currently used, so _PRS here means "provide interrupt resources
>> may consumed by devices connecting to it".
> See above.

Thanks
Hanjun