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 A7FC1A0548; Fri, 23 Apr 2021 13:37:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6B2B5410D8; Fri, 23 Apr 2021 13:37:56 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 0AB624014F for ; Fri, 23 Apr 2021 13:37:54 +0200 (CEST) IronPort-SDR: +oh5Vy13I179NE4a9evCVXPZwvqBxNGBHk5tzSDpteE3uQlMHSvfxzJ7xJq2iKI2r2Sjtlh4bz rgxAfMghOJ/g== X-IronPort-AV: E=McAfee;i="6200,9189,9962"; a="196113263" X-IronPort-AV: E=Sophos;i="5.82,245,1613462400"; d="scan'208";a="196113263" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2021 04:37:53 -0700 IronPort-SDR: d8EV6iRRrbsxxZ49ya6xTeyaq+NnV6P0vjjBIaVQf9L/z03+YKc6C+pDo/ftJYaUy5FRBrK8kt I2zbdTBVMxog== X-IronPort-AV: E=Sophos;i="5.82,245,1613462400"; d="scan'208";a="421736848" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.223.227]) ([10.213.223.227]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2021 04:37:52 -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: <6501cab9-455a-558d-83e4-e76ff7fee169@intel.com> Date: Fri, 23 Apr 2021 12:37:48 +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 11:02 AM, Richael Zhuang wrote: > > >> -----Original Message----- >> From: Burakov, Anatoly >> Sent: Thursday, April 22, 2021 6:00 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 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 > Thanks. I'll rework it to make it look better. > > Best Regards, > Richael > Hi, FYI i've updated my refactor patch [1] so that you could base your work off it and not have to do most of it yourself. Feel free to take over the patchset if you like! [1] http://patches.dpdk.org/project/dpdk/patch/83b1e89d14834251d4d7e72fcc19d82dfb52686d.1619175790.git.anatoly.burakov@intel.com/ -- Thanks, Anatoly