Re: [RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

Lorenzo Pieralisi
2017-08-11T09:31:46Z

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.

I can rework the patch to show you what I have in mind, please let
me know.

Thanks,
Lorenzo

> +
> ret = platform_device_add(pdev);
> if (ret)
> goto dma_deconfigure;
> --
> 1.7.12.4
>