Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bce86f79-9757-530b-b34e-1d2a71281e75@amd.com>
Date:   Fri, 23 Dec 2022 15:15:25 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working
 mode selection in amd_pstate_param()
Content-Language: en-US
To:     Perry Yuan <perry.yuan@amd.com>, rafael.j.wysocki@intel.com,
        Mario.Limonciello@amd.com, ray.huang@amd.com,
        viresh.kumar@linaro.org
Cc:     Deepak.Sharma@amd.com, Nathan.Fontenot@amd.com,
        Alexander.Deucher@amd.com, Shimmer.Huang@amd.com,
        Xiaojian.Du@amd.com, Li.Meng@amd.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221219064042.661122-1-perry.yuan@amd.com>
 <20221219064042.661122-6-perry.yuan@amd.com>
From:   Wyes Karny <wyes.karny@amd.com>
In-Reply-To: <20221219064042.661122-6-perry.yuan@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZjdvcTBVa0srYzNIUXRCU2hUc252dXBOaWRSV3FCSFJaeVpvOC9qVXpwQXZu?=
 =?utf-8?B?eXBvSVZidFg1cWM5Zm0rd1cvUit3L09jc3ltWXdFKzhubUdiRzlIOStmQito?=
 =?utf-8?B?RWFReFQwc3pRbTcrMnFLZENucjJCMWU3K0dBS3craUo5VExwUGZmcnc2U1RL?=
 =?utf-8?B?UFNtWTAvS0d6TFg5NnFPaWg3V1ZwVWh1KzJxZjhPTTd2amt2TDNneTFhQXRS?=
 =?utf-8?B?c3Z5L2t5aytEMDBxTTI1T2JYQXB2VUp6bU5lUUdaWEdOUE94eEJlTzNRSXRP?=
 =?utf-8?B?UlBKSXJwZ2lqc2NmS3RmOHF6aS83L2FVZXZlNkRiNEdmTVkrTFc4Y2dhemFB?=
 =?utf-8?B?M3RyT2FBMHpLcjE1bWczWFhnVUgyQnd4ejh6OWNPcmFqeFIyRmhVZ3BubkxJ?=
 =?utf-8?B?REdFZktOQzJlTjY3dm54Sm02Q1AwV2pwVlF6MitVL2M4eHdzY1VQQ2dMRDE0?=
 =?utf-8?B?NWk5dTdkRVVuZ1BMai9veWxicG5PNXhIdXJNamN5eUgvZEMwUWVOVTdkSm5S?=
 =?utf-8?B?WGVLZDJkU1cweTk5U0J3S2p2eDg3Y0NRaXFjak9OUDQ5M1JDY2lLLzY4dnZw?=
 =?utf-8?B?TWtVS1dKRG9PNmFDd3owRkNUQ0o5cldzUGpVZEVML2NjWlBVMjgxL3BmKytO?=
 =?utf-8?B?T2lrR0VwWWREcU16RGg0Uk8vN2FvOWU0SjJPRXMrUjJoVXVGa2NrZjRWVitN?=
 =?utf-8?B?WGNZcjNPUUZreWJDK0JQV1ZmMTdrY0pRUzBsTkwzTS9ncm44azNoMXZub3JN?=
 =?utf-8?B?cUhGSGt6eWc5aTVLT1gyTy9iSnExdlU3cWdQUkZER2N5WkRUSzFnMkw2Zm5Q?=
 =?utf-8?B?NUxtei8vY1ZXMEgrTzF1ZTk1N2crbXdnNjdOOWF4WnNHdUpHM3krRGNtMERE?=
 =?utf-8?B?QmZyYkM2cGlyMm40Z1BDM3E4RDlCUnkzMUpZOFFZMlQzQkkwL3V5WjlhTm1l?=
 =?utf-8?B?Tk54ems2Nm1NT3F6bitieCtKZEhUbUhKekVnWVlpRlAweE52d000N3B3MjIx?=
 =?utf-8?B?MkdRRCs1dmxuWmtSa2Vuc29xTkpUMVpNMVo4WGMvc2lsb3QxM0VEZVllaFU1?=
 =?utf-8?B?UlZwdU5VcTkrN2dhNmxzWHpsN3F4UUl6eWtWNE5tNVlNYUVocFZrRG14dUlw?=
 =?utf-8?B?U0JzR0pxMVc3VWsyTjFDbnVrMG9lYlZqR1NkN1JyWFdzbGVlN3lQUk5HVUZm?=
 =?utf-8?B?WXYyRURDeFF6d0M1dythOW9ubUdPaGFhUjdDMzJpaUd0RmltcDIyWVZBQWlj?=
 =?utf-8?B?cHRlNDQ1ZkM1MC9YdU41TUxscnNUejQzbHFGSVVSTlVhZ3ovNzF3V05meTRw?=
 =?utf-8?B?ZXJ3bzVtTlQ3RUJGZko2MGhId2hEQWJ2SXJINUJZcUg3SFlwbnczS0ljZ1hC?=
 =?utf-8?B?cVRNS1RoYStVYXB6M1RTUUo3bGJML1NjOVh1dXRDV1lnaGcxMmhkL0ZRNDZN?=
 =?utf-8?B?WG1ZZ0RkV3crU3dhTkorWFdqTERzSTBsSjdHWCsyVHhzS3ZhKzBZdzN0OE9H?=
 =?utf-8?B?R3Z2dFlxMXhNdGtGVjNjZEhnUW1tQ1JFalltYjFEcXBBMitnMTk5N2ZIZTRT?=
 =?utf-8?B?ZGM5cGtSRzFuWXpmVnptNngxYURQSXV2YXFZVXNSS3NyREZkYjUrOEVyMkJI?=
 =?utf-8?B?N2M3ODVOZjVFU3d2MVpJeTB3ZEE1cTRZaU82aTNlR29lMXF4bkdWa1Nxa1k3?=
 =?utf-8?B?ekhQbjZRRzhraDVCVEdJcllCUm9Xd1Y3RjJuK3MwYzRZSjY3d2g5aFRNaWJO?=
 =?utf-8?B?T3BUZitXVmVBdks2aHE3NUhUTjFqTG50bHJiM3QweUl2OWNxQTVYdVRaaFlK?=
 =?utf-8?B?QVFIQURkUUVyazRWbDhqdmw4TDhGY21ZSlBhV2Z0R0VWNGIxam5TQ0RqdzJY?=
 =?utf-8?B?L0pGZ29mQnZBVHlzYkMxdjIySzNZSm53QVM0L1loL0E2Wk1xWXZrcHpHTE1a?=
 =?utf-8?B?OUhFSmRGcVFPcnUxakRvZVR3dHhmRTFyQmxGdG80SWR3UkFpV3pLMy9FYTc2?=
 =?utf-8?B?YzR4ZUQyUGFBVGd5RGhwZzd3Uk1HUDZmVkIwbjJKa04wT2FlWnlTc2pwOFha?=
 =?utf-8?B?bGszYjlSNnBzRkNBVkdFblQyV2l5R3hEeFVqbDgwNTNnTmJUTnV4Wk5LS0VD?=
 =?utf-8?Q?Nh66oEm8f+aCRig5AvuSiskyq?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c3d25ea5-6f62-49ac-4b3c-08dae4ca7171
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3876.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Dec 2022 09:45:36.7873
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KQ5Jw8zkwOzrAE2xtok1c7dO5rPFoSa/hzxnMY+TcygF91aA6w4zxmLDb/5JzLElv8h5evX2itG2EHyUxGuvYA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4238
X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,
        SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 12/19/2022 12:10 PM, Perry Yuan wrote:
--------------------->8-----------------------------
>  
> +/**
> + * enum amd_pstate_mode - driver working mode of amd pstate
> + */
> +
> +enum amd_pstate_mode {
> +	/** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> +	AMD_PSTATE_DISABLE = 0,
> +
> +	/** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
> +	AMD_PSTATE_PASSIVE = 1,
> +
> +	/** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> +	AMD_PSTATE_ACTIVE = 2,
> +
> +	/** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> +	AMD_PSTATE_GUIDE = 3,
> +
> +	/** @AMD_PSTATE_MAX */
> +	AMD_PSTATE_MAX = 4,
> +};

IMO the above enum is self explanatory we don't need to annotate.

what about below?

/**
 * enum amd_pstate_mode - driver working mode
 * All supported modes are explained in kernel-parameters.txt
 */
enum amd_pstate_mode {
	AMD_PSTATE_DISABLE = 0,
	AMD_PSTATE_PASSIVE,
	AMD_PSTATE_ACTIVE,
	AMD_PSTATE_MAX,
};

Plz remove GUIDED mode here because it allows user to pass "amd_pstate=guided"
in kernel cmdline. Therefore it breaks the driver flow without guided patches.
I can update the enum in my guided patch.

> +
> +static const char * const amd_pstate_mode_string[] = {
> +	[AMD_PSTATE_DISABLE]     = "disable",
> +	[AMD_PSTATE_PASSIVE]     = "passive",
> +	[AMD_PSTATE_ACTIVE]      = "active",
> +	[AMD_PSTATE_GUIDE]      = "guide",
> +	NULL,
> +};
> +
>  #endif /* _LINUX_AMD_PSTATE_H */

-- 
Thanks & Regards,
Wyes