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. > 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. > > > > > 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 ? > > > > > 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. > > 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. 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 ? If so what is the problem running on such a platform. I see it is useless 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. > > 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. -- Regards, Sudeep
