From: Ionela Voinescu <ionela.voinescu@arm.com> To: Lukasz Luba <lukasz.luba@arm.com> Cc: catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com, maz@kernel.org, suzuki.poulose@arm.com, sudeep.holla@arm.com, valentin.schneider@arm.com, rjw@rjwysocki.net, peterz@infradead.org, mingo@redhat.com, vincent.guittot@linaro.org, viresh.kumar@linaro.org, linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH v3 6/7] arm64: use activity monitors for frequency invariance Date: Thu, 13 Feb 2020 09:47:04 +0000 [thread overview] Message-ID: <20200213094704.GA30335@arm.com> (raw) In-Reply-To: <a63fd15f-f02d-6afe-6e1f-817b9eb452d1@arm.com> Hi Lukasz, [..] > > + > > +/* Obtain max frequency (in KHz) as reported by hardware */ > > +__weak unsigned int cpu_get_max_freq(unsigned int cpu) > > +{ > > + return 0; > > +} > > + > > +#ifdef CONFIG_CPU_FREQ > > +/* Replace max frequency getter with cpufreq based function */ > > +#define cpu_get_max_freq cpufreq_get_hw_max_freq > > +#endif > > Can we just use cpufreq_get_hw_max_freq()? > We have cpufreq_get_hw_max_freq returning 0 in such case, so it should > be OK. > The reasoning for the implementation is the following: - For CONFIG_CPU_FREQ we use cpufreq_get_hw_max_freq (weak default or strong alternative) - For !CONFIG_CPU_FREQ cpufreq_get_hw_max_freq returns 0 - it signals that cpufreq cannot return the hardware max frequency. In this case cpu_get_max_freq is used (weak default or strong alternative implementation). > Is there a possibility that some platform which has !CONFIG_CPU_FREQ > would define its own cpu_get_max_freq() overwriting the weak function > above? > Based on the code which checks 'if (unlikely(!max_freq_hz))' it should, > otherwise 'valid_cpus' is not set. > > I would assume that we won't see such platform, interested > in AMU freq invariance without CONFIG_CPU_FREQ. > > We already have a lot of these defines or __weak functions, which is > hard to follow. There is no dependency between CONFIG_CPU_FREQ and frequency invariance. Therefore, I did not see a reason to potentially bypass the use of AMU for frequency invariance for !CONFIG_CPU_FREQ. But I agree it makes the code harder to read so I can remove cpu_get_max_freq and keep cpufreq_get_hw_max_freq only until there is a provable need for this. Thank you for the review, Ionela.
WARNING: multiple messages have this Message-ID (diff)
From: Ionela Voinescu <ionela.voinescu@arm.com> To: Lukasz Luba <lukasz.luba@arm.com> Cc: mark.rutland@arm.com, maz@kernel.org, suzuki.poulose@arm.com, peterz@infradead.org, catalin.marinas@arm.com, linux-pm@vger.kernel.org, linux-doc@vger.kernel.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, mingo@redhat.com, viresh.kumar@linaro.org, sudeep.holla@arm.com, will@kernel.org, valentin.schneider@arm.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 6/7] arm64: use activity monitors for frequency invariance Date: Thu, 13 Feb 2020 09:47:04 +0000 [thread overview] Message-ID: <20200213094704.GA30335@arm.com> (raw) In-Reply-To: <a63fd15f-f02d-6afe-6e1f-817b9eb452d1@arm.com> Hi Lukasz, [..] > > + > > +/* Obtain max frequency (in KHz) as reported by hardware */ > > +__weak unsigned int cpu_get_max_freq(unsigned int cpu) > > +{ > > + return 0; > > +} > > + > > +#ifdef CONFIG_CPU_FREQ > > +/* Replace max frequency getter with cpufreq based function */ > > +#define cpu_get_max_freq cpufreq_get_hw_max_freq > > +#endif > > Can we just use cpufreq_get_hw_max_freq()? > We have cpufreq_get_hw_max_freq returning 0 in such case, so it should > be OK. > The reasoning for the implementation is the following: - For CONFIG_CPU_FREQ we use cpufreq_get_hw_max_freq (weak default or strong alternative) - For !CONFIG_CPU_FREQ cpufreq_get_hw_max_freq returns 0 - it signals that cpufreq cannot return the hardware max frequency. In this case cpu_get_max_freq is used (weak default or strong alternative implementation). > Is there a possibility that some platform which has !CONFIG_CPU_FREQ > would define its own cpu_get_max_freq() overwriting the weak function > above? > Based on the code which checks 'if (unlikely(!max_freq_hz))' it should, > otherwise 'valid_cpus' is not set. > > I would assume that we won't see such platform, interested > in AMU freq invariance without CONFIG_CPU_FREQ. > > We already have a lot of these defines or __weak functions, which is > hard to follow. There is no dependency between CONFIG_CPU_FREQ and frequency invariance. Therefore, I did not see a reason to potentially bypass the use of AMU for frequency invariance for !CONFIG_CPU_FREQ. But I agree it makes the code harder to read so I can remove cpu_get_max_freq and keep cpufreq_get_hw_max_freq only until there is a provable need for this. Thank you for the review, Ionela. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-02-13 9:47 UTC|newest] Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-11 18:45 [PATCH v3 0/7] arm64: ARMv8.4 Activity Monitors support Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-11 18:45 ` [PATCH v3 1/7] arm64: add support for the AMU extension v1 Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 11:30 ` Suzuki Kuruppassery Poulose 2020-02-12 11:30 ` Suzuki Kuruppassery Poulose 2020-02-12 14:54 ` Valentin Schneider 2020-02-12 14:54 ` Valentin Schneider 2020-02-12 16:10 ` Ionela Voinescu 2020-02-12 16:10 ` Ionela Voinescu 2020-02-12 16:20 ` Suzuki Kuruppassery Poulose 2020-02-12 16:20 ` Suzuki Kuruppassery Poulose 2020-02-12 18:20 ` Ionela Voinescu 2020-02-12 18:20 ` Ionela Voinescu 2020-02-12 19:24 ` Suzuki K Poulose 2020-02-12 19:24 ` Suzuki K Poulose 2020-02-12 20:19 ` Ionela Voinescu 2020-02-12 20:19 ` Ionela Voinescu 2020-02-12 16:24 ` Vladimir Murzin 2020-02-12 16:24 ` Vladimir Murzin 2020-02-12 18:27 ` Ionela Voinescu 2020-02-12 18:27 ` Ionela Voinescu 2020-02-11 18:45 ` [PATCH v3 2/7] arm64: trap to EL1 accesses to AMU counters from EL0 Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 11:44 ` Suzuki Kuruppassery Poulose 2020-02-12 11:44 ` Suzuki Kuruppassery Poulose 2020-02-12 15:36 ` Valentin Schneider 2020-02-12 15:36 ` Valentin Schneider 2020-02-11 18:45 ` [PATCH v3 3/7] arm64/kvm: disable access to AMU registers from kvm guests Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 15:36 ` Valentin Schneider 2020-02-12 15:36 ` Valentin Schneider 2020-02-12 16:33 ` Suzuki Kuruppassery Poulose 2020-02-12 16:33 ` Suzuki Kuruppassery Poulose 2020-02-11 18:45 ` [PATCH v3 4/7] Documentation: arm64: document support for the AMU extension Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 15:36 ` Valentin Schneider 2020-02-12 15:36 ` Valentin Schneider 2020-02-11 18:45 ` [PATCH v3 5/7] cpufreq: add function to get the hardware max frequency Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 4:14 ` Viresh Kumar 2020-02-12 4:14 ` Viresh Kumar 2020-02-13 11:59 ` Valentin Schneider 2020-02-13 11:59 ` Valentin Schneider 2020-02-13 12:59 ` Ionela Voinescu 2020-02-13 12:59 ` Ionela Voinescu 2020-02-13 15:22 ` Valentin Schneider 2020-02-13 15:22 ` Valentin Schneider 2020-02-11 18:45 ` [PATCH v3 6/7] arm64: use activity monitors for frequency invariance Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 18:59 ` Lukasz Luba 2020-02-12 18:59 ` Lukasz Luba 2020-02-13 9:47 ` Ionela Voinescu [this message] 2020-02-13 9:47 ` Ionela Voinescu 2020-02-17 16:59 ` Valentin Schneider 2020-02-17 16:59 ` Valentin Schneider 2020-02-23 18:49 ` Ionela Voinescu 2020-02-23 18:49 ` Ionela Voinescu 2020-02-11 18:45 ` [PATCH v3 7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate Ionela Voinescu 2020-02-11 18:45 ` Ionela Voinescu 2020-02-12 9:30 ` Valentin Schneider 2020-02-12 9:30 ` Valentin Schneider 2020-02-12 10:32 ` Ionela Voinescu 2020-02-12 10:32 ` Ionela Voinescu 2020-02-12 10:01 ` Lukasz Luba 2020-02-12 10:01 ` Lukasz Luba 2020-02-12 10:12 ` Marc Zyngier 2020-02-12 10:12 ` Marc Zyngier 2020-02-12 10:54 ` Ionela Voinescu 2020-02-12 10:54 ` Ionela Voinescu 2020-02-12 10:55 ` Lukasz Luba 2020-02-12 10:55 ` Lukasz Luba 2020-02-12 11:10 ` Marc Zyngier 2020-02-12 11:10 ` Marc Zyngier 2020-02-12 11:43 ` Lukasz Luba 2020-02-12 11:43 ` Lukasz Luba 2020-02-12 11:12 ` Valentin Schneider 2020-02-12 11:12 ` Valentin Schneider 2020-02-14 0:35 ` Thomas Gleixner 2020-02-14 0:35 ` Thomas Gleixner 2020-02-14 15:45 ` Ionela Voinescu 2020-02-14 15:45 ` Ionela Voinescu 2020-02-14 15:57 ` Ionela Voinescu 2020-02-14 15:57 ` Ionela Voinescu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200213094704.GA30335@arm.com \ --to=ionela.voinescu@arm.com \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=lukasz.luba@arm.com \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=rjw@rjwysocki.net \ --cc=sudeep.holla@arm.com \ --cc=suzuki.poulose@arm.com \ --cc=valentin.schneider@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=viresh.kumar@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.