Message-ID: <aec13381-e9be-4f3d-1834-52f32f9f8418@huawei.com>
Date:   Thu, 18 May 2023 16:24:36 +0800
From:   "lihuisong (C)" <lihuisong@...wei.com>
To:     Sudeep Holla <sudeep.holla@....com>
CC:     Arnd Bergmann <arnd@...db.de>,
        Bjorn Andersson <andersson@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Shawn Guo <shawnguo@...nel.org>,
        <linux-kernel@...r.kernel.org>, <soc@...nel.org>,
        <wanghuiqiang@...wei.com>, <tanxiaofei@...wei.com>,
        <liuyonglong@...wei.com>, <huangdaode@...wei.com>,
        <linux-acpi@...r.kernel.org>, Len Brown <lenb@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC


在 2023/5/17 21:16, Sudeep Holla 写道:
> On Wed, May 17, 2023 at 07:35:25PM +0800, lihuisong (C) wrote:
>> 在 2023/5/17 17:30, Sudeep Holla 写道:
>>> On Wed, May 17, 2023 at 03:16:12PM +0800, lihuisong (C) wrote:
>>>
>>> [...]
>>>
>>>> No. I want to use this flag to make compability between different platforms.
>>>> This driver only use PCC OpRegion to access to the channel if platform
>>>> support use PCC OpRegion.
>>> What do you mean by that ? It is not correct. If there is a PCC Opregion,
>>> then you need to make it work with drivers/acpi/acpi_pcc.c
>>>
>>> You need to have all the other details in the firmware(ASL). By looking
>>> at the driver, it has no connection to PCC Opregion IMO unless I am missing
>>> something.
>> Driver just needs to call these APIs, such as acpi_evaluate_integer(), if
>> want to use PCC OpRegion.
> OK, please provide examples. I am definitely lost as it doesn't match with
> my understanding of how PCC Opregions are/can be used.
>
>> I know that. I have tested PCC OpRegion before.
> Cool, examples please.
>
>> You've completely misunderstood what I said.😅
>>
> Hmm, may be but I need examples.
As you said below, the driver works just for PCC not PCC Opregion for now.
not sure if we need to discuss how PCC Opregion is used here.
>
>> I mean that this driver plans to support both PCC and PCC OpRegion.
>> For example,
>> Platform A: this driver use PCC (as the current implementation)
> Good, then just keep what it needs in the implementation nothing more
> until you add support for something you have described below(not that
> I agree, just want you to make progress here based on what is actually
> required today)
Agreed.
>
>> Platform B: this driver use PCC OpRegion (Currently, this patch does not
>> implement it, but it may be available in the future.)
> Then let us discuss that in the future, don't add unnecessary complexity
> for some future use case today. You can always add it when you introduce
> that feature or support in the future.
Yes. We just need to focus on the current.
If there are any usage problems with PCC OpRegion in the future, we can 
discuss that later.

My original full scheme is as follows:
-->
dev_flags = get_device_flags();  // to know if use PCC OpRegion
if (USE_PCC_OPREGION_B in dev_flags is 0) {
     chan_id = get_pcc_chan_id();
     init_mbox_client();
     pcc_mbox_request_channel(cl, chan_id)
} else {
     /* we need to return unsupport now because of no this feature in 
this driver. */
     do_nothing();
}

void get_some_info(...) {
     if (USE_PCC_OPREGION_B in dev_flags is 0)
         pcc_cmd_send();  // use PCC to communicate with Platform
     else
         acpi_evaluate_object(); // will be used in future.
}

As described in the pseudocode above,
it is necessary to put "dev_flags" in this current driver first in case of
the version driver runs on the platform which just use PCC Opregion.
>
>> Note:
>> This driver selects only one of them (PCC and PCC OpRegion) to communicate
>> with firmware on one platform.
> Let us keep it simple(KISS). The driver works just for PCC not PCC Opregion
> for now.
ok.
>
>> We use one bit in device-flags to know which one this driver will use.
>>
> NACK again just to re-iterate my point if you have not yet accepted that
> fact.
Above is our plan. Do you still think we shouldn't add this device-flags?
please let me know.
>> I'm not sure if you can understand what I mean by saing that.
>> If you're not confused about this now, can you reply to my last email
>> again?😁
>>
> The example you had IIRC is use of System Memory Opregion to demonstrate
> some _DSM. That has nothing to do with PCC Opregion.
Yes, it doesn't matter.
I just want to have a way to get device-flags which contains many 
bits(every bits can be used to as one feature for expanding), rigtht?
>
> Commit 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for
> the PCC Type 3 subtype") has the example in the commit message. IIRC,
Your example is very useful to the user.
> you have even fixed couple of bugs in that driver. That is the reason
> why I don't understand how you think this driver and that can or must
Understand you, Sudeep.
At that time, I tested it by a simple demo driver on the platform 
supported type3.

This driver will support multiple platforms.
On some platforms, we can only use PCC with polling way.
And we will add PCC Opregion way for others platforms.
What's more, every platform just use one of them(PCC and PCC Opregion).
> work together. At least I fail to see how ATM(examples please, by that
> I mean ASL snippet for PCC vs PCC Opregion usage to work with this driver)
ok!
For PCC, ASL snippet is little.
I will add ASL snippet when this driver addes PCC Opregion way.
>

Powered by blists - more mailing lists