On Tue, 24 Mar 2009 11:57:19 +1100 Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> wrote: > On Mon, 2009-03-23 at 15:23 -0700, Jesse Barnes wrote: > > The thing I didn't like was that it made the radeon driver use an > > internal interface; I'd really prefer a proper return value from > > pci_set_power_state, which in turn means auditing all its current > > callers. But that doesn't seem worth it unless we see other drivers > > needing something similar... > > > > And if we did go with something like your first patch, I'd still > > rather see the timeout done in the driver, rather than having the > > attempts & delay included in the function... > > So what ? The driver would call pci_set_power_state() until it stops > failing ? Yeah, that's what I had in mind. > I'm not too fan of that, because it will change the access pattern > to the chip: > > - write PM to 2 > - short delay > - read PM, see 0, return error > - driver does big delay > - write PM to 2 > - short delay > - read PM .... > > vs. the current sequence which is > > - write PM to 2 > - long delay > - read PM, be happy > > Which -seems- to be pretty much what happens in practice, though on > that chip, I don't know for sure about others. > > I'm worried of the possible side effects of the first sequence that > you propose since it would do 2 things potentially confusing to the > HW: > > - read PM after a short delay... it -should- be harmless but you know > HW as well as I do ... > > - write PM to 2 a second time after the long delay. Again, it > -should- be harmless since the chip at this stage should already be > in D2 state but god knows how the HW will react. > > I'm especially worried about the later in fact. Maybe we can minimize > it by having pci_set_power_state() dbl check the content of the PM > reg before writing to it... Honestly I'm not too happy about any of the approaches, but yeah I see your point. The main thing is to prevent any config space access for a specified time after the first D-state transition, which I think we do correctly in the core. Beyond that, we just have to make sure the core state is updated correctly; Rafael's first patch does that correctly I think. Actually now that I think of it, maybe Alex can get us details on the errata here. If we just need a longer wait between a D-state transition and the next config space access for this chip (or set of chips), we could add that as a proper quirk to the core... Alex, any thoughts about the bug & patch in http://bugzilla.kernel.org/show_bug.cgi?id=12846 ? Looks like old radeon chips need a workaround when transitioning from D0 -> D2... Thanks, -- Jesse Barnes, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
