| Message ID | 1501876754-1064-2-git-send-email-nleeder@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Neil, On 04/08/17 20:59, 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(-) > > 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; Ugh, I see there's a nasty spec hole here - IORT only defines one base address, but SMMUv3 says *both* pages have imp-def base addresses. I guess this assumption of them being in consecutive 64K regions holds for your platform, but as things stand it doesn't appear possible to rely on it generally :( I'll follow up internally to see if we need to get IORT and/or SBSA updated or clarified. > + 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; > } > > 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 Hopefully these changes are merely here for reference, but just in case it needs to be said, they should go as a separate patch via ACPICA (presumably there's also some corresponding iASL stuff to compile PMCG nodes in the first place). Robin. > @@ -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; > +}; > + > /******************************************************************************* > * > * IVRS - I/O Virtualization Reporting Structure >
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. >
Hi Robin, Thank you for the review. On 8/7/2017 7:17 AM, Robin Murphy wrote: > Hi Neil, > > On 04/08/17 20:59, Neil Leeder wrote: [...] >> + res[1].start = pmcg->base_address + SZ_64K; > > Ugh, I see there's a nasty spec hole here - IORT only defines one base > address, but SMMUv3 says *both* pages have imp-def base addresses. I > guess this assumption of them being in consecutive 64K regions holds for > your platform, but as things stand it doesn't appear possible to rely on > it generally :( > > I'll follow up internally to see if we need to get IORT and/or SBSA > updated or clarified. > Thanks. I'll wait for the outcome before submitting a new patch. [...] >> 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 > > Hopefully these changes are merely here for reference, but just in case > it needs to be said, they should go as a separate patch via ACPICA > (presumably there's also some corresponding iASL stuff to compile PMCG > nodes in the first place). > Yes, I'll submit this to ACPICA once the IORT addresses get figured out. Neil
Hi Lorenzo, On 8/7/2017 12:44 PM, Lorenzo Pieralisi wrote: > On Fri, Aug 04, 2017 at 03:59:13PM -0400, Neil Leeder wrote: [...] >> + } 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. > Ok, I will rebase on top of it when it's ready. >> } >> >> 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. > OK, I will add this through ACPICA once the IORT decision is made. Thanks, Neil
Hi Neil/Lorenzo, > -----Original Message----- > From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org] > On Behalf Of Neil Leeder > Sent: Friday, August 04, 2017 8:59 PM > To: Will Deacon <will.deacon@arm.com>; Mark Rutland > <mark.rutland@arm.com> > Cc: Mark Langsdorf <mlangsdo@redhat.com>; Jon Masters > <jcm@redhat.com>; Timur Tabi <timur@codeaurora.org>; linux- > kernel@vger.kernel.org; Mark Brown <broonie@kernel.org>; Mark Salter > <msalter@redhat.com>; nleeder@codeaurora.org; linux-arm- > kernel@lists.infradead.org > Subject: [PATCH 1/2] acpi: arm64: add iort support for PMCG > > 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. [...] > 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; I understand this series will be revised based on the iort spec update. This Is to clarify few queries we have with respect to one of our hisilicon platform which has support for SMMU v3 PMCG. In our implementation the base address for the PMCG is defined at a IMP DEF address offset within the SMMUv3 page 0 address space. And as per SMMU spec, " The Performance Monitor Counter Groups are standalone monitoring facilities and, as such, can be implemented in separate components that are all associated with (but not necessarily part of) an SMMU" It looks like PMCG can be part of SMMU, it is not clear whether this kind of address mapping is forbidden or not. If this is an accepted scenario, then the devm_ioremap_resource() call in SMMv3 probe will fail as it reports conflict. As far as I can see, the above code just checks the iort node type is PMCG and assumes that its SMMU PMCG. As per IORT spec, it make sense to check the node reference filed and make that call. And to fix the resource conflict issue we have, is it possible to treat the PMCG node as the child of the SMMU and call the platform_device_add() appropriately to avoid the conflict? I am not sure this will fix the issue and also about the order in which SMMU and PMCG devices will be populated will have an impact on this. Please let me know your thoughts on this. Thanks, Shameer > 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; > } > > 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; > +}; > + > > /**************************************************************** > *************** > * > * 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. > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 30, 2018 at 10:39:20AM +0000, Shameerali Kolothum Thodi wrote: > Hi Neil/Lorenzo, > > > -----Original Message----- > > From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org] > > On Behalf Of Neil Leeder > > Sent: Friday, August 04, 2017 8:59 PM > > To: Will Deacon <will.deacon@arm.com>; Mark Rutland > > <mark.rutland@arm.com> > > Cc: Mark Langsdorf <mlangsdo@redhat.com>; Jon Masters > > <jcm@redhat.com>; Timur Tabi <timur@codeaurora.org>; linux- > > kernel@vger.kernel.org; Mark Brown <broonie@kernel.org>; Mark Salter > > <msalter@redhat.com>; nleeder@codeaurora.org; linux-arm- > > kernel@lists.infradead.org > > Subject: [PATCH 1/2] acpi: arm64: add iort support for PMCG > > > > 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. > > [...] > > > 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; > > I understand this series will be revised based on the iort spec update. > > This Is to clarify few queries we have with respect to one of our hisilicon > platform which has support for SMMU v3 PMCG. In our implementation > the base address for the PMCG is defined at a IMP DEF address offset > within the SMMUv3 page 0 address space. And as per SMMU spec, > > " The Performance Monitor Counter Groups are standalone monitoring > facilities and, as such, can be implemented in separate components that > are all associated with (but not necessarily part of) an SMMU" > > It looks like PMCG can be part of SMMU, it is not clear whether this kind > of address mapping is forbidden or not. If this is an accepted scenario, then > the devm_ioremap_resource() call in SMMv3 probe will fail as it reports conflict. > > As far as I can see, the above code just checks the iort node type is PMCG > and assumes that its SMMU PMCG. As per IORT spec, it make sense to check > the node reference filed and make that call. > > And to fix the resource conflict issue we have, is it possible to treat the PMCG > node as the child of the SMMU and call the platform_device_add() appropriately > to avoid the conflict? I am not sure this will fix the issue and also about the order > in which SMMU and PMCG devices will be populated will have an impact on this. > > Please let me know your thoughts on this. I went back and re-read the patches, I think the point here is that the perf driver (ie PATCH 2 that, by the way, is not maiinline) uses devm_ioremap_resource() to map the counters and that's what is causing failures when PMCG is part of SMMUv3 registers. It is the resources hierarchy that is wrong and in turn, I do not think devm_request_mem_region() is the right way of requesting it for the PMCG driver. I need to look into this but I suspect that's something that should be handled in the PMCG driver, that has to request the memory region _differently_ (ie ioremap copes with this overlap - it is the devm_request_mem_region() in devm_ioremap_resource() that fails, correct ?). Certainly we need to create in IORT the resources with the correct hierarchy (I reckon DT does it already if the right device hierarchy is specified). Lorenzo > > Thanks, > Shameer > > > 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; > > } > > > > 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; > > +}; > > + > > > > /**************************************************************** > > *************** > > * > > * 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. > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Lorenzo, > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > Sent: Tuesday, January 30, 2018 6:00 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: Neil Leeder <nleeder@codeaurora.org>; Mark Langsdorf > <mlangsdo@redhat.com>; Jon Masters <jcm@redhat.com>; Timur Tabi > <timur@codeaurora.org>; linux-kernel@vger.kernel.org; Mark Brown > <broonie@kernel.org>; Mark Salter <msalter@redhat.com>; linux-arm- > kernel@lists.infradead.org; Will Deacon <will.deacon@arm.com>; Mark > Rutland <mark.rutland@arm.com>; Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH 1/2] acpi: arm64: add iort support for PMCG > > On Tue, Jan 30, 2018 at 10:39:20AM +0000, Shameerali Kolothum Thodi wrote: > > Hi Neil/Lorenzo, > > > > > -----Original Message----- > > > From: linux-arm-kernel [mailto:linux-arm-kernel- > bounces@lists.infradead.org] > > > On Behalf Of Neil Leeder > > > Sent: Friday, August 04, 2017 8:59 PM > > > To: Will Deacon <will.deacon@arm.com>; Mark Rutland > > > <mark.rutland@arm.com> > > > Cc: Mark Langsdorf <mlangsdo@redhat.com>; Jon Masters > > > <jcm@redhat.com>; Timur Tabi <timur@codeaurora.org>; linux- > > > kernel@vger.kernel.org; Mark Brown <broonie@kernel.org>; Mark Salter > > > <msalter@redhat.com>; nleeder@codeaurora.org; linux-arm- > > > kernel@lists.infradead.org > > > Subject: [PATCH 1/2] acpi: arm64: add iort support for PMCG > > > > > > 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. > > > > [...] > > > > > 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; > > > > I understand this series will be revised based on the iort spec update. > > > > This Is to clarify few queries we have with respect to one of our hisilicon > > platform which has support for SMMU v3 PMCG. In our implementation > > the base address for the PMCG is defined at a IMP DEF address offset > > within the SMMUv3 page 0 address space. And as per SMMU spec, > > > > " The Performance Monitor Counter Groups are standalone monitoring > > facilities and, as such, can be implemented in separate components that > > are all associated with (but not necessarily part of) an SMMU" > > > > It looks like PMCG can be part of SMMU, it is not clear whether this kind > > of address mapping is forbidden or not. If this is an accepted scenario, then > > the devm_ioremap_resource() call in SMMv3 probe will fail as it reports > conflict. > > > > As far as I can see, the above code just checks the iort node type is PMCG > > and assumes that its SMMU PMCG. As per IORT spec, it make sense to check > > the node reference filed and make that call. > > > > And to fix the resource conflict issue we have, is it possible to treat the PMCG > > node as the child of the SMMU and call the platform_device_add() > appropriately > > to avoid the conflict? I am not sure this will fix the issue and also about the > order > > in which SMMU and PMCG devices will be populated will have an impact on > this. > > > > Please let me know your thoughts on this. > > I went back and re-read the patches, I think the point here is that the > perf driver (ie PATCH 2 that, by the way, is not maiinline) uses > devm_ioremap_resource() to map the counters and that's what is causing > failures when PMCG is part of SMMUv3 registers. Thanks for going through this. No, this is not where we are seeing the failure. May be I was not clear in my earlier mail. The failure happens in SMMUv3 driver probe function when it calls devm_ioremap_resource(). > It is the resources hierarchy that is wrong and in turn, I do not think > devm_request_mem_region() is the right way of requesting it for the > PMCG driver. > > I need to look into this but I suspect that's something that should > be handled in the PMCG driver, that has to request the memory region > _differently_ (ie ioremap copes with this overlap - it is the > devm_request_mem_region() in devm_ioremap_resource() that fails, correct > ?). It looks like, in IORT code, iort_add_platform_device()--> platform_device_add()-->insert_resource(), inserts both SMMUv3 and PMCG resources into the resource tree and then when the probe of SMMUv3 is called, it detects the conflict. [ 85.548749] arm-smmu-v3 arm-smmu-v3.0.auto: can't request region for resource [mem 0x148000000-0x14801ffff] Of course, changing devm_ioremap_resource() to devm_ioremap() in SMMv3 driver probe solves the issue for us, but not sure that's the right approach or not. Thanks, Shameer > Certainly we need to create in IORT the resources with the correct > hierarchy (I reckon DT does it already if the right device hierarchy is > specified).
On Wed, Jan 31, 2018 at 12:10:47PM +0000, Shameerali Kolothum Thodi wrote: [...] > > I went back and re-read the patches, I think the point here is that the > > perf driver (ie PATCH 2 that, by the way, is not maiinline) uses > > devm_ioremap_resource() to map the counters and that's what is causing > > failures when PMCG is part of SMMUv3 registers. > > Thanks for going through this. No, this is not where we are seeing the failure. > May be I was not clear in my earlier mail. The failure happens in SMMUv3 > driver probe function when it calls devm_ioremap_resource(). Understood - because the PMU PMCG driver calls it first, that's what I was referring to. My point is that: - the PMCG platform device resources should be built with the correct resource hierarchy - and even then, I do not think that using devm_ioremap_resource() in the PMCG PMU driver is the correct way of handling its resource reservation (ie the kernel must be able to detect that a resource is contained in a parent one but I am not sure devm_ioremap_resource() is the way to handle this correctly) > > It is the resources hierarchy that is wrong and in turn, I do not think > > devm_request_mem_region() is the right way of requesting it for the > > PMCG driver. > > > > I need to look into this but I suspect that's something that should > > be handled in the PMCG driver, that has to request the memory region > > _differently_ (ie ioremap copes with this overlap - it is the > > devm_request_mem_region() in devm_ioremap_resource() that fails, correct > > ?). > > It looks like, in IORT code, > > iort_add_platform_device()--> platform_device_add()-->insert_resource(), inserts > both SMMUv3 and PMCG resources into the resource tree and then when the probe > of SMMUv3 is called, it detects the conflict. > > [ 85.548749] arm-smmu-v3 arm-smmu-v3.0.auto: can't request region for resource [mem 0x148000000-0x14801ffff] > > Of course, changing devm_ioremap_resource() to devm_ioremap() in SMMv3 > driver probe solves the issue for us, but not sure that's the right approach or not. See above. Lorenzo
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; } 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; +}; + /******************************************************************************* * * IVRS - I/O Virtualization Reporting Structure
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(-)