[v7,15/15] irqchip: mbigen: Add ACPI support

Message ID 1484147199-4267-16-git-send-email-hanjun.guo@linaro.org
State New
Headers show

Commit Message

Hanjun Guo Jan. 11, 2017, 3:06 p.m.
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

Comments

Lorenzo Pieralisi Jan. 13, 2017, 10:21 a.m. | #1
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
Lorenzo Pieralisi Jan. 16, 2017, 11:38 a.m. | #2
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
Lorenzo Pieralisi Jan. 16, 2017, 3:24 p.m. | #3
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
Hanjun Guo Jan. 17, 2017, 11:59 a.m. | #4
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

Patch hide | download patch | download mbox

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,
 };