| Message ID | 1484147199-4267-16-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. 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 helpe of platform msi and interrupt producer, then devices > will get the virq from mbi-gen's irqdomain. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Reviewed-by: 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 4e11da5..17d35fa 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__PRS, > + 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 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; > @@ -288,9 +337,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; > @@ -302,10 +359,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.9.1 > -- 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 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. 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. > 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. 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 Mon, Jan 16, 2017 at 10:23:16PM +0800, Hanjun Guo wrote: > 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. Aha ! So here is why you are using _PRS because the kernel turns _CRS Interrupt resources (even producers) into GSIs which is probably a kernel bug, is that the reason ? We don't abuse firmware bindings to make the kernel work, that's _never_ a good idea. If the interrupt resource is a Resource Producer core ACPI should not register the IRQ because that's not a GSI, probably this should be part of Agustin changes too ? > > 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). I think this patch (and the FW that goes with it) is wrong, but the rest of the series, in particular the IORT bits, are ok with me. 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 2017/1/16 23:24, Lorenzo Pieralisi wrote: > On Mon, Jan 16, 2017 at 10:23:16PM +0800, Hanjun Guo wrote: >> 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. > Aha ! So here is why you are using _PRS because the kernel turns _CRS > Interrupt resources (even producers) into GSIs which is probably a > kernel bug, is that the reason ? I thought _CRS is for devices consuming resources, it's a kind of misunderstanding. > > We don't abuse firmware bindings to make the kernel work, that's _never_ > a good idea. > > If the interrupt resource is a Resource Producer core ACPI should not > register the IRQ because that's not a GSI, probably this should be part of > Agustin changes too ? Agreed. If it's a Resource Producer, - it's not a GSI - and it should not be mapped to any irq domains I think Agustin needs to add the changes to the patch set but only for CONFIG_ACPI_GENERIC_GSI=y, not bother the core code as the complex history of firmware in x86, what do you think? > >>> 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). > I think this patch (and the FW that goes with it) is wrong, but the rest > of the series, in particular the IORT bits, are ok with me. Thanks, I updated the patch using _CRS and firmware, with minor changes for Agustin's patch set, mbigen works pretty good as before. I will comment on Agustin's patch set. 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 4e11da5..17d35fa 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__PRS, + 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 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; @@ -288,9 +337,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; @@ -302,10 +359,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, };
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,....} }) } For devices, Device(COM0) { Name(_HID, "ACPIIDxx") Name(_UID, Zero) Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} }) } With the helpe of platform msi and interrupt producer, then devices will get the virq from mbi-gen's irqdomain. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> Reviewed-by: 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(-) -- 1.9.1 -- 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