On 2020-08-25 3:16 PM, Andy Shevchenko wrote:
On Mon, Aug 24, 2020 at 06:33:17PM +0200, Cezary Rojewski wrote:
On 2020-08-20 11:00 AM, Andy Shevchenko wrote:
...
Another question though: PCI_PM_CTRL. In order for me to make use of this, "pm_cap" member would have to be declared for my device. As this is no struct pci_dev, catpt has currently no separate member for that purpose. I don't believe you want me to add that field into struct's declaration. Second option is to define constant for pm_cap offset aka 0x80 within registers.h and then do the operations as follows: catpt_updatel_pci(cdev, CATPT_PM_CAP + PCI_PM_CTRL, ...)
However, in such case I won't be able to make use of current version of _updatel_pci() as definition of that macro allows me to skip prefix and type implicitly - PMCS (the rest is appended automatically). Maybe let's leave it within registers.h altogether so I can actually keep using said macro?
Basically what you do with accessing PCI configuration space via these methods (catpt_update_pci(), etc) is something repetitive / similar to what xHCI DbC support code does. I recommend to spend some time to look for similarities here (catpt) and there (PCI core, xHCI DbC, etc) and, if we were lucky, derive common helpers for traverse the capability list in more generalized way.
I wouldn't call direct-access a repetitive procedure, i.e. had procedure for enumerating PCI capabilities list been implemented individually by every PCI device type, then one can describe that as repetitiveness. Here, we are dealing with no procedure at all, just a writel & readl.
About xHCI, I believe you meant: xhci_find_next_ext_cap() https://elixir.bootlin.com/linux/latest/source/drivers/usb/host/xhci-ext-cap...
in case of PCI that's: pci_find_next_capability(), __pci_find_next_cap() and friends. pci_find_next_capability() is pci_dev dependent while most of the rest pci_bus instead. We fail both dependencies in catpt case.
xhci_find_next_ext_cap search method seems xHCI-specific, notice the 0x10 offset for HCCPARAMS1 and then the left-shift-by-2. PCI doesn't do that when enumerating capabilities, instead it checks Capabilities List-bit within Status reg and then begins iterating given the start pos - Capability Pointer, usually 0x34. Abstracting these (if even possible) would end up with 80% code gluing two different worlds with 20% left doing the actual job. Fact that those two are separated increases code readability.
While catpt is of platform_device type located on acpi bus, beneath there's a (incomplete?) description of PCI device.
PCI config catpt_acpi_probe00000000: 9cb68086 00100006 04010003 00000000 catpt_acpi_probe00000010: fe000000 fe100000 00000000 00000000 catpt_acpi_probe00000020: 00000000 00000000 00000000 00000000 catpt_acpi_probe00000030: 00000000 00000080 00000000 00000100
PCI base + 0x80 catpt_acpi_probe00000000: 40030001 0000000b 00000000 00000000 catpt_acpi_probe00000010: 00000000 00000000 00000000 00000000 catpt_acpi_probe00000020: fffffffd 00000000 80000fff 00000000 catpt_acpi_probe00000030: 00000000 00000000 00000000 00000000
Capabilities List-bit is set, start pos from Capabilitiy Pointer equals 0x80. What we have here is singular list of capabilities - PM as the only element. Following is the important DWORD (_PM_CTRL) - 0xb tells us that device is currently in D3hot. For LPT/WPT ADSP basically all other PM bits are hardwired to 0 or not supported.
So, quite frankly, had the BIOS offered correct ADSP device description, we wouldn't be dealing with ACPI device/ACPI bus at all. This has been corrected from SKL+ ADSP onward. To answer the immediate question: no, device of id 0x9cb6 won't be present within /sys/bus/pci/devices/ (cat './device' for id for every entry). Even converted catpt driver from acpi to pci just to make sure.
I don't mind adding new constant within register.h for transparency: #define CATPT_PCI_PMCAPID 0x80 #define CATPT_PCI_PMCS (CATPT_PCI_PMCAPID + PCI_PM_CTRL)
Current status for PM catpt_updatel_pci: catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D3hot) catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D0) which looks very good to me.