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 575B343BA8; Fri, 1 Mar 2024 11:39:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D3A3D4026C; Fri, 1 Mar 2024 11:39:32 +0100 (CET) Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.9]) by mails.dpdk.org (Postfix) with ESMTP id A8BF5400D5 for ; Fri, 1 Mar 2024 11:39:30 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1709289570; x=1740825570; h=message-id:date:subject:to:cc:references:from: in-reply-to:content-transfer-encoding:mime-version; bh=YXOGY0p3PkXfINAOeEO0U9GlJ+ZNrvs9vDpMy4IS7Tg=; b=ddu3BxDYzoLVeYBCFUZRwPdlL1Rpkw+Su6vopGaZtmZMWHvkbly7+aDk 8UpFn2g2E6FnzzcwFHQd87x6miCR3SPtYq8Bm3nBHdO9Nt1uy4Eokwv/J dQiP8nAPzf9spvJCshMrCjl2x5MfeYtAFRpsQi1r1AZXtJzAodFfBsEEf 7Fczz8f7quWdXOsrDl/zL0ctE3MyGWYtoBZef8nMFAastoameM+9dokhJ EjhiP8sXV4/HAbMRU4R2Gv/GSNbCmSr19W97hE3uDKq4uRGAG/v5khZhZ 9yXNCLzE4NuL7EJrYbaQwuGIurMUY4yS5dHbE0n7tNUq/3JXeGIaj04TJ g==; X-IronPort-AV: E=McAfee;i="6600,9927,10999"; a="14526973" X-IronPort-AV: E=Sophos;i="6.06,196,1705392000"; d="scan'208";a="14526973" Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by fmvoesa103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Mar 2024 02:39:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,196,1705392000"; d="scan'208";a="39176262" Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by fmviesa001.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 01 Mar 2024 02:39:29 -0800 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Fri, 1 Mar 2024 02:39:28 -0800 Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Fri, 1 Mar 2024 02:39:28 -0800 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend Transport; Fri, 1 Mar 2024 02:39:28 -0800 Received: from NAM04-MW2-obe.outbound.protection.outlook.com (104.47.73.168) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Fri, 1 Mar 2024 02:39:24 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bIaiyb8ID4TGoDW5s5tt9jASuAPTAjGpHj/2N7NHHru4kzTt6TGgVTpmcs4fDNchsBT6SeS8H4WhL37JKm6Q4ENJ/dMWz1UaVR/S0X3/2SUj+uGgTDXevXmLk0g9K+Osogq+wY6qHj3FEhwvR7G6rydvu516UTDVR9h5lIAoV89R4HdLdDEOpT1jcjiuQ/t4tkrrSHp1u54BnIS/oorrhzRzgOPMYoICSa7zlpofLL8KU4I3WNq72yttQOR3Md9EZxcIayWEeJfI3mfmUHOVsxLgaUhTX/TZmubDNo6GTUZ8BF+wf33R6re0fsQ6SB9WSEErugxVyafQs6sTSejW1g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=y/bs6TMfpTdG10z6N8SO7etaEKfv7YR25dR42e0rLoU=; b=Kb1QnVObuVuoSZpGD6uYZwPpyssdy45uY4SAmcqcR8/AXEYeB6zJaDsZwwgmFOZ/PTcUxAMXocMFF+5r2WW8upXeUcGmDSeQfFfMLPoJoBi4wq9Ookfwv3deZ4AFuA9AcDINRB8rcbXrOCrcSmjPyJJt16ZkrhzntAtd789P8kcBFXstI2kwZKoHMNcuqOt4dyVK6nNUstrO+3I0dz5Dv8oYwWurIlSCSbf4E5kkXRktCJgCsrHM512G83Ez9vusr2437eZPRqEwBuwkrYsOCI7CeMwDp2K3jcMOj+N7ThKrfD1n5OfFTEKGD9ddOIp+tU1k79rx3E7s4lYLrTqsBQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; Received: from LV3PR11MB8579.namprd11.prod.outlook.com (2603:10b6:408:1b6::7) by PH8PR11MB6658.namprd11.prod.outlook.com (2603:10b6:510:1c1::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7362.18; Fri, 1 Mar 2024 10:39:22 +0000 Received: from LV3PR11MB8579.namprd11.prod.outlook.com ([fe80::796b:62a5:96dd:68a2]) by LV3PR11MB8579.namprd11.prod.outlook.com ([fe80::796b:62a5:96dd:68a2%7]) with mapi id 15.20.7339.022; Fri, 1 Mar 2024 10:39:22 +0000 Message-ID: Date: Fri, 1 Mar 2024 10:39:18 +0000 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 1/2] power: refactor core power management library To: "lihuisong (C)" , Sivaprasad Tummala , , , , , , , CC: References: <20240220153326.6236-1-sivaprasad.tummala@amd.com> <20240220153326.6236-3-sivaprasad.tummala@amd.com> <6c7c0383-3eb4-049c-fedd-7d66b1b738da@huawei.com> Content-Language: en-US From: "Hunt, David" In-Reply-To: <6c7c0383-3eb4-049c-fedd-7d66b1b738da@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: DU7PR01CA0033.eurprd01.prod.exchangelabs.com (2603:10a6:10:50e::11) To LV3PR11MB8579.namprd11.prod.outlook.com (2603:10b6:408:1b6::7) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: LV3PR11MB8579:EE_|PH8PR11MB6658:EE_ X-MS-Office365-Filtering-Correlation-Id: 4ffc1105-2910-4054-b3d4-08dc39dbdb5d X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: W9mxSQKxZHAEi/elsAM4J9Ye3+AfoMMvIujuVCZ5DYs3DSI9JTkzpQ1oJdlFAbifzhCg7RZp0p8kqxZ8cRrksyYEI7MfO6JJLoDV22+TUsfXO+Wk90IpTi7gOlJH8MIbe5MR7UAME/lQ7pXZ4CsBNhp/7ZOb7lKq92pnqRkUK8YuAiYUNFoRdwSpEnRsFeLwt2TDX8rsmTX79tylSSWSHossJRGWPvTUtMJcDunCDg8S7OGKMCepi0qLokapkvd8aZy56w+W9zLKA3nT1fHKN0DmDn2B9TJuscCrcccmoJVrgrJNP/dMD8v51W3aPunzzmWyVUnvgC6arzux6wxykIgVkZ7lr58A1zSgSAWgqbin6keT6mjRpw+Kr4Jff3bNeFLM2ejZHBDBYLClAwZ0Ezz9p8q2RgJ0x4u7IidYE5410JYgrG0YYHjEtPwXHFbivwdtEU1xGUPLeenDPYhmgHh1yeW5tKIFYV4iKNHT22qZhjHEkxgLoJd+KInkRMqyQUGi1N7SySgaiz8YKKl4gYcw263S0YTZDt8MyvZEDZJP2Nw6mV7PZEm4W/H/IgyFXreST83mlMl5fMZbnoa2oc8t/aXyA1NeB+m9jJkTU44CrNdrvVQwyZY/fBiUtbdBjwwfOmOayEu82LR5VXS5Ew== X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:LV3PR11MB8579.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230031); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TXhycUJLeGdiRk9Pb1lCME5FWE9KZS9LRVkrTXJYUG9qZERRM01kU3Z3NjB6?= =?utf-8?B?cENXREVvbHlSTGh0UEtoRE5Ta3o5QlY4WlFIV0w5NmE3eFJscDY1bnhvVWlQ?= =?utf-8?B?czRLM1NmbVYvZ0p6S2NaUjJacTY5ZTVZNVdyYTJGUTE5Q0t0QWJrK05QeUNo?= =?utf-8?B?L3BxbGlYSmxHVjAwMkN5eDlOU09yTW5UeVBZMUlOUVBkZ2UrUGw5NFhYbm5C?= =?utf-8?B?aU5nSWhmbGNCaFhHRmhIUGFNRndTUk9EbTNOUUdaS0dIVzRrTnJWNkhTQzA4?= =?utf-8?B?dDJkZFhLRjN1U0g1WUo0V2hSUzhxaC9UNVBQY1orZlN6Z2g5NnlieDBubSsr?= =?utf-8?B?clJRb1E0VWNmRitPSWYrTHpBUXFGWHg4bWVOTlhiZHJLMzRlemljZWkrSGhE?= =?utf-8?B?Sko0MlhxM1ROUThWYVppSEZNSDVLQXJBOTcyUFNzdkJlZ2dxVGx5M2tzZkZR?= =?utf-8?B?Vy9DT1FuUnRGWXEwNnFZZ0xqUmk1alc4RlRJc20wcWU2YnRCTW1tRWhRN2ZD?= =?utf-8?B?WGR3ZDY3ckN1TnRKaDZBSEF5cktPaUwxVnhrN2UxQmRDWXkwSDFwejM3ODFo?= =?utf-8?B?LzI3WjNTTUNDclp0dHZVZmM5RmNKN2JMZjNGVjdKV1lyRkFpb0NmeVBXKzVy?= =?utf-8?B?MHM5c3owbk0wTndmU0tpTTNpSjR2UEpUR1pXanlZaW53N21icTNLb2hmSEQx?= =?utf-8?B?Y2xrOW56cEJNS3FnUGRRWndldFU1YXhocTlJVnZWSE9ScjhDaXNlclJ2aTgz?= =?utf-8?B?T0ZyOWI1VHBFUGZwSG44K004Mk1HMTVZTEhJalJ1TTBIWGYwU3F0aUkwNUl0?= =?utf-8?B?d3BERlIzMFY0L2lIcmE0ak5WNFo1SlkyMDVXazNCQXhxUmMyZ2xXMnpGQ1FF?= =?utf-8?B?NXlQNnl0SHFoMUFJQ2piaG5ycGJtNHFrbHhhdWM4SDdwY2F6cmZod2EzWXNH?= =?utf-8?B?WC96dEN1YVFIZDQxRHJrTFVuVzJxT3Y3dUJUZEZSYlJ6aUp6VG00dGVXdE0x?= =?utf-8?B?eVh0Z3NCeHA0WGl5Qit2ME5seEppdHRhdEFCTjFwbHlmdldFczVPQnJEalRr?= =?utf-8?B?Mnk0U280aGU1bnEwVjg3dUpXb2VTYzc3Z0JraGdXaFFHcVR2UVNWNlBJS2I2?= =?utf-8?B?K2dIL0xDUm1qV0x1R0kzSkxsakdYdVBLRnM4ZFMrWGFVamhFZHNSa092eENQ?= =?utf-8?B?NDQvME9zQVZWM0UrcnIvK2ZCMFFpRURDbjJsVFp1UmNMV0RJMDdhZHVSbFFO?= =?utf-8?B?aXdxSEVNRTNzVTVLS1BxRmU4RkhZNkNKdU5ESTFPeXhxS2NxWE8ySHB0L1lN?= =?utf-8?B?bkwxaTJzUnhuRGFDOG5Cd2xGY2JSRVBNeHVNU3ZUNG93Rm9aQVFJcE5XU1hm?= =?utf-8?B?WTJaTThkb2dLQWFrbW9qN0F2S2U5YnRQSXZTeDAzT0t5RGZrY0M2WW5jNjBo?= =?utf-8?B?dGg3OVBCOUhaUDkyOURncDNJbjQvZDBjNzY4cFRKcUJVVW04RExrNDY0cE5h?= =?utf-8?B?TjNsdUhLVnVneFJFU1U5NEN2WG9hTFFiaXB2ODRQSzNTbXMwR05hVEtZdit0?= =?utf-8?B?SGdESXIyWDhjSU41c0ZpeHhKY00rbnBFdkhxeGVHWFcyeWpQdWt3UzFqTzBj?= =?utf-8?B?RnNtQkplamVlNVFnRGw1QmR0SjU5VFd5Snc5WjV2UUdwOEZvSnZiUDFlcEQ4?= =?utf-8?B?Vy96SzIwbmR4eVFEUDI5U2trR3Fzbk1lSTlwMWloNHNEdVBYcXhMUGlBTng3?= =?utf-8?B?MnRMdERxWW5DY1NQUGRLMFNKdmhjbWlseHA4RHZrZGVZK29GalhQZURXb2xL?= =?utf-8?B?WXBCeG55cXgvaGEyc0JjeUhGL29OVC9JWk5uY1VvRlk3QkpiQWFQTkRxaklJ?= =?utf-8?B?ZGZRWDZDQ005RUN5R0hxTWZNYUpGUVFIekU5dU81bXRJVTlmOG1iQUZCcDdG?= =?utf-8?B?T2dwRHI0YW8xdUcwd21JVzVtaEtycHpFTkpkWktYeEdzMWdESENGRktzYUlp?= =?utf-8?B?SWpnc1BRcE1LRTJ6UitSNWRjQkkzTUxvVElNcG9QQ2hFWGNqRE5BaXNCL01W?= =?utf-8?B?RG82QWt2eXpZNmlZcU9qT0o2bVIzUit4WDlmWllRdG80R2lnUktiUXJpSmVU?= =?utf-8?Q?Ia+TwOEMTRHMYRq3GWL5v3IHJ?= X-MS-Exchange-CrossTenant-Network-Message-Id: 4ffc1105-2910-4054-b3d4-08dc39dbdb5d X-MS-Exchange-CrossTenant-AuthSource: LV3PR11MB8579.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Mar 2024 10:39:22.2441 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: 57iDHHcWgHtCNmaGpzb7C6JcVcIxDpXTYXAkopEtQoihmNISmzGtGrEU/aidC0kAiJVixfsToAWYqNR4n5wumg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6658 X-OriginatorOrg: intel.com 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 On 01/03/2024 02:56, lihuisong (C) wrote: > > 在 2024/2/20 23:33, Sivaprasad Tummala 写道: >> This patch introduces a comprehensive refactor to the core power >> management library. The primary focus is on improving modularity >> and organization by relocating specific driver implementations >> from the 'lib/power' directory to dedicated directories within >> 'drivers/power/core/*'. The adjustment of meson.build files >> enables the selective activation of individual drivers. >> >> These changes contribute to a significant enhancement in code >> organization, providing a clearer structure for driver implementations. >> The refactor aims to improve overall code clarity and boost >> maintainability. Additionally, it establishes a foundation for >> future development, allowing for more focused work on individual >> drivers and seamless integration of forthcoming enhancements. > > Good job. +1 to refacotor. > > <...> > Also a +1 from me, looks like a sensible re-organisation of the power code. Regards, Dave. >> diff --git a/drivers/meson.build b/drivers/meson.build >> index f2be71bc05..e293c3945f 100644 >> --- a/drivers/meson.build >> +++ b/drivers/meson.build >> @@ -28,6 +28,7 @@ subdirs = [ >>           'event',          # depends on common, bus, mempool and net. >>           'baseband',       # depends on common and bus. >>           'gpu',            # depends on common and bus. >> +        'power',          # depends on common (in future). >>   ] >>     if meson.is_cross_build() >> diff --git a/drivers/power/core/acpi/meson.build >> b/drivers/power/core/acpi/meson.build >> new file mode 100644 >> index 0000000000..d10ec8ee94 >> --- /dev/null >> +++ b/drivers/power/core/acpi/meson.build >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +sources = files('power_acpi_cpufreq.c') >> + >> +headers = files('power_acpi_cpufreq.h') >> + >> +deps += ['power'] >> diff --git a/lib/power/power_acpi_cpufreq.c >> b/drivers/power/core/acpi/power_acpi_cpufreq.c >> similarity index 95% >> rename from lib/power/power_acpi_cpufreq.c >> rename to drivers/power/core/acpi/power_acpi_cpufreq.c > This file is in power lib. > How about remove the 'power' prefix of this file name? > like acpi_cpufreq.c, cppc_cpufreq.c. >> index f8d978d03d..69d80ad2ae 100644 >> --- a/lib/power/power_acpi_cpufreq.c >> +++ b/drivers/power/core/acpi/power_acpi_cpufreq.c >> @@ -577,3 +577,22 @@ int power_acpi_get_capabilities(unsigned int >> lcore_id, >>         return 0; >>   } >> + >> +static struct rte_power_ops acpi_ops = { > How about use the following structure name? > "struct rte_power_cpufreq_ops" or "struct rte_power_core_ops" > After all, we also have other power ops, like uncore, right? >> +    .init = power_acpi_cpufreq_init, >> +    .exit = power_acpi_cpufreq_exit, >> +    .check_env_support = power_acpi_cpufreq_check_supported, >> +    .get_avail_freqs = power_acpi_cpufreq_freqs, >> +    .get_freq = power_acpi_cpufreq_get_freq, >> +    .set_freq = power_acpi_cpufreq_set_freq, >> +    .freq_down = power_acpi_cpufreq_freq_down, >> +    .freq_up = power_acpi_cpufreq_freq_up, >> +    .freq_max = power_acpi_cpufreq_freq_max, >> +    .freq_min = power_acpi_cpufreq_freq_min, >> +    .turbo_status = power_acpi_turbo_status, >> +    .enable_turbo = power_acpi_enable_turbo, >> +    .disable_turbo = power_acpi_disable_turbo, >> +    .get_caps = power_acpi_get_capabilities >> +}; >> + >> +RTE_POWER_REGISTER_OPS(acpi_ops); >> diff --git a/lib/power/power_acpi_cpufreq.h >> b/drivers/power/core/acpi/power_acpi_cpufreq.h >> similarity index 100% >> rename from lib/power/power_acpi_cpufreq.h >> rename to drivers/power/core/acpi/power_acpi_cpufreq.h >> diff --git a/drivers/power/core/amd-pstate/meson.build >> b/drivers/power/core/amd-pstate/meson.build >> new file mode 100644 >> index 0000000000..8ec4c960f5 >> --- /dev/null >> +++ b/drivers/power/core/amd-pstate/meson.build >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +sources = files('power_amd_pstate_cpufreq.c') >> + >> +headers = files('power_amd_pstate_cpufreq.h') >> + >> +deps += ['power'] >> diff --git a/lib/power/power_amd_pstate_cpufreq.c >> b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c >> similarity index 95% >> rename from lib/power/power_amd_pstate_cpufreq.c >> rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c >> index 028f84416b..9938de72a6 100644 >> --- a/lib/power/power_amd_pstate_cpufreq.c >> +++ b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c >> @@ -700,3 +700,22 @@ power_amd_pstate_get_capabilities(unsigned int >> lcore_id, >>         return 0; >>   } >> + >> +static struct rte_power_ops amd_pstate_ops = { >> +    .init = power_amd_pstate_cpufreq_init, >> +    .exit = power_amd_pstate_cpufreq_exit, >> +    .check_env_support = power_amd_pstate_cpufreq_check_supported, >> +    .get_avail_freqs = power_amd_pstate_cpufreq_freqs, >> +    .get_freq = power_amd_pstate_cpufreq_get_freq, >> +    .set_freq = power_amd_pstate_cpufreq_set_freq, >> +    .freq_down = power_amd_pstate_cpufreq_freq_down, >> +    .freq_up = power_amd_pstate_cpufreq_freq_up, >> +    .freq_max = power_amd_pstate_cpufreq_freq_max, >> +    .freq_min = power_amd_pstate_cpufreq_freq_min, >> +    .turbo_status = power_amd_pstate_turbo_status, >> +    .enable_turbo = power_amd_pstate_enable_turbo, >> +    .disable_turbo = power_amd_pstate_disable_turbo, >> +    .get_caps = power_amd_pstate_get_capabilities >> +}; >> + >> +RTE_POWER_REGISTER_OPS(amd_pstate_ops); >> diff --git a/lib/power/power_amd_pstate_cpufreq.h >> b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h >> similarity index 100% >> rename from lib/power/power_amd_pstate_cpufreq.h >> rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h >> diff --git a/drivers/power/core/cppc/meson.build >> b/drivers/power/core/cppc/meson.build >> new file mode 100644 >> index 0000000000..06f3b99bb8 >> --- /dev/null >> +++ b/drivers/power/core/cppc/meson.build >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +sources = files('power_cppc_cpufreq.c') >> + >> +headers = files('power_cppc_cpufreq.h') >> + >> +deps += ['power'] >> diff --git a/lib/power/power_cppc_cpufreq.c >> b/drivers/power/core/cppc/power_cppc_cpufreq.c >> similarity index 96% >> rename from lib/power/power_cppc_cpufreq.c >> rename to drivers/power/core/cppc/power_cppc_cpufreq.c >> index 3ddf39bd76..605f633309 100644 >> --- a/lib/power/power_cppc_cpufreq.c >> +++ b/drivers/power/core/cppc/power_cppc_cpufreq.c >> @@ -685,3 +685,22 @@ power_cppc_get_capabilities(unsigned int lcore_id, >>         return 0; >>   } >> + >> +static struct rte_power_ops cppc_ops = { >> +    .init = power_cppc_cpufreq_init, >> +    .exit = power_cppc_cpufreq_exit, >> +    .check_env_support = power_cppc_cpufreq_check_supported, >> +    .get_avail_freqs = power_cppc_cpufreq_freqs, >> +    .get_freq = power_cppc_cpufreq_get_freq, >> +    .set_freq = power_cppc_cpufreq_set_freq, >> +    .freq_down = power_cppc_cpufreq_freq_down, >> +    .freq_up = power_cppc_cpufreq_freq_up, >> +    .freq_max = power_cppc_cpufreq_freq_max, >> +    .freq_min = power_cppc_cpufreq_freq_min, >> +    .turbo_status = power_cppc_turbo_status, >> +    .enable_turbo = power_cppc_enable_turbo, >> +    .disable_turbo = power_cppc_disable_turbo, >> +    .get_caps = power_cppc_get_capabilities >> +}; >> + >> +RTE_POWER_REGISTER_OPS(cppc_ops); >> diff --git a/lib/power/power_cppc_cpufreq.h >> b/drivers/power/core/cppc/power_cppc_cpufreq.h >> similarity index 100% >> rename from lib/power/power_cppc_cpufreq.h >> rename to drivers/power/core/cppc/power_cppc_cpufreq.h >> diff --git a/lib/power/guest_channel.c >> b/drivers/power/core/kvm-vm/guest_channel.c >> similarity index 100% >> rename from lib/power/guest_channel.c >> rename to drivers/power/core/kvm-vm/guest_channel.c >> diff --git a/lib/power/guest_channel.h >> b/drivers/power/core/kvm-vm/guest_channel.h >> similarity index 100% >> rename from lib/power/guest_channel.h >> rename to drivers/power/core/kvm-vm/guest_channel.h >> diff --git a/drivers/power/core/kvm-vm/meson.build >> b/drivers/power/core/kvm-vm/meson.build >> new file mode 100644 >> index 0000000000..3150c6674b >> --- /dev/null >> +++ b/drivers/power/core/kvm-vm/meson.build >> @@ -0,0 +1,20 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(C) 2024 AMD Limited. >> +# >> + >> +if not is_linux >> +    build = false >> +    reason = 'only supported on Linux' >> +    subdir_done() >> +endif >> + >> +sources = files( >> +        'guest_channel.c', >> +        'power_kvm_vm.c', >> +) >> + >> +headers = files( >> +        'guest_channel.h', >> +        'power_kvm_vm.h', >> +) >> +deps += ['power'] >> diff --git a/lib/power/power_kvm_vm.c >> b/drivers/power/core/kvm-vm/power_kvm_vm.c >> similarity index 83% >> rename from lib/power/power_kvm_vm.c >> rename to drivers/power/core/kvm-vm/power_kvm_vm.c >> index f15be8fac5..a5d6984d26 100644 >> --- a/lib/power/power_kvm_vm.c >> +++ b/drivers/power/core/kvm-vm/power_kvm_vm.c >> @@ -137,3 +137,22 @@ int power_kvm_vm_get_capabilities(__rte_unused >> unsigned int lcore_id, >>       POWER_LOG(ERR, "rte_power_get_capabilities is not implemented >> for Virtual Machine Power Management"); >>       return -ENOTSUP; >>   } >> + >> +static struct rte_power_ops kvm_vm_ops = { >> +    .init = power_kvm_vm_init, >> +    .exit = power_kvm_vm_exit, >> +    .check_env_support = power_kvm_vm_check_supported, >> +    .get_avail_freqs = power_kvm_vm_freqs, >> +    .get_freq = power_kvm_vm_get_freq, >> +    .set_freq = power_kvm_vm_set_freq, >> +    .freq_down = power_kvm_vm_freq_down, >> +    .freq_up = power_kvm_vm_freq_up, >> +    .freq_max = power_kvm_vm_freq_max, >> +    .freq_min = power_kvm_vm_freq_min, >> +    .turbo_status = power_kvm_vm_turbo_status, >> +    .enable_turbo = power_kvm_vm_enable_turbo, >> +    .disable_turbo = power_kvm_vm_disable_turbo, >> +    .get_caps = power_kvm_vm_get_capabilities >> +}; >> + >> +RTE_POWER_REGISTER_OPS(kvm_vm_ops); >> diff --git a/lib/power/power_kvm_vm.h >> b/drivers/power/core/kvm-vm/power_kvm_vm.h >> similarity index 100% >> rename from lib/power/power_kvm_vm.h >> rename to drivers/power/core/kvm-vm/power_kvm_vm.h >> diff --git a/drivers/power/core/meson.build >> b/drivers/power/core/meson.build >> new file mode 100644 >> index 0000000000..4081dafaa0 >> --- /dev/null >> +++ b/drivers/power/core/meson.build >> @@ -0,0 +1,12 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +drivers = [ >> +        'acpi', >> +        'amd-pstate', >> +        'cppc', >> +        'kvm-vm', >> +        'pstate' >> +] >> + >> +std_deps = ['power'] >> diff --git a/drivers/power/core/pstate/meson.build >> b/drivers/power/core/pstate/meson.build >> new file mode 100644 >> index 0000000000..1025c64e48 >> --- /dev/null >> +++ b/drivers/power/core/pstate/meson.build >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +sources = files('power_pstate_cpufreq.c') >> + >> +headers = files('power_pstate_cpufreq.h') >> + >> +deps += ['power'] >> diff --git a/lib/power/power_pstate_cpufreq.c >> b/drivers/power/core/pstate/power_pstate_cpufreq.c >> similarity index 96% >> rename from lib/power/power_pstate_cpufreq.c >> rename to drivers/power/core/pstate/power_pstate_cpufreq.c >> index 73138dc4e4..d4c3645ff8 100644 >> --- a/lib/power/power_pstate_cpufreq.c >> +++ b/drivers/power/core/pstate/power_pstate_cpufreq.c >> @@ -888,3 +888,22 @@ int power_pstate_get_capabilities(unsigned int >> lcore_id, >>         return 0; >>   } >> + >> +static struct rte_power_ops pstate_ops = { >> +    .init = power_pstate_cpufreq_init, >> +    .exit = power_pstate_cpufreq_exit, >> +    .check_env_support = power_pstate_cpufreq_check_supported, >> +    .get_avail_freqs = power_pstate_cpufreq_freqs, >> +    .get_freq = power_pstate_cpufreq_get_freq, >> +    .set_freq = power_pstate_cpufreq_set_freq, >> +    .freq_down = power_pstate_cpufreq_freq_down, >> +    .freq_up = power_pstate_cpufreq_freq_up, >> +    .freq_max = power_pstate_cpufreq_freq_max, >> +    .freq_min = power_pstate_cpufreq_freq_min, >> +    .turbo_status = power_pstate_turbo_status, >> +    .enable_turbo = power_pstate_enable_turbo, >> +    .disable_turbo = power_pstate_disable_turbo, >> +    .get_caps = power_pstate_get_capabilities >> +}; >> + >> +RTE_POWER_REGISTER_OPS(pstate_ops); >> diff --git a/lib/power/power_pstate_cpufreq.h >> b/drivers/power/core/pstate/power_pstate_cpufreq.h >> similarity index 100% >> rename from lib/power/power_pstate_cpufreq.h >> rename to drivers/power/core/pstate/power_pstate_cpufreq.h >> diff --git a/drivers/power/meson.build b/drivers/power/meson.build >> new file mode 100644 >> index 0000000000..7d9034c7ac >> --- /dev/null >> +++ b/drivers/power/meson.build >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: BSD-3-Clause >> +# Copyright(c) 2024 AMD Limited >> + >> +drivers = [ >> +        'core', >> +] >> + >> +std_deps = ['power'] >> diff --git a/lib/power/meson.build b/lib/power/meson.build >> index b8426589b2..207d96d877 100644 >> --- a/lib/power/meson.build >> +++ b/lib/power/meson.build >> @@ -12,14 +12,8 @@ if not is_linux >>       reason = 'only supported on Linux' >>   endif >>   sources = files( >> -        'guest_channel.c', >> -        'power_acpi_cpufreq.c', >> -        'power_amd_pstate_cpufreq.c', >>           'power_common.c', >> -        'power_cppc_cpufreq.c', >> -        'power_kvm_vm.c', >>           'power_intel_uncore.c', >> -        'power_pstate_cpufreq.c', >>           'rte_power.c', >>           'rte_power_uncore.c', >>           'rte_power_pmd_mgmt.c', >> diff --git a/lib/power/power_common.h b/lib/power/power_common.h >> index 30966400ba..c90b611f4f 100644 >> --- a/lib/power/power_common.h >> +++ b/lib/power/power_common.h >> @@ -23,13 +23,24 @@ extern int power_logtype; >>   #endif >>     /* check if scaling driver matches one we want */ >> +__rte_internal >>   int cpufreq_check_scaling_driver(const char *driver); >> + >> +__rte_internal >>   int power_set_governor(unsigned int lcore_id, const char >> *new_governor, >>           char *orig_governor, size_t orig_governor_len); >> + >> +__rte_internal >>   int open_core_sysfs_file(FILE **f, const char *mode, const char >> *format, ...) >>           __rte_format_printf(3, 4); >> + >> +__rte_internal >>   int read_core_sysfs_u32(FILE *f, uint32_t *val); >> + >> +__rte_internal >>   int read_core_sysfs_s(FILE *f, char *buf, unsigned int len); >> + >> +__rte_internal >>   int write_core_sysfs_s(FILE *f, const char *str); >>     #endif /* _POWER_COMMON_H_ */ >> diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c >> index 36c3f3da98..70176807f4 100644 >> --- a/lib/power/rte_power.c >> +++ b/lib/power/rte_power.c >> @@ -8,64 +8,80 @@ >>   #include >>     #include "rte_power.h" >> -#include "power_acpi_cpufreq.h" >> -#include "power_cppc_cpufreq.h" >>   #include "power_common.h" >> -#include "power_kvm_vm.h" >> -#include "power_pstate_cpufreq.h" >> -#include "power_amd_pstate_cpufreq.h" >>     enum power_management_env global_default_env = PM_ENV_NOT_SET; > use a pointer to save the current power cpufreq ops? >>     static rte_spinlock_t global_env_cfg_lock = >> RTE_SPINLOCK_INITIALIZER; >> +static struct rte_power_ops rte_power_ops[PM_ENV_MAX]; >>   -/* function pointers */ >> -rte_power_freqs_t rte_power_freqs  = NULL; >> -rte_power_get_freq_t rte_power_get_freq = NULL; >> -rte_power_set_freq_t rte_power_set_freq = NULL; >> -rte_power_freq_change_t rte_power_freq_up = NULL; >> -rte_power_freq_change_t rte_power_freq_down = NULL; >> -rte_power_freq_change_t rte_power_freq_max = NULL; >> -rte_power_freq_change_t rte_power_freq_min = NULL; >> -rte_power_freq_change_t rte_power_turbo_status; >> -rte_power_freq_change_t rte_power_freq_enable_turbo; >> -rte_power_freq_change_t rte_power_freq_disable_turbo; >> -rte_power_get_capabilities_t rte_power_get_capabilities; >> - >> -static void >> -reset_power_function_ptrs(void) >> +/* register the ops struct in rte_power_ops, return 0 on success. */ >> +int >> +rte_power_register_ops(const struct rte_power_ops *op) >> +{ >> +    struct rte_power_ops *ops; >> + >> +    if (op->env >= PM_ENV_MAX) { >> +        POWER_LOG(ERR, "Unsupported power management environment\n"); >> +        return -EINVAL; >> +    } >> + >> +    if (op->status != 0) { >> +        POWER_LOG(ERR, "Power management env[%d] ops registered >> already\n", >> +            op->env); >> +        return -EINVAL; >> +    } >> + >> +    if (!op->init || !op->exit || !op->check_env_support || >> +        !op->get_avail_freqs || !op->get_freq || !op->set_freq || >> +        !op->freq_up || !op->freq_down || !op->freq_max || >> +        !op->freq_min || !op->turbo_status || !op->enable_turbo || >> +        !op->disable_turbo || !op->get_caps) { >> +        POWER_LOG(ERR, "Missing callbacks while registering power >> ops\n"); >> +        return -EINVAL; >> +    } >> + >> +    ops = &rte_power_ops[op->env]; > It is better to use a global linked list instead of an array. > And we should extract a list structure including this ops structure > and this ops's owner. >> +    ops->env = op->env; >> +    ops->init = op->init; >> +    ops->exit = op->exit; >> +    ops->check_env_support = op->check_env_support; >> +    ops->get_avail_freqs = op->get_avail_freqs; >> +    ops->get_freq = op->get_freq; >> +    ops->set_freq = op->set_freq; >> +    ops->freq_up = op->freq_up; >> +    ops->freq_down = op->freq_down; >> +    ops->freq_max = op->freq_max; >> +    ops->freq_min = op->freq_min; >> +    ops->turbo_status = op->turbo_status; >> +    ops->enable_turbo = op->enable_turbo; >> +    ops->disable_turbo = op->disable_turbo; > *ops = *op? >> +    ops->status = 1; /* registered */ > status --> registered? > But if use ops linked list, this flag also can be removed. >> + >> +    return 0; >> +} >> + >> +struct rte_power_ops * >> +rte_power_get_ops(int ops_index) > AFAICS, there is only one cpufreq driver on one platform and just have > one power_cpufreq_ops to use for user. > We don't need user to get other power ops, and user just want to know > the power ops using currently, right? > So using 'index' toget this ops is not good. >>   { >> -    rte_power_freqs  = NULL; >> -    rte_power_get_freq = NULL; >> -    rte_power_set_freq = NULL; >> -    rte_power_freq_up = NULL; >> -    rte_power_freq_down = NULL; >> -    rte_power_freq_max = NULL; >> -    rte_power_freq_min = NULL; >> -    rte_power_turbo_status = NULL; >> -    rte_power_freq_enable_turbo = NULL; >> -    rte_power_freq_disable_turbo = NULL; >> -    rte_power_get_capabilities = NULL; >> +    RTE_VERIFY((ops_index >= PM_ENV_NOT_SET) && (ops_index < >> PM_ENV_MAX)); >> +    RTE_VERIFY(rte_power_ops[ops_index].status != 0); >> + >> +    return &rte_power_ops[ops_index]; >>   } >>     int >>   rte_power_check_env_supported(enum power_management_env env) >>   { >> -    switch (env) { >> -    case PM_ENV_ACPI_CPUFREQ: >> -        return power_acpi_cpufreq_check_supported(); >> -    case PM_ENV_PSTATE_CPUFREQ: >> -        return power_pstate_cpufreq_check_supported(); >> -    case PM_ENV_KVM_VM: >> -        return power_kvm_vm_check_supported(); >> -    case PM_ENV_CPPC_CPUFREQ: >> -        return power_cppc_cpufreq_check_supported(); >> -    case PM_ENV_AMD_PSTATE_CPUFREQ: >> -        return power_amd_pstate_cpufreq_check_supported(); >> -    default: >> -        rte_errno = EINVAL; >> -        return -1; >> +    struct rte_power_ops *ops; >> + >> +    if ((env > PM_ENV_NOT_SET) && (env < PM_ENV_MAX)) { >> +        ops = rte_power_get_ops(env); >> +        return ops->check_env_support(); >>       } >> + >> +    rte_errno = EINVAL; >> +    return -1; >>   } >>     int >> @@ -80,80 +96,26 @@ rte_power_set_env(enum power_management_env env) >>       } >>         int ret = 0; >> +    struct rte_power_ops *ops; >> + >> +    if ((env == PM_ENV_NOT_SET) || (env >= PM_ENV_MAX)) { >> +        POWER_LOG(ERR, "Invalid Power Management Environment(%d)" >> +                " set\n", env); >> +        ret = -1; >> +    } > <...> >> +    ops = rte_power_get_ops(env); > To find the target ops from the global list according to the env? >> +    if (ops->status == 0) { >> +        POWER_LOG(ERR, WER, >> +            "Power Management Environment(%d) not" >> +            " registered\n", env); >>           ret = -1; >>       } >>         if (ret == 0) >>           global_default_env = env; > It is more convenient to use a global variable to point to the default > power_cpufreq ops or its list node. >> -    else { >> +    else >>           global_default_env = PM_ENV_NOT_SET; >> -        reset_power_function_ptrs(); >> -    } >>         rte_spinlock_unlock(&global_env_cfg_lock); >>       return ret; >> @@ -164,7 +126,6 @@ rte_power_unset_env(void) >>   { >>       rte_spinlock_lock(&global_env_cfg_lock); >>       global_default_env = PM_ENV_NOT_SET; >> -    reset_power_function_ptrs(); >>       rte_spinlock_unlock(&global_env_cfg_lock); >>   } >>   @@ -177,59 +138,76 @@ int >>   rte_power_init(unsigned int lcore_id) >>   { >>       int ret = -1; >> +    struct rte_power_ops *ops; >>   -    switch (global_default_env) { >> -    case PM_ENV_ACPI_CPUFREQ: >> -        return power_acpi_cpufreq_init(lcore_id); >> -    case PM_ENV_KVM_VM: >> -        return power_kvm_vm_init(lcore_id); >> -    case PM_ENV_PSTATE_CPUFREQ: >> -        return power_pstate_cpufreq_init(lcore_id); >> -    case PM_ENV_CPPC_CPUFREQ: >> -        return power_cppc_cpufreq_init(lcore_id); >> -    case PM_ENV_AMD_PSTATE_CPUFREQ: >> -        return power_amd_pstate_cpufreq_init(lcore_id); >> -    default: >> -        POWER_LOG(INFO, "Env isn't set yet!"); >> +    if (global_default_env != PM_ENV_NOT_SET) { >> +        ops = &rte_power_ops[global_default_env]; >> +        if (!ops->status) { >> +            POWER_LOG(ERR, "Power management env[%d] not" >> +                " supported\n", global_default_env); >> +            goto out; >> +        } >> +        return ops->init(lcore_id); >>       } >> +    POWER_LOG(INFO, POWER, "Env isn't set yet!\n"); >>         /* Auto detect Environment */ >> -    POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq power >> management..."); >> -    ret = power_acpi_cpufreq_init(lcore_id); >> -    if (ret == 0) { >> -        rte_power_set_env(PM_ENV_ACPI_CPUFREQ); >> -        goto out; >> +    POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq" >> +            " power management...\n"); >> +    ops = &rte_power_ops[PM_ENV_ACPI_CPUFREQ]; >> +    if (ops->status) { >> +        ret = ops->init(lcore_id); >> +        if (ret == 0) { >> +            rte_power_set_env(PM_ENV_ACPI_CPUFREQ); >> +            goto out; >> +        } >>       } >>   -    POWER_LOG(INFO, "Attempting to initialise PSTAT power >> management..."); >> -    ret = power_pstate_cpufreq_init(lcore_id); >> -    if (ret == 0) { >> -        rte_power_set_env(PM_ENV_PSTATE_CPUFREQ); >> -        goto out; >> +    POWER_LOG(INFO, "Attempting to initialise PSTAT" >> +            " power management...\n"); >> +    ops = &rte_power_ops[PM_ENV_PSTATE_CPUFREQ]; >> +    if (ops->status) { >> +        ret = ops->init(lcore_id); >> +        if (ret == 0) { >> +            rte_power_set_env(PM_ENV_PSTATE_CPUFREQ); >> +            goto out; >> +        } >>       } >>   -    POWER_LOG(INFO, "Attempting to initialise AMD PSTATE power >> management..."); >> -    ret = power_amd_pstate_cpufreq_init(lcore_id); >> -    if (ret == 0) { >> -        rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ); >> -        goto out; >> +    POWER_LOG(INFO,    "Attempting to initialise AMD PSTATE" >> +            " power management...\n"); >> +    ops = &rte_power_ops[PM_ENV_AMD_PSTATE_CPUFREQ]; >> +    if (ops->status) { >> +        ret = ops->init(lcore_id); >> +        if (ret == 0) { >> +            rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ); >> +            goto out; >> +        } >>       } >>   -    POWER_LOG(INFO, "Attempting to initialise CPPC power >> management..."); >> -    ret = power_cppc_cpufreq_init(lcore_id); >> -    if (ret == 0) { >> -        rte_power_set_env(PM_ENV_CPPC_CPUFREQ); >> -        goto out; >> +    POWER_LOG(INFO, "Attempting to initialise CPPC power" >> +            " management...\n"); >> +    ops = &rte_power_ops[PM_ENV_CPPC_CPUFREQ]; >> +    if (ops->status) { >> +        ret = ops->init(lcore_id); >> +        if (ret == 0) { >> +            rte_power_set_env(PM_ENV_CPPC_CPUFREQ); >> +            goto out; >> +        } >>       } >>   -    POWER_LOG(INFO, "Attempting to initialise VM power >> management..."); >> -    ret = power_kvm_vm_init(lcore_id); >> -    if (ret == 0) { >> -        rte_power_set_env(PM_ENV_KVM_VM); >> -        goto out; >> +    POWER_LOG(INFO, "Attempting to initialise VM power" >> +            " management...\n"); >> +    ops = &rte_power_ops[PM_ENV_KVM_VM]; >> +    if (ops->status) { >> +        ret = ops->init(lcore_id); >> +        if (ret == 0) { >> +            rte_power_set_env(PM_ENV_KVM_VM); >> +            goto out; >> +        } >>       } > If we use a linked list, above code can be simpled like this: > -> > for_each_power_cpufreq_ops(ops, ...) { >     ret = ops->init() >     if (ret) { >         .... >     } > } >> -    POWER_LOG(ERR, "Unable to set Power Management Environment for >> lcore " >> -            "%u", lcore_id); >> +    POWER_LOG(ERR, "Unable to set Power Management Environment" >> +            " for lcore %u\n", lcore_id); >>   out: >>       return ret; >>   } >> @@ -237,21 +215,14 @@ rte_power_init(unsigned int lcore_id) >>   int >>   rte_power_exit(unsigned int lcore_id) >>   { >> -    switch (global_default_env) { >> -    case PM_ENV_ACPI_CPUFREQ: >> -        return power_acpi_cpufreq_exit(lcore_id); >> -    case PM_ENV_KVM_VM: >> -        return power_kvm_vm_exit(lcore_id); >> -    case PM_ENV_PSTATE_CPUFREQ: >> -        return power_pstate_cpufreq_exit(lcore_id); >> -    case PM_ENV_CPPC_CPUFREQ: >> -        return power_cppc_cpufreq_exit(lcore_id); >> -    case PM_ENV_AMD_PSTATE_CPUFREQ: >> -        return power_amd_pstate_cpufreq_exit(lcore_id); >> -    default: >> -        POWER_LOG(ERR, "Environment has not been set, unable to exit >> gracefully"); >> +    struct rte_power_ops *ops; >>   +    if (global_default_env != PM_ENV_NOT_SET) { >> +        ops = &rte_power_ops[global_default_env]; >> +        return ops->exit(lcore_id); >>       } >> -    return -1; >> +    POWER_LOG(ERR, "Environment has not been set, unable " >> +            "to exit gracefully\n"); >>   +    return -1; >>   } >> diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h >> index 4fa4afe399..749bb823ab 100644 >> --- a/lib/power/rte_power.h >> +++ b/lib/power/rte_power.h >> @@ -1,5 +1,6 @@ >>   /* SPDX-License-Identifier: BSD-3-Clause >>    * Copyright(c) 2010-2014 Intel Corporation >> + * Copyright(c) 2024 AMD Limited >>    */ >>     #ifndef _RTE_POWER_H >> @@ -21,7 +22,7 @@ extern "C" { >>   /* Power Management Environment State */ >>   enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, >> PM_ENV_KVM_VM, >>           PM_ENV_PSTATE_CPUFREQ, PM_ENV_CPPC_CPUFREQ, >> -        PM_ENV_AMD_PSTATE_CPUFREQ}; >> +        PM_ENV_AMD_PSTATE_CPUFREQ, PM_ENV_MAX}; > "enum power_management_env" is not good. may be like "enum > power_cpufreq_driver_type"? > In previous linked list structure to be defined, may be directly use a > string name instead of a fixed enum is better. > Becuase the new "PM_ENV_MAX" will  lead to break ABI when add a new > cpufreq driver. >>     /** >>    * Check if a specific power management environment type is >> supported on a >> @@ -66,6 +67,97 @@ void rte_power_unset_env(void); >>    */ >>   enum power_management_env rte_power_get_env(void); >>   +typedef int (*rte_power_cpufreq_init_t)(unsigned int lcore_id); >> +typedef int (*rte_power_cpufreq_exit_t)(unsigned int lcore_id); >> +typedef int (*rte_power_check_env_support_t)(void); >> + >> +typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, >> uint32_t *freqs, >> +                    uint32_t num); >> +typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id); >> +typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t >> index); >> +typedef int (*rte_power_freq_change_t)(unsigned int lcore_id); >> + >> +/** >> + * Function pointer definition for generic frequency change >> functions. Review >> + * each environments specific documentation for usage. >> + * >> + * @param lcore_id >> + *  lcore id. >> + * >> + * @return >> + *  - 1 on success with frequency changed. >> + *  - 0 on success without frequency changed. >> + *  - Negative on error. >> + */ >> + >> +/** >> + * Power capabilities summary. >> + */ >> +struct rte_power_core_capabilities { >> +    union { >> +        uint64_t capabilities; >> +        struct { >> +            uint64_t turbo:1;       /**< Turbo can be enabled. */ >> +            uint64_t priority:1;    /**< SST-BF high freq core */ >> +        }; >> +    }; >> +}; >> + >> +typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id, >> +                struct rte_power_core_capabilities *caps); >> + >> +/** Structure defining core power operations structure */ >> +struct rte_power_ops { >> +uint8_t status;                         /**< ops register status. */ >> +    enum power_management_env env;          /**< power mgmt env. */ >> +    rte_power_cpufreq_init_t init;    /**< Initialize power >> management. */ >> +    rte_power_cpufreq_exit_t exit;    /**< Exit power management. */ >> +    rte_power_check_env_support_t check_env_support; /**< verify env >> is supported. */ >> +    rte_power_freqs_t get_avail_freqs; /**< Get the available >> frequencies. */ >> +    rte_power_get_freq_t get_freq; /**< Get frequency index. */ >> +    rte_power_set_freq_t set_freq; /**< Set frequency index. */ >> +    rte_power_freq_change_t freq_up;   /**< Scale up frequency. */ >> +    rte_power_freq_change_t freq_down; /**< Scale down frequency. */ >> +    rte_power_freq_change_t freq_max;  /**< Scale up frequency to >> highest. */ >> +    rte_power_freq_change_t freq_min;  /**< Scale up frequency to >> lowest. */ >> +    rte_power_freq_change_t turbo_status; /**< Get Turbo status. */ >> +    rte_power_freq_change_t enable_turbo; /**< Enable Turbo. */ >> +    rte_power_freq_change_t disable_turbo; /**< Disable Turbo. */ >> +    rte_power_get_capabilities_t get_caps; /**< power capabilities. */ >> +} __rte_cache_aligned; > Suggest that fix this sturcture, like: > struct rte_power_cpufreq_list { >     char name[];   // like "cppc_cpufreq", "pstate_cpufreq" >     struct rte_power_cpufreq *ops; >     struct rte_power_cpufreq_list *node; > } >> + >> +/** >> + * Register power cpu frequency operations. >> + * >> + * @param ops >> + *   Pointer to an ops structure to register. >> + * @return >> + *   - >=0: Success; return the index of the ops struct in the table. >> + *   - -EINVAL - error while registering ops struct. >> + */ >> +__rte_internal >> +int rte_power_register_ops(const struct rte_power_ops *ops); >> + >> +/** >> + * Macro to statically register the ops of a cpufreq driver. >> + */ >> +#define RTE_POWER_REGISTER_OPS(ops)        \ >> +    (RTE_INIT(power_hdlr_init_##ops)    \ >> +    {                    \ >> +        rte_power_register_ops(&ops);    \ >> +    }) >> + >> +/** >> + * @internal Get the power ops struct from its index. >> + * >> + * @param ops_index >> + *   The index of the ops struct in the ops struct table. >> + * @return >> + *   The pointer to the ops struct in the table if registered. >> + */ >> +struct rte_power_ops * >> +rte_power_get_ops(int ops_index); >> + >>   /** >>    * Initialize power management for a specific lcore. If >> rte_power_set_env() has >>    * not been called then an auto-detect of the environment will >> start and >> @@ -108,10 +200,14 @@ int rte_power_exit(unsigned int lcore_id); >>    * @return >>    *  The number of available frequencies. >>    */ >> -typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, >> uint32_t *freqs, >> -        uint32_t num); >> +static inline uint32_t >> +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n) >> +{ >> +    struct rte_power_ops *ops; >>   -extern rte_power_freqs_t rte_power_freqs; >> +    ops = rte_power_get_ops(rte_power_get_env()); >> +    return ops->get_avail_freqs(lcore_id, freqs, n); >> +} > nice. > <...>