Hi Sudeep,
Thanks for your reply.
在 2023/5/16 20:29, Sudeep Holla 写道:
On Tue, May 16, 2023 at 03:35:54PM +0800, lihuisong (C) wrote:
在 2023/5/15 21:08, Sudeep Holla 写道:
On Thu, May 04, 2023 at 09:16:16PM +0800, lihuisong (C) wrote:
I'm tring to use CRS with GAS to report PCC channel ID and get other
informations driver need by address.
OK you had pcc-chan-id pcc-type and device-flags in the DSD style bindings
to begin with. I haven't understood device-flags here so can't comment on
that.
We want to use the 'device-flags' to report some information by bit.
Please give more details, until then NACK for the idea.
ok.
Currently, this driver requests PCC channel and use type2 to communicate
with firmware.
OKAY...
But, if some platform support type3 and PCC Operation Region, driver can
choice this method to communicate with firmware.
So firmware and driver have to use this flag to make compatibility.
I would rather add such things to the spec if it is any sort of limitation
with the current specification.
Agreed. but I think there isn't any limitation for this with the current
specification.
There is no strong connection between PCC Operation Region and type3.
Driver can also use PCC to communicate with platform even if the type is
type3.
In other words, it depends on driver's choice.
Here, we want to use one bit in device-flags to indicates that
firmware and driver use PCC operation Region on feature platform instead
of only using PCC.
Sorry, I have to admit that the implementation of this driver about this
really needs
to be optimized to make it more clear.
I found a way to obtain the generic register information according to
"Referencing the PCC address space" in ACPI spec.
And driver also get the PCC generic register information successfully.
Can you elaborate ? I assume by that you must be able to get pcc-chan-id
Yes，driver can get pcc-chan-id by below register.
Register (PCC, RegisterBitWidth, RegisterBitOffset, RegisterAddress, AccessSize)
Good to know.
right ? You must not need pcc-type as the pcc mailbox driver must handle
the type for you. If not, we may need to fix or add any missing support.
Yes, PCC driver doesn't support it currently. And aother patch [1] we've
been talking about does it.
If it is applied to kernel, we can drop this pcc-type here.
[1] https://patchwork.kernel.org/project/linux-acpi/patch/20230423110335.2679-2-lihuisong@xxxxxxxxxx/
OK then we are good, no need for pcc-type then ?
If driver may support more PCC types, the type also need be known by driver.
Because not all types have the same header size.
The PCC type can be obtained from the PCCT by requesting PCC channel.
From this point, this pcc type here is unnecessary, right?
But I don't know how to set and use the address in PCC register.
It must be same as what you would have specified in you new bindings
under "pcc-chan-id". I am confused as you say you were able to get the
PCC generic register information successfully but you still claim you
don't know how to set or use the address.
My confusion about this address is mentioned below.
OK
Where should this address come from?
It seems that ACPI spec is not very detailed about this.
Do you have any suggestions?
I am afraid, I don't have any as I am failing to understand the exact issue
you are facing.
Let me try to ask the question explicity here:
If you are just referring to just the <RegisterAddress,> in
Register (PCC, RegisterBitWidth, RegisterBitOffset, RegisterAddress, AccessSize)
Yeah, this is what I'm using.
then,
RegisterAddress is usually the offset in the comms address associated with
Communication subspace in share memory of PCC subspace?
the PCC subspace ID specified in AccessSize. Yes the use of AccessSize for
the PCC subspace ID is bit confusing though.
You can either list all the registers with _CRS individually or the driver
List all the registers as following way?
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed,
NonCacheable, ReadWrite,
0x0000000000000000, // Granularity
0x0000000098190000, // Range Minimum
0x000000009819FFFF, // Range Maximum
0x0000000000000000, // Translation Offset
0x0000000000010000, // Length
,, , AddressRangeMemory, TypeStatic)
})
Not sure if you can use QWordMemory here TBH.
Above way is what you say?
can just use the PCC subspace ID in AccessSize and keep RegisterAddress = 0
but access individual offset based on its own knowledge. I haven't seen the
Following words come from ACPI spec.
-->
As an example, the following resource template refers to the feld occupying
bits 8 through 15 at address 0x30 in PCC
subspace 9:
ResourceTemplate()
{
Register (
PCC, //AddressSpaceKeyword
8, //RegisterBitWidth
8, //RegisterBitOffset
pcc 0x30, //RegisterAddress
9 //AccessSize (subspace ID)
)
}
If the width of the address is 32bit, set RegisterAddress to 0,
RegisterBitOffset to 0 and set RegisterBitWidth to 64 here.
Driver can access to the ((void __iomem *)pcc_comm_addr + 0x8 + 0) and
((void __iomem *)pcc_comm_addr + 0x8 + 4) address，right?
(This virtual address = pcc mapped address + header size + offset within PCC
subspace.)
Yes that's my understanding. I remember seeing the driver is just fetching
pcc-chan-id using DSD style key-value pair, which means you don't need
any other info other than the PCC subspace/channel ID, just have address
as 0.
But I still need the device-flags to report if use PCC operation Region.
If so I have to dig one address register from comm subspace, right?
Also I see the driver uses type for just rejecting the type 3 PCCT. The
question is will the driver probe and run on a platform with type 3 PCCT ?
Yes，some platforms may use PCC operation Region.
If so what is the problem running on such a platform. I see it is useless
I didn't found any problems. But this driver should consider the
possibility above mentioned.
check in the driver and can be dropped. Also the comment above enum
HCCS_DEV_FLAGS_INTR_B is confusing and so is the way flags is used.
Thanks for you bringing it up.
Indeed, this HCCS_DEV_FLAGS_INTR_B is not good.
I'm going to replace it with PCC operation Region flag.
full driver yet but I assuming that's how you would have used if you went with
your DSD pcc-chan-id proposal.
On the other hand, we think that System Memory space + method can also
achieve above goal. What do you think of that?
Again I don't understand what you mean by that.
Sorry, here is what I want to say.
-->
OperationRegion (CCS0, SystemMemory, 0x00000002081000CC, 0x04)
Field (CCS0, DWordAcc, NoLock, Preserve)
{
HAU1, 32
}
OperationRegion (CCS1, SystemMemory, 0x0000000201070410, 0x04)
Field (CCS1, DWordAcc, NoLock, Preserve)
{
HCGE, 32
}
Method (_DSM, 2, Serialized) // _DSM: Device-Specific Method
{
If ((Arg0 == ToUUID ("b06b81ab-0134-4a45-9b0c-483447b95fa7")))
{
If ((Arg1 == One))
{
Return (HAU1)
}
Return (HCGE)
}
}
Driver can call _DSM method to get some information, such as pcc_chan_id and
device_flags.
Big fat NACK for _DSM for the above purpose, please stop abusing _DSM or _DSD
for such information which can be obtained with the existing _CRS.
Get it. Thanks.
.
