| Message ID | 1488890410-15503-16-git-send-email-guohanjun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI platform MSI support and its example mbigen | expand |
On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: > From: Hanjun Guo <hanjun.guo@linaro.org> > > With the preparation of platform msi support and interrupt producer > in DSDT, we can add mbigen ACPI support now. > > We are using Interrupt resource type in _CRS methd to indicate number > of irq pins instead of num_pins in DT to avoid _DSD usage in this case. > > For mbigen, > Device(MBI0) { > Name(_HID, "HISI0152") > Name(_UID, Zero) > Name(_CRS, ResourceTemplate() { > Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) > Interrupt(ResourceProducer,...) {12,14,....} What do these interrupt numbers represent ? This looks wrong to me. An interrupt descriptor is there to describe the interrupts a device can generate; you are using it just to add a "standard" (that is not standard at all) way of counting the number of vectors allocated to this specific chip and that's just wrong. Can't you use something like Agustin did in the QCOM combiner: drivers/irqchip/qcom-irq-combiner.c to detect the MSI vector length (ie by describing the MBIgen through generic registers and use the bit width to compute the vector lenght) ? I am not sure how feasible it is given that my knowledge of MBIgen is pretty poor. I understand we want to avoid _DSD properties but we should not work around standard bindings to achieve that goal. Side effect: IIUC the kernel will allocate an array of resources for your MBIgen interrupts whose size equal the Interrupt descriptor, which is as bad as it can get given that those resources are to the best of my knowledge unused. Lorenzo > }) > } > > For devices, > Device(COM0) { > Name(_HID, "ACPIIDxx") > Name(_UID, Zero) > Name(_CRS, ResourceTemplate() { > Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) > Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} > }) > } > > With the help of platform msi and interrupt producer, then devices > will get the virq from mbigen's irqdomain. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Ma Jun <majun258@huawei.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/irqchip/irq-mbigen.c | 70 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 67 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c > index 3756408..e6bb503 100644 > --- a/drivers/irqchip/irq-mbigen.c > +++ b/drivers/irqchip/irq-mbigen.c > @@ -16,6 +16,7 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/acpi.h> > #include <linux/interrupt.h> > #include <linux/irqchip.h> > #include <linux/module.h> > @@ -180,7 +181,7 @@ static int mbigen_domain_translate(struct irq_domain *d, > unsigned long *hwirq, > unsigned int *type) > { > - if (is_of_node(fwspec->fwnode)) { > + if (is_of_node(fwspec->fwnode) || is_acpi_device_node(fwspec->fwnode)) { > if (fwspec->param_count != 2) > return -EINVAL; > > @@ -271,6 +272,54 @@ static int mbigen_of_create_domain(struct platform_device *pdev, > return 0; > } > > +#ifdef CONFIG_ACPI > +static acpi_status mbigen_acpi_process_resource(struct acpi_resource *ares, > + void *context) > +{ > + struct acpi_resource_extended_irq *ext_irq; > + u32 *num_irqs = context; > + > + switch (ares->type) { > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > + ext_irq = &ares->data.extended_irq; > + *num_irqs += ext_irq->interrupt_count; > + break; > + default: > + break; > + } > + > + return AE_OK; > +} > + > +static int mbigen_acpi_create_domain(struct platform_device *pdev, > + struct mbigen_device *mgn_chip) > +{ > + struct irq_domain *domain; > + u32 num_msis = 0; > + acpi_status status; > + > + status = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS, > + mbigen_acpi_process_resource, &num_msis); > + if (ACPI_FAILURE(status) || num_msis == 0) > + return -EINVAL; > + > + domain = platform_msi_create_device_domain(&pdev->dev, num_msis, > + mbigen_write_msg, > + &mbigen_domain_ops, > + mgn_chip); > + if (!domain) > + return -ENOMEM; > + > + return 0; > +} > +#else > +static inline int mbigen_acpi_create_domain(struct platform_device *pdev, > + struct mbigen_device *mgn_chip) > +{ > + return -ENODEV; > +} > +#endif > + > static int mbigen_device_probe(struct platform_device *pdev) > { > struct mbigen_device *mgn_chip; > @@ -289,9 +338,17 @@ static int mbigen_device_probe(struct platform_device *pdev) > if (IS_ERR(mgn_chip->base)) > return PTR_ERR(mgn_chip->base); > > - err = mbigen_of_create_domain(pdev, mgn_chip); > - if (err) > + if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) > + err = mbigen_of_create_domain(pdev, mgn_chip); > + else if (ACPI_COMPANION(&pdev->dev)) > + err = mbigen_acpi_create_domain(pdev, mgn_chip); > + else > + err = -EINVAL; > + > + if (err) { > + dev_err(&pdev->dev, "Failed to create mbi-gen@%p irqdomain", mgn_chip->base); > return err; > + } > > platform_set_drvdata(pdev, mgn_chip); > return 0; > @@ -303,10 +360,17 @@ static int mbigen_device_probe(struct platform_device *pdev) > }; > MODULE_DEVICE_TABLE(of, mbigen_of_match); > > +static const struct acpi_device_id mbigen_acpi_match[] = { > + { "HISI0152", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, mbigen_acpi_match); > + > static struct platform_driver mbigen_platform_driver = { > .driver = { > .name = "Hisilicon MBIGEN-V2", > .of_match_table = mbigen_of_match, > + .acpi_match_table = ACPI_PTR(mbigen_acpi_match), > }, > .probe = mbigen_device_probe, > }; > -- > 1.7.12.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marc Many thanks for your comments > -----Original Message----- > From: linuxarm-bounces@huawei.com [mailto:linuxarm-bounces@huawei.com] > On Behalf Of Marc Zyngier > Sent: 27 March 2017 09:47 > To: John Garry; Lorenzo Pieralisi; Guohanjun (Hanjun Guo) > Cc: Rafael J. Wysocki; Yimin (Leo); Greg KH; Linuxarm; linux- > kernel@vger.kernel.org; Sinan Kaya; linux-acpi@vger.kernel.org; Hanjun > Guo; Tomasz Nowicki; Thomas Gleixner; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support > > Hanjun, John, > > On 22/03/17 14:12, John Garry wrote: > > On 21/03/2017 14:45, Lorenzo Pieralisi wrote: > >> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: > >>> From: Hanjun Guo <hanjun.guo@linaro.org> > >>> > >>> With the preparation of platform msi support and interrupt producer > >>> in DSDT, we can add mbigen ACPI support now. > >>> > >>> We are using Interrupt resource type in _CRS methd to indicate > number > >>> of irq pins instead of num_pins in DT to avoid _DSD usage in this > case. > >>> > >>> For mbigen, > >>> Device(MBI0) { > >>> Name(_HID, "HISI0152") > >>> Name(_UID, Zero) > >>> Name(_CRS, ResourceTemplate() { > >>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) > >>> Interrupt(ResourceProducer,...) {12,14,....} > >> > >> What do these interrupt numbers represent ? This looks wrong to me. > >> An interrupt descriptor is there to describe the interrupts a device > >> can generate; you are using it just to add a "standard" (that is > >> not standard at all) way of counting the number of vectors allocated > >> to this specific chip and that's just wrong. > >> > > > > As I understand, the count of interrupts we are declaring for the > mbigen > > is the same as the sum of interrupts for that mbigen's children. > > > > So at the point we probe the mbigen, can we just deference the > children > > to count their interrupts, and use this as the #msis? > > > >> Can't you use something like Agustin did in the QCOM combiner: > >> > >> drivers/irqchip/qcom-irq-combiner.c > >> > >> to detect the MSI vector length (ie by describing the MBIgen through > >> generic registers and use the bit width to compute the vector > >> lenght) ? I am not sure how feasible it is given that my knowledge > >> of MBIgen is pretty poor. > >> > >> I understand we want to avoid _DSD properties but we should not > >> work around standard bindings to achieve that goal. > >> > > > > We use "num-pins" for dt solution, but it is not so welcome here. > > Well, this device is already completely out of any standard description > on the ACPI side. And given that it bloats both the ACPI tables and the > kernel data structures, I can only suggest that you take advantage of > _DSD here, as misusing the standard properties is not something that we > should condone. It will also make the driver more manageable, as it > will > use similar properties on both firmware implementations. > > I feel like I need to stress the urgency here. We're at -rc4, and still > with unsolved issues. None of us want to miss the next merge window. > As follow up our guys would work on a solution whose ACPI table looks like the following one: For mbigen, Device(MBI0) { Name(_HID, "HISI0152") Name(_UID, Zero) Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) }) Name(_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"num-pins", xxx} } }) } For devices, Device(COM0) { Name(_HID, "ACPIIDxx") Name(_UID, Zero) Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} }) } Marc, Lorenzo if you are ok with the above we will submit v10 based on this... Many Thanks Gab > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... > _______________________________________________ > linuxarm mailing list > linuxarm@huawei.com > http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+Al,Darren to comment on _DSD review process] On Mon, Mar 27, 2017 at 12:24:45PM +0000, Gabriele Paoloni wrote: > Hi Marc Many thanks for your comments > > > -----Original Message----- > > From: linuxarm-bounces@huawei.com [mailto:linuxarm-bounces@huawei.com] > > On Behalf Of Marc Zyngier > > Sent: 27 March 2017 09:47 > > To: John Garry; Lorenzo Pieralisi; Guohanjun (Hanjun Guo) > > Cc: Rafael J. Wysocki; Yimin (Leo); Greg KH; Linuxarm; linux- > > kernel@vger.kernel.org; Sinan Kaya; linux-acpi@vger.kernel.org; Hanjun > > Guo; Tomasz Nowicki; Thomas Gleixner; linux-arm- > > kernel@lists.infradead.org > > Subject: Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support > > > > Hanjun, John, > > > > On 22/03/17 14:12, John Garry wrote: > > > On 21/03/2017 14:45, Lorenzo Pieralisi wrote: > > >> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: > > >>> From: Hanjun Guo <hanjun.guo@linaro.org> > > >>> > > >>> With the preparation of platform msi support and interrupt producer > > >>> in DSDT, we can add mbigen ACPI support now. > > >>> > > >>> We are using Interrupt resource type in _CRS methd to indicate > > number > > >>> of irq pins instead of num_pins in DT to avoid _DSD usage in this > > case. > > >>> > > >>> For mbigen, > > >>> Device(MBI0) { > > >>> Name(_HID, "HISI0152") > > >>> Name(_UID, Zero) > > >>> Name(_CRS, ResourceTemplate() { > > >>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) > > >>> Interrupt(ResourceProducer,...) {12,14,....} > > >> > > >> What do these interrupt numbers represent ? This looks wrong to me. > > >> An interrupt descriptor is there to describe the interrupts a device > > >> can generate; you are using it just to add a "standard" (that is > > >> not standard at all) way of counting the number of vectors allocated > > >> to this specific chip and that's just wrong. > > >> > > > > > > As I understand, the count of interrupts we are declaring for the > > mbigen > > > is the same as the sum of interrupts for that mbigen's children. > > > > > > So at the point we probe the mbigen, can we just deference the > > children > > > to count their interrupts, and use this as the #msis? > > > > > >> Can't you use something like Agustin did in the QCOM combiner: > > >> > > >> drivers/irqchip/qcom-irq-combiner.c > > >> > > >> to detect the MSI vector length (ie by describing the MBIgen through > > >> generic registers and use the bit width to compute the vector > > >> lenght) ? I am not sure how feasible it is given that my knowledge > > >> of MBIgen is pretty poor. > > >> > > >> I understand we want to avoid _DSD properties but we should not > > >> work around standard bindings to achieve that goal. > > >> > > > > > > We use "num-pins" for dt solution, but it is not so welcome here. > > > > Well, this device is already completely out of any standard description > > on the ACPI side. And given that it bloats both the ACPI tables and the > > kernel data structures, I can only suggest that you take advantage of > > _DSD here, as misusing the standard properties is not something that we > > should condone. It will also make the driver more manageable, as it > > will > > use similar properties on both firmware implementations. > > > > I feel like I need to stress the urgency here. We're at -rc4, and still > > with unsolved issues. None of us want to miss the next merge window. > > > > As follow up our guys would work on a solution whose ACPI table looks > like the following one: > > For mbigen, > Device(MBI0) { > Name(_HID, "HISI0152") > Name(_UID, Zero) > Name(_CRS, ResourceTemplate() { > Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) > }) > > Name(_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () > { > Package () {"num-pins", xxx} > } > }) > } > > For devices, > Device(COM0) { > Name(_HID, "ACPIIDxx") > Name(_UID, Zero) > Name(_CRS, ResourceTemplate() { > Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) > Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} > }) > } > > > Marc, Lorenzo if you are ok with the above we will submit v10 based on this... I am ok with it. I am not 100% up-to-date on what's the status on _DSD bindings/review/guidelines but it would be certainly a good idea to kickstart the process for MBIgen which basically means following this as far as I know (and post to the relevant mailing list): https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt Al and Darren may add to that as they have more insights. I would like to send IORT patches to Catalin as soon as possible so as Marc pointed out the sooner we sort this out the better. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/27/2017 09:27 AM, Lorenzo Pieralisi wrote: > [+Al,Darren to comment on _DSD review process] > > On Mon, Mar 27, 2017 at 12:24:45PM +0000, Gabriele Paoloni wrote: >> Hi Marc Many thanks for your comments >> >>> -----Original Message----- >>> From: linuxarm-bounces@huawei.com [mailto:linuxarm-bounces@huawei.com] >>> On Behalf Of Marc Zyngier >>> Sent: 27 March 2017 09:47 >>> To: John Garry; Lorenzo Pieralisi; Guohanjun (Hanjun Guo) >>> Cc: Rafael J. Wysocki; Yimin (Leo); Greg KH; Linuxarm; linux- >>> kernel@vger.kernel.org; Sinan Kaya; linux-acpi@vger.kernel.org; Hanjun >>> Guo; Tomasz Nowicki; Thomas Gleixner; linux-arm- >>> kernel@lists.infradead.org >>> Subject: Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support >>> >>> Hanjun, John, >>> >>> On 22/03/17 14:12, John Garry wrote: >>>> On 21/03/2017 14:45, Lorenzo Pieralisi wrote: >>>>> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: >>>>>> From: Hanjun Guo <hanjun.guo@linaro.org> >>>>>> >>>>>> With the preparation of platform msi support and interrupt producer >>>>>> in DSDT, we can add mbigen ACPI support now. >>>>>> >>>>>> We are using Interrupt resource type in _CRS methd to indicate >>> number >>>>>> of irq pins instead of num_pins in DT to avoid _DSD usage in this >>> case. >>>>>> >>>>>> For mbigen, >>>>>> Device(MBI0) { >>>>>> Name(_HID, "HISI0152") >>>>>> Name(_UID, Zero) >>>>>> Name(_CRS, ResourceTemplate() { >>>>>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >>>>>> Interrupt(ResourceProducer,...) {12,14,....} >>>>> >>>>> What do these interrupt numbers represent ? This looks wrong to me. >>>>> An interrupt descriptor is there to describe the interrupts a device >>>>> can generate; you are using it just to add a "standard" (that is >>>>> not standard at all) way of counting the number of vectors allocated >>>>> to this specific chip and that's just wrong. >>>>> >>>> >>>> As I understand, the count of interrupts we are declaring for the >>> mbigen >>>> is the same as the sum of interrupts for that mbigen's children. >>>> >>>> So at the point we probe the mbigen, can we just deference the >>> children >>>> to count their interrupts, and use this as the #msis? >>>> >>>>> Can't you use something like Agustin did in the QCOM combiner: >>>>> >>>>> drivers/irqchip/qcom-irq-combiner.c >>>>> >>>>> to detect the MSI vector length (ie by describing the MBIgen through >>>>> generic registers and use the bit width to compute the vector >>>>> lenght) ? I am not sure how feasible it is given that my knowledge >>>>> of MBIgen is pretty poor. >>>>> >>>>> I understand we want to avoid _DSD properties but we should not >>>>> work around standard bindings to achieve that goal. >>>>> >>>> >>>> We use "num-pins" for dt solution, but it is not so welcome here. >>> >>> Well, this device is already completely out of any standard description >>> on the ACPI side. And given that it bloats both the ACPI tables and the >>> kernel data structures, I can only suggest that you take advantage of >>> _DSD here, as misusing the standard properties is not something that we >>> should condone. It will also make the driver more manageable, as it >>> will >>> use similar properties on both firmware implementations. >>> >>> I feel like I need to stress the urgency here. We're at -rc4, and still >>> with unsolved issues. None of us want to miss the next merge window. >>> >> >> As follow up our guys would work on a solution whose ACPI table looks >> like the following one: >> >> For mbigen, >> Device(MBI0) { >> Name(_HID, "HISI0152") >> Name(_UID, Zero) >> Name(_CRS, ResourceTemplate() { >> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >> }) >> >> Name(_DSD, Package () { >> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> Package () >> { >> Package () {"num-pins", xxx} >> } >> }) >> } Please fix the indentation here; the ASL parser doesn't care, but I don't see '}'s as clearly as it does :). In particular, _DSD usage *must* be for a device and it does not look like that if one doesn't carefully count all the parentheses and braces. >> >> For devices, >> Device(COM0) { >> Name(_HID, "ACPIIDxx") >> Name(_UID, Zero) >> Name(_CRS, ResourceTemplate() { >> Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) >> Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} >> }) >> } >> >> >> Marc, Lorenzo if you are ok with the above we will submit v10 based on this... > > I am ok with it. I am not 100% up-to-date on what's the status on _DSD > bindings/review/guidelines but it would be certainly a good idea to > kickstart the process for MBIgen which basically means following this > as far as I know (and post to the relevant mailing list): > > https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt So, let's correct this bit: consider that URL deprecated, please, and refer instead to the text that was agreed upon and in the kernel tree: Documentation/acpi/DSD-properties-rules.txt > Al and Darren may add to that as they have more insights. Since this use of _DSD is very specific to this device, and this device only, I don't have any real objections. I will say that "num-pins" is not terribly descriptive so it would be really good to either use a much more descriptive name or add plenty of commentary in the code -- and preferably both. I would recommend including the ASL in the code comments, with a description of how the property is to be used and what it means. > I would like to send IORT patches to Catalin as soon as possible so > as Marc pointed out the sooner we sort this out the better. > > Thanks, > Lorenzo > -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3@redhat.com ----------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Al, Thanks for your feedback, reply inline. On 03/28/2017 02:56 AM, Al Stone wrote: > On 03/27/2017 09:27 AM, Lorenzo Pieralisi wrote: >> [+Al,Darren to comment on _DSD review process] >> >> On Mon, Mar 27, 2017 at 12:24:45PM +0000, Gabriele Paoloni wrote: >>> Hi Marc Many thanks for your comments >>>> Hanjun, John, >>>> >>>> On 22/03/17 14:12, John Garry wrote: >>>>> On 21/03/2017 14:45, Lorenzo Pieralisi wrote: >>>>>> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: >>>>>>> From: Hanjun Guo <hanjun.guo@linaro.org> >>>>>>> >>>>>>> With the preparation of platform msi support and interrupt producer >>>>>>> in DSDT, we can add mbigen ACPI support now. >>>>>>> >>>>>>> We are using Interrupt resource type in _CRS methd to indicate >>>> number >>>>>>> of irq pins instead of num_pins in DT to avoid _DSD usage in this >>>> case. >>>>>>> >>>>>>> For mbigen, >>>>>>> Device(MBI0) { >>>>>>> Name(_HID, "HISI0152") >>>>>>> Name(_UID, Zero) >>>>>>> Name(_CRS, ResourceTemplate() { >>>>>>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >>>>>>> Interrupt(ResourceProducer,...) {12,14,....} >>>>>> >>>>>> What do these interrupt numbers represent ? This looks wrong to me. >>>>>> An interrupt descriptor is there to describe the interrupts a device >>>>>> can generate; you are using it just to add a "standard" (that is >>>>>> not standard at all) way of counting the number of vectors allocated >>>>>> to this specific chip and that's just wrong. >>>>>> >>>>> >>>>> As I understand, the count of interrupts we are declaring for the >>>> mbigen >>>>> is the same as the sum of interrupts for that mbigen's children. >>>>> >>>>> So at the point we probe the mbigen, can we just deference the >>>> children >>>>> to count their interrupts, and use this as the #msis? >>>>> >>>>>> Can't you use something like Agustin did in the QCOM combiner: >>>>>> >>>>>> drivers/irqchip/qcom-irq-combiner.c >>>>>> >>>>>> to detect the MSI vector length (ie by describing the MBIgen through >>>>>> generic registers and use the bit width to compute the vector >>>>>> lenght) ? I am not sure how feasible it is given that my knowledge >>>>>> of MBIgen is pretty poor. >>>>>> >>>>>> I understand we want to avoid _DSD properties but we should not >>>>>> work around standard bindings to achieve that goal. >>>>>> >>>>> >>>>> We use "num-pins" for dt solution, but it is not so welcome here. >>>> >>>> Well, this device is already completely out of any standard description >>>> on the ACPI side. And given that it bloats both the ACPI tables and the >>>> kernel data structures, I can only suggest that you take advantage of >>>> _DSD here, as misusing the standard properties is not something that we >>>> should condone. It will also make the driver more manageable, as it >>>> will >>>> use similar properties on both firmware implementations. >>>> >>>> I feel like I need to stress the urgency here. We're at -rc4, and still >>>> with unsolved issues. None of us want to miss the next merge window. >>>> >>> >>> As follow up our guys would work on a solution whose ACPI table looks >>> like the following one: >>> >>> For mbigen, >>> Device(MBI0) { >>> Name(_HID, "HISI0152") >>> Name(_UID, Zero) >>> Name(_CRS, ResourceTemplate() { >>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >>> }) >>> >>> Name(_DSD, Package () { >>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >>> Package () >>> { >>> Package () {"num-pins", xxx} >>> } >>> }) >>> } > > Please fix the indentation here; the ASL parser doesn't care, but I don't > see '}'s as clearly as it does :). In particular, _DSD usage *must* be for > a device and it does not look like that if one doesn't carefully count all > the parentheses and braces. I will fix it :) > >>> >>> For devices, >>> Device(COM0) { >>> Name(_HID, "ACPIIDxx") >>> Name(_UID, Zero) >>> Name(_CRS, ResourceTemplate() { >>> Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) >>> Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} >>> }) >>> } >>> >>> >>> Marc, Lorenzo if you are ok with the above we will submit v10 based on this... >> >> I am ok with it. I am not 100% up-to-date on what's the status on _DSD >> bindings/review/guidelines but it would be certainly a good idea to >> kickstart the process for MBIgen which basically means following this >> as far as I know (and post to the relevant mailing list): >> >> https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt > > So, let's correct this bit: consider that URL deprecated, please, and refer > instead to the text that was agreed upon and in the kernel tree: > > Documentation/acpi/DSD-properties-rules.txt > >> Al and Darren may add to that as they have more insights. > > Since this use of _DSD is very specific to this device, and this device > only, I don't have any real objections. Thanks for the confirmation. > I will say that "num-pins" is > not terribly descriptive so it would be really good to either use a much > more descriptive name or add plenty of commentary in the code -- and > preferably both. I would recommend including the ASL in the code comments, > with a description of how the property is to be used and what it means. "num-pins" is used for mbigen DT support: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt I think we can use the name unchanged and adding more comments as you suggested, will cc you for this updated patch. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c index 3756408..e6bb503 100644 --- a/drivers/irqchip/irq-mbigen.c +++ b/drivers/irqchip/irq-mbigen.c @@ -16,6 +16,7 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/acpi.h> #include <linux/interrupt.h> #include <linux/irqchip.h> #include <linux/module.h> @@ -180,7 +181,7 @@ static int mbigen_domain_translate(struct irq_domain *d, unsigned long *hwirq, unsigned int *type) { - if (is_of_node(fwspec->fwnode)) { + if (is_of_node(fwspec->fwnode) || is_acpi_device_node(fwspec->fwnode)) { if (fwspec->param_count != 2) return -EINVAL; @@ -271,6 +272,54 @@ static int mbigen_of_create_domain(struct platform_device *pdev, return 0; } +#ifdef CONFIG_ACPI +static acpi_status mbigen_acpi_process_resource(struct acpi_resource *ares, + void *context) +{ + struct acpi_resource_extended_irq *ext_irq; + u32 *num_irqs = context; + + switch (ares->type) { + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: + ext_irq = &ares->data.extended_irq; + *num_irqs += ext_irq->interrupt_count; + break; + default: + break; + } + + return AE_OK; +} + +static int mbigen_acpi_create_domain(struct platform_device *pdev, + struct mbigen_device *mgn_chip) +{ + struct irq_domain *domain; + u32 num_msis = 0; + acpi_status status; + + status = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS, + mbigen_acpi_process_resource, &num_msis); + if (ACPI_FAILURE(status) || num_msis == 0) + return -EINVAL; + + domain = platform_msi_create_device_domain(&pdev->dev, num_msis, + mbigen_write_msg, + &mbigen_domain_ops, + mgn_chip); + if (!domain) + return -ENOMEM; + + return 0; +} +#else +static inline int mbigen_acpi_create_domain(struct platform_device *pdev, + struct mbigen_device *mgn_chip) +{ + return -ENODEV; +} +#endif + static int mbigen_device_probe(struct platform_device *pdev) { struct mbigen_device *mgn_chip; @@ -289,9 +338,17 @@ static int mbigen_device_probe(struct platform_device *pdev) if (IS_ERR(mgn_chip->base)) return PTR_ERR(mgn_chip->base); - err = mbigen_of_create_domain(pdev, mgn_chip); - if (err) + if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) + err = mbigen_of_create_domain(pdev, mgn_chip); + else if (ACPI_COMPANION(&pdev->dev)) + err = mbigen_acpi_create_domain(pdev, mgn_chip); + else + err = -EINVAL; + + if (err) { + dev_err(&pdev->dev, "Failed to create mbi-gen@%p irqdomain", mgn_chip->base); return err; + } platform_set_drvdata(pdev, mgn_chip); return 0; @@ -303,10 +360,17 @@ static int mbigen_device_probe(struct platform_device *pdev) }; MODULE_DEVICE_TABLE(of, mbigen_of_match); +static const struct acpi_device_id mbigen_acpi_match[] = { + { "HISI0152", 0 }, + {} +}; +MODULE_DEVICE_TABLE(acpi, mbigen_acpi_match); + static struct platform_driver mbigen_platform_driver = { .driver = { .name = "Hisilicon MBIGEN-V2", .of_match_table = mbigen_of_match, + .acpi_match_table = ACPI_PTR(mbigen_acpi_match), }, .probe = mbigen_device_probe, };