From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4BE76C433F5 for ; Sun, 14 Nov 2021 13:47:11 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E68D360F45 for ; Sun, 14 Nov 2021 13:47:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E68D360F45 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=rosenzweig.io Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=/pqtJ3E0JveoO0WN9qbqjAbWL9YfcRloIwa4FQ2B6LM=; b=xDaHYGe1d0ssCq v9eQ7hcPn+CYll5IFqGpzy9i4UPjM0YMJmFJIc2b6h1VPXUclUE4NdOdqwSxsOPj0C0V3h8t8gyN3 rCX1vQo89ZI5rWHG36G+8iJ77ev9VsNMxKXhitu8k23rJd8gFOJshq+1r3doxo7I7v6Zzelrqkh7q xEI4GbQlQhiPgVTir4xTz3TOqNgaDT3ku5xJkDd6D3p9rRYVfbB1l0dfegYKggQj4RCK6K3inUPcO Bd0WOrjV4IeMWAdFQ7+mTZto7IUjqRAo3JwI8A1isYkDcoP9HVNvLI5M/VuPaALbAjayUbg4sucy2 YSc9GfnWm5dMuhTOUkiA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mmFpa-00Di2y-D1; Sun, 14 Nov 2021 13:46:02 +0000 Received: from rosenzweig.io ([138.197.143.207]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mmFpM-00Di0I-Sg for linux-arm-kernel@lists.infradead.org; Sun, 14 Nov 2021 13:45:50 +0000 Date: Sun, 14 Nov 2021 08:45:30 -0500 From: Alyssa Rosenzweig To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Mark Rutland , Will Deacon , Hector Martin , Sven Peter , Rob Herring , Thomas Gleixner Subject: Re: [PATCH 8/8] drivers/perf: Add Apple icestorm/firestorm CPU PMU driver Message-ID: References: <20211113115429.4027571-1-maz@kernel.org> <20211113115429.4027571-9-maz@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20211113115429.4027571-9-maz@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211114_054549_045659_0AC836CC X-CRM114-Status: GOOD ( 22.37 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org > +/* Counters */ > +#define SYS_IMP_APL_PMC0_EL1 sys_reg(3, 2, 15, 0, 0) > +#define SYS_IMP_APL_PMC1_EL1 sys_reg(3, 2, 15, 1, 0) > +#define SYS_IMP_APL_PMC2_EL1 sys_reg(3, 2, 15, 2, 0) > +#define SYS_IMP_APL_PMC3_EL1 sys_reg(3, 2, 15, 3, 0) > +#define SYS_IMP_APL_PMC4_EL1 sys_reg(3, 2, 15, 4, 0) > +#define SYS_IMP_APL_PMC5_EL1 sys_reg(3, 2, 15, 5, 0) > +#define SYS_IMP_APL_PMC6_EL1 sys_reg(3, 2, 15, 6, 0) > +#define SYS_IMP_APL_PMC7_EL1 sys_reg(3, 2, 15, 7, 0) --gap-- > +#define SYS_IMP_APL_PMC8_EL1 sys_reg(3, 2, 15, 9, 0) > +#define SYS_IMP_APL_PMC9_EL1 sys_reg(3, 2, 15, 10, 0) Do we know what the gap is? > +/* > + * Description of the events we actually know about, as well as those with > + * a specific counter affinity. Yes, this is a grand total of two known > + * counters, and the rest is anybody's guess. > + * > + * Not all counters can count all events. Counters #0 and #1 are wired to > + * count cycles and instructions respectively, and some events have > + * bizarre mappings (every other counter, or even *one* counter). These > + * restrictins equally apply to both P and E cores. restrictions > +/* Low level accessors. No synchronisation. */ > +#define PMU_READ_COUNTER(_idx) \ > + case _idx: return read_sysreg_s(SYS_IMP_APL_PMC## _idx ##_EL1) > + > +#define PMU_WRITE_COUNTER(_val, _idx) \ > + case _idx: \ > + write_sysreg_s(_val, SYS_IMP_APL_PMC## _idx ##_EL1); \ > + return > + > +static u64 m1_pmu_read_hw_counter(unsigned int index) > +{ > + switch (index) { > + PMU_READ_COUNTER(0); > + PMU_READ_COUNTER(1); > + PMU_READ_COUNTER(2); > + PMU_READ_COUNTER(3); > + PMU_READ_COUNTER(4); > + PMU_READ_COUNTER(5); > + PMU_READ_COUNTER(6); > + PMU_READ_COUNTER(7); > + PMU_READ_COUNTER(8); > + PMU_READ_COUNTER(9); > + } > + > + BUG(); > +} > + > +static void m1_pmu_write_hw_counter(u64 val, unsigned int index) > +{ > + switch (index) { > + PMU_WRITE_COUNTER(val, 0); > + PMU_WRITE_COUNTER(val, 1); > + PMU_WRITE_COUNTER(val, 2); > + PMU_WRITE_COUNTER(val, 3); > + PMU_WRITE_COUNTER(val, 4); > + PMU_WRITE_COUNTER(val, 5); > + PMU_WRITE_COUNTER(val, 6); > + PMU_WRITE_COUNTER(val, 7); > + PMU_WRITE_COUNTER(val, 8); > + PMU_WRITE_COUNTER(val, 9); > + } > + > + BUG(); > +} Probbaly cleaner to use a single switch and no macros, registers become greppable and the code is shorter too. Caveat: didn't check if it compiles. static inline u64 m1_pmu_hw_counter(unsigned int index) { switch (index) { case 0: return SYS_IMP_APL_PMC0_EL1; case 1: return SYS_IMP_APL_PMC1_EL1; case 2: return SYS_IMP_APL_PMC2_EL1; case 3: return SYS_IMP_APL_PMC3_EL1; case 4: return SYS_IMP_APL_PMC4_EL1; case 5: return SYS_IMP_APL_PMC5_EL1; case 6: return SYS_IMP_APL_PMC6_EL1; case 7: return SYS_IMP_APL_PMC7_EL1; case 8: return SYS_IMP_APL_PMC8_EL1; case 9: return SYS_IMP_APL_PMC9_EL1; } BUG(); } static u64 m1_pmu_read_hw_counter(unsigned int index) { return read_sysreg_s(m1_pmu_hw_counter(index)); } static void m1_pmu_write_hw_counter(u64 val, unsigned int index) { write_sysreg_s(val, m1_pmu_hw_counter(index)); } > +static void __m1_pmu_enable_counter(unsigned int index, bool en) > +{ > + u64 val, bit; > + > + switch (index) { > + case 0 ... 7: > + bit = BIT(get_bit_offset(index, PMCR0_CNT_ENABLE_0_7)); > + break; > + case 8 ... 9: > + bit = BIT(get_bit_offset(index - 8, PMCR0_CNT_ENABLE_8_9)); > + break; > + default: > + BUG(); > + } > + > + val = read_sysreg_s(SYS_IMP_APL_PMCR0_EL1); > + > + if (en) > + val |= bit; > + else > + val &= ~bit; > + > + write_sysreg_s(val, SYS_IMP_APL_PMCR0_EL1); > +} ... > +static void __m1_pmu_enable_counter_interrupt(unsigned int index, bool en) > +{ > + u64 val, bit; > + > + switch (index) { > + case 0 ... 7: > + bit = BIT(get_bit_offset(index, PMCR0_PMI_ENABLE_0_7)); > + break; > + case 8 ... 9: > + bit = BIT(get_bit_offset(index - 8, PMCR0_PMI_ENABLE_8_9)); > + break; > + default: > + BUG(); > + } > + > + val = read_sysreg_s(SYS_IMP_APL_PMCR0_EL1); > + > + if (en) > + val |= bit; > + else > + val &= ~bit; > + > + write_sysreg_s(val, SYS_IMP_APL_PMCR0_EL1); > +} These two helper functions have basically the same logic -- maybe worth combining? > +static void m1_pmu_configure_counter(unsigned int index, u8 event, > + bool user, bool kernel) > +{ .... > + switch (index) { > + case 0 ... 1: > + /* 0 and 1 have fixed events */ > + break; > + case 2 ... 5: > + shift = (index - 2) * 8; > + val = read_sysreg_s(SYS_IMP_APL_PMESR0_EL1); > + val &= ~((u64)0xff << shift); > + val |= (u64)event << shift; > + write_sysreg_s(val, SYS_IMP_APL_PMESR0_EL1); > + break; > + case 6 ... 9: > + shift = (index - 6) * 8; > + val = read_sysreg_s(SYS_IMP_APL_PMESR1_EL1); > + val &= ~((u64)0xff << shift); > + val |= (u64)event << shift; > + write_sysreg_s(val, SYS_IMP_APL_PMESR1_EL1); > + break; > + } > +} I'd love an explanation what's happening here. > + /* > + * Place the event on the first free counter that can count > + * this event. > + * > + * We could do a better job if we had a view of all the events > + * counting on the PMU at any given time, and by placing the > + * most constraint events first. > + */ constraining > +static int m1_pmu_device_probe(struct platform_device *pdev) > +{ > + int ret; > + > + ret = arm_pmu_device_probe(pdev, m1_pmu_of_device_ids, NULL); > + if (!ret) { > + /* > + * If probe succeeds, taint the kernel as this is all > + * undocumented, implementation defined black magic. > + */ > + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); > + } > + > + return ret; > +} What are the implications of this taint? You could say that about every driver we've written for the M1, but... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel