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

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

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 must select one of them (PCC and PCC OpRegion) to communicate with
> firmware on one platform.

No for reasons mentioned above. PCC Opregion support in the kernel will
be minimal and already there. Fix that if it is not working. If you are
attempting to do something with PCC Opregion in this driver, it is just
wrong and I will NACK it if I see anything around that.

> > If so that may not work as the current implementation of PCC Opregion
> > assumes the exclusive access to the channel. Since it is initialised
> > quite early, Opregion must succeed to get the mbox channel acquired and
> > this driver must fail if they are sharing the channel. Making the sharing
> > across firmware and this driver may need changes in the PCC Opregion
> Only using PCC OpRegion after requesting and releasing PCC channel shouldn't
> change PCC OpRegion code?

I don't understand what exactly that means. The spec states clearly that
PCC subspaces that are used for PCC Operation Regions must not be used
as PCC subspaces for other std ACPI features. I don't understand what
really is going on, on this platform as I don't see what you are saying
(which is wrong and I disagree with approach) in the code posted yet.

> > support code. One possible way is to acquire and release the channel for
> > each transaction which will be definitely overhead.
> Yes, transaction will be definitely overhead.
> The following method should be no such problem.
> -->
> If driver want to obtain other info by RegisterAddress and offset in PCC
> Register(), driver generally needs to do it as follows:
> 1> get channel ID and RegisterAddress and offset.
> 2> call pcc_mbox_request_channel to acquire the channel.
> 3> ioremap 'shmem_base_addr' to get 'pcc_comm_addr'
> 4> obtain other info based on RegisterAddress, offset and 'pcc_comm_addr'.

Above sound good but it is not PCC Opregion. Either you are not giving
full context or you are confusing what PCC Opregion means. There is a
section "Declaring PCC Operation Regions", please refer and see if that
is what you have on your platform.

> If driver selects PCC OpRegion method, driver may also need to release this
> PCC channel by calling pcc_mbox_free_channel.

As I mentioned, the driver must not do anything related to PCC Opregion.

> Because this channel will be requested when PCC OpRegion method is executed
> for the first time.
>

drivers/acpi/acpi_pcc.c must take care of that. If not patch that and get
it working. It must be generic and nothing to do with your platform.

> 
> Overall, the above process is a bit cumbersome if this driver only use PCC
> OpRegion.

Yes and hence must not touch anything around PCC Opregion.

> In addition, I have to dig one address from comm space in share memory,
> which will cause the available size of comm space to decrease, right?
> So it is better to use other way to do get channel ID and other info if it
> is possible.
> What do you think?

I am more confused about your platform than yesterday, so I don't have
much valuable suggestions ATM.

-- 
Regards,
Sudeep