Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC

[Sudeep Holla <sudeep.holla@arm.com>]

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.

> 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)

> 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.

> 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.

> 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.

> 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.

Commit 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for
the PCC Type 3 subtype") has the example in the commit message. IIRC,
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
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)

-- 
Regards,
Sudeep