Re: [PATCH 1/2] acpi: arm64: add iort support for PMCG

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Fri, Aug 04, 2017 at 03:59:13PM -0400, Neil Leeder wrote:
> Add support for the SMMU Performance Monitor Counter Group
> information from ACPI. This is in preparation for its use
> in the SMMU v3 PMU driver.
> 
> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> ---
>  drivers/acpi/arm64/iort.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/actbl2.h     |  9 +++++++-
>  2 files changed, 62 insertions(+), 1 deletion(-)

Please CC me for next versions.

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index a3215ee..5a998cd 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -970,6 +970,40 @@ static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
>  	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
>  }
>  
> +static int __init arm_smmu_pmu_count_resources(struct acpi_iort_node *node)
> +{
> +	struct acpi_iort_pmcg *pmcg;
> +
> +	/* Retrieve PMCG specific data */
> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> +
> +	/*
> +	 * There are always 2 memory resources.
> +	 * If the overflow_gsiv is present then add that for a total of 3.
> +	 */
> +	return pmcg->overflow_gsiv > 0 ? 3 : 2;
> +}
> +
> +static void __init arm_smmu_pmu_init_resources(struct resource *res,
> +					       struct acpi_iort_node *node)
> +{
> +	struct acpi_iort_pmcg *pmcg;
> +
> +	/* Retrieve PMCG specific data */
> +	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> +
> +	res[0].start = pmcg->base_address;
> +	res[0].end = pmcg->base_address + SZ_4K - 1;
> +	res[0].flags = IORESOURCE_MEM;
> +	res[1].start = pmcg->base_address + SZ_64K;
> +	res[1].end = pmcg->base_address + SZ_64K + SZ_4K - 1;
> +	res[1].flags = IORESOURCE_MEM;
> +
> +	if (pmcg->overflow_gsiv)
> +		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
> +				       ACPI_EDGE_SENSITIVE, &res[2]);
> +}
> +
>  struct iort_iommu_config {
>  	const char *name;
>  	int (*iommu_init)(struct acpi_iort_node *node);
> @@ -993,6 +1027,12 @@ struct iort_iommu_config {
>  	.iommu_init_resources = arm_smmu_init_resources
>  };
>  
> +static const struct iort_iommu_config iort_arm_smmu_pmcg_cfg __initconst = {
> +	.name = "arm-smmu-pmu",
> +	.iommu_count_resources = arm_smmu_pmu_count_resources,
> +	.iommu_init_resources = arm_smmu_pmu_init_resources
> +};
> +
>  static __init
>  const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
>  {
> @@ -1001,6 +1041,8 @@ const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
>  		return &iort_arm_smmu_v3_cfg;
>  	case ACPI_IORT_NODE_SMMU:
>  		return &iort_arm_smmu_cfg;
> +	case ACPI_IORT_NODE_PMCG:
> +		return &iort_arm_smmu_pmcg_cfg;
>  	default:
>  		return NULL;
>  	}
> @@ -1056,6 +1098,15 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>  	if (ret)
>  		goto dev_put;
>  
> +	/* End of init for PMCG */
> +	if (node->type == ACPI_IORT_NODE_PMCG) {
> +		ret = platform_device_add(pdev);
> +		if (ret)
> +			goto dev_put;
> +
> +		return 0;
> +	}
> +
>  	/*
>  	 * We expect the dma masks to be equivalent for
>  	 * all SMMUs set-ups
> @@ -1131,6 +1182,9 @@ static void __init iort_init_platform_devices(void)
>  				acpi_free_fwnode_static(fwnode);
>  				return;
>  			}
> +		} else if (iort_node->type == ACPI_IORT_NODE_PMCG) {
> +			if (iort_add_smmu_platform_device(iort_node))
> +				return;


It is becoming a bit messy, probably it makes sense to rework the
iommu platform device creation to make room for more generic (ie
iommu platform device creation is not really iommu specific) behaviour
that can accommodate the PMCG too, it can be made cleaner.

I do not know if we can make it for this cycle but I am happy to put
a patch together shortly with this in mind.

>  		}
>  
>  		iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 707dda74..2169b6f 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -695,7 +695,8 @@ enum acpi_iort_node_type {
>  	ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
>  	ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
>  	ACPI_IORT_NODE_SMMU = 0x03,
> -	ACPI_IORT_NODE_SMMU_V3 = 0x04
> +	ACPI_IORT_NODE_SMMU_V3 = 0x04,
> +	ACPI_IORT_NODE_PMCG = 0x05
>  };
>  
>  struct acpi_iort_id_mapping {
> @@ -811,6 +812,12 @@ struct acpi_iort_smmu_v3 {
>  #define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE   (1)
>  #define ACPI_IORT_SMMU_V3_HTTU_OVERRIDE     (1<<1)
>  
> +struct acpi_iort_pmcg {
> +	u64 base_address;	/* PMCG base address */
> +	u32 overflow_gsiv;
> +	u32 node_reference;
> +};

As Robin already said this hunk should be made an ACPICA pull but
NOT before seeking IORT specs clarification as per his comments.

Thanks,
Lorenzo

> +
>  /*******************************************************************************
>   *
>   * IVRS - I/O Virtualization Reporting Structure
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>