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: <7785f9c3-93f2-7e7b-9c8d-8f86a0c9a76c@amd.com>
Date:   Fri, 13 Jan 2023 12:01:01 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 1/6] acpi: cppc: Add min and max perf reg writing
 support
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Rafael J Wysocki <rafael@kernel.org>,
        Huang Rui <ray.huang@amd.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Viresh Kumar <viresh.kumar@linaro.org>, Perry.Yuan@amd.com,
        Ananth Narayan <ananth.narayan@amd.com>, gautham.shenoy@amd.com
Cc:     linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, Bagas Sanjaya <bagasdotme@gmail.com>,
        santosh.shukla@amd.com
References: <20230113052141.2874296-1-wyes.karny@amd.com>
 <20230113052141.2874296-2-wyes.karny@amd.com>
 <85925bce-f30a-b2df-309f-bd4b510a117c@amd.com>
From:   Wyes Karny <wyes.karny@amd.com>
In-Reply-To: <85925bce-f30a-b2df-309f-bd4b510a117c@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YUJLYUZJQmtVRjE5eTJ1WGpFVithS21YRFdPbHBPT0NDazU3L1Z4cmZDQU1R?=
 =?utf-8?B?S2luV3F3Z0NXaGkrM1REMGc2MWREWFJ0RkpDRmwvaGIzc2ZhNExYWkhkVTVU?=
 =?utf-8?B?VExMa0FtUVJ1UVBSQk11UkFWRjh5L3MxVjJiL3ROaEdLSS8yRXdFUVpXaTQ2?=
 =?utf-8?B?U2FJOU5wR0dXakVmUHlsZzdjQXpSNXZpaVNZQ0VrMTY3RUdXSldIS0ErQzk3?=
 =?utf-8?B?Ym1oWmIvdzdZWnpWem9jWk92ZEorUngrWWlGYmhIMm9zb3BFMlRJazFmTGx0?=
 =?utf-8?B?em9zR3FyRFVlb3hGT2d3UGd1SnRVc2lTN1NxUHpWWDQ0ZjdobHU2QlR2bWFS?=
 =?utf-8?B?Y0pTcjRKbVJFdUJwV0RqNCtEaCttcEhnelloT3E2NmZtZFc2TTk2RW5UNDRs?=
 =?utf-8?B?Z2NjVGhvaHNGUlRHZVNyc0dDOERuMlE3RHM4bnFIcmV2Z2oxcThVVXJXUk1U?=
 =?utf-8?B?QWkzcEVvVVNPdDRZcjRyUWo5OUdwN1JnQmxVWUc1eGZoNjNMOW8weitrQlJj?=
 =?utf-8?B?RjY5ZjJnVTVvcVI5K09zMDZpd0o4VkQ4TGw3ajRJK3BMS3lBRFp0dWkzbGNF?=
 =?utf-8?B?OFNBcjk5VXgxcW5yMkJnU2tNcjZOL1g4b1MyWTZFQkprcjVNTXVKaWlpTGxY?=
 =?utf-8?B?blVBTGpUWVhRaFpDRHZOVXFnTnYzUlhTaFRjRTBjdUJMVDJreTVLK0Q1VDNo?=
 =?utf-8?B?OFp3NmdlVmhUbU9DVnYvdUVXNkNlcDZBc0YvREZMWEVJQjNPRE5aQWh2NFor?=
 =?utf-8?B?MTJqaVZsQ1ZuRkpHdDVIK3MzbTZub3Q2bmdGZWNSYlk2YmNEUGI3c1RjOTBW?=
 =?utf-8?B?MjcvMUplYXZCa2NzOG1CY3ppRExWK0RMaGpPRS9vY1YrUTNIZW9uWFE4eklY?=
 =?utf-8?B?SnZBS1owNFdBK1pvbE43cGxaaFlGanlYRHl4RTRDWHhmajc4VUxRUTBZWm01?=
 =?utf-8?B?WTdWVytad0RYU3JNVStGUkV0dmQ5TFlIOHRlV1hPakJQUUtkcVkrd0M3aFlV?=
 =?utf-8?B?VjVxbEE2Wm8wKy9VUnZsbldoYjNJN1NKUUt3NDQ5N0ZGRzg1bGdBS1VEOU1G?=
 =?utf-8?B?YTNXVWpMM1VwVHpDRHdWejMxbWU5L1VVUThPbmxCNHp2aGxiNzJwbWxrVkR4?=
 =?utf-8?B?SWRwUDlxY0R4OFh2U3I1S2J6cldaaytGMWtCSTJzSjJFTS9ybzNKb3dOSm00?=
 =?utf-8?B?U3hITVdiMTdJSDRGeEY4NW5ubVBzZHRudkUzdWJnQjRjTWIvNklFUFQrc0tC?=
 =?utf-8?B?TWx1TXErRzhHVmJNeWc1ekRKaG80Tk9wTUZuSkdBNDVKVkt5VFNJckxER1pE?=
 =?utf-8?B?UDMvRkwvbEdRYlhKVG5Qc0xrUzFYUS9XS1FkK1Vmczh6REJLSUNVbC9ZNUtt?=
 =?utf-8?B?QmYxNDVhTE0vV1lkUllYRlpZbWlGM3lGRWZVY2NCcUl3K0o5MGsrc2paTkVy?=
 =?utf-8?B?c1NlcGhDTkpiTTRsc1JsRXlSUVV2QXJhUGMwSkgyZEpqZ3huRklYL0NEZHVD?=
 =?utf-8?B?T3JjYUMxTGl3N3VyaGxnTG9wZmtQNnVJYzVUOTN2aU5KTGxsTFJ0K3lENEJY?=
 =?utf-8?B?ME1NZ0FVUllsMDl6eERhSXlWVHlUWkJ2MGhpRzJzV1ROU2JadHdUaWU4Q29R?=
 =?utf-8?B?aitVUVRoRVNJMlJtRS94UHBqaDI3WTE4K1VDakJEZkZaelN6Nk5WVGZhTDJo?=
 =?utf-8?B?NGpVT1B2VXYzYkpIVVJYdzA0dlUvSFFqVUlrWDdjWSt6SHJqNEVFNWtMOFhz?=
 =?utf-8?B?NmJpbmlWRHJmS040dVhKR01rMlJzUk1yQ1RRbzYzRjZJak5TRjRoMFhIa0lO?=
 =?utf-8?B?L3dtZjVtQUdqQVB6TGJmblhxMkk5QmxNTGpOak9ybWJuR0RxVDBWVkFoQ1l6?=
 =?utf-8?B?VzlyTUp0MnRqUnQ4K2g5QXNFUUdVL2xGVEVtSmJKZ1RpaG9KdE02Mlc0dEpB?=
 =?utf-8?B?MjhTRmg4aGxvUjlSM3l0NHNzYkhoTUF1L05DRDJBWjY3V1I4RUQvUlVwSEYw?=
 =?utf-8?B?dTVoc0lzNVRJMTVqbVpPaVJwdzQzcXVlSGMyUWpKY0kzVXF4TXVmWlVMU3pm?=
 =?utf-8?B?U0h2RituditnSThoOXpvMGdYVndKU3p3NnJBbmc4emY1YzlucmtUNmg3WFhm?=
 =?utf-8?Q?uITtKdz7hb147GBp/4ghaIPSa?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 03b66664-542f-425f-65d1-08daf52fc4cd
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3876.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jan 2023 06:31:14.5721
 (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: eWVEWP7Et8AUemeW2jZYgoKYd7C25K1D+q7Rlui8DZK1nk5fMFuNQpDSDcvJp23vCnkURhoPV/H/+paZbvKsNw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4871
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,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

Hi Mario,

On 1/13/2023 11:07 AM, Mario Limonciello wrote:
> On 1/12/23 23:21, Wyes Karny wrote:
>> Currently writing of min and max perf register is deferred in
>> cppc_set_perf function. In CPPC guided mode, these registers needed to
>> be written to guide PMFW about min and max perf levels. Add this support
> 
> This is generic code, so  I think rather than PMFW you should just say "the platform".
> 
>> to make guided mode work properly on shared memory systems.
> 
> on AMD shared memory systems.
> 
>>
>> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> 
> With the commit message cleaned up:
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Will do the cleanup part. Thanks for reviewing!

>> ---
>>   drivers/acpi/cppc_acpi.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 02d83c807271..c936ff503965 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1487,7 +1487,7 @@ EXPORT_SYMBOL_GPL(cppc_set_enable);
>>   int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>   {
>>       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> -    struct cpc_register_resource *desired_reg;
>> +    struct cpc_register_resource *desired_reg, *min_perf_reg, *max_perf_reg;
>>       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>       struct cppc_pcc_data *pcc_ss_data = NULL;
>>       int ret = 0;
>> @@ -1498,6 +1498,8 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>       }
>>         desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
>> +    min_perf_reg = &cpc_desc->cpc_regs[MIN_PERF];
>> +    max_perf_reg = &cpc_desc->cpc_regs[MAX_PERF];
>>         /*
>>        * This is Phase-I where we want to write to CPC registers
>> @@ -1506,7 +1508,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>        * Since read_lock can be acquired by multiple CPUs simultaneously we
>>        * achieve that goal here
>>        */
>> -    if (CPC_IN_PCC(desired_reg)) {
>> +    if (CPC_IN_PCC(desired_reg) || CPC_IN_PCC(min_perf_reg) || CPC_IN_PCC(max_perf_reg)) {
>>           if (pcc_ss_id < 0) {
>>               pr_debug("Invalid pcc_ss_id\n");
>>               return -ENODEV;
>> @@ -1529,13 +1531,11 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>           cpc_desc->write_cmd_status = 0;
>>       }
>>   -    /*
>> -     * Skip writing MIN/MAX until Linux knows how to come up with
>> -     * useful values.
>> -     */
>>       cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
>> +    cpc_write(cpu, min_perf_reg, perf_ctrls->min_perf);
>> +    cpc_write(cpu, max_perf_reg, perf_ctrls->max_perf);
>>   -    if (CPC_IN_PCC(desired_reg))
>> +    if (CPC_IN_PCC(desired_reg) || CPC_IN_PCC(min_perf_reg) || CPC_IN_PCC(max_perf_reg))
>>           up_read(&pcc_ss_data->pcc_lock);    /* END Phase-I */
>>       /*
>>        * This is Phase-II where we transfer the ownership of PCC to Platform
>> @@ -1583,7 +1583,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>        * case during a CMD_READ and if there are pending writes it delivers
>>        * the write command before servicing the read command
>>        */
>> -    if (CPC_IN_PCC(desired_reg)) {
>> +    if (CPC_IN_PCC(desired_reg) || CPC_IN_PCC(min_perf_reg) || CPC_IN_PCC(max_perf_reg)) {
>>           if (down_write_trylock(&pcc_ss_data->pcc_lock)) {/* BEGIN Phase-II */
>>               /* Update only if there are pending write commands */
>>               if (pcc_ss_data->pending_pcc_write_cmd)
> 

-- 
Thanks & Regards,
Wyes