2023-04-25 13:20:37

by Sudeep Holla

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

On Tue, Apr 25, 2023 at 09:00:31PM +0800, lihuisong (C) wrote:
>
> For firmware, DSD way is simpler and easier to manage these virtual platform
> devices, and it's an usual way in kernel.

Any specific examples you are referring here. We had lots of debate when
DSD was introduced. It must be used only when there is no standard ACPI
way to achieve the same. But in this I don't (yet) think that is the case.
Further "simplicity" is remotely not the reason why you must use DSD.
So until you provide me technical reasons as why _CRS can't work, I
have to NACK this approach. DSD in this case seems like pure hack.

> Driver only needs to get a fixed value, like pcc-id and type, here.
>

Yes and _CRS is used to get similar such properties in ACPI. It includes
normally MMIO and interrupts and since GAS supports PCC and _CRS can
contain GAS, you must simply use that.

> Any vantage if using _CRS with GAS compared with DSD?

Simple IMO, it is also existing standard to achieve things you are trying
to here and DSD is not. You are defining new properties to make DSD work.

So the real question is if _CRS can be used what is the point in defining
DSD for that. Unless I hear more technical and solid reasoning, I see
DSD as just hack and misuse here. It wasn't designed for that and must not
be allowed to make use of it for such use case.

Anyways in case we decide to take DSD route(after more deeper and technical
discussions), as in the kernel docs, please refer [1] for DSD. You need
to publish properties there so that no one comes up with similar but
alternate solution to do exactly this.

> > quite understand what magic the flags contain here to provide any info
> > there.
> This flag is used to report other properties, and every bit means a
> property.
> For instance, driver doesn't need to request PCC channel during the probing
> phase if driver use PCC operation Region.

Sorry I still don't understand it fully.

--
Regards,
Sudeep

[1] https://github.com/UEFI/DSD-Guide