| Message ID | 20170815171525.GB13707@e107981-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lorenzo, On 2017/8/16 1:15, Lorenzo Pieralisi wrote: > On Fri, Aug 11, 2017 at 09:56:31PM +0800, Hanjun Guo wrote: >> Hi Lorenzo, >> >> On 11 August 2017 at 17:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: >>> On Wed, Aug 09, 2017 at 06:53:37PM +0800, Hanjun Guo wrote: >>>> From: Hanjun Guo <hanjun.guo@linaro.org> >>>> >>>> Since we have a mapping index for SMMUv3 MSI, we can >>>> directly use that index to get the map entry, then >>>> retrieve dev ID and ITS parent to add SMMUv3 MSI >>>> support. >>>> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++---------- >>>> 1 file changed, 36 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>>> index 9439f02..ce03298 100644 >>>> --- a/drivers/acpi/arm64/iort.c >>>> +++ b/drivers/acpi/arm64/iort.c >>>> @@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, >>>> /* Single mapping does not care for input id */ >>>> if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) { >>>> if (type == ACPI_IORT_NODE_NAMED_COMPONENT || >>>> - type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) { >>>> + type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX || >>>> + type == ACPI_IORT_NODE_SMMU_V3) { >>>> *rid_out = map->output_base; >>>> return 0; >>>> } >>>> @@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node, >>>> >>>> if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) { >>>> if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT || >>>> - node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) { >>>> + node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX || >>>> + node->type == ACPI_IORT_NODE_SMMU_V3) { >>>> *id_out = map->output_base; >>>> return parent; >>>> } >>>> @@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id) >>>> if (!node) >>>> return -ENODEV; >>>> >>>> - for (i = 0; i < node->mapping_count; i++) { >>>> - if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i)) >>>> + if (node->type == ACPI_IORT_NODE_SMMU_V3) { >>>> + u32 index; >>>> + >>>> + if (iort_get_smmu_v3_id_mapping_index(node, &index)) >>>> + return -ENODEV; >>>> + >>>> + if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, >>>> + index)) >>>> return 0; >>>> + } else { >>>> + for (i = 0; i < node->mapping_count; i++) { >>>> + if (iort_node_map_platform_id(node, dev_id, >>>> + IORT_MSI_TYPE, i)) >>>> + return 0; >>>> + } >>>> } >>>> >>>> return -ENODEV; >>>> @@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev) >>>> struct acpi_iort_node *node, *msi_parent; >>>> struct fwnode_handle *iort_fwnode; >>>> struct acpi_iort_its_group *its; >>>> - int i; >>>> >>>> /* find its associated iort node */ >>>> - node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, >>>> - iort_match_node_callback, dev); >>>> + node = iort_find_dev_node(dev); >>>> if (!node) >>>> return NULL; >>>> >>>> /* then find its msi parent node */ >>>> - for (i = 0; i < node->mapping_count; i++) { >>>> + if (node->type == ACPI_IORT_NODE_SMMU_V3) { >>>> + u32 index; >>>> + >>>> + if (iort_get_smmu_v3_id_mapping_index(node, &index)) >>>> + return NULL; >>>> + >>>> msi_parent = iort_node_map_platform_id(node, NULL, >>>> + IORT_MSI_TYPE, index); >>>> + } else { >>>> + int i; >>>> + >>>> + for (i = 0; i < node->mapping_count; i++) { >>>> + msi_parent = iort_node_map_platform_id(node, NULL, >>>> IORT_MSI_TYPE, i); >>>> - if (msi_parent) >>>> - break; >>>> + if (msi_parent) >>>> + break; >>>> + } >>>> } >>>> >>>> if (!msi_parent) >>>> @@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) >>>> /* Configure DMA for the page table walker */ >>>> acpi_dma_configure(&pdev->dev, attr); >>>> >>>> + acpi_configure_pmsi_domain(&pdev->dev); >>> >>> I think this is just overkill. There are two separate things to solve >>> here: >>> >>> 1) Make single mappings valid for SMMUv3 (and PMCG); that's fair enough >>> and goes with the logic to skip the ITS DeviceID index for "normal" >>> mappings, I can live with that >>> 2) Find the MSI domain for an SMMUv3 (or any other IORT table node); I >>> do not think you need all this complexity to do it via >>> acpi_configure_pmsi_domain(), it can be done in a easier way with >>> an ad-hoc stub (it does not even have to be SMMUv3 specific) >>> >>> My worry is that we are peppering the generic IORT mapping code with >>> node types specific kludges and it is becoming a mess. >> >> Agreed. >> >>> >>> I can rework the patch to show you what I have in mind, please let >>> me know. >> >> Please, thank you very much for doing so. > > Here, untested just to get your opinion, please let me know. > > Thanks, > Lorenzo > > -- >8 -- > Subject: [PATCH 4/4] ACPI/IORT: IORT nodes MSI handling > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > --- > drivers/acpi/arm64/iort.c | 76 +++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 63 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 24678c3..21a0aab 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -366,7 +366,7 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node, > return NULL; > } > > -static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node, > +static int iort_get_id_mapping_index(struct acpi_iort_node *node, > u32 *index) > { > struct acpi_iort_smmu_v3 *smmu; > @@ -378,20 +378,25 @@ static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node, > if (node->revision < 1) > return -EINVAL; > > - smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > - /* if any of the gsi for control interrupts is not 0, ignore the MSI */ > - if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv > - || smmu->sync_gsiv) > - return -EINVAL; > + switch (node->type) { > + case ACPI_IORT_NODE_SMMU_V3: > + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > + /* if any of the gsi for control interrupts is not 0, ignore the MSI */ > + if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv > + || smmu->sync_gsiv) > + return -EINVAL; > + > + if (smmu->id_mapping_index >= node->mapping_count) { > + pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n", > + node, node->type); > + return -EINVAL; > + } > > - if (smmu->id_mapping_index >= node->mapping_count) { > - pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n", > - node, node->type); > + *index = smmu->id_mapping_index; > + return 0; > + default: > return -EINVAL; > } > - > - *index = smmu->id_mapping_index; > - return 0; > } > > static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node, > @@ -431,7 +436,7 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node, > * associated ID map for single mapping cases. > */ > if (node->type == ACPI_IORT_NODE_SMMU_V3) > - ret = iort_get_smmu_v3_id_mapping_index(node, &index); > + ret = iort_get_id_mapping_index(node, &index); > > /* Do the ID translation */ > for (i = 0; i < node->mapping_count; i++, map++) { > @@ -620,6 +625,49 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id) > return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI); > } > > +static void iort_set_device_domain(struct device *dev, > + struct acpi_iort_node *node) > +{ > + struct acpi_iort_its_group *its; > + struct acpi_iort_node *msi_parent; > + struct acpi_iort_id_mapping *map; > + struct fwnode_handle *iort_fwnode; > + struct irq_domain *domain; > + int ret, index; > + > + ret = iort_get_id_mapping_index(node, &index); > + if (ret < 0) > + return; > + > + map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node, > + node->mapping_offset + index * sizeof(*map)); > + > + /* Firmware bug! */ > + if (!map->output_reference || > + !(map->flags & ACPI_IORT_ID_SINGLE_MAPPING)) { > + pr_err(FW_BUG "[node %p type %d] Invalid MSI mapping\n", > + node, node->type); > + return; > + } > + > + msi_parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table, > + map->output_reference); > + > + if (!msi_parent || msi_parent->type != ACPI_IORT_NODE_ITS_GROUP) > + return; > + > + /* Move to ITS specific data */ > + its = (struct acpi_iort_its_group *)msi_parent->node_data; > + > + iort_fwnode = iort_find_domain_token(its->identifiers[0]); > + if (!iort_fwnode) > + return; > + > + domain = irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI); > + if (domain) > + dev_set_msi_domain(dev, domain); > +} > + > /** > * iort_get_platform_device_domain() - Find MSI domain related to a > * platform device > @@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) > /* Configure DMA for the page table walker */ > acpi_dma_configure(&pdev->dev, attr); > > + iort_set_device_domain(&pdev->dev, node); This is fine to me, but I think we still need to retrieve the output ID as the request id for ITS, which means we need to do that in iort_pmsi_get_dev_id(), right? Thanks Hanjun
On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote: [...] > > /** > > * iort_get_platform_device_domain() - Find MSI domain related to a > > * platform device > >@@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) > > /* Configure DMA for the page table walker */ > > acpi_dma_configure(&pdev->dev, attr); > >+ iort_set_device_domain(&pdev->dev, node); > > This is fine to me, but I think we still need to retrieve > the output ID as the request id for ITS, which means we > need to do that in iort_pmsi_get_dev_id(), right? Yes, probably we can add code there like: ret = iort_get_id_mapping_index(&index); if (!ret) iort_node_get_id(node, &id, index); ... This requires updating iort_node_get_id() to allow single mappings for SMMU/PMCG as you flagged up before. I will think a bit more to see if there is a cleaner way to reshuffle the mapping API. Thanks, Lorenzo
Hi Lorenzo, On 2017/8/17 21:01, Lorenzo Pieralisi wrote: > On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote: > > [...] > >>> /** >>> * iort_get_platform_device_domain() - Find MSI domain related to a >>> * platform device >>> @@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) >>> /* Configure DMA for the page table walker */ >>> acpi_dma_configure(&pdev->dev, attr); >>> + iort_set_device_domain(&pdev->dev, node); >> >> This is fine to me, but I think we still need to retrieve >> the output ID as the request id for ITS, which means we >> need to do that in iort_pmsi_get_dev_id(), right? > > Yes, probably we can add code there like: > > ret = iort_get_id_mapping_index(&index); > if (!ret) > iort_node_get_id(node, &id, index); > ... > > This requires updating iort_node_get_id() to allow single mappings > for SMMU/PMCG as you flagged up before. > > I will think a bit more to see if there is a cleaner way to reshuffle > the mapping API. I would like to send a updated version after the merge window to gather more comments, and at the same time we can work on the cleaner way to reshuffle the mapping API, what do you thing? Thanks Hanjun
On Tue, Sep 12, 2017 at 11:27:13AM +0800, Hanjun Guo wrote: > Hi Lorenzo, > > On 2017/8/17 21:01, Lorenzo Pieralisi wrote: > >On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote: > > > >[...] > > > >>> /** > >>> * iort_get_platform_device_domain() - Find MSI domain related to a > >>> * platform device > >>>@@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) > >>> /* Configure DMA for the page table walker */ > >>> acpi_dma_configure(&pdev->dev, attr); > >>>+ iort_set_device_domain(&pdev->dev, node); > >> > >>This is fine to me, but I think we still need to retrieve > >>the output ID as the request id for ITS, which means we > >>need to do that in iort_pmsi_get_dev_id(), right? > > > >Yes, probably we can add code there like: > > > >ret = iort_get_id_mapping_index(&index); > >if (!ret) > > iort_node_get_id(node, &id, index); > > ... > > > >This requires updating iort_node_get_id() to allow single mappings > >for SMMU/PMCG as you flagged up before. > > > >I will think a bit more to see if there is a cleaner way to reshuffle > >the mapping API. > > I would like to send a updated version after the merge window to gather > more comments, and at the same time we can work on the cleaner way to > reshuffle the mapping API, what do you thing? I think you should send an updated patch at -rc1 with comments above, we can rework the API later or I can do it on top of your patches for the upcoming merge window. Thanks ! Lorenzo
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 24678c3..21a0aab 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -366,7 +366,7 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node, return NULL; } -static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node, +static int iort_get_id_mapping_index(struct acpi_iort_node *node, u32 *index) { struct acpi_iort_smmu_v3 *smmu; @@ -378,20 +378,25 @@ static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node, if (node->revision < 1) return -EINVAL; - smmu = (struct acpi_iort_smmu_v3 *)node->node_data; - /* if any of the gsi for control interrupts is not 0, ignore the MSI */ - if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv - || smmu->sync_gsiv) - return -EINVAL; + switch (node->type) { + case ACPI_IORT_NODE_SMMU_V3: + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; + /* if any of the gsi for control interrupts is not 0, ignore the MSI */ + if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv + || smmu->sync_gsiv) + return -EINVAL; + + if (smmu->id_mapping_index >= node->mapping_count) { + pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n", + node, node->type); + return -EINVAL; + } - if (smmu->id_mapping_index >= node->mapping_count) { - pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n", - node, node->type); + *index = smmu->id_mapping_index; + return 0; + default: return -EINVAL; } - - *index = smmu->id_mapping_index; - return 0; } static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node, @@ -431,7 +436,7 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node, * associated ID map for single mapping cases. */ if (node->type == ACPI_IORT_NODE_SMMU_V3) - ret = iort_get_smmu_v3_id_mapping_index(node, &index); + ret = iort_get_id_mapping_index(node, &index); /* Do the ID translation */ for (i = 0; i < node->mapping_count; i++, map++) { @@ -620,6 +625,49 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id) return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI); } +static void iort_set_device_domain(struct device *dev, + struct acpi_iort_node *node) +{ + struct acpi_iort_its_group *its; + struct acpi_iort_node *msi_parent; + struct acpi_iort_id_mapping *map; + struct fwnode_handle *iort_fwnode; + struct irq_domain *domain; + int ret, index; + + ret = iort_get_id_mapping_index(node, &index); + if (ret < 0) + return; + + map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node, + node->mapping_offset + index * sizeof(*map)); + + /* Firmware bug! */ + if (!map->output_reference || + !(map->flags & ACPI_IORT_ID_SINGLE_MAPPING)) { + pr_err(FW_BUG "[node %p type %d] Invalid MSI mapping\n", + node, node->type); + return; + } + + msi_parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table, + map->output_reference); + + if (!msi_parent || msi_parent->type != ACPI_IORT_NODE_ITS_GROUP) + return; + + /* Move to ITS specific data */ + its = (struct acpi_iort_its_group *)msi_parent->node_data; + + iort_fwnode = iort_find_domain_token(its->identifiers[0]); + if (!iort_fwnode) + return; + + domain = irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI); + if (domain) + dev_set_msi_domain(dev, domain); +} + /** * iort_get_platform_device_domain() - Find MSI domain related to a * platform device @@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node) /* Configure DMA for the page table walker */ acpi_dma_configure(&pdev->dev, attr); + iort_set_device_domain(&pdev->dev, node); + ret = platform_device_add(pdev); if (ret) goto dma_deconfigure;