| Message ID | 1484147199-4267-16-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
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. 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". Should I add this into the commit message? Thanks Hanjun
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
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
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
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
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(-)