Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331103 > unrolled thread

Re: [PATCH V3 10/10] acpi, gicv3, its: Use MADT ITS subtable to do PCI/MSI domain initialization.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 10/10] acpi, gicv3, its: Use MADT ITS subtable to do  PCI/MSI domain initialization. Marc Zyngier <marc.zyngier@arm.com> - 2016-02-10 13:10 +0100
    Re: [PATCH V3 10/10] acpi, gicv3, its: Use MADT ITS subtable to do  PCI/MSI domain initialization. Tomasz Nowicki <tn@semihalf.com> - 2016-02-12 13:30 +0100
      Re: [PATCH V3 10/10] acpi, gicv3, its: Use MADT ITS subtable to do  PCI/MSI domain initialization. Marc Zyngier <marc.zyngier@arm.com> - 2016-02-12 14:30 +0100

#1331103 — Re: [PATCH V3 10/10] acpi, gicv3, its: Use MADT ITS subtable to do PCI/MSI domain initialization.

On 19/01/16 13:11, Tomasz Nowicki wrote:
> After refactoring DT code, we let ACPI to build ITS PCI MSI domain
> and do requester ID to device ID translation using IORT table.
> 
> We have now full PCI MSI domain stack, thus we can enable ITS initialization
> from GICv3 core driver for ACPI scenario.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++-
>  drivers/irqchip/irq-gic-v3.c             |  3 +--
>  drivers/pci/msi.c                        |  3 +++
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index 06165cb..7f0a958 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -15,6 +15,8 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/acpi.h>
> +#include <linux/iort.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +
> +static int __init
> +its_pci_msi_parse_madt(struct acpi_subtable_header *header,
> +		    const unsigned long end)
> +{
> +	struct acpi_madt_generic_translator *its_entry;
> +	struct fwnode_handle *domain_handle;
> +
> +	its_entry = (struct acpi_madt_generic_translator *)header;
> +	domain_handle = iort_find_its_domain_token(its_entry->translation_id);
> +	if (!domain_handle) {
> +		pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n",
> +		       (long)its_entry->base_address);
> +		return 0;
> +	}
> +
> +	if (its_pci_msi_init_one(domain_handle))
> +		return 0;
> +
> +	pci_msi_register_fwnode_provider(&iort_find_pci_domain_token);

I'm a bit worried by this. You are registering this for each and every
ITS that gets probed (useless, but why not). But also, you're using a
hook that is designed to work at the bus level, without caring for the
actual PCI devices. That's fine for something like GICv2m, which exposes
a single domain, but I can't picture how this works when you have
devices sitting behind a single RC that talk to different ITSs.

My understanding is that IORT was behaving in a similar way the msi-map
property works, so I'm a bit puzzled here.

Can you please shed some light on that?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [next] | [standalone]

#1332701

+ Charles

On 10.02.2016 13:02, Marc Zyngier wrote:
> On 19/01/16 13:11, Tomasz Nowicki wrote:
>> After refactoring DT code, we let ACPI to build ITS PCI MSI domain
>> and do requester ID to device ID translation using IORT table.
>>
>> We have now full PCI MSI domain stack, thus we can enable ITS initialization
>> from GICv3 core driver for ACPI scenario.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++-
>>   drivers/irqchip/irq-gic-v3.c             |  3 +--
>>   drivers/pci/msi.c                        |  3 +++
>>   3 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> index 06165cb..7f0a958 100644
>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> @@ -15,6 +15,8 @@
>>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>    */
>>
>> +#include <linux/acpi.h>
>> +#include <linux/iort.h>
>>   #include <linux/msi.h>
>>   #include <linux/of.h>
>>   #include <linux/of_irq.h>
>> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void)
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +
>> +static int __init
>> +its_pci_msi_parse_madt(struct acpi_subtable_header *header,
>> +		    const unsigned long end)
>> +{
>> +	struct acpi_madt_generic_translator *its_entry;
>> +	struct fwnode_handle *domain_handle;
>> +
>> +	its_entry = (struct acpi_madt_generic_translator *)header;
>> +	domain_handle = iort_find_its_domain_token(its_entry->translation_id);
>> +	if (!domain_handle) {
>> +		pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n",
>> +		       (long)its_entry->base_address);
>> +		return 0;
>> +	}
>> +
>> +	if (its_pci_msi_init_one(domain_handle))
>> +		return 0;
>> +
>> +	pci_msi_register_fwnode_provider(&iort_find_pci_domain_token);
>
> I'm a bit worried by this. You are registering this for each and every
> ITS that gets probed (useless, but why not). But also, you're using a
> hook that is designed to work at the bus level, without caring for the
> actual PCI devices. That's fine for something like GICv2m, which exposes
> a single domain, but I can't picture how this works when you have
> devices sitting behind a single RC that talk to different ITSs.
>
> My understanding is that IORT was behaving in a similar way the msi-map
> property works, so I'm a bit puzzled here.
>
> Can you please shed some light on that?
>

I see your point now. It is possible to describe such case in IORT, for 
example:

********************************************
RC0 node:
---------------
Mapping 0:
<input ID range> -> <output ID range>
<0:100> -> <0:100>
parent -> ITS0
---------------
Mapping 1:
<input ID range> -> <output ID range>
<101:200> -> <101:200>
parent -> ITS1
---------------
********************************************

So for this scenario I cannot use pci_host_bridge_acpi_msi_domain() to 
find IRQ domain based on bus device (unless there is only one ITS bound 
to e.g. RC), I should rather add ACPI implementation to 
pci_msi_get_device_domain on per-device MSI basis. Do you agree?

BTW. I should have put IORT specification link to changelog:
http://infocenter.arm.com/help/topic/com.arm.doc.den0049a/DEN0049A_IO_Remapping_Table.pdf

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1332733

On 12/02/16 12:26, Tomasz Nowicki wrote:
> + Charles
> 
> On 10.02.2016 13:02, Marc Zyngier wrote:
>> On 19/01/16 13:11, Tomasz Nowicki wrote:
>>> After refactoring DT code, we let ACPI to build ITS PCI MSI domain
>>> and do requester ID to device ID translation using IORT table.
>>>
>>> We have now full PCI MSI domain stack, thus we can enable ITS initialization
>>> from GICv3 core driver for ACPI scenario.
>>>
>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>> ---
>>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++-
>>>   drivers/irqchip/irq-gic-v3.c             |  3 +--
>>>   drivers/pci/msi.c                        |  3 +++
>>>   3 files changed, 47 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>> index 06165cb..7f0a958 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>> @@ -15,6 +15,8 @@
>>>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>    */
>>>
>>> +#include <linux/acpi.h>
>>> +#include <linux/iort.h>
>>>   #include <linux/msi.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_irq.h>
>>> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void)
>>>   	return 0;
>>>   }
>>>
>>> +#ifdef CONFIG_ACPI
>>> +
>>> +static int __init
>>> +its_pci_msi_parse_madt(struct acpi_subtable_header *header,
>>> +		    const unsigned long end)
>>> +{
>>> +	struct acpi_madt_generic_translator *its_entry;
>>> +	struct fwnode_handle *domain_handle;
>>> +
>>> +	its_entry = (struct acpi_madt_generic_translator *)header;
>>> +	domain_handle = iort_find_its_domain_token(its_entry->translation_id);
>>> +	if (!domain_handle) {
>>> +		pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n",
>>> +		       (long)its_entry->base_address);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (its_pci_msi_init_one(domain_handle))
>>> +		return 0;
>>> +
>>> +	pci_msi_register_fwnode_provider(&iort_find_pci_domain_token);
>>
>> I'm a bit worried by this. You are registering this for each and every
>> ITS that gets probed (useless, but why not). But also, you're using a
>> hook that is designed to work at the bus level, without caring for the
>> actual PCI devices. That's fine for something like GICv2m, which exposes
>> a single domain, but I can't picture how this works when you have
>> devices sitting behind a single RC that talk to different ITSs.
>>
>> My understanding is that IORT was behaving in a similar way the msi-map
>> property works, so I'm a bit puzzled here.
>>
>> Can you please shed some light on that?
>>
> 
> I see your point now. It is possible to describe such case in IORT, for 
> example:
> 
> ********************************************
> RC0 node:
> ---------------
> Mapping 0:
> <input ID range> -> <output ID range>
> <0:100> -> <0:100>
> parent -> ITS0
> ---------------
> Mapping 1:
> <input ID range> -> <output ID range>
> <101:200> -> <101:200>
> parent -> ITS1
> ---------------
> ********************************************
> 
> So for this scenario I cannot use pci_host_bridge_acpi_msi_domain() to 
> find IRQ domain based on bus device (unless there is only one ITS bound 
> to e.g. RC), I should rather add ACPI implementation to 
> pci_msi_get_device_domain on per-device MSI basis. Do you agree?

That's what I was angling for, having had that in mind when writing that
particular piece of code (see 54fa97eeb for the rational).

You should also split the plugging of IORT into the MSI layer from the
ITS driver (last hunk of this patch).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web