From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4240EA09E4; Thu, 22 Apr 2021 11:59:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 29A104068E; Thu, 22 Apr 2021 11:59:46 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 3E4CA4003D for ; Thu, 22 Apr 2021 11:59:43 +0200 (CEST) IronPort-SDR: 1w1/+Z4llQwFZQw+ZSNem2q5/hpIw5sezGYusWM6d/iH6jhNBctaON45pE080ryqxIcdqWASoh 8BH7gqD8kJIQ== X-IronPort-AV: E=McAfee;i="6200,9189,9961"; a="195972097" X-IronPort-AV: E=Sophos;i="5.82,241,1613462400"; d="scan'208";a="195972097" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2021 02:59:42 -0700 IronPort-SDR: yqaAnLC75WgcnwUkmrk0vQ5b/sWpYGH+UPQC08t9hloGRxFq4S5e0JQQ7Jy934ofa+bX06NzZm tdSxQtK+9CxQ== X-IronPort-AV: E=Sophos;i="5.82,242,1613462400"; d="scan'208";a="384721325" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.222.135]) ([10.213.222.135]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2021 02:59:41 -0700 To: Richael Zhuang , "dev@dpdk.org" Cc: nd , David Hunt References: <20210422061540.23304-1-richael.zhuang@arm.com> <20210422061540.23304-2-richael.zhuang@arm.com> <05d09823-3b20-0192-b03c-9f086b041b36@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Thu, 22 Apr 2021 10:59:38 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc cpufreq X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 22-Apr-21 10:29 AM, Richael Zhuang wrote: > > >> -----Original Message----- >> From: Burakov, Anatoly >> Sent: Thursday, April 22, 2021 5:06 PM >> To: Richael Zhuang ; dev@dpdk.org >> Cc: nd ; David Hunt >> Subject: Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc cpufreq >> >> On 22-Apr-21 7:15 AM, Richael Zhuang wrote: >>> Currently in DPDK only acpi_cpufreq and pstate_cpufreq drivers are >>> supported, which are both not available on arm64 platforms. Add >>> support for cppc_cpufreq driver which works on most arm64 platforms. >>> >>> Signed-off-by: Richael Zhuang >>> --- >> >> Just a general note: this looks like a copy-paste of pstate code. Which is >> perfectly fine, except that we can do better than copying some faults of the >> pstate code to other drivers. I've submitted a patch [1] attempting to fix >> some of the pressing issues and code duplication in pstate driver, but i'm >> sure with a fresh driver, you can do even better :) >> >> [1] >> http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1- >> anatoly.burakov@intel.com/ >> >> -- >> Thanks, >> Anatoly > > For CPPC is defined in acpi v5.0+ spec, I reused most code in acpi_cpufreq to get a quick workable version on our platform with only cppc driver. I have verified its basic functions. If you find some problems please help to point out thus I can rework it. Thanks . > > Best Regards, > Richael > Well, pstate code was copied from ACPI so it does share the same flaws: - Lots of code duplication (e.g. snprintf for filename, fopen sequences, etc.) - Confusing and bug-prone error handling (e.g. return macros in the middle of a function) - Mixing power management logic and gory details of string handling Good examples of the above are in your `power_check_turbo()` function - lots of string handling code interspersed with file opens, and actual logic of power management. Please see the patch i linked earlier [1] to understand what kind of changes i'm suggesting. Perhaps you could do even better :) [1] http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1-anatoly.burakov@intel.com/ -- Thanks, Anatoly