Update the pci_cfg_save/restore routines to operate on bridge devices
(type 1 and type 2) as well as leaf devices (type 0). In particular,
this allows the existing PCI bus logic to save and restore capability
registers such as MSI and PCI-express for bridge devices rather than
duplicating that code in bridge drivers. It also means that bridge
drivers do not need to save and restore basic registers such as the
PCI command register or BARs nor manage powerstates for the bridge device.
Details
Details
- My x220 resumes fine with this. This does not test the cardbus changes, but does test the PCI-PCI bridge changes. (In particular I believe that this tests powering the bridges down and up.)
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I can't think of anything else to change.
I just tested this on my laptop w/ my pciehp changes, and w/o any other changes, I can now hot plug devices after suspend/resume, so looks good to me.
I have not tested, so obviously imp needs to test on his cardbus laptops.
Comment Actions
I think this is mostly right, but have a few questions about power states, and when the transition occurs and if it *ALWAYS* occurs or only mostly. Powering up the CBB was added in sequence to get around issues relating to powering it up later (where the config space would reset, even though it was read/writeable in D3 state).
I'm also a bit worried about the bar saving / restoring. Having some explaination about how that works would help me understand this change a bit better.
| sys/dev/pccbb/pccbb_pci.c | ||
|---|---|---|
| 261 ↗ | (On Diff #4700) | After careful review, and relying on type 2 devices to not add bars, I think this is the only bit of the change I need help understanding. Will the power state setting now be done in the bus level? |
| 332 ↗ | (On Diff #4700) | If this actually ever did power-up the device, the pci_Write_config that's done above here would have failed to survive the power state transition. |
| 914 ↗ | (On Diff #4700) | I'm not sure I see where we set this up as a PCI map, so it won't have a bar mapping. Have I missed something? |
| sys/dev/pci/pci.c | ||
| 5253 ↗ | (On Diff #4700) | This now affects carbus bridges, where before it didn't. Since it is just for the case where cbb isn't loaded, that may be Ok. |
| sys/dev/pci/pci_pci.c | ||
| 1124–1129 ↗ | (On Diff #4700) | Why was this removed? |
Comment Actions
Here's the scenario I think is broken by the power command removal:
The BIOS writes the bus information into the config registers.
We come up, and power the device down into D3 since cbb isn't loaded (or alternatively, the BIOS does this).
We load cbb. The pci code now "helpfully" powers up the device, but this causes all the bus info to disappear because D3 -> D0 transition clears all config registers.
We now have to rely on the NEW_PCIB to not screw up the bus numbers (and possibly leak them, haven't studied the code).
Before the change, we'd properly preserve the stuff. Over time, that preservation became imperfect, perhaps (NEW_PCIB definitely breaks things by writing to registers before we power up, for example, though I'm sure there's others).
Comment Actions
I would be quite happy to add pribus/secbus/subbus save/restore to pci_cfg_save/restore. That would better handle other bizarre-o cases where the drivers aren't involved (e.g. using devctl to power down an arbitrary bridge).
NEW_PCIB would not actually lose the values written since chipinit() would currently restore them from the softc copies that NEW_PCIB updates when it allocates bus numbers. That said, I don't think it's really correct to require no power state changes to a device by the bus but only via the device's driver. Power state changes are something the bus driver does to a device.
| sys/dev/pccbb/pccbb_pci.c | ||
|---|---|---|
| 261 ↗ | (On Diff #4700) | Yes. |
| 332 ↗ | (On Diff #4700) | OTOH, the d0 powerup routine didn't restore the SECBUS/SUBBUS registers either, so I don't know that this could have ever worked. |
| 914 ↗ | (On Diff #4700) | In pci.c cfg->nummaps is 1 for type 2: /* some PCI bus constants */ So the sole memory bar is added as a normal BAR by PCI already (and has been the entire time). |
| sys/dev/pci/pci_pci.c | ||
| 1124–1129 ↗ | (On Diff #4700) | It's been redundant since r214065 (and that commit probably should have removed this). |
| sys/dev/pccbb/pccbb_pci.c | ||
|---|---|---|
| 332 ↗ | (On Diff #4700) | Correction, I do see that chipinit() would have restored them, but it wasn't obvious from powerstate_d0()'s choice to only preserve a subset of config registers. |
Comment Actions
That works for me, and would be better, imho, than filtering everything. Type 2 devices are an odd-duck, and I really don't want the pci code knowing too much about them that isn't in the standard. I think you've convinced me that all the weird quirk-o-rama that's going on in pccbb happens in such a way as to be compatible with this.
NEW_PCIB would not actually lose the values written since chipinit() would currently restore them from the softc copies that NEW_PCIB updates when it allocates bus numbers. That said, I don't think it's really correct to require no power state changes to a device by the bus but only via the device's driver. Power state changes are something the bus driver does to a device.
I think the device should be notified of power changes, since the bus can't possibly know that the Toshiba TOBIC 95B needed to have this or that bizarre workaround when transitioning from D3 to D0 under some circumstances when certain conditions were true. While power state change is something the bus can command, an attached driver should be able to (a) reject the request and (b) be able to respond to the change. I haven't looked lately, is that now possible?
| sys/dev/pccbb/pccbb_pci.c | ||
|---|---|---|
| 332 ↗ | (On Diff #4700) | Yea, it worked. Maybe not well, and maybe a bit hap-hazard or disorganized, but it did work. Thanks for taking the time to tease out why and to organize related things better. |
| 914 ↗ | (On Diff #4700) | Thanks for the pointer. I'm not sure how far back in the past 'the entire time' goes, but I think it post-dates my reading and re-reading the pci code when actively supporting cbb. |
| sys/dev/pci/pci_pci.c | ||
| 1124–1129 ↗ | (On Diff #4700) | Cool. Any chance you can tease that bit out of the patch and commit separately? |
Comment Actions
- Fix two type 0-only regs and start on todo list for other standard type1/2.
- Save/restore bridge registers at the PCI bus layer.
- Read initial sub/sec regs in pci_setup_secbus().
- Tidy.
- Save/restore common config regs before header-specific.
- Compile.
- Compile.
Comment Actions
I've updated it to do this. I've tested this on my x220, but I haven't tested it with cardbus yet. I do have a t440 that has cardbus, but I can't find any of my cardbus cards to test the slot. :-/
NEW_PCIB would not actually lose the values written since chipinit() would currently restore them from the softc copies that NEW_PCIB updates when it allocates bus numbers. That said, I don't think it's really correct to require no power state changes to a device by the bus but only via the device's driver. Power state changes are something the bus driver does to a device.
I think the device should be notified of power changes, since the bus can't possibly know that the Toshiba TOBIC 95B needed to have this or that bizarre workaround when transitioning from D3 to D0 under some circumstances when certain conditions were true. While power state change is something the bus can command, an attached driver should be able to (a) reject the request and (b) be able to respond to the change. I haven't looked lately, is that now possible?
If a driver is attached, then power requests occur in the form of DEVICE_SUSPEND() and DEVICE_RESUME() and the driver can reject them (though it cannot veto a system-wide suspend). However, there is no way for the driver to hook into the case when the device is powered up by PCI before the probe and attach routines are run or to hook into the power down if no driver attaches to a device.
| sys/dev/pci/pci_pci.c | ||
|---|---|---|
| 1124–1129 ↗ | (On Diff #4700) | Yes, certainly. After my last round of updates I will probably split these changes up into a few commits (e.g. fixing the code to not read mingnt/maxlat from non-type 0 devices on attach or supporting those ivars on non-type 0 devices). I think it would still be useful to review them as a "whole", and once you are happy with them I will commit some of the smaller changes and then revise this to be what is left. |
Comment Actions
I did try this out on my T400 with a cardbus slot and nothing blew up during boot or suspend/resume. However, I can't find a pccard or cardbus card, so I can't say that I've really tested it. Would someone else be able to test this with a cardbus card?
Hmm, this is supposed an Atheros part:
Under Linux it is '07:00.0 Ethernet controller: Atheros Communications, Inc. AR5005G 802.11abg NIC (rev 01)'. Is this the one chipset that ath(4) doesn't support (AR5005VL)?
Comment Actions
Alright, I managed to find my (meager) treasure trove of cardbus/pccard cards including a rl(4) cardbus card, a "gold" wi(4) pccard, and a firewire cardbus card. The firewire card does not seem to work, but the other two cards do seem to work both before and after suspend/resume. I also tested suspend/resume with wi(4) in the slot on the T400 and it came back after resume ok. OTOH, wi(4) couldn't find APs to associate with (and wpa threw a hissy fit trying to use it), and I didn't actually plug rl(4) in (though the MAC address looked sane and the rlphy probed ok).
So, I think this doesn't break cardbus and is probably ok to go in?