From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [PATCH v7 15/15] irqchip: mbigen: Add ACPI support Date: Mon, 16 Jan 2017 22:23:16 +0800 Message-ID: <587CD754.1050808@huawei.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170116113804.GB23703@red-moon> Sender: linux-kernel-owner@vger.kernel.org To: Lorenzo Pieralisi , "Rafael J. Wysocki" Cc: Hanjun Guo , huxinwei@huawei.com, Kefeng Wang , yimin@huawei.com, Jon Masters , Marc Zyngier , Greg KH , linux-kernel@vger.kernel.org, linuxarm@huawei.com, Sinan Kaya , linux-acpi@vger.kernel.org, Xinwei Kong , Matthias Brugger , Tomasz Nowicki , Thomas Gleixner , Agustin Vega-Frias , linux-arm-kernel@lists.infradead.org, Ma Jun List-Id: linux-acpi@vger.kernel.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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751134AbdAPO3U (ORCPT ); Mon, 16 Jan 2017 09:29:20 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:53256 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750742AbdAPO3R (ORCPT ); Mon, 16 Jan 2017 09:29:17 -0500 Subject: Re: [PATCH v7 15/15] irqchip: mbigen: Add ACPI support To: Lorenzo Pieralisi , "Rafael J. Wysocki" 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> CC: Hanjun Guo , , Kefeng Wang , , Jon Masters , Marc Zyngier , Greg KH , , , Sinan Kaya , , Xinwei Kong , Matthias Brugger , Tomasz Nowicki , Thomas Gleixner , Agustin Vega-Frias , , Ma Jun From: Hanjun Guo Message-ID: <587CD754.1050808@huawei.com> Date: Mon, 16 Jan 2017 22:23:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20170116113804.GB23703@red-moon> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.17.188] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090204.587CD769.0298,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: b12ea5e2bb3594631b9845ba4796d385 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 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