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-caps.h#L97
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.
