> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@xxxxxxx] > Sent: 25 January 2019 18:33 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; > lorenzo.pieralisi@xxxxxxx > Cc: jean-philippe.brucker@xxxxxxx; will.deacon@xxxxxxx; > mark.rutland@xxxxxxx; Guohanjun (Hanjun Guo) <guohanjun@xxxxxxxxxx>; > John Garry <john.garry@xxxxxxxxxx>; pabba@xxxxxxxxxxxxxx; > vkilari@xxxxxxxxxxxxxx; rruigrok@xxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linuxarm > <linuxarm@xxxxxxxxxx>; neil.m.leeder@xxxxxxxxx > Subject: Re: [PATCH v5 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum > 162001800 quirk > > On 30/11/2018 15:47, Shameer Kolothum wrote: > > HiSilicon erratum 162001800 describes the limitation of > > SMMUv3 PMCG implementation on HiSilicon Hip08 platforms. > > > > On these platforms, the PMCG event counter registers > > (SMMU_PMCG_EVCNTRn) are read only and as a result it > > is not possible to set the initial counter period value > > on event monitor start. > > > > To work around this, the current value of the counter > > is read and used for delta calculations. OEM information > > from ACPI header is used to identify the affected hardware > > platforms. > > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> > > --- > > drivers/acpi/arm64/iort.c | 30 +++++++++++++++++++++++++++--- > > drivers/perf/arm_smmuv3_pmu.c | 35 > +++++++++++++++++++++++++++++------ > > include/linux/acpi_iort.h | 3 +++ > > 3 files changed, 59 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 2da08e1..d174379 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -1364,6 +1364,22 @@ static void __init > arm_smmu_v3_pmcg_init_resources(struct resource *res, > > ACPI_EDGE_SENSITIVE, &res[2]); > > } > > > > +static struct acpi_platform_list pmcg_evcntr_rdonly_list[] __initdata = { > > + /* HiSilicon Erratum 162001800 */ > > + {"HISI ", "HIP08 ", 0, ACPI_SIG_IORT, greater_than_or_equal}, > > + { } > > +}; > > + > > +static int __init arm_smmu_v3_pmcg_add_platdata(struct platform_device > *pdev) > > +{ > > + u32 options = 0; > > + > > + if (acpi_match_platform_list(pmcg_evcntr_rdonly_list) >= 0) > > + options |= IORT_PMCG_EVCNTR_RDONLY; > > Hmm, do we want IORT code to have to understand a (potential) whole load > of PMCG-specific quirks directly, or do we really only need to pass some > unambiguous identifier for the PMCG implementation, and let the driver > handle the details in private - much like the SMMU model field, only > without an external spec to constrain us :) Could do that, but was not sure about coming up with an identifier which is not really part of the spec and placing that in IORT code. Personally I prefer having all this private to driver rather than in IORT code. But I see your point that this will be more like smmu if we can pass identifiers here. > If we ever want to have named imp-def events, we'd need to do something > like that anyway, so perhaps we might be better off taking that approach > to begin with (and if so, I'd be inclined to push the basic platdata > initialisation for "generic PMCG" into patch #1). Ok. I will give that a try in next respin. > > + > > + return platform_device_add_data(pdev, &options, sizeof(options)); > > +} > > + > > struct iort_dev_config { > > const char *name; > > int (*dev_init)(struct acpi_iort_node *node); > > @@ -1374,6 +1390,7 @@ struct iort_dev_config { > > struct acpi_iort_node *node); > > void (*dev_set_proximity)(struct device *dev, > > struct acpi_iort_node *node); > > + int (*dev_add_platdata)(struct platform_device *pdev); > > }; > > > > static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = { > > @@ -1395,6 +1412,7 @@ static const struct iort_dev_config > iort_arm_smmu_v3_pmcg_cfg __initconst = { > > .name = "arm-smmu-v3-pmu", > > .dev_count_resources = arm_smmu_v3_pmcg_count_resources, > > .dev_init_resources = arm_smmu_v3_pmcg_init_resources, > > + .dev_add_platdata = arm_smmu_v3_pmcg_add_platdata, > > }; > > > > static __init const struct iort_dev_config *iort_get_dev_cfg( > > @@ -1455,10 +1473,16 @@ static int __init > iort_add_platform_device(struct acpi_iort_node *node, > > goto dev_put; > > > > /* > > - * Add a copy of IORT node pointer to platform_data to > > - * be used to retrieve IORT data information. > > + * Platform devices based on PMCG nodes uses platform_data to > > + * pass quirk flags to the driver. For others, add a copy of > > + * IORT node pointer to platform_data to be used to retrieve > > + * IORT data information. > > */ > > - ret = platform_device_add_data(pdev, &node, sizeof(node)); > > + if (ops->dev_add_platdata) > > + ret = ops->dev_add_platdata(pdev); > > + else > > + ret = platform_device_add_data(pdev, &node, sizeof(node)); > > + > > if (ret) > > goto dev_put; > > > > diff --git a/drivers/perf/arm_smmuv3_pmu.c > b/drivers/perf/arm_smmuv3_pmu.c > > index 71d10a0..02107a1 100644 > > --- a/drivers/perf/arm_smmuv3_pmu.c > > +++ b/drivers/perf/arm_smmuv3_pmu.c > > @@ -35,6 +35,7 @@ > > */ > > > > #include <linux/acpi.h> > > +#include <linux/acpi_iort.h> > > #include <linux/bitfield.h> > > #include <linux/bitops.h> > > #include <linux/cpuhotplug.h> > > @@ -111,6 +112,7 @@ struct smmu_pmu { > > struct device *dev; > > void __iomem *reg_base; > > void __iomem *reloc_base; > > + u32 options; > > u64 counter_present_mask; > > u64 counter_mask; > > }; > > @@ -224,12 +226,25 @@ static void smmu_pmu_set_period(struct > smmu_pmu *smmu_pmu, > > u32 idx = hwc->idx; > > u64 new; > > > > - /* > > - * We limit the max period to half the max counter value of the counter > > - * size, so that even in the case of extreme interrupt latency the > > - * counter will (hopefully) not wrap past its initial value. > > - */ > > - new = smmu_pmu->counter_mask >> 1; > > + if (smmu_pmu->options & IORT_PMCG_EVCNTR_RDONLY) { > > + /* > > + * On platforms that require this quirk, if the counter starts > > + * at < half_counter value and wraps, the current logic of > > + * handling the overflow may not work. It is expected that, > > + * those platforms will have full 64 counter bits implemented > > + * so that such a possibility is remote(eg: HiSilicon HIP08). > > + */ > > + new = smmu_pmu_counter_get_value(smmu_pmu, idx); > > + } else { > > + /* > > + * We limit the max period to half the max counter value > > + * of the counter size, so that even in the case of extreme > > + * interrupt latency the counter will (hopefully) not wrap > > + * past its initial value. > > + */ > > + new = smmu_pmu->counter_mask >> 1; > > + smmu_pmu_counter_set_value(smmu_pmu, idx, new); > > + } > > > > local64_set(&hwc->prev_count, new); > > smmu_pmu_counter_set_value(smmu_pmu, idx, new); > > Either we've just done this already, or it's not going to have any > effect anyway, so it can definitely go. My bad. Forgot to remove that. v4 didn’t have this here. Thanks, Shameer > Robin. > > > @@ -670,6 +685,12 @@ static void smmu_pmu_reset(struct smmu_pmu > *smmu_pmu) > > smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0); > > } > > > > +static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > > +{ > > + smmu_pmu->options = *(u32 *)dev_get_platdata(smmu_pmu->dev); > > + dev_notice(smmu_pmu->dev, "option mask 0x%x\n", > smmu_pmu->options); > > +} > > + > > static int smmu_pmu_probe(struct platform_device *pdev) > > { > > struct smmu_pmu *smmu_pmu; > > @@ -749,6 +770,8 @@ static int smmu_pmu_probe(struct platform_device > *pdev) > > return -EINVAL; > > } > > > > + smmu_pmu_get_acpi_options(smmu_pmu); > > + > > /* Pick one CPU to be the preferred one to use */ > > smmu_pmu->on_cpu = get_cpu(); > > WARN_ON(irq_set_affinity(smmu_pmu->irq, > cpumask_of(smmu_pmu->on_cpu))); > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > > index 38cd77b..4a7ae69 100644 > > --- a/include/linux/acpi_iort.h > > +++ b/include/linux/acpi_iort.h > > @@ -26,6 +26,9 @@ > > #define IORT_IRQ_MASK(irq) (irq & 0xffffffffULL) > > #define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xffffffffULL) > > > > +/* PMCG node option or quirk flags */ > > +#define IORT_PMCG_EVCNTR_RDONLY (1 << 0) > > + > > int iort_register_domain_token(int trans_id, phys_addr_t base, > > struct fwnode_handle *fw_node); > > void iort_deregister_domain_token(int trans_id); > >
