Message ID | 20211117144844.241072-4-jean-philippe@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | perf/smmuv3: Support devicetree | expand |
On 17/11/2021 14:48, Jean-Philippe Brucker wrote: > From: Robin Murphy<robin.murphy@arm.com> > > The SMMU_PMCG_IIDR register was not present in older revisions of the > Arm SMMUv3 spec. On Arm Ltd. implementations, the IIDR value consists of > fields from several PIDR registers, allowing us to present a > standardized identifier to userspace. > So is there some userspace part to go with this now? Thanks, John
On 2021-12-07 09:14, John Garry wrote: > On 17/11/2021 14:48, Jean-Philippe Brucker wrote: >> From: Robin Murphy<robin.murphy@arm.com> >> >> The SMMU_PMCG_IIDR register was not present in older revisions of the >> Arm SMMUv3 spec. On Arm Ltd. implementations, the IIDR value consists of >> fields from several PIDR registers, allowing us to present a >> standardized identifier to userspace. >> > So is there some userspace part to go with this now? FWIW I've not looked into it - is it just a case of someone knocking out some JSON from the MMU-600/700 TRMs, or is there still mroe to do? I had the impression that *some* part of the process was stalled until implementations can start providing meaningful IIDRs, but I wasn't sure whether that was tooling or just data. I just work the low-level enablement angle :) Robin.
On 07/12/2021 12:04, Robin Murphy wrote: >>> >> So is there some userspace part to go with this now? > > FWIW I've not looked into it - is it just a case of someone knocking out > some JSON from the MMU-600/700 TRMs, or is there still mroe to do? That should just be it. > I had > the impression that *some* part of the process was stalled until > implementations can start providing meaningful IIDRs, but I wasn't sure > whether that was tooling or just data. I just work the low-level > enablement angle :) Tooling should be ok, but I would just like to see more of these JSONs so any tooling issues can be ironed out. Cheers, John
On 2021-12-07 12:28, John Garry via iommu wrote: > On 07/12/2021 12:04, Robin Murphy wrote: >>>> >>> So is there some userspace part to go with this now? >> >> FWIW I've not looked into it - is it just a case of someone knocking >> out some JSON from the MMU-600/700 TRMs, or is there still mroe to do? > > That should just be it. > >> I had the impression that *some* part of the process was stalled until >> implementations can start providing meaningful IIDRs, but I wasn't >> sure whether that was tooling or just data. I just work the low-level >> enablement angle :) > > Tooling should be ok, but I would just like to see more of these JSONs > so any tooling issues can be ironed out. Sounds good - Jean, Leo, is that something Linaro might like to pick up as part of the PMCG interest, or shall I make a note on my to-do list for the new year? Thanks, Robin. > > Cheers, > John > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Tue, Dec 07, 2021 at 12:48:13PM +0000, Robin Murphy wrote: > On 2021-12-07 12:28, John Garry via iommu wrote: > > On 07/12/2021 12:04, Robin Murphy wrote: > > > > > > > > > So is there some userspace part to go with this now? > > > > > > FWIW I've not looked into it - is it just a case of someone knocking > > > out some JSON from the MMU-600/700 TRMs, or is there still mroe to > > > do? > > > > That should just be it. Hope I didn't arrive too late :) Yes, I think we just missed two things: the DT binding for SMMUv3 PMU which is just addressed by this patchset; and the PMU event aliasing for SMMUv3 PMU, before I inquired with John and John said he would upstream the related patches after kernel can export a IIDR value via sysfs node. Seems to me, after this patchset for DT binding and PMU event alias patches are landed to the mainline kernel, it would be perfect. > > > I had the impression that *some* part of the process was stalled > > > until implementations can start providing meaningful IIDRs, but I > > > wasn't sure whether that was tooling or just data. I just work the > > > low-level enablement angle :) > > > > Tooling should be ok, but I would just like to see more of these JSONs > > so any tooling issues can be ironed out. > > Sounds good - Jean, Leo, is that something Linaro might like to pick up as > part of the PMCG interest, or shall I make a note on my to-do list for the > new year? I took a look for current patch for using PIDR to synthesize IIDR, it looks good to me. But I tested it on Hisilicon D06 board and observed the composed IIDR values are still zeros. I added a printk sentence to dump iidr value at the end of the function smmu_pmu_get_iidr(): leoy@ubuntu:~$ dmesg | grep iidr [ 28.674087] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.8.auto: iidr=0x0 [ 28.705239] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.9.auto: iidr=0x0 [ 28.729924] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.10.auto: iidr=0x0 [ 28.754855] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.11.auto: iidr=0x0 [ 28.779811] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.12.auto: iidr=0x0 [ 28.804755] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.13.auto: iidr=0x0 [ 28.829825] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.14.auto: iidr=0x0 [ 28.854767] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.15.auto: iidr=0x0 Please confirm if this is expected or not? I think this might introduce difficulty for John for the PMU event alias patches, which is dependent on a non-zero IIDR. At last, very appreciate your (Jean-Philippe, Robin and John) help! Thanks, Leo
On 2021-12-07 13:20, Leo Yan wrote: > On Tue, Dec 07, 2021 at 12:48:13PM +0000, Robin Murphy wrote: >> On 2021-12-07 12:28, John Garry via iommu wrote: >>> On 07/12/2021 12:04, Robin Murphy wrote: >>>>>> >>>>> So is there some userspace part to go with this now? >>>> >>>> FWIW I've not looked into it - is it just a case of someone knocking >>>> out some JSON from the MMU-600/700 TRMs, or is there still mroe to >>>> do? >>> >>> That should just be it. > > Hope I didn't arrive too late :) > > Yes, I think we just missed two things: the DT binding for SMMUv3 PMU > which is just addressed by this patchset; and the PMU event aliasing > for SMMUv3 PMU, before I inquired with John and John said he would > upstream the related patches after kernel can export a IIDR value via > sysfs node. > > Seems to me, after this patchset for DT binding and PMU event alias > patches are landed to the mainline kernel, it would be perfect. > >>>> I had the impression that *some* part of the process was stalled >>>> until implementations can start providing meaningful IIDRs, but I >>>> wasn't sure whether that was tooling or just data. I just work the >>>> low-level enablement angle :) >>> >>> Tooling should be ok, but I would just like to see more of these JSONs >>> so any tooling issues can be ironed out. >> >> Sounds good - Jean, Leo, is that something Linaro might like to pick up as >> part of the PMCG interest, or shall I make a note on my to-do list for the >> new year? > > I took a look for current patch for using PIDR to synthesize IIDR, it > looks good to me. But I tested it on Hisilicon D06 board and observed > the composed IIDR values are still zeros. > > I added a printk sentence to dump iidr value at the end of the function > smmu_pmu_get_iidr(): > > leoy@ubuntu:~$ dmesg | grep iidr > [ 28.674087] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.8.auto: iidr=0x0 > [ 28.705239] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.9.auto: iidr=0x0 > [ 28.729924] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.10.auto: iidr=0x0 > [ 28.754855] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.11.auto: iidr=0x0 > [ 28.779811] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.12.auto: iidr=0x0 > [ 28.804755] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.13.auto: iidr=0x0 > [ 28.829825] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.14.auto: iidr=0x0 > [ 28.854767] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.15.auto: iidr=0x0 > > Please confirm if this is expected or not? I think this might > introduce difficulty for John for the PMU event alias patches, which > is dependent on a non-zero IIDR. Yes, from previous discussions I believe the HiSilicon implementations don't have much meaningful ID information at all (hence why we have to match ACPI table headers to identify the counter erratum). My trick only works for Arm Ltd. implementations since they happen to have the IMP-DEF CoreSight registers with the same information as would be in the future IIDR. To clarify, the proposal at this point is to write up JSON files for MMU-600/MMU-700, based on this patch, in order to pipe-clean the process for future SMMUv3.3 PMCG implementations with real IIDRs. Whether other implementers might retroactively define "equivalent" IIDR values for their existing implementations in a way we could potentially quirk in the driver is an orthogonal question. Cheers, Robin. > > At last, very appreciate your (Jean-Philippe, Robin and John) help! > > Thanks, > Leo >
On Tue, Dec 07, 2021 at 01:46:49PM +0000, Robin Murphy wrote: [...] > > [ 28.854767] arm-smmu-v3-pmcg arm-smmu-v3-pmcg.15.auto: iidr=0x0 > > > > Please confirm if this is expected or not? I think this might > > introduce difficulty for John for the PMU event alias patches, which > > is dependent on a non-zero IIDR. > > Yes, from previous discussions I believe the HiSilicon implementations don't > have much meaningful ID information at all (hence why we have to match ACPI > table headers to identify the counter erratum). My trick only works for Arm > Ltd. implementations since they happen to have the IMP-DEF CoreSight > registers with the same information as would be in the future IIDR. > > To clarify, the proposal at this point is to write up JSON files for > MMU-600/MMU-700, based on this patch, in order to pipe-clean the process for > future SMMUv3.3 PMCG implementations with real IIDRs. > > Whether other implementers might retroactively define "equivalent" IIDR > values for their existing implementations in a way we could potentially > quirk in the driver is an orthogonal question. Agreed, it makes sense that supports the standard IP modules in the mainline kernel at this stage. Thanks for explanation. Leo
On 07/12/2021 13:59, Leo Yan wrote: >> Whether other implementers might retroactively define "equivalent" IIDR >> values for their existing implementations in a way we could potentially >> quirk in the driver is an orthogonal question. > Agreed, it makes sense that supports the standard IP modules in > the mainline kernel at this stage. > > Thanks for explanation. Leo, if you really want this to work on D06, I could also hack some out-of-tree perf tool patches for you. I'm not sure if you're interested in that. Let me know. Thanks, John
On Tue, Dec 07, 2021 at 02:00:35PM +0000, John Garry wrote: > On 07/12/2021 13:59, Leo Yan wrote: > > > Whether other implementers might retroactively define "equivalent" IIDR > > > values for their existing implementations in a way we could potentially > > > quirk in the driver is an orthogonal question. > > Agreed, it makes sense that supports the standard IP modules in > > the mainline kernel at this stage. > > > > Thanks for explanation. > > Leo, if you really want this to work on D06, I could also hack some > out-of-tree perf tool patches for you. I'm not sure if you're interested in > that. Let me know. No, please don't spend time on this. I just use D06 platform to verify SMMUv3 relevant patches, but have no requirement for profiling SMMUv3 on it. Anyway, thanks a lot! Leo
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index 19697617153a..598d6978280d 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -76,6 +76,10 @@ #define SMMU_PMCG_CR 0xE04 #define SMMU_PMCG_CR_ENABLE BIT(0) #define SMMU_PMCG_IIDR 0xE08 +#define SMMU_PMCG_IIDR_PRODUCTID GENMASK(31, 20) +#define SMMU_PMCG_IIDR_VARIANT GENMASK(19, 16) +#define SMMU_PMCG_IIDR_REVISION GENMASK(15, 12) +#define SMMU_PMCG_IIDR_IMPLEMENTER GENMASK(11, 0) #define SMMU_PMCG_CEID0 0xE20 #define SMMU_PMCG_CEID1 0xE28 #define SMMU_PMCG_IRQ_CTRL 0xE50 @@ -84,6 +88,20 @@ #define SMMU_PMCG_IRQ_CFG1 0xE60 #define SMMU_PMCG_IRQ_CFG2 0xE64 +/* IMP-DEF ID registers */ +#define SMMU_PMCG_PIDR0 0xFE0 +#define SMMU_PMCG_PIDR0_PART_0 GENMASK(7, 0) +#define SMMU_PMCG_PIDR1 0xFE4 +#define SMMU_PMCG_PIDR1_DES_0 GENMASK(7, 4) +#define SMMU_PMCG_PIDR1_PART_1 GENMASK(3, 0) +#define SMMU_PMCG_PIDR2 0xFE8 +#define SMMU_PMCG_PIDR2_REVISION GENMASK(7, 4) +#define SMMU_PMCG_PIDR2_DES_1 GENMASK(2, 0) +#define SMMU_PMCG_PIDR3 0xFEC +#define SMMU_PMCG_PIDR3_REVAND GENMASK(7, 4) +#define SMMU_PMCG_PIDR4 0xFD0 +#define SMMU_PMCG_PIDR4_DES_2 GENMASK(3, 0) + /* MSI config fields */ #define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2) #define MSI_CFG2_MEMATTR_DEVICE_nGnRE 0x1 @@ -755,6 +773,41 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); } +static bool smmu_pmu_coresight_id_regs(struct smmu_pmu *smmu_pmu) +{ + return of_device_is_compatible(smmu_pmu->dev->of_node, + "arm,mmu-600-pmcg"); +} + +static void smmu_pmu_get_iidr(struct smmu_pmu *smmu_pmu) +{ + u32 iidr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_IIDR); + + if (!iidr && smmu_pmu_coresight_id_regs(smmu_pmu)) { + u32 pidr0 = readl(smmu_pmu->reg_base + SMMU_PMCG_PIDR0); + u32 pidr1 = readl(smmu_pmu->reg_base + SMMU_PMCG_PIDR1); + u32 pidr2 = readl(smmu_pmu->reg_base + SMMU_PMCG_PIDR2); + u32 pidr3 = readl(smmu_pmu->reg_base + SMMU_PMCG_PIDR3); + u32 pidr4 = readl(smmu_pmu->reg_base + SMMU_PMCG_PIDR4); + + u32 productid = FIELD_GET(SMMU_PMCG_PIDR0_PART_0, pidr0) | + (FIELD_GET(SMMU_PMCG_PIDR1_PART_1, pidr1) << 8); + u32 variant = FIELD_GET(SMMU_PMCG_PIDR2_REVISION, pidr2); + u32 revision = FIELD_GET(SMMU_PMCG_PIDR3_REVAND, pidr3); + u32 implementer = + FIELD_GET(SMMU_PMCG_PIDR1_DES_0, pidr1) | + (FIELD_GET(SMMU_PMCG_PIDR2_DES_1, pidr2) << 4) | + (FIELD_GET(SMMU_PMCG_PIDR4_DES_2, pidr4) << 8); + + iidr = FIELD_PREP(SMMU_PMCG_IIDR_PRODUCTID, productid) | + FIELD_PREP(SMMU_PMCG_IIDR_VARIANT, variant) | + FIELD_PREP(SMMU_PMCG_IIDR_REVISION, revision) | + FIELD_PREP(SMMU_PMCG_IIDR_IMPLEMENTER, implementer); + } + + smmu_pmu->iidr = iidr; +} + static int smmu_pmu_probe(struct platform_device *pdev) { struct smmu_pmu *smmu_pmu; @@ -826,7 +879,7 @@ static int smmu_pmu_probe(struct platform_device *pdev) return err; } - smmu_pmu->iidr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_IIDR); + smmu_pmu_get_iidr(smmu_pmu); name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "smmuv3_pmcg_%llx", (res_0->start) >> SMMU_PMCG_PA_SHIFT);