Re: [RFC PATCH v2 10/11] irqchip: mbigen: Add ACPI support

[Hanjun Guo]

Hi Marc,

On 2016/9/15 16:49, Marc Zyngier wrote:

On 14/09/16 15:21, Hanjun Guo wrote:

From: Hanjun Guo 

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.

For mbi-gen,
Device(MBI0) {
  Name(_HID, "HISI0152")
  Name(_UID, Zero)
  Name(_CRS, ResourceTemplate() {
  Memory32Fixed(ReadWrite, 0xa008, 0x1)
  })

  Name (_PRS, ResourceTemplate() {
  Interrupt(ResourceProducer,...) {12,14,}
  })


Since I know next to nothing about all of this, I'm going to play the
village idiot. What makes it legal to use _PRS as a way to describe the
interrupts that are exposed by MBI0? Looking at the 6.0 spec, I do not
see why the interrupts would be put there instead of _CRS, and why you'd
have a _PRS at all.


_PRS describes possible resource settings for the device, which returns
a list of a device's possible resource settings such as memory range,
interrupt descriptors, and the format of the data in a _PRS object
follows the same format as _CRS object (ACPI 6.1, section 6.2.12),
so Interrupts can be put in the _PRS.

And in ACPI 6.1, section 6.2, it describes the _PRS usage:

"Some resources, however, may be shared amongst several devices. To
describe this, devices that share a resource (resource consumers) must
use the extended resource descriptors (0x7-0xA) described in Section
6.4.3, “Large Resource Data Type.” These descriptors point to a single
device object (resource producer) that claims the shared resource in
its _PRS."

As mbigen is a interrupt producer which provide interrupt resoures
for devices, which matches the usage of _PRS in the spec.



Also, are you going to exhaustively describe all the possible interrupts
via this method? Knowing that the mbigen can expose thousands of
interrupts, I find it slightly mad. Can't you express a range?


Yes, a little bit mad. I can't express a interrupt range in ACPI at
least in current version of spec. But that just adding more lines
of ACPI DSDT code, it's fine to me. or we need to use _DSD to present
similar property "num_pins" in ACPI which I avoid using.

Thanks
Hanjun

Re: [RFC PATCH v2 10/11] irqchip: mbigen: Add ACPI support

[Hanjun Guo]

Hi Marc,

On 2016/9/15 16:49, Marc Zyngier wrote:

On 14/09/16 15:21, Hanjun Guo wrote:

From: Hanjun Guo 

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.

For mbi-gen,
Device(MBI0) {
  Name(_HID, "HISI0152")
  Name(_UID, Zero)
  Name(_CRS, ResourceTemplate() {
  Memory32Fixed(ReadWrite, 0xa008, 0x1)
  })

  Name (_PRS, ResourceTemplate() {
  Interrupt(ResourceProducer,...) {12,14,}
  })


Since I know next to nothing about all of this, I'm going to play the
village idiot. What makes it legal to use _PRS as a way to describe the
interrupts that are exposed by MBI0? Looking at the 6.0 spec, I do not
see why the interrupts would be put there instead of _CRS, and why you'd
have a _PRS at all.


_PRS describes possible resource settings for the device, which returns
a list of a device's possible resource settings such as memory range,
interrupt descriptors, and the format of the data in a _PRS object
follows the same format as _CRS object (ACPI 6.1, section 6.2.12),
so Interrupts can be put in the _PRS.

And in ACPI 6.1, section 6.2, it describes the _PRS usage:

"Some resources, however, may be shared amongst several devices. To
describe this, devices that share a resource (resource consumers) must
use the extended resource descriptors (0x7-0xA) described in Section
6.4.3, “Large Resource Data Type.” These descriptors point to a single
device object (resource producer) that claims the shared resource in
its _PRS."

As mbigen is a interrupt producer which provide interrupt resoures
for devices, which matches the usage of _PRS in the spec.



Also, are you going to exhaustively describe all the possible interrupts
via this method? Knowing that the mbigen can expose thousands of
interrupts, I find it slightly mad. Can't you express a range?


Yes, a little bit mad. I can't express a interrupt range in ACPI at
least in current version of spec. But that just adding more lines
of ACPI DSDT code, it's fine to me. or we need to use _DSD to present
similar property "num_pins" in ACPI which I avoid using.

Thanks
Hanjun

Re: [RFC PATCH v2 10/11] irqchip: mbigen: Add ACPI support

[Marc Zyngier]

On 14/09/16 15:21, Hanjun Guo wrote:
> From: Hanjun Guo 
> 
> 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.
> 
> For mbi-gen,
> Device(MBI0) {
>   Name(_HID, "HISI0152")
>   Name(_UID, Zero)
>   Name(_CRS, ResourceTemplate() {
>   Memory32Fixed(ReadWrite, 0xa008, 0x1)
>   })
> 
>   Name (_PRS, ResourceTemplate() {
> Interrupt(ResourceProducer,...) {12,14,}
>   })

Since I know next to nothing about all of this, I'm going to play the
village idiot. What makes it legal to use _PRS as a way to describe the
interrupts that are exposed by MBI0? Looking at the 6.0 spec, I do not
see why the interrupts would be put there instead of _CRS, and why you'd
have a _PRS at all.

Also, are you going to exhaustively describe all the possible interrupts
via this method? Knowing that the mbigen can expose thousands of
interrupts, I find it slightly mad. Can't you express a range?

> }
> 
> For devices,
> 
>Device(COM0) {
>   Name(_HID, "ACPIIDxx")
>   Name(_UID, Zero)
>   Name(_CRS, ResourceTemplate() {
>  Memory32Fixed(ReadWrite, 0xb003, 0x1)
>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.
> 
> Cc: Marc Zyngier 
> Cc: Thomas Gleixner 
> Cc: Ma Jun 
> Signed-off-by: Hanjun Guo 
> ---
>  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 9484ea0..ca6add1 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,6 +16,7 @@
>   * along with this program.  If not, see .
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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 = >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(>dev), METHOD_NAME__PRS,
> +  mbigen_acpi_process_resource, _msis);
> +if (ACPI_FAILURE(status) || num_msis == 0)
> + return -EINVAL;
> +
> + domain = platform_msi_create_device_domain(>dev, num_msis,
> +mbigen_write_msg,
> +_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(>dev))
> + err = mbigen_acpi_create_domain(pdev, mgn_chip);
> + else
> + err = -EINVAL;
> +
> + if (err) {
> + 

Re: [RFC PATCH v2 10/11] irqchip: mbigen: Add ACPI support

[Marc Zyngier]

On 14/09/16 15:21, Hanjun Guo wrote:
> From: Hanjun Guo 
> 
> 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.
> 
> For mbi-gen,
> Device(MBI0) {
>   Name(_HID, "HISI0152")
>   Name(_UID, Zero)
>   Name(_CRS, ResourceTemplate() {
>   Memory32Fixed(ReadWrite, 0xa008, 0x1)
>   })
> 
>   Name (_PRS, ResourceTemplate() {
> Interrupt(ResourceProducer,...) {12,14,}
>   })

Since I know next to nothing about all of this, I'm going to play the
village idiot. What makes it legal to use _PRS as a way to describe the
interrupts that are exposed by MBI0? Looking at the 6.0 spec, I do not
see why the interrupts would be put there instead of _CRS, and why you'd
have a _PRS at all.

Also, are you going to exhaustively describe all the possible interrupts
via this method? Knowing that the mbigen can expose thousands of
interrupts, I find it slightly mad. Can't you express a range?

> }
> 
> For devices,
> 
>Device(COM0) {
>   Name(_HID, "ACPIIDxx")
>   Name(_UID, Zero)
>   Name(_CRS, ResourceTemplate() {
>  Memory32Fixed(ReadWrite, 0xb003, 0x1)
>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.
> 
> Cc: Marc Zyngier 
> Cc: Thomas Gleixner 
> Cc: Ma Jun 
> Signed-off-by: Hanjun Guo 
> ---
>  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 9484ea0..ca6add1 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,6 +16,7 @@
>   * along with this program.  If not, see .
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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 = >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(>dev), METHOD_NAME__PRS,
> +  mbigen_acpi_process_resource, _msis);
> +if (ACPI_FAILURE(status) || num_msis == 0)
> + return -EINVAL;
> +
> + domain = platform_msi_create_device_domain(>dev, num_msis,
> +mbigen_write_msg,
> +_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(>dev))
> + err = mbigen_acpi_create_domain(pdev, mgn_chip);
> + else
> + err = -EINVAL;
> +
> + if (err) {
> + dev_err(>dev, "Failed to create mbi-gen@%p irqdomain", 
> mgn_chip->base);
>   return err;
> + }
>