| Message ID | 20180724114515.21764-1-shameerali.kolothum.thodi@huawei.com |
|---|---|
| Headers | show |
| Series | arm64 SMMUv3 PMU driver with IORT support | expand |
Hi Lorenzo/Robin, Just a gentle ping on this series. This is a v2 for smmu pmcg support based on Neil Leeder's v1[1]. Main changes include, -an helper function to IORT to retrieve the associated SMMU info. -MSI support to the PMU driver. Please take a look and let me know your thoughts. Thanks, Shameer [1]https://www.spinics.net/lists/arm-kernel/msg598591.html > -----Original Message----- > From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of > Shameer Kolothum > Sent: 24 July 2018 12:45 > To: lorenzo.pieralisi@arm.com; robin.murphy@arm.com > Cc: mark.rutland@arm.com; vkilari@codeaurora.org; > neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com; > rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux- > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: [PATCH v2 0/4] arm64 SMMUv3 PMU driver with IORT support > > This adds a driver for the SMMUv3 PMU into the perf framework. > It includes an IORT update to support PM Counter Groups. > > This is based on the initial work done by Neil Leeder[1] > > SMMUv3 PMCG devices are named as arm_smmu_v3_x_pmcg_y where x > denotes the associated smmuv3 dev id(if any) and y denotes the > pmu dev id. > > Usage example: > For common arch supported events: > perf stat -e arm_smmu_v3_0_pmcg_6/transaction,filter_enable=1, > filter_span=1,filter_stream_id=0x42/ -a pwd > > For IMP DEF events: > perf stat -e arm_smmu_v3.0_pmcg.6/event=id/ -a pwd > > Sanity tested on HiSilicon platform. Further testing on supported > platforms are very much welcome. > > v1 --> v2 > > - Addressed comments from Robin. > - Added an helper to retrieve the associated smmu dev and named PMUs > to make the association visible to user. > - Added MSI support for overflow irq > > [1]https://www.spinics.net/lists/arm-kernel/msg598591.html > > Neil Leeder (2): > acpi: arm64: add iort support for PMCG > perf: add arm64 smmuv3 pmu driver > > Shameer Kolothum (2): > acpi: arm64: iort helper to find the associated smmu of pmcg node > perf/smmuv3: Add MSI irq support > > drivers/acpi/arm64/iort.c | 179 +++++++-- > drivers/perf/Kconfig | 9 + > drivers/perf/Makefile | 1 + > drivers/perf/arm_smmuv3_pmu.c | 901 > ++++++++++++++++++++++++++++++++++++++++++ > include/linux/acpi_iort.h | 4 + > 5 files changed, 1063 insertions(+), 31 deletions(-) > create mode 100644 drivers/perf/arm_smmuv3_pmu.c > > -- > 2.7.4 > > > _______________________________________________ > Linuxarm mailing list > Linuxarm@huawei.com > http://hulk.huawei.com/mailman/listinfo/linuxarm -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24/07/18 12:45, Shameer Kolothum wrote: > From: Neil Leeder <nleeder@codeaurora.org> > > 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> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > drivers/acpi/arm64/iort.c | 95 +++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 83 insertions(+), 12 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 7a3a541..ac4d0d6 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -356,7 +356,8 @@ static 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_SMMU_V3) { > + node->type == ACPI_IORT_NODE_SMMU_V3 || > + node->type == ACPI_IORT_NODE_PMCG) { > *id_out = map->output_base; > return parent; > } > @@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct acpi_iort_node *node) > } > > return smmu->id_mapping_index; > + case ACPI_IORT_NODE_PMCG: > + return 0; Why do we need a PMCG case here? AIUI this whole get_id_mapping_index business is only relevant to SMMUv3 nodes where we have some need to disambiguate the difference between the SMMU's own IDs and StreamID-to-DeviceID mappings within the same table. PMCGs simply have zero or one single ID mappings so should be equivalent to most named components (other than their mappings pointing straight to the ITS). > default: > return -EINVAL; > } > @@ -1287,6 +1290,63 @@ static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node) > return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK; > } > > +static void __init arm_smmu_common_dma_configure(struct device *dev, > + enum dev_dma_attr attr) > +{ > + /* We expect the dma masks to be equivalent for all SMMUs set-ups */ > + dev->dma_mask = &dev->coherent_dma_mask; > + > + /* Configure DMA for the page table walker */ > + acpi_dma_configure(dev, attr); Hmm, I don't think we actually need this call any more, since it should now happen later anyway via platform_dma_configure() as the relevant SMMU/PMCG driver binds. > +} > + > +static int __init arm_smmu_v3_pmu_count_resources(struct acpi_iort_node *node) Can we be consistent with "pmcg" rather than "pmu" within IORT please? > +{ > + 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_v3_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->page0_base_address; > + res[0].end = pmcg->page0_base_address + SZ_4K - 1; > + res[0].flags = IORESOURCE_MEM; > + res[1].start = pmcg->page1_base_address; > + res[1].end = pmcg->page1_base_address + 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]); > +} > + > +static struct acpi_iort_node *iort_find_pmcg_ref(struct acpi_iort_node *node) > +{ > + struct acpi_iort_pmcg *pmcg; > + struct acpi_iort_node *ref_node = NULL; > + > + /* Retrieve PMCG specific data */ > + pmcg = (struct acpi_iort_pmcg *)node->node_data; > + if (pmcg->node_reference) > + ref_node = ACPI_ADD_PTR(struct acpi_iort_node, > + iort_table, pmcg->node_reference); > + return ref_node; > +} > + > struct iort_dev_config { > const char *name; > int (*dev_init)(struct acpi_iort_node *node); > @@ -1296,6 +1356,8 @@ struct iort_dev_config { > struct acpi_iort_node *node); > void (*dev_set_proximity)(struct device *dev, > struct acpi_iort_node *node); > + void (*dev_dma_configure)(struct device *dev, > + enum dev_dma_attr attr); > }; > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > @@ -1304,23 +1366,38 @@ static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > .dev_count_resources = arm_smmu_v3_count_resources, > .dev_init_resources = arm_smmu_v3_init_resources, > .dev_set_proximity = arm_smmu_v3_set_proximity, > + .dev_dma_configure = arm_smmu_common_dma_configure > }; > > static const struct iort_dev_config iort_arm_smmu_cfg __initconst = { > .name = "arm-smmu", > .dev_is_coherent = arm_smmu_is_coherent, > .dev_count_resources = arm_smmu_count_resources, > - .dev_init_resources = arm_smmu_init_resources > + .dev_init_resources = arm_smmu_init_resources, > + .dev_dma_configure = arm_smmu_common_dma_configure > +}; > + > +static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = { > + .name = "arm-smmu-v3-pmu", > + .dev_count_resources = arm_smmu_v3_pmu_count_resources, > + .dev_init_resources = arm_smmu_v3_pmu_init_resources > }; > > static __init const struct iort_dev_config *iort_get_dev_cfg( > struct acpi_iort_node *node) > { > + struct acpi_iort_node *ref_node; > + > switch (node->type) { > case ACPI_IORT_NODE_SMMU_V3: > return &iort_arm_smmu_v3_cfg; > case ACPI_IORT_NODE_SMMU: > return &iort_arm_smmu_cfg; > + case ACPI_IORT_NODE_PMCG: > + /* Check this is associated with SMMUv3 */ > + ref_node = iort_find_pmcg_ref(node); This seems overly restrictive - admittedly the only non-SMMU DTI components I know of don't actually implement a PMCG for their internal TBU, but I'm sure something will eventually, and there doesn't seem to be any good reason for Linux to forcibly ignore it when it does. > + if (ref_node && ref_node->type == ACPI_IORT_NODE_SMMU_V3) > + return &iort_arm_smmu_v3_pmcg_cfg; > default: > return NULL; > } > @@ -1376,12 +1453,6 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node, > if (ret) > goto dev_put; > > - /* > - * We expect the dma masks to be equivalent for > - * all SMMUs set-ups > - */ > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > - > fwnode = iort_get_fwnode(node); > > if (!fwnode) { > @@ -1391,11 +1462,11 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node, > > pdev->dev.fwnode = fwnode; > > - attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ? > + if (ops->dev_dma_configure) { > + attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ? > DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT; Oh, right, these guys don't have an ACPI companion or a standard DMA attr because they're from a static table, so platform_dma_configure() won't pick them up. Oh well. We probably don't want to start dragging IORT internals into the platform bus code, so I guess it's not worth trying to change that. Robin. > - > - /* Configure DMA for the page table walker */ > - acpi_dma_configure(&pdev->dev, attr); > + ops->dev_dma_configure(&pdev->dev, attr); > + } > > iort_set_device_domain(&pdev->dev, node); > >
On 2018-07-24 12:45 PM, Shameer Kolothum wrote: > This adds support for MSI based counter overflow interrupt. > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > drivers/perf/arm_smmuv3_pmu.c | 105 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 84 insertions(+), 21 deletions(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index b3dc394..ca69813 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -94,6 +94,10 @@ > #define SMMU_PMCG_IRQ_CFG2 0xE64 > #define SMMU_PMCG_IRQ_STATUS 0xE68 > > +/* MSI config fields */ > +#define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2) > +#define MSI_CFG2_MEMATTR_DEVICE_nGnRE 0x1 > + > #define SMMU_COUNTER_RELOAD BIT(31) > #define SMMU_DEFAULT_FILTER_SEC 0 > #define SMMU_DEFAULT_FILTER_SPAN 1 > @@ -657,14 +661,89 @@ static irqreturn_t smmu_pmu_handle_irq(int irq_num, void *data) > return IRQ_HANDLED; > } > > +static void smmu_pmu_free_msis(void *data) > +{ > + struct device *dev = data; > + > + platform_msi_domain_free_irqs(dev); > +} > + > +static void smmu_pmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg) > +{ > + phys_addr_t doorbell; > + struct device *dev = msi_desc_to_dev(desc); > + struct smmu_pmu *pmu = dev_get_drvdata(dev); > + > + doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo; > + doorbell &= MSI_CFG0_ADDR_MASK; > + > + writeq_relaxed(doorbell, pmu->reg_base + SMMU_PMCG_IRQ_CFG0); > + writel_relaxed(msg->data, pmu->reg_base + SMMU_PMCG_IRQ_CFG1); > + writel_relaxed(MSI_CFG2_MEMATTR_DEVICE_nGnRE, > + pmu->reg_base + SMMU_PMCG_IRQ_CFG2); > +} > + > +static void smmu_pmu_setup_msi(struct smmu_pmu *pmu) > +{ > + struct msi_desc *desc; > + struct device *dev = pmu->dev; > + int ret; > + > + /* Clear MSI address reg */ > + writeq_relaxed(0, pmu->reg_base + SMMU_PMCG_IRQ_CFG0); > + > + /* MSI supported or not */ > + if (!(readl(pmu->reg_base + SMMU_PMCG_CFGR) & SMMU_PMCG_CFGR_MSI)) > + return; > + > + ret = platform_msi_domain_alloc_irqs(dev, 1, smmu_pmu_write_msi_msg); > + if (ret) { > + dev_warn(dev, "failed to allocate MSIs\n"); > + return; > + } > + > + desc = first_msi_entry(dev); > + if (desc) > + pmu->irq = desc->irq; > + > + /* Add callback to free MSIs on teardown */ > + devm_add_action(dev, smmu_pmu_free_msis, dev); > +} > + > +static int smmu_pmu_setup_irq(struct smmu_pmu *pmu) > +{ > + int irq, ret = -ENXIO; > + > + smmu_pmu_setup_msi(pmu); > + > + irq = pmu->irq; > + if (irq) > + ret = devm_request_irq(pmu->dev, irq, smmu_pmu_handle_irq, > + IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD, > + "smmu-v3-pmu", pmu); > + return ret; > +} > + > static int smmu_pmu_reset(struct smmu_pmu *smmu_pmu) > { > + int ret; > + > /* Disable counter and interrupt */ > writeq(smmu_pmu->counter_present_mask, > smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0); > writeq(smmu_pmu->counter_present_mask, > smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0); > > + ret = smmu_pmu_setup_irq(smmu_pmu); Why are we moving this out of probe? We may perform a reset more than once (e.g. if we get round to system PM support), at which point this looks logically wrong. Robin. > + if (ret) { > + dev_err(smmu_pmu->dev, "failed to setup irqs\n"); > + return ret; > + } > + > + /* Pick one CPU to be the preferred one to use */ > + smmu_pmu->on_cpu = smp_processor_id(); > + WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu))); > + > smmu_pmu_disable(&smmu_pmu->pmu); > return 0; > } > @@ -738,26 +817,8 @@ static int smmu_pmu_probe(struct platform_device *pdev) > } > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - dev_err(dev, "Failed to get valid irq for smmu @%pa\n", > - &mem_resource_0->start); > - return irq; > - } > - > - err = devm_request_irq(dev, irq, smmu_pmu_handle_irq, > - IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD, > - "smmu-pmu", smmu_pmu); > - if (err) { > - dev_err(dev, > - "Unable to request IRQ%d for SMMU PMU counters\n", irq); > - return err; > - } > - > - smmu_pmu->irq = irq; > - > - /* Pick one CPU to be the preferred one to use */ > - smmu_pmu->on_cpu = smp_processor_id(); > - WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu))); > + if (irq > 0) > + smmu_pmu->irq = irq; > > smmu_pmu->num_counters = get_num_counters(smmu_pmu); > smmu_pmu->counter_present_mask = GENMASK(smmu_pmu->num_counters - 1, 0); > @@ -765,7 +826,9 @@ static int smmu_pmu_probe(struct platform_device *pdev) > SMMU_PMCG_CFGR_SIZE_MASK) >> SMMU_PMCG_CFGR_SIZE_SHIFT; > smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0); > > - smmu_pmu_reset(smmu_pmu); > + err = smmu_pmu_reset(smmu_pmu); > + if (err) > + return err; > > err = cpuhp_state_add_instance_nocalls(cpuhp_state_num, > &smmu_pmu->node); >
Hi Robin, Thanks for going through this series, > -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: 07 September 2018 16:36 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > lorenzo.pieralisi@arm.com > Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo) > <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>; > pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org; > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>; > neil.m.leeder@gmail.com > Subject: Re: [PATCH v2 1/4] acpi: arm64: add iort support for PMCG > > On 24/07/18 12:45, Shameer Kolothum wrote: > > From: Neil Leeder <nleeder@codeaurora.org> > > > > 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> > > Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > drivers/acpi/arm64/iort.c | 95 > +++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 83 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 7a3a541..ac4d0d6 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -356,7 +356,8 @@ static 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_SMMU_V3) { > > + node->type == ACPI_IORT_NODE_SMMU_V3 || > > + node->type == ACPI_IORT_NODE_PMCG) { > > *id_out = map->output_base; > > return parent; > > } > > @@ -394,6 +395,8 @@ static int iort_get_id_mapping_index(struct > acpi_iort_node *node) > > } > > > > return smmu->id_mapping_index; > > + case ACPI_IORT_NODE_PMCG: > > + return 0; > > Why do we need a PMCG case here? AIUI this whole get_id_mapping_index > business is only relevant to SMMUv3 nodes where we have some need to > disambiguate the difference between the SMMU's own IDs and > StreamID-to-DeviceID mappings within the same table. PMCGs simply have > zero or one single ID mappings so should be equivalent to most named > components (other than their mappings pointing straight to the ITS). ITRC this is required for the iort_set_device_domain() function as otherwise, dev_set_msi_domain() won't be called for PMCGs with MSI support. > > default: > > return -EINVAL; > > } > > @@ -1287,6 +1290,63 @@ static bool __init arm_smmu_is_coherent(struct > acpi_iort_node *node) > > return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK; > > } > > > > +static void __init arm_smmu_common_dma_configure(struct device *dev, > > + enum dev_dma_attr attr) > > +{ > > + /* We expect the dma masks to be equivalent for all SMMUs > set-ups */ > > + dev->dma_mask = &dev->coherent_dma_mask; > > + > > + /* Configure DMA for the page table walker */ > > + acpi_dma_configure(dev, attr); > > Hmm, I don't think we actually need this call any more, since it should > now happen later anyway via platform_dma_configure() as the relevant > SMMU/PMCG driver binds. This is only applicable to SMMU nodes. As you have noted below, these devices are from the static table, so I am not sure platform_dma_configure() applies here. I will double check. > > +} > > + > > +static int __init arm_smmu_v3_pmu_count_resources(struct acpi_iort_node > *node) > > Can we be consistent with "pmcg" rather than "pmu" within IORT please? Ok. > > > +{ > > + 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_v3_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->page0_base_address; > > + res[0].end = pmcg->page0_base_address + SZ_4K - 1; > > + res[0].flags = IORESOURCE_MEM; > > + res[1].start = pmcg->page1_base_address; > > + res[1].end = pmcg->page1_base_address + 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]); > > +} > > + > > +static struct acpi_iort_node *iort_find_pmcg_ref(struct acpi_iort_node > *node) > > +{ > > + struct acpi_iort_pmcg *pmcg; > > + struct acpi_iort_node *ref_node = NULL; > > + > > + /* Retrieve PMCG specific data */ > > + pmcg = (struct acpi_iort_pmcg *)node->node_data; > > + if (pmcg->node_reference) > > + ref_node = ACPI_ADD_PTR(struct acpi_iort_node, > > + iort_table, pmcg->node_reference); > > + return ref_node; > > +} > > + > > struct iort_dev_config { > > const char *name; > > int (*dev_init)(struct acpi_iort_node *node); > > @@ -1296,6 +1356,8 @@ struct iort_dev_config { > > struct acpi_iort_node *node); > > void (*dev_set_proximity)(struct device *dev, > > struct acpi_iort_node *node); > > + void (*dev_dma_configure)(struct device *dev, > > + enum dev_dma_attr attr); > > }; > > > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > > @@ -1304,23 +1366,38 @@ static const struct iort_dev_config > iort_arm_smmu_v3_cfg __initconst = { > > .dev_count_resources = arm_smmu_v3_count_resources, > > .dev_init_resources = arm_smmu_v3_init_resources, > > .dev_set_proximity = arm_smmu_v3_set_proximity, > > + .dev_dma_configure = arm_smmu_common_dma_configure > > }; > > > > static const struct iort_dev_config iort_arm_smmu_cfg __initconst = { > > .name = "arm-smmu", > > .dev_is_coherent = arm_smmu_is_coherent, > > .dev_count_resources = arm_smmu_count_resources, > > - .dev_init_resources = arm_smmu_init_resources > > + .dev_init_resources = arm_smmu_init_resources, > > + .dev_dma_configure = arm_smmu_common_dma_configure > > +}; > > + > > +static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg > __initconst = { > > + .name = "arm-smmu-v3-pmu", > > + .dev_count_resources = arm_smmu_v3_pmu_count_resources, > > + .dev_init_resources = arm_smmu_v3_pmu_init_resources > > }; > > > > static __init const struct iort_dev_config *iort_get_dev_cfg( > > struct acpi_iort_node *node) > > { > > + struct acpi_iort_node *ref_node; > > + > > switch (node->type) { > > case ACPI_IORT_NODE_SMMU_V3: > > return &iort_arm_smmu_v3_cfg; > > case ACPI_IORT_NODE_SMMU: > > return &iort_arm_smmu_cfg; > > + case ACPI_IORT_NODE_PMCG: > > + /* Check this is associated with SMMUv3 */ > > + ref_node = iort_find_pmcg_ref(node); > > This seems overly restrictive - admittedly the only non-SMMU DTI > components I know of don't actually implement a PMCG for their internal > TBU, but I'm sure something will eventually, and there doesn't seem to > be any good reason for Linux to forcibly ignore it when it does. Right. I think that’s a misread of IORT spec from my part thinking there might be non SMMUv3 specific PMCG associated with NC/RC. I will remove this. Thanks, Shameer > > + if (ref_node && ref_node->type == > ACPI_IORT_NODE_SMMU_V3) > > + return &iort_arm_smmu_v3_pmcg_cfg; > > default: > > return NULL; > > } > > @@ -1376,12 +1453,6 @@ static int __init iort_add_platform_device(struct > acpi_iort_node *node, > > if (ret) > > goto dev_put; > > > > - /* > > - * We expect the dma masks to be equivalent for > > - * all SMMUs set-ups > > - */ > > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > - > > fwnode = iort_get_fwnode(node); > > > > if (!fwnode) { > > @@ -1391,11 +1462,11 @@ static int __init iort_add_platform_device(struct > acpi_iort_node *node, > > > > pdev->dev.fwnode = fwnode; > > > > - attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ? > > + if (ops->dev_dma_configure) { > > + attr = ops->dev_is_coherent && ops->dev_is_coherent(node) ? > > DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT; > > Oh, right, these guys don't have an ACPI companion or a standard DMA > attr because they're from a static table, so platform_dma_configure() > won't pick them up. Oh well. We probably don't want to start dragging > IORT internals into the platform bus code, so I guess it's not worth > trying to change that. > > Robin. > > > - > > - /* Configure DMA for the page table walker */ > > - acpi_dma_configure(&pdev->dev, attr); > > + ops->dev_dma_configure(&pdev->dev, attr); > > + } > > > > iort_set_device_domain(&pdev->dev, node); > > > >
> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: 10 September 2018 12:15 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > lorenzo.pieralisi@arm.com > Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo) > <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>; > pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org; > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>; > neil.m.leeder@gmail.com > Subject: Re: [PATCH v2 4/4] perf/smmuv3: Add MSI irq support > > On 2018-07-24 12:45 PM, Shameer Kolothum wrote: > > This adds support for MSI based counter overflow interrupt. > > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > drivers/perf/arm_smmuv3_pmu.c | 105 > +++++++++++++++++++++++++++++++++--------- > > 1 file changed, 84 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/perf/arm_smmuv3_pmu.c > b/drivers/perf/arm_smmuv3_pmu.c > > index b3dc394..ca69813 100644 > > --- a/drivers/perf/arm_smmuv3_pmu.c > > +++ b/drivers/perf/arm_smmuv3_pmu.c > > @@ -94,6 +94,10 @@ > > #define SMMU_PMCG_IRQ_CFG2 0xE64 > > #define SMMU_PMCG_IRQ_STATUS 0xE68 > > > > +/* MSI config fields */ > > +#define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2) > > +#define MSI_CFG2_MEMATTR_DEVICE_nGnRE 0x1 > > + > > #define SMMU_COUNTER_RELOAD BIT(31) > > #define SMMU_DEFAULT_FILTER_SEC 0 > > #define SMMU_DEFAULT_FILTER_SPAN 1 > > @@ -657,14 +661,89 @@ static irqreturn_t smmu_pmu_handle_irq(int > irq_num, void *data) > > return IRQ_HANDLED; > > } > > > > +static void smmu_pmu_free_msis(void *data) > > +{ > > + struct device *dev = data; > > + > > + platform_msi_domain_free_irqs(dev); > > +} > > + > > +static void smmu_pmu_write_msi_msg(struct msi_desc *desc, struct > msi_msg *msg) > > +{ > > + phys_addr_t doorbell; > > + struct device *dev = msi_desc_to_dev(desc); > > + struct smmu_pmu *pmu = dev_get_drvdata(dev); > > + > > + doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo; > > + doorbell &= MSI_CFG0_ADDR_MASK; > > + > > + writeq_relaxed(doorbell, pmu->reg_base + SMMU_PMCG_IRQ_CFG0); > > + writel_relaxed(msg->data, pmu->reg_base + > SMMU_PMCG_IRQ_CFG1); > > + writel_relaxed(MSI_CFG2_MEMATTR_DEVICE_nGnRE, > > + pmu->reg_base + SMMU_PMCG_IRQ_CFG2); > > +} > > + > > +static void smmu_pmu_setup_msi(struct smmu_pmu *pmu) > > +{ > > + struct msi_desc *desc; > > + struct device *dev = pmu->dev; > > + int ret; > > + > > + /* Clear MSI address reg */ > > + writeq_relaxed(0, pmu->reg_base + SMMU_PMCG_IRQ_CFG0); > > + > > + /* MSI supported or not */ > > + if (!(readl(pmu->reg_base + SMMU_PMCG_CFGR) & > SMMU_PMCG_CFGR_MSI)) > > + return; > > + > > + ret = platform_msi_domain_alloc_irqs(dev, 1, > smmu_pmu_write_msi_msg); > > + if (ret) { > > + dev_warn(dev, "failed to allocate MSIs\n"); > > + return; > > + } > > + > > + desc = first_msi_entry(dev); > > + if (desc) > > + pmu->irq = desc->irq; > > + > > + /* Add callback to free MSIs on teardown */ > > + devm_add_action(dev, smmu_pmu_free_msis, dev); > > +} > > + > > +static int smmu_pmu_setup_irq(struct smmu_pmu *pmu) > > +{ > > + int irq, ret = -ENXIO; > > + > > + smmu_pmu_setup_msi(pmu); > > + > > + irq = pmu->irq; > > + if (irq) > > + ret = devm_request_irq(pmu->dev, irq, > smmu_pmu_handle_irq, > > + IRQF_NOBALANCING | IRQF_SHARED | > IRQF_NO_THREAD, > > + "smmu-v3-pmu", pmu); > > + return ret; > > +} > > + > > static int smmu_pmu_reset(struct smmu_pmu *smmu_pmu) > > { > > + int ret; > > + > > /* Disable counter and interrupt */ > > writeq(smmu_pmu->counter_present_mask, > > smmu_pmu->reg_base + SMMU_PMCG_CNTENCLR0); > > writeq(smmu_pmu->counter_present_mask, > > smmu_pmu->reg_base + SMMU_PMCG_INTENCLR0); > > > > + ret = smmu_pmu_setup_irq(smmu_pmu); > > Why are we moving this out of probe? We may perform a reset more than > once (e.g. if we get round to system PM support), at which point this > looks logically wrong. I didn’t consider that scenario. I will modify this in next. Thanks, Shameer > Robin. > > > + if (ret) { > > + dev_err(smmu_pmu->dev, "failed to setup irqs\n"); > > + return ret; > > + } > > + > > + /* Pick one CPU to be the preferred one to use */ > > + smmu_pmu->on_cpu = smp_processor_id(); > > + WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu- > >on_cpu))); > > + > > smmu_pmu_disable(&smmu_pmu->pmu); > > return 0; > > } > > @@ -738,26 +817,8 @@ static int smmu_pmu_probe(struct platform_device > *pdev) > > } > > > > irq = platform_get_irq(pdev, 0); > > - if (irq < 0) { > > - dev_err(dev, "Failed to get valid irq for smmu @%pa\n", > > - &mem_resource_0->start); > > - return irq; > > - } > > - > > - err = devm_request_irq(dev, irq, smmu_pmu_handle_irq, > > - IRQF_NOBALANCING | IRQF_SHARED | > IRQF_NO_THREAD, > > - "smmu-pmu", smmu_pmu); > > - if (err) { > > - dev_err(dev, > > - "Unable to request IRQ%d for SMMU PMU > counters\n", irq); > > - return err; > > - } > > - > > - smmu_pmu->irq = irq; > > - > > - /* Pick one CPU to be the preferred one to use */ > > - smmu_pmu->on_cpu = smp_processor_id(); > > - WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu- > >on_cpu))); > > + if (irq > 0) > > + smmu_pmu->irq = irq; > > > > smmu_pmu->num_counters = get_num_counters(smmu_pmu); > > smmu_pmu->counter_present_mask = GENMASK(smmu_pmu- > >num_counters - 1, 0); > > @@ -765,7 +826,9 @@ static int smmu_pmu_probe(struct platform_device > *pdev) > > SMMU_PMCG_CFGR_SIZE_MASK) >> > SMMU_PMCG_CFGR_SIZE_SHIFT; > > smmu_pmu->counter_mask = GENMASK_ULL(reg_size, 0); > > > > - smmu_pmu_reset(smmu_pmu); > > + err = smmu_pmu_reset(smmu_pmu); > > + if (err) > > + return err; > > > > err = cpuhp_state_add_instance_nocalls(cpuhp_state_num, > > &smmu_pmu->node); > >